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;
Date:   Thu, 15 Jun 2023 20:35:50 +0200
From:   Lukas Wunner <lukas@wunner.de>
To:     Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc:     Bjorn Helgaas <bhelgaas@google.com>, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1] PCI: pciehp: Make sure DPC trigger status is reset in
 PDC handler
Message-ID: <20230615183550.GA9773@wunner.de>
References: <20230615062559.1268404-1-sathyanarayanan.kuppuswamy@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230615062559.1268404-1-sathyanarayanan.kuppuswamy@linux.intel.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

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