[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 3 Mar 2022 14:00:37 -0600
From: Jeremy Linton <jeremy.linton@....com>
To: Jakub Kicinski <kuba@...nel.org>,
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
Hi,
On 2/23/22 16:48, Jakub Kicinski wrote:
> 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
>
I duplicated the problem on rpi4/ACPI by moving to gcc12, so I have a/b
config that is close as I can achieve using gcc11 vs 12 and the one
built with gcc12 fails pretty consistently while the gcc11 works.
So, I assumed that applying this patch would fix it, but it doesn't. I
may have messed something up, but I'm trying to figure out what is
actually different in the two modules between gcc11 and 12.
Powered by blists - more mailing lists