Date: Fri, 29 Dec 2023 20:30:00 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: Takashi Sakamoto <o-takashi@...amocchi.jp>,
 linux1394-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Cc: adamg@...ox.com, stable@...r.kernel.org, Jiri Slaby
 <jirislaby@...nel.org>, Tobias Gruetzmacher <tobias-lists@...gs>
Subject: Re: [PATCH] firewire: ohci: suppress unexpected system reboot in AMD
 Ryzen machines and ASM108x/VT630x PCIe cards

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@...r.kernel.org
> Reported-by: Jiri Slaby <jirislaby@...nel.org>
> Closes: https://bugzilla.suse.com/show_bug.cgi?id=1215436
> Reported-by: Mario Limonciello <mario.limonciello@....com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217994
> Reported-by: Tobias Gruetzmacher <tobias-lists@...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@...amocchi.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/

Powered by blists - more mailing lists