Message-ID: <CALeDE9OFbOoCNDF7CCRG=cXjsdOcgwqbkS=2XaQ-aK_W6tJCHg@mail.gmail.com>
Date:   Wed, 23 Feb 2022 17:41:03 +0000
From:   Peter Robinson <pbrobinson@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Doug Berger <opendmb@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        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, Feb 23, 2022 at 5:35 PM Florian Fainelli <f.fainelli@...il.com> wrote:
>
>
>
> On 2/23/2022 3:40 AM, Peter Robinson wrote:
> > On Tue, Feb 22, 2022 at 8:15 PM Florian Fainelli <f.fainelli@...il.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@...il.com>
> >>>>> Suggested-by: Javier Martinez Canillas <javierm@...hat.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():

This system is not booting ACPI, it's booting UEFI+DT, the system is
Fedora so overall the OS supports ACPI or device tree, but the error
is from a system booted with device tree.

>            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

Powered by blists - more mailing lists