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

On Wed, Mar 04, 2026 at 01:55:14AM -0800, Bartosz Golaszewski wrote:

On Tue, Mar 3, 2026 at 4:57 PM Johan Hovold [off-list ref] wrote:

quoted

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

quoted

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.

No, it does not impose any specific pattern to use for subsystems other than
requiring each device that's been *initialized* to provide a .release() callback
called when the last reference is dropped.

Reference counting is a core part of the driver model and this is
reflected in the way subsystems manage lifetime.

quoted

quoted

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.

And I've never said that it should not use reference counting. I'm not sure
what you're implying here.

You have posted changes that will prevent driver from accessing the
struct device of core i2c structures. This is unexpected, non-idiomatic
and subsystem specific and therefore a bad idea.

quoted

quoted

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 know how struct device works. I'm pointing out that this is a bad API (just
to be absolutely clear: not the reference counting of struct device itself but
using it in a way tha looks like it's not refcounted but becomes so after an
API call) because it's confusing. I'm not buying the argument that if it
confuses you then you should not be doing kernel development because it's not
the goal of API design to make it as complex and confusing as possible - quite
the contrary. And it *is* confusing given the amount of misuse present. I've
heard Greg KH say on multiple occasions during his talks that we try to offload
complex code to subsystems so that drivers can remain fairly simple. I agree
with that.

Again, this is a core feature of the driver model. You can't just ignore
it and come up with random ways to work around just because you disagree
with design decisions that were made 25 years ago.

quoted

quoted

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.

quoted

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.

Yeah, that's awesome but that's not what's being done in i2c. We do:

struct foo_i2c_driver_data {
	struct i2c_adapter adap {
		struct device dev;
		...
	};
	...
};

instead which is a completely different story. It makes foo_i2c_driver_data
implicitly reference counted despite its lifetime being tied to the bound-state
of the device. It becomes painfully obvious in rust when the compiler starts
enforcing proper lifetime management.

I'm quite aware of that and that's why we are discussing how to change
it.

But sure, your example works fine too. My point is: getting to that state would
require more churn than allowing drivers to continue allocating i2c_adapter
struct themselves with struct device being moved out of it - making reference
counting of it work properly.

And I agree: doing the above would be even better but you'd need - for every
driver - to move the i2c_adapter struct out of driver data and make it a
pointer. That's in addition to providing new APIs and using them. I2C drivers
are spread treewide. There's a reason why nobody attempted it for decades. I'm
proposing something a bit less complex: allow drivers to free i2c_adapter at
unbind but make i2c core keep a private, reference-counted structure for as
long as it's needed.

But its non-idiomatic and therefore not a good idea. Sometimes you just
have to dig in and fix the real problem instead of trying to work around
it.

quoted

quoted

quoted

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.

Sure, but my point all along has been that - with struct device currently
embedded in struct i2c_adapter - that's not the case. Driver data *and*
i2c_adapter are tied together. You need a major rework in either case.

And I've being saying that the driver data should be *decoupled* from
the i2c_adapter.

I'm frustrated because I'm spending time working on an actual solution. I've
explained what I'm doing and what the end result will look like based on what
works for GPIO (struct gpio_chip's lifetime is bound to device's "bound" state,
struct gpio_device is refcounted, I want to mirror it with i2c_adapter and
whatever we eventually call its refcounted counterpart - let's say:
i2c_bus_device). If you claim you have a better alternative - will you do the
work to make it happen?

Sure. I'll fix this properly.

Johan