linux-kernel - Re: [PATCH v2] amba: Remove deferred device addition

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] [day] [month] [year] [list]

Message-ID: <CAGETcx_WHkZOtF+iv4gx3KnNDYiAg1Ys8MQbW89E4H2bRAUCkw@mail.gmail.com>
Date:   Thu, 4 Mar 2021 07:48:06 -0800
From:   Saravana Kannan <saravanak@...gle.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     Philipp Zabel <p.zabel@...gutronix.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh@...nel.org>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        John Stultz <john.stultz@...aro.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Sudeep Holla <sudeep.holla@....com>,
        Nicolas Saenz Julienne <nsaenzjulienne@...e.de>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Android Kernel Team <kernel-team@...roid.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] amba: Remove deferred device addition

On Thu, Mar 4, 2021 at 6:12 AM Russell King - ARM Linux admin
<linux@...linux.org.uk> wrote:
>
> On Wed, Mar 03, 2021 at 08:08:44PM -0800, Saravana Kannan wrote:
> > Marek,
> >
> > I tested it and saw the device get added before the resources were
> > available and the uevent file looked okay. Would you mind testing it
> > further?
>
> To put it bluntly, if you have tested this, the testing was not very
> effective. Deleting the lines that are removed by the patch so we can
> see what the new code looks like below:
>
> > > +int amba_device_add(struct amba_device *dev, struct resource *parent)
> > >  {
> > > +       int ret;
> > >
> > >         WARN_ON(dev->irq[0] == (unsigned int)-1);
> > >         WARN_ON(dev->irq[1] == (unsigned int)-1);
> > >
> > >         ret = request_resource(parent, &dev->res);
> > >         if (ret)
> > > +               return ret;
> > >
> > > +       /* If primecell ID isn't hard-coded, figure it out */
> > > +       if (dev->periphid) {
> > > +               ret = amba_read_periphid(dev);
>
> So, if the peripheral ID has _already_ been set, we attempt to read the
> peripheral ID from the device. Isn't that just wrong?
>
> > > +               if (ret && ret != -EPROBE_DEFER)
> > > +                       goto err_release;
> > >                 /*
> > > +                * AMBA device uevents require reading its pid and cid
> > > +                * registers.  To do this, the device must be on, clocked and
> > > +                * out of reset.  However in some cases those resources might
> > > +                * not yet be available.  If that's the case, we suppress the
> > > +                * generation of uevents until we can read the pid and cid
> > > +                * registers.  See also amba_match().
> > >                  */
> > > +               if (ret)
> > > +                       dev_set_uevent_suppress(&dev->dev, true);
> > >         }
>
> If the peripheral ID has not been set, we don't attempt to read it, and
> we generate an add event when the amba device is added with a zero
> peripheral ID.
>
> I guess that if() statement should be negated - and with such an error,
> I fail to see how this code could have been properly tested.

Yeah, the if() needs to be flipped. I even flipped it and then
unflipped it before I sent the patch. Thanks for catching it.

It worked in my testing because the device didn't have hard coded PID.
So it worked out fine.

But I now realize I still have a chicken-and-egg problem if ALL amba
drivers are modules. amba_match() will never be called because none of
the amba drivers have been loaded. None of the amba drivers would be
loaded (depending on the set up) because none of the uevents were sent
out. But there's a simple fix for this. I'll send that as part of v3.

Marek,

It'd still be nice if you can test this with the if() above flipped.
If all your amba drivers are modules and loaded based on uevents,
manually loading one of them will kick off everything.

-Saravana