On Thu, Feb 13, 2025 at 10:56:46AM -0500, Luiz Augusto von Dentz wrote:

Hi Greg,

On Thu, Feb 13, 2025 at 10:39 AM Greg KH [off-list ref] wrote:

quoted

On Thu, Feb 13, 2025 at 09:22:28AM -0500, Luiz Augusto von Dentz wrote:

quoted

Hi Greg,

On Thu, Feb 13, 2025 at 8:45 AM Greg KH [off-list ref] wrote:

quoted

On Thu, Feb 13, 2025 at 09:33:34PM +0800, Hsin-chen Chuang wrote:

quoted

On Thu, Feb 13, 2025 at 8:10 PM Greg KH [off-list ref] wrote:

quoted

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Thu, Feb 13, 2025 at 07:57:15PM +0800, Hsin-chen Chuang wrote:

quoted

The btusb driver data is allocated by devm_kzalloc and is
automatically freed on driver detach, so I guess we don't have
anything to do here.

What?  A struct device should NEVER be allocated with devm_kzalloc.
That's just not going to work at all.

Noted. Perhaps that needs to be refactored together.

quoted

quoted

Or perhaps we should move btusb_disconnect's content here? Luiz, what
do you think?

I think something is really wrong here.  Why are you adding a new struct
device to the system?  What requires that?  What is this new device
going to be used for?

The new device is only for exposing a new sysfs attribute.

That feels crazy.

quoted

So originally we had a device called hci_dev, indicating the
implementation of the Bluetooth HCI layer. hci_dev is directly the
child of the usb_interface (the Bluetooth chip connected through USB).
Now I would like to add an attribute for something that's not defined
in the HCI layer, but lower layer only in Bluetooth USB.
Thus we want to rephrase the structure: usb_interface -> btusb (new
device) -> hci_dev, and then we could place the new attribute in the
new device.

Basically I kept the memory management in btusb unchanged in this
patch, as the new device is only used for a new attribute.
Would you suggest we revise the memory management since we added a
device in this module?

If you add a new device in the tree, it HAS to work properly with the
driver core (i.e. life cycles are unique, you can't have empty release
functions, etc.)  Put it on the proper bus it belongs to, bind the
needed drivers to it, and have it work that way, don't make a "fake"
device for no good reason.

Well we could just introduce it to USB device, since alternate setting
is a concept that is coming from there, but apparently the likes of
/sys/bus/usb/devices/usbX/bAlternateSetting is read-only, some
Bluetooth profiles (HFP) requires switching the alternate setting and
because Google is switching to handle this via userspace thus why
there was this request to add a new sysfs to control it.

That's fine, just don't add devices where there shouldn't be devices, or
if you do, make them actually work properly (i.e. do NOT have empty
release callbacks...)

If you want to switch alternate settings in a USB device, do it the
normal way from userspace that has been there for decades!  Don't make
up some other random sysfs file for this please, that would be crazy,
and wrong.

So what's wrong with the current api we have today that doesn't work for
bluetooth devices?

Perhaps it is just lack of knowledge then, how userspace can request
to switch alternate settings? If I recall correctly Hsin-chen tried
with libusb, or something like that, but it didn't work for some
reason.

The USBDEVFS_SETINTERFACE usbfs ioctl should do this.  If not, pleaase
let us know.  libusb supports this through the
libusb_set_interface_alt_setting() function.

Here's an 11 year old stackoverflow question about this:
	https://stackoverflow.com/questions/17546709/what-does-libusb-set-interface-alt-setting-do

thanks,

greg k-h