[PATCH] PCI: Expand quirk's handling of CS553x devices

[PATCH] PCI: Expand quirk's handling of CS553x devices

[Myron Stowe]

There seem to be a number of issues with CS553x devices and due to a
recent patch series that detects PCI read-only BARs [1], we've encountered
more.

It appears that not only are the BAR values associated with this device
often greater than the largest range that an IO decoder can request, they
can also be non-conformant with respect to PCI's BAR sizing aspects,
behaving instead, in a read-only manner [2].

This patch addresses read-only BAR values corresponding to CS553x devices
by expanding the existing quirk, manually inserting regions based on the
device's BIOS settings (as opposed to basing such on normal BAR sizing
actions) when necessary.

[1] https://lkml.org/lkml/2014/10/30/637
    [PATCH 0/3] PCI: Fix detection of read-only BARs
      36e8164882ca  PCI: Restore detection of read-only BARs
      f795d86aaa57  PCI: Shrink decoding-disabled window while sizing BARs
      7e79c5f8cad2  PCI: Add informational printk for invalid BARs
[2] https://bugzilla.kernel.org/show_bug.cgi?id=85991 (Comment #4 forward)
Reference: support.amd.com/TechDocs/31506_cs5535_databook.pdf

Reported-by: Nix <nix@esperi.org.uk>
Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
Fixes: 36e8164882ca ("PCI: Restore detection of read-only BARs")
CC: stable@vger.kernel.org  # v.2.6.27+
---
 drivers/pci/quirks.c |   40 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index ed6f89b..aac98c5 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -324,18 +324,52 @@ static void quirk_s3_64M(struct pci_dev *dev)
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_S3,	PCI_DEVICE_ID_S3_868,		quirk_s3_64M);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_S3,	PCI_DEVICE_ID_S3_968,		quirk_s3_64M);
 
+static void quirk_io(struct pci_dev *dev, int pos, unsigned size,
+		     const char *name)
+{
+	u32 region;
+	struct pci_bus_region bus_region;
+	struct resource *res = dev->resource + pos;
+
+	pci_read_config_dword(dev, PCI_BASE_ADDRESS_0 + (pos << 2), &region);
+
+	if (!region)
+		return;
+
+	res->name = pci_name(dev);
+	res->flags = region & ~PCI_BASE_ADDRESS_IO_MASK;
+	res->flags |=
+		(IORESOURCE_IO | IORESOURCE_PCI_FIXED | IORESOURCE_SIZEALIGN);
+	region &= ~(size - 1);
+
+	/* Convert from PCI bus to resource space */
+	bus_region.start = region;
+	bus_region.end = region + size - 1;
+	pcibios_bus_to_resource(dev->bus, res, &bus_region);
+
+	dev_info(&dev->dev, FW_BUG "%s quirk: reg 0x%x: %pR\n",
+		 name, PCI_BASE_ADDRESS_0 + (pos << 2), res);
+}
+
 /*
  * Some CS5536 BIOSes (for example, the Soekris NET5501 board w/ comBIOS
  * ver. 1.33  20070103) don't set the correct ISA PCI region header info.
  * BAR0 should be 8 bytes; instead, it may be set to something like 8k
  * (which conflicts w/ BAR1's memory range).
+ *
+ * CS553x's ISA PCI BARs may also be read-only (ref:
+ * https://bugzilla.kernel.org/show_bug.cgi?id=85991 - Comment #4 forward).
  */
 static void quirk_cs5536_vsa(struct pci_dev *dev)
 {
+	static char *name = "CS5536 ISA bridge";
+
 	if (pci_resource_len(dev, 0) != 8) {
-		struct resource *res = &dev->resource[0];
-		res->end = res->start + 8 - 1;
-		dev_info(&dev->dev, "CS5536 ISA bridge bug detected (incorrect header); workaround applied\n");
+		quirk_io(dev, 0,   8, name);
+		quirk_io(dev, 1, 256, name);
+		quirk_io(dev, 2, 512, name);
+		dev_info(&dev->dev, "%s bug detected (incorrect header); workaround applied\n",
+			 name);
 	}
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_CS5536_ISA, quirk_cs5536_vsa);

Re: [PATCH] PCI: Expand quirk's handling of CS553x devices

[Nix]

On 3 Feb 2015, Myron Stowe told this:

> There seem to be a number of issues with CS553x devices and due to a
> recent patch series that detects PCI read-only BARs [1], we've encountered
> more.
>
> It appears that not only are the BAR values associated with this device
> often greater than the largest range that an IO decoder can request, they
> can also be non-conformant with respect to PCI's BAR sizing aspects,
> behaving instead, in a read-only manner [2].
>
> This patch addresses read-only BAR values corresponding to CS553x devices
> by expanding the existing quirk, manually inserting regions based on the
> device's BIOS settings (as opposed to basing such on normal BAR sizing
> actions) when necessary.

Looks good!

[    0.270107] PCI: Probing PCI hardware
[    0.280187] PCI host bridge to bus 0000:00
[    0.290028] pci_bus 0000:00: root bus resource [io  0x0000-0xffff]
[    0.300021] pci_bus 0000:00: root bus resource [mem 0x00000000-0xffffffff]
[    0.310018] pci_bus 0000:00: No busn resource found for root bus, will use [bus 00-ff]
[    0.325514] pci 0000:00:14.0: [Firmware Bug]: CS5536 ISA bridge quirk: reg 0x10: [io  0x6000-0x6007]
[    0.330042] pci 0000:00:14.0: [Firmware Bug]: CS5536 ISA bridge quirk: reg 0x14: [io  0x6100-0x61ff]
[    0.340039] pci 0000:00:14.0: [Firmware Bug]: CS5536 ISA bridge quirk: reg 0x18: [io  0x6200-0x63ff]
[    0.350017] pci 0000:00:14.0: CS5536 ISA bridge bug detected (incorrect header); workaround applied
[    0.361456] pci 0000:00:14.2: legacy IDE quirk: reg 0x10: [io  0x01f0-0x01f7]
[    0.370019] pci 0000:00:14.2: legacy IDE quirk: reg 0x14: [io  0x03f6]
[    0.380019] pci 0000:00:14.2: legacy IDE quirk: reg 0x18: [io  0x0170-0x0177]
[    0.390017] pci 0000:00:14.2: legacy IDE quirk: reg 0x1c: [io  0x0376]
[    0.405842] pci 0000:00:0e.0: PCI bridge to [bus 01]
[    0.412043] Switched to clocksource pit
[...]
[    0.780013] cs5535-gpio cs5535-gpio: reserved resource region [io  0x6100-0x61ff]
[    0.785102] cs5535-mfgpt cs5535-mfgpt: reserved resource region [io  0x6200-0x63ff]
[    0.801002] cs5535-mfgpt cs5535-mfgpt: 8 MFGPT timers available
[    0.806684] cs5535-mfd 0000:00:14.0: 5 devices registered.
[...]
[    1.451754] cs5535-smb cs5535-smb: SCx200 device 'CS5535 ACB0' registered
[    1.452515] pc87360: Device 0x09 not activated
[    1.470755] cs5535-mfgpt cs5535-mfgpt: registered timer 0
[    1.473869] Geode LX AES 0000:00:01.2: GEODE AES engine enabled.
[    1.489999] cs5535-mfgpt cs5535-mfgpt: registered timer 1
[    1.492402] cs5535-clockevt: Registering MFGPT timer as a clock event, using IRQ 7
[...]
[    1.621402] Switched to clocksource tsc

nix@fold 3 /home/nix% grep cs5535 /proc/timer_list
Clock Event Device: cs5535-clockevt
nix@fold 4 /home/nix% ls -l /dev/watchdog
crw------- 1 root root 10, 130 Feb  4 00:14 /dev/watchdog

Thank you for such a prompt fix on hardware as obscure as this :)

-- 
NULL && (void)

Re: [PATCH] PCI: Expand quirk's handling of CS553x devices

[Bjorn Helgaas]

[+cc Andres, Leigh, Jens because they were involved in 73d2eaac8a3f
("CS5536: apply pci quirk for BIOS SMBUS bug")]

[+cc Bill, Martin, Matthew, Greg, Linus because they saw the original
report and might be interested in the resolution]

On Tue, Feb 03, 2015 at 04:01:24PM -0700, Myron Stowe wrote:
> There seem to be a number of issues with CS553x devices and due to a
> recent patch series that detects PCI read-only BARs [1], we've encountered
> more.
> 
> It appears that not only are the BAR values associated with this device
> often greater than the largest range that an IO decoder can request, they
> can also be non-conformant with respect to PCI's BAR sizing aspects,
> behaving instead, in a read-only manner [2].
> 
> This patch addresses read-only BAR values corresponding to CS553x devices
> by expanding the existing quirk, manually inserting regions based on the
> device's BIOS settings (as opposed to basing such on normal BAR sizing
> actions) when necessary.
> 
> [1] https://lkml.org/lkml/2014/10/30/637
>     [PATCH 0/3] PCI: Fix detection of read-only BARs
>       36e8164882ca  PCI: Restore detection of read-only BARs
>       f795d86aaa57  PCI: Shrink decoding-disabled window while sizing BARs
>       7e79c5f8cad2  PCI: Add informational printk for invalid BARs
> [2] https://bugzilla.kernel.org/show_bug.cgi?id=85991 (Comment #4 forward)
> Reference: support.amd.com/TechDocs/31506_cs5535_databook.pdf
> 
> Reported-by: Nix <nix@esperi.org.uk>
> Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
> Fixes: 36e8164882ca ("PCI: Restore detection of read-only BARs")
> CC: stable@vger.kernel.org  # v.2.6.27+
> ---
>  drivers/pci/quirks.c |   40 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index ed6f89b..aac98c5 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -324,18 +324,52 @@ static void quirk_s3_64M(struct pci_dev *dev)
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_S3,	PCI_DEVICE_ID_S3_868,		quirk_s3_64M);
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_S3,	PCI_DEVICE_ID_S3_968,		quirk_s3_64M);
>  
> +static void quirk_io(struct pci_dev *dev, int pos, unsigned size,
> +		     const char *name)
> +{
> +	u32 region;
> +	struct pci_bus_region bus_region;
> +	struct resource *res = dev->resource + pos;
> +
> +	pci_read_config_dword(dev, PCI_BASE_ADDRESS_0 + (pos << 2), &region);
> +
> +	if (!region)
> +		return;
> +
> +	res->name = pci_name(dev);
> +	res->flags = region & ~PCI_BASE_ADDRESS_IO_MASK;
> +	res->flags |=
> +		(IORESOURCE_IO | IORESOURCE_PCI_FIXED | IORESOURCE_SIZEALIGN);
> +	region &= ~(size - 1);
> +
> +	/* Convert from PCI bus to resource space */
> +	bus_region.start = region;
> +	bus_region.end = region + size - 1;
> +	pcibios_bus_to_resource(dev->bus, res, &bus_region);
> +
> +	dev_info(&dev->dev, FW_BUG "%s quirk: reg 0x%x: %pR\n",
> +		 name, PCI_BASE_ADDRESS_0 + (pos << 2), res);
> +}
> +
>  /*
>   * Some CS5536 BIOSes (for example, the Soekris NET5501 board w/ comBIOS
>   * ver. 1.33  20070103) don't set the correct ISA PCI region header info.
>   * BAR0 should be 8 bytes; instead, it may be set to something like 8k
>   * (which conflicts w/ BAR1's memory range).
> + *
> + * CS553x's ISA PCI BARs may also be read-only (ref:
> + * https://bugzilla.kernel.org/show_bug.cgi?id=85991 - Comment #4 forward).
>   */
>  static void quirk_cs5536_vsa(struct pci_dev *dev)
>  {
> +	static char *name = "CS5536 ISA bridge";
> +
>  	if (pci_resource_len(dev, 0) != 8) {
> -		struct resource *res = &dev->resource[0];
> -		res->end = res->start + 8 - 1;
> -		dev_info(&dev->dev, "CS5536 ISA bridge bug detected (incorrect header); workaround applied\n");
> +		quirk_io(dev, 0,   8, name);
> +		quirk_io(dev, 1, 256, name);
> +		quirk_io(dev, 2, 512, name);

Per sec 5.6.1 of the datasheets, I think BAR 2 (MFGPT) is only 64 bytes.

On Nix's system, we detected it as 512 bytes prior to 36e8164882ca.  That
was because the BAR contained 0x6200, and the lowest-order set bit
determines the BAR size, so it was 512 in that case.  So forcing it to be
512 certainly works on Nix's system (though it may consume unnecessary
space after the BAR).

But this quirk ONLY works if the system makes that BAR 512-byte aligned.
If the BAR is at an address that is only aligned to 64 bytes, not 512, this
quirk will forcibly align the start to 512.  For example, if we had:

  pci 0000:00:14.0: reg 0x18: [io  0x6240-0x627f]  (a read-only BAR)

this quirk would read 0x6240 from the BAR and align it to 0x6200 (the
"region &= ~(size - 1)" part) so we end up with [io 0x6200-0x63ff].  I
don't think that will work.

I tweaked the patch (as below) and applied to my for-linus branch for
v3.19.  I haven't asked Linus to pull it yet, so let me know if we still
need to tweak it some more.

Bjorn

> +		dev_info(&dev->dev, "%s bug detected (incorrect header); workaround applied\n",
> +			 name);
>  	}
>  }
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_CS5536_ISA, quirk_cs5536_vsa);
> 


commit f13ad4b2718e5900c1b8a8eeb500860145a5991f
Author: Myron Stowe <myron.stowe@redhat.com>
Date:   Tue Feb 3 16:01:24 2015 -0700

    PCI: Handle read-only BARs on AMD CS553x devices
    
    Some AMD CS553x devices have read-only BARs because of a firmware or
    hardware defect.  There's a workaround in quirk_cs5536_vsa(), but it no
    longer works after 36e8164882ca ("PCI: Restore detection of read-only
    BARs").  Prior to 36e8164882ca, we filled in res->start; afterwards we
    leave it zeroed out.  The quirk only updated the size, so the driver tried
    to use a region starting at zero, which didn't work.
    
    Expand quirk_cs5536_vsa() to read the base addresses from the BARs and
    hard-code the sizes.
    
    Prior to 36e8164882ca, BAR 2 was detected as 512 bytes based on a read-only
    BAR value of 0x6200.  The lowest-order set bit determines the largest
    possible BAR size: 512 in this case.  Per sec 5.6.1 of the datasheets, I
    think BAR 2 (MFGPT) requires only 64 bytes, so set the resource to that.
    If a platform puts this BAR at only 64-byte alignment, we don't want to
    align the address to 512 bytes by throwing away those low-order bits.
    
    [bhelgaas: changelog, reduce BAR 2 size to 64]
    Fixes: 36e8164882ca ("PCI: Restore detection of read-only BARs")
    Link: https://bugzilla.kernel.org/show_bug.cgi?id=85991#c4
    Link: http://support.amd.com/TechDocs/31506_cs5535_databook.pdf
    Link: http://support.amd.com/TechDocs/33238G_cs5536_db.pdf
    Reported-and-tested-by: Nix <nix@esperi.org.uk>
    Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    CC: stable@vger.kernel.org	# v.2.6.27+

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index e52356aa09b8..903d5078b5ed 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -324,18 +324,52 @@ static void quirk_s3_64M(struct pci_dev *dev)
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_S3,	PCI_DEVICE_ID_S3_868,		quirk_s3_64M);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_S3,	PCI_DEVICE_ID_S3_968,		quirk_s3_64M);
 
+static void quirk_io(struct pci_dev *dev, int pos, unsigned size,
+		     const char *name)
+{
+	u32 region;
+	struct pci_bus_region bus_region;
+	struct resource *res = dev->resource + pos;
+
+	pci_read_config_dword(dev, PCI_BASE_ADDRESS_0 + (pos << 2), &region);
+
+	if (!region)
+		return;
+
+	res->name = pci_name(dev);
+	res->flags = region & ~PCI_BASE_ADDRESS_IO_MASK;
+	res->flags |=
+		(IORESOURCE_IO | IORESOURCE_PCI_FIXED | IORESOURCE_SIZEALIGN);
+	region &= ~(size - 1);
+
+	/* Convert from PCI bus to resource space */
+	bus_region.start = region;
+	bus_region.end = region + size - 1;
+	pcibios_bus_to_resource(dev->bus, res, &bus_region);
+
+	dev_info(&dev->dev, FW_BUG "%s quirk: reg 0x%x: %pR\n",
+		 name, PCI_BASE_ADDRESS_0 + (pos << 2), res);
+}
+
 /*
  * Some CS5536 BIOSes (for example, the Soekris NET5501 board w/ comBIOS
  * ver. 1.33  20070103) don't set the correct ISA PCI region header info.
  * BAR0 should be 8 bytes; instead, it may be set to something like 8k
  * (which conflicts w/ BAR1's memory range).
+ *
+ * CS553x's ISA PCI BARs may also be read-only (ref:
+ * https://bugzilla.kernel.org/show_bug.cgi?id=85991 - Comment #4 forward).
  */
 static void quirk_cs5536_vsa(struct pci_dev *dev)
 {
+	static char *name = "CS5536 ISA bridge";
+
 	if (pci_resource_len(dev, 0) != 8) {
-		struct resource *res = &dev->resource[0];
-		res->end = res->start + 8 - 1;
-		dev_info(&dev->dev, "CS5536 ISA bridge bug detected (incorrect header); workaround applied\n");
+		quirk_io(dev, 0,   8, name);	/* SMB */
+		quirk_io(dev, 1, 256, name);	/* GPIO */
+		quirk_io(dev, 2,  64, name);	/* MFGPT */
+		dev_info(&dev->dev, "%s bug detected (incorrect header); workaround applied\n",
+			 name);
 	}
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_CS5536_ISA, quirk_cs5536_vsa);

Re: [PATCH] PCI: Expand quirk's handling of CS553x devices

[Myron Stowe]

On Tue, Feb 3, 2015 at 9:04 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Andres, Leigh, Jens because they were involved in 73d2eaac8a3f
> ("CS5536: apply pci quirk for BIOS SMBUS bug")]
>
> [+cc Bill, Martin, Matthew, Greg, Linus because they saw the original
> report and might be interested in the resolution]
>
> On Tue, Feb 03, 2015 at 04:01:24PM -0700, Myron Stowe wrote:
>> There seem to be a number of issues with CS553x devices and due to a
>> recent patch series that detects PCI read-only BARs [1], we've encountered
>> more.
>>
>> It appears that not only are the BAR values associated with this device
>> often greater than the largest range that an IO decoder can request, they
>> can also be non-conformant with respect to PCI's BAR sizing aspects,
>> behaving instead, in a read-only manner [2].
>>
>> This patch addresses read-only BAR values corresponding to CS553x devices
>> by expanding the existing quirk, manually inserting regions based on the
>> device's BIOS settings (as opposed to basing such on normal BAR sizing
>> actions) when necessary.
>>
>> [1] https://lkml.org/lkml/2014/10/30/637
>>     [PATCH 0/3] PCI: Fix detection of read-only BARs
>>       36e8164882ca  PCI: Restore detection of read-only BARs
>>       f795d86aaa57  PCI: Shrink decoding-disabled window while sizing BARs
>>       7e79c5f8cad2  PCI: Add informational printk for invalid BARs
>> [2] https://bugzilla.kernel.org/show_bug.cgi?id=85991 (Comment #4 forward)
>> Reference: support.amd.com/TechDocs/31506_cs5535_databook.pdf
>>
>> Reported-by: Nix <nix@esperi.org.uk>
>> Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
>> Fixes: 36e8164882ca ("PCI: Restore detection of read-only BARs")
>> CC: stable@vger.kernel.org  # v.2.6.27+
>> ---
>>  drivers/pci/quirks.c |   40 +++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 37 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index ed6f89b..aac98c5 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -324,18 +324,52 @@ static void quirk_s3_64M(struct pci_dev *dev)
>>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_S3,   PCI_DEVICE_ID_S3_868,           quirk_s3_64M);
>>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_S3,   PCI_DEVICE_ID_S3_968,           quirk_s3_64M);
>>
>> +static void quirk_io(struct pci_dev *dev, int pos, unsigned size,
>> +                  const char *name)
>> +{
>> +     u32 region;
>> +     struct pci_bus_region bus_region;
>> +     struct resource *res = dev->resource + pos;
>> +
>> +     pci_read_config_dword(dev, PCI_BASE_ADDRESS_0 + (pos << 2), &region);
>> +
>> +     if (!region)
>> +             return;
>> +
>> +     res->name = pci_name(dev);
>> +     res->flags = region & ~PCI_BASE_ADDRESS_IO_MASK;
>> +     res->flags |=
>> +             (IORESOURCE_IO | IORESOURCE_PCI_FIXED | IORESOURCE_SIZEALIGN);
>> +     region &= ~(size - 1);
>> +
>> +     /* Convert from PCI bus to resource space */
>> +     bus_region.start = region;
>> +     bus_region.end = region + size - 1;
>> +     pcibios_bus_to_resource(dev->bus, res, &bus_region);
>> +
>> +     dev_info(&dev->dev, FW_BUG "%s quirk: reg 0x%x: %pR\n",
>> +              name, PCI_BASE_ADDRESS_0 + (pos << 2), res);
>> +}
>> +
>>  /*
>>   * Some CS5536 BIOSes (for example, the Soekris NET5501 board w/ comBIOS
>>   * ver. 1.33  20070103) don't set the correct ISA PCI region header info.
>>   * BAR0 should be 8 bytes; instead, it may be set to something like 8k
>>   * (which conflicts w/ BAR1's memory range).
>> + *
>> + * CS553x's ISA PCI BARs may also be read-only (ref:
>> + * https://bugzilla.kernel.org/show_bug.cgi?id=85991 - Comment #4 forward).
>>   */
>>  static void quirk_cs5536_vsa(struct pci_dev *dev)
>>  {
>> +     static char *name = "CS5536 ISA bridge";
>> +
>>       if (pci_resource_len(dev, 0) != 8) {
>> -             struct resource *res = &dev->resource[0];
>> -             res->end = res->start + 8 - 1;
>> -             dev_info(&dev->dev, "CS5536 ISA bridge bug detected (incorrect header); workaround applied\n");
>> +             quirk_io(dev, 0,   8, name);
>> +             quirk_io(dev, 1, 256, name);
>> +             quirk_io(dev, 2, 512, name);
>
> Per sec 5.6.1 of the datasheets, I think BAR 2 (MFGPT) is only 64 bytes.
>
> On Nix's system, we detected it as 512 bytes prior to 36e8164882ca.  That
> was because the BAR contained 0x6200, and the lowest-order set bit
> determines the BAR size, so it was 512 in that case.  So forcing it to be
> 512 certainly works on Nix's system (though it may consume unnecessary
> space after the BAR).
>
> But this quirk ONLY works if the system makes that BAR 512-byte aligned.
> If the BAR is at an address that is only aligned to 64 bytes, not 512, this
> quirk will forcibly align the start to 512.  For example, if we had:
>
>   pci 0000:00:14.0: reg 0x18: [io  0x6240-0x627f]  (a read-only BAR)
>
> this quirk would read 0x6240 from the BAR and align it to 0x6200 (the
> "region &= ~(size - 1)" part) so we end up with [io 0x6200-0x63ff].  I
> don't think that will work.

Agreed, we had talked about the current sizes being out-of-range (too large) for
two of the BARs.  I was trying to be safe and not screw up more than I
had already
by sticking with the existing, known working, values.  I hadn't
thought through the
ramifications going forward with respect to alignment however.  Nice catch!

>
> I tweaked the patch (as below) and applied to my for-linus branch for
> v3.19.  I haven't asked Linus to pull it yet, so let me know if we still
> need to tweak it some more.
>
> Bjorn
>
>> +             dev_info(&dev->dev, "%s bug detected (incorrect header); workaround applied\n",
>> +                      name);
>>       }
>>  }
>>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_CS5536_ISA, quirk_cs5536_vsa);
>>
>
>
> commit f13ad4b2718e5900c1b8a8eeb500860145a5991f
> Author: Myron Stowe <myron.stowe@redhat.com>
> Date:   Tue Feb 3 16:01:24 2015 -0700
>
>     PCI: Handle read-only BARs on AMD CS553x devices
>
>     Some AMD CS553x devices have read-only BARs because of a firmware or
>     hardware defect.  There's a workaround in quirk_cs5536_vsa(), but it no
>     longer works after 36e8164882ca ("PCI: Restore detection of read-only
>     BARs").  Prior to 36e8164882ca, we filled in res->start; afterwards we
>     leave it zeroed out.  The quirk only updated the size, so the driver tried
>     to use a region starting at zero, which didn't work.
>
>     Expand quirk_cs5536_vsa() to read the base addresses from the BARs and
>     hard-code the sizes.
>
>     Prior to 36e8164882ca, BAR 2 was detected as 512 bytes based on a read-only
>     BAR value of 0x6200.  The lowest-order set bit determines the largest
>     possible BAR size: 512 in this case.  Per sec 5.6.1 of the datasheets, I
>     think BAR 2 (MFGPT) requires only 64 bytes, so set the resource to that.
>     If a platform puts this BAR at only 64-byte alignment, we don't want to
>     align the address to 512 bytes by throwing away those low-order bits.
>
>     [bhelgaas: changelog, reduce BAR 2 size to 64]
>     Fixes: 36e8164882ca ("PCI: Restore detection of read-only BARs")
>     Link: https://bugzilla.kernel.org/show_bug.cgi?id=85991#c4
>     Link: http://support.amd.com/TechDocs/31506_cs5535_databook.pdf
>     Link: http://support.amd.com/TechDocs/33238G_cs5536_db.pdf
>     Reported-and-tested-by: Nix <nix@esperi.org.uk>
>     Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>     CC: stable@vger.kernel.org  # v.2.6.27+
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index e52356aa09b8..903d5078b5ed 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -324,18 +324,52 @@ static void quirk_s3_64M(struct pci_dev *dev)
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_S3,     PCI_DEVICE_ID_S3_868,           quirk_s3_64M);
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_S3,     PCI_DEVICE_ID_S3_968,           quirk_s3_64M);
>
> +static void quirk_io(struct pci_dev *dev, int pos, unsigned size,
> +                    const char *name)
> +{
> +       u32 region;
> +       struct pci_bus_region bus_region;
> +       struct resource *res = dev->resource + pos;
> +
> +       pci_read_config_dword(dev, PCI_BASE_ADDRESS_0 + (pos << 2), &region);
> +
> +       if (!region)
> +               return;
> +
> +       res->name = pci_name(dev);
> +       res->flags = region & ~PCI_BASE_ADDRESS_IO_MASK;
> +       res->flags |=
> +               (IORESOURCE_IO | IORESOURCE_PCI_FIXED | IORESOURCE_SIZEALIGN);
> +       region &= ~(size - 1);
> +
> +       /* Convert from PCI bus to resource space */
> +       bus_region.start = region;
> +       bus_region.end = region + size - 1;
> +       pcibios_bus_to_resource(dev->bus, res, &bus_region);
> +
> +       dev_info(&dev->dev, FW_BUG "%s quirk: reg 0x%x: %pR\n",
> +                name, PCI_BASE_ADDRESS_0 + (pos << 2), res);
> +}
> +
>  /*
>   * Some CS5536 BIOSes (for example, the Soekris NET5501 board w/ comBIOS
>   * ver. 1.33  20070103) don't set the correct ISA PCI region header info.
>   * BAR0 should be 8 bytes; instead, it may be set to something like 8k
>   * (which conflicts w/ BAR1's memory range).
> + *
> + * CS553x's ISA PCI BARs may also be read-only (ref:
> + * https://bugzilla.kernel.org/show_bug.cgi?id=85991 - Comment #4 forward).
>   */
>  static void quirk_cs5536_vsa(struct pci_dev *dev)
>  {
> +       static char *name = "CS5536 ISA bridge";
> +
>         if (pci_resource_len(dev, 0) != 8) {
> -               struct resource *res = &dev->resource[0];
> -               res->end = res->start + 8 - 1;
> -               dev_info(&dev->dev, "CS5536 ISA bridge bug detected (incorrect header); workaround applied\n");
> +               quirk_io(dev, 0,   8, name);    /* SMB */
> +               quirk_io(dev, 1, 256, name);    /* GPIO */
> +               quirk_io(dev, 2,  64, name);    /* MFGPT */
> +               dev_info(&dev->dev, "%s bug detected (incorrect header); workaround applied\n",
> +                        name);
>         }
>  }
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_CS5536_ISA, quirk_cs5536_vsa);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html