Re: [PATCH v6] Bluetooth: btintel_pcie: Add vendor_reset PCI sysfs for PLDR

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] The `vendor_reset` sysfs attribute silently drops manual PLDR requests if the device setup failed.

Pre-existing issues:
- [High] ABBA deadlocks between driver unbind/hot-remove and `reset_work`.
- [High] Missing state cleanup on reset failures permanently blocks all future reset attempts.
- [Medium] Leaked PCI device reference when reset work is canceled during module removal.
--

--- Patch [1]: [PATCH v6] Bluetooth: btintel_pcie: Add vendor_reset PCI sysfs for PLDR ---
commit 405f703cae1beabdc354fcdf50ce09274b48b2a9
Author: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com>

Bluetooth: btintel_pcie: Add vendor_reset PCI sysfs for PLDR

Add a read-write sysfs entry at /sys/bus/pci/devices/<BDF>/vendor_reset
to allow userspace to trigger PLDR (Product Level Device Reset).

> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> --- a/drivers/bluetooth/btintel_pcie.c
> +++ b/drivers/bluetooth/btintel_pcie.c
[ ... ]
> @@ -2790,6 +2790,43 @@ static void btintel_pcie_hci_reset(struct hci_dev *hdev)
>  	btintel_pcie_request_reset(data, BTINTEL_PCIE_IOSF_PRR_FLR);
>  }
>  
> +static ssize_t vendor_reset_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	unsigned int val;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct btintel_pcie_data *data = pci_get_drvdata(pdev);
> +
> +	if (!data || !data->hdev)
> +		return -ENODEV;
> +
> +	if (kstrtouint(buf, 10, &val) || val != 0) {
> +		bt_dev_warn(data->hdev, "PLDR rejected: invalid input");
> +		return -EINVAL;
> +	}
> +
> +	bt_dev_info(data->hdev, "PLDR triggered via sysfs");
> +	btintel_pcie_request_reset(data, BTINTEL_PCIE_IOSF_PRR_PLDR);

[Severity: Medium]
If the device fails during its initial probe and BTINTEL_PCIE_SETUP_DONE is
never set, will this silently drop the manual PLDR request?

Looking at btintel_pcie_request_reset(), it aborts early if the setup flag
is missing:

static void btintel_pcie_request_reset(struct btintel_pcie_data *data, ...)
{
    if (!test_bit(BTINTEL_PCIE_SETUP_DONE, &data->flags))
        return;
    ...
}

Since vendor_reset_store() returns count and logs success, won't userspace
be unaware that the recovery attempt was ignored?

[Severity: High]
This is a pre-existing issue, but does exposing this reset via sysfs
introduce an ABBA deadlock against device unbind or PCI hot-remove?

If a user writes to the remove sysfs file of the PCI device, the PCI core
acquires pci_lock_rescan_remove() and calls the driver remove function.
Then btintel_pcie_remove() waits for the reset work:

btintel_pcie_remove()
    disable_work_sync(&data->reset_work);

If the reset work has already started, it attempts to acquire the lock:

btintel_pcie_reset_work()
    pci_lock_rescan_remove();

Alternatively, if the device unbind sysfs file is written, device_lock() is
held. Then the reset work calls btintel_pcie_acpi_reset_method() which
attempts to acquire the same lock:

btintel_pcie_acpi_reset_method()
    pci_dev_lock(data->pdev);

Could a concurrent sysfs reset and driver unbind cause a deadlock?

[Severity: High]
This is a pre-existing issue, but can this recovery mechanism become
permanently blocked if a reset fails?

When btintel_pcie_request_reset() is called, it sets a flag to prevent
concurrent resets:

if (test_and_set_bit(BTINTEL_PCIE_RECOVERY_IN_PROGRESS, &data->flags))
    return;

If the reset operation fails (for example, in btintel_pcie_perform_pldr),
it skips device_reprobe(). Because reprobing is the only path that clears
the IN_PROGRESS flag, won't the flag remain set permanently?

If this happens, won't all future writes to this sysfs attribute be ignored?

[Severity: Medium]
This is a pre-existing issue, but can this leak a PCI device reference if
the module is removed?

Calling btintel_pcie_request_reset() increments the reference count:

btintel_pcie_request_reset()
    pci_dev_get(data->pdev);
    schedule_work(&data->reset_work);

If the driver is unbound before the work executes, btintel_pcie_remove()
cancels it:

btintel_pcie_remove()
    disable_work_sync(&data->reset_work);

If disable_work_sync() successfully cancels the pending work, the work
never runs and pci_dev_put() is never called. Could this leak the pdev
structure?

> +
> +	return count;
> +}
> +
> +static ssize_t vendor_reset_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727052104.1026824-1-chandrashekar.devegowda@intel.com?part=1