| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <653173db3fd440028e166c2d0e3d922e@infineon.com>
Date: Wed, 22 May 2024 07:03:50 +0000
From: <Nobuaki.Tsunashima@...ineon.com>
To: <gargaditya08@...e.com>, <pmenzel@...gen.mpg.de>
CC: <marcel@...tmann.org>, <luiz.dentz@...il.com>,
<linux-bluetooth@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v3] Bluetooth: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER
to CYW4373
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@...e.com>
Sent: Wednesday, May 22, 2024 3:59 PM
To: Paul Menzel <pmenzel@...gen.mpg.de>
Cc: Tsunashima Nobuaki (SMD C3 JP RM WLS AE) <Nobuaki.Tsunashima@...ineon.com>; Marcel Holtmann <marcel@...tmann.org>; Luiz Augusto von Dentz <luiz.dentz@...il.com>; linux-bluetooth@...r.kernel.org; linux-kernel@...r.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@...gen.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@...ineon.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@...ineon.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
Powered by blists - more mailing lists