Re: [PATCH] Bluetooth: btusb: Configure altsetting for USER_CHANNEL

[Hsin-chen Chuang]

Hi Luiz,

On Mon, Feb 24, 2025 at 11:36 AM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> Hi Hsin-chen,
>
> On Sun, Feb 23, 2025 at 10:21 PM Hsin-chen Chuang <chharry@xxxxxxxxxx> wrote:
> >
> > Hi Luiz,
> >
> > On Mon, Feb 24, 2025 at 10:53 AM Luiz Augusto von Dentz
> > <luiz.dentz@xxxxxxxxx> wrote:
> > >
> > > Hi Hsin-chen,
> > >
> > > On Sun, Feb 23, 2025 at 9:26 PM Hsin-chen Chuang <chharry@xxxxxxxxxx> wrote:
> > > >
> > > > From: Hsin-chen Chuang <chharry@xxxxxxxxxxxx>
> > > >
> > > > Automatically configure the altsetting for USER_CHANNEL when a SCO is
> > > > connected or disconnected. This adds support for the USER_CHANNEL to
> > > > transfer SCO data over USB transport.
> > >
> > > I guess the tracking of handles is about handling disconnections,
> > > right? I wonder if we can get away without doing that, I didn't intend
> > > to add a whole bunch of changes in order to switch to the right mode,
> > > I get that you may want to disable the isochronous endpoint in case
> > > there is no connection, but I guess that only matters if we are
> > > talking about power but the impact is probably small so I don't think
> > > it is worth it. There is an alternative to match the SCO/eSCO handle
> > > via mask, like we do with ISO handles, that is probably a lot cheaper
> > > than attempting to add a whole list for tracking handles, but it has
> > > the downside that it is vendor/model specific.
> >
> > Yes, that's for handling disconnection. What do you think if we keep
> > only one handle in the driver data? That is, assume there's at most
> > one SCO connection. Then we don't need a list but just a u16.
>
> Hmm, if you can guarantee that it only support at most 1 SCO
> connection that is fine, that said the user channel can be
> opened/closed in between so we would have to monitor things like reset
> as well, so I think we actually need to depend on the Kconfig/module
> parameter to ensure that only user mode will be used so it is safe to
> track the handle, that said I think you will need to intercept things
> like reset anyway since it does affect any handles the driver would
> have stored so you probably need to change the alt setting in case an
> SCO connection was established.

Thanks for the explanation and I understood your concern. Indeed
tracking handles would require way too much effort to ensure it's
correct. I will follow your first approach to keep it simple for now.


>
> Btw, if we really want to be safe here we should probably think about
> ways to test loading the btusb on bluez CI and adding testing to it,
> that said that would require emulation to USB vid/pid and possibly the
> vendor commands necessary in order to set up the controller.
>
> > >
> > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > > > Signed-off-by: Hsin-chen Chuang <chharry@xxxxxxxxxxxx>
> > > > ---
> > > >
> > > >  drivers/bluetooth/btusb.c | 224 +++++++++++++++++++++++++++++++-------
> > > >  1 file changed, 186 insertions(+), 38 deletions(-)
> > > >
> > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > > index de3fa725d210..dfb0918dfe98 100644
> > > > --- a/drivers/bluetooth/btusb.c
> > > > +++ b/drivers/bluetooth/btusb.c
> > > > @@ -854,6 +854,11 @@ struct qca_dump_info {
> > > >  #define BTUSB_ALT6_CONTINUOUS_TX       16
> > > >  #define BTUSB_HW_SSR_ACTIVE    17
> > > >
> > > > +struct sco_handle_list {
> > > > +       struct list_head list;
> > > > +       u16 handle;
> > > > +};
> > > > +
> > > >  struct btusb_data {
> > > >         struct hci_dev       *hdev;
> > > >         struct usb_device    *udev;
> > > > @@ -920,6 +925,9 @@ struct btusb_data {
> > > >         int oob_wake_irq;   /* irq for out-of-band wake-on-bt */
> > > >
> > > >         struct qca_dump_info qca_dump;
> > > > +
> > > > +       /* Records the exsiting SCO handles for HCI_USER_CHANNEL */
> > > > +       struct list_head sco_handle_list;
> > > >  };
> > > >
> > > >  static void btusb_reset(struct hci_dev *hdev)
> > > > @@ -1113,6 +1121,131 @@ static inline void btusb_free_frags(struct btusb_data *data)
> > > >         spin_unlock_irqrestore(&data->rxlock, flags);
> > > >  }
> > > >
> > > > +static void btusb_sco_handle_list_clear(struct list_head *list)
> > > > +{
> > > > +       struct sco_handle_list *entry, *n;
> > > > +
> > > > +       list_for_each_entry_safe(entry, n, list, list) {
> > > > +               list_del(&entry->list);
> > > > +               kfree(entry);
> > > > +       }
> > > > +}
> > > > +
> > > > +static struct sco_handle_list *btusb_sco_handle_list_find(
> > > > +       struct list_head *list,
> > > > +       u16 handle)
> > > > +{
> > > > +       struct sco_handle_list *entry;
> > > > +
> > > > +       list_for_each_entry(entry, list, list)
> > > > +               if (entry->handle == handle)
> > > > +                       return entry;
> > > > +
> > > > +       return NULL;
> > > > +}
> > > > +
> > > > +static int btusb_sco_handle_list_add(struct list_head *list, u16 handle)
> > > > +{
> > > > +       struct sco_handle_list *entry;
> > > > +
> > > > +       if (btusb_sco_handle_list_find(list, handle))
> > > > +               return -EEXIST;
> > > > +
> > > > +       entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> > > > +       if (!entry)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       entry->handle = handle;
> > > > +       list_add(&entry->list, list);
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static int btusb_sco_handle_list_del(struct list_head *list, u16 handle)
> > > > +{
> > > > +       struct sco_handle_list *entry;
> > > > +
> > > > +       entry = btusb_sco_handle_list_find(list, handle);
> > > > +       if (!entry)
> > > > +               return -ENOENT;
> > > > +
> > > > +       list_del(&entry->list);
> > > > +       kfree(entry);
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static void btusb_sco_conn_change(struct btusb_data *data, struct sk_buff *skb)
> > > > +{
> > > > +       struct hci_event_hdr *hdr = (void *) skb->data;
> > > > +       struct hci_dev *hdev = data->hdev;
> > > > +
> > > > +       if (hci_skb_pkt_type(skb) != HCI_EVENT_PKT || skb->len < sizeof(*hdr))
> > > > +               return;
> > > > +
> > > > +       switch (hdr->evt) {
> > > > +       case HCI_EV_DISCONN_COMPLETE: {
> > > > +               struct hci_ev_disconn_complete *ev =
> > > > +                       (void *) skb->data + sizeof(*hdr);
> > > > +               u16 handle;
> > > > +
> > > > +               if (skb->len != sizeof(*hdr) + sizeof(*ev) || ev->status)
> > > > +                       return;
> > > > +
> > > > +               handle = __le16_to_cpu(ev->handle);
> > > > +               if (btusb_sco_handle_list_del(&data->sco_handle_list,
> > > > +                                             handle) < 0)
> > > > +                       return;
> > > > +
> > > > +               bt_dev_info(hdev, "disabling SCO");
> > > > +               data->sco_num--;
> > > > +               data->air_mode = HCI_NOTIFY_DISABLE_SCO;
> > > > +               schedule_work(&data->work);
> > > > +
> > > > +               break;
> > > > +       }
> > > > +       case HCI_EV_SYNC_CONN_COMPLETE: {
> > > > +               struct hci_ev_sync_conn_complete *ev =
> > > > +                       (void *) skb->data + sizeof(*hdr);
> > > > +               unsigned int notify_air_mode;
> > > > +               u16 handle;
> > > > +
> > > > +               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;
> > > > +               }
> > > > +
> > > > +               handle = __le16_to_cpu(ev->handle);
> > > > +               if (btusb_sco_handle_list_add(&data->sco_handle_list,
> > > > +                                             handle) < 0) {
> > > > +                       bt_dev_err(hdev, "failed to add the new SCO handle");
> > > > +                       return;
> > > > +               }
> > > > +
> > > > +               bt_dev_info(hdev, "enabling SCO with air mode %u",
> > > > +                           ev->air_mode);
> > > > +               data->sco_num++;
> > > > +               data->air_mode = notify_air_mode;
> > > > +               schedule_work(&data->work);
> > > > +
> > > > +               break;
> > > > +       }
> > > > +       default:
> > > > +               break;
> > > > +       }
> > > > +}
> > > > +
> > > >  static int btusb_recv_event(struct btusb_data *data, struct sk_buff *skb)
> > > >  {
> > > >         if (data->intr_interval) {
> > > > @@ -1120,6 +1253,10 @@ 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 dis/connected */
> > > > +       if (hci_dev_test_flag(data->hdev, HCI_USER_CHANNEL))
> > > > +               btusb_sco_conn_change(data, skb);
> > >
> > > I'd recommend adding a check for Kconfig/module parameter in the if
> > > statement so btusb_sco_conn_change only runs on distros with it
> > > enabled so we don't risk something not working as expected just
> > > because someone decided to open the user channel.
> >
> > Sure will add it in the next patch.
> >
> > >
> > > >         return data->recv_event(data->hdev, skb);
> > > >  }
> > > >
> > > > @@ -1919,44 +2056,6 @@ static void btusb_stop_traffic(struct btusb_data *data)
> > > >         usb_kill_anchored_urbs(&data->ctrl_anchor);
> > > >  }
> > > >
> > > > -static int btusb_close(struct hci_dev *hdev)
> > > > -{
> > > > -       struct btusb_data *data = hci_get_drvdata(hdev);
> > > > -       int err;
> > > > -
> > > > -       BT_DBG("%s", hdev->name);
> > > > -
> > > > -       cancel_delayed_work(&data->rx_work);
> > > > -       cancel_work_sync(&data->work);
> > > > -       cancel_work_sync(&data->waker);
> > > > -
> > > > -       skb_queue_purge(&data->acl_q);
> > > > -
> > > > -       clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> > > > -       clear_bit(BTUSB_BULK_RUNNING, &data->flags);
> > > > -       clear_bit(BTUSB_INTR_RUNNING, &data->flags);
> > > > -       clear_bit(BTUSB_DIAG_RUNNING, &data->flags);
> > > > -
> > > > -       btusb_stop_traffic(data);
> > > > -       btusb_free_frags(data);
> > > > -
> > > > -       err = usb_autopm_get_interface(data->intf);
> > > > -       if (err < 0)
> > > > -               goto failed;
> > > > -
> > > > -       data->intf->needs_remote_wakeup = 0;
> > > > -
> > > > -       /* Enable remote wake up for auto-suspend */
> > > > -       if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
> > > > -               data->intf->needs_remote_wakeup = 1;
> > > > -
> > > > -       usb_autopm_put_interface(data->intf);
> > > > -
> > > > -failed:
> > > > -       usb_scuttle_anchored_urbs(&data->deferred);
> > > > -       return 0;
> > > > -}
> > > > -
> > > >  static int btusb_flush(struct hci_dev *hdev)
> > > >  {
> > > >         struct btusb_data *data = hci_get_drvdata(hdev);
> > > > @@ -2327,6 +2426,51 @@ static void btusb_work(struct work_struct *work)
> > > >         }
> > > >  }
> > > >
> > > > +static int btusb_close(struct hci_dev *hdev)
> > > > +{
> > > > +       struct btusb_data *data = hci_get_drvdata(hdev);
> > > > +       int err;
> > > > +
> > > > +       BT_DBG("%s", hdev->name);
> > > > +
> > > > +       if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
> > > > +               btusb_sco_handle_list_clear(&data->sco_handle_list);
> > > > +               data->sco_num = 0;
> > > > +               if (data->isoc_altsetting != 0)
> > > > +                       btusb_switch_alt_setting(hdev, 0);
> > > > +       }
> > > > +
> > > > +       cancel_delayed_work(&data->rx_work);
> > > > +       cancel_work_sync(&data->work);
> > > > +       cancel_work_sync(&data->waker);
> > > > +
> > > > +       skb_queue_purge(&data->acl_q);
> > > > +
> > > > +       clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> > > > +       clear_bit(BTUSB_BULK_RUNNING, &data->flags);
> > > > +       clear_bit(BTUSB_INTR_RUNNING, &data->flags);
> > > > +       clear_bit(BTUSB_DIAG_RUNNING, &data->flags);
> > > > +
> > > > +       btusb_stop_traffic(data);
> > > > +       btusb_free_frags(data);
> > > > +
> > > > +       err = usb_autopm_get_interface(data->intf);
> > > > +       if (err < 0)
> > > > +               goto failed;
> > > > +
> > > > +       data->intf->needs_remote_wakeup = 0;
> > > > +
> > > > +       /* Enable remote wake up for auto-suspend */
> > > > +       if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
> > > > +               data->intf->needs_remote_wakeup = 1;
> > > > +
> > > > +       usb_autopm_put_interface(data->intf);
> > > > +
> > > > +failed:
> > > > +       usb_scuttle_anchored_urbs(&data->deferred);
> > > > +       return 0;
> > > > +}
> > > > +
> > > >  static void btusb_waker(struct work_struct *work)
> > > >  {
> > > >         struct btusb_data *data = container_of(work, struct btusb_data, waker);
> > > > @@ -3782,6 +3926,8 @@ static int btusb_probe(struct usb_interface *intf,
> > > >         data->udev = interface_to_usbdev(intf);
> > > >         data->intf = intf;
> > > >
> > > > +       INIT_LIST_HEAD(&data->sco_handle_list);
> > > > +
> > > >         INIT_WORK(&data->work, btusb_work);
> > > >         INIT_WORK(&data->waker, btusb_waker);
> > > >         INIT_DELAYED_WORK(&data->rx_work, btusb_rx_work);
> > > > @@ -4117,6 +4263,8 @@ static void btusb_disconnect(struct usb_interface *intf)
> > > >         hdev = data->hdev;
> > > >         usb_set_intfdata(data->intf, NULL);
> > > >
> > > > +       btusb_sco_handle_list_clear(&data->sco_handle_list);
> > > > +
> > > >         if (data->isoc) {
> > > >                 device_remove_file(&intf->dev, &dev_attr_isoc_alt);
> > > >                 usb_set_intfdata(data->isoc, NULL);
> > > > --
> > > > 2.48.1.601.g30ceb7b040-goog
> > > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> > --
> > Best Regards,
> > Hsin-chen
>
>
>
> --
> Luiz Augusto von Dentz

--
Best Regards,
Hsin-chen