Bluetooth: hci_bcm4377: do not mark valid bd_addr as invalid

Commit Message

Johan Hovold Dec. 27, 2023, 10:10 a.m. UTC

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: [e-mail]      # 6.5
Reported-by: Felix Zhang <[e-mail]>
Link: https://lore.kernel.org/r/[e-mail]/
Signed-off-by: Johan Hovold <[e-mail]>
---
 drivers/bluetooth/hci_bcm4377.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

[e-mail] Dec. 27, 2023, 10:57 a.m. UTC | #1

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=813030

---Test result---

Test Summary:
CheckPatch                    FAIL      1.51 seconds
GitLint                       FAIL      0.56 seconds
SubjectPrefix                 PASS      2.35 seconds
BuildKernel                   PASS      27.82 seconds
CheckAllWarning               PASS      30.14 seconds
CheckSparse                   PASS      35.81 seconds
CheckSmatch                   PASS      98.18 seconds
BuildKernel32                 PASS      27.09 seconds
TestRunnerSetup               PASS      429.15 seconds
TestRunner_l2cap-tester       PASS      22.65 seconds
TestRunner_iso-tester         PASS      44.40 seconds
TestRunner_bnep-tester        PASS      6.80 seconds
TestRunner_mgmt-tester        PASS      165.38 seconds
TestRunner_rfcomm-tester      PASS      10.63 seconds
TestRunner_sco-tester         PASS      14.27 seconds
TestRunner_ioctl-tester       PASS      11.72 seconds
TestRunner_mesh-tester        PASS      8.70 seconds
TestRunner_smp-tester         PASS      9.71 seconds
TestRunner_userchan-tester    PASS      7.34 seconds
IncrementalBuild              PASS      25.58 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
Bluetooth: hci_bcm4377: do not mark valid bd_addr as invalid
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#73: 
Reported-by: Felix Zhang <[e-mail]>
Link: https://lore.kernel.org/r/[e-mail]/

total: 0 errors, 1 warnings, 15 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13505281.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
Bluetooth: hci_bcm4377: do not mark valid bd_addr as invalid

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
19: B1 Line exceeds max length (93>80): "Link: https://lore.kernel.org/r/[e-mail]/"


---
Regards,
Linux Bluetooth

Felix Zhang Dec. 27, 2023, 6:19 p.m. UTC | #2

On Wed, 2023-12-27 at 11:10 +0100, Johan Hovold 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: [e-mail]      # 6.5
> Reported-by: Felix Zhang <[e-mail]>
> Link:
> https://lore.kernel.org/r/[e-mail]/
> Signed-off-by: Johan Hovold <[e-mail]>
> ---
>  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)
Hi,

Thanks for the patch!  I have just tested the patch on my MacBookAir9,1
with some modifications to make it patch with the master branch of the
Linux kernel, and it works great!  The device is recognized by the
system; and I can scan, pair, connect, and play audio just fine.  If
you want me to send over the modified patch that I have used, please
let me know.

Thanks!

Johan Hovold Dec. 27, 2023, 6:35 p.m. UTC | #3

On Wed, Dec 27, 2023 at 01:19:39PM -0500, Felix Zhang wrote:
> On Wed, 2023-12-27 at 11:10 +0100, Johan Hovold 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: [e-mail]      # 6.5
> > Reported-by: Felix Zhang <[e-mail]>
> > Link:
> > https://lore.kernel.org/r/[e-mail]/
> > Signed-off-by: Johan Hovold <[e-mail]>

> Thanks for the patch!  I have just tested the patch on my MacBookAir9,1
> with some modifications to make it patch with the master branch of the
> Linux kernel, and it works great!  The device is recognized by the
> system; and I can scan, pair, connect, and play audio just fine.  If
> you want me to send over the modified patch that I have used, please
> let me know.

Thanks for confirming. You should not need to modify the patch, though,
as it has been created against 6.7-rc6 and there hasn't been any changes
to this driver since rc1.

Johan

Neal Gompa Dec. 29, 2023, 3:11 p.m. UTC | #4

On Wed, Dec 27, 2023 at 5:10 AM Johan Hovold <[e-mail]> 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: [e-mail]      # 6.5
> Reported-by: Felix Zhang <[e-mail]>
> Link: https://lore.kernel.org/r/[e-mail]/
> Signed-off-by: Johan Hovold <[e-mail]>
> ---
>  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 <[e-mail]>

[1]: https://lore.kernel.org/asahi/[e-mail]/

Johan Hovold Dec. 29, 2023, 3:51 p.m. UTC | #5

On Fri, Dec 29, 2023 at 10:11:32AM -0500, Neal Gompa wrote:
> On Wed, Dec 27, 2023 at 5:10 AM Johan Hovold <[e-mail]> 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: [e-mail]      # 6.5
> > Reported-by: Felix Zhang <[e-mail]>
> > Link: https://lore.kernel.org/r/[e-mail]/
> > Signed-off-by: Johan Hovold <[e-mail]>

> 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/[e-mail]/
 
> Reviewed-by: Neal Gompa <[e-mail]>

Thanks for reviewing.

> [1]: https://lore.kernel.org/asahi/[e-mail]/

Johan

[e-mail] Jan. 2, 2024, 5:40 p.m. UTC | #6

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <[e-mail]>:

On Wed, 27 Dec 2023 11:10:03 +0100 you 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.
> 
> [...]

Here is the summary with links:
  - Bluetooth: hci_bcm4377: do not mark valid bd_addr as invalid
    https://git.kernel.org/bluetooth/bluetooth-next/c/cceb1ba62823

You are awesome, thank you!

Patch

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)