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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The driver dynamically overwrites the user-controlled `dev->power.should_wakeup` policy during suspend.
--

--- Patch [12]: [PATCH V4 12/14] i3c: mipi-i3c-hci-pci: Propagate I3C wakeup requirements to PCI ---
commit 317489fcad21bddef17def19e22782f047f99dc7
Author: Adrian Hunter <[email protected]>

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

This commit updates the driver to keep the PCI wakeup state aligned with
the wakeup requirements of the attached I3C devices. During suspend, it
checks whether any HCI instance has a wakeup-enabled device and alters the
PCI function wakeup state 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..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

[ ... ]

> @@ -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]
The commit message notes that the PCI device power/wakeup sysfs attribute no
longer provides independent wakeup control. However, does this approach
conflict with the PM core architecture expectation that the should_wakeup
state and device_set_wakeup_enable() belong strictly to userspace?

By dynamically altering the core device PM state here in
mipi_i3c_hci_pci_suspend(), the driver clobbers any user-configured wakeup
policy on every system or runtime suspend. Is there an alternative way to
handle the I3C wakeup propagation without overriding the sysfs UAPI contract?

> +
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12