* [PATCH] bluetooth: add quirk using packet size 60
@ 2024-10-30 10:08 Hilda Wu
2024-10-31 12:49 ` kernel test robot
2024-10-31 20:47 ` Luiz Augusto von Dentz
0 siblings, 2 replies; 4+ messages in thread
From: Hilda Wu @ 2024-10-30 10:08 UTC (permalink / raw)
To: marcel; +Cc: luiz.dentz, linux-bluetooth, linux-kernel, max.chou, alex_lu,
kidman
The RTL8852BE-VT supports USB alternate setting 6.
However, its descriptor does not report this capability to the host.
Therefore, a quirk is needed to bypass the RTL8852BE-VT's descriptor
and allow it to use USB ALT 6 directly.
Signed-off-by: Hilda Wu <hildawu@realtek.com>
---
drivers/bluetooth/btrtl.c | 3 ++
drivers/bluetooth/btrtl.h | 1 +
drivers/bluetooth/btusb.c | 89 ++++++++++++++++++++++++++++++---------
3 files changed, 73 insertions(+), 20 deletions(-)
diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index 0bcb44cf7b31..b75f0b36a09b 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -1312,6 +1312,9 @@ void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev)
btrtl_dev->project_id == CHIP_ID_8852C)
set_bit(HCI_QUIRK_USE_MSFT_EXT_ADDRESS_FILTER, &hdev->quirks);
+ if (btrtl_dev->project_id == CHIP_ID_8852BT)
+ btrealtek_set_flag(hdev, REALTEK_ALT6_FORCE);
+
hci_set_aosp_capable(hdev);
break;
default:
diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h
index a2d9d34f9fb0..ffec2fca88ec 100644
--- a/drivers/bluetooth/btrtl.h
+++ b/drivers/bluetooth/btrtl.h
@@ -105,6 +105,7 @@ struct rtl_vendor_cmd {
enum {
REALTEK_ALT6_CONTINUOUS_TX_CHIP,
+ REALTEK_ALT6_FORCE,
__REALTEK_NUM_FLAGS,
};
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 514d593923ad..eca0b173232e 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -804,6 +804,7 @@ struct qca_dump_info {
#define BTUSB_USE_ALT3_FOR_WBS 15
#define BTUSB_ALT6_CONTINUOUS_TX 16
#define BTUSB_HW_SSR_ACTIVE 17
+#define BTUSB_ALT_CHANGED 18
struct btusb_data {
struct hci_dev *hdev;
@@ -2130,16 +2131,61 @@ static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
}
}
+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)
+ return NULL;
+
+ for (i = 0; i < intf->num_altsetting; i++) {
+ if (intf->altsetting[i].desc.bAlternateSetting == alt)
+ return &intf->altsetting[i];
+ }
+
+ return NULL;
+}
+
static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
{
struct btusb_data *data = hci_get_drvdata(hdev);
struct usb_interface *intf = data->isoc;
struct usb_endpoint_descriptor *ep_desc;
+ struct usb_host_interface *alt;
int i, err;
if (!data->isoc)
return -ENODEV;
+ /* For some Realtek chips, they actually have the altsetting 6, but its
+ * altsetting descriptor is not exposed. We can activate altsetting 6 by
+ * replacing the altsetting 5.
+ */
+ if (altsetting == 6 && !btusb_find_altsetting(data, 6) &&
+ btrealtek_test_flag(hdev, REALTEK_ALT6_FORCE)) {
+ alt = NULL;
+ for (i = 0; i < intf->num_altsetting; i++) {
+ if (intf->altsetting[i].desc.bAlternateSetting == 5) {
+ alt = &intf->altsetting[i];
+ break;
+ }
+ }
+ if (alt) {
+ for (i = 0; i < alt->desc.bNumEndpoints; i++) {
+ ep_desc = &alt->endpoint[i].desc;
+ if (usb_endpoint_is_isoc_out(ep_desc) ||
+ usb_endpoint_is_isoc_in(ep_desc))
+ ep_desc->wMaxPacketSize = 63;
+ }
+ alt->desc.bAlternateSetting = 6;
+ set_bit(BTUSB_ALT_CHANGED, &data->flags);
+ }
+ }
+
err = usb_set_interface(data->udev, data->isoc_ifnum, altsetting);
if (err < 0) {
bt_dev_err(hdev, "setting interface failed (%d)", -err);
@@ -2151,6 +2197,27 @@ static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
data->isoc_tx_ep = NULL;
data->isoc_rx_ep = NULL;
+ /* Recover alt 5 desc if alt 0 is set. */
+ if (!altsetting && test_bit(BTUSB_ALT_CHANGED, &data->flags)) {
+ alt = NULL;
+ for (i = 0; i < intf->num_altsetting; i++) {
+ if (intf->altsetting[i].desc.bAlternateSetting == 6) {
+ alt = &intf->altsetting[i];
+ break;
+ }
+ }
+ if (alt) {
+ for (i = 0; i < alt->desc.bNumEndpoints; i++) {
+ ep_desc = &alt->endpoint[i].desc;
+ if (usb_endpoint_is_isoc_out(ep_desc) ||
+ usb_endpoint_is_isoc_in(ep_desc))
+ ep_desc->wMaxPacketSize = 49;
+ }
+ alt->desc.bAlternateSetting = 5;
+ clear_bit(BTUSB_ALT_CHANGED, &data->flags);
+ }
+ }
+
for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
ep_desc = &intf->cur_altsetting->endpoint[i].desc;
@@ -2213,25 +2280,6 @@ static int btusb_switch_alt_setting(struct hci_dev *hdev, int new_alts)
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)
- 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);
@@ -2269,7 +2317,8 @@ static void btusb_work(struct work_struct *work)
* MTU >= 3 (packets) * 25 (size) - 3 (headers) = 72
* see also Core spec 5, vol 4, B 2.1.1 & Table 2.1.
*/
- if (btusb_find_altsetting(data, 6))
+ if (btusb_find_altsetting(data, 6) ||
+ btrealtek_test_flag(hdev, REALTEK_ALT6_FORCE))
new_alts = 6;
else if (btusb_find_altsetting(data, 3) &&
hdev->sco_mtu >= 72 &&
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] bluetooth: add quirk using packet size 60
2024-10-30 10:08 [PATCH] bluetooth: add quirk using packet size 60 Hilda Wu
@ 2024-10-31 12:49 ` kernel test robot
2024-10-31 20:47 ` Luiz Augusto von Dentz
1 sibling, 0 replies; 4+ messages in thread
From: kernel test robot @ 2024-10-31 12:49 UTC (permalink / raw)
To: Hilda Wu, marcel
Cc: oe-kbuild-all, luiz.dentz, linux-bluetooth, linux-kernel,
max.chou, alex_lu, kidman
Hi Hilda,
kernel test robot noticed the following build warnings:
[auto build test WARNING on bluetooth/master]
[also build test WARNING on bluetooth-next/master linus/master v6.12-rc5 next-20241031]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Hilda-Wu/bluetooth-add-quirk-using-packet-size-60/20241030-181008
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git master
patch link: https://lore.kernel.org/r/20241030100804.2743115-1-hildawu%40realtek.com
patch subject: [PATCH] bluetooth: add quirk using packet size 60
config: x86_64-randconfig-123-20241031 (https://download.01.org/0day-ci/archive/20241031/202410312046.r3WbTClD-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241031/202410312046.r3WbTClD-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410312046.r3WbTClD-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> drivers/bluetooth/btusb.c:2156:65: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __le16 [usertype] wMaxPacketSize @@ got int @@
drivers/bluetooth/btusb.c:2156:65: sparse: expected restricted __le16 [usertype] wMaxPacketSize
drivers/bluetooth/btusb.c:2156:65: sparse: got int
drivers/bluetooth/btusb.c:2188:65: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __le16 [usertype] wMaxPacketSize @@ got int @@
drivers/bluetooth/btusb.c:2188:65: sparse: expected restricted __le16 [usertype] wMaxPacketSize
drivers/bluetooth/btusb.c:2188:65: sparse: got int
>> drivers/bluetooth/btusb.c:2156:65: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __le16 [usertype] wMaxPacketSize @@ got int @@
drivers/bluetooth/btusb.c:2156:65: sparse: expected restricted __le16 [usertype] wMaxPacketSize
drivers/bluetooth/btusb.c:2156:65: sparse: got int
drivers/bluetooth/btusb.c:2188:65: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __le16 [usertype] wMaxPacketSize @@ got int @@
drivers/bluetooth/btusb.c:2188:65: sparse: expected restricted __le16 [usertype] wMaxPacketSize
drivers/bluetooth/btusb.c:2188:65: sparse: got int
vim +2156 drivers/bluetooth/btusb.c
2126
2127 static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
2128 {
2129 struct btusb_data *data = hci_get_drvdata(hdev);
2130 struct usb_interface *intf = data->isoc;
2131 struct usb_endpoint_descriptor *ep_desc;
2132 struct usb_host_interface *alt;
2133 int i, err;
2134
2135 if (!data->isoc)
2136 return -ENODEV;
2137
2138 /* For some Realtek chips, they actually have the altsetting 6, but its
2139 * altsetting descriptor is not exposed. We can activate altsetting 6 by
2140 * replacing the altsetting 5.
2141 */
2142 if (altsetting == 6 && !btusb_find_altsetting(data, 6) &&
2143 btrealtek_test_flag(hdev, REALTEK_ALT6_FORCE)) {
2144 alt = NULL;
2145 for (i = 0; i < intf->num_altsetting; i++) {
2146 if (intf->altsetting[i].desc.bAlternateSetting == 5) {
2147 alt = &intf->altsetting[i];
2148 break;
2149 }
2150 }
2151 if (alt) {
2152 for (i = 0; i < alt->desc.bNumEndpoints; i++) {
2153 ep_desc = &alt->endpoint[i].desc;
2154 if (usb_endpoint_is_isoc_out(ep_desc) ||
2155 usb_endpoint_is_isoc_in(ep_desc))
> 2156 ep_desc->wMaxPacketSize = 63;
2157 }
2158 alt->desc.bAlternateSetting = 6;
2159 set_bit(BTUSB_ALT_CHANGED, &data->flags);
2160 }
2161 }
2162
2163 err = usb_set_interface(data->udev, data->isoc_ifnum, altsetting);
2164 if (err < 0) {
2165 bt_dev_err(hdev, "setting interface failed (%d)", -err);
2166 return err;
2167 }
2168
2169 data->isoc_altsetting = altsetting;
2170
2171 data->isoc_tx_ep = NULL;
2172 data->isoc_rx_ep = NULL;
2173
2174 /* Recover alt 5 desc if alt 0 is set. */
2175 if (!altsetting && test_bit(BTUSB_ALT_CHANGED, &data->flags)) {
2176 alt = NULL;
2177 for (i = 0; i < intf->num_altsetting; i++) {
2178 if (intf->altsetting[i].desc.bAlternateSetting == 6) {
2179 alt = &intf->altsetting[i];
2180 break;
2181 }
2182 }
2183 if (alt) {
2184 for (i = 0; i < alt->desc.bNumEndpoints; i++) {
2185 ep_desc = &alt->endpoint[i].desc;
2186 if (usb_endpoint_is_isoc_out(ep_desc) ||
2187 usb_endpoint_is_isoc_in(ep_desc))
2188 ep_desc->wMaxPacketSize = 49;
2189 }
2190 alt->desc.bAlternateSetting = 5;
2191 clear_bit(BTUSB_ALT_CHANGED, &data->flags);
2192 }
2193 }
2194
2195 for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
2196 ep_desc = &intf->cur_altsetting->endpoint[i].desc;
2197
2198 if (!data->isoc_tx_ep && usb_endpoint_is_isoc_out(ep_desc)) {
2199 data->isoc_tx_ep = ep_desc;
2200 continue;
2201 }
2202
2203 if (!data->isoc_rx_ep && usb_endpoint_is_isoc_in(ep_desc)) {
2204 data->isoc_rx_ep = ep_desc;
2205 continue;
2206 }
2207 }
2208
2209 if (!data->isoc_tx_ep || !data->isoc_rx_ep) {
2210 bt_dev_err(hdev, "invalid SCO descriptors");
2211 return -ENODEV;
2212 }
2213
2214 return 0;
2215 }
2216
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] bluetooth: add quirk using packet size 60
2024-10-30 10:08 [PATCH] bluetooth: add quirk using packet size 60 Hilda Wu
2024-10-31 12:49 ` kernel test robot
@ 2024-10-31 20:47 ` Luiz Augusto von Dentz
2024-11-18 9:01 ` Hilda Wu
1 sibling, 1 reply; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2024-10-31 20:47 UTC (permalink / raw)
To: Hilda Wu; +Cc: marcel, linux-bluetooth, linux-kernel, max.chou, alex_lu, kidman
Hi Hilda,
On Wed, Oct 30, 2024 at 6:08 AM Hilda Wu <hildawu@realtek.com> wrote:
>
> The RTL8852BE-VT supports USB alternate setting 6.
> However, its descriptor does not report this capability to the host.
> Therefore, a quirk is needed to bypass the RTL8852BE-VT's descriptor
> and allow it to use USB ALT 6 directly.
It is getting very hackish trying to fixup the descriptor in software,
isn't it possible to update the USB descriptors via firmware update?
Not to mention you didn't include any logs of how this was tested, I
suppose this is for HFP/SCO wideband (using msbc), is that just plain
broken right now?
> Signed-off-by: Hilda Wu <hildawu@realtek.com>
> ---
> drivers/bluetooth/btrtl.c | 3 ++
> drivers/bluetooth/btrtl.h | 1 +
> drivers/bluetooth/btusb.c | 89 ++++++++++++++++++++++++++++++---------
> 3 files changed, 73 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> index 0bcb44cf7b31..b75f0b36a09b 100644
> --- a/drivers/bluetooth/btrtl.c
> +++ b/drivers/bluetooth/btrtl.c
> @@ -1312,6 +1312,9 @@ void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev)
> btrtl_dev->project_id == CHIP_ID_8852C)
> set_bit(HCI_QUIRK_USE_MSFT_EXT_ADDRESS_FILTER, &hdev->quirks);
>
> + if (btrtl_dev->project_id == CHIP_ID_8852BT)
> + btrealtek_set_flag(hdev, REALTEK_ALT6_FORCE);
> +
> hci_set_aosp_capable(hdev);
> break;
> default:
> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h
> index a2d9d34f9fb0..ffec2fca88ec 100644
> --- a/drivers/bluetooth/btrtl.h
> +++ b/drivers/bluetooth/btrtl.h
> @@ -105,6 +105,7 @@ struct rtl_vendor_cmd {
>
> enum {
> REALTEK_ALT6_CONTINUOUS_TX_CHIP,
> + REALTEK_ALT6_FORCE,
>
> __REALTEK_NUM_FLAGS,
> };
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 514d593923ad..eca0b173232e 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -804,6 +804,7 @@ struct qca_dump_info {
> #define BTUSB_USE_ALT3_FOR_WBS 15
> #define BTUSB_ALT6_CONTINUOUS_TX 16
> #define BTUSB_HW_SSR_ACTIVE 17
> +#define BTUSB_ALT_CHANGED 18
>
> struct btusb_data {
> struct hci_dev *hdev;
> @@ -2130,16 +2131,61 @@ static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
> }
> }
>
> +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)
> + return NULL;
> +
> + for (i = 0; i < intf->num_altsetting; i++) {
> + if (intf->altsetting[i].desc.bAlternateSetting == alt)
> + return &intf->altsetting[i];
> + }
> +
> + return NULL;
> +}
> +
> static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
> {
> struct btusb_data *data = hci_get_drvdata(hdev);
> struct usb_interface *intf = data->isoc;
> struct usb_endpoint_descriptor *ep_desc;
> + struct usb_host_interface *alt;
> int i, err;
>
> if (!data->isoc)
> return -ENODEV;
>
> + /* For some Realtek chips, they actually have the altsetting 6, but its
> + * altsetting descriptor is not exposed. We can activate altsetting 6 by
> + * replacing the altsetting 5.
> + */
> + if (altsetting == 6 && !btusb_find_altsetting(data, 6) &&
> + btrealtek_test_flag(hdev, REALTEK_ALT6_FORCE)) {
> + alt = NULL;
> + for (i = 0; i < intf->num_altsetting; i++) {
> + if (intf->altsetting[i].desc.bAlternateSetting == 5) {
> + alt = &intf->altsetting[i];
> + break;
> + }
> + }
I believe you can replace the code above with btusb_find_altsetting so
please use that instead of duplicating its logic.
> + if (alt) {
> + for (i = 0; i < alt->desc.bNumEndpoints; i++) {
> + ep_desc = &alt->endpoint[i].desc;
> + if (usb_endpoint_is_isoc_out(ep_desc) ||
> + usb_endpoint_is_isoc_in(ep_desc))
> + ep_desc->wMaxPacketSize = 63;
> + }
> + alt->desc.bAlternateSetting = 6;
> + set_bit(BTUSB_ALT_CHANGED, &data->flags);
> + }
> + }
> +
> err = usb_set_interface(data->udev, data->isoc_ifnum, altsetting);
> if (err < 0) {
> bt_dev_err(hdev, "setting interface failed (%d)", -err);
> @@ -2151,6 +2197,27 @@ static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
> data->isoc_tx_ep = NULL;
> data->isoc_rx_ep = NULL;
>
> + /* Recover alt 5 desc if alt 0 is set. */
> + if (!altsetting && test_bit(BTUSB_ALT_CHANGED, &data->flags)) {
> + alt = NULL;
> + for (i = 0; i < intf->num_altsetting; i++) {
> + if (intf->altsetting[i].desc.bAlternateSetting == 6) {
> + alt = &intf->altsetting[i];
> + break;
> + }
> + }
Ditto.
> + if (alt) {
> + for (i = 0; i < alt->desc.bNumEndpoints; i++) {
> + ep_desc = &alt->endpoint[i].desc;
> + if (usb_endpoint_is_isoc_out(ep_desc) ||
> + usb_endpoint_is_isoc_in(ep_desc))
> + ep_desc->wMaxPacketSize = 49;
> + }
> + alt->desc.bAlternateSetting = 5;
> + clear_bit(BTUSB_ALT_CHANGED, &data->flags);
> + }
> + }
> +
> for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
> ep_desc = &intf->cur_altsetting->endpoint[i].desc;
>
> @@ -2213,25 +2280,6 @@ static int btusb_switch_alt_setting(struct hci_dev *hdev, int new_alts)
> 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)
> - 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);
> @@ -2269,7 +2317,8 @@ static void btusb_work(struct work_struct *work)
> * MTU >= 3 (packets) * 25 (size) - 3 (headers) = 72
> * see also Core spec 5, vol 4, B 2.1.1 & Table 2.1.
> */
> - if (btusb_find_altsetting(data, 6))
> + if (btusb_find_altsetting(data, 6) ||
> + btrealtek_test_flag(hdev, REALTEK_ALT6_FORCE))
> new_alts = 6;
> else if (btusb_find_altsetting(data, 3) &&
> hdev->sco_mtu >= 72 &&
> --
> 2.34.1
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] bluetooth: add quirk using packet size 60
2024-10-31 20:47 ` Luiz Augusto von Dentz
@ 2024-11-18 9:01 ` Hilda Wu
0 siblings, 0 replies; 4+ messages in thread
From: Hilda Wu @ 2024-11-18 9:01 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: marcel@holtmann.org, linux-bluetooth@vger.kernel.org,
linux-kernel@vger.kernel.org, Max Chou, alex_lu@realsil.com.cn,
KidmanLee
Hi Luiz,
Thanks for your feedback.
I just send the V2.
Please see the reply below. Thank you
-----Original Message-----
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Sent: Friday, November 1, 2024 4:47 AM
To: Hilda Wu <hildawu@realtek.com>
Cc: marcel@holtmann.org; linux-bluetooth@vger.kernel.org; linux-kernel@vger.kernel.org; Max Chou <max.chou@realtek.com>; alex_lu@realsil.com.cn; KidmanLee <kidman@realtek.com>
Subject: Re: [PATCH] bluetooth: add quirk using packet size 60
External mail.
Hi Hilda,
On Wed, Oct 30, 2024 at 6:08 AM Hilda Wu <hildawu@realtek.com> wrote:
>
> The RTL8852BE-VT supports USB alternate setting 6.
> However, its descriptor does not report this capability to the host.
> Therefore, a quirk is needed to bypass the RTL8852BE-VT's descriptor
> and allow it to use USB ALT 6 directly.
It is getting very hackish trying to fixup the descriptor in software, isn't it possible to update the USB descriptors via firmware update?
Not to mention you didn't include any logs of how this was tested, I suppose this is for HFP/SCO wideband (using msbc), is that just plain broken right now?
Since there is currently no way for update the USB descriptors even if it is firmware. That’s why we do it through the driver.
This is for HFP/SCO wideband (using msbc), the broken state is other USB ALT will be used and there will be no sound.
> Signed-off-by: Hilda Wu <hildawu@realtek.com>
> ---
> drivers/bluetooth/btrtl.c | 3 ++
> drivers/bluetooth/btrtl.h | 1 +
> drivers/bluetooth/btusb.c | 89
> ++++++++++++++++++++++++++++++---------
> 3 files changed, 73 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> index 0bcb44cf7b31..b75f0b36a09b 100644
> --- a/drivers/bluetooth/btrtl.c
> +++ b/drivers/bluetooth/btrtl.c
> @@ -1312,6 +1312,9 @@ void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev)
> btrtl_dev->project_id == CHIP_ID_8852C)
> set_bit(HCI_QUIRK_USE_MSFT_EXT_ADDRESS_FILTER,
> &hdev->quirks);
>
> + if (btrtl_dev->project_id == CHIP_ID_8852BT)
> + btrealtek_set_flag(hdev, REALTEK_ALT6_FORCE);
> +
> hci_set_aosp_capable(hdev);
> break;
> default:
> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h
> index a2d9d34f9fb0..ffec2fca88ec 100644
> --- a/drivers/bluetooth/btrtl.h
> +++ b/drivers/bluetooth/btrtl.h
> @@ -105,6 +105,7 @@ struct rtl_vendor_cmd {
>
> enum {
> REALTEK_ALT6_CONTINUOUS_TX_CHIP,
> + REALTEK_ALT6_FORCE,
>
> __REALTEK_NUM_FLAGS,
> };
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 514d593923ad..eca0b173232e 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -804,6 +804,7 @@ struct qca_dump_info { #define
> BTUSB_USE_ALT3_FOR_WBS 15
> #define BTUSB_ALT6_CONTINUOUS_TX 16
> #define BTUSB_HW_SSR_ACTIVE 17
> +#define BTUSB_ALT_CHANGED 18
>
> struct btusb_data {
> struct hci_dev *hdev;
> @@ -2130,16 +2131,61 @@ static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
> }
> }
>
> +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)
> + return NULL;
> +
> + for (i = 0; i < intf->num_altsetting; i++) {
> + if (intf->altsetting[i].desc.bAlternateSetting == alt)
> + return &intf->altsetting[i];
> + }
> +
> + return NULL;
> +}
> +
> static inline int __set_isoc_interface(struct hci_dev *hdev, int
> altsetting) {
> struct btusb_data *data = hci_get_drvdata(hdev);
> struct usb_interface *intf = data->isoc;
> struct usb_endpoint_descriptor *ep_desc;
> + struct usb_host_interface *alt;
> int i, err;
>
> if (!data->isoc)
> return -ENODEV;
>
> + /* For some Realtek chips, they actually have the altsetting 6, but its
> + * altsetting descriptor is not exposed. We can activate altsetting 6 by
> + * replacing the altsetting 5.
> + */
> + if (altsetting == 6 && !btusb_find_altsetting(data, 6) &&
> + btrealtek_test_flag(hdev, REALTEK_ALT6_FORCE)) {
> + alt = NULL;
> + for (i = 0; i < intf->num_altsetting; i++) {
> + if (intf->altsetting[i].desc.bAlternateSetting == 5) {
> + alt = &intf->altsetting[i];
> + break;
> + }
> + }
I believe you can replace the code above with btusb_find_altsetting so please use that instead of duplicating its logic.
Adjusted in V2.
> + if (alt) {
> + for (i = 0; i < alt->desc.bNumEndpoints; i++) {
> + ep_desc = &alt->endpoint[i].desc;
> + if (usb_endpoint_is_isoc_out(ep_desc) ||
> + usb_endpoint_is_isoc_in(ep_desc))
> + ep_desc->wMaxPacketSize = 63;
> + }
> + alt->desc.bAlternateSetting = 6;
> + set_bit(BTUSB_ALT_CHANGED, &data->flags);
> + }
> + }
> +
> err = usb_set_interface(data->udev, data->isoc_ifnum, altsetting);
> if (err < 0) {
> bt_dev_err(hdev, "setting interface failed (%d)",
> -err); @@ -2151,6 +2197,27 @@ static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
> data->isoc_tx_ep = NULL;
> data->isoc_rx_ep = NULL;
>
> + /* Recover alt 5 desc if alt 0 is set. */
> + if (!altsetting && test_bit(BTUSB_ALT_CHANGED, &data->flags)) {
> + alt = NULL;
> + for (i = 0; i < intf->num_altsetting; i++) {
> + if (intf->altsetting[i].desc.bAlternateSetting == 6) {
> + alt = &intf->altsetting[i];
> + break;
> + }
> + }
Ditto.
Adjusted in V2.
> + if (alt) {
> + for (i = 0; i < alt->desc.bNumEndpoints; i++) {
> + ep_desc = &alt->endpoint[i].desc;
> + if (usb_endpoint_is_isoc_out(ep_desc) ||
> + usb_endpoint_is_isoc_in(ep_desc))
> + ep_desc->wMaxPacketSize = 49;
> + }
> + alt->desc.bAlternateSetting = 5;
> + clear_bit(BTUSB_ALT_CHANGED, &data->flags);
> + }
> + }
> +
> for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
> ep_desc = &intf->cur_altsetting->endpoint[i].desc;
>
> @@ -2213,25 +2280,6 @@ static int btusb_switch_alt_setting(struct hci_dev *hdev, int new_alts)
> 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)
> - 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); @@ -2269,7 +2317,8 @@ static void btusb_work(struct work_struct *work)
> * MTU >= 3 (packets) * 25 (size) - 3 (headers) = 72
> * see also Core spec 5, vol 4, B 2.1.1 & Table 2.1.
> */
> - if (btusb_find_altsetting(data, 6))
> + if (btusb_find_altsetting(data, 6) ||
> + btrealtek_test_flag(hdev,
> + REALTEK_ALT6_FORCE))
> new_alts = 6;
> else if (btusb_find_altsetting(data, 3) &&
> hdev->sco_mtu >= 72 &&
> --
> 2.34.1
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-11-18 9:01 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-30 10:08 [PATCH] bluetooth: add quirk using packet size 60 Hilda Wu
2024-10-31 12:49 ` kernel test robot
2024-10-31 20:47 ` Luiz Augusto von Dentz
2024-11-18 9:01 ` Hilda Wu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).