Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <8ab986f2-6aa5-401a-aa21-e8b21f68eaad@amd.com>
Date:   Mon, 22 May 2023 15:23:57 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH v2 1/2] PCI: pciehp: Add support for async hotplug with
 native AER and DPC/EDR
Content-Language: en-US
To:     Lukas Wunner <lukas@wunner.de>
Cc:     linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
        Bjorn Helgaas <bhelgaas@google.com>, oohall@gmail.com,
        Mahesh J Salgaonkar <mahesh@linux.ibm.com>,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        Yazen Ghannam <yazen.ghannam@amd.com>,
        Fontenot Nathan <Nathan.Fontenot@amd.com>
References: <20230418210526.36514-1-Smita.KoralahalliChannabasappa@amd.com>
 <20230418210526.36514-2-Smita.KoralahalliChannabasappa@amd.com>
 <20230516101001.GA18952@wunner.de>
From:   Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
In-Reply-To: <20230516101001.GA18952@wunner.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?S1BvZVh4di90MUV4T3kwek9vb1BBUXVjaW1YaS9qMUp6RkpQZ05hcUpPcCsz?=
 =?utf-8?B?RldGWVRFRmpHM2VQbDhCbXJlTUttVmxGcWVoZmZhWjh0ZHhkckoyYXllMWZ6?=
 =?utf-8?B?cWhGN2g3eFVkRmF5dEl1Mk1hb3M0eko1TVJnVTBJK1gwVnlsekd0L3llaXEz?=
 =?utf-8?B?UmRRQzViVDdXd3E0YjQzcHFjS0ZSM2ZYWUlIUlZ2anlpS0trUklpZmNQeEhW?=
 =?utf-8?B?VU90eDJaejFDdkxWVmZlTnVRWHBZSjZzb0x2eUdxcU9ia0RHMjVnRG5Rb2N6?=
 =?utf-8?B?SHNWTzgxTXlVM2pSc21BbHVKZFhpS2FUYTY5c2pZeFpVU25BMkg5Z2toWFI2?=
 =?utf-8?B?eU4waUF6Q3huc09xdU1OaEpSNjRsUW9QUnZBN25SVUg3aXptN00xQWU0WC84?=
 =?utf-8?B?NkRraTZQYTJjRVg0dVljNnZvL0h1TnQwMzhSL0Nhd1hvOHpvRmlWTDQ5RHRm?=
 =?utf-8?B?TmxXYU5naGZjRklHS1A4azJUY1Z1NXozeXoyY1JDZjBKTmg4eXgyQnhyU01u?=
 =?utf-8?B?eFBDS0VMNkp5cDhNcmk1MGZ3S3FZM3I5OHhEMzUyeWZvYVhjc25vWjV6R1Bt?=
 =?utf-8?B?ajFWWUs0L0ZUYnp0WXdBNGUzRkl0VlI4bXBDMUpQbUxuR2pvZHlwSlhnRjQ0?=
 =?utf-8?B?MUJUdVZMUG8yRXArZGZ3RHhsMmJYY2VHbVRWV2JST0RGc2d0ZzlNZGhxcWRp?=
 =?utf-8?B?dnZtTi9VWldsZTFIanQxVmNPQWdZUnVyWnF2cmtjM2dXYUovS1F0dWVSZUlV?=
 =?utf-8?B?VVh3ZzZBa21mS0wzLzFHbnpNYmR5bUh6L2llZVU3RXZFU2R4UGJmZy9FcXJO?=
 =?utf-8?B?R09UZ2ttVmxjQnFlWkZXcktLK3VnV3M2TXJUdnMwOWZWanE3RUtaK04vNFdB?=
 =?utf-8?B?bzU3MUZMckp2OFdhR0NNN2NsRWpOUkJYQmFkRVpmV2lKaXJMWkNCSWlLNmVC?=
 =?utf-8?B?NzJ1aUEvZU1BZ3UrK1lhL2lMcDRoV0xMS3VKNWJkNjg0ZXBRd3ZHcEc2YTIz?=
 =?utf-8?B?LytlbzBmUlZGVDFkWnR6Zk5MVTJXQUVPUDdYYTJmbll6S2JDTGdIOHlNZ2pk?=
 =?utf-8?B?WktxV0ZvVnJsMWJPdmlHZWtwUngrQUV5SjgxTkpXdE0weGg0cjJMNGNqSjNN?=
 =?utf-8?B?SjREdDdnSWNoNHFyWS9NQWV3eS93L3RWWlBxRm01RCtCbHY0T3FJT3VPbUtz?=
 =?utf-8?B?ZzZ4Z0kvTkNzSnRPVzZXVTg1MVF0ckxnLzRLeHhUUlJ0WDZid2NZNVRUenlp?=
 =?utf-8?B?SDltNGRGRVVTUzlNOWRnVk80UnJxUkxIWVpFWk95elM2cmRmZldreThodGZN?=
 =?utf-8?B?NExHUlh1WG1aV0hUUDkvVXdHRW02TUlXaXBQNUhPRHV3b3BER3dhb3NTRnlS?=
 =?utf-8?B?djA1YWZ1ekNvdGdZV2lQQm9FWkc5d0tIamljbXpTcDZvUUxtT0JmOE9TSFA1?=
 =?utf-8?B?bmc2UERtMTdaaFJBVlR0aVFyZTJaTWxsL1JERlhmZk5UUzFtb3JndE51WVZq?=
 =?utf-8?B?QkQ4SkFXQnJkd2pHVU1wWng2MDFmdDhuV1lyWlB4OGpqcHhXS0NKVFlNeG1x?=
 =?utf-8?B?L0Y3Q1JpUDlveUs3bnNsdTkweUE4ZlJvc1FQekRwcmZXckJoWDMyRm9VWjBk?=
 =?utf-8?B?M1lEWjd1eGhOSnhUQWF6c0hyU1lZZUdZUTg5SGhudnR5UWlLekNNbXgwbkZ2?=
 =?utf-8?B?N3M2U1JnVGowL1Q0Y3RwQzNpUktzekJrNDZtVXBkSmorNDJXYnoyanZQMnhq?=
 =?utf-8?B?YXRJc2Fza3ZSL0xVR2grZ29aSWlvdTN5YWViSjFGaUpJVWdtVWUrOWUyVmYx?=
 =?utf-8?B?WWpDaHdmV3VlbWJQZE5ScmRaOXFMMGdqaFN4bWQycnVHSmZ4QjQxOFVhWHYw?=
 =?utf-8?B?TlFJTXZPenFhQlBoY3M3MXFYQVhRRWFxTzNxM0h1RTdRMDlXNlFlQ2txUjdE?=
 =?utf-8?B?RzRLT0Z2dE5YL3JBZ0o3YWZOZ0FNb2JIcXZVYm90MzYyaDA0cEMrQVNINzhW?=
 =?utf-8?B?V0Jjb0NDSTdidk5mQXVWY1BMVXh2Q3NkZ0t2Q3JFNkVjYkJOVHdhM1lNRlJY?=
 =?utf-8?B?RndhT1Q0MDNpWVRsckhKMFQzWmN0cGQ1cjJScVZQdkZWUlNOVG1CVU9Ya1Zo?=
 =?utf-8?Q?MPg5tQYn2EIzyj9XkzkEPuqog?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e65a659e-570d-4f18-6454-08db5b133ddf
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB2876.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 May 2023 22:24:02.2259
 (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: pJ5YcReznUlmcHnsEsD3KDrMJ7OUeLciElxWp8jQeUG6veTLWBJuEd7Mfi5vPALSa8SvGRSH/qQ59OMkOd6f2Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ1PR12MB6219
X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi,

On 5/16/2023 3:10 AM, Lukas Wunner wrote:
> On Tue, Apr 18, 2023 at 09:05:25PM +0000, Smita Koralahalli wrote:
>> According to Section 6.7.6 of PCIe Base Specification [1], async removal
>> with DPC and EDR may be unexpected and may result in a Surprise Down error.
>> This error is just a side effect of hot remove. Most of the time, these
>> errors will be abstract to the OS as current systems rely on Firmware-First
>> model for AER and DPC, where the error handling (side effects of async
>> remove) and other necessary HW sequencing actions is taken care by the FW
>> and is abstract to the OS. However, FW-First model poses issues while
>> rolling out updates or fixing bugs as the servers need to be brought down
>> for firmware updates.
>>
>> Add support for async hot-plug with native AER and DPC/EDR. Here, OS is
>> responsible for handling async add and remove along with handling of AER
>> and DPC events which are generated as a side-effect of async remove.
> 
> I think you can probably leave out the details about Firmware-First.
> Pointing to 6.7.6 and the fact that Surprise Down Errors may occur as
> an expected side-effect of surprise removal is sufficient.  They should
> be treated as such.
> 
> You also want to point out what you're trying to achieve here, i.e. get
> rid of irritating log messages and a 1 sec delay while pciehp waits for
> DPC recovery.

Okay.

> 
> 
>> Please note that, I have provided explanation why I'm not setting the
>> Surprise Down bit in uncorrectable error mask register in AER.
>> https://lore.kernel.org/all/fba22d6b-c225-4b44-674b-2c62306135ed@amd.com/
> 
> Add an explanation to the commit message that masking Surprise Down Errors
> was explored as an alternative approach, but scrapped due to the odd
> behavior that masking only avoids the interrupt, but still records an
> error per PCIe r6.0.1 sec 6.2.3.2.2.  That stale error is going to be
> reported the next time some error other than Surprise Down is handled.
> 
> 

Okay.

>> Also, while testing I noticed PCI_STATUS and PCI_EXP_DEVSTA will be set
>> on an async remove and will not be cleared while the device is brought
>> down. I have included clearing them here in order to mask any kind of
>> appearance that there was an error and as well duplicating our BIOS
>> functionality. I can remove if its not necessary.
> 
> Which bits are set exactly?  Can you constrain the register write to
> those bits?
>
Hmm, I was mostly trying to follow the similar approach done for AER.
pci_aer_raw_clear_status(), where they clear status registers 
unconditionally. Also, thought might be better if we are dealing with 
legacy endpoints or so.

I see these bits set in status registers:
PCI_ERR_UNCOR_STATUS 0x20 (Surprise Down)
PCI_EXP_DPC_RP_PIO_STATUS 0x10000 (Memory Request received URCompletion)
PCI_EXP_DEVSTA 0x604 (fatal error detected)

> 
>> +static void dpc_handle_surprise_removal(struct pci_dev *pdev)
>> +{
>> +	if (pdev->dpc_rp_extensions && dpc_wait_rp_inactive(pdev))
>> +		return;
> 
> Emit an error message here?

Okay

> 
> 
>> +	/*
>> +	 * According to Section 6.7.6 of the PCIe Base Spec 6.0, since async
>> +	 * removal might be unexpected, errors might be reported as a side
>> +	 * effect of the event and software should handle them as an expected
>> +	 * part of this event.
>> +	 */
> 
> I'd move that code comment to into dpc_handler() above the
> "if (dpc_is_surprise_removal(pdev))" check.

Okay.

> 
> 
>> +	pci_aer_raw_clear_status(pdev);
>> +	pci_clear_surpdn_errors(pdev);
>> +
>> +	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_STATUS,
>> +			      PCI_EXP_DPC_STATUS_TRIGGER);
>> +}
> 
> Do you need a "wake_up_all(&dpc_completed_waitqueue);" at the end
> of the function to wake up a pciehp handler waiting for DPC recovery?

I don't think so. The pciehp handler is however getting invoked 
simultaneously due to PDSC or DLLSC state change right.. Let me know if 
I'm missing anything here.

> 
> 
>> +static bool dpc_is_surprise_removal(struct pci_dev *pdev)
>> +{
>> +	u16 status;
>> +
>> +	pci_read_config_word(pdev, pdev->aer_cap + PCI_ERR_UNCOR_STATUS, &status);
>> +
>> +	if (!(status & PCI_ERR_UNC_SURPDN))
>> +		return false;
>> +
> 
> You need an additional check for pdev->is_hotplug_bridge here.
> 
> And you need to read PCI_EXP_SLTCAP and check for PCI_EXP_SLTCAP_HPS.
> 
> Return false if either of them isn't set.

Return false, if PCI_EXP_SLTCAP isn't set only correct? 
PCI_EXP_SLTCAP_HPS should be disabled if DPC is enabled.

Implementation notes in 6.7.6 says that:
"The Hot-Plug Surprise (HPS) mechanism, as indicated by the Hot-Plug
Surprise bit in the Slot Capabilities Register being Set, is deprecated
for use with async hot-plug. DPC is the recommended mechanism for 
supporting async hot-plug."

Platform FW will disable the SLTCAP_HPS bit at boot time to enable async 
hotplug on AMD devices.

Probably check if SLTCAP_HPS bit is set and return false?

> 
> 
>> +	dpc_handle_surprise_removal(pdev);
>> +
>> +	return true;
>> +}
> 
> A function named "..._is_..." should not have any side effects.
> So move the dpc_handle_surprise_removal() call down into dpc_handler()
> before the "return IRQ_HANDLED;" statement.

Okay.

> 
> 
> What about ports which support AER but not DPC?  Don't you need to
> amend aer.c in that case?  I suppose you don't have hardware without
> DPC to test that?

Yeah right. Also, if DPC isn't supported we leave HPS bit set and we 
won't see the DPC event at that time.

Thanks,
Smita

> 
> Thanks,
> 
> Lukas