Re: [PATCH] rust: i2c: avoid locking when calling I2cAdapter::inc_ref

[Nicolás Antinori]

Hello Gary,

On Thu Jun 18, 2026 at 12:32 PM -03, Nicolás Antinori wrote:
> Hello Gary,
>
> On Wed Jun 17, 2026 at 11:12 AM -03, Gary Guo wrote:
>> On Mon Jun 15, 2026 at 9:10 PM BST, Nicolás Antinori wrote:
>>> diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
>>> index 624b971ca8b0..d89c42691dfe 100644
>>> --- a/rust/kernel/i2c.rs
>>> +++ b/rust/kernel/i2c.rs
>>> @@ -426,8 +426,11 @@ pub fn get(index: i32) -> Result<ARef<Self>> {
>>>  // SAFETY: Instances of `I2cAdapter` are always reference-counted.
>>>  unsafe impl AlwaysRefCounted for I2cAdapter {
>>>      fn inc_ref(&self) {
>>> -        // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
>>> -        unsafe { bindings::i2c_get_adapter(self.index()) };
>>> +        // SAFETY: The existence of a shared reference guarantees that the refcounts are non-zero.
>>> +        unsafe {
>>> +            bindings::__module_get((*self.as_raw()).owner);
>>> +            bindings::get_device(&raw mut (*self.as_raw()).dev);
>>
>> Instead of open coding this sequence, it would be better to add a C API that
>> does exactly this (getting another reference from existing one).
>
> I based this solution on the logic used in I2cClient (where only
> get_device is needed) and by verifying which counters the C
> implementation (i2c_get_adapter) increments.
>
> The i2c_get_adapter function in i2c-core-base.c performs two increments
> (module increment is done by calling try_module_get, but in this case,
> because inc_ref operates on an already live instance, unconditionally 
> incrementing the count with `__module_get` should be safe).
>
> If I understand correctly, the idea would be to introduce a helper 
> function on the C side, for example:
> void i2c_adapter_increment(struct i2c_adap *adap);
>
> We would perform the increments there and call it from Rust. Using this 
> in inc_ref would be safe because the existence of &self guarantees that 
> we already have a valid, live instance.
>
> Is there any concern regarding this function beign exposed to the C
> side? To use it safely in C, callers would have to ensure that *adap
> points to a valid instance.

I tried implementing this with the following C helper:

...
void i2c_adapter_inc_ref(struct i2c_adapter *adapter)
{
	__module_get(adapter->owner);
	get_device(&adapter->dev);
}
...

Calling this from Rust results in much cleaner code. The function does 
skips a NULL check for *adapter because the AlwaysRefCounted::inc_ref 
signature suggest a non-fallibe operation. Did I understand your 
suggestion correctly?

Also, does it make sense to declare a C API wrapper whose sole purpose
is to be called from the Rust side?

>
>>
>> That said, is there an actual user that needs this, or are we just implementing
>> AlwaysRefCounted preemptively?
>
> The only user I could find at the moment is the example driver
> (impl platform::Driver for SampleDriver ..) in
> samples/rust/rust_i2c_client.rs.

Does it make sense to move forward with this patch since there are no
real users yet? Does the Sashiko diagnostic makes sense? Maybe I
misunderstood it and it does not apply.

Thank you!
Nicolás