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]

Date:   Thu, 24 Feb 2022 09:34:58 +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 2/23/2022 9:45 AM, Peter Robinson wrote:
> >>> 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.
> >
> > 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.

Yes, I can do that, may be quicker if you send me a rough patch of the
debugs you like as that'll be likely less round trips, happy to deal
offline.

P