Re: [PATCH v2 10/10] PCI/FLIT: Expose the error counter controls through sysfs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] PCI: data race in trigger_count_store allows flit_info->trigger to diverge
- [Medium] PCI: data race on capability save buffer between sysfs and pci_save_state
--

commit 3ec14595160ef1f891e99d5be3983ae720f59eb3
Author: Yazen Ghannam <yazen.ghannam@amd.com>
Subject: PCI/FLIT: Expose the error counter controls through sysfs

This patch exposes the Trigger Event on Error Count and Events to Count fields 
of the Flit Error Counter Control register via sysfs. It tracks the trigger 
in struct flit_info and applies sysfs writes to the extended capability save 
buffer to ensure they persist across resets and recovery paths.

> diff --git a/drivers/pci/pcie/flit.c b/drivers/pci/pcie/flit.c
> index 252bbb54e6172..33ec1b9c385c3 100644
> --- a/drivers/pci/pcie/flit.c
> +++ b/drivers/pci/pcie/flit.c
[ ... ]
> @@ -186,9 +209,130 @@ static void flit_cntr_ctrl_update(struct pci_dev *pdev, u16 clear, u16 set)
>  PCI_RATELIMIT_INTERVAL_ATTR(ratelimit_interval_ms, flit_info->ratelimit);
>  PCI_RATELIMIT_BURST_ATTR(ratelimit_burst, flit_info->ratelimit);
>  
> +/*
> + * Program a durable setting into the register and the save buffer.
> + * pcie_portdrv_slot_reset() restores that buffer after running .slot_reset, so
> + * it is the last writer on the recovery path and would otherwise put the
> + * probe-time word back.
> + *
> + * Both writes happen under the lock, so the register and the buffer cannot
> + * disagree if trigger_count and events_to_count are written at once. The
> + * buffer gets the same clear and set rather than a fresh sample, which would
> + * pick up the Counter Enable that flit_isr() leaves clear for flit_ist().
> + */
> +static void flit_cntr_ctrl_set(struct pci_dev *pdev, u16 clear, u16 set)
> +{
> +	struct pci_cap_saved_state *save_state;
> +	unsigned long flags;
> +	u16 *cap;
> +
> +	save_state = pci_find_saved_ext_cap(pdev, PCI_EXT_CAP_ID_FLIT);
> +	cap = save_state ? (u16 *)&save_state->cap.data[0] : NULL;
> +
> +	pci_config_pm_runtime_get(pdev);
> +
> +	spin_lock_irqsave(&pdev->flit_info->lock, flags);
> +	__flit_cntr_ctrl_update(pdev, clear, set);
> +	if (cap) {
> +		*cap &= ~clear;
> +		*cap |= set;
> +	}

[Severity: Medium]
Could this sysfs write to the capability save buffer race with
pci_save_flit_state()?

While flit_cntr_ctrl_set() modifies the save buffer (*cap) under
flit_info->lock, the preexisting pci_save_flit_state() function modifies
this same buffer locklessly.

If a userspace process writes to the sysfs attributes (e.g., trigger_count)
exactly while the device is probed and pci_save_state(pdev) is called, could
this concurrently modify the buffer and corrupt it? Upon resuming or error
recovery, pci_restore_flit_state() might then restore corrupted or stale
values to the hardware register.

> +	spin_unlock_irqrestore(&pdev->flit_info->lock, flags);
> +
> +	pci_config_pm_runtime_put(pdev);
> +}
[ ... ]
> +static ssize_t trigger_count_store(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	u16 val;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	if (kstrtou16(buf, 0, &val) < 0)
> +		return -EINVAL;
> +
> +	if (val > FIELD_MAX(PCI_FLIT_ERR_CNTR_CTRL_TRIGGER))
> +		return -EINVAL;
> +
> +	pdev->flit_info->trigger = val;
> +	flit_cntr_ctrl_set(pdev, PCI_FLIT_ERR_CNTR_CTRL_TRIGGER,
> +			   FIELD_PREP(PCI_FLIT_ERR_CNTR_CTRL_TRIGGER, val));

[Severity: Medium]
Is there a potential data race here when multiple threads write to the
trigger_count sysfs attribute concurrently?

Because kernfs doesn't serialize writes from different open file descriptors,
two concurrent writes could interleave the assignment to
pdev->flit_info->trigger and the lock acquisition inside flit_cntr_ctrl_set().

For example:
Thread A sets pdev->flit_info->trigger = 10
Thread B sets pdev->flit_info->trigger = 20
Thread B calls flit_cntr_ctrl_set() and programs the hardware to 20
Thread A calls flit_cntr_ctrl_set() and programs the hardware to 10

The hardware would be programmed with 10, but the software state stored in
pdev->flit_info->trigger would remain 20. On the next enable (like resume or
recovery), the stale software state (20) would overwrite the hardware
register. Should the assignment to pdev->flit_info->trigger be done inside
flit_cntr_ctrl_set() under the spinlock?

> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(trigger_count);

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