| Message ID | 20250210103244.646150-1-chharry@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
Hi Hsin-chen, On Mon, Feb 10, 2025 at 5:32 AM Hsin-chen Chuang <chharry@google.com> wrote: > > From: Hsin-chen Chuang <chharry@chromium.org> > > The functionality was completed in commit 5e5c3898ef49 ("Bluetooth: Fix > possible race with userspace of sysfs isoc_alt") > > Fixes: 5e5c3898ef49 ("Bluetooth: Fix possible race with userspace of sysfs isoc_alt") > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org> > --- > > (no changes since v1) > > Documentation/ABI/stable/sysfs-class-bluetooth | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth > index 36be02471174..1168e0318e35 100644 > --- a/Documentation/ABI/stable/sysfs-class-bluetooth > +++ b/Documentation/ABI/stable/sysfs-class-bluetooth > @@ -7,3 +7,15 @@ Description: This write-only attribute allows users to trigger the vendor reset > The reset may or may not be done through the device transport > (e.g., UART/USB), and can also be done through an out-of-band > approach such as GPIO. > + > +What: /sys/class/bluetooth/hci<index>/isoc_alt > +Date: 10-Feb-2025 > +KernelVersion: 6.13 > +Contact: linux-bluetooth@vger.kernel.org > +Description: This attribute allows users to configure the USB Alternate setting > + for the specific HCI device. Reading this attribute returns the > + current setting, and writing any supported numbers would change > + the setting. See the USB Alternate setting definition in Bluetooth > + core spec 5, vol 4, part B, table 2.1. > + If the data is not a valid number, the write fails with -EINVAL. > + The other failures are vendor specific. Still not really convinced this is the right way to expose it, it is not an HCI attribute to begin with, not sure if we couldn't perhaps add another node to control it or add via USB device?
Hi Luiz, Thanks for the feedback. On Tue, Feb 11, 2025 at 2:17 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Hsin-chen, > > On Mon, Feb 10, 2025 at 5:32 AM Hsin-chen Chuang <chharry@google.com> wrote: > > > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > The functionality was completed in commit 5e5c3898ef49 ("Bluetooth: Fix > > possible race with userspace of sysfs isoc_alt") > > > > Fixes: 5e5c3898ef49 ("Bluetooth: Fix possible race with userspace of sysfs isoc_alt") > > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org> > > --- > > > > (no changes since v1) > > > > Documentation/ABI/stable/sysfs-class-bluetooth | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth > > index 36be02471174..1168e0318e35 100644 > > --- a/Documentation/ABI/stable/sysfs-class-bluetooth > > +++ b/Documentation/ABI/stable/sysfs-class-bluetooth > > @@ -7,3 +7,15 @@ Description: This write-only attribute allows users to trigger the vendor reset > > The reset may or may not be done through the device transport > > (e.g., UART/USB), and can also be done through an out-of-band > > approach such as GPIO. > > + > > +What: /sys/class/bluetooth/hci<index>/isoc_alt > > +Date: 10-Feb-2025 > > +KernelVersion: 6.13 > > +Contact: linux-bluetooth@vger.kernel.org > > +Description: This attribute allows users to configure the USB Alternate setting > > + for the specific HCI device. Reading this attribute returns the > > + current setting, and writing any supported numbers would change > > + the setting. See the USB Alternate setting definition in Bluetooth > > + core spec 5, vol 4, part B, table 2.1. > > + If the data is not a valid number, the write fails with -EINVAL. > > + The other failures are vendor specific. > > Still not really convinced this is the right way to expose it, it is > not an HCI attribute to begin with, not sure if we couldn't perhaps Could you tell more about why this is not an HCI attr to begin with? The alt setting is bonded to the USB device which is now under btusb's control, and btusb creates a sysfs node for this. This attr location decision seems natural to me. > add another node to control it or add via USB device? I feel allowing this in the USB sysfs API might be an overkill as that makes all USB devices' alt settings be controllable from the user space. Limiting this usage in the scope of BT makes more sense to me. > > -- > Luiz Augusto von Dentz
Hi Hsin-chen, On Mon, Feb 10, 2025 at 11:23 PM Hsin-chen Chuang <chharry@google.com> wrote: > > Hi Luiz, > > Thanks for the feedback. > > On Tue, Feb 11, 2025 at 2:17 AM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Hsin-chen, > > > > On Mon, Feb 10, 2025 at 5:32 AM Hsin-chen Chuang <chharry@google.com> wrote: > > > > > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > > > The functionality was completed in commit 5e5c3898ef49 ("Bluetooth: Fix > > > possible race with userspace of sysfs isoc_alt") > > > > > > Fixes: 5e5c3898ef49 ("Bluetooth: Fix possible race with userspace of sysfs isoc_alt") > > > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org> > > > --- > > > > > > (no changes since v1) > > > > > > Documentation/ABI/stable/sysfs-class-bluetooth | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth > > > index 36be02471174..1168e0318e35 100644 > > > --- a/Documentation/ABI/stable/sysfs-class-bluetooth > > > +++ b/Documentation/ABI/stable/sysfs-class-bluetooth > > > @@ -7,3 +7,15 @@ Description: This write-only attribute allows users to trigger the vendor reset > > > The reset may or may not be done through the device transport > > > (e.g., UART/USB), and can also be done through an out-of-band > > > approach such as GPIO. > > > + > > > +What: /sys/class/bluetooth/hci<index>/isoc_alt > > > +Date: 10-Feb-2025 > > > +KernelVersion: 6.13 > > > +Contact: linux-bluetooth@vger.kernel.org > > > +Description: This attribute allows users to configure the USB Alternate setting > > > + for the specific HCI device. Reading this attribute returns the > > > + current setting, and writing any supported numbers would change > > > + the setting. See the USB Alternate setting definition in Bluetooth > > > + core spec 5, vol 4, part B, table 2.1. > > > + If the data is not a valid number, the write fails with -EINVAL. > > > + The other failures are vendor specific. > > > > Still not really convinced this is the right way to expose it, it is > > not an HCI attribute to begin with, not sure if we couldn't perhaps > > Could you tell more about why this is not an HCI attr to begin with? > The alt setting is bonded to the USB device which is now under btusb's > control, and btusb creates a sysfs node for this. This attr location > decision seems natural to me. Well alt setting is (obviously) not covered as part of HCI specification, USB is one possible transport bus of HCI, but that itself is already saying that we would be mixing the transport with actual HCI protocol. > > add another node to control it or add via USB device? > > I feel allowing this in the USB sysfs API might be an overkill as that > makes all USB devices' alt settings be controllable from the user > space. Limiting this usage in the scope of BT makes more sense to me. Not all, just btusb ones, that said what we could add another level to control transport specific attribute e.g /sys/class/bluetooth/usb#/isoc_alt than that becomes the parent of /sys/class/bluetooth/hci<index> > > > > -- > > Luiz Augusto von Dentz > > -- > Best Regards, > Hsin-chen
Hi Luiz, Thanks, that makes sense to me. I'll resolve this comment in the next version. On Wed, Feb 12, 2025 at 6:01 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Hsin-chen, > > On Mon, Feb 10, 2025 at 11:23 PM Hsin-chen Chuang <chharry@google.com> wrote: > > > > Hi Luiz, > > > > Thanks for the feedback. > > > > On Tue, Feb 11, 2025 at 2:17 AM Luiz Augusto von Dentz > > <luiz.dentz@gmail.com> wrote: > > > > > > Hi Hsin-chen, > > > > > > On Mon, Feb 10, 2025 at 5:32 AM Hsin-chen Chuang <chharry@google.com> wrote: > > > > > > > > From: Hsin-chen Chuang <chharry@chromium.org> > > > > > > > > The functionality was completed in commit 5e5c3898ef49 ("Bluetooth: Fix > > > > possible race with userspace of sysfs isoc_alt") > > > > > > > > Fixes: 5e5c3898ef49 ("Bluetooth: Fix possible race with userspace of sysfs isoc_alt") > > > > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org> > > > > --- > > > > > > > > (no changes since v1) > > > > > > > > Documentation/ABI/stable/sysfs-class-bluetooth | 12 ++++++++++++ > > > > 1 file changed, 12 insertions(+) > > > > > > > > diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth > > > > index 36be02471174..1168e0318e35 100644 > > > > --- a/Documentation/ABI/stable/sysfs-class-bluetooth > > > > +++ b/Documentation/ABI/stable/sysfs-class-bluetooth > > > > @@ -7,3 +7,15 @@ Description: This write-only attribute allows users to trigger the vendor reset > > > > The reset may or may not be done through the device transport > > > > (e.g., UART/USB), and can also be done through an out-of-band > > > > approach such as GPIO. > > > > + > > > > +What: /sys/class/bluetooth/hci<index>/isoc_alt > > > > +Date: 10-Feb-2025 > > > > +KernelVersion: 6.13 > > > > +Contact: linux-bluetooth@vger.kernel.org > > > > +Description: This attribute allows users to configure the USB Alternate setting > > > > + for the specific HCI device. Reading this attribute returns the > > > > + current setting, and writing any supported numbers would change > > > > + the setting. See the USB Alternate setting definition in Bluetooth > > > > + core spec 5, vol 4, part B, table 2.1. > > > > + If the data is not a valid number, the write fails with -EINVAL. > > > > + The other failures are vendor specific. > > > > > > Still not really convinced this is the right way to expose it, it is > > > not an HCI attribute to begin with, not sure if we couldn't perhaps > > > > Could you tell more about why this is not an HCI attr to begin with? > > The alt setting is bonded to the USB device which is now under btusb's > > control, and btusb creates a sysfs node for this. This attr location > > decision seems natural to me. > > Well alt setting is (obviously) not covered as part of HCI > specification, USB is one possible transport bus of HCI, but that > itself is already saying that we would be mixing the transport with > actual HCI protocol. > > > > add another node to control it or add via USB device? > > > > I feel allowing this in the USB sysfs API might be an overkill as that > > makes all USB devices' alt settings be controllable from the user > > space. Limiting this usage in the scope of BT makes more sense to me. > > Not all, just btusb ones, that said what we could add another level to > control transport specific attribute e.g > /sys/class/bluetooth/usb#/isoc_alt than that becomes the parent of > /sys/class/bluetooth/hci<index> > > > > > > > -- > > > Luiz Augusto von Dentz > > > > -- > > Best Regards, > > Hsin-chen > > > > -- > Luiz Augusto von Dentz
diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth index 36be02471174..1168e0318e35 100644 --- a/Documentation/ABI/stable/sysfs-class-bluetooth +++ b/Documentation/ABI/stable/sysfs-class-bluetooth @@ -7,3 +7,15 @@ Description: This write-only attribute allows users to trigger the vendor reset The reset may or may not be done through the device transport (e.g., UART/USB), and can also be done through an out-of-band approach such as GPIO. + +What: /sys/class/bluetooth/hci<index>/isoc_alt +Date: 10-Feb-2025 +KernelVersion: 6.13 +Contact: linux-bluetooth@vger.kernel.org +Description: This attribute allows users to configure the USB Alternate setting + for the specific HCI device. Reading this attribute returns the + current setting, and writing any supported numbers would change + the setting. See the USB Alternate setting definition in Bluetooth + core spec 5, vol 4, part B, table 2.1. + If the data is not a valid number, the write fails with -EINVAL. + The other failures are vendor specific.