Re: [PATCH V5 12/14] i3c: mipi-i3c-hci-pci: Propagate I3C wakeup requirements to PCI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] AB-BA deadlock between I3C subsystem locks (bus.lock / ibi_lock) and PCI runtime suspend.
- [Medium] Dynamically modifying user-controlled sysfs wakeup attribute during suspend.

Pre-existing issues:
- [High] Missing child driver bound check and PM API bypass leads to NULL pointer dereferences.
--

--- Patch [12]: [PATCH V5 12/14] i3c: mipi-i3c-hci-pci: Propagate I3C wakeup requirements to PCI ---
commit 84ca2ce77fe5f7e3e2fb196059be4ac7c80842fc
Author: Adrian Hunter <adrian.hunter@intel.com>
Subject: i3c: mipi-i3c-hci-pci: Propagate I3C wakeup requirements to PCI

This patch aligns the PCI controller's wakeup state with the wakeup
requirements of its attached I3C devices. During suspend, it enables PCI
wakeup only if at least one I3C device has wakeup enabled.

> diff --git a/drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c b/drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c
> index 5a9e2a43eff8d..42c172e82622f 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c

[ ... ]

> @@ -272,6 +274,13 @@ static bool mipi_i3c_hci_pci_is_mfd(struct device *dev)
>  	return dev_is_platform(dev) && mfd_get_cell(to_platform_device(dev));
>  }
>  
> +static bool mipi_i3c_hci_pci_any_wakeup_enabled(struct device *dev)
> +{
> +	struct i3c_hci *hci = dev_get_drvdata(dev);
> +
> +	return i3c_master_has_wakeup_enabled_devs(&hci->master);
> +}
> +

[Severity: High]
This is a pre-existing issue, but does this code dereference the driver data
without checking if it is NULL?

If the child mipi-i3c-hci driver fails to probe or is unbound by a user via
sysfs, dev_get_drvdata() returns NULL.

The parent PCI driver iterates over child devices during suspend and checks
if the hardware is running:

mipi_i3c_hci_pci_suspend_instance()
    if (!mipi_i3c_hci_pci_is_mfd(dev) ||
        !mipi_i3c_hci_pci_is_operational(dev, true))
        return 0;

Because the hardware can still be operational even if unbound, the code
proceeds to call i3c_hci_rpm_suspend() and now
mipi_i3c_hci_pci_any_wakeup_enabled(), both of which blindly dereference
the unverified driver data.

>  static int mipi_i3c_hci_pci_suspend_instance(struct device *dev, void *data)
>  {

[ ... ]

> @@ -287,6 +296,9 @@ static int mipi_i3c_hci_pci_suspend_instance(struct device *dev, void *data)
>  
>  	pm_data->dev[pm_data->dev_cnt++] = dev;
>  
> +	if (pm_data->can_wakeup && mipi_i3c_hci_pci_any_wakeup_enabled(dev))
> +		pm_data->may_wakeup = true;
> +

[Severity: High]
Is there a potential AB-BA deadlock between the I3C subsystem locks and the
PCI runtime suspend lock here?

The new i3c_master_has_wakeup_enabled_devs() function is called during the
PCI device's suspend, acquiring bus.lock and ibi_lock:

mipi_i3c_hci_pci_suspend_instance()
  mipi_i3c_hci_pci_any_wakeup_enabled()
    i3c_master_has_wakeup_enabled_devs()
      i3c_bus_normaluse_lock(&master->bus);
      guard(mutex)(&desc->ibi_lock);

However, other I3C core paths take these locks first before triggering a PM
operation that requires the PM lock:

i3c_device_free_ibi()
  i3c_bus_normaluse_lock(dev->bus);
  mutex_lock(&dev->desc->ibi_lock);
  i3c_dev_free_ibi_locked(dev->desc);
    ...
    i3c_master_rpm_get()
      pm_runtime_resume_and_get()

If an IBI is freed concurrently while the PCI parent enters autosuspend, one
thread holds bus.lock waiting for the PM lock, while the suspend path holds
the PM lock waiting for bus.lock.

>  	return 0;
>  }
>  
> @@ -317,12 +329,19 @@ static int mipi_i3c_hci_pci_suspend(struct device *dev)
>  	if (!hci->info->control_instance_pm)
>  		return 0;
>  
> +	pm_data.can_wakeup = device_can_wakeup(dev);
> +
>  	ret = device_for_each_child_reverse(dev, &pm_data, mipi_i3c_hci_pci_suspend_instance);
> -	if (ret)
> +	if (ret) {
>  		for (int i = 0; i < pm_data.dev_cnt; i++)
>  			i3c_hci_rpm_resume(pm_data.dev[i]);
> +		return ret;
> +	}
>  
> -	return ret;
> +	if (device_may_wakeup(dev) != pm_data.may_wakeup)
> +		device_set_wakeup_enable(dev, pm_data.may_wakeup);
> +

[Severity: Medium]
Does this directly overwrite the user-controlled sysfs wakeup attribute
during suspend?

The official driver API guidelines mandate that device drivers should not
call device_set_wakeup_enable() directly, as this field is reserved for
sysfs user policy.

Should the driver align its hardware PME registers directly without modifying
the generic PM subsystem's sysfs toggle?

> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807145638.168865-1-adrian.hunter@intel.com?part=12

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c