netdev - Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220223144818.2f9ce725@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 23 Feb 2022 14:48:18 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Peter Robinson <pbrobinson@...il.com>,
        Doug Berger <opendmb@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        bcm-kernel-feedback-list@...adcom.com, netdev@...r.kernel.org,
        Javier Martinez Canillas <javierm@...hat.com>
Subject: Re: [PATCH] net: bcmgenet: Return not supported if we don't have a
 WoL IRQ

On Wed, 23 Feb 2022 09:54:26 -0800 Florian Fainelli wrote:
> > I have no problems working with you to improve the driver, the problem
> > I have is this is currently a regression in 5.17 so I would like to
> > see something land, whether it's reverting the other patch, landing
> > thing one or another straight forward fix and then maybe revisit as
> > whole in 5.18.  
> 
> Understood and I won't require you or me to complete this investigating 
> before fixing the regression, this is just so we understand where it 
> stemmed from and possibly fix the IRQ layer if need be. Given what I 
> just wrote, do you think you can sprinkle debug prints throughout the 
> kernel to figure out whether enable_irq_wake() somehow messes up the 
> interrupt descriptor of interrupt and test that theory? We can do that 
> offline if you want.

Let me mark v2 as Deferred for now, then. I'm not really sure if that's
what's intended but we have 3 weeks or so until 5.17 is cut so we can
afford a few days of investigating.

I'm likely missing the point but sounds like the IRQ subsystem treats
IRQ numbers as unsigned so if we pass a negative value "fun" is sort 
of expected. Isn't the problem that device somehow comes with wakeup
capable being set already? Isn't it better to make sure device is not
wake capable if there's no WoL irq instead of adding second check?

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index cfe09117fe6c..7dea44803beb 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -4020,12 +4020,12 @@ static int bcmgenet_probe(struct platform_device *pdev)
 
 	/* Request the WOL interrupt and advertise suspend if available */
 	priv->wol_irq_disabled = true;
-	if (priv->wol_irq > 0) {
+	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);
-	}
+	else
+		err = -ENOENT;
+	device_set_wakeup_capable(&pdev->dev, !err);
 
 	/* Set the needed headroom to account for any possible
 	 * features enabling/disabling at runtime