Re: [PATCH v2 00/13] i2c: add and start using i2c_adapter-specific printk helpers

[Johan Hovold <johan@kernel.org>]

On Mon, Mar 02, 2026 at 12:03:19PM -0600, Bartosz Golaszewski wrote:
> On Fri, Feb 27, 2026 at 5:41 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > On Fri, Feb 27, 2026 at 04:42:09PM +0100, Bartosz Golaszewski wrote:
> > > On Fri, Feb 27, 2026 at 11:06 AM Johan Hovold <johan@kernel.org> wrote:
> >
> > > > It seems all that is needed is to decouple the struct i2c_adapter from
> > > > the driver data and have core manage the lifetime of the former using
> > > > the reference count of the embedded struct device.
> >
> > > This is a weird pattern you sometimes see where a driver allocates
> > > something and passes the ownership to the subsystem.
> >
> > It's not weird at all, this is the standard way to handle this. We have
> > these things called reference counts for a reason.
> >
> 
> I wouldn't say it's *the* standard way. There are at least several different
> ways driver subsystems handle resource ownership. And even so: the fact that
> something's done a lot does not make it automatically correct.

It's the way the driver model works.

> > > This often
> > > causes confusion among driver authors, who logically assume that if
> > > you allocate something, you are responsible for freeing it.Since this
> > > is C and not Rust (where such things are tracked by the compiler), I
> > > strongly believe we should strive to keep ownership consistent: the
> > > driver should free resources it allocated within the bounds of the
> > > lifetime of the device it controls. The subsystem should manage the
> > > data it allocated - in this case the i2c adapter struct device.
> >
> > Drivers are responsible for dropping *their* reference, it doesn't mean
> > that the resource is necessarily freed immediately as someone else may
> > be holding a reference. Anyone surprised by this should not be doing
> > kernel development.
> 
> I disagree. For some reason, you're defending a suboptimal programming
> interface. I'm all for reference counting but mixing reference-counted data
> with non-counted is simply not a good idea. An API should be easy to use and
> hard to misuse. Given the amount of issues, this approach is definitely easy
> to misuse.
> 
> I'm advocating for a hard split between the subsystem data (reference-counted)
> and driver data (living from probe() until remove()). A logical struct device
> managed entirely by the subsystem should live in a separate structure than
> driver data and be allocated - and freed - by the subsystem module.

It doesn't really matter what you think. You can't just go around
making up new subsystem specific rules at your whim. The linux driver
model uses reference counting and that's what developers expect to be
used.

> Let's put aside kernel code for a minute and work with an abstract C example,
> where the equivalent of what you're proposing would look like this:
> 
> struct bar {
> 	struct foo foo;
> 	...
> };
> 
> struct bar *bar = malloc(sizeof(*bar));
> 
> ret = foo_register(&bar->foo);
> 
> And the corresponding free() lives who knows where because foo_register()
> automagically introduces reference counting (nevermind the need to calculate
> where bar is in relations to foo).

No, that's not what I'm suggesting here, but it would be compatible with
the driver model (ever heard of struct device which works exactly like
this?).

> I strongly believe that this makes more sense:
> 
> struct bar {
> 	...
> };
> 
> struct bar *bar = malloc();
> 
> struct foo *foo = foo_register(bar);
> 
> // foo is reference counted and allocated in the provider of foo_register()
> 
> foo_put(foo);
> free(bar);
> 
> The equivalent of which is moving struct device out of struct i2c_adapter.

No, it's not.

> In fact: I would love to see i2c_adapter become a truly reference-counted
> object detached from driver data but due to it being embedded in every bus
> driver data structure it realistically won't happen.

And this is what I've been suggesting all along, separating the driver
data and making the adapter reference counted.

The idiomatic way to handle this is:

	xyz_probe()
	{
		adap = i2c_adapter_alloc();
		// initialise driver data, store pointer in adap
		i2c_adapter_register(adap);
	}

	xyz_remove()
	{
		i2c_adapter_deregister(adap);
		i2c_adapter_put(adap);
	}

Exactly where the driver data is stored is secondary, it could be memory
allocated by core or by the driver.

But the adapter is reference counted and kept around until all users are
gone.

> > > I know there are a lot of places where this is done in the kernel but
> > > let's not introduce new ones. This is a bad pattern.
> >
> > No, it's not. It's literally the standard way of doing this.
> >
> > > But even if you decided this is the way to go, I fail to see how it
> > > would be easier than what I'm trying to do. You would have to modify
> > > *all* I2C bus drivers as opposed to only modifying those that access
> > > the underlying struct device. Or am I missing something?
> >
> > Yes, you have to update the allocation and replace container_of() with
> > dev_get_drvdata() but it's a straight-forward transformation that brings
> > the i2c subsystem more in line with the driver model (unlike whatever it
> > is you're trying to do).
> >
> 
> No, it's not that simple. The .release() callback of struct device embedded
> in struct i2c_adapter is assigned from the bus type and only calls complete()
> (yeah, I too don't think it looks right, one would expect to see the associated
> kfree() here, right?). It relies on the bus driver freeing the data in its
> remove() path. That's why we wait until all references to said struct device
> are dropped. After your proposed change, if your new release() lives in the
> driver module, it must not be removed until all the references are dropped
> - basically where we are now. If on the other hand, the release() callback's
> functionality is moved into i2c-core, how would you handle the fact i2c_adapter
> can be embedded in a larger driver data structure? Provide yet another callback
> in i2c_adapter called from the device's .release()? Sure, can be done but I
> doubt it's a better solution.

You seem to be constructing some kind of straw man here. Obviously, the
release function would free the memory allocated for the adapter struct.

An adapter driver can free its driver data on unbind as core will
guarantee that there are no further callbacks after the adapter has been
deregistered.

Johan