| Message ID | 9738e169d83a96f18de417e62b3cf4c20f51f885.1690890774.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | fix fwnode_irq_get[_byname()] returnvalue | expand |
On Tue, Aug 01, 2023 at 03:04:24PM +0300, Matti Vaittinen wrote: > fwnode_irq_get[_byname]() were changed to not return 0 anymore. > > Drop check for return value 0. > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> Sorry, but I don't think you've properly considered the effects of your patch. > @@ -5833,7 +5833,7 @@ static int mvpp2_multi_queue_vectors_init(struct mvpp2_port *port, > v->irq = of_irq_get_byname(port_node, irqname); > else > v->irq = fwnode_irq_get(port->fwnode, i); > - if (v->irq <= 0) { > + if (v->irq < 0) { You're making this change based on the assumption that fwnode_irq_get() has changed its return values, but I really don't think you've looked at the code and considered the return value behaviour of the DT function above. Reading it's documentation, it states that of_irq_get_byname() may return 0 on IRQ mapping failure. So, by making this change, you are allowing IRQ mapping failure in the DT path to succeed rather than fail. > ret = -EINVAL; > goto err; > } > @@ -6764,7 +6764,7 @@ static int mvpp2_port_probe(struct platform_device *pdev, > err = -EPROBE_DEFER; > goto err_deinit_qvecs; > } > - if (port->port_irq <= 0) > + if (port->port_irq < 0) Exactly the same problem here, but... > /* the link irq is optional */ > port->port_irq = 0; this is less critical... but still wrong. So, given that this patch is basically incorrect... NAK.
On 8/1/23 15:31, Russell King (Oracle) wrote: > On Tue, Aug 01, 2023 at 03:04:24PM +0300, Matti Vaittinen wrote: >> fwnode_irq_get[_byname]() were changed to not return 0 anymore. >> >> Drop check for return value 0. >> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > > Sorry, but I don't think you've properly considered the effects of your > patch. Don't be sorry. > >> @@ -5833,7 +5833,7 @@ static int mvpp2_multi_queue_vectors_init(struct mvpp2_port *port, >> v->irq = of_irq_get_byname(port_node, irqname); >> else >> v->irq = fwnode_irq_get(port->fwnode, i); >> - if (v->irq <= 0) { >> + if (v->irq < 0) { > > You're making this change based on the assumption that fwnode_irq_get() > has changed its return values, but I really don't think you've looked > at the code and considered the return value behaviour of the DT function > above. Reading it's documentation, it states that of_irq_get_byname() > may return 0 on IRQ mapping failure. You're correct. Thanks for spotting this! Seems like I really overlooked the behaviour of the of_irq_get_byname(). > So, by making this change, you are allowing IRQ mapping failure in the > DT path to succeed rather than fail. > >> ret = -EINVAL; >> goto err; >> } >> @@ -6764,7 +6764,7 @@ static int mvpp2_port_probe(struct platform_device *pdev, >> err = -EPROBE_DEFER; >> goto err_deinit_qvecs; >> } >> - if (port->port_irq <= 0) >> + if (port->port_irq < 0) > > Exactly the same problem here, but... > >> /* the link irq is optional */ >> port->port_irq = 0; > > this is less critical... but still wrong. > > So, given that this patch is basically incorrect... > > NAK. >
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c index 1fec84b4c068..2632ffe91ca5 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c @@ -5833,7 +5833,7 @@ static int mvpp2_multi_queue_vectors_init(struct mvpp2_port *port, v->irq = of_irq_get_byname(port_node, irqname); else v->irq = fwnode_irq_get(port->fwnode, i); - if (v->irq <= 0) { + if (v->irq < 0) { ret = -EINVAL; goto err; } @@ -6764,7 +6764,7 @@ static int mvpp2_port_probe(struct platform_device *pdev, err = -EPROBE_DEFER; goto err_deinit_qvecs; } - if (port->port_irq <= 0) + if (port->port_irq < 0) /* the link irq is optional */ port->port_irq = 0;
fwnode_irq_get[_byname]() were changed to not return 0 anymore. Drop check for return value 0. Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> --- Revision history: v5 =>: - No changes v4 = v5: Fix the subject, mb1232 => mvpp2 The patch changing the fwnode_irq_get() got merged during 5.4: https://lore.kernel.org/all/fb7241d3-d1d1-1c37-919b-488d6d007484@gmail.com/ This is a clean-up as agreed. --- drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)