| Message ID | 2b49e965-850c-9f71-cd54-6ca9b7571cc3@omp.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9deb48b53e7f4056c2eaa2dc2ee3338df619e4f6 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bcmgenet: add WOL IRQ check | expand |
On Thu, Jan 13, 2022 at 10:46:07PM +0300, Sergey Shtylyov wrote: > The driver neglects to check the result of platform_get_irq_optional()'s > call and blithely passes the negative error codes to devm_request_irq() > (which takes *unsigned* IRQ #), causing it to fail with -EINVAL. > Stop calling devm_request_irq() with the invalid IRQ #s. > > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > --- > This patch is against DaveM's 'net.git' repo. Since this is for net, it needs a Fixes: tag. Fixes: 8562056f267d ("net: bcmgenet: request Wake-on-LAN interrupt") Andrew
On 1/13/2022 1:00 PM, Andrew Lunn wrote: > On Thu, Jan 13, 2022 at 10:46:07PM +0300, Sergey Shtylyov wrote: >> The driver neglects to check the result of platform_get_irq_optional()'s >> call and blithely passes the negative error codes to devm_request_irq() >> (which takes *unsigned* IRQ #), causing it to fail with -EINVAL. >> Stop calling devm_request_irq() with the invalid IRQ #s. >> >> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> >> >> --- >> This patch is against DaveM's 'net.git' repo. > > Since this is for net, it needs a Fixes: tag. > > Fixes: 8562056f267d ("net: bcmgenet: request Wake-on-LAN interrupt") I don't have strong objections whether we want to consider this a bug fix or not, but since the code only acts upon devm_request_irq() returning 0 meaning success, this seems more like an improvement rather than fixing an actual issue.
On 1/13/2022 1:37 PM, Florian Fainelli wrote: > > > On 1/13/2022 1:00 PM, Andrew Lunn wrote: >> On Thu, Jan 13, 2022 at 10:46:07PM +0300, Sergey Shtylyov wrote: >>> The driver neglects to check the result of platform_get_irq_optional()'s >>> call and blithely passes the negative error codes to devm_request_irq() >>> (which takes *unsigned* IRQ #), causing it to fail with -EINVAL. >>> Stop calling devm_request_irq() with the invalid IRQ #s. >>> >>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> >>> >>> --- >>> This patch is against DaveM's 'net.git' repo. >> >> Since this is for net, it needs a Fixes: tag. >> >> Fixes: 8562056f267d ("net: bcmgenet: request Wake-on-LAN interrupt") > > I don't have strong objections whether we want to consider this a bug > fix or not, but since the code only acts upon devm_request_irq() > returning 0 meaning success, this seems more like an improvement rather > than fixing an actual issue. If you do repost, please subject your patch with "net: bcmgenet" for consistency with previous commits done to that file, and feel free to add: Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Hello! On 1/14/22 12:37 AM, Florian Fainelli wrote: >>> The driver neglects to check the result of platform_get_irq_optional()'s >>> call and blithely passes the negative error codes to devm_request_irq() >>> (which takes *unsigned* IRQ #), causing it to fail with -EINVAL. >>> Stop calling devm_request_irq() with the invalid IRQ #s. >>> >>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> >>> >>> --- >>> This patch is against DaveM's 'net.git' repo. >> >> Since this is for net, it needs a Fixes: tag. >> >> Fixes: 8562056f267d ("net: bcmgenet: request Wake-on-LAN interrupt") > > I don't have strong objections whether we want to consider this a bug fix or not, Formally, it's a fix -- as you shouldn't call devm_request_irq() with "negative" IRQ #s. > but since the code only acts upon devm_request_irq() returning 0 meaning success, this seems more like an improvement rather than fixing an actual issue. More like a cleanup (no, not improvement). MBR, Sergey
Hello! On 1/14/22 12:00 AM, Andrew Lunn wrote: >> The driver neglects to check the result of platform_get_irq_optional()'s >> call and blithely passes the negative error codes to devm_request_irq() >> (which takes *unsigned* IRQ #), causing it to fail with -EINVAL. >> Stop calling devm_request_irq() with the invalid IRQ #s. >> >> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> >> >> --- >> This patch is against DaveM's 'net.git' repo. > > Since this is for net, it needs a Fixes: tag. > > Fixes: 8562056f267d ("net: bcmgenet: request Wake-on-LAN interrupt") Indeed, I completely forgot about it. Thanks! :-) > Andrew MBR, Sergey
Hello: This patch was applied to netdev/net.git (master) by David S. Miller <davem@davemloft.net>: On Thu, 13 Jan 2022 22:46:07 +0300 you wrote: > The driver neglects to check the result of platform_get_irq_optional()'s > call and blithely passes the negative error codes to devm_request_irq() > (which takes *unsigned* IRQ #), causing it to fail with -EINVAL. > Stop calling devm_request_irq() with the invalid IRQ #s. > > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > [...] Here is the summary with links: - bcmgenet: add WOL IRQ check https://git.kernel.org/netdev/net/c/9deb48b53e7f You are awesome, thank you!
Index: net/drivers/net/ethernet/broadcom/genet/bcmgenet.c =================================================================== --- net.orig/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ net/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -4020,10 +4020,12 @@ static int bcmgenet_probe(struct platfor /* Request the WOL interrupt and advertise suspend if available */ priv->wol_irq_disabled = true; - err = devm_request_irq(&pdev->dev, priv->wol_irq, bcmgenet_wol_isr, 0, - dev->name, priv); - if (!err) - device_set_wakeup_capable(&pdev->dev, 1); + if (priv->wol_irq > 0) { + err = devm_request_irq(&pdev->dev, priv->wol_irq, + bcmgenet_wol_isr, 0, dev->name, priv); + if (!err) + device_set_wakeup_capable(&pdev->dev, 1); + } /* Set the needed headroom to account for any possible * features enabling/disabling at runtime
The driver neglects to check the result of platform_get_irq_optional()'s call and blithely passes the negative error codes to devm_request_irq() (which takes *unsigned* IRQ #), causing it to fail with -EINVAL. Stop calling devm_request_irq() with the invalid IRQ #s. Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> --- This patch is against DaveM's 'net.git' repo. drivers/net/ethernet/broadcom/genet/bcmgenet.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)