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
