[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMRc=McXfSOt4b3AHRcOZoKhkQQsYDt=yWGgS_doO7HC69R2rg@mail.gmail.com>
Date: Fri, 30 Jan 2026 12:19:59 +0100
From: Bartosz Golaszewski <brgl@...nel.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Johan Hovold <johan@...nel.org>,
Bartosz Golaszewski <bartosz.golaszewski@....qualcomm.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Danilo Krummrich <dakr@...nel.org>,
"Rafael J . Wysocki" <rafael@...nel.org>, Tzung-Bi Shih <tzungbi@...nel.org>,
Linus Walleij <linusw@...nel.org>, Jonathan Corbet <corbet@....net>, Shuah Khan <shuah@...nel.org>,
Wolfram Sang <wsa+renesas@...g-engineering.com>, Simona Vetter <simona.vetter@...ll.ch>,
Dan Williams <dan.j.williams@...el.com>, Jason Gunthorpe <jgg@...dia.com>, linux-doc@...r.kernel.org,
linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/3] Revert "revocable: Revocable resource management"
On Thu, Jan 29, 2026 at 3:49 PM Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> On Thu, Jan 29, 2026 at 08:50:30AM -0500, Bartosz Golaszewski wrote:
> > On Thu, 29 Jan 2026 11:56:34 +0100, Laurent Pinchart said:
> > > On Thu, Jan 29, 2026 at 10:11:46AM +0100, Bartosz Golaszewski wrote:
> > >>
> > >> For I2C both the problem is different (subsystem waiting forever for
> > >> consumers to release all references) and the culprit: memory used to
> > >> hold the reference-counted struct device is released the supplier
> > >> unbind unconditionally. Unfortunately there's no way around it other
> > >> than to first move it into a separate chunk managed by i2c core.
> > >
> > > Isn't there ? Can't the driver-specific data structure be
> > > reference-counted instead of unconditionally freed at unbind time ?
> >
> > Oh, for sure, if we did from the start. But we did not and there are now
> > hundreds of i2c drivers that do:
> >
> > struct my_i2c_drv_data {
> > struct i2c_adapter adap;
> > int my_other_drv_data;
> > };
> >
> > and in probe:
> >
> > struct my_i2c_drv_data *data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> >
> > (or just kzalloc() with kfree() in remove, it doesn't matter)
> >
> > and the ownership of that data belongs to the driver. There's no way we could
> > address it now so the next best thing is to work towards moving the ownership
> > of struct i2c_adapter to the i2c core and make it reference counted using the
> > internal kobject of the associated struct device.
>
> What I'm reading here is essentially that we rolled out devm_kzalloc()
> too quickly without understanding the consequences, and it has spread so
> much that it can't be fixed properly now, so we need to find a
> workaround. And now we're trying to work around the problem by rolling
> out a revocable API that has barely seen any testing, and is known to
> have design issues. Does any one else see the irony ? :-)
>
No, this has nothing to do with devm_anything(). It's resource
ownership. What driver creates at probe(), the driver should destroy
at remove(). I really hate with a passion the pattern where a driver
creates something in probe() and then tosses it over to the subsystem
for management. If an entity registers the struct device, it should
also be the one who allocates and manages the memory for it. Devres
just made it a bit easier to commit this kind of errors but they would
exist nevertheless.
Bartosz
Powered by blists - more mailing lists