Message-ID: <20200923102215.hrfzl7c7q2omeiws@pali>
Date:   Wed, 23 Sep 2020 12:22:15 +0200
From:   Pali Rohár <pali@...nel.org>
To:     Joseph Hwang <josephsih@...gle.com>
Cc:     linux-bluetooth <linux-bluetooth@...r.kernel.org>,
        Marcel Holtmann <marcel@...tmann.org>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        chromeos-bluetooth-upstreaming 
        <chromeos-bluetooth-upstreaming@...omium.org>,
        Alain Michaud <alainm@...omium.org>,
        Abhishek Pandit-Subedi <abhishekpandit@...omium.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v3 1/2] Bluetooth: btusb: define HCI packet sizes of USB
 Alts

Hello!

On Monday 14 September 2020 20:18:27 Joseph Hwang wrote:
> On Thu, Sep 10, 2020 at 4:18 PM Pali Rohár <pali@...nel.org> wrote:
> > And this part of code which you write is Realtek specific.
> 
> We currently only have Intel and Realtek platforms to test with. If
> making it generic without proper testing platforms is fine, I will
> make it generic. Or do you think it might be better to make it
> customized with particular vendors for now; and make it generic later
> when it works well with sufficient vendors?

I understood that those packet size changes are generic to bluetooth
specification and therefore it is not vendor specific code. Those packet
sizes for me really seems to be USB specific.

Therefore it should apply for all vendors, not only for Realtek and
Intel.

I would propose to fix it globally for all USB adapters and not only for
Intel and Realtek USB adapters.

> >
> > I thought that this is something generic to bluetooth usb as you pointed
> > to bluetooth documentation "core spec 5, vol 4, part B, table 2.1".
> >
> > > +             } else
> > >                       bt_dev_err(hdev, "Device does not support ALT setting 1");
> > >       }
> >
> > Also this patch seems to be for me incomplete or not fully correct as
> > USB altsetting is chosen in function btusb_work() and it depends on
> > selected AIR mode (which is configured by another setsockopt).
> >
> > So despite what is written in commit message, this patch looks for me
> > like some hack for Intel and Realtek bluetooth adapters and does not
> > solve problems in vendor independent manner.
> 
> You are right that sco_mtu should be changed according to the air
> mode. Here are some issues to handle and what I plan to do. I would
> like to solicit your comments before I submit the next series.
> 
> [Issue 1] The air mode is determined in btusb_work() which is
> triggered by hci_sync_conn_complete_evt(). So “conn->mtu =
> hdev->sco_mtu” should not be done in  sco_conn_add() in the early
> connecting stage. Instead, it will be moved to near the end of
> hci_sync_conn_complete_evt().
> 
> [Issue 2] The btusb_work() is performed by a worker. There would be a
> timing issue here if we let btusb_work() to do “hdev->sco_mtu =
> hci_packet_size_usb_alt[i]” because there is no guarantee how soon the
> btusb_work() can be finished and get “hdev->sco_mtu” value set
> correctly. In order to avoid the potential race condition, I suggest
> to determine air_mode in btusb_notify() before
> schedule_work(&data->work) is executed so that “hdev->sco_mtu =
> hci_packet_size_usb_alt[i]” is guaranteed to be performed when
> btusb_notify() finished. In this way, hci_sync_conn_complete_evt() can
> set conn->mtu correctly as described in [Issue 1] above.

If it really fixes this issue, I'm fine with it.

You have Realtek and Intel HW for testing, so I think if doing some
heavy tests did not trigger any issue / race condition then it should be
fine.

> [Issue 3] Concerning CVSD: The above flow is good when the transparent
> mode is selected. When it is the CVSD mode, we should set
> hdev->sco_mtu and conn->mtu back to the original mtu size returned by
> hci_cc_read_buffer_size().

Beware that kernel does not support CVSD mode between (USB) bluetooth
adapter and kernel yet. Therefore it does not make sense to discussion
this option.

MTU size affects packets between kernel (or userspace) and bluetooth
adapter. Bluetooth adapter then do HW encoding/decoding if
non-transparent mode is used.

So I think you rather mean PCMs16 mode between bluetooth adapter and
kernel because this mode is used for CVSD air codec (between two
bluetooth adapters over the air). This is default mode, userspace
application pass PCMs16 data to kernel which pass it over USB to
bluetooth adapter. And bluetooth adapter then do HW encoding, encodes
PCMs16 to CVSD codec and transmit it over the air. So over USB are sent
PCM data, not CVSD.

> This is because we do not have a reliable
> way to determine what size is used with CVSD. AFAIK, controllers
> connected through USB use 48 bytes; and controllers connected through
> UART use 60 bytes.

I think for USB adapters we have that size. From bluetooth specification
I understood that it is that size which you calculated in that array
based on alt interface.

I understood that bluetooth specification that packet size on USB
depends only on chosen alt interface. And choice of alt interface
depends on voice codec (PCM or transparent) and on number of active
transports.

But correct me, if I'm wrong. Maybe I did not understand bluetooth
specification correctly.

And for UART I do not know, I have not looked at it it yet.

> It seems to me that these numbers are not recorded
> in the kernel(?). It seems beyond the scope of this patch to set the
> proper value for CVSD. So we will let hdev->sco_mtu and conn->mtu go
> back to their original values and are not affected by this patch.
> 
> I am wondering if such directions of fixing this patch look good to you?
> 
> Thanks and regards!
> Joseph

Powered by blists - more mailing lists