Re: [PATCH v2 06/13] i3c: master: Add support for devices without PID

sashiko-bot
2026-04-09T12:08:02Z

Sashiko AI review found 3 potential issue(s):
- [Medium] Dead code in `i3c_master_attach_boardinfo()` and broken duplicate detection for PID-less devices.
- [Medium] Validation bypass allows non-compliant SETDASA devices with `pid == 0` and malformed PIDs for SETAASA devices.
- [Medium] PID-less devices will break module autoloading due to missing OF/ACPI modalias in `uevent`.
--

commit 3602fe937b2780b8c359e0f18a06557c23b9b124
Author: Akhil R <akhilrajeev@nvidia.com>

i3c: master: Add support for devices without PID

This commit introduces support for I3C devices without a 48-bit PID, such as
those using SETAASA for address assignment. It uses the static address for
naming and matching when the PID is not present.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index a0f7a0d35f3f1..324a0440724e2 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1835,8 +1835,17 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
> desc->dev->dev.type = &i3c_device_type;
> desc->dev->dev.bus = &i3c_bus_type;
> desc->dev->dev.release = i3c_device_release;
> - dev_set_name(&desc->dev->dev, "%d-%llx", master->bus.id,
> - desc->info.pid);
> +
> + /*
> + * For devices without PID (e.g., SETAASA devices), use
> + * static address for naming instead.
> + */
> + if (desc->info.pid)
> + dev_set_name(&desc->dev->dev, "%d-%llx", master->bus.id,
> + desc->info.pid);
> + else
> + dev_set_name(&desc->dev->dev, "%d-%02x", master->bus.id,
> + desc->info.static_addr);

For devices with pid == 0, i3c_device_uevent() and modalias_show() will
generate an identical, generic string (e.g.,
MODALIAS=i3c:dcr00manuf0000part0000ext0000).

Will userspace tools like udev fail to automatically load the correct kernel
modules for these devices since the uevent callback does not emit the
OF_COMPATIBLE or ACPI modalias environment variables?

>
> if (desc->boardinfo)
> device_set_node(&desc->dev->dev, desc->boardinfo->fwnode);
[ ... ]
> @@ -2285,8 +2294,18 @@ static void i3c_master_attach_boardinfo(struct i3c_dev_desc *i3cdev)
> struct i3c_dev_boardinfo *i3cboardinfo;
>
> list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
> - if (i3cdev->info.pid != i3cboardinfo->pid)
> - continue;
> + /*
> + * For devices without PID (e.g., SETAASA devices), match by
> + * static address. For devices with PID, match by PID.
> + */
> + if (i3cboardinfo->pid) {
> + if (i3cdev->info.pid != i3cboardinfo->pid)
> + continue;
> + } else {
> + if (!i3cboardinfo->static_addr ||
> + i3cdev->info.static_addr != i3cboardinfo->static_addr)

Is it possible for this matching logic to fail during dynamic device discovery?

Since this function is called from i3c_master_add_i3c_dev_locked() during
DAA, the core initializes info.dyn_addr, but info.static_addr is likely still
0 because there is no CCC command to retrieve the static address during DAA.

This means i3cdev->info.static_addr != i3cboardinfo->static_addr will always
evaluate to true, causing the loop to continue and fail to attach the
boardinfo.

> + continue;
> + }
>
> i3cdev->boardinfo = i3cboardinfo;
> i3cdev->info.static_addr = i3cboardinfo->static_addr;
> @@ -2300,8 +2319,12 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
> struct i3c_master_controller *master = i3c_dev_get_master(refdev);
> struct i3c_dev_desc *i3cdev;
>
> + if (!refdev->info.pid)
> + return NULL;

By unconditionally returning NULL here for PID-less devices, does this bypass
duplicate detection?

Should the static address be used as a fallback identifier to prevent duplicate
device registration?

> +
> i3c_bus_for_each_i3cdev(&master->bus, i3cdev) {
> - if (i3cdev != refdev && i3cdev->info.pid == refdev->info.pid)
> + if (i3cdev != refdev && i3cdev->info.pid &&
> + i3cdev->info.pid == refdev->info.pid)
> return i3cdev;
> }
>
[ ... ]
> @@ -2601,9 +2624,15 @@ i3c_master_add_i3c_boardinfo(struct i3c_master_controller *master,
>
> boardinfo->pid = ((u64)reg[1] << 32) | reg[2];
>
> - if ((boardinfo->pid & GENMASK_ULL(63, 48)) ||
> - I3C_PID_RND_LOWER_32BITS(boardinfo->pid))
> - return -EINVAL;
> + /* For SETAASA devices, validate the static address instead of PID */
> + if (boardinfo->static_addr_method & I3C_ADDR_METHOD_SETAASA) {
> + if (!boardinfo->static_addr)
> + return -EINVAL;
> + } else {
> + if ((boardinfo->pid & GENMASK_ULL(63, 48)) ||
> + I3C_PID_RND_LOWER_32BITS(boardinfo->pid))
> + return -EINVAL;
> + }

Does this validation block inadvertently allow non-compliant SETDASA devices
without a PID?

If a device is processed here with static_addr_method == I3C_ADDR_METHOD_SETDASA
and pid == 0, the bitwise checks (0 & GENMASK) and I3C_PID_RND_LOWER_32BITS(0)
will both evaluate to 0, allowing the device to be registered without a valid
48-bit PID.

Additionally, for SETAASA devices, this entirely skips the else block where PID
validation occurs. If a Device Tree explicitly provides a malformed PID (e.g.,
non-zero reserved bits) for a SETAASA device, it seems it would be accepted
without validation and exposed in sysfs.

Should malformed PIDs still be rejected even if SETAASA is used?

>
> boardinfo->init_dyn_addr = init_dyn_addr;
> boardinfo->fwnode = fwnode_handle_get(fwnode);

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260409105747.48158-1-akhilrajeev@nvidia.com?part=6