Re: [PATCH 0/6] leds: fix broken devres usage

[Andy Shevchenko]

On Mon, Jun 1, 2020 at 6:31 PM Johan Hovold <johan@xxxxxxxxxx> wrote:
> On Mon, Jun 01, 2020 at 06:09:23PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 1, 2020 at 5:29 PM Johan Hovold <johan@xxxxxxxxxx> wrote:
> > > On Mon, Jun 01, 2020 at 05:08:40PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Jun 1, 2020 at 5:01 PM Johan Hovold <johan@xxxxxxxxxx> wrote:
> > > > > On Mon, Jun 01, 2020 at 04:51:01PM +0300, Andy Shevchenko wrote:

...

> > > > > That's not the issue here. The child driver is allocating memory for the
> > > > > class device (for example using devres), and that will end up being
> > > > > freed on unbind while said device is still registered. The child driver
> > > > > may then even be unloaded. No extra reference can fix this.
> > > >
> > > > Okay, I didn't still get how dropping devres will help here.
>
> > > Any driver which frees a resource before making sure it's no longer used
> > > it is just plain broken. Unfortunately, devres makes this harder to
> > > reason about and people get it wrong. This is roughly the current
> > > situation with these drivers:
> > >
> > >         drv->probe(dev)
> > >           foo = devm_kzalloc(dev);
> >
> > >           devm_foo_register(dev->parent, foo);  // NOTE: dev->parent
> >
> > A-ha! Thanks for this detail.
> > But why are they doing so?
>
> As I mentioned in a commit message, we have quite a few MFD drivers that
> register class devices under the parent device directly and have been
> doing so for a very long time.
>
> As this is reflected in sysfs and we may have users relying on the
> current topology, changing this shouldn't be taken too lightly (drivers
> may also depend on it). And in any case, it wouldn't be stable material
> to fix the regressions at hand.

I see.

> > >         drv->remove(dev)
> > >         devres_release_all(dev)
> > >           kfree(foo);                           // foo still registered
> >
> > > but foo remains registered until the parent driver is unbound.
> >
> > Since the last fixes against kobject elimination, shouldn't be this
> > simple fixed by not supplying dev->parent above?
>
> No, that's a separate issue as it also changes the device tree.

Thanks for elaboration.

-- 
With Best Regards,
Andy Shevchenko