Re: [PATCH] firewire: ohci: suppress unexpected system reboot in AMD Ryzen machines and ASM108x/VT630x PCIe cards

Mario Limonciello
2023-12-30T02:30:06Z

On 12/28/2023 21:57, Takashi Sakamoto wrote:
> VIA VT6306/6307/6308 provides PCI interface compliant to 1394 OHCI. When
> the hardware is combined with Asmedia ASM1083/1085 PCIe-to-PCI bus bridge,
> it appears that accesses to its 'Isochronous Cycle Timer' register (offset
> 0xf0 on PCI I/O space) often causes unexpected system reboot in any type
> of AMD Ryzen machine (both 0x17 and 0x19 families). It does not appears in
> the other type of machine (AMD pre-Ryzen machine, Intel machine, at least),
> or in the other OHCI 1394 hardware (e.g. Texas Instruments).
>
> The issue explicitly appears at a commit dcadfd7f7c74 ("firewire: core:
> use union for callback of transaction completion") added to v6.5 kernel.
> It changed 1394 OHCI driver to access to the register every time to
> dispatch local asynchronous transaction. However, the issue exists in
> older version of kernel as long as it runs in AMD Ryzen machine, since
> the access to the register is required to maintain bus time. It is not
> hard to imagine that users experience the unexpected system reboot when
> generating bus reset by plugging any devices in, or reading the register
> by time-aware application programs; e.g. audio sample processing.
>
> Well, this commit suppresses the system reboot in the combination of
> hardware. It avoids the access itself. As a result, the software stack can
> not provide the hardware time anymore to unit drivers, userspace
> applications, and nodes in the same IEEE 1394 bus. It brings apparent
> disadvantage since time-aware application programs require it, while
> time-unaware applications are available again; e.g. sbp2.
>
> Cc: stable@vger.kernel.org
> Reported-by: Jiri Slaby <jirislaby@kernel.org>
> Closes: https://bugzilla.suse.com/show_bug.cgi?id=1215436
> Reported-by: Mario Limonciello <mario.limonciello@amd.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217994
> Reported-by: Tobias Gruetzmacher <tobias-lists@23.gs>
> Closes: https://sourceforge.net/p/linux1394/mailman/message/58711901/
> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2240973
> Closes: https://bugs.launchpad.net/linux/+bug/2043905
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
> drivers/firewire/ohci.c | 49 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
>
> diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
> index 7e88fd489741..62af3fa39a70 100644
> --- a/drivers/firewire/ohci.c
> +++ b/drivers/firewire/ohci.c
> @@ -279,6 +279,8 @@ static char ohci_driver_name[] = KBUILD_MODNAME;
> #define QUIRK_TI_SLLZ059 0x20
> #define QUIRK_IR_WAKE 0x40
>
> +#define QUIRK_REBOOT_BY_CYCLE_TIMER_READ 0x80000000
> +
> /* In case of multiple matches in ohci_quirks[], only the first one is used. */
> static const struct {
> unsigned short vendor, device, revision, flags;
> @@ -1724,6 +1726,11 @@ static u32 get_cycle_time(struct fw_ohci *ohci)
> s32 diff01, diff12;
> int i;
>
> +#if IS_ENABLED(CONFIG_X86)
> + if (ohci->quirks & QUIRK_REBOOT_BY_CYCLE_TIMER_READ)
> + return 0;
> +#endif
> +
> c2 = reg_read(ohci, OHCI1394_IsochronousCycleTimer);
>
> if (ohci->quirks & QUIRK_CYCLE_TIMER) {
> @@ -3527,6 +3534,45 @@ static const struct fw_card_driver ohci_driver = {
> .stop_iso = ohci_stop_iso,
> };
>
> +// On PCI Express Root Complex in any type of AMD Ryzen machine, VIA VT6306/6307/6308 with Asmedia
> +// ASM1083/1085 brings an inconvenience that read accesses to 'Isochronous Cycle Timer' register
> +// (at offset 0xf0 in PCI I/O space) often causes unexpected system reboot. The mechanism is not
> +// clear, since the read access to the other registers is enough safe; e.g. 'Node ID' register,
> +// while it is probable due to detection of any type of PCIe error.
> +#if IS_ENABLED(CONFIG_X86)
> +
> +#define PCI_DEVICE_ID_ASMEDIA_ASM108X 0x1080
> +
> +static bool detect_vt630x_with_asm1083_on_amd_ryzen_machine(const struct pci_dev *pdev,
> + struct fw_ohci *ohci)
> +{
> + const struct pci_dev *pcie_to_pci_bridge;
> + const struct cpuinfo_x86 *cinfo = &cpu_data(0);
> +
> + // Detect any type of AMD Ryzen machine.
> + if (cinfo->x86_vendor != X86_VENDOR_AMD || cinfo->x86 < 0x17)
> + return false;

Maybe it's better to use X86_FEATURE_ZEN?

> +
> + // Detect VIA VT6306/6307/6308.
> + if (pdev->vendor != PCI_VENDOR_ID_VIA)
> + return false;
> + if (pdev->device != PCI_DEVICE_ID_VIA_VT630X)
> + return false;
> +
> + // Detect Asmedia ASM1083/1085.
> + pcie_to_pci_bridge = pdev->bus->self;
> + if (pcie_to_pci_bridge->vendor != PCI_VENDOR_ID_ASMEDIA)
> + return false;
> + if (pcie_to_pci_bridge->device != PCI_DEVICE_ID_ASMEDIA_ASM108X)
> + return false;
> +
> + return true;
> +}
> +
> +#else
> +#define detect_vt630x_with_asm1083_on_amd_ryzen_machine(pdev) false
> +#endif
> +
> #ifdef CONFIG_PPC_PMAC
> static void pmac_ohci_on(struct pci_dev *dev)
> {
> @@ -3630,6 +3676,9 @@ static int pci_probe(struct pci_dev *dev,
> if (param_quirks)
> ohci->quirks = param_quirks;
>
> + if (detect_vt630x_with_asm1083_on_amd_ryzen_machine(dev, ohci))
> + ohci->quirks |= QUIRK_REBOOT_BY_CYCLE_TIMER_READ;
> +
> /*
> * Because dma_alloc_coherent() allocates at least one page,
> * we save space by using a common buffer for the AR request/