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
