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

From: Hsin-chen Chuang
Date: Sun Feb 23 2025 - 22:21:14 EST

Next message: Baolin Wang: "Re: Hang when swapping huge=within_size tmpfs from zram"
Previous message: Kairui Song: "Re: [PATCH 6/7] mm, swap: remove swap slot cache"
In reply to: Luiz Augusto von Dentz: "Re: [PATCH] Bluetooth: btusb: Configure altsetting for USER_CHANNEL"
Next in thread: Luiz Augusto von Dentz: "Re: [PATCH] Bluetooth: btusb: Configure altsetting for USER_CHANNEL"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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.

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

Next message: Baolin Wang: "Re: Hang when swapping huge=within_size tmpfs from zram"
Previous message: Kairui Song: "Re: [PATCH 6/7] mm, swap: remove swap slot cache"
In reply to: Luiz Augusto von Dentz: "Re: [PATCH] Bluetooth: btusb: Configure altsetting for USER_CHANNEL"
Next in thread: Luiz Augusto von Dentz: "Re: [PATCH] Bluetooth: btusb: Configure altsetting for USER_CHANNEL"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]