2023-12-29 15:12:23

by Neal Gompa

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: hci_bcm4377: do not mark valid bd_addr as invalid

On Wed, Dec 27, 2023 at 5:10 AM Johan Hovold <[email protected]> wrote:
>
> A recent commit restored the original (and still documented) semantics
> for the HCI_QUIRK_USE_BDADDR_PROPERTY quirk so that the device address
> is considered invalid unless an address is provided by firmware.
>
> This specifically means that this flag must only be set for devices with
> invalid addresses, but the Broadcom BCM4377 driver has so far been
> setting this flag unconditionally.
>
> Fortunately the driver already checks for invalid addresses during setup
> and sets the HCI_QUIRK_INVALID_BDADDR flag, which can simply be replaced
> with HCI_QUIRK_USE_BDADDR_PROPERTY to indicate that the default address
> is invalid but can be overridden by firmware (long term, this should
> probably just always be allowed).
>
> Fixes: 6945795bc81a ("Bluetooth: fix use-bdaddr-property quirk")
> Cc: [email protected] # 6.5
> Reported-by: Felix Zhang <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]/
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/bluetooth/hci_bcm4377.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_bcm4377.c b/drivers/bluetooth/hci_bcm4377.c
> index a61757835695..9a7243d5db71 100644
> --- a/drivers/bluetooth/hci_bcm4377.c
> +++ b/drivers/bluetooth/hci_bcm4377.c
> @@ -1417,7 +1417,7 @@ static int bcm4377_check_bdaddr(struct bcm4377_data *bcm4377)
>
> bda = (struct hci_rp_read_bd_addr *)skb->data;
> if (!bcm4377_is_valid_bdaddr(bcm4377, &bda->bdaddr))
> - set_bit(HCI_QUIRK_INVALID_BDADDR, &bcm4377->hdev->quirks);
> + set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &bcm4377->hdev->quirks);
>
> kfree_skb(skb);
> return 0;
> @@ -2368,7 +2368,6 @@ static int bcm4377_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> hdev->set_bdaddr = bcm4377_hci_set_bdaddr;
> hdev->setup = bcm4377_hci_setup;
>
> - set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
> if (bcm4377->hw->broken_mws_transport_config)
> set_bit(HCI_QUIRK_BROKEN_MWS_TRANSPORT_CONFIG, &hdev->quirks);
> if (bcm4377->hw->broken_ext_scan)
> --
> 2.41.0
>
>

Looks good to me. This replaces the other patch[1], I take it?

Reviewed-by: Neal Gompa <[email protected]>

[1]: https://lore.kernel.org/asahi/[email protected]/

--
真実はいつも一つ!/ Always, there's only one truth!


2023-12-29 15:51:52

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: hci_bcm4377: do not mark valid bd_addr as invalid

On Fri, Dec 29, 2023 at 10:11:32AM -0500, Neal Gompa wrote:
> On Wed, Dec 27, 2023 at 5:10 AM Johan Hovold <[email protected]> wrote:
> >
> > A recent commit restored the original (and still documented) semantics
> > for the HCI_QUIRK_USE_BDADDR_PROPERTY quirk so that the device address
> > is considered invalid unless an address is provided by firmware.
> >
> > This specifically means that this flag must only be set for devices with
> > invalid addresses, but the Broadcom BCM4377 driver has so far been
> > setting this flag unconditionally.
> >
> > Fortunately the driver already checks for invalid addresses during setup
> > and sets the HCI_QUIRK_INVALID_BDADDR flag, which can simply be replaced
> > with HCI_QUIRK_USE_BDADDR_PROPERTY to indicate that the default address
> > is invalid but can be overridden by firmware (long term, this should
> > probably just always be allowed).
> >
> > Fixes: 6945795bc81a ("Bluetooth: fix use-bdaddr-property quirk")
> > Cc: [email protected] # 6.5
> > Reported-by: Felix Zhang <[email protected]>
> > Link: https://lore.kernel.org/r/[email protected]/
> > Signed-off-by: Johan Hovold <[email protected]>

> Looks good to me. This replaces the other patch[1], I take it?

Yes, but as Sven explained here, that patch is also correct even though
that wasn't obvious from just reading the commit message (which should
be amended in my opinion):

https://lore.kernel.org/asahi/[email protected]/

> Reviewed-by: Neal Gompa <[email protected]>

Thanks for reviewing.

> [1]: https://lore.kernel.org/asahi/[email protected]/

Johan