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

[Meagan Lloyd <meaganlloyd@linux.microsoft.com>]

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:

drivers/platform/wmi/core.c:    .driver_override = true,
drivers/hv/vmbus_drv.c: .driver_override =      true,
drivers/base/platform.c:        .driver_override = true,
drivers/cdx/cdx.c:      .driver_override = true,
drivers/s390/cio/css.c: .driver_override = true,
drivers/vdpa/vdpa.c:    .driver_override = true,
drivers/bus/fsl-mc/fsl-mc-bus.c:        .driver_override = true,
drivers/pci/pci-driver.c:       .driver_override = true,
drivers/rpmsg/rpmsg_core.c:     .driver_override = true,
drivers/amba/bus.c:     .driver_override = true,

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.

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.

> > 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>
> 
> ...
> 
> > +	struct i3c_xfer xfer = { +		.rnw = I3C_WRITE
> 
> In such cases always leave a trailing comma. It will reduce possible
> churn in the future.
>

Good point, I'll fix that.
 
> > +	};
> 
> ...
> 
> > +	return !ret ? len : ret;
> 
> My gosh, wouldn't Elvis just work naturally?
> 
> 	return ret ?: len;
> 

You're right, that is much nicer!

> ...
> 
> > +	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().
 
> > +		ret = copy_struct_from_user(k_uxfer, +
> > sizeof(*k_uxfers), + uxfer, + metadata->xfer_size); + if (ret) +
> > goto out_free_k_uxfers; + +		/* Enforce that padding must be
> > zero */ +		if (memchr_inv(k_uxfer->pad, 0,
> > sizeof(k_uxfer->pad))) { +			ret = -EINVAL; + goto
> > out_free_k_uxfers; +		} + +		uxfer +=
> > metadata->xfer_size; /* u8 pointer so use xfer_size */ + k_uxfer++;
> > /* struct i3cdev_xfer pointer */ + }
> 
> ...
> 
> > +		if (!ret) +			total_bytes +=
> > i3c_xfers[i].len; +		else +			return ret;
> 
> Yeah, you really need to reconsider patterns you use in the code. Here
> 'else' is redundant. Homework to understand how (#easy).
>

Thanks for pointing this out. It would read better if I removed the else
block, opting to bail out early if ret is non-zero.
 
> ...
> 
> > +/** + * 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.

> > + */
> 
> ...
> 
> Please, rely less on AI and more on the common sense and
> proof-reading.
> 
> -- With Best Regards, Andy Shevchenko

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.

Thank you,
Meagan