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

[sashiko-bot@kernel.org]

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