RE: [PATCH v4] Bluetooth: btintel_pcie: Add vendor_rst PCI sysfs for PLDR

Hi Bjorn,
     Thanks for the comments

> -----Original Message-----
> From: Bjorn Helgaas <[email protected]>
> Sent: Thursday, July 23, 2026 3:49 AM
> To: Devegowda, Chandrashekar <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; Srivatsa, Ravishankar
> <[email protected]>; Tumkur Narayan, Chethan
> <[email protected]>; K, Kiran <[email protected]>; Alex
> Williamson <[email protected]>; [email protected]
> Subject: Re: [PATCH v4] Bluetooth: btintel_pcie: Add vendor_rst PCI sysfs for
> PLDR
> 
> [+cc Alex, linux-pci]
> 
> On Wed, Jul 22, 2026 at 06:51:57AM +0530, Chandrashekar Devegowda
> wrote:
> > Add a read-write sysfs entry at /sys/bus/pci/devices/<BDF>/vendor_rst
> > to allow userspace to trigger PLDR (Product Level Device Reset).
> 
> IMO there's no need to abbreviate "reset" as "rst".  I'd rather spell it out as
> "vendor_reset".
> 

Ack, will update in v5

> Several other drivers that add files in /sys/bus/pci/devices/ have
> documentation in Documentation/ABI/testing/sysfs-bus-pci-drivers-*.
> 
> It seems important to mention the fact that IIUC, this PLDR will release any
> driver attached to the wifi device that shares hardware with this bluetooth
> device, do the reset, and reprobe the wifi.
> 

Ack, will update the documentation and mention the impact on wifi.

> I guess this can't be integrated into the generic pci_reset_function() reachable
> via the sysfs 'reset' attribute because of the fact that it affects both the BT and
> the wifi device, which are different PCI functions.  But I cc'd Alex anyway since
> he's the real guru on resets.
> 
> FWIW, I learned a lot about the idiosyncrasies of these connected BT and wifi
> devices in this old conversation:
> https://lore.kernel.org/all/20250314101613.3682010-1-
> [email protected]
> 
> > 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 v4:
> >   - Rebased on latest bluetooth-next (6f55ad8fb0ac) to fix
> >     CI apply failure
> 
> v4: https://lore.kernel.org/all/20260722012158.813793-1-
> [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
> >   - Handle schedule_work() failure: release pci_dev_get refcount
> >     and clear RECOVERY_IN_PROGRESS flag
> >   - Fix remove ordering: device_remove_file before disable_work_sync
> 
> v3: https://lore.kernel.org/all/20260722001434.804931-1-
> [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/20260618085016.9173-1-
> [email protected]/
> 
> > Changes in v1:
> >   - Initial vendor_rst PCI sysfs implementation
> > drivers/bluetooth/btintel_pcie.c | 42
> +++++++++++++++++++++++++++++++-
> 
> v1: https://lore.kernel.org/all/20260612012832.2395034-1-
> [email protected]/
> 
> >  1 file changed, 41 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/bluetooth/btintel_pcie.c
> > b/drivers/bluetooth/btintel_pcie.c
> > index 2e28847263ab..a412ca7ff3ad 100644
> > --- a/drivers/bluetooth/btintel_pcie.c
> > +++ b/drivers/bluetooth/btintel_pcie.c
> > @@ -2781,7 +2781,10 @@ static void btintel_pcie_request_reset(struct
> btintel_pcie_data *data,
> >  	data->reset_type = type;
> >
> >  	pci_dev_get(data->pdev);
> > -	schedule_work(&data->reset_work);
> > +	if (!schedule_work(&data->reset_work)) {
> > +		pci_dev_put(data->pdev);
> > +		clear_bit(BTINTEL_PCIE_RECOVERY_IN_PROGRESS, &data-
> >flags);
> > +	}
> >  }
> >
> >  static void btintel_pcie_hci_reset(struct hci_dev *hdev) @@ -2791,6
> > +2794,36 @@ static void btintel_pcie_hci_reset(struct hci_dev *hdev)
> >  	btintel_pcie_request_reset(data, BTINTEL_PCIE_IOSF_PRR_FLR);  }
> >
> > +static ssize_t vendor_rst_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);
> > +
> > +	return count;
> > +}
> > +
> > +static ssize_t vendor_rst_show(struct device *dev,
> > +			       struct device_attribute *attr, char *buf) {
> > +	return sysfs_emit(buf, "0 - PLDR\n"); }
> > +
> > +static DEVICE_ATTR_RW(vendor_rst);
> > +
> >  static void btintel_pcie_hw_error(struct hci_dev *hdev, u8 code)  {
> >  	struct btintel_pcie_dev_recovery *rec; @@ -3010,6 +3043,11 @@
> static
> > int btintel_pcie_probe(struct pci_dev *pdev,
> >  	if (err)
> >  		goto exit_error;
> >
> > +	err = device_create_file(&pdev->dev, &dev_attr_vendor_rst);
> 
> Can you avoid the manual device_create_file() by taking advantage of
> 23b6904442d0 ("driver core: add dev_groups to all drivers") and setting
> .dev_groups in your struct driver?
> 
> If you can, I think it would avoid a race between the device becoming visible in
> sysfs (and the uevent announcing that) and the addition of this sysfs attribute.
> 

Ack, will change to dev_groups in v5

> > +	if (err)
> > +		bt_dev_warn(data->hdev, "Failed to create vendor_rst sysfs
> (%d)",
> > +			    err);
> > +
> >  	bt_dev_dbg(data->hdev, "cnvi: 0x%8.8x cnvr: 0x%8.8x", data->cnvi,
> >  		   data->cnvr);
> >  	return 0;
> > @@ -3046,6 +3084,8 @@ static void btintel_pcie_remove(struct pci_dev
> *pdev)
> >  	disable_work_sync(&data->hwexp_work);
> >  	disable_work_sync(&data->fwtrigger_work);
> >
> > +	device_remove_file(&pdev->dev, &dev_attr_vendor_rst);
> > +
> >  	/* Cancel pending reset work. Skip only when remove() is called from
> >  	 * within the reset work itself (PLDR device_reprobe path) to avoid
> >  	 * deadlock. current_work() returns the work_struct of the caller if
> > --
> > 2.43.0
> >