Re: [PATCH v1] PCI: pciehp: Make sure DPC trigger status is reset in PDC handler

[Lukas Wunner]

On Wed, Jun 14, 2023 at 11:25:59PM -0700, Kuppuswamy Sathyanarayanan wrote:
> During the EDR-based DPC recovery process, for devices with persistent
> issues, the firmware may choose not to handle the DPC error and leave
> the port in DPC triggered state. In such scenarios, if the user
> replaces the faulty device with a new one, the OS is expected to clear
> the DPC trigger status in the hotplug error handler to enable the new
> device enumeration.

You're clearing the DPC trigger status upon a PDC event, yet are saying
here the purpose is to reset port state for a future hotplugged device.

A PDC event may be synthesized, e.g. to trigger slot bringup via
sysfs, so using a PDC event to clear DPC trigger status feels wrong.
pciehp_unconfigure_device() seems like a more appropriate place to me.


> More details about this issue can be found in PCIe
> firmware specification, r3.3, sec titled "DPC Event Handling"
> Implementation note.

That Implementation Note contains a lot of text and a fairly complex
flow chart. If you could point to specific paragraphs or numbers in
the Implementation Note that would make life easier for a reviewer
to make the connection between your code and the spec.


> Similar issue might also happen if the DPC or EDR recovery handler
> exits before clearing the trigger status. To fix this issue, clear the
> DPC trigger status in PDC interrupt handler.

I was about to ask why the code is added to dpc.c, not edr.c,
and why it's not constrained to CONFIG_PCIE_EDR, but I assume
that's the reason?  Because it "might" happen for OS-native DPC
as well?


> +/**
> + * pci_reset_trigger - Clear DPC trigger status
> + * @pdev: PCI device
> + *
> + * It is called from the PCIe hotplug driver to clean the DPC
> + * trigger status in the PDC interrupt handler.
> + */
> +void pci_dpc_reset_trigger(struct pci_dev *pdev)
> +{
> +	if (!pdev->dpc_cap)
> +		return;
> +
> +	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_STATUS,
> +			      PCI_EXP_DPC_STATUS_TRIGGER);
> +}

This may run concurrently to dpc_reset_link(), so I'd expect that
you need some kind of serialization.  What happens if pciehp clears
trigger status behind the DPC driver's back while it is handling an
error?

Thanks,

Lukas