Re: [PATCH v6 05/12] i3c: master: Add support for devices without PID

On Tue, 21 Jul 2026 04:26:04 +0000, Sashiko AI review <[email protected]> wrote:
> commit 671c3fae66002e874a13051a9c956e9987d76674
> Author: Akhil R <[email protected]>
>
> i3c: master: Add support for devices without PID
>
> Devices using SETAASA for address assignment are not required to have
> a 48-bit PID according to the I3C specification. Allow such devices to
> register and use the static address where PID was required.
>
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index b18dda89c4737..7b2f819bf4437 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
>
> [ ... ]
>
> > @@ -2389,8 +2398,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;
> > +		if (i3cboardinfo->pid) {
> > +			if (i3cdev->info.pid != i3cboardinfo->pid)
> > +				continue;
> > +		} else {
> > +			if (!i3cboardinfo->static_addr ||
> > +			    i3cdev->info.static_addr != i3cboardinfo->static_addr)
> > +				continue;
> > +		}
>
> [Severity: Medium]
> Will this static address matching logic ever evaluate to false?
>
> Since i3c_master_attach_boardinfo() is only called during the dynamic
> address assignment (ENTDAA) phase, i3cdev->info.static_addr is inherently 0
> in this context.
>
> Because i3cboardinfo->static_addr is validated to be non-zero at creation,
> the condition i3cdev->info.static_addr != i3cboardinfo->static_addr will
> evaluate to 0 != non-zero, which is always true.
>
> This means the loop will always continue, making this branch effectively
> dead code that fails to perform the intended matching.

This branch is harmless and does not affect functionality. It is kept for
symmetry/clarity and can be cleaned up if preferred.

> > 
> >  		i3cdev->boardinfo = i3cboardinfo;
> >  		i3cdev->info.static_addr = i3cboardinfo->static_addr;
>
> [ ... ]
>
> > @@ -2848,9 +2871,16 @@ 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 ||
> > +		    (boardinfo->pid & GENMASK_ULL(63, 48)) ||
> > +		    I3C_PID_RND_LOWER_32BITS(boardinfo->pid))
> > +			return -EINVAL;
> > +	}
>
> [ ... ]
>
> > @@ -2873,10 +2903,10 @@ static int i3c_master_add_of_dev(struct i3c_master_controller *master,
> > -	if (!reg[1])
> > +	if (!reg[1] && !fwnode_property_present(fwnode, "mipi-i3c-static-method"))
> >  		ret = i3c_master_add_i2c_boardinfo(master, fwnode, reg);
> >  	else
> >  		ret = i3c_master_add_i3c_boardinfo(master, fwnode, reg);
>
> [Severity: Medium]
> Could this change allow devices requiring a PID to be registered with an
> invalid Manufacturer ID of 0?
>
> By permitting reg[1] == 0 when mipi-i3c-static-method is present in
> i3c_master_add_of_dev(), it appears we can bypass the strict Manufacturer ID
> validation in i3c_master_add_i3c_boardinfo().
>
> If the method is SETDASA, which still requires a PID, boardinfo->pid is set
> to ((u64)0 << 32) | reg[2].  If reg[2] is non-zero, !boardinfo->pid is false.
>
> Would this result in successfully registering the device despite the zero
> Manufacturer ID, violating the MIPI I3C specification?

This is only if the reg property is intentionally malformed. If needed,
'!boardinfo->pid' can be replaced with !I3C_PID_MANUF_ID(pid) to fix this.

Best Regards,
Akhil

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c