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

Message-ID: <20210304141205.GC1463@shell.armlinux.org.uk>
Date:   Thu, 4 Mar 2021 14:12:05 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Saravana Kannan <saravanak@...gle.com>
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 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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!