On 21.01.23 16:10, 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. :) Just wondering: what's the status here? This looks stalled. I'm asking, as the patches in this thread are supposed to fix this regression: https://bugzilla.kernel.org/show_bug.cgi?id=216884 Or should we try to find a different fix/workaround because the proper solution discussed in this thread needs more time? Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr If I did something stupid, please tell me, as explained on that page. #regzbot poke >> 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. > > Huacai >> >>>> I know there's no *existing* way to mark devices that we need to use >>>> all the way through shutdown or reboot, but if it makes sense, there's >>>> no reason we couldn't add one. That has the potential of being more >>>> generic, e.g., we could do it for all console devices, as opposed to >>>> quirking a Root Port that just happens to be in the path to the >>>> console. >>>> >>>>> 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 >>>>>>>> >>>>>>>> Then how is userspace able to use a device after the >>>>>>>> device is removed? >>>>>>>> >>>>>>>> To give more details, let's take the graphics driver (e.g. >>>>>>>> amdgpu) as an example. The userspace programs call >>>>>>>> printf() to display "shutting down xxx service" during >>>>>>>> shutdown/reboot, or the kernel calls printk() to display >>>>>>>> something during shutdown/reboot. These can happen at any >>>>>>>> time, even after we call pcie_port_device_remove() to >>>>>>>> disable the pcie port on the graphic card. >>>>>>>> >>>>>>>> The call stack is: printk() --> call_console_drivers() --> >>>>>>>> con->write() --> vt_console_print() --> fbcon_putcs() >>>>>>>> >>>>>>>> This scenario happens because userspace programs (or the >>>>>>>> kernel itself) don't know whether a device is 'usable', >>>>>>>> they just use it, at any time. >>>>>>> >>>>>>> Thanks for this background. So basically we want to call >>>>>>> .remove() on a console device (or a bridge leading to it), >>>>>>> but we expect it to keep working as usual afterwards? >>>>>>> >>>>>>> That seems a little weird. Is that the design we want? >>>>>>> Maybe we should have a way to mark devices so we don't >>>>>>> remove them during shutdown or reboot? >>>>>> >>>>>> Sounds reasonable, but it seems no existing way can mark this. >>>>>> >>>>>> Huacai >>>>>>> >>>>>>>> This hardware behavior is a PCIe protocol violation (Bus Master should >>>>>>>> not be involved in CPU MMIO transactions), and it will be fixed in new >>>>>>>> revisions of hardware (add timeout mechanism for CPU read request, >>>>>>>> whether or not Bus Master bit is cleared). >>>>>>>> >>>>>>>> On some x86 platforms, radeon/amdgpu devices can cause similar problems >>>>>>>> [1][2]. Once before I wanted to make a single patch to solve "all of >>>>>>>> these problems" together, but it seems unreasonable because maybe they >>>>>>>> are not exactly the same problem. So, this patch add a new function >>>>>>>> pcie_portdrv_shutdown(), a slight modified copy of pcie_portdrv_remove() >>>>>>>> dedicated for the shutdown path, and then add a quirk just for LS7A to >>>>>>>> avoid clearing Bus Master bit in pcie_portdrv_shutdown(). Leave other >>>>>>>> platforms behave as before. >>>>>>>> >>>>>>>> [1] https://bugs.freedesktop.org/show_bug.cgi?id=97980 >>>>>>>> [2] https://bugs.freedesktop.org/show_bug.cgi?id=98638 >>>>>>>> >>>>>>>> Signed-off-by: Huacai Chen <chenhuacai@xxxxxxxxxxx> >>>>>>>> --- >>>>>>>> drivers/pci/controller/pci-loongson.c | 17 +++++++++++++++++ >>>>>>>> drivers/pci/pcie/portdrv.c | 21 +++++++++++++++++++-- >>>>>>>> include/linux/pci.h | 1 + >>>>>>>> 3 files changed, 37 insertions(+), 2 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/pci/controller/pci-loongson.c b/drivers/pci/controller/pci-loongson.c >>>>>>>> index 759ec211c17b..641308ba4126 100644 >>>>>>>> --- a/drivers/pci/controller/pci-loongson.c >>>>>>>> +++ b/drivers/pci/controller/pci-loongson.c >>>>>>>> @@ -93,6 +93,24 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON, >>>>>>>> DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON, >>>>>>>> DEV_PCIE_PORT_2, loongson_mrrs_quirk); >>>>>>>> >>>>>>>> +static void loongson_bmaster_quirk(struct pci_dev *pdev) >>>>>>>> +{ >>>>>>>> + /* >>>>>>>> + * Some Loongson PCIe ports will cause CPU deadlock if there is >>>>>>>> + * MMIO access to a downstream device when the root port disable >>>>>>>> + * the Bus Master bit during poweroff/reboot. >>>>>>>> + */ >>>>>>>> + struct pci_host_bridge *bridge = pci_find_host_bridge(pdev->bus); >>>>>>>> + >>>>>>>> + bridge->no_dis_bmaster = 1; >>>>>>>> +} >>>>>>>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON, >>>>>>>> + DEV_PCIE_PORT_0, loongson_bmaster_quirk); >>>>>>>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON, >>>>>>>> + DEV_PCIE_PORT_1, loongson_bmaster_quirk); >>>>>>>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON, >>>>>>>> + DEV_PCIE_PORT_2, loongson_bmaster_quirk); >>>>>>>> + >>>>>>>> static void loongson_pci_pin_quirk(struct pci_dev *pdev) >>>>>>>> { >>>>>>>> pdev->pin = 1 + (PCI_FUNC(pdev->devfn) & 3); >>>>>>>> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c >>>>>>>> index 2cc2e60bcb39..96f45c444422 100644 >>>>>>>> --- a/drivers/pci/pcie/portdrv.c >>>>>>>> +++ b/drivers/pci/pcie/portdrv.c >>>>>>>> @@ -501,7 +501,6 @@ static void pcie_port_device_remove(struct pci_dev *dev) >>>>>>>> { >>>>>>>> device_for_each_child(&dev->dev, NULL, remove_iter); >>>>>>>> pci_free_irq_vectors(dev); >>>>>>>> - pci_disable_device(dev); >>>>>>>> } >>>>>>>> >>>>>>>> /** >>>>>>>> @@ -727,6 +726,24 @@ static void pcie_portdrv_remove(struct pci_dev *dev) >>>>>>>> } >>>>>>>> >>>>>>>> pcie_port_device_remove(dev); >>>>>>>> + >>>>>>>> + pci_disable_device(dev); >>>>>>>> +} >>>>>>>> + >>>>>>>> +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); >>>>>>>> } >>>>>>>> >>>>>>>> static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev, >>>>>>>> @@ -777,7 +794,7 @@ static struct pci_driver pcie_portdriver = { >>>>>>>> >>>>>>>> .probe = pcie_portdrv_probe, >>>>>>>> .remove = pcie_portdrv_remove, >>>>>>>> - .shutdown = pcie_portdrv_remove, >>>>>>>> + .shutdown = pcie_portdrv_shutdown, >>>>>>>> >>>>>>>> .err_handler = &pcie_portdrv_err_handler, >>>>>>>> >>>>>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h >>>>>>>> index 3df2049ec4a8..a64dbcb89231 100644 >>>>>>>> --- a/include/linux/pci.h >>>>>>>> +++ b/include/linux/pci.h >>>>>>>> @@ -573,6 +573,7 @@ struct pci_host_bridge { >>>>>>>> unsigned int ignore_reset_delay:1; /* For entire hierarchy */ >>>>>>>> unsigned int no_ext_tags:1; /* No Extended Tags */ >>>>>>>> unsigned int no_inc_mrrs:1; /* No Increase MRRS */ >>>>>>>> + unsigned int no_dis_bmaster:1; /* No Disable Bus Master */ >>>>>>>> unsigned int native_aer:1; /* OS may use PCIe AER */ >>>>>>>> unsigned int native_pcie_hotplug:1; /* OS may use PCIe hotplug */ >>>>>>>> unsigned int native_shpc_hotplug:1; /* OS may use SHPC hotplug */ >>>>>>>> -- >>>>>>>> 2.31.1 >>>>>>>>
