Re: [PATCH v5] Bluetooth: Fix possible race with userspace of sysfs isoc_alt

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Greg,

On Tue, Feb 18, 2025 at 4:23 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Feb 18, 2025 at 12:24:07PM +0800, Hsin-chen Chuang wrote:
> > Hi Greg,
> >
> > On Mon, Feb 17, 2025 at 4:53 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Mon, Feb 17, 2025 at 04:44:35PM +0800, Hsin-chen Chuang wrote:
> > > > On Fri, Feb 14, 2025 at 7:37 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:
> > > > > > From: Hsin-chen Chuang <chharry@xxxxxxxxxxxx>
> > > > > >
> > > > > > Expose the isoc_alt attr with device group to avoid the racing.
> > > > > >
> > > > > > Now we create a dev node for btusb. The isoc_alt attr belongs to it and
> > > > > > it also becomes the parent device of hci dev.
> > > > > >
> > > > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > > > >
> > > > > Wait, step back, why is this commit needed if you can change the alt
> > > > > setting already today through usbfs/libusb without needing to mess with
> > > > > the bluetooth stack at all?
> > > >
> > > > In short: We want to configure the alternate settings without
> > > > detaching the btusb driver, while detaching seems necessary for
> > > > libusb_set_interface_alt_setting to work (Please correct me if I'm
> > > > wrong!)
> > >
> > > I think changing the alternate setting should work using usbfs as you
> > > would send that command to the device, not the interface, so the driver
> > > bound to the existing interface would not need to be removed.
> >
> > I thought USBDEVFS_SETINTERFACE was the right command to begin with,
> > but it seems not working in this case.
> > The command itself attempts to claim the interface, but the interface
> > is already claimed by btusb so it failed with Device or resource busy
> >
> > drivers/usb/core/devio.c:
> >   USBDEVFS_SETINTERFACE -> proc_setintf -> checkintf -> claimintf
>
> Ah, ok, thanks for checking.  So as you control this device, why not
> just disconnect it, change the setting, and then reconnect it?

After dis/reconnecting, a Bluetooth chipset would lose all its state:
Existing connections/scanners/advertisers are all dropped.
This is as bad as (just an analogy) "Whenever you access a http web
page, you need to bring your ethernet interface down and up, and after
the page is downloaded, do that again".

>
> Also, see my other review comment, how does BlueZ do this today?

BlueZ handles that in their MGMT command, that is, through Control
channel -> BlueZ kernel space code -> driver callbacks.
Once a Bluetooth chipset is opened with the User channel, it can't be
used with the Control channel simultaneously, and vice versa.

-- 
Best Regards,
Hsin-chen





[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux