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

On Mon, Mar 02, 2026 at 12:03:19PM -0600, Bartosz Golaszewski wrote:

On Fri, Feb 27, 2026 at 5:41 PM Johan Hovold [off-list ref] wrote:

quoted

On Fri, Feb 27, 2026 at 04:42:09PM +0100, Bartosz Golaszewski wrote:

quoted

On Fri, Feb 27, 2026 at 11:06 AM Johan Hovold [off-list ref] wrote:

quoted

quoted

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.

quoted

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.

quoted

quoted

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.

quoted

quoted

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.

quoted

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