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

[Florian Fainelli <f.fainelli@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4635 bytes --]



On 3/4/2022 9:33 AM, Jeremy Linton wrote:
> Hi,
> 
> On 3/3/22 14:04, Javier Martinez Canillas wrote:
>> Hello Jeremy,
>>
>> On 3/3/22 21:00, Jeremy Linton wrote:
>>> 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.
>>>
>>
>> Did Peter's patch instead of this one help ?
>>
> 
> No, it seems to be the same problem. The second irq is registered, but 
> never seems to fire. There are a couple odd compiler warnings about 
> infinite recursion in memcpy()/etc I was looking at, but nothing really 
> pops out. Its like the adapter never gets the command submissions 
> (although link/up/down appear to be working/etc).

There are two "main" interrupt lines which are required and an optional 
third interrupt line which is the side band Wake-on-LAN interrupt from 
the second level interrupt controller that aggregates all wake-up sources.

The first interrupt line collects the the default RX/TX queue interrupts 
(ring 16) as well as the MAC link up/down and other interrupts that we 
are not using. The second interrupt line is only for the TX queues 
(rings 0 through 3) transmit done completion signaling. Because the 
driver is multi-queue aware and enabled, the network stack will chose 
any of those 5 queues before transmitting packets based upon a hash, so 
if you want to reliably prove/disprove that the second interrupt line is 
non-functional, you would need to force a given type of packet(s) to use 
that queue specifically. There is an example on how to do that here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/multiqueue.rst#n47

With that said, please try the following debug patch so we can get more 
understanding of how we managed to prevent the second interrupt line 
from getting its interrupt handler serviced. Thanks
-- 
Florian

[-- Attachment #2: debug.diff --]
[-- Type: text/plain, Size: 3982 bytes --]

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index cfe09117fe6c..7c7ab2cb0ebf 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -3991,6 +3991,10 @@ static int bcmgenet_probe(struct platform_device *pdev)
 	}
 	priv->wol_irq = platform_get_irq_optional(pdev, 2);
 
+	dev_info(&pdev->dev, "IRQ0: %d (%u), IRQ1: %d (%u), Wol IRQ: %d (%u)\n",
+		 priv->irq0, priv->irq1, priv->wol_irq,
+		 priv->irq0, priv->irq1, priv->wol_irq);
+
 	priv->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(priv->base)) {
 		err = PTR_ERR(priv->base);
@@ -4021,10 +4025,13 @@ 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) {
+		dev_info(&pdev->dev, "Wol IRQ > 0 requesting WoL ISR\n");
 		err = devm_request_irq(&pdev->dev, priv->wol_irq,
 				       bcmgenet_wol_isr, 0, dev->name, priv);
-		if (!err)
+		if (!err) {
+			dev_info(&pdev->dev, "Marking device as wake-up capable\n");
 			device_set_wakeup_capable(&pdev->dev, 1);
+		}
 	}
 
 	/* Set the needed headroom to account for any possible
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c b/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c
index e31a5a397f11..26efa4d175a7 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c
@@ -57,11 +57,15 @@ int bcmgenet_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	struct device *kdev = &priv->pdev->dev;
 
-	if (!device_can_wakeup(kdev))
+	if (!device_can_wakeup(kdev)) {
+		dev_err(kdev, "Device cannot wake-up\n");
 		return -ENOTSUPP;
+	}
 
-	if (wol->wolopts & ~(WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_FILTER))
+	if (wol->wolopts & ~(WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_FILTER)) {
+		dev_err(kdev, "Invalid wolopts\n");
 		return -EINVAL;
+	}
 
 	if (wol->wolopts & WAKE_MAGICSECURE)
 		memcpy(priv->sopass, wol->sopass, sizeof(priv->sopass));
@@ -69,6 +73,7 @@ int bcmgenet_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 	/* Flag the device and relevant IRQ as wakeup capable */
 	if (wol->wolopts) {
 		device_set_wakeup_enable(kdev, 1);
+		dev_info(kdev, "Enabling device for wake-up\n");
 		/* Avoid unbalanced enable_irq_wake calls */
 		if (priv->wol_irq_disabled)
 			enable_irq_wake(priv->wol_irq);
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index f23ffd30385b..d1a436f29a3a 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -841,11 +841,15 @@ static int set_irq_wake_real(unsigned int irq, unsigned int on)
 	struct irq_desc *desc = irq_to_desc(irq);
 	int ret = -ENXIO;
 
-	if (irq_desc_get_chip(desc)->flags &  IRQCHIP_SKIP_SET_WAKE)
+	if (irq_desc_get_chip(desc)->flags &  IRQCHIP_SKIP_SET_WAKE) {
+		pr_info("%s: IRQ chip is marked with IRQCHIP_SKIP_SET_WAKE\n", __func__);
 		return 0;
+	}
 
-	if (desc->irq_data.chip->irq_set_wake)
+	if (desc->irq_data.chip->irq_set_wake) {
 		ret = desc->irq_data.chip->irq_set_wake(&desc->irq_data, on);
+		pr_info("%s: irq_set_wake returned: %d for IRQ: %d\n", __func__, irq);
+	}
 
 	return ret;
 }
@@ -875,8 +879,12 @@ int irq_set_irq_wake(unsigned int irq, unsigned int on)
 	struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
 	int ret = 0;
 
-	if (!desc)
+	pr_info("%s: called with IRQ: %d on: %d\n", __func__, irq, on);
+
+	if (!desc) {
+		pr_err("%s: invalid descriptor\n", __func__);
 		return -EINVAL;
+	}
 
 	/* Don't use NMIs as wake up interrupts please */
 	if (desc->istate & IRQS_NMI) {
@@ -909,6 +917,7 @@ int irq_set_irq_wake(unsigned int irq, unsigned int on)
 
 out_unlock:
 	irq_put_desc_busunlock(desc, flags);
+	pr_info("%s: returning %d for IRQ: %d\n", __func__, ret, irq);
 	return ret;
 }
 EXPORT_SYMBOL(irq_set_irq_wake);