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

[Myron Stowe]

On Tue, Feb 3, 2015 at 9:04 PM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> Signed-off-by: Myron Stowe <myron.stowe@xxxxxxxxxx>
>> Fixes: 36e8164882ca ("PCI: Restore detection of read-only BARs")
>> CC: stable@xxxxxxxxxxxxxxx  # 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@xxxxxxxxxx>
> 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@xxxxxxxxxxxxx>
>     Signed-off-by: Myron Stowe <myron.stowe@xxxxxxxxxx>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>     CC: stable@xxxxxxxxxxxxxxx  # 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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/