Hi Luiz,
On Thu, Feb 27, 2025 at 11:59 PM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> Hi Hsin-chen,
>
> On Thu, Feb 27, 2025 at 10:37 AM Hsin-chen Chuang <chharry@xxxxxxxxxx> wrote:
> >
> > Hi Luiz,
> >
> > On Thu, Feb 27, 2025 at 10:37 PM Luiz Augusto von Dentz
> > <luiz.dentz@xxxxxxxxx> wrote:
> > >
> > > Hi Hsin-chen,
> > >
> > > On Wed, Feb 26, 2025 at 9:22 PM Hsin-chen Chuang <chharry@xxxxxxxxxx> wrote:
> > > >
> > > > Hi Luiz,
> > > >
> > > > On Thu, Feb 27, 2025 at 4:55 AM Luiz Augusto von Dentz
> > > > <luiz.dentz@xxxxxxxxx> wrote:
> > > > >
> > > > > Hi Hsin-chen,
> > > > >
> > > > > On Mon, Feb 24, 2025 at 2:13 AM Hsin-chen Chuang <chharry@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > On Mon, Feb 24, 2025 at 2:44 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > On Mon, Feb 24, 2025 at 02:25:52PM +0800, Hsin-chen Chuang wrote:
> > > > > > > > Hi Greg,
> > > > > > > >
> > > > > > > > On Mon, Feb 24, 2025 at 2:10 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > > > > > >
> > > > > > > > > On Mon, Feb 24, 2025 at 12:52:32PM +0800, Hsin-chen Chuang wrote:
> > > > > > > > > > From: Hsin-chen Chuang <chharry@xxxxxxxxxxxx>
> > > > > > > > > >
> > > > > > > > > > Automatically configure the altsetting for USER_CHANNEL when a SCO is
> > > > > > > > > > connected. This adds support for the USER_CHANNEL to transfer SCO data
> > > > > > > > > > over USB transport.
> > > > > > > > > >
> > > > > > > > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > > > > > > > > > Signed-off-by: Hsin-chen Chuang <chharry@xxxxxxxxxxxx>
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > Changes in v2:
> > > > > > > > > > - Give up tracking the SCO handles. Only configure the altsetting when
> > > > > > > > > > SCO connected.
> > > > > > > > > > - Put the change behind Kconfig/module parameter
> > > > > > > > > >
> > > > > > > > > > drivers/bluetooth/Kconfig | 11 ++++++++++
> > > > > > > > > > drivers/bluetooth/btusb.c | 46 +++++++++++++++++++++++++++++++++++++++
> > > > > > > > > > 2 files changed, 57 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > > > > > > > > > index 4ab32abf0f48..7c497f878732 100644
> > > > > > > > > > --- a/drivers/bluetooth/Kconfig
> > > > > > > > > > +++ b/drivers/bluetooth/Kconfig
> > > > > > > > > > @@ -56,6 +56,17 @@ config BT_HCIBTUSB_POLL_SYNC
> > > > > > > > > > Say Y here to enable USB poll_sync for Bluetooth USB devices by
> > > > > > > > > > default.
> > > > > > > > > >
> > > > > > > > > > +config BT_HCIBTUSB_AUTO_SET_ISOC_ALT
> > > > > > > > > > + bool "Auto set isoc_altsetting for USER_CHANNEL when SCO connected"
> > > > > > > > > > + depends on BT_HCIBTUSB
> > > > > > > > > > + default n
> > > > >
> > > > > Maybe we can do just:
> > > > >
> > > > > default y if CHROME_PLATFORMS
> > > > >
> > > > > > > > > > + help
> > > > > > > > > > + Say Y here to enable auto set isoc_altsetting for USER_CHANNEL
> > > > > > > > > > + when SCO connected
> > > > > > > > > > +
> > > > > > > > > > + This can be overridden by passing btusb.auto_set_isoc_alt=[y|n]
> > > > > > > > > > + on the kernel commandline.
> > > > > > > > > > +
> > > > > > > > > > config BT_HCIBTUSB_BCM
> > > > > > > > > > bool "Broadcom protocol support"
> > > > > > > > > > depends on BT_HCIBTUSB
> > > > > > > > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > > > > > > > > index de3fa725d210..af93d757911b 100644
> > > > > > > > > > --- a/drivers/bluetooth/btusb.c
> > > > > > > > > > +++ b/drivers/bluetooth/btusb.c
> > > > > > > > > > @@ -34,6 +34,8 @@ static bool force_scofix;
> > > > > > > > > > static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
> > > > > > > > > > static bool enable_poll_sync = IS_ENABLED(CONFIG_BT_HCIBTUSB_POLL_SYNC);
> > > > > > > > > > static bool reset = true;
> > > > > > > > > > +static bool auto_set_isoc_alt =
> > > > > > > > > > + IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTO_SET_ISOC_ALT);
> > > > > > > > > >
> > > > > > > > > > static struct usb_driver btusb_driver;
> > > > > > > > > >
> > > > > > > > > > @@ -1113,6 +1115,42 @@ static inline void btusb_free_frags(struct btusb_data *data)
> > > > > > > > > > spin_unlock_irqrestore(&data->rxlock, flags);
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > +static void btusb_sco_connected(struct btusb_data *data, struct sk_buff *skb)
> > > > > > > > > > +{
> > > > > > > > > > + struct hci_event_hdr *hdr = (void *) skb->data;
> > > > > > > > > > + struct hci_ev_sync_conn_complete *ev =
> > > > > > > > > > + (void *) skb->data + sizeof(*hdr);
> > > > > > > > > > + struct hci_dev *hdev = data->hdev;
> > > > > > > > > > + unsigned int notify_air_mode;
> > > > > > > > > > +
> > > > > > > > > > + if (hci_skb_pkt_type(skb) != HCI_EVENT_PKT)
> > > > > > > > > > + return;
> > > > > > > > > > +
> > > > > > > > > > + if (skb->len < sizeof(*hdr) || hdr->evt != HCI_EV_SYNC_CONN_COMPLETE)
> > > > > > > > > > + return;
> > > > > > > > > > +
> > > > > > > > > > + if (skb->len != sizeof(*hdr) + sizeof(*ev) || ev->status)
> > > > > > > > > > + return;
> > > > > > > > > > +
> > > > > > > > > > + switch (ev->air_mode) {
> > > > > > > > > > + case BT_CODEC_CVSD:
> > > > > > > > > > + notify_air_mode = HCI_NOTIFY_ENABLE_SCO_CVSD;
> > > > > > > > > > + break;
> > > > > > > > > > +
> > > > > > > > > > + case BT_CODEC_TRANSPARENT:
> > > > > > > > > > + notify_air_mode = HCI_NOTIFY_ENABLE_SCO_TRANSP;
> > > > > > > > > > + break;
> > > > > > > > > > +
> > > > > > > > > > + default:
> > > > > > > > > > + return;
> > > > > > > > > > + }
> > > > > > > > > > +
> > > > > > > > > > + bt_dev_info(hdev, "enabling SCO with air mode %u", ev->air_mode);
> > > > > > > > > > + data->sco_num = 1;
> > > > > > > > > > + data->air_mode = notify_air_mode;
> > > > > > > > > > + schedule_work(&data->work);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > static int btusb_recv_event(struct btusb_data *data, struct sk_buff *skb)
> > > > > > > > > > {
> > > > > > > > > > if (data->intr_interval) {
> > > > > > > > > > @@ -1120,6 +1158,11 @@ static int btusb_recv_event(struct btusb_data *data, struct sk_buff *skb)
> > > > > > > > > > schedule_delayed_work(&data->rx_work, 0);
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > + /* Configure altsetting for HCI_USER_CHANNEL on SCO connected */
> > > > > > > > > > + if (auto_set_isoc_alt &&
> > > > > > > > > > + hci_dev_test_flag(data->hdev, HCI_USER_CHANNEL))
> > > > > > > > > > + btusb_sco_connected(data, skb);
> > > > > > > > > > +
> > > > > > > > > > return data->recv_event(data->hdev, skb);
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > @@ -4354,6 +4397,9 @@ MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> > > > > > > > > > module_param(reset, bool, 0644);
> > > > > > > > > > MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> > > > > > > > > >
> > > > > > > > > > +module_param(auto_set_isoc_alt, bool, 0644);
> > > > > > > > > > +MODULE_PARM_DESC(auto_set_isoc_alt, "Auto set isoc_altsetting for USER_CHANNEL when SCO connected");
> > > > > > > > >
> > > > > > > > > This is not the 1990's, why are you adding new module parameters when we
> > > > > > > > > have so many other more proper ways to do this? And really, this would
> > > > > > > >
> > > > > > > > Sorry but could you please provide an example to guard a feature like this.
> > > > > > >
> > > > > > > Depends on what you want to do with this configuration. Why is it an
> > > > > > > option at all? Why can't it "just work"? Module parameters are a pain
> > > > > >
> > > > > > I would like to hand this question to Luiz. I believe this patch just
> > > > > > works because this configuration is defined in the spec.
> > > > > > I think Luiz's point is to project the potential existing user, but
> > > > > > there's probably no User channel user sending SCO data with the latest
> > > > > > btusb driver because:
> > > > > > a) There's no way to configure alt setting from userspace
> > > > > > b) Before eafcfcfca97d, SCO data would be rejected since User channel
> > > > > > shouldn't be able to modify hci_conn_num
> > > > >
> > > > > Perhaps you can just use CHROME_PLATFORMS (suggested above) in Kconfig
> > > > > to enable intercepting of the events, etc, so we don't need any
> > > > > runtime parameters.
> > > >
> > > > I'm afraid that this doesn't resolve Greg's comment below because the
> > > > multiple controllers are still bonded to the same config.
> > >
> > > Well that would be enabled for every controller plugged into the system.
> >
> > To clarify, I'm totally fine with runtime parameters and/or any
> > Kconfig like CHROME_PLATFORMS, although I'd prefer the current patch
> > more.
> >
> > But I guess Greg's point is that the multiple controllers should be
> > able to have different configurations at the same time. I'd respect
> > your decision to accept this patch or another patch with
> > CHROME_PLATFORMS, but I guess it's better to convince Greg first.
>
> Perhaps I got Greg's response wrong, but I think he was suggesting
> that it should just work without any special configuration, which is
> what I'm suggesting here, the use of CHROME_PLATFORMS is more of a
> stopgap until proper test automation is introduced.
>
> You can already have different configurations with respect to
> HCI_CHANNEL_USER since that is done per controller, btw that is used
> by the likes of btproxy tool for example which is normally used to
> hook the controller into a VM.
Thanks for the explanation. I'll follow the suggestion in v3.
Best Regards,
Hsin-chen
