Re: [PATCH v2] amba: Remove deferred device addition

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

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!