[PATCH net-next] cirrus: cs89x0: fix the return value handle and remove redundant dev_warn() for platform_get_irq()

[PATCH net-next] cirrus: cs89x0: fix the return value handle and remove redundant dev_warn() for platform_get_irq()

[Ruan Jinjie]

There is no possible for platform_get_irq() to return 0
and the return value of platform_get_irq() is more sensible
to show the error reason.

And there is no need to call the dev_warn() function directly to print
a custom message when handling an error from platform_get_irq() function as
it is going to display an appropriate error message in case of a failure.

Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
---
 drivers/net/ethernet/cirrus/cs89x0.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cirrus/cs89x0.c b/drivers/net/ethernet/cirrus/cs89x0.c
index 7c51fd9fc9be..d323c5c23521 100644
--- a/drivers/net/ethernet/cirrus/cs89x0.c
+++ b/drivers/net/ethernet/cirrus/cs89x0.c
@@ -1854,9 +1854,8 @@ static int __init cs89x0_platform_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	dev->irq = platform_get_irq(pdev, 0);
-	if (dev->irq <= 0) {
-		dev_warn(&dev->dev, "interrupt resource missing\n");
-		err = -ENXIO;
+	if (dev->irq < 0) {
+		err = dev->irq;
 		goto free;
 	}
 
-- 
2.34.1

Re: [PATCH net-next] cirrus: cs89x0: fix the return value handle and remove redundant dev_warn() for platform_get_irq()

[Simon Horman]

+ Alex Elder

On Tue, Aug 01, 2023 at 09:31:21PM +0800, Ruan Jinjie wrote:
> There is no possible for platform_get_irq() to return 0
> and the return value of platform_get_irq() is more sensible
> to show the error reason.
> 
> And there is no need to call the dev_warn() function directly to print
> a custom message when handling an error from platform_get_irq() function as
> it is going to display an appropriate error message in case of a failure.
> 
> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>

Reviewed-by: Simon Horman <horms@kernel.org>

> ---
>  drivers/net/ethernet/cirrus/cs89x0.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cirrus/cs89x0.c b/drivers/net/ethernet/cirrus/cs89x0.c
> index 7c51fd9fc9be..d323c5c23521 100644
> --- a/drivers/net/ethernet/cirrus/cs89x0.c
> +++ b/drivers/net/ethernet/cirrus/cs89x0.c
> @@ -1854,9 +1854,8 @@ static int __init cs89x0_platform_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	dev->irq = platform_get_irq(pdev, 0);
> -	if (dev->irq <= 0) {
> -		dev_warn(&dev->dev, "interrupt resource missing\n");
> -		err = -ENXIO;
> +	if (dev->irq < 0) {
> +		err = dev->irq;
>  		goto free;
>  	}
>  
> -- 
> 2.34.1
> 
> 

Re: [PATCH net-next] cirrus: cs89x0: fix the return value handle and remove redundant dev_warn() for platform_get_irq()

[Alex Elder]

On 8/2/23 3:32 AM, Simon Horman wrote:
> + Alex Elder
> 
> On Tue, Aug 01, 2023 at 09:31:21PM +0800, Ruan Jinjie wrote:
>> There is no possible for platform_get_irq() to return 0
>> and the return value of platform_get_irq() is more sensible
>> to show the error reason.
>>
>> And there is no need to call the dev_warn() function directly to print
>> a custom message when handling an error from platform_get_irq() function as
>> it is going to display an appropriate error message in case of a failure.
>>
>> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>

First, I agree that the dev_warn() is unnecessary.

On the "<" versus "<=" issue is something I've commented on before.

It's true that 0 is not (or should not be) a valid IRQ number.  But
at one time a several years back I couldn't convince myself that it
100% could not happen.  I no longer remember the details, and it
might not have even been in this particular case (i.e., return
from platform_get_irq()).

I do see that a85a6c86c25be ("driver core: platform: Clarify that
IRQ 0 is invalid)" got added in 2020, and it added a WARN_ON()
in platform_get_irq_optional() before returning the IRQ number if
it's zero.  So in this case, if it *did* happen to return 0,
you'd at least get a warning.

So given that you'll get a warning on a bogus 0 IRQ number, I have
no problem with this part of the patch either.

					-Alex

Reviewed-by: Alex Elder <elder@linaro.org>

> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
>> ---
>>   drivers/net/ethernet/cirrus/cs89x0.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cirrus/cs89x0.c b/drivers/net/ethernet/cirrus/cs89x0.c
>> index 7c51fd9fc9be..d323c5c23521 100644
>> --- a/drivers/net/ethernet/cirrus/cs89x0.c
>> +++ b/drivers/net/ethernet/cirrus/cs89x0.c
>> @@ -1854,9 +1854,8 @@ static int __init cs89x0_platform_probe(struct platform_device *pdev)
>>   		return -ENOMEM;
>>   
>>   	dev->irq = platform_get_irq(pdev, 0);
>> -	if (dev->irq <= 0) {
>> -		dev_warn(&dev->dev, "interrupt resource missing\n");
>> -		err = -ENXIO;
>> +	if (dev->irq < 0) {
>> +		err = dev->irq;
>>   		goto free;
>>   	}
>>   
>> -- 
>> 2.34.1
>>
>>

Re: [PATCH net-next] cirrus: cs89x0: fix the return value handle and remove redundant dev_warn() for platform_get_irq()

[Arnd Bergmann]

On Wed, Aug 2, 2023, at 15:33, Alex Elder wrote:
> On 8/2/23 3:32 AM, Simon Horman wrote:
>> On Tue, Aug 01, 2023 at 09:31:21PM +0800, Ruan Jinjie wrote:
>>> There is no possible for platform_get_irq() to return 0
>>> and the return value of platform_get_irq() is more sensible
>>> to show the error reason.
>>>
>>> And there is no need to call the dev_warn() function directly to print
>>> a custom message when handling an error from platform_get_irq() function as
>>> it is going to display an appropriate error message in case of a failure.
>>>
>>> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
>
> First, I agree that the dev_warn() is unnecessary.
>
> On the "<" versus "<=" issue is something I've commented on before.
>
> It's true that 0 is not (or should not be) a valid IRQ number.  But
> at one time a several years back I couldn't convince myself that it
> 100% could not happen.  I no longer remember the details, and it
> might not have even been in this particular case (i.e., return
> from platform_get_irq()).
>
> I do see that a85a6c86c25be ("driver core: platform: Clarify that
> IRQ 0 is invalid)" got added in 2020, and it added a WARN_ON()
> in platform_get_irq_optional() before returning the IRQ number if
> it's zero.  So in this case, if it *did* happen to return 0,
> you'd at least get a warning.

Some of the older arm32 platforms used to start IRQ numbers at 0
instead of 1, but those should all have been converted by now,
and it's unlikely that the first interrupt would be the network
controller on any of those that did.

      Arnd

Re: [PATCH net-next] cirrus: cs89x0: fix the return value handle and remove redundant dev_warn() for platform_get_irq()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 1 Aug 2023 21:31:21 +0800 you wrote:
> There is no possible for platform_get_irq() to return 0
> and the return value of platform_get_irq() is more sensible
> to show the error reason.
> 
> And there is no need to call the dev_warn() function directly to print
> a custom message when handling an error from platform_get_irq() function as
> it is going to display an appropriate error message in case of a failure.
> 
> [...]

Here is the summary with links:
  - [net-next] cirrus: cs89x0: fix the return value handle and remove redundant dev_warn() for platform_get_irq()
    https://git.kernel.org/netdev/net-next/c/497c3a5fb3ed

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html