On Tue, Feb 18, 2025 at 06:01:42PM +0800, Hsin-chen Chuang wrote:

Hi Greg,

On Tue, Feb 18, 2025 at 5:21 PM Greg KH [off-list ref] wrote:

quoted

On Tue, Feb 18, 2025 at 04:57:38PM +0800, Hsin-chen Chuang wrote:

quoted

Hi Greg,

On Tue, Feb 18, 2025 at 4:23 PM Greg KH [off-list ref] wrote:

quoted

On Tue, Feb 18, 2025 at 12:24:07PM +0800, Hsin-chen Chuang wrote:

quoted

Hi Greg,

On Mon, Feb 17, 2025 at 4:53 PM Greg KH [off-list ref] wrote:

quoted

On Mon, Feb 17, 2025 at 04:44:35PM +0800, Hsin-chen Chuang wrote:

quoted

On Fri, Feb 14, 2025 at 7:37 PM Greg KH [off-list ref] wrote:

quoted

On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:

quoted

From: Hsin-chen Chuang <redacted>

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.

If you are changing the alternate USB configuration, all state should be
dropped, right?  If not, huh how does the device know to keep that
state?

No, the Bluetooth chip doesn't drop any info when the alt is changed.
It only affects the data transfer bandwidth on that interface.

quoted

quoted

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".

Your ethernet interface does not contain state like this, we handle
chainging IP addresses and devices all the time, so perhaps wrong
analogy :)

quoted

quoted

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.

So why not use that same control channel in your code?  Why are you

Because we're using the User channel, and they can't be used at the same time.

This doesn't make sense.  Either BlueZ has this same problem, or it
doesn't.  As you say it does not, then again, why can't you use the
exact same user/kernel api to achieve this?

The user/kernel api is "fixed" right now, if you wish to replace the
userspace side of the BlueZ code with your own, then you should/must use
that same user/kernel api.  Don't go adding duplicate interfaces please.

quoted

reinventing a new control channel for something that is obviously there
already?

Not quite the same as "reinventing". The Control channel command does
much more than just setting the alt; It just doesn't work with the
User channel.

quoted

So in short, what's preventing you from using the same exact driver
callbacks, OR the same exact kernel api.  Surely you all are not

The answer is the same as the above. This feature is missing in the
User channel, and I'm completing it with this patch.

Again, that seems to be your userspace's issue, not the kernel's.  Just
use the same api that bluez uses here.

quoted

replacing all of the in-kernel BlueZ code with an external kernel driver
just for this, right?  If so, that's not ok at all.

Sorry I don't quite get it. What do you mean by the external kernel driver?

You said you are not using the bluez kernel code, right?  So you must
have some kernel code to implement this instead for the same
functionality.  Otherwise you are using the bluez kernel api here.

Again, just use the same api please, don't go adding new one-off apis
through sysfs for this when it is not needed.

I'll also step back further and say, why not use bluez?  What is so
wrong with that that you all need a totally different bluetooth stack?
Why not just fix the bluez code for anything that is currently missing
or lacking there that required you to write a new one.

And yes, I know the inclination of Android to constantly rewrite the
bluetooth stack, it's on the what, third or fourth iteration already?
What's to guarantee that this really will be the last one?  :)

thanks,

greg k-h