| Message ID | 20230801133121.416319-1-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 497c3a5fb3ed4a59c4d3c460c70393dd609815dc |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] cirrus: cs89x0: fix the return value handle and remove redundant dev_warn() for platform_get_irq() | expand |
+ 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 > >
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 >> >>
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!
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
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; }
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(-)