Re: [PATCH v6 06/12] i3c: master: match I3C device through DT and ACPI

On Tue, 21 Jul 2026 04:25:58 +0000, Sashiko AI review <[email protected]> wrote:
> commit 7db2eb24719ceb614bd3ec6e5efb369d3b92fe6e
> Author: Akhil R <[email protected]>
>
> i3c: master: match I3C device through DT and ACPI
>
> SETAASA-based devices cannot always be identified by PID or DCR; the
> standard I3C id_table matching may not be applicable. Allow such devices to
> match through Device Tree or ACPI.
>
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index 7b2f819bf4437..43bf530bc6619 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
>
> [ ... ]
>
> > @@ -345,15 +346,32 @@ static int i3c_device_match(struct device *dev, const struct device_driver *drv)
> >  {
> >  	struct i3c_device *i3cdev;
> >  	const struct i3c_driver *i3cdrv;
> > +	u8 static_addr_method = 0;
> >  
> >  	if (dev->type != &i3c_device_type)
> >  		return 0;
> >  
> >  	i3cdev = dev_to_i3cdev(dev);
> >  	i3cdrv = drv_to_i3cdrv(drv);
> > -	if (i3c_device_match_id(i3cdev, i3cdrv->id_table))
> > +
> > +	if (i3cdev->desc && i3cdev->desc->boardinfo)
> > +		static_addr_method = i3cdev->desc->boardinfo->static_addr_method;
>
> [Severity: High]
> Can i3cdev->desc be modified concurrently here?
>
> Looking at i3c_master_unregister_i3c_devs(), it locklessly sets
> i3cdev->dev->desc = NULL during device unregistration.
>
> Since the check in i3c_device_match() is done without locks or READ_ONCE(),
> could the compiler emit multiple loads for i3cdev->desc, leading to a NULL
> pointer dereference when subsequently accessing boardinfo?

i3c_device_match() already dereferences i3cdev->desc without a lock (e.g.
via the existing i3c_device_match_id() path); this patch adds the DT/ACPI
branch in the same context. Serializing desc against concurrent DAA /
hot-join spans the whole function and its pre-existing callers, so this
will have to be fixed separately.

> > +
> > +	if (i3cdrv->id_table && i3c_device_match_id(i3cdev, i3cdrv->id_table))
> >  		return 1;
> >  
> > +	if (static_addr_method & I3C_ADDR_METHOD_SETAASA) {
> > +		if (of_driver_match_device(dev, drv))
> > +			return 1;
> > +		if (acpi_driver_match_device(dev, drv))
> > +			return 1;
> > +	}
>
> [Severity: Medium]
> Will this fallback matching break the assumptions in existing driver probe
> functions?
>
> Currently, drivers typically call i3c_device_match_id() in their probe
> routine and unconditionally dereference the returned pointer.
>
> When an I3C device binds via DT or ACPI through this new path,
> i3c_device_match_id() will return NULL. If developers add an of_match_table
> to existing drivers to support SETAASA devices, won't this cause an immediate
> NULL pointer dereference during probe?

The i3c_driver probe callback does not receive an i3c_device_id, so a
driver that looks one up must handle its absence for firmware-matched
devices. The spd5118 driver added in this series matches via
of_match_table and does not rely on a class-table ID.

> [Severity: High]
> How does this change affect module autoloading for newly supported devices?
>
> While the patch allows devices to match via DT or ACPI, it appears the bus
> uevent callback, i3c_device_uevent(), was not updated and only emits the
> generic I3C modalias.
>
> Since of_device_uevent_modalias() or acpi_device_uevent_modalias() are not
> called, won't the uevent lack the required of:N... string, preventing udev
> from matching the device against the driver's MODULE_DEVICE_TABLE(of, ...)?

This is a deliberate trade-off. Emitting the OF/ACPI modalias from the I3C
uevent suppressed the native I3C modalias for standard devices (the
regression raised on v5), so this patch keeps the native I3C modalias.
Firmware-described devices are still matched via their firmware node
(of_/acpi_driver_match_device), and probe/bind was verified on the target
ACPI platform (SPD5118). If OF-modalias-based autoload also needs to be
restored for such devices, it can be added without clobbering the native
I3C modalias as a follow-up.

> [Severity: High]
> This is a pre-existing issue, but can the removal of an I3C device leak
> uninitialized kernel stack memory to user-space?
>
> When an I3C device is removed, i3c_master_unregister_i3c_devs() sets
> i3cdev->dev->desc = NULL before calling device_unregister(), which
> triggers a KOBJ_REMOVE uevent.
>
> In i3c_device_uevent():
>
> 	struct i3c_device_info devinfo;
>
> 	if (i3cdev->desc)
> 		devinfo = i3cdev->desc->info;
>
> Since i3cdev->desc is unconditionally NULL during removal, devinfo is left
> uninitialized. The function then proceeds to construct the MODALIAS string
> using devinfo:
>
> 	return add_uevent_var(env,
> 			      "MODALIAS=i3c:dcr%02Xmanuf%04Xpart%04Xext%04X",
> 			      devinfo.dcr, manuf, part, ext);
>
> Doesn't this deterministically embed uninitialized kernel stack data into the
> uevent environment variables, exposing it to user-space programs like udev?

Pre-existing (the desc == NULL case in i3c_device_uevent()); not introduced
here. Better fixed separately.

> > +
> >  	return 0;
> >  }

Best Regards,
Akhil