Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-4935-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240524013127.434500-1-nobuaki.tsunashima@infineon.com>
In-Reply-To: <20240524013127.434500-1-nobuaki.tsunashima@infineon.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Fri, 24 May 2024 15:22:22 -0400
Message-ID: <CABBYNZ+yvQL0KBagUXtzrBUFmFxp-ek_5aFDJNVyUKsA-xJ-Zg@mail.gmail.com>
Subject: Re: [PATCH v4] Bluetooth: btbcm: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER
 to CYW4373
To: Nobuaki Tsunashima <nobuaki.tsunashima@infineon.com>
Cc: Marcel Holtmann <marcel@holtmann.org>, linux-bluetooth@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Nobuaki,

On Thu, May 23, 2024 at 9:31=E2=80=AFPM Nobuaki Tsunashima
<nobuaki.tsunashima@infineon.com> wrote:
>
> From: Nobuaki Tsunashima <Nobuaki.Tsunashima@infineon.com>
>
> 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. Because Bluetooth driver of kernel 5.11 added sending the
> LE_Read_Transmit_Power command in initialize phase, hci up fails due to t=
he
> issue.
>
> Especially in USB i/f case, it would be difficult to download patch FW th=
at
> includes its fix unless hci is up.
>
> The driver already contains infrastructure to apply the quirk for the
> issue, but currently it only supports DMI based matching. Add support to
> match by chip id and baseline FW version to detect CYW4373 ROM FW build
> in generic system.
>
> Fixes: 7c395ea521e6 ("Bluetooth: Query LE tx power on startup")
> Signed-off-by: Nobuaki Tsunashima <Nobuaki.Tsunashima@infineon.com>
> ---
> V3 -> V4: Fix a few coding style warnings and refine comments for clarify=
.
> 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..29e3f83a19fa 100644
> --- a/drivers/bluetooth/btbcm.c
> +++ b/drivers/bluetooth/btbcm.c
> @@ -437,18 +437,48 @@ static const struct dmi_system_id disable_broken_re=
ad_transmit_power[] =3D {
>         { }
>  };
>
> +struct bcm_chip_version_table {
> +       u8 chip_id;                     /* Chip ID */
> +       u16 baseline;           /* Baseline version of patch FW */
> +};
> +#define BCM_ROMFW_BASELINE_NUM 0xFFFF
> +static const struct bcm_chip_version_table disable_broken_read_transmit_=
power_by_chip_ver[] =3D {
> +       { 0x87, BCM_ROMFW_BASELINE_NUM }                /* CYW4373/4373E =
*/
> +};
> +static bool btbcm_is_disable_broken_read_tx_power_by_chip_ver(u8 chip_id=
, u16 baseline)
> +{
> +       int i;
> +       size_t table_size =3D ARRAY_SIZE(disable_broken_read_transmit_pow=
er_by_chip_ver);
> +       const struct bcm_chip_version_table *entry =3D
> +                                               &disable_broken_read_tran=
smit_power_by_chip_ver[0];
> +
> +       for (i =3D 0 ; i < table_size ; i++, entry++)     {
> +               if ((chip_id =3D=3D entry->chip_id) && (baseline =3D=3D e=
ntry->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 =3D btbcm_read_verbose_config(hdev);
>         if (IS_ERR(skb))
>                 return PTR_ERR(skb);
> -
> +       chip_id =3D skb->data[1];
> +       baseline =3D skb->data[3] | (skb->data[4] << 8);

This is not really safe, you shouldn't attempt to access skb->data
without first checking skb->len, actually it would be much better that
you would use skb_pull_data which does skb->len check before pulling
data.

>         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, ba=
seline))
> +               set_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, &hdev->quir=
ks);
> +
>         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[] =3D =
{
>         { USB_VENDOR_AND_INTERFACE_INFO(0x04ca, 0xff, 0x01, 0x01),
>           .driver_info =3D BTUSB_BCM_PATCHRAM },
>
> +       /* Cypress devices with vendor specific id */
> +       { USB_VENDOR_AND_INTERFACE_INFO(0x04b4, 0xff, 0x01, 0x01),
> +         .driver_info =3D BTUSB_BCM_PATCHRAM },
> +
>         /* Broadcom devices with vendor specific id */
>         { USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01),
>           .driver_info =3D BTUSB_BCM_PATCHRAM },
> --
> 2.25.1
>


--=20
Luiz Augusto von Dentz