Re: [PATCH 2/2] usb: pci-quirks: Minor cleanup for AMD PLL quirk
On Fri, Jul 5, 2019 at 3:10 PM Alan Stern 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
>
> > @@ -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
>
> Alan Stern
>
Re: [PATCH 1/2] usb: pci-quirks: Correct AMD PLL quirk detection
On Fri, Jul 5, 2019 at 1:22 AM Greg KH 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 > > --- > > 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
[PATCH 2/2] usb: pci-quirks: Minor cleanup for AMD PLL quirk
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
---
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(_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(_lock, flags);
- return amd_chipset.probe_result;
+ return;
}
memset(, 0, sizeof(info));
spin_unlock_irqrestore(_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(_lock, flags);
@@ -283,10 +281,7 @@ int usb_amd_find_chipset_info(void)
amd_chipset = info;
spin_unlock_irqrestore(_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
[PATCH 1/2] usb: pci-quirks: Correct AMD PLL quirk detection
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
---
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(_lock, flags);
@@ -219,21 +219,28 @@ int usb_amd_find_chipset_info(void)
spin_unlock_irqrestore(_lock, flags);
if (!amd_chipset_sb_type_init()) {
- 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(_lock, flags);
@@ -277,7 +284,7 @@ int usb_amd_find_chipset_info(void)
spin_unlock_irqrestore(_lock, flags);
}
- return ret;
+ return need_pll_quirk;
}
EXPORT_SYMBOL_GPL(usb_amd_find_chipset_info);
--
2.21.0
[PATCH 0/2] usb: pci-quirks: AMD PLL quirk fix
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
