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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 15 Jun 2023 20:35:50 +0200
From:   Lukas Wunner <lukas@...ner.de>
To:     Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] PCI: pciehp: Make sure DPC trigger status is reset in
 PDC handler

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