Message-ID: <4568187f-ab63-4c86-b327-90720ad20ac9@kernel.org>
Date: Sun, 26 Oct 2025 20:20:17 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Igor Korotin <igor.korotin.linux@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
 Wolfram Sang <wsa+renesas@...g-engineering.com>,
 Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Viresh Kumar <viresh.kumar@...aro.org>,
 Asahi Lina <lina+kernel@...hilina.net>,
 Wedson Almeida Filho <wedsonaf@...il.com>, Alex Hung <alex.hung@....com>,
 Tamir Duberstein <tamird@...il.com>,
 Xiangfei Ding <dingxiangfei2009@...il.com>, linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org, linux-i2c@...r.kernel.org
Subject: Re: [PATCH v6 2/3] rust: i2c: Add basic I2C driver abstractions

On 10/26/25 7:41 PM, Igor Korotin wrote:
> Hello Danilo
> 
>> On 10/5/25 12:23 PM, Igor Korotin wrote:
>>> +impl Registration {
>>> +    /// The C `i2c_new_client_device` function wrapper for manual I2C client creation.
>>> +    pub fn new(i2c_adapter: &I2cAdapter, i2c_board_info: &I2cBoardInfo) -> Result<Self> {
>>> +        // SAFETY: the kernel guarantees that `i2c_new_client_device()` returns either a valid
>>> +        // pointer or NULL. `from_err_ptr` separates errors. Following `NonNull::new` checks for NULL.
>>> +        let raw_dev = from_err_ptr(unsafe {
>>> +            bindings::i2c_new_client_device(i2c_adapter.as_raw(), i2c_board_info.as_raw())
>>> +        })?;
>>> +
>>> +        let dev_ptr = NonNull::new(raw_dev).ok_or(ENODEV)?;
>>> +
>>> +        Ok(Self(dev_ptr))
>>> +    }
>>> +}
>>
>> I wonder if we want to ensure that a Registration can't out-live the driver that
>> registers the I2C client device.
>>
>> This should only ever be called by drivers bound to more complex devices, so if
>> the parent driver is unbound I don't think I2C client device registered by this
>> driver should be able to survive.
>>
>> Hence, I think Registration::new() should return
>> impl PinInit<Devres<Self>, Error> instead.
> 
> Maybe I'm missing something here, but as far as I understand, Devres is bound to 
> an existing device. However `Registration::new` creates new device and registers 
> new i2c_client using function `i2c_new_client_device`. Created i2c_client uses 
> i2c_adapter as its parent.

Correct, but the question is what's the correct lifetime boundary for this
i2c:Registration.
> The driver that declares Registration doesn't own that i2c_adapter. `Registration` 
> itself is not part of the new client’s managed resources, so returning 
> `impl PinInit<Devres<Self>, Error>` wouldn’t make sense here.

It does make sense, it's just not required for safety reasons.

This is an API that should be used by drivers operating complicated devices
(DRM, NET, etc.) where there is no point in keeping an i2c::Registration alive
after the driver that registered the I2C client has been unbound itself.

For instance, a GPU driver may call this in probe() to register an I2C device
for some redriver, repeater, multiplexer, etc. So, it makes no sense to allow a
corresponding i2c::Registration to still exist beyond the GPU driver being unbound.

Hence, besides not really being necessary for safety reasons, it still seems
reasonable to enforce this for semantic reasons.

> Drop for Registration calls `i2c_unregister_client()`, which gracefully unregisters
> and deallocates the i2c_client.

Not quite, it unregisters the I2C client (which is why we call the object
Registration), but it does not necessarily free the I2C client. Someone else can
still hold a reference count of the I2C client.

Powered by blists - more mailing lists