[PATCH] xhci: Change pm_runtime_forbid to pm_runtime_disable

[PATCH] xhci: Change pm_runtime_forbid to pm_runtime_disable

[Basavaraj Natikar]

Change pm_runtime_forbid to pm_runtime_disable to disable RPM on
platforms that don't support runtime D3, as re-enabling it through sysfs
auto power control may cause the controller to malfunction. This can lead
to issues such as hotplug devices not being detected due to failed
interrupt generation.

Fixes: a5d6264b638e ("xhci: Enable RPM on controllers that support low-power states")
Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
 drivers/usb/host/xhci-pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index dc1e345ab67e..39ab32fed3f4 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -635,7 +635,7 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	pm_runtime_put_noidle(&dev->dev);
 
 	if (pci_choose_state(dev, PMSG_SUSPEND) == PCI_D0)
-		pm_runtime_forbid(&dev->dev);
+		pm_runtime_disable(&dev->dev);
 	else if (xhci->quirks & XHCI_DEFAULT_PM_RUNTIME_ALLOW)
 		pm_runtime_allow(&dev->dev);
 
-- 
2.25.1

Re: [PATCH] xhci: Change pm_runtime_forbid to pm_runtime_disable

[Mathias Nyman]

On 25.9.2024 19.15, Basavaraj Natikar wrote:
> Change pm_runtime_forbid to pm_runtime_disable to disable RPM on
> platforms that don't support runtime D3, as re-enabling it through sysfs
> auto power control may cause the controller to malfunction. This can lead
> to issues such as hotplug devices not being detected due to failed
> interrupt generation.
> 
> Fixes: a5d6264b638e ("xhci: Enable RPM on controllers that support low-power states")
> Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> ---

Added to queue

Thanks
-Mathias

Re: [PATCH] xhci: Change pm_runtime_forbid to pm_runtime_disable

[Johan Hovold]

On Wed, Sep 25, 2024 at 09:45:20PM +0530, Basavaraj Natikar wrote:
> Change pm_runtime_forbid to pm_runtime_disable to disable RPM on
> platforms that don't support runtime D3, as re-enabling it through sysfs
> auto power control may cause the controller to malfunction. This can lead
> to issues such as hotplug devices not being detected due to failed
> interrupt generation.
> 
> Fixes: a5d6264b638e ("xhci: Enable RPM on controllers that support low-power states")
> Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> ---
>  drivers/usb/host/xhci-pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index dc1e345ab67e..39ab32fed3f4 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -635,7 +635,7 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	pm_runtime_put_noidle(&dev->dev);
>  
>  	if (pci_choose_state(dev, PMSG_SUSPEND) == PCI_D0)
> -		pm_runtime_forbid(&dev->dev);
> +		pm_runtime_disable(&dev->dev);

This looks broken. Runtime PM enable/disable should always be balanced,
and unlike forbid(), disable() doesn't make sure that the device is
actually active.

If you want to prevent the device from runtime suspending you can just
increment the usage counter here in probe() and drop it in remove().

>  	else if (xhci->quirks & XHCI_DEFAULT_PM_RUNTIME_ALLOW)
>  		pm_runtime_allow(&dev->dev);

NAK.

Johan

Re: [PATCH] xhci: Change pm_runtime_forbid to pm_runtime_disable

[Mathias Nyman]

On 30.9.2024 15.57, Johan Hovold wrote:
> On Wed, Sep 25, 2024 at 09:45:20PM +0530, Basavaraj Natikar wrote:
>> Change pm_runtime_forbid to pm_runtime_disable to disable RPM on
>> platforms that don't support runtime D3, as re-enabling it through sysfs
>> auto power control may cause the controller to malfunction. This can lead
>> to issues such as hotplug devices not being detected due to failed
>> interrupt generation.
>>
>> Fixes: a5d6264b638e ("xhci: Enable RPM on controllers that support low-power states")
>> Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
>> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>> ---
>>   drivers/usb/host/xhci-pci.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
>> index dc1e345ab67e..39ab32fed3f4 100644
>> --- a/drivers/usb/host/xhci-pci.c
>> +++ b/drivers/usb/host/xhci-pci.c
>> @@ -635,7 +635,7 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>   	pm_runtime_put_noidle(&dev->dev);
>>   
>>   	if (pci_choose_state(dev, PMSG_SUSPEND) == PCI_D0)
>> -		pm_runtime_forbid(&dev->dev);
>> +		pm_runtime_disable(&dev->dev);
> 
> This looks broken. Runtime PM enable/disable should always be balanced,
> and unlike forbid(), disable() doesn't make sure that the device is
> actually active.
> 
> If you want to prevent the device from runtime suspending you can just
> increment the usage counter here in probe() and drop it in remove().

Good point.
Adding a pm_runtime_get() here, and pm_runtime_put() in remove sounds much better
The PCI code should have set it active and enabled it already.

Dropping this

Thanks
Mathias