From: Nobuaki Tsunashima <[email protected]>
CYW4373 ROM FW has an issue that it claims LE_Read_Transmit_Power command
as supported in a response of Read_Local_Supported_Command command but
rejects the LE_Read_Transmit_Power command with "Unknown HCI Command"
status. Because Bluetooth driver of kernel 5.11 added sending the
LE_Read_Transmit_Power command in initialize phase, hci up fails due to the
issue.
Especially in USB i/f case, it would be difficult to download patch FW that
includes its fix unless hci is up.
The driver already contains infrastructure to apply the quirk for the
issue, but currently it only supports DMI based matching. Add support to
match by chip id and baseline FW version to detect CYW4373 ROM FW build
in generic system.
Fixes: 7c395ea521e6 ("Bluetooth: Query LE tx power on startup")
Signed-off-by: Nobuaki Tsunashima <[email protected]>
---
V3 -> V4: Fix a few coding style warnings and refine comments for clarify.
V2 -> V3: Fix a few coding style warnings and change the subject as more specific.
V1 -> V2: Fix several coding style warnings.
drivers/bluetooth/btbcm.c | 32 +++++++++++++++++++++++++++++++-
drivers/bluetooth/btusb.c | 4 ++++
2 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
index 0a5445ac5e1b..29e3f83a19fa 100644
--- a/drivers/bluetooth/btbcm.c
+++ b/drivers/bluetooth/btbcm.c
@@ -437,18 +437,48 @@ static const struct dmi_system_id disable_broken_read_transmit_power[] = {
{ }
};
+struct bcm_chip_version_table {
+ u8 chip_id; /* Chip ID */
+ u16 baseline; /* Baseline version of patch FW */
+};
+#define BCM_ROMFW_BASELINE_NUM 0xFFFF
+static const struct bcm_chip_version_table disable_broken_read_transmit_power_by_chip_ver[] = {
+ { 0x87, BCM_ROMFW_BASELINE_NUM } /* CYW4373/4373E */
+};
+static bool btbcm_is_disable_broken_read_tx_power_by_chip_ver(u8 chip_id, u16 baseline)
+{
+ int i;
+ size_t table_size = ARRAY_SIZE(disable_broken_read_transmit_power_by_chip_ver);
+ const struct bcm_chip_version_table *entry =
+ &disable_broken_read_transmit_power_by_chip_ver[0];
+
+ for (i = 0 ; i < table_size ; i++, entry++) {
+ if ((chip_id == entry->chip_id) && (baseline == entry->baseline))
+ return true;
+ }
+
+ return false;
+}
+
static int btbcm_read_info(struct hci_dev *hdev)
{
struct sk_buff *skb;
+ u8 chip_id;
+ u16 baseline;
/* Read Verbose Config Version Info */
skb = btbcm_read_verbose_config(hdev);
if (IS_ERR(skb))
return PTR_ERR(skb);
-
+ chip_id = skb->data[1];
+ baseline = skb->data[3] | (skb->data[4] << 8);
bt_dev_info(hdev, "BCM: chip id %u", skb->data[1]);
kfree_skb(skb);
+ /* Check Chip ID and disable broken Read LE Min/Max Tx Power */
+ if (btbcm_is_disable_broken_read_tx_power_by_chip_ver(chip_id, baseline))
+ set_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, &hdev->quirks);
+
return 0;
}
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index d31edad7a056..52561c8d8828 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -142,6 +142,10 @@ static const struct usb_device_id btusb_table[] = {
{ USB_VENDOR_AND_INTERFACE_INFO(0x04ca, 0xff, 0x01, 0x01),
.driver_info = BTUSB_BCM_PATCHRAM },
+ /* Cypress devices with vendor specific id */
+ { USB_VENDOR_AND_INTERFACE_INFO(0x04b4, 0xff, 0x01, 0x01),
+ .driver_info = BTUSB_BCM_PATCHRAM },
+
/* Broadcom devices with vendor specific id */
{ USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01),
.driver_info = BTUSB_BCM_PATCHRAM },
--
2.25.1
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=855541
---Test result---
Test Summary:
CheckPatch PASS 0.70 seconds
GitLint FAIL 0.45 seconds
SubjectPrefix PASS 0.06 seconds
BuildKernel PASS 30.18 seconds
CheckAllWarning PASS 32.50 seconds
CheckSparse PASS 38.18 seconds
CheckSmatch FAIL 35.54 seconds
BuildKernel32 PASS 29.01 seconds
TestRunnerSetup PASS 517.53 seconds
TestRunner_l2cap-tester PASS 20.46 seconds
TestRunner_iso-tester PASS 28.92 seconds
TestRunner_bnep-tester PASS 4.89 seconds
TestRunner_mgmt-tester PASS 109.37 seconds
TestRunner_rfcomm-tester PASS 7.40 seconds
TestRunner_sco-tester PASS 14.96 seconds
TestRunner_ioctl-tester PASS 7.81 seconds
TestRunner_mesh-tester PASS 5.87 seconds
TestRunner_smp-tester PASS 6.87 seconds
TestRunner_userchan-tester PASS 5.02 seconds
IncrementalBuild PASS 27.34 seconds
Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v4] Bluetooth: btbcm: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER to CYW4373
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
23: B1 Line exceeds max length (82>80): "V2 -> V3: Fix a few coding style warnings and change the subject as more specific."
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139
make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2
---
Regards,
Linux Bluetooth
Hi Nobuaki,
On Thu, May 23, 2024 at 9:31 PM Nobuaki Tsunashima
<[email protected]> wrote:
>
> From: Nobuaki Tsunashima <[email protected]>
>
> CYW4373 ROM FW has an issue that it claims LE_Read_Transmit_Power command
> as supported in a response of Read_Local_Supported_Command command but
> rejects the LE_Read_Transmit_Power command with "Unknown HCI Command"
> status. Because Bluetooth driver of kernel 5.11 added sending the
> LE_Read_Transmit_Power command in initialize phase, hci up fails due to the
> issue.
>
> Especially in USB i/f case, it would be difficult to download patch FW that
> includes its fix unless hci is up.
>
> The driver already contains infrastructure to apply the quirk for the
> issue, but currently it only supports DMI based matching. Add support to
> match by chip id and baseline FW version to detect CYW4373 ROM FW build
> in generic system.
>
> Fixes: 7c395ea521e6 ("Bluetooth: Query LE tx power on startup")
> Signed-off-by: Nobuaki Tsunashima <[email protected]>
> ---
> V3 -> V4: Fix a few coding style warnings and refine comments for clarify.
> V2 -> V3: Fix a few coding style warnings and change the subject as more specific.
> V1 -> V2: Fix several coding style warnings.
>
> drivers/bluetooth/btbcm.c | 32 +++++++++++++++++++++++++++++++-
> drivers/bluetooth/btusb.c | 4 ++++
> 2 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
> index 0a5445ac5e1b..29e3f83a19fa 100644
> --- a/drivers/bluetooth/btbcm.c
> +++ b/drivers/bluetooth/btbcm.c
> @@ -437,18 +437,48 @@ static const struct dmi_system_id disable_broken_read_transmit_power[] = {
> { }
> };
>
> +struct bcm_chip_version_table {
> + u8 chip_id; /* Chip ID */
> + u16 baseline; /* Baseline version of patch FW */
> +};
> +#define BCM_ROMFW_BASELINE_NUM 0xFFFF
> +static const struct bcm_chip_version_table disable_broken_read_transmit_power_by_chip_ver[] = {
> + { 0x87, BCM_ROMFW_BASELINE_NUM } /* CYW4373/4373E */
> +};
> +static bool btbcm_is_disable_broken_read_tx_power_by_chip_ver(u8 chip_id, u16 baseline)
> +{
> + int i;
> + size_t table_size = ARRAY_SIZE(disable_broken_read_transmit_power_by_chip_ver);
> + const struct bcm_chip_version_table *entry =
> + &disable_broken_read_transmit_power_by_chip_ver[0];
> +
> + for (i = 0 ; i < table_size ; i++, entry++) {
> + if ((chip_id == entry->chip_id) && (baseline == entry->baseline))
> + return true;
> + }
> +
> + return false;
> +}
> +
> static int btbcm_read_info(struct hci_dev *hdev)
> {
> struct sk_buff *skb;
> + u8 chip_id;
> + u16 baseline;
>
> /* Read Verbose Config Version Info */
> skb = btbcm_read_verbose_config(hdev);
> if (IS_ERR(skb))
> return PTR_ERR(skb);
> -
> + chip_id = skb->data[1];
> + baseline = skb->data[3] | (skb->data[4] << 8);
This is not really safe, you shouldn't attempt to access skb->data
without first checking skb->len, actually it would be much better that
you would use skb_pull_data which does skb->len check before pulling
data.
> bt_dev_info(hdev, "BCM: chip id %u", skb->data[1]);
> kfree_skb(skb);
>
> + /* Check Chip ID and disable broken Read LE Min/Max Tx Power */
> + if (btbcm_is_disable_broken_read_tx_power_by_chip_ver(chip_id, baseline))
> + set_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, &hdev->quirks);
> +
> return 0;
> }
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index d31edad7a056..52561c8d8828 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -142,6 +142,10 @@ static const struct usb_device_id btusb_table[] = {
> { USB_VENDOR_AND_INTERFACE_INFO(0x04ca, 0xff, 0x01, 0x01),
> .driver_info = BTUSB_BCM_PATCHRAM },
>
> + /* Cypress devices with vendor specific id */
> + { USB_VENDOR_AND_INTERFACE_INFO(0x04b4, 0xff, 0x01, 0x01),
> + .driver_info = BTUSB_BCM_PATCHRAM },
> +
> /* Broadcom devices with vendor specific id */
> { USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01),
> .driver_info = BTUSB_BCM_PATCHRAM },
> --
> 2.25.1
>
--
Luiz Augusto von Dentz
Hi Luiz,
Thanks for your review.
>> static int btbcm_read_info(struct hci_dev *hdev) {
>> struct sk_buff *skb;
>> + u8 chip_id;
>> + u16 baseline;
>>
>> /* Read Verbose Config Version Info */
>> skb = btbcm_read_verbose_config(hdev);
>> if (IS_ERR(skb))
>> return PTR_ERR(skb);
>> -
>> + chip_id = skb->data[1];
>> + baseline = skb->data[3] | (skb->data[4] << 8);
>
>This is not really safe, you shouldn't attempt to access skb->data without first checking skb->len, actually it would be much better that >you would use skb_pull_data which does skb->len check before pulling data.
I think it could be safe because its length is checked inside btbcm_read_verbose_config() as below.
Please let me know if further checking is needed.
>>>
static struct sk_buff *btbcm_read_verbose_config(struct hci_dev *hdev)
{
struct sk_buff *skb;
skb = __hci_cmd_sync(hdev, 0xfc79, 0, NULL, HCI_INIT_TIMEOUT);
if (IS_ERR(skb)) {
bt_dev_err(hdev, "BCM: Read verbose config info failed (%ld)",
PTR_ERR(skb));
return skb;
}
if (skb->len != 7) {
bt_dev_err(hdev, "BCM: Verbose config length mismatch");
kfree_skb(skb);
return ERR_PTR(-EIO);
}
return skb;
}
<<<
Best Regards,
Nobuaki Tsunashima