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
