Re: [PATCH V2 6/8] i3c: mipi-i3c-hci-pci: Propagate I3C wakeup requirements to PCI

[Adrian Hunter <adrian.hunter@intel.com>]

On 28/07/2026 20:18, sashiko-bot@kernel.org wrote:
> 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

It never is.  Doesn't happen.

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

No, the PCI driver is a glue driver for the platform driver.
They are in the same directory and are meant to work together.

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

There is no standard API for wakeup propagation.

> 
> [ ... ]
> 
>> @@ -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?

It is not unprecedented.  Changes to the wakeup state should be
relatively rare i.e. mostly device_may_wakeup(dev) == pm_data.may_wakeup
so no actual update.

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

There is a debug message, but that is all.

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

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

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


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