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

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

On 2/23/2022 3:40 AM, Peter Robinson wrote:
> On Tue, Feb 22, 2022 at 8:15 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>
>>
>> On 2/22/2022 12:07 PM, Peter Robinson wrote:
>>>> On 2/22/2022 1:53 AM, Peter Robinson wrote:
>>>>> The ethtool WoL enable function wasn't checking if the device
>>>>> has the optional WoL IRQ and hence on platforms such as the
>>>>> Raspberry Pi 4 which had working ethernet prior to the last
>>>>> fix regressed with the last fix, so also check if we have a
>>>>> WoL IRQ there and return ENOTSUPP if not.
>>>>>
>>>>> Fixes: 9deb48b53e7f ("bcmgenet: add WOL IRQ check")
>>>>> Fixes: 8562056f267d ("net: bcmgenet: request Wake-on-LAN interrupt")
>>>>> Signed-off-by: Peter Robinson <pbrobinson@gmail.com>
>>>>> Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
>>>>> ---
>>>>>     drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c | 4 ++++
>>>>>     1 file changed, 4 insertions(+)
>>>>>
>>>>> We're seeing this crash on the Raspberry Pi 4 series of devices on
>>>>> Fedora on 5.17-rc with the top Fixes patch and wired ethernet doesn't work.
>>>>
>>>> Are you positive these two things are related to one another? The
>>>> transmit queue timeout means that the TX DMA interrupt is not firing up
>>>> what is the relationship with the absence/presence of the Wake-on-LAN
>>>> interrupt line?
>>>
>>> The first test I did was revert 9deb48b53e7f and the problem went
>>> away, then poked at a few bits and the patch also fixes it without
>>> having to revert the other fix. I don't know the HW well enough to
>>> know more.
>>>
>>> It seems there's other fixes/improvements that could be done around
>>> WOL in the driver, the bcm2711 SoC at least in the upstream DT doesn't
>>> support/implement a WOL IRQ, yet the RPi4 reports it supports WOL.
>>
>> There is no question we can report information more accurately and your
>> patch fixes that.
>>
>>>
>>> This fix at least makes it work again in 5.17, I think improvements
>>> can be looked at later by something that actually knows their way
>>> around the driver and IP.
>>
>> I happen to be that something, or rather consider myself a someone. But
>> the DTS is perfectly well written and the Wake-on-LAN interrupt is
>> optional, the driver assumes as per the binding documents that the
>> Wake-on-LAN is the 3rd interrupt, when available.
>>
>> What I was hoping to get at is the output of /proc/interrupts for the
>> good and the bad case so we can find out if by accident we end-up not
>> using the appropriate interrupt number for the TX path. Not that I can
>> see how that would happen, but since we have had some interesting issues
>> being reported before when mixing upstream and downstream DTBs, I just
>> don't fancy debugging that again:
> 
> The top two are pre/post plugging an ethernet cable with the patched
> kernel, the last two are the broken kernel. There doesn't seem to be a
> massive difference in interrupts but you likely know more of what
> you're looking for.

There is not a difference in the hardware interrupt numbers being 
claimed by GENET which are both GIC interrupts 189 and 190 (157 + 32 and 
158 + 32). In the broken case we can see that the second interrupt line 
(interrupt 190), which is the one that services the non-default TX 
queues does not fire up at all whereas it does in the patched case.

The transmit queue timeout makes sense given that transmit queue 2 
(which is not the default one, default is 0) has its interrupt serviced 
by the second interrupt line (190). We can see it not firing up, hence 
the timeout.

What I *think* might be happening here is the following:

- priv->wol_irq = platform_get_irq_optional(pdev, 2) returns a negative 
error code we do not install the interrupt handler for the WoL interrupt 
since it is not valid

- bcmgenet_set_wol() is called, we do not check priv->wol_irq, so we 
call enable_irq_wake(priv->wol_irq) and somehow irq_set_irq_wake() is 
able to resolve that irq number to a valid interrupt descriptor

- eventually we just mess up the interrupt descriptor for interrupt 49 
and it stops working

Now since this appears to be an ACPI-enabled system, we may be hitting 
this part of the code in platform_get_irq_optional():

           r = platform_get_resource(dev, IORESOURCE_IRQ, num);
           if (has_acpi_companion(&dev->dev)) {
                   if (r && r->flags & IORESOURCE_DISABLED) {
                           ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), 
num, r);
                           if (ret)
                                   goto out;
                   }
           }

and then I am not clear what interrupt this translates into here, or 
whether it is possible to get a valid interrupt descriptor here.

The patch is fine in itself, but I would really prefer that we get to 
the bottom of this rather than have a superficial understanding of the 
nature of the problem.

Thanks for providing these dumps.
-- 
Florian