Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191205083229.29890-1-sathish.narasimman@intel.com> <F0282871-5558-4582-A1EC-F938BA8F77B8@holtmann.org>
In-Reply-To: <F0282871-5558-4582-A1EC-F938BA8F77B8@holtmann.org>
From:   Sathish Narasimman <nsathish41@gmail.com>
Date:   Thu, 2 Apr 2020 20:42:49 +0530
Message-ID: <CAOVXEJJhbFLPb3_4SsG_2b4Sjtzvozq3QrbTO-LbJjo_tUwE9w@mail.gmail.com>
Subject: Re: [PATCH v6 2/2] bluetooth:btusb: handle msbc audio over USB Endpoints
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     Bluetooth Kernel Mailing List <linux-bluetooth@vger.kernel.org>,
        Sathish Narasimman <sathish.narasimman@intel.com>,
        Chethan T N <chethan.tumkur.narayan@intel.com>,
        Hsin-Yu Chao <hychao@chromium.org>,
        Amit K Bag <amit.k.bag@intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Marcel,

On Tue, Mar 31, 2020 at 4:08 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> 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 <lkp@intel.com>
> > Signed-off-by: Sathish Narasimman <sathish.narasimman@intel.com>
> > Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> > Signed-off-by: Hsin-Yu Chao <hychao@chromium.org>
> > Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
> > ---
> > 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?
>
Will move it to the beginning of fn

> > +
> > +     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.
>
Will fix them in next version

> >       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).
>
will remove the error check

> > +             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.
>
will change it

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

Regards
Sathish N