From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id F0616C43331 for ; Mon, 30 Mar 2020 22:38:29 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C23FC20733 for ; Mon, 30 Mar 2020 22:38:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729445AbgC3Wi3 convert rfc822-to-8bit (ORCPT ); Mon, 30 Mar 2020 18:38:29 -0400 Received: from coyote.holtmann.net ([212.227.132.17]:49740 "EHLO mail.holtmann.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728876AbgC3Wi3 (ORCPT ); Mon, 30 Mar 2020 18:38:29 -0400 Received: from marcel-macbook.fritz.box (p4FEFC5A7.dip0.t-ipconnect.de [79.239.197.167]) by mail.holtmann.org (Postfix) with ESMTPSA id 5058BCECB0; Tue, 31 Mar 2020 00:48:00 +0200 (CEST) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\)) Subject: Re: [PATCH v6 2/2] bluetooth:btusb: handle msbc audio over USB Endpoints From: Marcel Holtmann In-Reply-To: <20191205083229.29890-1-sathish.narasimman@intel.com> Date: Tue, 31 Mar 2020 00:38:27 +0200 Cc: Bluetooth Kernel Mailing List , Sathish Narasimman , Chethan T N , Hsin-Yu Chao , Amit K Bag Content-Transfer-Encoding: 8BIT Message-Id: References: <20191205083229.29890-1-sathish.narasimman@intel.com> To: Sathish Narsimman X-Mailer: Apple Mail (2.3608.80.23.2.2) Sender: linux-bluetooth-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-bluetooth@vger.kernel.org Hi Sathish, > For msbc encoded audio stream over usb transport, btusb driver > to be set to alternate settings 6 as per BT core spec 5.0. The > type of air mode is used to differenting which alt setting to be > used. > The changes are made considering some discussion over the similar > patch submitted earlier from Kuba Pawlak(link below) > https://www.spinics.net/lists/linux-bluetooth/msg64577.html > > Reported-by: kbuild test robot > Signed-off-by: Sathish Narasimman > Signed-off-by: Chethan T N > Signed-off-by: Hsin-Yu Chao > Signed-off-by: Amit K Bag > --- > drivers/bluetooth/btusb.c | 157 +++++++++++++++++++++++++++++--------- > 1 file changed, 121 insertions(+), 36 deletions(-) > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index d9cd0677d41c..5563569f4473 100644 > --- a/drivers/bluetooth/btusb.c > +++ b/drivers/bluetooth/btusb.c > @@ -483,6 +483,8 @@ struct btusb_data { > __u8 cmdreq; > > unsigned int sco_num; > + unsigned int air_mode; > + bool usb_alt6_packet_flow; > int isoc_altsetting; > int suspend_count; > > @@ -974,6 +976,42 @@ static void btusb_isoc_complete(struct urb *urb) > } > } > > +static inline void __fill_isoc_descriptor_msbc(struct urb *urb, int len, > + int mtu, struct btusb_data *data) > +{ > + int i, offset = 0; > + unsigned int interval; > + > + /* For msbc ALT 6 setting the host will send the packet at continuous > + * flow. As per core spec 5, vol 4, part B, table 2.1. For ALT setting > + * 6 the HCI PACKET INTERVAL should be 7.5ms for every usb packets. > + * To maintain the rate we send 63bytes of usb packets alternatively for > + * 7ms and 8ms to maintain the rate as 7.5ms. > + */ > + if (data->usb_alt6_packet_flow) { > + interval = 7; > + data->usb_alt6_packet_flow = false; > + } else { > + interval = 6; > + data->usb_alt6_packet_flow = true; > + } > + > + BT_DBG("interval:%d len %d mtu %d", interval, len, mtu); do you really want to keep this BT_DBG here? > + > + for (i = 0; i < interval; i++) { > + urb->iso_frame_desc[i].offset = offset; > + urb->iso_frame_desc[i].length = offset; > + } > + > + if (len && i < BTUSB_MAX_ISOC_FRAMES) { > + urb->iso_frame_desc[i].offset = offset; > + urb->iso_frame_desc[i].length = len; > + i++; > + } > + > + urb->number_of_packets = i; > +} > + > static inline void __fill_isoc_descriptor(struct urb *urb, int len, int mtu) > { > int i, offset = 0; > @@ -1377,9 +1415,13 @@ static struct urb *alloc_isoc_urb(struct hci_dev *hdev, struct sk_buff *skb) > > urb->transfer_flags = URB_ISO_ASAP; > > - __fill_isoc_descriptor(urb, skb->len, > + if (data->isoc_altsetting == 6) > + __fill_isoc_descriptor_msbc(urb, skb->len, > + le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize), > + data); > + else > + __fill_isoc_descriptor(urb, skb->len, > le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize)); > - Your changes are now breaking the coding style. Please fix them. > skb->dev = (void *)hdev; > > return urb; > @@ -1475,6 +1517,7 @@ static void btusb_notify(struct hci_dev *hdev, unsigned int evt) > > if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) { > data->sco_num = hci_conn_num(hdev, SCO_LINK); > + data->air_mode = evt; > schedule_work(&data->work); > } > } > @@ -1522,11 +1565,70 @@ static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting) > return 0; > } > > +static int bt_switch_alt_setting(struct hci_dev *hdev, int new_alts) > +{ > + struct btusb_data *data = hci_get_drvdata(hdev); > + int err; > + > + if (data->isoc_altsetting != new_alts) { > + unsigned long flags; > + > + clear_bit(BTUSB_ISOC_RUNNING, &data->flags); > + usb_kill_anchored_urbs(&data->isoc_anchor); > + > + /* When isochronous alternate setting needs to be > + * changed, because SCO connection has been added > + * or removed, a packet fragment may be left in the > + * reassembling state. This could lead to wrongly > + * assembled fragments. > + * > + * Clear outstanding fragment when selecting a new > + * alternate setting. > + */ > + spin_lock_irqsave(&data->rxlock, flags); > + kfree_skb(data->sco_skb); > + data->sco_skb = NULL; > + spin_unlock_irqrestore(&data->rxlock, flags); > + > + err = __set_isoc_interface(hdev, new_alts); > + if (err < 0) > + return err; > + } > + if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) { > + if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0) > + clear_bit(BTUSB_ISOC_RUNNING, &data->flags); > + else > + btusb_submit_isoc_urb(hdev, GFP_KERNEL); > + } > + > + return 0; > +} > + > +static struct usb_host_interface *btusb_find_altsetting(struct btusb_data *data, int alt) > +{ > + struct usb_interface *intf = data->isoc; > + int i; > + > + BT_DBG("Looking for Alt no :%d", alt); > + > + if (intf == NULL) { > + BT_ERR("INterface NULL"); I would not put an error here. And we check if (!intf). > + return NULL; > + } > + > + for (i = 0; i < intf->num_altsetting; i++) { > + if (intf->altsetting[i].desc.bAlternateSetting == alt) > + return &intf->altsetting[i]; > + } > + > + return NULL; > +} > + > static void btusb_work(struct work_struct *work) > { > struct btusb_data *data = container_of(work, struct btusb_data, work); > struct hci_dev *hdev = data->hdev; > - int new_alts; > + int new_alts = 0; > int err; > > if (data->sco_num > 0) { > @@ -1541,44 +1643,27 @@ static void btusb_work(struct work_struct *work) > set_bit(BTUSB_DID_ISO_RESUME, &data->flags); > } > > - if (hdev->voice_setting & 0x0020) { > - static const int alts[3] = { 2, 4, 5 }; > - > - new_alts = alts[data->sco_num - 1]; > - } else { > - new_alts = data->sco_num; > - } > - > - if (data->isoc_altsetting != new_alts) { > - unsigned long flags; > + if (data->air_mode == HCI_NOTIFY_ENABLE_SCO_CVSD) { > + if (hdev->voice_setting & 0x0020) { > + static const int alts[3] = { 2, 4, 5 }; > > - clear_bit(BTUSB_ISOC_RUNNING, &data->flags); > - usb_kill_anchored_urbs(&data->isoc_anchor); > - > - /* When isochronous alternate setting needs to be > - * changed, because SCO connection has been added > - * or removed, a packet fragment may be left in the > - * reassembling state. This could lead to wrongly > - * assembled fragments. > - * > - * Clear outstanding fragment when selecting a new > - * alternate setting. > - */ > - spin_lock_irqsave(&data->rxlock, flags); > - kfree_skb(data->sco_skb); > - data->sco_skb = NULL; > - spin_unlock_irqrestore(&data->rxlock, flags); > + new_alts = alts[data->sco_num - 1]; > + } else { > + new_alts = data->sco_num; > + } > + } else if (data->air_mode == HCI_NOTIFY_ENABLE_SCO_TRANSP) { > > - if (__set_isoc_interface(hdev, new_alts) < 0) > - return; > - } > + data->usb_alt6_packet_flow = true; > > - if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) { > - if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0) > - clear_bit(BTUSB_ISOC_RUNNING, &data->flags); > + /* Check if Alt 6 is supported for Transparent audio*/ > + if (btusb_find_altsetting(data, 6)) > + new_alts = 6; > else > - btusb_submit_isoc_urb(hdev, GFP_KERNEL); > + BT_ERR("%s Device does not support ALT setting 6", hdev->name); Please use bt_dev_err. > } > + > + if (bt_switch_alt_setting(hdev, new_alts) < 0) > + BT_ERR("%s Set USB Alt: %d failed!", hdev->name, new_alts); Same here. bt_dev_err please. > } else { > clear_bit(BTUSB_ISOC_RUNNING, &data->flags); > usb_kill_anchored_urbs(&data->isoc_anchor); Regards Marcel