Re: [PATCH V2 2/2] PCI: Add quirk for LS7A to avoid reboot failure

[Bjorn Helgaas]

On Sat, Jan 21, 2023 at 11:10:09PM +0800, Huacai Chen wrote:
> On Fri, Jan 20, 2023 at 11:36 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > On Fri, Jan 20, 2023 at 09:31:43PM +0800, Huacai Chen wrote:
> > > On Thu, Jan 19, 2023 at 8:50 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > > > On Thu, Jan 19, 2023 at 08:25:20PM +0800, Huacai Chen wrote:
> > > > > Ping?
> > > >
> > > > I suggested another possible way to do this that wasn't so much of a
> > > > special case.  Did you explore that at all?
> > >
> > > That is a little difficult for me, but what is worse is that the root
> > > cause doesn't come from gpu or console drivers, but from the root
> > > port. That means: even if we can workaround the gpu issue in another
> > > way, there are still problems on other devices. Besides the graphics
> > > card, the most frequent problematic device is the sata controller
> > > connected on LS7A chipset, there are incomplete I/O accesses after the
> > > root port disabled and also cause reboot failure.
> >
> > Yes, SATA sounds like another case where we want to use the device
> > after we call the driver's remove/shutdown method.  That's not
> > *worse*, it's just another case where we might have to mark devices
> > for special handling.
>
> That needs too much effort because we need to modify nearly every pci
> driver, and it exceeds my ability. :)

We would only modify drivers that need this special handling, so it's
only console/graphics/disks/network/..., well, OK, I see your point,
it probably *would* be nearly every driver!

> > If we remove/shutdown *any* Root Port, not just LS7A, I think the idea
> > of assuming downstream devices can continue to work as usual is a
> > little suspect.  They might continue to work by accident today, but it
> > doesn't seem like a robust design.
>
> The existing design works for so many years, so it is mostly
> reasonable. For the LS7A case, the root cause comes from the root
> port, so a workaround on the root port seems somewhat reasonable.

Yeah, I think you're right.  A few more notes below.

> > > > > On Sat, Jan 7, 2023 at 10:25 AM Huacai Chen <chenhuacai@xxxxxxxxx> wrote:
> > > > > > On Fri, Jan 6, 2023 at 11:38 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > > > > > > On Fri, Jan 06, 2023 at 05:51:43PM +0800, Huacai Chen wrote:
> > > > > > > > After cc27b735ad3a7557 ("PCI/portdrv: Turn off PCIe
> > > > > > > > services during shutdown") we observe poweroff/reboot
> > > > > > > > failures on systems with LS7A chipset.
> > > > > > > >
> > > > > > > > We found that if we remove "pci_command &=
> > > > > > > > ~PCI_COMMAND_MASTER" in do_pci_disable_device(), it can
> > > > > > > > work well. The hardware engineer says that the root cause
> > > > > > > > is that CPU is still accessing PCIe devices while
> > > > > > > > poweroff/reboot, and if we disable the Bus Master Bit at
> > > > > > > > this time, the PCIe controller doesn't forward requests to
> > > > > > > > downstream devices, and also does not send TIMEOUT to CPU,
> > > > > > > > which causes CPU wait forever (hardware deadlock).
> > > > > > > >
> > > > > > > > To be clear, the sequence is like this:
> > > > > > > >
> > > > > > > >   - CPU issues MMIO read to device below Root Port
> > > > > > > >
> > > > > > > >   - LS7A Root Port fails to forward transaction to secondary bus
> > > > > > > >     because of LS7A Bus Master defect
> > > > > > > >
> > > > > > > >   - CPU hangs waiting for response to MMIO read
> ...

> > > > > > > > +
> > > > > > > > +static void pcie_portdrv_shutdown(struct pci_dev *dev)
> > > > > > > > +{
> > > > > > > > +     struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
> > > > > > > > +
> > > > > > > > +     if (pci_bridge_d3_possible(dev)) {
> > > > > > > > +             pm_runtime_forbid(&dev->dev);
> > > > > > > > +             pm_runtime_get_noresume(&dev->dev);
> > > > > > > > +             pm_runtime_dont_use_autosuspend(&dev->dev);
> > > > > > > > +     }
> > > > > > > > +
> > > > > > > > +     pcie_port_device_remove(dev);
> > > > > > > > +
> > > > > > > > +     if (!bridge->no_dis_bmaster)
> > > > > > > > +             pci_disable_device(dev);

I think there's an argument that pcie_portdrv_shutdown() doesn't
actually need to clear bus mastering on *any* platform.

For reboot and poweroff, we only use .shutdown(), and .shutdown() only
needs to stop DMA and interrupts.  Clearing bus master enable stops
MSI/MSI-X since that's a DMA, but doesn't do anything to stop INTx,
which portdrv does use in some cases.

But those .remove() methods *do* clear the interrupt enables for each
service (PCI_ERR_ROOT_COMMAND, PCI_EXP_DPC_CTL, PCI_EXP_SLTCTL, and
PCI_EXP_RTCTL), so all the interrupts should be disabled regardless of
whether they are MSI/MSI-X or INTx, even without disabling bus
mastering.

So I would argue that omitting the pci_disable_device() here might be
enough, and we wouldn't need the quirk at all.

Bjorn