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

[Aditya Garg]

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