RE: [PATCH v3] Bluetooth: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER to CYW4373

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxx> 
Sent: Wednesday, May 22, 2024 3:59 PM
To: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
Cc: Tsunashima Nobuaki (SMD C3 JP RM WLS AE) <Nobuaki.Tsunashima@xxxxxxxxxxxx>; Marcel Holtmann <marcel@xxxxxxxxxxxx>; Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>; linux-bluetooth@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
>> ---
>> 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




[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux