[PATCH 0/6] PCI: Fix class code usage

[PATCH 0/6] PCI: Fix class code usage

[Bjorn Helgaas]

PCI class code management is confusing.  The pci_dev.class element contains
a three-byte value: base class/sub-class/interface.

The PCI_BASE_CLASS_* definitions are a single byte, i.e., dev->class >> 16.
The PCI_CLASS_* definitions are either two or three bytes, i.e., either
dev->class >> 8 or just dev->class.

We had several places where we used a two-byte PCI_CLASS_* definition but
forgot to shift it to the right place.  These patches fix that for NCR
53c810, TI816X, and Intel USB devices.

---

Bjorn Helgaas (6):
      PCI: Use PCI_CLASS_SERIAL_USB instead of bare number
      PCI: Fix generic NCR 53c810 class code quirk
      PCI: Fix TI816X class code quirk
      PCI: Fix Intel generic reset quirk class code check
      PCI: Simplify reset_intel_generic_dev()
      PCI: Shift PCI_CLASS_NOT_DEFINED consistently with other classes


 arch/x86/pci/fixup.c |   13 ----------
 drivers/pci/probe.c  |    2 +-
 drivers/pci/quirks.c |   64 ++++++++++++++++++++++++++++----------------------
 3 files changed, 37 insertions(+), 42 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in

[PATCH 1/6] PCI: Use PCI_CLASS_SERIAL_USB instead of bare number

[Bjorn Helgaas]

be6646bfbaec ("PCI: Prevent xHCI driver from claiming AMD Nolan USB3 DRD
device") added a quirk to override the PCI class code of the AMD Nolan
device.

Use PCI_CLASS_SERIAL_USB instead of a bare number to improve greppability.
Also add a log message about what we're doing.

No functional change except the new message.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: Huang Rui <ray.huang@amd.com>
CC: Jason Chang <jason.chang@amd.com>
CC: Felipe Balbi <balbi@ti.com>
---
 drivers/pci/quirks.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index cbe0351..2dab425 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -424,10 +424,12 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI,	PCI_DEVICE_ID_ATI_RS100,   quirk_ati_
  */
 static void quirk_amd_nl_class(struct pci_dev *pdev)
 {
-	/*
-	 * Use 'USB Device' (0x0c03fe) instead of PCI header provided
-	 */
-	pdev->class = 0x0c03fe;
+	u32 class = pdev->class;
+
+	/* Use "USB Device (not host controller)" class */
+	pdev->class = (PCI_CLASS_SERIAL_USB << 8) | 0xfe;
+	dev_info(&pdev->dev, "PCI class overridden (%#08x -> %#08x) so dwc3 driver can claim this instead of xhci\n",
+		 class, pdev->class);
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_NL_USB,
 		quirk_amd_nl_class);

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in

[PATCH 2/6] PCI: Fix generic NCR 53c810 class code quirk

[Bjorn Helgaas]

In the generic quirk fixup_rev1_53c810(), added by a5312e28c195 ("[PATCH]
PCI: NCR 53c810 quirk"), we assigned "class = PCI_CLASS_STORAGE_SCSI".  But
PCI_CLASS_STORAGE_SCSI is only the two-byte base class/sub-class and needs
to be shifted to make space for the low-order interface byte.

Furthermore, we had a similar quirk, pci_fixup_ncr53c810(), for arch/x86,
which assigned class correctly.  The arch code is linked before the PCI
core, so arch quirks run before generic quirks.  Therefore, on x86, the x86
arch quirk ran first, and the generic quirk did nothing because it saw that
dev->class was already set.  But on other arches, the generic quirk set the
wrong class code.

Fix the generic quirk to set the correct class code and remove the
now-unnecessary x86-specific quirk.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: Matthew Wilcox <matthew@wil.cx>
---
 arch/x86/pci/fixup.c |   13 -------------
 drivers/pci/quirks.c |   14 +++++++++-----
 2 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index 9a2b710..e585655 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -62,19 +62,6 @@ static void pci_fixup_umc_ide(struct pci_dev *d)
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_UMC, PCI_DEVICE_ID_UMC_UM8886BF, pci_fixup_umc_ide);
 
-static void pci_fixup_ncr53c810(struct pci_dev *d)
-{
-	/*
-	 * NCR 53C810 returns class code 0 (at least on some systems).
-	 * Fix class to be PCI_CLASS_STORAGE_SCSI
-	 */
-	if (!d->class) {
-		dev_warn(&d->dev, "Fixing NCR 53C810 class code\n");
-		d->class = PCI_CLASS_STORAGE_SCSI << 8;
-	}
-}
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_NCR, PCI_DEVICE_ID_NCR_53C810, pci_fixup_ncr53c810);
-
 static void pci_fixup_latency(struct pci_dev *d)
 {
 	/*
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 2dab425..64177a6 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1997,14 +1997,18 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1508, quirk_disable_aspm_l0s);
 
 static void fixup_rev1_53c810(struct pci_dev *dev)
 {
-	/* rev 1 ncr53c810 chips don't set the class at all which means
+	u32 class = dev->class;
+
+	/*
+	 * rev 1 ncr53c810 chips don't set the class at all which means
 	 * they don't get their resources remapped. Fix that here.
 	 */
+	if (class)
+		return;
 
-	if (dev->class == PCI_CLASS_NOT_DEFINED) {
-		dev_info(&dev->dev, "NCR 53c810 rev 1 detected; setting PCI class\n");
-		dev->class = PCI_CLASS_STORAGE_SCSI;
-	}
+	dev->class = PCI_CLASS_STORAGE_SCSI << 8;
+	dev_info(&dev->dev, "NCR 53c810 rev 1 PCI class overridden (%#08x -> %#08x)\n",
+		 class, dev->class);
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_NCR, PCI_DEVICE_ID_NCR_53C810, fixup_rev1_53c810);
 

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in

[PATCH 3/6] PCI: Fix TI816X class code quirk

[Bjorn Helgaas]

In fixup_ti816x_class(), we assigned "class = PCI_CLASS_MULTIMEDIA_VIDEO".
But PCI_CLASS_MULTIMEDIA_VIDEO is only the two-byte base class/sub-class
and needs to be shifted to make space for the low-order interface byte.

Shift PCI_CLASS_MULTIMEDIA_VIDEO to set the correct class code.

Fixes: 63c4408074cb ("PCI: Add quirk for setting valid class for TI816X Endpoint")
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: Hemant Pedanekar <hemantp@ti.com>
---
 drivers/pci/quirks.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 64177a6..8bc60c2 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2844,12 +2844,15 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x3c28, vtd_mask_spec_errors);
 
 static void fixup_ti816x_class(struct pci_dev *dev)
 {
+	u32 class = dev->class;
+
 	/* TI 816x devices do not have class code set when in PCIe boot mode */
-	dev_info(&dev->dev, "Setting PCI class for 816x PCIe device\n");
-	dev->class = PCI_CLASS_MULTIMEDIA_VIDEO;
+	dev->class = PCI_CLASS_MULTIMEDIA_VIDEO << 8;
+	dev_info(&dev->dev, "PCI class overridden (%#08x -> %#08x)\n",
+		 class, dev->class);
 }
 DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_TI, 0xb800,
-				 PCI_CLASS_NOT_DEFINED, 0, fixup_ti816x_class);
+			      PCI_CLASS_NOT_DEFINED, 0, fixup_ti816x_class);
 
 /* Some PCIe devices do not work reliably with the claimed maximum
  * payload size supported.

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in

[PATCH 4/6] PCI: Fix Intel generic reset quirk class code check

[Bjorn Helgaas]

We checked for "dev->class == PCI_CLASS_SERIAL_USB", but dev->class
contains the entire three-byte base class/sub-class/interface, while
PCI_CLASS_SERIAL_USB is only the two-byte base class/sub-class.

This error meant that we used the Intel device-specific reset on devices
with class code 0x000c03 instead of those with class code 0x0c03xx.
0x000c03 is a reserved value in the 0x00 backwards compatibility base class
and shouldn't match any devices, so I think reset_intel_generic_dev()
always failed.

Shift dev->class to discard the interface byte before comparing with
PCI_CLASS_SERIAL_USB.

Fixes: aeb30016fec3 ("PCI: add Intel USB specific reset method")
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: Dexuan Cui <dexuan.cui@intel.com>
CC: Yu Zhao <yu.zhao@intel.com>
---
 drivers/pci/quirks.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 8bc60c2..356a401 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3358,7 +3358,7 @@ static int reset_intel_generic_dev(struct pci_dev *dev, int probe)
 	int pos;
 
 	/* only implement PCI_CLASS_SERIAL_USB at present */
-	if (dev->class == PCI_CLASS_SERIAL_USB) {
+	if ((dev->class >> 8) == PCI_CLASS_SERIAL_USB) {
 		pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
 		if (!pos)
 			return -ENOTTY;

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in

[PATCH 5/6] PCI: Simplify reset_intel_generic_dev()

[Bjorn Helgaas]

Restructure reset_intel_generic_dev() slightly to return early for non-USB
devices.  That makes the body of the function non-indented.

No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: Dexuan Cui <dexuan.cui@intel.com>
CC: Yu Zhao <yu.zhao@intel.com>
---
 drivers/pci/quirks.c |   23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 356a401..ed2763b2 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3358,21 +3358,20 @@ static int reset_intel_generic_dev(struct pci_dev *dev, int probe)
 	int pos;
 
 	/* only implement PCI_CLASS_SERIAL_USB at present */
-	if ((dev->class >> 8) == PCI_CLASS_SERIAL_USB) {
-		pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
-		if (!pos)
-			return -ENOTTY;
-
-		if (probe)
-			return 0;
+	if ((dev->class >> 8) != PCI_CLASS_SERIAL_USB)
+		return -ENOTTY;
 
-		pci_write_config_byte(dev, pos + 0x4, 1);
-		msleep(100);
+	pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
+	if (!pos)
+		return -ENOTTY;
 
+	if (probe)
 		return 0;
-	} else {
-		return -ENOTTY;
-	}
+
+	pci_write_config_byte(dev, pos + 0x4, 1);
+	msleep(100);
+
+	return 0;
 }
 
 static int reset_intel_82599_sfp_virtfn(struct pci_dev *dev, int probe)

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in

[PATCH 6/6] PCI: Shift PCI_CLASS_NOT_DEFINED consistently with other classes

[Bjorn Helgaas]

The PCI class in dev->class is a three-byte value comprising a base class,
sub-class, and interface type.  PCI_CLASS_NOT_DEFINED includes the base
class and sub-class, but not the interface type, so it should be shifted to
make space for the interface.  It happens that PCI_CLASS_NOT_DEFINED is
zero, so it doesn't matter in the end, but we should still use it
consistently with other class definitions.

Treat PCI_CLASS_NOT_DEFINED as a base class/sub-class value that should
appear in bits 8-23 of dev->class.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/probe.c  |    2 +-
 drivers/pci/quirks.c |   10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6675a7a..80e45c3 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1232,7 +1232,7 @@ int pci_setup_device(struct pci_dev *dev)
 	bad:
 		dev_err(&dev->dev, "ignoring class %#08x (doesn't match header type %02x)\n",
 			dev->class, dev->hdr_type);
-		dev->class = PCI_CLASS_NOT_DEFINED;
+		dev->class = PCI_CLASS_NOT_DEFINED << 8;
 	}
 
 	/* We found a fine healthy device, go go go... */
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index ed2763b2..dc0df55 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2852,7 +2852,7 @@ static void fixup_ti816x_class(struct pci_dev *dev)
 		 class, dev->class);
 }
 DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_TI, 0xb800,
-			      PCI_CLASS_NOT_DEFINED, 0, fixup_ti816x_class);
+			      PCI_CLASS_NOT_DEFINED, 8, fixup_ti816x_class);
 
 /* Some PCIe devices do not work reliably with the claimed maximum
  * payload size supported.
@@ -3691,13 +3691,13 @@ static void quirk_tw686x_class(struct pci_dev *pdev)
 	dev_info(&pdev->dev, "TW686x PCI class overridden (%#08x -> %#08x)\n",
 		 class, pdev->class);
 }
-DECLARE_PCI_FIXUP_CLASS_EARLY(0x1797, 0x6864, PCI_CLASS_NOT_DEFINED, 0,
+DECLARE_PCI_FIXUP_CLASS_EARLY(0x1797, 0x6864, PCI_CLASS_NOT_DEFINED, 8,
 			      quirk_tw686x_class);
-DECLARE_PCI_FIXUP_CLASS_EARLY(0x1797, 0x6865, PCI_CLASS_NOT_DEFINED, 0,
+DECLARE_PCI_FIXUP_CLASS_EARLY(0x1797, 0x6865, PCI_CLASS_NOT_DEFINED, 8,
 			      quirk_tw686x_class);
-DECLARE_PCI_FIXUP_CLASS_EARLY(0x1797, 0x6868, PCI_CLASS_NOT_DEFINED, 0,
+DECLARE_PCI_FIXUP_CLASS_EARLY(0x1797, 0x6868, PCI_CLASS_NOT_DEFINED, 8,
 			      quirk_tw686x_class);
-DECLARE_PCI_FIXUP_CLASS_EARLY(0x1797, 0x6869, PCI_CLASS_NOT_DEFINED, 0,
+DECLARE_PCI_FIXUP_CLASS_EARLY(0x1797, 0x6869, PCI_CLASS_NOT_DEFINED, 8,
 			      quirk_tw686x_class);
 
 /*

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in

Re: [PATCH 2/6] PCI: Fix generic NCR 53c810 class code quirk

[Bjorn Helgaas]

[-cc Dexuan Cui <dexuan.cui@intel.com>, matthew@wil.cx (bounced)]
[+to willy@linux.intel.com]

Might want to clean up MAINTAINERS, Matthew :)


On Fri, Jun 19, 2015 at 5:42 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> In the generic quirk fixup_rev1_53c810(), added by a5312e28c195 ("[PATCH]
> PCI: NCR 53c810 quirk"), we assigned "class = PCI_CLASS_STORAGE_SCSI".  But
> PCI_CLASS_STORAGE_SCSI is only the two-byte base class/sub-class and needs
> to be shifted to make space for the low-order interface byte.
>
> Furthermore, we had a similar quirk, pci_fixup_ncr53c810(), for arch/x86,
> which assigned class correctly.  The arch code is linked before the PCI
> core, so arch quirks run before generic quirks.  Therefore, on x86, the x86
> arch quirk ran first, and the generic quirk did nothing because it saw that
> dev->class was already set.  But on other arches, the generic quirk set the
> wrong class code.
>
> Fix the generic quirk to set the correct class code and remove the
> now-unnecessary x86-specific quirk.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: Matthew Wilcox <matthew@wil.cx>
> ---
>  arch/x86/pci/fixup.c |   13 -------------
>  drivers/pci/quirks.c |   14 +++++++++-----
>  2 files changed, 9 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index 9a2b710..e585655 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -62,19 +62,6 @@ static void pci_fixup_umc_ide(struct pci_dev *d)
>  }
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_UMC, PCI_DEVICE_ID_UMC_UM8886BF, pci_fixup_umc_ide);
>
> -static void pci_fixup_ncr53c810(struct pci_dev *d)
> -{
> -       /*
> -        * NCR 53C810 returns class code 0 (at least on some systems).
> -        * Fix class to be PCI_CLASS_STORAGE_SCSI
> -        */
> -       if (!d->class) {
> -               dev_warn(&d->dev, "Fixing NCR 53C810 class code\n");
> -               d->class = PCI_CLASS_STORAGE_SCSI << 8;
> -       }
> -}
> -DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_NCR, PCI_DEVICE_ID_NCR_53C810, pci_fixup_ncr53c810);
> -
>  static void pci_fixup_latency(struct pci_dev *d)
>  {
>         /*
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 2dab425..64177a6 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -1997,14 +1997,18 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1508, quirk_disable_aspm_l0s);
>
>  static void fixup_rev1_53c810(struct pci_dev *dev)
>  {
> -       /* rev 1 ncr53c810 chips don't set the class at all which means
> +       u32 class = dev->class;
> +
> +       /*
> +        * rev 1 ncr53c810 chips don't set the class at all which means
>          * they don't get their resources remapped. Fix that here.
>          */
> +       if (class)
> +               return;
>
> -       if (dev->class == PCI_CLASS_NOT_DEFINED) {
> -               dev_info(&dev->dev, "NCR 53c810 rev 1 detected; setting PCI class\n");
> -               dev->class = PCI_CLASS_STORAGE_SCSI;
> -       }
> +       dev->class = PCI_CLASS_STORAGE_SCSI << 8;
> +       dev_info(&dev->dev, "NCR 53c810 rev 1 PCI class overridden (%#08x -> %#08x)\n",
> +                class, dev->class);
>  }
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_NCR, PCI_DEVICE_ID_NCR_53C810, fixup_rev1_53c810);
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in

Re: [PATCH 4/6] PCI: Fix Intel generic reset quirk class code check

[Bjorn Helgaas]

[-cc Dexuan, Matthew (bounced)]
[+cc Mathias, since MAINTAINERS mentions USB, Intel, and you in one entry :)]

On Fri, Jun 19, 2015 at 5:42 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> We checked for "dev->class == PCI_CLASS_SERIAL_USB", but dev->class
> contains the entire three-byte base class/sub-class/interface, while
> PCI_CLASS_SERIAL_USB is only the two-byte base class/sub-class.
>
> This error meant that we used the Intel device-specific reset on devices
> with class code 0x000c03 instead of those with class code 0x0c03xx.
> 0x000c03 is a reserved value in the 0x00 backwards compatibility base class
> and shouldn't match any devices, so I think reset_intel_generic_dev()
> always failed.
>
> Shift dev->class to discard the interface byte before comparing with
> PCI_CLASS_SERIAL_USB.
>
> Fixes: aeb30016fec3 ("PCI: add Intel USB specific reset method")
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: Dexuan Cui <dexuan.cui@intel.com>
> CC: Yu Zhao <yu.zhao@intel.com>
> ---
>  drivers/pci/quirks.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 8bc60c2..356a401 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3358,7 +3358,7 @@ static int reset_intel_generic_dev(struct pci_dev *dev, int probe)
>         int pos;
>
>         /* only implement PCI_CLASS_SERIAL_USB at present */
> -       if (dev->class == PCI_CLASS_SERIAL_USB) {
> +       if ((dev->class >> 8) == PCI_CLASS_SERIAL_USB) {

I'm a little bit concerned about this change because it *looks* like
this has always been broken, but I assume it was tested and somebody
would have noticed.

>                 pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
>                 if (!pos)
>                         return -ENOTTY;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in

Re: [PATCH 1/6] PCI: Use PCI_CLASS_SERIAL_USB instead of bare number

[Huang Rui]

On Sat, Jun 20, 2015 at 06:42:23AM +0800, Bjorn Helgaas wrote:
> be6646bfbaec ("PCI: Prevent xHCI driver from claiming AMD Nolan USB3 DRD
> device") added a quirk to override the PCI class code of the AMD Nolan
> device.
> 
> Use PCI_CLASS_SERIAL_USB instead of a bare number to improve greppability.
> Also add a log message about what we're doing.
> 
> No functional change except the new message.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: Huang Rui <ray.huang@amd.com>
> CC: Jason Chang <jason.chang@amd.com>
> CC: Felipe Balbi <balbi@ti.com>

Looks good for me.

Acked-by: Huang Rui <ray.huang@amd.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in

Re: [PATCH 4/6] PCI: Fix Intel generic reset quirk class code check

[Bjorn Helgaas]

On Fri, Jun 19, 2015 at 6:06 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [-cc Dexuan, Matthew (bounced)]
> [+cc Mathias, since MAINTAINERS mentions USB, Intel, and you in one entry :)]
>
> On Fri, Jun 19, 2015 at 5:42 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> We checked for "dev->class == PCI_CLASS_SERIAL_USB", but dev->class
>> contains the entire three-byte base class/sub-class/interface, while
>> PCI_CLASS_SERIAL_USB is only the two-byte base class/sub-class.
>>
>> This error meant that we used the Intel device-specific reset on devices
>> with class code 0x000c03 instead of those with class code 0x0c03xx.
>> 0x000c03 is a reserved value in the 0x00 backwards compatibility base class
>> and shouldn't match any devices, so I think reset_intel_generic_dev()
>> always failed.
>>
>> Shift dev->class to discard the interface byte before comparing with
>> PCI_CLASS_SERIAL_USB.
>>
>> Fixes: aeb30016fec3 ("PCI: add Intel USB specific reset method")
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> CC: Dexuan Cui <dexuan.cui@intel.com>
>> CC: Yu Zhao <yu.zhao@intel.com>
>> ---
>>  drivers/pci/quirks.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 8bc60c2..356a401 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -3358,7 +3358,7 @@ static int reset_intel_generic_dev(struct pci_dev *dev, int probe)
>>         int pos;
>>
>>         /* only implement PCI_CLASS_SERIAL_USB at present */
>> -       if (dev->class == PCI_CLASS_SERIAL_USB) {
>> +       if ((dev->class >> 8) == PCI_CLASS_SERIAL_USB) {
>
> I'm a little bit concerned about this change because it *looks* like
> this has always been broken, but I assume it was tested and somebody
> would have noticed.

Unless we get confirmation that somebody cares about
reset_intel_generic_dev() and it has been tested, I'm inclined to
revert aeb30016fec3 ("PCI: add Intel USB specific reset method")
instead of applying this fix.

I think reset_intel_generic_dev() always returns -ENOTTY except for
devices with "class == 0x000c03", which is an invalid class code.  If
I "fix" it by putting in the shift, it will start poking at config
space on Intel USB devices.  I have no way of testing that, and I
haven't seen a problem report related to this, so a change is just as
likely to break something as to fix something.

So the safest thing looks like just dropping the quirk altogether,
which shouldn't change behavior at all.

>>                 pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
>>                 if (!pos)
>>                         return -ENOTTY;
>>