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

On 06/08/2026 16:56, [email protected] wrote:
> 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?

Not really.  The user sets policy via I3C devices' wakeup attributes not the
bus controller's wakeup attribute.

> 
> 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?

Not really overriding, and no alternative.

> 
>> +
>> +	return 0;
>>  }
>