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

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Greg, USB maintainer; I'm proposing ohci_pci_quirks for this]

On Tue, Mar 18, 2025 at 09:07:10PM +0800, Huacai Chen wrote:
> On Sat, Mar 15, 2025 at 3:59 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > 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?
>
> Yes.
> 
> > And you're saying that in that BAR, the 0x1000-0x1fff offsets are
> > aliases of the 0x0000-0x0fff area?
>
> Yes.
> 
> > 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?
>
> Yes.
> 
> > 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?
>
> Yes.
> 
> > > +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.
>
> In the iomem_resource tree the whole 32KB is requested for the OHCI
> controller.

I'm skeptical.  The quirk only updates the start, not the end, so I
think the resource is only 28KB after the quirk.

This is a final quirk, so I think the quirk runs after the BAR
resource has already been put in the iomem_resource tree.  We
shouldn't update the resource start/end after that point.

> > 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.
>
> OHCI only uses 4KB io resources (hardware designer told me that this
> is from USB specification). So if the BAR is [mem
> 0x20000000-0x20007fff] and without this quirk, software will only
> use [mem 0x20000000-0x20000fff]; and if we do this quirk, software
> will only use [mem 0x20001000-0x20001fff]. Since the whole 32KB
> belongs to OHCI, there won't be corruption.

Here's the path where I think the BAR is requested.  Assume the BAR
contains 0x20000000 when we boot:

  acpi_pci_root_add
    pci_acpi_scan_root                  # loongson
      acpi_pci_root_create
        pci_scan_child_bus
          pci_scan_single_device        # details trimmed
            pci_scan_device
              pci_read_bases
                resource[0].start = 0x20000000
                resource[0].end   = 0x20007fff          # size 32KB
            pci_device_add
              pci_fixup_device(pci_fixup_header)        # <--
      pci_bus_assign_resources          # details trimmed
        pci_assign_resource
          pci_bus_alloc_resource
            allocate_resource
              __request_resource        # inserted in iomem_resource
    pci_bus_add_devices
      pci_bus_add_device
        pci_fixup_device(pci_fixup_final)
          resource[0].start += 0x1000                   # now 0x20001000

  ohci_pci_probe
    usb_hcd_pci_probe(& ohci_pci_hc_driver)
      if (driver->flags & HCD_MEMORY)
        hcd->rsrc_start = pci_resource_start(dev, 0)    # 0x20001000
        hcd->rsrc_len = pci_resource_len(dev, 0)        # now 28KB
        devm_request_mem_region
        hcd->regs = devm_ioremap
      usb_add_hcd
        hcd->driver->reset
          ohci_pci_reset                                # <--

So I suspect:

  - BAR contains 0x20000000 initially (for example)

  - pci_bus_assign_resources() inserts 0x20000000-0x20007fff in
    iomem_resource

  - final quirk updates res->start, so resource is now
    0x20001000-0x20007fff

  - OHCI driver binds and sees 0x20001000-0x20007fff

  - OHCI driver requests 0x20001000-0x20007fff (28KB)

  - OHCI may only use 4KB, but AFAICS it actually reserves the entire
    resource[0], which is now only 28KB

  - BAR still contains 0x20000000 ("lspci -b" should confirm this)

  - /proc/iomem contains something like this:

      20001000-20007fff 0000:3e:00.0
        20001000-20007fff ohci-hcd

Now the device still responds to accesses at 0x20000000 because that's
what's in the BAR, but the kernel doesn't know that anybody is using
the 0x20000000-0x20000fff region.

I think the best solution would be to add an entry in
ohci_pci_quirks[] that bumps hcd->regs up by 0x1000.  Then we wouldn't
have to touch the struct resource, it would stay 32KB, and it would
accurately reflect the hardware.

If that doesn't work, I guess you could also make this a *header*
fixup instead of a final one.  Then we would at least update the
resource before inserting it in iomem_resource.  But I don't like it
because the device still responds to an area not mentioned in
iomem_resource, and the resource is now 28KB, which is an illegal size
for a PCI BAR.

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