Re: [PATCH 3/3] i3c: add i3cdev character device module for user-space access

[Armin Wolf <W_Armin@gmx.de>]

Am 17.09.26 um 07:49 schrieb Andy Shevchenko:

> On Wed, Sep 16, 2026 at 03:57:10PM -0700, Meagan Lloyd wrote:
>> On Sat, Sep 12, 2026 at 04:34:01PM +0300, Andy Shevchenko wrote:
>>> On Fri, Sep 11, 2026 at 02:09:35PM -0700, Meagan Lloyd wrote:
>>>> The i3cdev driver is a character device driver that allows user-space
>>>> to control and interact with I3C devices.
>>>> Currently, it has the ability to perform Single Data Rate (SDR)
>>>> transfers - basic reads/writes.
>>>>
>>>> With the addition of sysfs driver_override, there is now a
>>>> straightforward and direct way to match the i3cdev driver to any i3c
>>>> device without stepping on the toes of more specialized drivers that are
>>>> loaded automatically.
>>> Is it safe? Why on the earth do we need this? The commit message has not enough
>>> information.
>> I can't see a reason that it'd be unsafe. To give additional confidence,
>> it's already in-use in many bus_types:
> This argument has nothing to do with i³c. Each bus is different on a physical
> layer, electrical protocols and programming flow. Each of them has own
> constraints.
>
>> To answer why we need it:
>> If we want to write i3cdev as a standard device driver, it can't
>> actually match anything by default. This is because, some devices on the
>> system may need specific drivers and i3cdev is generic and should
>> technically match every device.
> Yes, but I have seen no reason why we should expose i³c bus to the user
> space. With i²c we already know very well that it was (and still is)
> a bad idea. Why i³c is better (especially taking into account i²c
> compatible mode and more complex programming flow)?

Hi,

in my experience sometimes during driver development you want to access the device
directly to test things, but of course this is incredibly unsafe.

I think we should taint the kernel as soon as userspace applications perform raw
i3c accesses, but the idea itself is fine from my point of view.

Thanks,
Armin Wolf

>> Since the driver_override is default NULL and is set via sysfs, this
>> allows any specific drivers on boot to be loaded up and would allow
>> explicit control on what device i3cdev gets bound to.
>>
>> This was my rational. I will refine the commit message with more details.
> Put a real life example why the exposing i³c devices into user space is
> absolutely necessary.
>
>>>> This is accomplished by the i3cdev driver not having any entries in
>>>> the i3c_device_id table. After boot, simply set the driver_override
>>>> to "i3cdev" and bind the device manually via the sysfs bind knob.
>>>> This can also be automated with udev rules as well.
>>>>
>>>> The character device interface will be exposed at: /dev/bus/i3c/<bus
>>>> id>-<Provisional ID>
> ...
>
>>>> +	for (int i = 0; i < metadata->nxfers; i++) {
>>> Why is 'i' signed?
>> Mostly for readability and to make sure the line length on loop headers
>> is kept below 80 chars. As a precaution, to make sure that 'i' can
>> represent any metadata->nxfers value without overflow during loops, I
>> check that metadata->nxfers is less than/equal to INT_MAX in
>> get_metadata().
> No need to add useless checks.
>
> ...
>
>>>> +/** + * print_i3c_err() - Prints the I3C error encountered during
>>>> the prior + * call to the core's transfer function.  + * @i3cdev:
>>>> i3cdev_data object + * @metadata: Kernel's copy of i3cdev_xfers
>>>> (ioctl I3CDEV_XFER input) + * @i3c_xfers: i3c_xfer array that was
>>>> sent to the I3C core
>>>> + * Returns: void
>>> Huh?! Where is this coming from?
>> In i3cdev_ioctl_do_xfers, if i3c_device_do_xfers failed, I wanted to
>> print out the first I3C controller error encountered. The controller
>> drivers can set this in the i3c_xfer.err field. Hence this function.
>>
>> It's to aid debugging and provide useful error information.
>> I can certainly refine the wording on the print_i3c_err documentation
>> header to make this more clear.
> My point is about kernel-doc. Why do we need the return section for void?
> Where it comes from?
>
>>>> + */
> ...
>
>>> Please, rely less on AI and more on the common sense and
>>> proof-reading.
>> I think I gave you the wrong impression. The new contributions in this
>> series were written and developed by me. I used AI for quality assurance
>> and cross-referencing. Since I incorporated some AI-flagged suggestions,
>> I tried to acknowledge that with the Assisted-by tag.
> I see, then there is a room to improve the code. But the main question is
> why do we even need this whole interface to begin with?
>