On Wed, May 20, 2020 at 10:31:45AM +0000, Krzysztof Wilczynski wrote:
> As per the specifications from the vendor:
>
> - "M1543 Preliminary Data Sheet", "M1543: Desktop South Bridge",
> Acer Laboratories Inc., January 1998, Version 1.25, p. 78
>
> - "M1543C Preliminary Datasheet", "M1543C Desktop Southbridge",
> Acer Laboratories Inc., September 1998, Version 1.10, p. 126
>
> Both the ACPI I/O and SMB I/O registers should be mapped into I/O space,
> but the second quirk used to claimed an I/O resource from the memory
> window which is not correct.
>
> Signed-off-by: Krzysztof Wilczynski <kw@xxxxxxxxx>
> ---
> 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 ca9ed5774eb1..c71fdd8bd6f8 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -654,7 +654,7 @@ DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
> static void quirk_ali7101_acpi(struct pci_dev *dev)
> {
> quirk_io_region(dev, 0xE0, 64, PCI_BRIDGE_RESOURCES, "ali7101 ACPI");
> - quirk_io_region(dev, 0xE2, 32, PCI_BRIDGE_RESOURCES+1, "ali7101 SMB");
> + quirk_io_region(dev, 0xE2, 32, PCI_BRIDGE_RESOURCES, "ali7101 SMB");
Wow, this is way too subtle. I proposed this fix, but I was wrong.
I thought PCI_BRIDGE_RESOURCES was identifying a bridge window that
the region should be claimed from: dev->resource[PCI_BRIDGE_RESOURCES]
would be the I/O port window of a bridge.
But that's not what's happening at all. quirk_io_region() is using
that index to fill in a dev->resource[] entry with an IORESOURCE_IO
resource, and then it calls pci_claim_resource(), which looks for an
upstream bridge window that contains the resource (in
pci_find_parent_resource()).
So the use of PCI_BRIDGE_RESOURCES here is just a convenient way to
find an empty slot in dev->resource[]. We know it's empty because
the ali7101 device is not a bridge, so it has no windows.
And PCI_BRIDGE_RESOURCES+1 *looks* like a reference to a bridge's
memory window, but it's not; it's just a way to find another empty
dev->resource[] slot.
Sigh. This is ugly and misleading. But in the absence of good ideas,
I guess we should just leave it alone.
> }
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M7101, quirk_ali7101_acpi);
>
> --
> 2.26.2
>
