diff mbox series

[1/2] Revert "xhci: Enable RPM on controllers that support low-power states"

Message ID 20231204100859.1332772-1-mathias.nyman@linux.intel.com
State New
Headers show
Series [1/2] Revert "xhci: Enable RPM on controllers that support low-power states" | expand

Commit Message

Mathias Nyman Dec. 4, 2023, 10:08 a.m. UTC
This reverts commit a5d6264b638efeca35eff72177fd28d149e0764b.

This patch was an attempt to solve issues seen when enabling runtime PM
as default for all AMD 1.1 xHC hosts. see commit 4baf12181509
("xhci: Loosen RPM as default policy to cover for AMD xHC 1.1")

This was not enough, regressions are still seen, so start from a clean
slate and revert both of them.

This patch went to stable and should be reverted from there as well

Fixes: a5d6264b638e ("xhci: Enable RPM on controllers that support low-power states")
Cc: stable@vger.kernel.org
Cc: Mario Limonciello <mario.limonciello@amd.com>
Cc: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-pci.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Basavaraj Natikar Dec. 4, 2023, 10:49 a.m. UTC | #1
On 12/4/2023 3:38 PM, Mathias Nyman wrote:
> This reverts commit a5d6264b638efeca35eff72177fd28d149e0764b.
>
> This patch was an attempt to solve issues seen when enabling runtime PM
> as default for all AMD 1.1 xHC hosts. see commit 4baf12181509
> ("xhci: Loosen RPM as default policy to cover for AMD xHC 1.1")

AFAK, only 4baf12181509 commit has regression on AMD xHc 1.1 below is not regression
patch and its unrelated to AMD xHC 1.1.

Only [PATCH 2/2] Revert "xhci: Loosen RPM as default policy to cover for AMD xHC 1.1"
alone in this series solves regression issues.

>
> This was not enough, regressions are still seen, so start from a clean
> slate and revert both of them.
>
> This patch went to stable and should be reverted from there as well
>
> Fixes: a5d6264b638e ("xhci: Enable RPM on controllers that support low-power states")
> Cc: stable@vger.kernel.org
> Cc: Mario Limonciello <mario.limonciello@amd.com>
> Cc: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/host/xhci-pci.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 95ed9404f6f8..bde43cef8846 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -695,9 +695,7 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	/* USB-2 and USB-3 roothubs initialized, allow runtime pm suspend */
>  	pm_runtime_put_noidle(&dev->dev);
>  
> -	if (pci_choose_state(dev, PMSG_SUSPEND) == PCI_D0)
> -		pm_runtime_forbid(&dev->dev);
> -	else if (xhci->quirks & XHCI_DEFAULT_PM_RUNTIME_ALLOW)
> +	if (xhci->quirks & XHCI_DEFAULT_PM_RUNTIME_ALLOW)
>  		pm_runtime_allow(&dev->dev);
>  
>  	dma_set_max_seg_size(&dev->dev, UINT_MAX);
Mathias Nyman Dec. 4, 2023, 2:22 p.m. UTC | #2
On 4.12.2023 12.49, Basavaraj Natikar wrote:
> 
> On 12/4/2023 3:38 PM, Mathias Nyman wrote:
>> This reverts commit a5d6264b638efeca35eff72177fd28d149e0764b.
>>
>> This patch was an attempt to solve issues seen when enabling runtime PM
>> as default for all AMD 1.1 xHC hosts. see commit 4baf12181509
>> ("xhci: Loosen RPM as default policy to cover for AMD xHC 1.1")
> 
> AFAK, only 4baf12181509 commit has regression on AMD xHc 1.1 below is not regression
> patch and its unrelated to AMD xHC 1.1.
> 
> Only [PATCH 2/2] Revert "xhci: Loosen RPM as default policy to cover for AMD xHC 1.1"
> alone in this series solves regression issues.
> 

Patch a5d6264b638e ("xhci: Enable RPM on controllers that support low-power states")
was originally not supposed to go to stable. It was added later as it solved some
cases triggered by 4baf12181509 ("xhci: Loosen RPM as default policy to cover for AMD xHC 1.1")
see:
https://lore.kernel.org/linux-usb/5993222.lOV4Wx5bFT@natalenko.name/

Turns out it wasn't enough.

If we now revert 4baf12181509 "xhci: Loosen RPM as default policy to cover for AMD xHC 1.1"
I still think it makes sense to also revert a5d6264b638e.
Especially from the stable kernels.

This way we roll back this whole issue to a known working state.

Thanks
Mathias
Basavaraj Natikar Dec. 4, 2023, 2:49 p.m. UTC | #3
On 12/4/2023 7:52 PM, Mathias Nyman wrote:
> On 4.12.2023 12.49, Basavaraj Natikar wrote:
>>
>> On 12/4/2023 3:38 PM, Mathias Nyman wrote:
>>> This reverts commit a5d6264b638efeca35eff72177fd28d149e0764b.
>>>
>>> This patch was an attempt to solve issues seen when enabling runtime PM
>>> as default for all AMD 1.1 xHC hosts. see commit 4baf12181509
>>> ("xhci: Loosen RPM as default policy to cover for AMD xHC 1.1")
>>
>> AFAK, only 4baf12181509 commit has regression on AMD xHc 1.1 below is
>> not regression
>> patch and its unrelated to AMD xHC 1.1.
>>
>> Only [PATCH 2/2] Revert "xhci: Loosen RPM as default policy to cover
>> for AMD xHC 1.1"
>> alone in this series solves regression issues.
>>
>
> Patch a5d6264b638e ("xhci: Enable RPM on controllers that support
> low-power states")
> was originally not supposed to go to stable. It was added later as it
> solved some
> cases triggered by 4baf12181509 ("xhci: Loosen RPM as default policy
> to cover for AMD xHC 1.1")
> see:
> https://lore.kernel.org/linux-usb/5993222.lOV4Wx5bFT@natalenko.name/
>
> Turns out it wasn't enough.
>
> If we now revert 4baf12181509 "xhci: Loosen RPM as default policy to
> cover for AMD xHC 1.1"
> I still think it makes sense to also revert a5d6264b638e.
> Especially from the stable kernels.

Yes , a5d6264b638e still solves other issues if underlying hardware doesn't support RPM
if we revert a5d6264b638e on stable releases then new issues (not related to regression)
other than AMD xHC 1.1 controllers including xHC 1.2 will still exist on stable releases.
If revert then we can backport to stable release later if required.

Sure, will send a follow up patch to fix 4baf12181509 alone on mainline if revert on all releases.

>
> This way we roll back this whole issue to a known working state.

Sure, for at-least a5d6264b638e if not revert on mainline then will not resend the same patch.

Thanks,
--
Basavaraj

>
> Thanks
> Mathias
Greg KH Dec. 4, 2023, 11:55 p.m. UTC | #4
On Mon, Dec 04, 2023 at 08:59:35PM +0530, Basavaraj Natikar wrote:
> 
> On 12/4/2023 8:36 PM, Mathias Nyman wrote:
> > On 4.12.2023 16.49, Basavaraj Natikar wrote:
> >>
> >> On 12/4/2023 7:52 PM, Mathias Nyman wrote:
> >>> On 4.12.2023 12.49, Basavaraj Natikar wrote:
> >>>>
> >>>> On 12/4/2023 3:38 PM, Mathias Nyman wrote:
> >>>>> This reverts commit a5d6264b638efeca35eff72177fd28d149e0764b.
> >>>>>
> >>>>> This patch was an attempt to solve issues seen when enabling
> >>>>> runtime PM
> >>>>> as default for all AMD 1.1 xHC hosts. see commit 4baf12181509
> >>>>> ("xhci: Loosen RPM as default policy to cover for AMD xHC 1.1")
> >>>>
> >>>> AFAK, only 4baf12181509 commit has regression on AMD xHc 1.1 below is
> >>>> not regression
> >>>> patch and its unrelated to AMD xHC 1.1.
> >>>>
> >>>> Only [PATCH 2/2] Revert "xhci: Loosen RPM as default policy to cover
> >>>> for AMD xHC 1.1"
> >>>> alone in this series solves regression issues.
> >>>>
> >>>
> >>> Patch a5d6264b638e ("xhci: Enable RPM on controllers that support
> >>> low-power states")
> >>> was originally not supposed to go to stable. It was added later as it
> >>> solved some
> >>> cases triggered by 4baf12181509 ("xhci: Loosen RPM as default policy
> >>> to cover for AMD xHC 1.1")
> >>> see:
> >>> https://lore.kernel.org/linux-usb/5993222.lOV4Wx5bFT@natalenko.name/
> >>>
> >>> Turns out it wasn't enough.
> >>>
> >>> If we now revert 4baf12181509 "xhci: Loosen RPM as default policy to
> >>> cover for AMD xHC 1.1"
> >>> I still think it makes sense to also revert a5d6264b638e.
> >>> Especially from the stable kernels.
> >>
> >> Yes , a5d6264b638e still solves other issues if underlying hardware
> >> doesn't support RPM
> >> if we revert a5d6264b638e on stable releases then new issues (not
> >> related to regression)
> >> other than AMD xHC 1.1 controllers including xHC 1.2 will still exist
> >> on stable releases.
> >
> > Ok, got it, so a5d6264b638e also solves other issues than those
> > exposed by 4baf12181509.
> > And that one (a5d6264b638) should originally have been marked for stable.
> >
> > So only revert 4baf12181509, PATCH 2/2 in this series
> 
> Thank you, that is correct.

So just take patch 2/2 here, or will someone be sending me a new patch?

thanks,

greg k-h
patchwork-bot+bluetooth@kernel.org Dec. 15, 2023, 4:53 p.m. UTC | #5
Hello:

This series was applied to bluetooth/bluetooth-next.git (master)
by Greg Kroah-Hartman <gregkh@linuxfoundation.org>:

On Mon,  4 Dec 2023 12:08:58 +0200 you wrote:
> This reverts commit a5d6264b638efeca35eff72177fd28d149e0764b.
> 
> This patch was an attempt to solve issues seen when enabling runtime PM
> as default for all AMD 1.1 xHC hosts. see commit 4baf12181509
> ("xhci: Loosen RPM as default policy to cover for AMD xHC 1.1")
> 
> This was not enough, regressions are still seen, so start from a clean
> slate and revert both of them.
> 
> [...]

Here is the summary with links:
  - [1/2] Revert "xhci: Enable RPM on controllers that support low-power states"
    (no matching commit)
  - [2/2] Revert "xhci: Loosen RPM as default policy to cover for AMD xHC 1.1"
    https://git.kernel.org/bluetooth/bluetooth-next/c/24be0b3c4059

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 95ed9404f6f8..bde43cef8846 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -695,9 +695,7 @@  static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	/* USB-2 and USB-3 roothubs initialized, allow runtime pm suspend */
 	pm_runtime_put_noidle(&dev->dev);
 
-	if (pci_choose_state(dev, PMSG_SUSPEND) == PCI_D0)
-		pm_runtime_forbid(&dev->dev);
-	else if (xhci->quirks & XHCI_DEFAULT_PM_RUNTIME_ALLOW)
+	if (xhci->quirks & XHCI_DEFAULT_PM_RUNTIME_ALLOW)
 		pm_runtime_allow(&dev->dev);
 
 	dma_set_max_seg_size(&dev->dev, UINT_MAX);