From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 113DBEDA697 for ; Tue, 3 Mar 2026 15:57:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=fknkIDPnX9yvUUxDQk3Uq4zMFR0+YqB9sxfzCCJ1ryE=; b=rCrE9/fafrGpKkUsy052IzNvZd KqTCbQ0HLuZITqhfauaz1NyxVNYMxI3ClsQwYiwmTYtksdlY7X+Vgb5tEwOFFBOiaK+70RSm0jEXE MpPYLCLkRf5oKjl471L+vWiCgXCMuV1bWJEttI9qvetGIhfc1kp2kAn9sDG5KC2xbSCFrTUDN+ILV POmmBZz27O+KBJvnufBzfy1WeHkvn2MardAtwoh3kZG1E2h8vylY2AAS6b/uiluXrQ/JRH6U+1tmn I8GkrWuew85fkOrwEGstlpLEa+k9BtupFHMjD21FX2LDNg9q+/DyqbRMUd2kZ+evdH1O05zRLP8XY Ahze+BRg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1vxS88-0000000FUYJ-17MM; Tue, 03 Mar 2026 15:57:52 +0000 Received: from tor.source.kernel.org ([2600:3c04:e001:324:0:1991:8:25]) by bombadil.infradead.org with esmtps (Exim 4.98.2 #2 (Red Hat Linux)) id 1vxS87-0000000FUY8-0mlc; Tue, 03 Mar 2026 15:57:51 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by tor.source.kernel.org (Postfix) with ESMTP id 81C4E60053; Tue, 3 Mar 2026 15:57:50 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 0863FC116C6; Tue, 3 Mar 2026 15:57:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1772553470; bh=7wnmUVJYz31ztCiGKAzjwdnFCaEeyvoYoDHodVWWhOo=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=t7Gb1CcmG3mQHFd+prxFW6iEuOe+P6JnjIxvh6iepVeRugk7VrzgXF6XTaJhoe/Qs S6yrYamuMMDh+xPCNlQ7kZi1h1LWQ3kzPZK2Ax6Rz5lDORwIJ9AZI/VIHEIqRk3eLa P/m1lno3hmNdo24vovY1c/w1olKbkkHhoJdZ+Z+1P1CmPBfTa0tpQ+j69wb5XkoYBi I2jmibQclRi+seToB5vI/R18HBWaeMj4d0Piao2pTO/gj7dFbO/Pl2oR9sgdaBcW6x sZ74UOiOemSucZb0IEztzKBPw7XFvhGBi8yjGc8aIcoqLXJZCxpr5sItUOANMLmR4j w1/j45YkWhccw== Received: from johan by xi.lan with local (Exim 4.98.2) (envelope-from ) id 1vxS83-000000004sw-0okG; Tue, 03 Mar 2026 16:57:47 +0100 Date: Tue, 3 Mar 2026 16:57:47 +0100 From: Johan Hovold To: Bartosz Golaszewski Cc: Wolfram Sang , Bartosz Golaszewski , Andi Shyti , Chen-Yu Tsai , Jernej Skrabec , Samuel Holland , Khalil Blaiech , Asmaa Mnebhi , Jean Delvare , Madhavan Srinivasan , Michael Ellerman , Nicholas Piggin , "Christophe Leroy (CS GROUP)" , Andreas =?utf-8?Q?F=C3=A4rber?= , Manivannan Sadhasivam , Mauro Carvalho Chehab , linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linuxppc-dev@lists.ozlabs.org, linux-actions@lists.infradead.org, linux-media@vger.kernel.org Subject: Re: [PATCH v2 00/13] i2c: add and start using i2c_adapter-specific printk helpers Message-ID: References: <20260223-i2c-printk-helpers-v2-0-13b2a97762af@oss.qualcomm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.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 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 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