Re: [PATCH V2 6/8] 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] Modifying device wakeup state dynamically during the suspend callback violates PM architecture, overrides sysfs UAPI, and triggers allocations and PM core warnings during system transitions.
- [Medium] The driver violates MFD architectural guidelines by dynamically allocating the `mfd_cell` array, calling MFD APIs outside `drivers/mfd/`, and the patch introduces a bespoke accessor that tightly couples parent and child drivers.

Pre-existing issues:
- [High] Unbound child devices cause a NULL pointer dereference and guaranteed kernel panic during the parent's suspend loop.
--

--- Patch [6]: [PATCH V2 6/8] i3c: mipi-i3c-hci-pci: Propagate I3C wakeup requirements to PCI ---
commit bd9a09f1c602f19882ed3fc13b554af06b2d2298
Author: Adrian Hunter <adrian.hunter@intel.com>

i3c: mipi-i3c-hci-pci: Propagate I3C wakeup requirements to PCI

This commit updates the PCI function's wakeup state based on the wakeup
requirements of the I3C devices attached to the HCI instances during suspend.
It checks if any attached device is enabled as a wakeup source and updates
the PCI device's wakeup capability accordingly.

> 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..2b3bf6fa74f29 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_any_wakeup_enabled(&hci->master);
> +}

[Severity: High]
This is a pre-existing issue, but does this code handle unbound child devices
safely?

If the child platform driver is unbound manually via sysfs, or if it fails
to probe, dev_get_drvdata() will return NULL. If the hardware bus was left
enabled, mipi_i3c_hci_pci_is_operational() can still return true. The parent's
suspend loop will then pass the device here, and to the existing
i3c_hci_rpm_suspend() function, where both will unconditionally dereference
the NULL pointer.

Could a check for a NULL hci pointer be added to prevent a kernel panic
during suspend?

[Severity: Medium]
Does this custom accessor bypass MFD architectural guidelines?

The MFD subsystem rules specify that parent drivers should not use custom
accessors to dig into the private driver data of child devices. Here, the
parent driver is directly extracting the child's internal state (&hci->master)
instead of using standard APIs.

Could this state propagation be handled through standard device PM wake
APIs rather than directly coupling the parent and child drivers?

[ ... ]

> @@ -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: High]
Is it safe to call device_set_wakeup_enable() during the suspend execution
path?

Calling this function during a system suspend triggers memory allocations
via kzalloc(..., GFP_KERNEL) and registers sysfs nodes while the PM transition
is actively in progress. This triggers the PM core warning "Suspicious
device_wakeup_enable() during system transition!" and introduces memory
deadlock risks.

Additionally, does this unconditionally override the user's explicit PM policy
configured via the power/wakeup sysfs attribute?

> +
> +	return 0;
>  }

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