Re: [PATCH v2 07/10] PCI/FLIT: Add support for Flit Logging Extended Capability

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pcie/flit: IRQ storm due to free_irq() before disabling hardware
--

commit c76b2fb611bb532cfef2fa0ae176b99d25d187a0
Author: Yazen Ghannam <yazen.ghannam@amd.com>

PCI/FLIT: Add support for Flit Logging Extended Capability

This commit adds a PCIe port service driver for the Flit Logging Extended
Capability, including _OSC negotiation, interrupt handling, and error
logging features.

Link: https://pcisig.com/specification/extend-osc-negotiate-control-pcie-flit-logging-extended-capability

> diff --git a/drivers/pci/pcie/flit.c b/drivers/pci/pcie/flit.c
> new file mode 100644
> index 0000000000000..7c0b9c8dd4811
> --- /dev/null
> +++ b/drivers/pci/pcie/flit.c

[ ... ]

> +static irqreturn_t flit_ist(int irq, void *context)
> +{
> +	struct pcie_device *dev = (struct pcie_device *)context;
> +	struct pci_dev *pdev = dev->port;

[ ... ]

> +	/*
> +	 * Re-enable the counter. Per PCIe r6.4, sec 7.7.8.5, table 7-96, the
> +	 * status bit is "Cleared on 0b to 1b transition of Flit Error Counter
> +	 * Enable", which also clears the counter and re-arms interrupt
> +	 * generation.
> +	 */
> +	flit_cntr_ctrl_update(pdev, 0, PCI_FLIT_ERR_CNTR_CTRL_EN);
> +	return IRQ_HANDLED;
> +}

[Severity: High]
Does unconditionally re-enabling the counter here prevent a safe teardown
sequence in flit_cntr_remove()?

If this checked pdev->flit_info->cntr_enabled before re-enabling, could we
then safely disable the counter before freeing the IRQ during removal?

[ ... ]

> +static int flit_probe(struct pcie_device *dev)
> +{
> +	struct pci_dev *pdev = dev->port;
> +	int status;
> +
> +	/*
> +	 * Not devm_request_threaded_irq(): devres would free the IRQ only
> +	 * after .remove() runs, too late to stop flit_ist() re-enabling the
> +	 * counter.
> +	 */

[Severity: High]
Does this deliberate teardown order choice introduce an IRQ storm
regression?

> +	status = request_threaded_irq(dev->irq, flit_isr, flit_ist,
> +				      IRQF_SHARED, "pcie-flit", dev);

[ ... ]

> +static void flit_cntr_remove(struct pcie_device *dev)
> +{
> +	/* Drop the IRQ first: free_irq() waits for the threaded handler. */
> +	free_irq(dev->irq, dev);
> +	flit_cntr_disable(dev->port);
> +}

[Severity: High]
Could calling free_irq() before flit_cntr_disable() cause a regression
with an unhandled level-triggered interrupt storm?

Because the IRQ is requested with IRQF_SHARED (shared with other PCIe
port services like PME and Hotplug), the interrupt may be a
level-triggered INTx line.

If the Flit hardware asserts an interrupt during the window between
free_irq() and flit_cntr_disable(), the other shared handlers will be
invoked but will return IRQ_NONE since they cannot clear the Flit status
register.

Could this prompt the kernel to permanently disable the shared IRQ line
("irq XX: nobody cared") and break all other services sharing it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918145619.3016889-1-yazen.ghannam@amd.com?part=7