[v5] Bluetooth: btbcm: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER to CYW4373

Commit Message

Nobuaki Tsunashima July 5, 2024, 1:15 a.m. UTC

From: Nobuaki Tsunashima <[e-mail]>

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 <[e-mail]>
---
V4 -> V5: Use skb_pull_data() to access skb->data as safer manner.
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 | 41 +++++++++++++++++++++++++++++++++++++--
 drivers/bluetooth/btusb.c |  4 ++++
 2 files changed, 43 insertions(+), 2 deletions(-)

Comments

Luiz Augusto von Dentz July 5, 2024, 1:34 a.m. UTC | #1

Hi Nobuaki,

On Thu, Jul 4, 2024 at 9:16 PM Nobuaki Tsunashima
<[e-mail]> wrote:
>
> From: Nobuaki Tsunashima <[e-mail]>
>
> 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 <[e-mail]>
> ---
> V4 -> V5: Use skb_pull_data() to access skb->data as safer manner.
> 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 | 41 +++++++++++++++++++++++++++++++++++++--
>  drivers/bluetooth/btusb.c |  4 ++++
>  2 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
> index 0a5445ac5e1b..dd7262a8dc8e 100644
> --- a/drivers/bluetooth/btbcm.c
> +++ b/drivers/bluetooth/btbcm.c
> @@ -437,16 +437,53 @@ 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 */
> +};

Can we have a little less verbose names? e.g. broken_read_tx_power and
btbcm_broken_read_tx_power sounds a lot better imo.

> +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);
> -
> -       bt_dev_info(hdev, "BCM: chip id %u", skb->data[1]);
> +       skb_pull_data(skb, 1);
> +       chip_id = skb_pull_data(skb, sizeof(*chip_id));
> +       skb_pull_data(skb, 1);
> +       baseline = skb_pull_data(skb, sizeof(*baseline));
> +
> +       if (chip_id) {
> +               bt_dev_info(hdev, "BCM: chip id %u", *chip_id);
> +
> +               if (baseline) {
> +                       /* 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);
> +               }
> +       }
>         kfree_skb(skb);
>
>         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
>

Nobuaki Tsunashima July 5, 2024, 2:07 a.m. UTC | #2

Hi Luiz,

Thanks for your comment.

>> +#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 */
>> +};
>
> Can we have a little less verbose names? e.g. broken_read_tx_power and btbcm_broken_read_tx_power sounds a lot better imo.

We already have a table named "disable_broken_read_transmit_power" as below.

> static const struct dmi_system_id disable_broken_read_transmit_power[] = {
>	{
>		 .matches = {
>			DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
>			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,1"),
>		},
>	},

So, as my new table is same role of the original table but referring chip version, I think the name is straight forward.

Or, we may shorten name of the original table as well like below. Do you like it?

Original table
static const struct dmi_system_id broken_read_tx_power_dmi[] 

New table
static const struc bcm_chip_version_table broken_read_tx_power_chip_ver[]

Best Regards,
Nobuaki Tsunashima

--
---Original Message-----
From: Luiz Augusto von Dentz <[e-mail]> 
Sent: Friday, July 5, 2024 10:34 AM
To: Tsunashima Nobuaki (SMD C3 JP RM WLS AE) <[e-mail]>
Cc: Marcel Holtmann <[e-mail]>; [e-mail]; [e-mail]
Subject: Re: [PATCH v5] Bluetooth: btbcm: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER to CYW4373

Caution: This e-mail originated outside Infineon Technologies. Please be cautious when sharing information or opening attachments especially from unknown senders. Refer to our intranet guide<https://intranet-content.infineon.com/explore/aboutinfineon/rules/informationsecurity/ug/SocialEngineering/Pages/SocialEngineeringElements_en.aspx> to help you identify Phishing email.



Hi Nobuaki,

On Thu, Jul 4, 2024 at 9:16 PM Nobuaki Tsunashima <[e-mail]> wrote:
>
> From: Nobuaki Tsunashima <[e-mail]>
>
> 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 <[e-mail]>
> ---
> V4 -> V5: Use skb_pull_data() to access skb->data as safer manner.
> 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 | 41 
> +++++++++++++++++++++++++++++++++++++--
>  drivers/bluetooth/btusb.c |  4 ++++
>  2 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c 
> index 0a5445ac5e1b..dd7262a8dc8e 100644
> --- a/drivers/bluetooth/btbcm.c
> +++ b/drivers/bluetooth/btbcm.c
> @@ -437,16 +437,53 @@ 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 */
> +};

Can we have a little less verbose names? e.g. broken_read_tx_power and btbcm_broken_read_tx_power sounds a lot better imo.

> +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);
> -
> -       bt_dev_info(hdev, "BCM: chip id %u", skb->data[1]);
> +       skb_pull_data(skb, 1);
> +       chip_id = skb_pull_data(skb, sizeof(*chip_id));
> +       skb_pull_data(skb, 1);
> +       baseline = skb_pull_data(skb, sizeof(*baseline));
> +
> +       if (chip_id) {
> +               bt_dev_info(hdev, "BCM: chip id %u", *chip_id);
> +
> +               if (baseline) {
> +                       /* 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);
> +               }
> +       }
>         kfree_skb(skb);
>
>         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

[e-mail] July 5, 2024, 2:24 a.m. UTC | #3

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=868600

---Test result---

Test Summary:
CheckPatch                    PASS      0.95 seconds
GitLint                       FAIL      0.50 seconds
SubjectPrefix                 PASS      0.22 seconds
BuildKernel                   PASS      29.73 seconds
CheckAllWarning               PASS      32.67 seconds
CheckSparse                   PASS      38.10 seconds
CheckSmatch                   PASS      107.93 seconds
BuildKernel32                 PASS      36.90 seconds
TestRunnerSetup               PASS      527.27 seconds
TestRunner_l2cap-tester       PASS      22.11 seconds
TestRunner_iso-tester         PASS      37.77 seconds
TestRunner_bnep-tester        PASS      4.86 seconds
TestRunner_mgmt-tester        PASS      112.17 seconds
TestRunner_rfcomm-tester      PASS      7.53 seconds
TestRunner_sco-tester         PASS      23.65 seconds
TestRunner_ioctl-tester       PASS      8.17 seconds
TestRunner_mesh-tester        PASS      6.05 seconds
TestRunner_smp-tester         PASS      7.04 seconds
TestRunner_userchan-tester    PASS      5.06 seconds
IncrementalBuild              PASS      28.16 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v5] 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
24: B1 Line exceeds max length (82>80): "V2 -> V3: Fix a few coding style warnings and change the subject as more specific."


---
Regards,
Linux Bluetooth

Nobuaki Tsunashima July 30, 2024, 5:32 a.m. UTC | #4

Hello,

Please let me know if there are any additional actions needed from my end to facilitate progress on this patch.
Do you like to shorten names of original and newly added tables?
Your feedback would be greatly appreciated.

Thank you for your attention to this matter.

Best Regards,
Nobuaki Tsunashima

-----Original Message-----
From: Tsunashima Nobuaki (SMD C3 JP RM WLS AE) 
Sent: Friday, July 5, 2024 11:07 AM
To: 'Luiz Augusto von Dentz' <[e-mail]>
Cc: Marcel Holtmann <[e-mail]>; [e-mail]; [e-mail]
Subject: RE: [PATCH v5] Bluetooth: btbcm: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER to CYW4373

Hi Luiz,

Thanks for your comment.

>> +#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 */
>> +};
>
> Can we have a little less verbose names? e.g. broken_read_tx_power and btbcm_broken_read_tx_power sounds a lot better imo.

We already have a table named "disable_broken_read_transmit_power" as below.

> static const struct dmi_system_id disable_broken_read_transmit_power[] = {
>	{
>		 .matches = {
>			DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
>			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,1"),
>		},
>	},

So, as my new table is same role of the original table but referring chip version, I think the name is straight forward.

Or, we may shorten name of the original table as well like below. Do you like it?

Original table
static const struct dmi_system_id broken_read_tx_power_dmi[] 

New table
static const struc bcm_chip_version_table broken_read_tx_power_chip_ver[]

Best Regards,
Nobuaki Tsunashima

--
---Original Message-----
From: Luiz Augusto von Dentz <[e-mail]>
Sent: Friday, July 5, 2024 10:34 AM
To: Tsunashima Nobuaki (SMD C3 JP RM WLS AE) <[e-mail]>
Cc: Marcel Holtmann <[e-mail]>; [e-mail]; [e-mail]
Subject: Re: [PATCH v5] Bluetooth: btbcm: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER to CYW4373

Caution: This e-mail originated outside Infineon Technologies. Please be cautious when sharing information or opening attachments especially from unknown senders. Refer to our intranet guide<https://intranet-content.infineon.com/explore/aboutinfineon/rules/informationsecurity/ug/SocialEngineering/Pages/SocialEngineeringElements_en.aspx> to help you identify Phishing email.



Hi Nobuaki,

On Thu, Jul 4, 2024 at 9:16 PM Nobuaki Tsunashima <[e-mail]> wrote:
>
> From: Nobuaki Tsunashima <[e-mail]>
>
> 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 <[e-mail]>
> ---
> V4 -> V5: Use skb_pull_data() to access skb->data as safer manner.
> 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 | 41
> +++++++++++++++++++++++++++++++++++++--
>  drivers/bluetooth/btusb.c |  4 ++++
>  2 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c 
> index 0a5445ac5e1b..dd7262a8dc8e 100644
> --- a/drivers/bluetooth/btbcm.c
> +++ b/drivers/bluetooth/btbcm.c
> @@ -437,16 +437,53 @@ 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 */
> +};

Can we have a little less verbose names? e.g. broken_read_tx_power and btbcm_broken_read_tx_power sounds a lot better imo.

> +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);
> -
> -       bt_dev_info(hdev, "BCM: chip id %u", skb->data[1]);
> +       skb_pull_data(skb, 1);
> +       chip_id = skb_pull_data(skb, sizeof(*chip_id));
> +       skb_pull_data(skb, 1);
> +       baseline = skb_pull_data(skb, sizeof(*baseline));
> +
> +       if (chip_id) {
> +               bt_dev_info(hdev, "BCM: chip id %u", *chip_id);
> +
> +               if (baseline) {
> +                       /* 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);
> +               }
> +       }
>         kfree_skb(skb);
>
>         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

Nobuaki Tsunashima Aug. 23, 2024, 6:57 a.m. UTC | #5

Hi Luiz,

Let me refrain my question below.

As you proposed to shorten a table name, "disable_broken_read_transmit_power_by_chip_ver",
added in my patch, we may be better to shorten another table name, 
" disable_broken_read_transmit_power", that the existing code already has.

So, my proposal is to change both names as below.

1) From " disable_broken_read_transmit_power" to "broken_read_tx_power_dmi"
2) From " disable_broken_read_transmit_power_by_chip_ver " to " broken_read_tx_power_chip_ver"

Does it make sense?

Best Regards,
Nobuaki Tsunashima

-----Original Message-----
From: Tsunashima Nobuaki (SMD C3 JP RM WLS AE) 
Sent: Tuesday, July 30, 2024 2:32 PM
To: 'Luiz Augusto von Dentz' <[e-mail]>
Cc: 'Marcel Holtmann' <[e-mail]>; '[e-mail]' <[e-mail]>; '[e-mail]' <[e-mail]>
Subject: RE: [PATCH v5] Bluetooth: btbcm: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER to CYW4373

Hello,

Please let me know if there are any additional actions needed from my end to facilitate progress on this patch.
Do you like to shorten names of original and newly added tables?
Your feedback would be greatly appreciated.

Thank you for your attention to this matter.

Best Regards,
Nobuaki Tsunashima

-----Original Message-----
From: Tsunashima Nobuaki (SMD C3 JP RM WLS AE)
Sent: Friday, July 5, 2024 11:07 AM
To: 'Luiz Augusto von Dentz' <[e-mail]>
Cc: Marcel Holtmann <[e-mail]>; [e-mail]; [e-mail]
Subject: RE: [PATCH v5] Bluetooth: btbcm: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER to CYW4373

Hi Luiz,

Thanks for your comment.

>> +#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 */
>> +};
>
> Can we have a little less verbose names? e.g. broken_read_tx_power and btbcm_broken_read_tx_power sounds a lot better imo.

We already have a table named "disable_broken_read_transmit_power" as below.

> static const struct dmi_system_id disable_broken_read_transmit_power[] = {
>	{
>		 .matches = {
>			DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
>			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,1"),
>		},
>	},

So, as my new table is same role of the original table but referring chip version, I think the name is straight forward.

Or, we may shorten name of the original table as well like below. Do you like it?

Original table
static const struct dmi_system_id broken_read_tx_power_dmi[] 

New table
static const struc bcm_chip_version_table broken_read_tx_power_chip_ver[]

Best Regards,
Nobuaki Tsunashima

--
---Original Message-----
From: Luiz Augusto von Dentz <[e-mail]>
Sent: Friday, July 5, 2024 10:34 AM
To: Tsunashima Nobuaki (SMD C3 JP RM WLS AE) <[e-mail]>
Cc: Marcel Holtmann <[e-mail]>; [e-mail]; [e-mail]
Subject: Re: [PATCH v5] Bluetooth: btbcm: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER to CYW4373

Caution: This e-mail originated outside Infineon Technologies. Please be cautious when sharing information or opening attachments especially from unknown senders. Refer to our intranet guide<https://intranet-content.infineon.com/explore/aboutinfineon/rules/informationsecurity/ug/SocialEngineering/Pages/SocialEngineeringElements_en.aspx> to help you identify Phishing email.



Hi Nobuaki,

On Thu, Jul 4, 2024 at 9:16 PM Nobuaki Tsunashima <[e-mail]> wrote:
>
> From: Nobuaki Tsunashima <[e-mail]>
>
> 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 <[e-mail]>
> ---
> V4 -> V5: Use skb_pull_data() to access skb->data as safer manner.
> 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 | 41
> +++++++++++++++++++++++++++++++++++++--
>  drivers/bluetooth/btusb.c |  4 ++++
>  2 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c 
> index 0a5445ac5e1b..dd7262a8dc8e 100644
> --- a/drivers/bluetooth/btbcm.c
> +++ b/drivers/bluetooth/btbcm.c
> @@ -437,16 +437,53 @@ 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 */
> +};

Can we have a little less verbose names? e.g. broken_read_tx_power and btbcm_broken_read_tx_power sounds a lot better imo.

> +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);
> -
> -       bt_dev_info(hdev, "BCM: chip id %u", skb->data[1]);
> +       skb_pull_data(skb, 1);
> +       chip_id = skb_pull_data(skb, sizeof(*chip_id));
> +       skb_pull_data(skb, 1);
> +       baseline = skb_pull_data(skb, sizeof(*baseline));
> +
> +       if (chip_id) {
> +               bt_dev_info(hdev, "BCM: chip id %u", *chip_id);
> +
> +               if (baseline) {
> +                       /* 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);
> +               }
> +       }
>         kfree_skb(skb);
>
>         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

Patch

diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
index 0a5445ac5e1b..dd7262a8dc8e 100644
--- a/drivers/bluetooth/btbcm.c
+++ b/drivers/bluetooth/btbcm.c
@@ -437,16 +437,53 @@  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);
-
-	bt_dev_info(hdev, "BCM: chip id %u", skb->data[1]);
+	skb_pull_data(skb, 1);
+	chip_id = skb_pull_data(skb, sizeof(*chip_id));
+	skb_pull_data(skb, 1);
+	baseline = skb_pull_data(skb, sizeof(*baseline));
+
+	if (chip_id) {
+		bt_dev_info(hdev, "BCM: chip id %u", *chip_id);
+
+		if (baseline) {
+			/* 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);
+		}
+	}
 	kfree_skb(skb);
 
 	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 },