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

On Mon, Jul 27, 2026 at 02:39:28PM +0000, Akhil R wrote:
> 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.

Okay, we can fix later.

Frank
>
> > > +
> > > +	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

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