Re: [PATCH 1/2] PCI: pciehp: Add support for OS-First Hotplug and AER/DPC

[Smita Koralahalli]

On 11/4/2022 3:15 AM, Lukas Wunner wrote:
> On Tue, Nov 01, 2022 at 12:07:18AM +0000, Smita Koralahalli wrote:
> > The implementation is as follows: On an async remove a DPC is triggered as
> > a side-effect along with an MSI to the OS. Determine it's an async remove
> > by checking for DPC Trigger Status in DPC Status Register and Surprise
> > Down Error Status in AER Uncorrected Error Status to be non-zero. If true,
> > treat the DPC event as a side-effect of async remove, clear the error
> > status registers and continue with hot-plug tear down routines. If not,
> > follow the existing routine to handle AER/DPC errors.
> Instead of having the OS recognize and filter Surprise Down events,
> it would also be possible to simply set the Surprise Down bit in the
> Uncorrectable Error Mask Register.  This could be constrained to
> Downstream Ports capable of surprise removal, i.e. those where the
> is_hotplug_bridge in struct pci_dev is set.  And that check and the
> register change could be performed in pci_dpc_init().
>
> Have you considered such an alternative approach?  If you have, what
> was the reason to prefer the more complex solution you're proposing?

By setting the Surprise down bit in Uncorrectable Error Mask register we 
will
not get a DPC event. What I know so far is, we cannot set this bit at 
run-time
after we determine its a surprise down event or probably I don't know
enough how to do it! (once an pciehp interrupt is triggered..).

And setting this bit at initialization might not trigger true DPC events..

Second thing, is masking Surprise Down bit has no impact on logging 
errors in
AER registers.

So, I think that approach probably will not resolve the issue of 
clearing the logs
in AER registers and complicate things while differentiating true errors 
vs surprise
down events. Please correct me if I'm wrong!!

I did few testing after I read your comments.  What I realized is that, 
these DPC
events (side effects of hot remove) are actually benign on AMD systems. 
On a hot
remove I see a Presence Detect change and a DPC event. This PD state change
will trigger a pciehp isr and calls pciehp_handle_presence_or_link_change()
and disables the slot normally. So essentially, this patch will boil 
down to the point
to clearing the logs in AER registers and also handling those error 
messages ("device
recovery failed"....) in dmesg which might confuse users on a hot remove.
What do you think?

Now, I'm not sure whether there will be a PD state change across other 
systems on a
hot remove when AER/DPC is native (OS First) in which case we should 
call the
pciehp_disable_slot() from dpc handler as well.. Any inputs would be
appreciated here..
> > +static void pci_clear_surpdn_errors(struct pci_dev *pdev)
> > +{
> > +	u16 reg16;
> > +	u32 reg32;
> > +
> > +	pci_read_config_dword(pdev, pdev->dpc_cap + PCI_EXP_DPC_RP_PIO_STATUS, &reg32);
> > +	pci_write_config_dword(pdev, pdev->dpc_cap + PCI_EXP_DPC_RP_PIO_STATUS, reg32);
> > +
> > +	pci_read_config_word(pdev, PCI_STATUS, &reg16);
> > +	pci_write_config_word(pdev, PCI_STATUS, reg16);
> > +
> > +	pcie_capability_read_word(pdev, PCI_EXP_DEVSTA, &reg16);
> > +	pcie_capability_write_word(pdev, PCI_EXP_DEVSTA, reg16);
> > +}
> I don't understand why PCI_STATUS and PCI_EXP_DEVSTA need to be
> touched here?

This is just to mask any kind of appearance that there was an error since
the errors would have been induced by the hot plug event (just duplicating
our BIOS functionality here..).  But, please let me know if OS is already
handling the things here and if it is not required.
> > +static void pciehp_handle_surprise_removal(struct pci_dev *pdev)
> Since this function is located in dpc.c and is strictly called from
> other functions in the same file, it should be prefixed dpc_, not
> pciehp_.

Sure, will fix in v2.
> > +	/*
> > +	 * According to Section 6.13 and 6.15 of the PCIe Base Spec 6.0,
> > +	 * following a hot-plug event, clear the ARI Forwarding Enable bit
> > +	 * and AtomicOp Requester Enable as its not determined whether the
> > +	 * next device inserted will support these capabilities. AtomicOp
> > +	 * capabilities are not supported on PCI Express to PCI/PCI-X Bridges
> > +	 * and any newly added component may not be an ARI device.
> > +	 */
> > +	pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL2,
> > +				   (PCI_EXP_DEVCTL2_ARI | PCI_EXP_DEVCTL2_ATOMIC_REQ));
> That looks like a reasonable change, but it belongs in a separate
> patch.  And I think it should be performed as part of (de-)enumeration,
> not as part of DPC error handling.  What about Downstream Ports which
> are not DPC-capable, I guess the bits should be cleared as well, no?

AFAIK, DPC will work on all AMD root ports. I'm not sure how could we handle
on a per port basis if the bridges/ports downstream to root ports don't 
support
DPC..
> How about clearing the bits in pciehp_unconfigure_device()?

Okay.

Thanks,
Smita
> Thanks,
>
> Lukas