Re: [PATCH v2 1/2] mfd: nct6694: Switch to devm_mfd_add_devices() and drop IDA

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Use-After-Free and WARN_ON due to Teardown Inversion on USB disconnect and probe failure
- [High] Sysfs Namespace Collision breaking support for multiple USB devices
--

commit 35c3701128ee68ba67a073bb4beea066282a9dba
Author: Ming Yu <a0282524688@gmail.com>

mfd: nct6694: Switch to devm_mfd_add_devices() and drop IDA
    
This commit switches the nct6694 core driver to use devm_mfd_add_devices()
for simplified device lifecycle management. It also drops the custom IDA
implementation and transitions to using pdev->id.

> diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c
> index 308b2fda3055c..8ce2c4985aaba 100644
> --- a/drivers/mfd/nct6694.c
> +++ b/drivers/mfd/nct6694.c

[ ... ]

> @@ -332,11 +326,11 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  
>  	usb_set_intfdata(iface, nct6694);
>  
> -	ret = mfd_add_hotplug_devices(dev, nct6694_devs, ARRAY_SIZE(nct6694_devs));
> +	ret = devm_mfd_add_devices(dev, 0, nct6694_devs, ARRAY_SIZE(nct6694_devs), NULL, 0, NULL);

The previous mfd_add_hotplug_devices() call automatically used
PLATFORM_DEVID_AUTO, which assigns globally unique IDs to prevent sysfs
name collisions.

By switching to devm_mfd_add_devices() with an ID argument of 0, does this
bind pdev->id to the static cell->id (0, 1, 2...)?

If a user connects a second identical NCT6694 USB device, it seems the MFD
core might attempt to register devices with the exact same names (e.g.,
nct6694-gpio.0) on the global platform bus. Will this cause the platform bus
to reject the second device with a -EEXIST error, breaking hotplug support
for multiple devices?

[ ... ]

> @@ -344,11 +338,7 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  
>  err_mfd:
>  	usb_kill_urb(nct6694->int_in_urb);
> -err_ida:
> -	ida_destroy(&nct6694->wdt_ida);
> -	ida_destroy(&nct6694->canfd_ida);
> -	ida_destroy(&nct6694->i2c_ida);
> -	ida_destroy(&nct6694->gpio_ida);
> +err_irq_domain:
>  	irq_domain_remove(nct6694->domain);
>  err_urb:
>  	usb_free_urb(nct6694->int_in_urb);
> @@ -359,12 +349,7 @@ static void nct6694_usb_disconnect(struct usb_interface *iface)
>  {
>  	struct nct6694 *nct6694 = usb_get_intfdata(iface);
>  
> -	mfd_remove_devices(nct6694->dev);
>  	usb_kill_urb(nct6694->int_in_urb);
> -	ida_destroy(&nct6694->wdt_ida);
> -	ida_destroy(&nct6694->canfd_ida);
> -	ida_destroy(&nct6694->i2c_ida);
> -	ida_destroy(&nct6694->gpio_ida);
>  	irq_domain_remove(nct6694->domain);

By relying on devres for MFD device removal, the child devices will now be
unregistered during the devres cleanup phase, which happens after
nct6694_usb_disconnect() returns.

However, irq_domain_remove() is still explicitly called here. Will this
destroy and free the domain memory while the child devices are still
registered and holding active IRQ mappings?

If the domain is removed early, it looks like it would trigger a WARN_ON in
irq_domain_remove() due to active mappings. Furthermore, when devres later
unregisters the child devices, their teardown paths (such as
nct6694_irq_dispose_mapping() and nct6694_canfd_remove()) call
irq_dispose_mapping() and access the already-freed IRQ domain. Can this
lead to a use-after-free?

A similar ordering issue might also happen in the probe error path if
devm_mfd_add_devices() partially fails, as the error path frees the IRQ
domain before devres cleans up any successfully added child devices.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260408053037.1867092-1-a0282524688@gmail.com?part=1