Re: [PATCH] PCI: loongson: Add quirk for OHCI device rev 0x02

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Jan 21, 2025 at 07:42:25PM +0800, Huacai Chen wrote:
> The OHCI controller (rev 0x02) under LS7A PCI host has a hardware flaw.
> MMIO register with offset 0x60/0x64 is treated as legacy PS2-compatible
> keyboard/mouse interface, which confuse the OHCI controller. Since OHCI
> only use a 4KB BAR resource indeed, the LS7A OHCI controller's 32KB BAR
> is wrapped around (the second 4KB BAR space is the same as the first 4KB
> internally). So we can add an 4KB offset (0x1000) to the BAR resource as
> a workaround.

It looks like usb_hcd_pci_probe() only uses BAR 0 in the OHCI case, so
I assume this OHCI controller has a single 32KB BAR?

And you're saying that in that BAR, the 0x1000-0x1fff offsets are
aliases of the 0x0000-0x0fff area?

And this causes some kind of problem because the OCHI driver looks at
offsets 0x60 and 0x64 into the BAR and sees something it doesn't like?

And this quirk adds 0x1000 to the BAR start, so the OHCI driver looks
at offsets 0x1060 and 0x1064 of the original BAR, and that somehow
avoids a problem?  Even though those are aliases of 0x0060 and 0x0064
of the original BAR?

> +static void loongson_ohci_quirk(struct pci_dev *dev)
> +{
> +	if (dev->revision == 0x2)
> +		dev->resource[0].start += 0x1000;

What does this do to the iomem_resource tree?  IIUC, dev->resource[0]
no longer correctly describes the PCI address space consumed by the
device.  

If the BAR is actually programmed with [mem 0x20000000-0x20007fff],
the device responds to PCI accesses in that range.  Now you update
resource[0] so it describes the space [mem 0x20001000-0x20008fff].  So
the kernel *thinks* the space at [mem 0x20000000-0x20000fff] is free
and available for something else, which is not true, and that the
device responds at [mem 0x0x20008000-0x20008fff], which is also not
true.

I think the resource has already been put into the iomem_resource tree
by the time the final fixups are run, so this also may corrupt the
sorting of the tree.

This just doesn't look safe to me.

> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, DEV_LS7A_OHCI, loongson_ohci_quirk);