[PATCH 0/2] usb: pci-quirks: AMD PLL quirk fix

[PATCH 0/2] usb: pci-quirks: AMD PLL quirk fix

[Ryan Kennedy]

This series contains a minor fix for the AMD PLL USB quirk plus
some clean up to the related code.

Ryan Kennedy (2):
  usb: pci-quirks: Correct AMD PLL quirk detection
  usb: pci-quirks: Minor cleanup for AMD PLL quirk

 drivers/usb/host/ehci-pci.c   |  4 ++--
 drivers/usb/host/ohci-pci.c   |  2 +-
 drivers/usb/host/pci-quirks.c | 45 +++++++++++++++++++++--------------
 drivers/usb/host/pci-quirks.h |  2 +-
 drivers/usb/host/xhci-pci.c   |  2 +-
 5 files changed, 32 insertions(+), 23 deletions(-)

-- 
2.21.0

[PATCH 1/2] usb: pci-quirks: Correct AMD PLL quirk detection

[Ryan Kennedy]

The AMD PLL USB quirk is incorrectly enabled on newer Ryzen
chipsets. The logic in usb_amd_find_chipset_info currently checks
for unaffected chipsets rather than affected ones. This broke
once a new chipset was added in e788787ef. It makes more sense
to reverse the logic so it won't need to be updated as new
chipsets are added. Note that the core of the workaround in
usb_amd_quirk_pll does correctly check the chipset.

Signed-off-by: Ryan Kennedy <ryan5544@gmail.com>
---
 drivers/usb/host/pci-quirks.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 3ce71cbfbb58..ad05c27b3a7b 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -205,7 +205,7 @@ int usb_amd_find_chipset_info(void)
 {
 	unsigned long flags;
 	struct amd_chipset_info info;
-	int ret;
+	int need_pll_quirk = 0;
 
 	spin_lock_irqsave(&amd_lock, flags);
 
@@ -219,21 +219,28 @@ int usb_amd_find_chipset_info(void)
 	spin_unlock_irqrestore(&amd_lock, flags);
 
 	if (!amd_chipset_sb_type_init(&info)) {
-		ret = 0;
 		goto commit;
 	}
 
-	/* Below chipset generations needn't enable AMD PLL quirk */
-	if (info.sb_type.gen == AMD_CHIPSET_UNKNOWN ||
-			info.sb_type.gen == AMD_CHIPSET_SB600 ||
-			info.sb_type.gen == AMD_CHIPSET_YANGTZE ||
-			(info.sb_type.gen == AMD_CHIPSET_SB700 &&
-			info.sb_type.rev > 0x3b)) {
+	switch (info.sb_type.gen) {
+	case AMD_CHIPSET_SB700:
+		need_pll_quirk = info.sb_type.rev <= 0x3B;
+		break;
+	case AMD_CHIPSET_SB800:
+	case AMD_CHIPSET_HUDSON2:
+	case AMD_CHIPSET_BOLTON:
+		need_pll_quirk = 1;
+		break;
+	default:
+		need_pll_quirk = 0;
+		break;
+	}
+
+	if (!need_pll_quirk) {
 		if (info.smbus_dev) {
 			pci_dev_put(info.smbus_dev);
 			info.smbus_dev = NULL;
 		}
-		ret = 0;
 		goto commit;
 	}
 
@@ -252,7 +259,7 @@ int usb_amd_find_chipset_info(void)
 		}
 	}
 
-	ret = info.probe_result = 1;
+	need_pll_quirk = info.probe_result = 1;
 	printk(KERN_DEBUG "QUIRK: Enable AMD PLL fix\n");
 
 commit:
@@ -263,7 +270,7 @@ int usb_amd_find_chipset_info(void)
 
 		/* Mark that we where here */
 		amd_chipset.probe_count++;
-		ret = amd_chipset.probe_result;
+		need_pll_quirk = amd_chipset.probe_result;
 
 		spin_unlock_irqrestore(&amd_lock, flags);
 
@@ -277,7 +284,7 @@ int usb_amd_find_chipset_info(void)
 		spin_unlock_irqrestore(&amd_lock, flags);
 	}
 
-	return ret;
+	return need_pll_quirk;
 }
 EXPORT_SYMBOL_GPL(usb_amd_find_chipset_info);
 
-- 
2.21.0

[PATCH 2/2] usb: pci-quirks: Minor cleanup for AMD PLL quirk

[Ryan Kennedy]

usb_amd_find_chipset_info() is used for chipset detection for
several quirks. It is strange that its return value indicates
the need for the PLL quirk, which means it is often ignored.
This patch adds a function specifically for checking the PLL
quirk like the other ones. Additionally, rename probe_result to
something more appropriate.

Signed-off-by: Ryan Kennedy <ryan5544@gmail.com>
---
 drivers/usb/host/ehci-pci.c   |  4 ++--
 drivers/usb/host/ohci-pci.c   |  2 +-
 drivers/usb/host/pci-quirks.c | 30 ++++++++++++++++--------------
 drivers/usb/host/pci-quirks.h |  2 +-
 drivers/usb/host/xhci-pci.c   |  2 +-
 5 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index fe9422d3bcdc..b0882c13a1d1 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -149,7 +149,7 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
 		break;
 	case PCI_VENDOR_ID_AMD:
 		/* AMD PLL quirk */
-		if (usb_amd_find_chipset_info())
+		if (usb_amd_quirk_pll_check())
 			ehci->amd_pll_fix = 1;
 		/* AMD8111 EHCI doesn't work, according to AMD errata */
 		if (pdev->device == 0x7463) {
@@ -186,7 +186,7 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
 		break;
 	case PCI_VENDOR_ID_ATI:
 		/* AMD PLL quirk */
-		if (usb_amd_find_chipset_info())
+		if (usb_amd_quirk_pll_check())
 			ehci->amd_pll_fix = 1;
 
 		/*
diff --git a/drivers/usb/host/ohci-pci.c b/drivers/usb/host/ohci-pci.c
index fbcd34911025..208abaaef8f6 100644
--- a/drivers/usb/host/ohci-pci.c
+++ b/drivers/usb/host/ohci-pci.c
@@ -152,7 +152,7 @@ static int ohci_quirk_amd700(struct usb_hcd *hcd)
 {
 	struct ohci_hcd *ohci = hcd_to_ohci(hcd);
 
-	if (usb_amd_find_chipset_info())
+	if (usb_amd_quirk_pll_check())
 		ohci->flags |= OHCI_QUIRK_AMD_PLL;
 
 	/* SB800 needs pre-fetch fix */
diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index ad05c27b3a7b..f6d04491df60 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -132,7 +132,7 @@ static struct amd_chipset_info {
 	struct amd_chipset_type sb_type;
 	int isoc_reqs;
 	int probe_count;
-	int probe_result;
+	bool need_pll_quirk;
 } amd_chipset;
 
 static DEFINE_SPINLOCK(amd_lock);
@@ -201,11 +201,11 @@ void sb800_prefetch(struct device *dev, int on)
 }
 EXPORT_SYMBOL_GPL(sb800_prefetch);
 
-int usb_amd_find_chipset_info(void)
+static void usb_amd_find_chipset_info(void)
 {
 	unsigned long flags;
 	struct amd_chipset_info info;
-	int need_pll_quirk = 0;
+	info.need_pll_quirk = 0;
 
 	spin_lock_irqsave(&amd_lock, flags);
 
@@ -213,7 +213,7 @@ int usb_amd_find_chipset_info(void)
 	if (amd_chipset.probe_count > 0) {
 		amd_chipset.probe_count++;
 		spin_unlock_irqrestore(&amd_lock, flags);
-		return amd_chipset.probe_result;
+		return;
 	}
 	memset(&info, 0, sizeof(info));
 	spin_unlock_irqrestore(&amd_lock, flags);
@@ -224,19 +224,19 @@ int usb_amd_find_chipset_info(void)
 
 	switch (info.sb_type.gen) {
 	case AMD_CHIPSET_SB700:
-		need_pll_quirk = info.sb_type.rev <= 0x3B;
+		info.need_pll_quirk = info.sb_type.rev <= 0x3B;
 		break;
 	case AMD_CHIPSET_SB800:
 	case AMD_CHIPSET_HUDSON2:
 	case AMD_CHIPSET_BOLTON:
-		need_pll_quirk = 1;
+		info.need_pll_quirk = 1;
 		break;
 	default:
-		need_pll_quirk = 0;
+		info.need_pll_quirk = 0;
 		break;
 	}
 
-	if (!need_pll_quirk) {
+	if (!info.need_pll_quirk) {
 		if (info.smbus_dev) {
 			pci_dev_put(info.smbus_dev);
 			info.smbus_dev = NULL;
@@ -259,7 +259,6 @@ int usb_amd_find_chipset_info(void)
 		}
 	}
 
-	need_pll_quirk = info.probe_result = 1;
 	printk(KERN_DEBUG "QUIRK: Enable AMD PLL fix\n");
 
 commit:
@@ -270,7 +269,6 @@ int usb_amd_find_chipset_info(void)
 
 		/* Mark that we where here */
 		amd_chipset.probe_count++;
-		need_pll_quirk = amd_chipset.probe_result;
 
 		spin_unlock_irqrestore(&amd_lock, flags);
 
@@ -283,10 +281,7 @@ int usb_amd_find_chipset_info(void)
 		amd_chipset = info;
 		spin_unlock_irqrestore(&amd_lock, flags);
 	}
-
-	return need_pll_quirk;
 }
-EXPORT_SYMBOL_GPL(usb_amd_find_chipset_info);
 
 int usb_hcd_amd_remote_wakeup_quirk(struct pci_dev *pdev)
 {
@@ -322,6 +317,13 @@ bool usb_amd_prefetch_quirk(void)
 }
 EXPORT_SYMBOL_GPL(usb_amd_prefetch_quirk);
 
+bool usb_amd_quirk_pll_check(void)
+{
+	usb_amd_find_chipset_info();
+	return amd_chipset.need_pll_quirk;
+}
+EXPORT_SYMBOL_GPL(usb_amd_quirk_pll_check);
+
 /*
  * The hardware normally enables the A-link power management feature, which
  * lets the system lower the power consumption in idle states.
@@ -527,7 +529,7 @@ void usb_amd_dev_put(void)
 	amd_chipset.nb_type = 0;
 	memset(&amd_chipset.sb_type, 0, sizeof(amd_chipset.sb_type));
 	amd_chipset.isoc_reqs = 0;
-	amd_chipset.probe_result = 0;
+	amd_chipset.need_pll_quirk = 0;
 
 	spin_unlock_irqrestore(&amd_lock, flags);
 
diff --git a/drivers/usb/host/pci-quirks.h b/drivers/usb/host/pci-quirks.h
index 63c633077d9e..e729de21fad7 100644
--- a/drivers/usb/host/pci-quirks.h
+++ b/drivers/usb/host/pci-quirks.h
@@ -5,11 +5,11 @@
 #ifdef CONFIG_USB_PCI
 void uhci_reset_hc(struct pci_dev *pdev, unsigned long base);
 int uhci_check_and_reset_hc(struct pci_dev *pdev, unsigned long base);
-int usb_amd_find_chipset_info(void);
 int usb_hcd_amd_remote_wakeup_quirk(struct pci_dev *pdev);
 bool usb_amd_hang_symptom_quirk(void);
 bool usb_amd_prefetch_quirk(void);
 void usb_amd_dev_put(void);
+bool usb_amd_quirk_pll_check(void);
 void usb_amd_quirk_pll_disable(void);
 void usb_amd_quirk_pll_enable(void);
 void usb_asmedia_modifyflowcontrol(struct pci_dev *pdev);
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index c2fe218e051f..1e0236e90687 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -130,7 +130,7 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 		xhci->quirks |= XHCI_AMD_0x96_HOST;
 
 	/* AMD PLL quirk */
-	if (pdev->vendor == PCI_VENDOR_ID_AMD && usb_amd_find_chipset_info())
+	if (pdev->vendor == PCI_VENDOR_ID_AMD && usb_amd_quirk_pll_check())
 		xhci->quirks |= XHCI_AMD_PLL_FIX;
 
 	if (pdev->vendor == PCI_VENDOR_ID_AMD &&
-- 
2.21.0

Re: [PATCH 1/2] usb: pci-quirks: Correct AMD PLL quirk detection

[Greg KH]

On Thu, Jul 04, 2019 at 11:35:28AM -0400, Ryan Kennedy wrote:
> The AMD PLL USB quirk is incorrectly enabled on newer Ryzen
> chipsets. The logic in usb_amd_find_chipset_info currently checks
> for unaffected chipsets rather than affected ones. This broke
> once a new chipset was added in e788787ef. It makes more sense
> to reverse the logic so it won't need to be updated as new
> chipsets are added. Note that the core of the workaround in
> usb_amd_quirk_pll does correctly check the chipset.
> 
> Signed-off-by: Ryan Kennedy <ryan5544@gmail.com>
> ---
>  drivers/usb/host/pci-quirks.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)

Should this be backported to stable kernels?

thanks,

greg k-h

Re: [PATCH 1/2] usb: pci-quirks: Correct AMD PLL quirk detection

[Ryan Kennedy]

On Fri, Jul 5, 2019 at 1:22 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jul 04, 2019 at 11:35:28AM -0400, Ryan Kennedy wrote:
> > The AMD PLL USB quirk is incorrectly enabled on newer Ryzen
> > chipsets. The logic in usb_amd_find_chipset_info currently checks
> > for unaffected chipsets rather than affected ones. This broke
> > once a new chipset was added in e788787ef. It makes more sense
> > to reverse the logic so it won't need to be updated as new
> > chipsets are added. Note that the core of the workaround in
> > usb_amd_quirk_pll does correctly check the chipset.
> >
> > Signed-off-by: Ryan Kennedy <ryan5544@gmail.com>
> > ---
> >  drivers/usb/host/pci-quirks.c | 31 +++++++++++++++++++------------
> >  1 file changed, 19 insertions(+), 12 deletions(-)
>
> Should this be backported to stable kernels?

The bug is fairly harmless, so I wouldn't say it's a must-have. I only
noticed this because I saw the log message and was curious what the
quirk was for. The fix saves us calling usb_amd_quirk_pll() and taking
the lock in there. Others here should know better than I what's stable
worthy.

Ryan

>
> thanks,
>
> greg k-h

Re: [PATCH 1/2] usb: pci-quirks: Correct AMD PLL quirk detection

[Alan Stern]

On Thu, 4 Jul 2019, Ryan Kennedy wrote:

> The AMD PLL USB quirk is incorrectly enabled on newer Ryzen
> chipsets. The logic in usb_amd_find_chipset_info currently checks
> for unaffected chipsets rather than affected ones. This broke
> once a new chipset was added in e788787ef. It makes more sense
> to reverse the logic so it won't need to be updated as new
> chipsets are added. Note that the core of the workaround in
> usb_amd_quirk_pll does correctly check the chipset.
> 
> Signed-off-by: Ryan Kennedy <ryan5544@gmail.com>
> ---
>  drivers/usb/host/pci-quirks.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 3ce71cbfbb58..ad05c27b3a7b 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -205,7 +205,7 @@ int usb_amd_find_chipset_info(void)
>  {
>  	unsigned long flags;
>  	struct amd_chipset_info info;
> -	int ret;
> +	int need_pll_quirk = 0;
>  
>  	spin_lock_irqsave(&amd_lock, flags);
>  
> @@ -219,21 +219,28 @@ int usb_amd_find_chipset_info(void)
>  	spin_unlock_irqrestore(&amd_lock, flags);
>  
>  	if (!amd_chipset_sb_type_init(&info)) {
> -		ret = 0;
>  		goto commit;
>  	}
>  
> -	/* Below chipset generations needn't enable AMD PLL quirk */
> -	if (info.sb_type.gen == AMD_CHIPSET_UNKNOWN ||
> -			info.sb_type.gen == AMD_CHIPSET_SB600 ||
> -			info.sb_type.gen == AMD_CHIPSET_YANGTZE ||
> -			(info.sb_type.gen == AMD_CHIPSET_SB700 &&
> -			info.sb_type.rev > 0x3b)) {
> +	switch (info.sb_type.gen) {
> +	case AMD_CHIPSET_SB700:
> +		need_pll_quirk = info.sb_type.rev <= 0x3B;
> +		break;
> +	case AMD_CHIPSET_SB800:
> +	case AMD_CHIPSET_HUDSON2:
> +	case AMD_CHIPSET_BOLTON:
> +		need_pll_quirk = 1;
> +		break;
> +	default:
> +		need_pll_quirk = 0;
> +		break;
> +	}
> +
> +	if (!need_pll_quirk) {
>  		if (info.smbus_dev) {
>  			pci_dev_put(info.smbus_dev);
>  			info.smbus_dev = NULL;
>  		}
> -		ret = 0;
>  		goto commit;
>  	}
>  
> @@ -252,7 +259,7 @@ int usb_amd_find_chipset_info(void)
>  		}
>  	}
>  
> -	ret = info.probe_result = 1;
> +	need_pll_quirk = info.probe_result = 1;
>  	printk(KERN_DEBUG "QUIRK: Enable AMD PLL fix\n");
>  
>  commit:
> @@ -263,7 +270,7 @@ int usb_amd_find_chipset_info(void)
>  
>  		/* Mark that we where here */
>  		amd_chipset.probe_count++;
> -		ret = amd_chipset.probe_result;
> +		need_pll_quirk = amd_chipset.probe_result;
>  
>  		spin_unlock_irqrestore(&amd_lock, flags);
>  
> @@ -277,7 +284,7 @@ int usb_amd_find_chipset_info(void)
>  		spin_unlock_irqrestore(&amd_lock, flags);
>  	}
>  
> -	return ret;
> +	return need_pll_quirk;
>  }
>  EXPORT_SYMBOL_GPL(usb_amd_find_chipset_info);

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH 2/2] usb: pci-quirks: Minor cleanup for AMD PLL quirk

[Alan Stern]

On Thu, 4 Jul 2019, Ryan Kennedy wrote:

> usb_amd_find_chipset_info() is used for chipset detection for
> several quirks. It is strange that its return value indicates
> the need for the PLL quirk, which means it is often ignored.
> This patch adds a function specifically for checking the PLL
> quirk like the other ones. Additionally, rename probe_result to
> something more appropriate.
> 
> Signed-off-by: Ryan Kennedy <ryan5544@gmail.com>

> @@ -322,6 +317,13 @@ bool usb_amd_prefetch_quirk(void)
>  }
>  EXPORT_SYMBOL_GPL(usb_amd_prefetch_quirk);
>  
> +bool usb_amd_quirk_pll_check(void)
> +{
> +	usb_amd_find_chipset_info();
> +	return amd_chipset.need_pll_quirk;
> +}
> +EXPORT_SYMBOL_GPL(usb_amd_quirk_pll_check);

I really don't see the point of separating out all but one line into a
different function.  You might as well just rename 
usb_amd_find_chipset_info to usb_amd_quirk_pll_check (along with the 
other code adjustments) and be done with it.

However, in the end I don't care if you still want to do this.  Either 
way:

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern

Re: [PATCH 2/2] usb: pci-quirks: Minor cleanup for AMD PLL quirk

[Ryan Kennedy]

On Fri, Jul 5, 2019 at 3:10 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Thu, 4 Jul 2019, Ryan Kennedy wrote:
>
> > usb_amd_find_chipset_info() is used for chipset detection for
> > several quirks. It is strange that its return value indicates
> > the need for the PLL quirk, which means it is often ignored.
> > This patch adds a function specifically for checking the PLL
> > quirk like the other ones. Additionally, rename probe_result to
> > something more appropriate.
> >
> > Signed-off-by: Ryan Kennedy <ryan5544@gmail.com>
>
> > @@ -322,6 +317,13 @@ bool usb_amd_prefetch_quirk(void)
> >  }
> >  EXPORT_SYMBOL_GPL(usb_amd_prefetch_quirk);
> >
> > +bool usb_amd_quirk_pll_check(void)
> > +{
> > +     usb_amd_find_chipset_info();
> > +     return amd_chipset.need_pll_quirk;
> > +}
> > +EXPORT_SYMBOL_GPL(usb_amd_quirk_pll_check);
>
> I really don't see the point of separating out all but one line into a
> different function.  You might as well just rename
> usb_amd_find_chipset_info to usb_amd_quirk_pll_check (along with the
> other code adjustments) and be done with it.

I did this for consistency with the others:

usb_amd_prefetch_quirk()
usb_amd_hang_symptom_quirk()
usb_hcd_amd_remote_wakeup_quirk()

They all need to ensure the chipset information exists then decide if
the particular quirk should be applied to the chipset.

Ryan

>
> However, in the end I don't care if you still want to do this.  Either
> way:
>
> Acked-by: Alan Stern <stern@rowland.harvard.edu>
>
> Alan Stern
>