diff mbox series

[v3] Bluetooth: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER to CYW4373

Message ID 20240522081735.469503-1-nobuaki.tsunashima@infineon.com (mailing list archive)
State New, archived
Headers show
Series [v3] Bluetooth: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER to CYW4373 | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch success CheckPatch PASS
tedd_an/GitLint fail 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 19: B1 Line exceeds max length (82>80): "V2 -> V3: Fix a few coding style warnings and change the subject as more specific."
tedd_an/SubjectPrefix success Gitlint PASS
tedd_an/BuildKernel success BuildKernel PASS
tedd_an/CheckAllWarning success CheckAllWarning PASS
tedd_an/CheckSparse success CheckSparse PASS
tedd_an/CheckSmatch fail CheckSparse: FAIL: 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
tedd_an/BuildKernel32 success BuildKernel32 PASS
tedd_an/TestRunnerSetup success TestRunnerSetup PASS
tedd_an/TestRunner_l2cap-tester success TestRunner PASS
tedd_an/TestRunner_iso-tester success TestRunner PASS
tedd_an/TestRunner_bnep-tester success TestRunner PASS
tedd_an/TestRunner_mgmt-tester success TestRunner PASS
tedd_an/TestRunner_rfcomm-tester success TestRunner PASS
tedd_an/TestRunner_sco-tester success TestRunner PASS
tedd_an/TestRunner_ioctl-tester success TestRunner PASS
tedd_an/TestRunner_mesh-tester success TestRunner PASS
tedd_an/TestRunner_smp-tester success TestRunner PASS
tedd_an/TestRunner_userchan-tester success TestRunner PASS
tedd_an/IncrementalBuild success Incremental Build PASS

Commit Message

Nobuaki Tsunashima May 22, 2024, 8:17 a.m. UTC 
From: Nobuaki Tsunashima <Nobuaki.Tsunashima@infineon.com>

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. Due to the issue, Bluetooth driver of 5.15 and later kernel fails
to hci up.

Especially in USB i/f case, it would be difficult to download patch FW that
includes Its fix unless hci is up.

The patch forces the driver to skip LE_Read_Transmit_Power Command when it
detects CYW4373 with ROM FW build.

Signed-off-by: Nobuaki Tsunashima <Nobuaki.Tsunashima@infineon.com>
---
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(-)

Comments

bluez.test.bot@gmail.com May 22, 2024, 2:15 a.m. UTC | #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=854875

---Test result---

Test Summary:
CheckPatch                    PASS      0.83 seconds
GitLint                       FAIL      0.50 seconds
SubjectPrefix                 PASS      0.11 seconds
BuildKernel                   PASS      30.47 seconds
CheckAllWarning               PASS      33.62 seconds
CheckSparse                   PASS      39.56 seconds
CheckSmatch                   FAIL      35.04 seconds
BuildKernel32                 PASS      29.28 seconds
TestRunnerSetup               PASS      534.24 seconds
TestRunner_l2cap-tester       PASS      18.44 seconds
TestRunner_iso-tester         PASS      31.21 seconds
TestRunner_bnep-tester        PASS      4.77 seconds
TestRunner_mgmt-tester        PASS      120.94 seconds
TestRunner_rfcomm-tester      PASS      7.39 seconds
TestRunner_sco-tester         PASS      14.95 seconds
TestRunner_ioctl-tester       PASS      7.73 seconds
TestRunner_mesh-tester        PASS      5.85 seconds
TestRunner_smp-tester         PASS      6.87 seconds
TestRunner_userchan-tester    PASS      4.95 seconds
IncrementalBuild              PASS      28.69 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v3] Bluetooth: 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
19: 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
Paul Menzel May 22, 2024, 4:42 a.m. UTC | #2 
Dear Nobuaki,


Thank you for your patch and addressing the comments. Please note, that 
the time on the system you sent the patch from is in the future:

     Date: Wed, 22 May 2024 17:17:35 +0900

But:

     Received: from smtp9.infineon.com (smtp9.infineon.com [217.10.52.204])
     	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
     	(No client certificate requested)
     	by smtp.subspace.kernel.org (Postfix) with ESMTPS id 83EC328EA;
     	Wed, 22 May 2024 01:28:45 +0000 (UTC)

Am 22.05.24 um 10:17 schrieb Nobuaki Tsunashima:
> From: Nobuaki Tsunashima <Nobuaki.Tsunashima@infineon.com>

I forgot to add btbcm in the summary:

Bluetooth: btbcm: …

> 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. Due to the issue, Bluetooth driver of 5.15 and later kernel fails
> to hci up.

As written in the other thread, it’d be great if you bisected the commit.

> Especially in USB i/f case, it would be difficult to download patch FW that
> includes Its fix unless hci is up.

lowercase: its

Which firmware versions are fixed?

> The patch forces the driver to skip LE_Read_Transmit_Power Command when it
> detects CYW4373 with ROM FW build.

Maybe add something like:

The driver already contains infrastructure to apply the quirk, but 
currently it only supports DMI based matching. Add support to match by 
chip id and baseline, which ….

> Signed-off-by: Nobuaki Tsunashima <Nobuaki.Tsunashima@infineon.com>
> ---
> 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..c763e368d6ad 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;

Please use one space. (Please also check the line below.)

> +	u16 baseline;

Add a comment above the struct, what baseline means?

> +};
> +#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 */

Add one space after { and before }?

> +};
> +static bool btbcm_is_disable_broken_read_tx_power_by_chip_ver(u8 chip_id, u16 baseline)
> +{
> +	int i;
> +	int table_size = ARRAY_SIZE(disable_broken_read_transmit_power_by_chip_ver);

Use size_t?

> +	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);
> +

Commit 801b4c027b44 (Bluetooth: btbcm: disable read tx power for some 
Macs with the T2 Security chip) added the check in 
`btbcm_print_controller_features()`? No idea, where the best place is.

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

Order 0x04b4 before 0x04ca?

>   	/* Broadcom devices with vendor specific id */
>   	{ USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01),
>   	  .driver_info = BTUSB_BCM_PATCHRAM },


Kind regards,

Paul
Aditya Garg May 22, 2024, 6:58 a.m. UTC | #3 
Hi

> On 22 May 2024, at 10:13 AM, Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> 
> Dear Nobuaki,
> 
> 
> Thank you for your patch and addressing the comments. Please note, that the time on the system you sent the patch from is in the future:
> 
>    Date: Wed, 22 May 2024 17:17:35 +0900
> 
> But:
> 
>    Received: from smtp9.infineon.com (smtp9.infineon.com [217.10.52.204])
>        (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
>        (No client certificate requested)
>        by smtp.subspace.kernel.org (Postfix) with ESMTPS id 83EC328EA;
>        Wed, 22 May 2024 01:28:45 +0000 (UTC)
> 
>> Am 22.05.24 um 10:17 schrieb Nobuaki Tsunashima:
>> From: Nobuaki Tsunashima <Nobuaki.Tsunashima@infineon.com>
> 
> I forgot to add btbcm in the summary:
> 
> Bluetooth: btbcm: …
> 
>> 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. Due to the issue, Bluetooth driver of 5.15 and later kernel fails
>> to hci up.

I remember the LE Transmit power issue came up in 5.11 kernel, so if you are getting the issue starting from 5.15, you probably want to bisect.
> 
> As written in the other thread, it’d be great if you bisected the commit.
> 
>> Especially in USB i/f case, it would be difficult to download patch FW that
>> includes Its fix unless hci is up.
> 
> lowercase: its
> 
> Which firmware versions are fixed?
> 
>> The patch forces the driver to skip LE_Read_Transmit_Power Command when it
>> detects CYW4373 with ROM FW build.
> 
> Maybe add something like:
> 
> The driver already contains infrastructure to apply the quirk, but currently it only supports DMI based matching. Add support to match by chip id and baseline, which ….
> 
>> Signed-off-by: Nobuaki Tsunashima <Nobuaki.Tsunashima@infineon.com>
>> ---
>> 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..c763e368d6ad 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;
> 
> Please use one space. (Please also check the line below.)
> 
>> +    u16 baseline;
> 
> Add a comment above the struct, what baseline means?
> 
>> +};
>> +#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 */
> 
> Add one space after { and before }?
> 
You may want to rename the existing variable btbcm_is_disable_broken_read_tx_power to btbcm_is_disable_broken_read_tx_power_by_dmi to avoid confusion. Although, I'm not a maintainer so consider it as just a suggestion.
> 
>> +};
>> +static bool btbcm_is_disable_broken_read_tx_power_by_chip_ver(u8 chip_id, u16 baseline)
>> +{
>> +    int i;
>> +    int table_size = ARRAY_SIZE(disable_broken_read_transmit_power_by_chip_ver);
> 
> Use size_t?
> 
>> +    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);
>> +
> 
> Commit 801b4c027b44 (Bluetooth: btbcm: disable read tx power for some Macs with the T2 Security chip) added the check in `btbcm_print_controller_features()`? No idea, where the best place is.

I added the check in `btbcm_print_controller_features()` because the the issue was not being fixed at other places. I remember compiling and testing it at various other places. I'm not really sure why it specifically works in `btbcm_print_controller_features()`
> 
> 
>>      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 },
>> +
> 
> Order 0x04b4 before 0x04ca?
> 
>>      /* Broadcom devices with vendor specific id */
>>      { USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01),
>>        .driver_info = BTUSB_BCM_PATCHRAM },
> 
> 
> Kind regards,
> 
> Paul

Regards

Aditya
Nobuaki Tsunashima May 22, 2024, 7:03 a.m. UTC | #4 
Hi Aditya,

>> 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. Due to the issue, Bluetooth driver of 5.15 and later kernel 
>> fails to hci up.
>
> I remember the LE Transmit power issue came up in 5.11 kernel, so if you are getting the issue starting
> from 5.15, you probably want to bisect.
Yes, I've just found below commit added sending LE_Read_Transmit_Power command, introduced in v5.11.
https://github.com/torvalds/linux/commit/7c395ea521e6c8d77f643be61bf2f0f3a1f5b3e8

Best Regards,
Nobuaki Tsunashima

-----Original Message-----
From: Aditya Garg <gargaditya08@live.com> 
Sent: Wednesday, May 22, 2024 3:59 PM
To: Paul Menzel <pmenzel@molgen.mpg.de>
Cc: Tsunashima Nobuaki (SMD C3 JP RM WLS AE) <Nobuaki.Tsunashima@infineon.com>; Marcel Holtmann <marcel@holtmann.org>; Luiz Augusto von Dentz <luiz.dentz@gmail.com>; linux-bluetooth@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] Bluetooth: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER to CYW4373

Caution: This e-mail originated outside Infineon Technologies. Do not click on links or open attachments unless you validate it is safe<https://intranet-content.infineon.com/explore/aboutinfineon/rules/informationsecurity/ug/SocialEngineering/Pages/SocialEngineeringElements_en.aspx>.



Hi

> On 22 May 2024, at 10:13 AM, Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Nobuaki,
>
>
> Thank you for your patch and addressing the comments. Please note, that the time on the system you sent the patch from is in the future:
>
>    Date: Wed, 22 May 2024 17:17:35 +0900
>
> But:
>
>    Received: from smtp9.infineon.com (smtp9.infineon.com [217.10.52.204])
>        (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
>        (No client certificate requested)
>        by smtp.subspace.kernel.org (Postfix) with ESMTPS id 83EC328EA;
>        Wed, 22 May 2024 01:28:45 +0000 (UTC)
>
>> Am 22.05.24 um 10:17 schrieb Nobuaki Tsunashima:
>> From: Nobuaki Tsunashima <Nobuaki.Tsunashima@infineon.com>
>
> I forgot to add btbcm in the summary:
>
> Bluetooth: btbcm: …
>
>> 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. Due to the issue, Bluetooth driver of 5.15 and later kernel 
>> fails to hci up.

I remember the LE Transmit power issue came up in 5.11 kernel, so if you are getting the issue starting from 5.15, you probably want to bisect.
>
> As written in the other thread, it’d be great if you bisected the commit.
>
>> Especially in USB i/f case, it would be difficult to download patch 
>> FW that includes Its fix unless hci is up.
>
> lowercase: its
>
> Which firmware versions are fixed?
>
>> The patch forces the driver to skip LE_Read_Transmit_Power Command 
>> when it detects CYW4373 with ROM FW build.
>
> Maybe add something like:
>
> The driver already contains infrastructure to apply the quirk, but currently it only supports DMI based matching. Add support to match by chip id and baseline, which ….
>
>> Signed-off-by: Nobuaki Tsunashima <Nobuaki.Tsunashima@infineon.com>
>> ---
>> 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..c763e368d6ad 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;
>
> Please use one space. (Please also check the line below.)
>
>> +    u16 baseline;
>
> Add a comment above the struct, what baseline means?
>
>> +};
>> +#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 */
>
> Add one space after { and before }?
>
You may want to rename the existing variable btbcm_is_disable_broken_read_tx_power to btbcm_is_disable_broken_read_tx_power_by_dmi to avoid confusion. Although, I'm not a maintainer so consider it as just a suggestion.
>
>> +};
>> +static bool btbcm_is_disable_broken_read_tx_power_by_chip_ver(u8 
>> +chip_id, u16 baseline) {
>> +    int i;
>> +    int table_size = 
>> +ARRAY_SIZE(disable_broken_read_transmit_power_by_chip_ver);
>
> Use size_t?
>
>> +    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);
>> +
>
> Commit 801b4c027b44 (Bluetooth: btbcm: disable read tx power for some Macs with the T2 Security chip) added the check in `btbcm_print_controller_features()`? No idea, where the best place is.

I added the check in `btbcm_print_controller_features()` because the the issue was not being fixed at other places. I remember compiling and testing it at various other places. I'm not really sure why it specifically works in `btbcm_print_controller_features()`
>
>
>>      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 },
>> +
>
> Order 0x04b4 before 0x04ca?
>
>>      /* Broadcom devices with vendor specific id */
>>      { USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01),
>>        .driver_info = BTUSB_BCM_PATCHRAM },
>
>
> Kind regards,
>
> Paul

Regards

Aditya
Aditya Garg May 22, 2024, 8:12 a.m. UTC | #5 
Hi

> On 22 May 2024, at 12:34 PM, Nobuaki.Tsunashima@infineon.com wrote:
> 
> Hi Aditya,
> 
>>> 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. Due to the issue, Bluetooth driver of 5.15 and later kernel
>>> fails to hci up.
>> 
>> I remember the LE Transmit power issue came up in 5.11 kernel, so if you are getting the issue starting
>> from 5.15, you probably want to bisect.
> Yes, I've just found below commit added sending LE_Read_Transmit_Power command, introduced in v5.11.
> https://github.com/torvalds/linux/commit/7c395ea521e6c8d77f643be61bf2f0f3a1f5b3e8

Yes that's the same commit which had caused issues for me. But was the device working from 5.11 to 5.14? If yes, what broke it in 5.15?

I'm sorry if I am missing any information, I was added by Paul in this thread, probably because I introduced this quirk in the kernel to fix Bluetooth on my Mac.
> 
> Best Regards,
> Nobuaki Tsunashima
> 
> -----Original Message-----
> From: Aditya Garg <gargaditya08@live.com>
> Sent: Wednesday, May 22, 2024 3:59 PM
> To: Paul Menzel <pmenzel@molgen.mpg.de>
> Cc: Tsunashima Nobuaki (SMD C3 JP RM WLS AE) <Nobuaki.Tsunashima@infineon.com>; Marcel Holtmann <marcel@holtmann.org>; Luiz Augusto von Dentz <luiz.dentz@gmail.com>; linux-bluetooth@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3] Bluetooth: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER to CYW4373
> 
> Caution: This e-mail originated outside Infineon Technologies. Do not click on links or open attachments unless you validate it is safe<https://intranet-content.infineon.com/explore/aboutinfineon/rules/informationsecurity/ug/SocialEngineering/Pages/SocialEngineeringElements_en.aspx>.
> 
> 
> 
> Hi
> 
>> On 22 May 2024, at 10:13 AM, Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>> 
>> Dear Nobuaki,
>> 
>> 
>> Thank you for your patch and addressing the comments. Please note, that the time on the system you sent the patch from is in the future:
>> 
>>   Date: Wed, 22 May 2024 17:17:35 +0900
>> 
>> But:
>> 
>>   Received: from smtp9.infineon.com (smtp9.infineon.com [217.10.52.204])
>>       (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
>>       (No client certificate requested)
>>       by smtp.subspace.kernel.org (Postfix) with ESMTPS id 83EC328EA;
>>       Wed, 22 May 2024 01:28:45 +0000 (UTC)
>> 
>>>> Am 22.05.24 um 10:17 schrieb Nobuaki Tsunashima:
>>> From: Nobuaki Tsunashima <Nobuaki.Tsunashima@infineon.com>
>> 
>> I forgot to add btbcm in the summary:
>> 
>> Bluetooth: btbcm: …
>> 
>>> 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. Due to the issue, Bluetooth driver of 5.15 and later kernel
>>> fails to hci up.
> 
> I remember the LE Transmit power issue came up in 5.11 kernel, so if you are getting the issue starting from 5.15, you probably want to bisect.
>> 
>> As written in the other thread, it’d be great if you bisected the commit.
>> 
>>> Especially in USB i/f case, it would be difficult to download patch
>>> FW that includes Its fix unless hci is up.
>> 
>> lowercase: its
>> 
>> Which firmware versions are fixed?
>> 
>>> The patch forces the driver to skip LE_Read_Transmit_Power Command
>>> when it detects CYW4373 with ROM FW build.
>> 
>> Maybe add something like:
>> 
>> The driver already contains infrastructure to apply the quirk, but currently it only supports DMI based matching. Add support to match by chip id and baseline, which ….
>> 
>>> Signed-off-by: Nobuaki Tsunashima <Nobuaki.Tsunashima@infineon.com>
>>> ---
>>> 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..c763e368d6ad 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;
>> 
>> Please use one space. (Please also check the line below.)
>> 
>>> +    u16 baseline;
>> 
>> Add a comment above the struct, what baseline means?
>> 
>>> +};
>>> +#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 */
>> 
>> Add one space after { and before }?
>> 
> You may want to rename the existing variable btbcm_is_disable_broken_read_tx_power to btbcm_is_disable_broken_read_tx_power_by_dmi to avoid confusion. Although, I'm not a maintainer so consider it as just a suggestion.
>> 
>>> +};
>>> +static bool btbcm_is_disable_broken_read_tx_power_by_chip_ver(u8
>>> +chip_id, u16 baseline) {
>>> +    int i;
>>> +    int table_size =
>>> +ARRAY_SIZE(disable_broken_read_transmit_power_by_chip_ver);
>> 
>> Use size_t?
>> 
>>> +    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);
>>> +
>> 
>> Commit 801b4c027b44 (Bluetooth: btbcm: disable read tx power for some Macs with the T2 Security chip) added the check in `btbcm_print_controller_features()`? No idea, where the best place is.
> 
> I added the check in `btbcm_print_controller_features()` because the the issue was not being fixed at other places. I remember compiling and testing it at various other places. I'm not really sure why it specifically works in `btbcm_print_controller_features()`
>> 
>> 
>>>     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 },
>>> +
>> 
>> Order 0x04b4 before 0x04ca?
>> 
>>>     /* Broadcom devices with vendor specific id */
>>>     { USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01),
>>>       .driver_info = BTUSB_BCM_PATCHRAM },
>> 
>> 
>> Kind regards,
>> 
>> Paul
> 
> Regards
> 
> Aditya
Nobuaki Tsunashima May 22, 2024, 8:13 a.m. UTC | #6 
Hi Paul,

Thanks for your comments.

> Please note, that the time on the system you sent the patch from is in the future:
>
>     Date: Wed, 22 May 2024 17:17:35 +0900

It was due to wrong time setting on my Linux system. I've corrected it.

> I forgot to add btbcm in the summary:
> Bluetooth: btbcm: …
Sure. I will add in next version.

>> 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. Due to the issue, Bluetooth driver of 5.15 and later kernel 
>>  fails to hci up.
>
> As written in the other thread, it’d be great if you bisected the commit.
As I talked in another email, the issue happened after below commit. In this case, should I
add "Fixes: " tag?

>> Especially in USB i/f case, it would be difficult to download patch FW 
>> that includes Its fix unless hci is up.
>
>lowercase: its
Acked.

> Which firmware versions are fixed?
CYW4373A0_001.001.025.0081.* and later version are fixed.

>> The patch forces the driver to skip LE_Read_Transmit_Power Command 
>> when it detects CYW4373 with ROM FW build.
>
>Maybe add something like:
>
>The driver already contains infrastructure to apply the quirk, but currently it only
>supports DMI based matching. Add support to match by chip id and baseline, which ….
Acked.

>> --- 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;
>
> Please use one space. (Please also check the line below.)
Acked.

>> +     u16 baseline;
>
> Add a comment above the struct, what baseline means?
It means baseline version of patch FW. I will add a comment there.

>> +};
>> +#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 */
>
> Add one space after { and before }?
Acked.

>> +};
>> +static bool btbcm_is_disable_broken_read_tx_power_by_chip_ver(u8 
>> +chip_id, u16 baseline) {
>> +     int i;
>> +     int table_size = 
>> +ARRAY_SIZE(disable_broken_read_transmit_power_by_chip_ver);
>
> Use size_t?
Acked.

>>   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);
>> +

> Commit 801b4c027b44 (Bluetooth: btbcm: disable read tx power for some Macs with the
> T2 Security chip) added the check in `btbcm_print_controller_features()`? No idea, where
> the best place is.
I wondered it too but finally decided setting the bit inside btbcm_read_info() to avoid
duplicated call of btbcm_read_verbose_config().

>> 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 },
>> +
>
> Order 0x04b4 before 0x04ca?
Entire order of the table is not sorted but looks like random order.
The reason of the entry location is just before of Broadcom ID as our chip
was derived from them.

>       /* Broadcom devices with vendor specific id */
>       { USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01),
>         .driver_info = BTUSB_BCM_PATCHRAM },

> Kind regards,

> Paul

Best Regards,
Nobuaki Tsunashima
Nobuaki Tsunashima May 22, 2024, 8:19 a.m. UTC | #7 
Hi Aditya,

> Yes that's the same commit which had caused issues for me. But was the device
> working from 5.11 to 5.14? If yes, what broke it in 5.15?
Last version confirmed worked well was 5.10 and the first version the issue was observed with was 5.15,
So, interim version from 5.11 to 5.14 are not yet tested. I need correct the commit comment anyway.

Best Regards,
Nobuaki Tsunashima

-----Original Message-----
From: Aditya Garg <gargaditya08@live.com> 
Sent: Wednesday, May 22, 2024 5:13 PM
To: Tsunashima Nobuaki (SMD C3 JP RM WLS AE) <Nobuaki.Tsunashima@infineon.com>
Cc: pmenzel@molgen.mpg.de; marcel@holtmann.org; luiz.dentz@gmail.com; linux-bluetooth@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] Bluetooth: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER to CYW4373

Caution: This e-mail originated outside Infineon Technologies. Do not click on links or open attachments unless you validate it is safe<https://intranet-content.infineon.com/explore/aboutinfineon/rules/informationsecurity/ug/SocialEngineering/Pages/SocialEngineeringElements_en.aspx>.



Hi

> On 22 May 2024, at 12:34 PM, Nobuaki.Tsunashima@infineon.com wrote:
>
> Hi Aditya,
>
>>> 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. Due to the issue, Bluetooth driver of 5.15 and later kernel 
>>> fails to hci up.
>>
>> I remember the LE Transmit power issue came up in 5.11 kernel, so if 
>> you are getting the issue starting from 5.15, you probably want to bisect.
> Yes, I've just found below commit added sending LE_Read_Transmit_Power command, introduced in v5.11.
> https://github.com/torvalds/linux/commit/7c395ea521e6c8d77f643be61bf2f
> 0f3a1f5b3e8

Yes that's the same commit which had caused issues for me. But was the device working from 5.11 to 5.14? If yes, what broke it in 5.15?

I'm sorry if I am missing any information, I was added by Paul in this thread, probably because I introduced this quirk in the kernel to fix Bluetooth on my Mac.
>
> Best Regards,
> Nobuaki Tsunashima
>
> -----Original Message-----
> From: Aditya Garg <gargaditya08@live.com>
> Sent: Wednesday, May 22, 2024 3:59 PM
> To: Paul Menzel <pmenzel@molgen.mpg.de>
> Cc: Tsunashima Nobuaki (SMD C3 JP RM WLS AE) 
> <Nobuaki.Tsunashima@infineon.com>; Marcel Holtmann 
> <marcel@holtmann.org>; Luiz Augusto von Dentz <luiz.dentz@gmail.com>; 
> linux-bluetooth@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3] Bluetooth: Apply 
> HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER to CYW4373
>
> Caution: This e-mail originated outside Infineon Technologies. Do not click on links or open attachments unless you validate it is safe<https://intranet-content.infineon.com/explore/aboutinfineon/rules/informationsecurity/ug/SocialEngineering/Pages/SocialEngineeringElements_en.aspx>.
>
>
>
> Hi
>
>> On 22 May 2024, at 10:13 AM, Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>>
>> Dear Nobuaki,
>>
>>
>> Thank you for your patch and addressing the comments. Please note, that the time on the system you sent the patch from is in the future:
>>
>>   Date: Wed, 22 May 2024 17:17:35 +0900
>>
>> But:
>>
>>   Received: from smtp9.infineon.com (smtp9.infineon.com [217.10.52.204])
>>       (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
>>       (No client certificate requested)
>>       by smtp.subspace.kernel.org (Postfix) with ESMTPS id 83EC328EA;
>>       Wed, 22 May 2024 01:28:45 +0000 (UTC)
>>
>>>> Am 22.05.24 um 10:17 schrieb Nobuaki Tsunashima:
>>> From: Nobuaki Tsunashima <Nobuaki.Tsunashima@infineon.com>
>>
>> I forgot to add btbcm in the summary:
>>
>> Bluetooth: btbcm: …
>>
>>> 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. Due to the issue, Bluetooth driver of 5.15 and later kernel 
>>> fails to hci up.
>
> I remember the LE Transmit power issue came up in 5.11 kernel, so if you are getting the issue starting from 5.15, you probably want to bisect.
>>
>> As written in the other thread, it’d be great if you bisected the commit.
>>
>>> Especially in USB i/f case, it would be difficult to download patch 
>>> FW that includes Its fix unless hci is up.
>>
>> lowercase: its
>>
>> Which firmware versions are fixed?
>>
>>> The patch forces the driver to skip LE_Read_Transmit_Power Command 
>>> when it detects CYW4373 with ROM FW build.
>>
>> Maybe add something like:
>>
>> The driver already contains infrastructure to apply the quirk, but currently it only supports DMI based matching. Add support to match by chip id and baseline, which ….
>>
>>> Signed-off-by: Nobuaki Tsunashima <Nobuaki.Tsunashima@infineon.com>
>>> ---
>>> 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..c763e368d6ad 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;
>>
>> Please use one space. (Please also check the line below.)
>>
>>> +    u16 baseline;
>>
>> Add a comment above the struct, what baseline means?
>>
>>> +};
>>> +#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 */
>>
>> Add one space after { and before }?
>>
> You may want to rename the existing variable btbcm_is_disable_broken_read_tx_power to btbcm_is_disable_broken_read_tx_power_by_dmi to avoid confusion. Although, I'm not a maintainer so consider it as just a suggestion.
>>
>>> +};
>>> +static bool btbcm_is_disable_broken_read_tx_power_by_chip_ver(u8
>>> +chip_id, u16 baseline) {
>>> +    int i;
>>> +    int table_size =
>>> +ARRAY_SIZE(disable_broken_read_transmit_power_by_chip_ver);
>>
>> Use size_t?
>>
>>> +    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);
>>> +
>>
>> Commit 801b4c027b44 (Bluetooth: btbcm: disable read tx power for some Macs with the T2 Security chip) added the check in `btbcm_print_controller_features()`? No idea, where the best place is.
>
> I added the check in `btbcm_print_controller_features()` because the 
> the issue was not being fixed at other places. I remember compiling 
> and testing it at various other places. I'm not really sure why it 
> specifically works in `btbcm_print_controller_features()`
>>
>>
>>>     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 },
>>> +
>>
>> Order 0x04b4 before 0x04ca?
>>
>>>     /* Broadcom devices with vendor specific id */
>>>     { USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01),
>>>       .driver_info = BTUSB_BCM_PATCHRAM },
>>
>>
>> Kind regards,
>>
>> Paul
>
> Regards
>
> Aditya
Aditya Garg May 22, 2024, 8:21 a.m. UTC | #8 
> On 22 May 2024, at 1:49 PM, Nobuaki.Tsunashima@infineon.com wrote:
> 
> Hi Aditya,
> 
>> Yes that's the same commit which had caused issues for me. But was the device
>> working from 5.11 to 5.14? If yes, what broke it in 5.15?
> Last version confirmed worked well was 5.10 and the first version the issue was observed with was 5.15,

Makes sense then

> So, interim version from 5.11 to 5.14 are not yet tested. I need correct the commit comment anyway.
diff mbox series

Patch

diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
index 0a5445ac5e1b..c763e368d6ad 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;
+	u16 baseline;
+};
+#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;
+	int 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 },