Re: [PATCH 0/6] leds: fix broken devres usage
From: Andy Shevchenko
Date: Tue Jun 02 2020 - 04:33:09 EST
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.
> > > 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,