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

[Bartosz Golaszewski]

On Tue, Mar 3, 2026 at 4:57 PM Johan Hovold <johan at kernel.org> wrote:
>
> 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 at 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 at 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.
>

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.

> > > > 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.
>

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

> > 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.

> > 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.
>

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.

What you showed above is totally fine. For an even simpler API, my personal
preference would be to:

xyz_probe()
{
	struct drv_data *data = kzalloc();
	/*
	 * ... stands for any other arguments or a config struct. In the concrete
	 * example of i2c, we'd supply the algo struct, fwnode, etc.
	 */
	struct i2c_adapter *adap = i2c_adapter_register(data, ...);
}

xyz_remove()
{
	kfree(data);
	i2c_adapter_unregister();
}

The reference counting of i2c_adapter happens behind the scenes in the
subsystem code. We're hiding the implementation details from the driver as it
has no business knowing it - it always only needs a single reference.

This way you have a kfree() corresponding with the kmalloc() and an
unregister() corresponding with the register().

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.

> > > > 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.
>

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.

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?

Bartosz