Received-SPF: pass (google.com: domain of linux-kernel+bounces-13310-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <af04e964-28b9-4c31-a2e2-93d8410b5e8b@amd.com>
Date: Fri, 29 Dec 2023 20:30:00 -0600
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] firewire: ohci: suppress unexpected system reboot in AMD
 Ryzen machines and ASM108x/VT630x PCIe cards
Content-Language: en-US
To: Takashi Sakamoto <o-takashi@sakamocchi.jp>,
 linux1394-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org
Cc: adamg@pobox.com, stable@vger.kernel.org, Jiri Slaby
 <jirislaby@kernel.org>, Tobias Gruetzmacher <tobias-lists@23.gs>
References: <20231229035735.11127-1-o-takashi@sakamocchi.jp>
From: Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <20231229035735.11127-1-o-takashi@sakamocchi.jp>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?Q0JIYktCLzFhOG9rZGZXS201U1lNNzdtNXF0RFNoOXQyVm4vK3pLdFRRZE9Y?=
 =?utf-8?B?T1lrYnV0bUxkVHM4ZVBOMWx2QnFGdTRXUlZXd0cxbCtKcDRCNE5sRmd3VDl1?=
 =?utf-8?B?cFZ1QXVmcHV5dUROM0dvTm1TT1ErSGEyb2kybll2NU9veHJ1VXM5eUFHOEhw?=
 =?utf-8?B?QmZoS3FScGFXOFpnYTVlVVNNMm9VK1UyUURjc1BlWEhaVjdBK0lUUkFHQ0xy?=
 =?utf-8?B?WThDNGxrVEZBeExuaGt5UWFsZFdIVVBXRHNjZ1pUT3lVajhONTR5RW9sSDlI?=
 =?utf-8?B?ZWNkSWJqOFdlNkV6VkxFSGxwdy9RdFdObElXL096bTJSMCtIbUtXb0Y2YzNG?=
 =?utf-8?B?NjdLWkVJT0txQ2c0RElhc21oVWxocCtPWmx5WnQ3bi9aVVphTzN3anJzQyti?=
 =?utf-8?B?dHloMi94R0xac2xZdlNOeVlRdXNSUHd3RWdlb3RYZzByY1JycVI4RkVMc2J0?=
 =?utf-8?B?eGxSdWtYVVd0a1U3a2UwaE9LbzJ0Z2RkWFh5VWhLWjF5dkloTnVVRHp2NkMr?=
 =?utf-8?B?alR1bG12UldoK1c5QUtQamtwbHZDT0JLeFlDZ05pVjBRT21aVTd5VHd4ME5M?=
 =?utf-8?B?U3crQWV1RW1OcXo0aENqdWZFWTJDdnROcDdhbmtoajdwZjMyQU83MnFublVE?=
 =?utf-8?B?RXQxdWZDeThRQkw4Y2s2QXZjb3JGbTgycnFmZVNyeEtsc0laZHFhU3ZBaE5x?=
 =?utf-8?B?SWJtVm5FMFhiaUxld3FkU3dPaGthcmRzanNVa0R4dVRsaTBMMTlUYTcrWFp3?=
 =?utf-8?B?R3F4MnN6a2FtSFJ6bVdhZEJVRUdMNXE0aVU4NThrUmRETVRFZGRyTW1TYXB6?=
 =?utf-8?B?bE9KcW1VUkhKY3FCWjFxZzF3OGl4dTlqS3JXaUJuTlRWNFJVRVEvMHNtc3Bu?=
 =?utf-8?B?Mm83Zi9BR1hBalk2eW91dnNWUlFWdG0vQUVVTm1weDYyaGM2eEZvd0Erbk1m?=
 =?utf-8?B?Qk15bGhoQmFPTS82c3NGTDNoZXhwbzFRRjNPcm1VdjVxY0NFdW9PVGRxUVV4?=
 =?utf-8?B?MTdMV2ZTaWx3R1JUdnhJYjlVR2hzdnhxOHFkMGNuZjBlMVNEZlVrdkRRYmhP?=
 =?utf-8?B?aUZHUnpZWDRlUWRzQ2MyK3dQcHlKdXZaNzk0U3NTTkZKdWpWenZZSkNOai9y?=
 =?utf-8?B?VGJPWERqRWV5SWVQQ3B4bjBScVBZRjQwbU9xZlN3dDk0cDRGRFNidHJTcVE1?=
 =?utf-8?B?RHRvd3lHUzY0cjVqNElVODN3WWNRWVVlRHF6WWhodUJpZGYwRFpTQk9xaE11?=
 =?utf-8?B?QUtkQlFwcHFLRWV5OXFJNVljMEhwMXpvYkpXcTVWMys4dUVMR2c3N0ZZRStU?=
 =?utf-8?B?WnpoUEdTd1lGRkJ6SlZOTUNSU2t4UXcxVXI1SHJNaitmUUg2Y1c0bVZ6VUJS?=
 =?utf-8?B?S1o1Q1pYTkFmWmhsR04zM2dsZVVjalRLWFRVdVFvYStvZTArTHFqNjVvVDZX?=
 =?utf-8?B?SmRiK21XSnFjczl6TzhjNTI4dFArdnVOb2NkT2luaThqUW5CVTBJZHU5Ui9R?=
 =?utf-8?B?cEt6WGFlVERvbTg0ekg4Y3BYN25BU0FTTmNzZTB2WitpNjdyenlvb2VBR1dH?=
 =?utf-8?B?OThrc1I1aXBxYzdUak90c3YxUEJEbUNtdEttMm83K21XTFp5TVEyVVRWRFhU?=
 =?utf-8?B?MWxSa1NLQm9MWnpXQTgybzJGdXdtd1pvZER2SmRMVy9IRXVnYUU0djNHaDFt?=
 =?utf-8?B?SXREcFl5SzBTblBQakh1VTFOSEF2eVYzcDVHOFBBNWNKdjIxbTg0d2lwSWU4?=
 =?utf-8?B?Tm1wQXhZQitRZy82cmErM1N1OUcveXBYb2lESDRBRVdvRUdWQld0QWlLcnp6?=
 =?utf-8?B?dFhZWkFmSWRiN2ticEMvZStBQndLRGFwMFNPRTg2cno5WXo4K0JqRUtFZFgw?=
 =?utf-8?B?ZW13cys2Z2gwRThma2huYmZGZm5GOTNRTVVqM2dYU3BrN0pwTHR1bDczbEd5?=
 =?utf-8?B?Rk5sRDJ2azZJYy9mMmJhVWV4T0FJRXpDYmxmT2k2RUJiR3BtY01WNk16cEd5?=
 =?utf-8?B?Uys2aVRLTnM1Qkt2Z0t5dGh1TDUxOFV2U1lFUlVuWkZUTVZTTmEwQnhCMksr?=
 =?utf-8?B?NnQzVUZ1emxaT1VuaFRHK1lFTFkrNWZ0V0F2aU5MS3RqVDc3YzI5Q1luVU1K?=
 =?utf-8?Q?Fu81CZy8Ldn5bi9or28cDGdHR?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4e5effc4-b774-4c24-d372-08dc08df3a69
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Dec 2023 02:30:03.1826
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: AWSI9fnKwvmtBkVbAk5il6r+uWplYj1/sajctZTzt9yEflJ7PngOunKiTotp9tIGXieEKOOvUato/zSexut/hQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR12MB5439

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/