[PATCH v8 7/8] net-next: mvpp2: relax return value check for IRQ get

Matti Vaittinen posted 8 patches 8 months, 3 weeks ago
[PATCH v8 7/8] net-next: mvpp2: relax return value check for IRQ get
Posted by Matti Vaittinen 8 months, 3 weeks ago
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(-)

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;
 
-- 
2.40.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 
Re: [PATCH v8 7/8] net-next: mvpp2: relax return value check for IRQ get
Posted by Russell King (Oracle) 8 months, 3 weeks ago
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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH v8 7/8] net-next: mvpp2: relax return value check for IRQ get
Posted by Matti Vaittinen 8 months, 3 weeks ago
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.
> 

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~