Re: [PATCH v4 3/3] i3c: add i3cdev module to expose i3c dev in /dev

[Sam Agazaryan]

On Wed Sep 9, 2026 at 9:51 PM UTC, Frank Li wrote:
> On Sun, Sep 06, 2026 at 08:27:47PM +0000, Sam Agazaryan wrote:
>> From: Vitor Soares <vitor.soares@xxxxxxxxxxx>
>>
>> This patch adds userspace character device support for I3C SDR private
>> transfers via /dev.
>
> Add userspace character device ...

Updated in v5.


>>
>>   - Dynamically exposes /dev/bus/i3c/<device> character devices for I3C
>>     devices when unbound from kernel drivers.
>>   - Dynamically allocates character device minor numbers using the IDA
>>     allocator.
>>   - Implements private SDR read/write transfers via I3C_IOC_PRIV_XFER ioctl
>>     with 64-bit aligned UAPI data structures.
>
> Consider PRIV for SDR, need unified HDR also,
>
> I3C_IOC_XFER
>

At this very moment the production use case we have (OCP Recovery) supports SDR mode. That being said
supporting HDR mode also makes sense.

>> + * Copyright (c) 2020 Synopsys, Inc. and/or its affiliates.
>
> new post, should be 2026

Updated in v5.

>> +
>> +	i3cdev = kzalloc(sizeof(*i3cdev), GFP_KERNEL);
>
> kzalloc_obj()
>

Updated in v5.

>> +	dev_dbg(&i3c->dev, "Reading %zu bytes.\n", count);
>> +
>> +	ret = i3c_device_do_xfers(i3c, &xfers, 1, I3C_SDR);
>
> why force to SDR here.
>

For reads and writes we don't really have an option of passing down a struct denoting cmd mode.
Is there a different default behavior you had in mind for read/write besides SDR mode?

>> +err_out:
>> +	mutex_unlock(&i3cdev->xfer_lock);
>
> use cleanup guard()
>

Updated in v5.

>> +	for (i = 0; i < nxfers; i++) {
>> +		if (xfers[i].rnw) {
>> +			if (copy_to_user(u64_to_user_ptr(xfers[i].data),
>> +					 data_ptrs[i], xfers[i].len))
>
> Use actual_len,
>

Updated in v5.

>> +				ret = -EFAULT;
>> +		}
>> +	}
>> +
>> +err_free_mem:
>> +	for (j = 0; j < i; j++)
>> +		kfree(data_ptrs[j]);
>> +	kfree(k_xfers);
>
>
> use clean up __free(kfree) when alloc.
>

Updated in v5.

>> +	case BUS_NOTIFY_ADD_DEVICE:
>> +	case BUS_NOTIFY_UNBOUND_DRIVER:
>> +		return i3cdev_attach(dev, NULL);
>
> Does all i3c devices attach this driver?
>

Just i3c target devices without a bound driver, the controller should not attach.
The controller itself is explicitly skipped:
        if (dev->type == &i3c_masterdev_type || dev->driver)
            return 0;

>> +struct i3c_ioc_priv_xfer {
>> +	__u64 data;
>> +	__u16 len;
>> +	__u8 rnw;
>
> In HDR mode,  rnw is command.
>

Addressing this in the final comment in this message alongside the rest of
the HDR suggestions.

>> +	__u8 pad[5];
>> +};
>> +
>> +#define I3C_PRIV_XFER_SIZE(N)	\
>> +	((((sizeof(struct i3c_ioc_priv_xfer)) * (N)) < (1 << _IOC_SIZEBITS)) \
>> +	? ((sizeof(struct i3c_ioc_priv_xfer)) * (N)) : 0)
>> +
>> +#define I3C_IOC_PRIV_XFER(N)	\
>> +	_IOC(_IOC_READ|_IOC_WRITE, I3C_DEV_IOC_MAGIC, 30, I3C_PRIV_XFER_SIZE(N))
>
> I3C_IOC_SDR_XFER, support DDR, TSP, TSL future.
>
> Frank


On HDR modes - I think it makes sense to ensure the UAPI supports HDR modes now that
they're integrated into the i3c core. Would you prefer a scheme where we pass
in a more unified struct to specify either: SDR mode r/w or an HDR mode r/w
with ioctl?

Drawing inspiration from i3c_xfer:

    struct i3c_ioc_xfer {
        __u64 data;
        __u16 len;
        __u16 actual_len;
        union {
            __u8 rnw;
            __u8 cmd;
        };
        __u8 mode;
        __u8 pad[2];
    };



Thanks,
Sam