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

Hi Chandru,

On Mon, Jul 27, 2026 at 5:57 PM Alex Williamson <[email protected]> wrote:
>
> On Mon, 27 Jul 2026 10:51:02 +0530
> Chandrashekar Devegowda <[email protected]> wrote:
>
> > Add a read-write sysfs entry at /sys/bus/pci/devices/<BDF>/vendor_reset
> > to allow userspace to trigger PLDR (Product Level Device Reset).
> > Reading the attribute displays supported reset types. Writing
> > integer 0 triggers PLDR. Any other input is rejected with
> > -EINVAL and a warning log.
> >
> > Signed-off-by: Chandrashekar Devegowda <[email protected]>
> > ---
> > Changes in v6:
> >   - Fixed .driver.dev_groups -> .dev_groups on struct pci_driver
> >     (__pci_register_driver overwrites .driver.dev_groups with .dev_groups)
> >   - Removed unrelated schedule_work() failure handling from
> >     btintel_pcie_request_reset() that caused CI context mismatch
> >
> > Changes in v5:
> >   - Renamed sysfs from vendor_rst to vendor_reset (reviewer feedback)
> >   - Switched from device_create_file() to driver dev_groups for
> >     automatic race-free sysfs lifecycle management
> >   - Added ABI doc at Documentation/ABI/testing/sysfs-bus-pci-drivers-btintel_pcie
> >   - Added WiFi impact description to ABI documentation
> >   - Added MAINTAINERS F: entry for ABI doc
> >   - v5: https://lore.kernel.org/all/[email protected]/
> >
> > Changes in v4:
> >   - Rebased on latest bluetooth-next to fix CI apply failure
> >   - v4: https://lore.kernel.org/all/[email protected]/
> >
> > Changes in v3:
> >   - Dropped reset_type parameter approach from hdev->reset()
> >   - Directly call btintel_pcie_request_reset() instead of manual
> >     flag manipulation and schedule_work()
> >   - Accept only integer 0 for PLDR trigger
> >   - v3: https://lore.kernel.org/all/[email protected]/
> >
> > Changes in v2:
> >   - Added reset_type parameter to hdev->reset() callback (1/2)
> >   - vendor_rst sysfs used reset_type to select PLDR (2/2)
> >   - v2: https://lore.kernel.org/all/[email protected]/
> >
> > Changes in v1:
> >   - Initial implementation
> >   - v1: https://lore.kernel.org/all/[email protected]/
> >  .../sysfs-bus-pci-drivers-btintel_pcie        | 15 ++++++++
> >  MAINTAINERS                                   |  1 +
> >  drivers/bluetooth/btintel_pcie.c              | 38 +++++++++++++++++++
> >  3 files changed, 54 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-pci-drivers-btintel_pcie
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-pci-drivers-btintel_pcie b/Documentation/ABI/testing/sysfs-bus-pci-drivers-btintel_pcie
> > new file mode 100644
> > index 000000000000..cceec6ac96bc
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-pci-drivers-btintel_pcie
> > @@ -0,0 +1,15 @@
> > +What:                /sys/bus/pci/devices/<BDF>/vendor_reset
>
> Nit, domain is present too, not just BDF.
>
> > +Date:                22-Jul-2026
> > +KernelVersion:       6.17

This needs fixing.

> Kernel 6.17 was 10 months ago.
>
> > +Contact:     [email protected]
> > +Description: This read-write attribute allows userspace to trigger a
> > +             Product Level Device Reset (PLDR) on Intel PCIe Bluetooth
> > +             controllers. Reading the attribute displays the supported
> > +             reset type. Writing integer 0 triggers PLDR. Any other
> > +             input is rejected with -EINVAL.
>
> This is opposite of /sys/bus/pci/devices/<BDF>/reset for no apparent
> reason and seems like it infringes on the namespace of sysfs-bus-pci.

Meaning this should write 1 to reset, right?

> > +
> > +             PLDR resets the entire on-chip platform shared between
> > +             Bluetooth and WiFi. This means any driver attached to
> > +             the WiFi device that shares hardware with this Bluetooth
> > +             device will be released, the platform will be reset, and
> > +             both the Bluetooth and WiFi devices will be re-probed.
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index eb8cdcc76324..39d390f1ceb4 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4625,6 +4625,7 @@ S:      Supported
> >  W:   http://www.bluez.org/
> >  T:   git git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git
> >  T:   git git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git
> > +F:   Documentation/ABI/testing/sysfs-bus-pci-drivers-btintel_pcie
> >  F:   Documentation/devicetree/bindings/net/bluetooth/
> >  F:   drivers/bluetooth/
> >
> > diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> > index ef42b8d11d4d..005c77a4f5eb 100644
> > --- 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);
>
> This is pretty scary on its own, scanning for specific wifi device IDs,
> finding the first match and releasing the driver for it while holding
> pci_lock_rescan_remove().  So an attribute on one device that unbinds
> the driver for another device (or worse, maybe reset it anyway if the
> SKUs are out of sync), and the reset happens asynchronously at some
> point in the future.  If the wifi device happens to be in use by
> vfio-pci, that delay is unbounded as it relies on a userspace driver or
> VM to release the device.

@ChandraShekar Please clarify. I guess we have no option since there
is no parent device to abstract the combo Wifi/BT design. This should
only be used when both Wifi and Bluetooth need a reset, so at that
point they shouldn't be operational. That said, maybe we should check
if control has been handed over to a VM, in which case we probably
need to reject resetting it.

> > +
> > +     return count;
> > +}
> > +
> > +static ssize_t vendor_reset_show(struct device *dev,
> > +                              struct device_attribute *attr, char *buf)
> > +{
> > +     return sysfs_emit(buf, "0 - PLDR\n");
>
> It seems like this is implying an enumerable list of resets, but the
> API is committing to zero/-EINVAL.  The string also isn't very sysfs
> compliant.  Thanks,

I guess you will need to explain what a 'compliant' sysfs read would
be; is this supposed to just return the current value (e.g. "0")?

> Alex
>
> > +}
> > +
> > +static DEVICE_ATTR_RW(vendor_reset);
> > +
> > +static struct attribute *btintel_pcie_attrs[] = {
> > +     &dev_attr_vendor_reset.attr,
> > +     NULL,
> > +};
> > +
> > +ATTRIBUTE_GROUPS(btintel_pcie);
> > +
> >  static void btintel_pcie_hw_error(struct hci_dev *hdev, u8 code)
> >  {
> >       struct btintel_pcie_dev_recovery *rec;
> > @@ -3250,6 +3287,7 @@ static struct pci_driver btintel_pcie_driver = {
> >       .probe = btintel_pcie_probe,
> >       .remove = btintel_pcie_remove,
> >       .driver.pm = pm_sleep_ptr(&btintel_pcie_pm_ops),
> > +     .dev_groups = btintel_pcie_groups,
> >  #ifdef CONFIG_DEV_COREDUMP
> >       .driver.coredump = btintel_pcie_coredump
> >  #endif
>


-- 
Luiz Augusto von Dentz