| Message ID | 20211001083412.3078-1-redecorating@protonmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCHv2] Bluetooth: quirk disabling LE Read Transmit Power | expand |
Hi Orlando, > The LE Read Transmit Power command is Advertised on some Broadcom > controlers, but not supported. Using this command breaks Bluetooth > on the MacBookPro16,1 and iMac20,1. Added a quirk disabling LE Read > Transmit Power for these devices, based off their common chip id 150. > > Link: https://lore.kernel.org/r/4970a940-211b-25d6-edab-21a815313954@protonmail.com > Signed-off-by: Orlando Chamberlain <redecorating@protonmail.com> > --- > v1->v2: Clarified quirk description > > drivers/bluetooth/btbcm.c | 4 ++++ > include/net/bluetooth/hci.h | 11 +++++++++++ > net/bluetooth/hci_core.c | 3 ++- > 3 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c > index e4182acee488..4ecc50d93107 100644 > --- a/drivers/bluetooth/btbcm.c > +++ b/drivers/bluetooth/btbcm.c > @@ -353,6 +353,10 @@ static int btbcm_read_info(struct hci_dev *hdev) > return PTR_ERR(skb); > > bt_dev_info(hdev, "BCM: chip id %u", skb->data[1]); > + > + if (skb->data[1] == 150) > + set_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, &hdev->quirks); > + > kfree_skb(skb); I would really prefer to do that via the ACPI table matching in hci_bcm.c and not via some magic chip id check. We actually don’t know how Broadcom assigns their chip ids. Regards Marcel
On Fri, 01 Oct 2021 19:35:16 +1000 "Marcel Holtmann" <marcel@holtmann.org> wrote: > I would really prefer to do that via the ACPI table matching in > hci_bcm.c and not via some magic chip id check. Initially I thought we may be able to do this based off BCM2E7C (which is in the DSDT table which I'll attach), however it seems like many Macs also have that (i.e. MacBookPro14,1, MacBookAir8,1, MacBook9,1), so unless all these don't support LE Read Transmit Power, (which would be hard to determine), I don't know if BCM2E7C can be used to quirk it. I'll try to see if I can find something else in the ACPI tables that can be used as a quirk. (I'll see if I can get the table of a similar model that wasn't affected and compare the BLTH sections) If ACPI tables other than DSDT could be relevant, I can send them too. These are the ones that are present: APIC DMAR DSDT ECDT FACP FACS HPET MCFG SBST SSDT1 SSDT10 SSDT11 SSDT12 SSDT13 SSDT14 SSDT15 SSDT2 SSDT3 SSDT4 SSDT5 SSDT6 SSDT7 SSDT8 SSDT9 VFCT > We actually don’t know how Broadcom assigns their chip ids. That's a good point. I've seen 150, 123 and 133 in some recent Macs, but I also don't know what they refer to or how they are assigned.
On Fri, 01 Oct 2021 23:28:03 +1000 "Orlando Chamberlain" <redecorating@protonmail.com> wrote: > On Fri, 01 Oct 2021 19:35:16 +1000 > "Marcel Holtmann" <marcel@holtmann.org> wrote: > > > I would really prefer to do that via the ACPI table matching in > > hci_bcm.c and not via some magic chip id check. > > Initially I thought we may be able to do this based off BCM2E7C (which > is in the DSDT table which I'll attach), however it seems like many > Macs also have that (i.e. MacBookPro14,1, MacBookAir8,1, MacBook9,1), > so unless all these don't support LE Read Transmit Power, (which > would be hard to determine), I don't know if BCM2E7C can be used to > quirk it. I think there aren't any Macs that support LE Read Transmit Power. I checked the Bluetooth spec here https://www.bluetooth.com/specifications/specs/core-specification-5-1/ and it seems like 5.1 is the first version that mentions LE Read Transmit Power. It says 5.1 was adopted on 21 Jan 2019. As far as I know, all of the models released after that date that have ever had working Bluetooth were affected, while unaffected models were released before that date (and thus shouldn't support LE Read Transmit Power? This is at least true for the MacBookPro15,1, a 2018 model that doesn't support the command). I think this means that no Apple computer released so far supports the command, so disabling LE Read Transmit Power for all Apple controllers based off "apple-uart-blth" (probably a better choice than "BCM2E7C") won't affect any controllers that actually support it. Device (BLTH) { Name (_HID, EisaId ("BCM2E7C")) // _HID: Hardware ID Name (_CID, "apple-uart-blth") // _CID: Compatible ID Name (_UID, One) // _UID: Unique ID Name (_ADR, Zero) // _ADR: Address As to future Apple computers, they seem to no longer be using UART and instead have a second Broadcom PCI device (the first being for WiFi) that is for Bluetooth. 3 Intel Macs Models have this second device (MacBookPro15,4, MacBookPro16,3 and MacBookAir9,1), and so do the M1 ones. I can't say that they won't move back to UART at some point and then support LE Read Transmit Power, but if they do, I don't think they would move back to x86_64, so only having this quirk activated when compiling for x86_64 might be an option if that's an issue. > I'll try to see if I can find something else in the ACPI tables that > can be used as a quirk. (I'll see if I can get the table of a similar > model that wasn't affected and compare the BLTH sections) The BLTH sections were the same for affected and unaffected macs. Would disabling LE Read Transmit Power if the controller is "apple-uart-blth" work for you? --
Lo, this is your Linux kernel regression tracker speaking. I have this regression on the radar of regzbot, my Linux regression tracking bot: https://linux-regtracking.leemhuis.info/regzbot/regression/4970a940-211b-25d6-edab-21a815313954@protonmail.com/ Has any progress been made since below mail? If not: how can we get the ball rolling again and get this regression fixed? Ciao, Thorsten P.S.: I have no personal interest in this issue. Hence, feel free to exclude me on further messages in this thread after the first reply, as I'm only posting this mail to hopefully get a status update and things rolling again. #regzbot poke On 04.10.21 13:15, Orlando Chamberlain wrote: > On Fri, 01 Oct 2021 23:28:03 +1000 > "Orlando Chamberlain" <redecorating@protonmail.com> wrote: >> On Fri, 01 Oct 2021 19:35:16 +1000 >> "Marcel Holtmann" <marcel@holtmann.org> wrote: >> >>> I would really prefer to do that via the ACPI table matching in >>> hci_bcm.c and not via some magic chip id check. >> >> Initially I thought we may be able to do this based off BCM2E7C (which >> is in the DSDT table which I'll attach), however it seems like many >> Macs also have that (i.e. MacBookPro14,1, MacBookAir8,1, MacBook9,1), >> so unless all these don't support LE Read Transmit Power, (which >> would be hard to determine), I don't know if BCM2E7C can be used to >> quirk it. > > I think there aren't any Macs that support LE Read Transmit Power. > > I checked the Bluetooth spec here > https://www.bluetooth.com/specifications/specs/core-specification-5-1/ > and it seems like 5.1 is the first version that mentions LE Read > Transmit Power. It says 5.1 was adopted on 21 Jan 2019. > > As far as I know, all of the models released after that date that have > ever had working Bluetooth were affected, while unaffected models were > released before that date (and thus shouldn't support LE Read Transmit > Power? This is at least true for the MacBookPro15,1, a 2018 model that > doesn't support the command). > > I think this means that no Apple computer released so far supports the > command, so disabling LE Read Transmit Power for all Apple controllers > based off "apple-uart-blth" (probably a better choice than "BCM2E7C") > won't affect any controllers that actually support it. > > Device (BLTH) > { > Name (_HID, EisaId ("BCM2E7C")) // _HID: Hardware ID > Name (_CID, "apple-uart-blth") // _CID: Compatible ID > Name (_UID, One) // _UID: Unique ID > Name (_ADR, Zero) // _ADR: Address > > As to future Apple computers, they seem to no longer be using UART and > instead have a second Broadcom PCI device (the first being for WiFi) > that is for Bluetooth. 3 Intel Macs Models have this second device > (MacBookPro15,4, MacBookPro16,3 and MacBookAir9,1), and so do the M1 > ones. > > I can't say that they won't move back to UART at some point and then > support LE Read Transmit Power, but if they do, I don't think they > would move back to x86_64, so only having this quirk activated when > compiling for x86_64 might be an option if that's an issue. > >> I'll try to see if I can find something else in the ACPI tables that >> can be used as a quirk. (I'll see if I can get the table of a similar >> model that wasn't affected and compare the BLTH sections) > > The BLTH sections were the same for affected and unaffected macs. > > > > Would disabling LE Read Transmit Power if the controller is > "apple-uart-blth" work for you? >
Hi Orlando, On Fri, Oct 1, 2021 at 1:56 AM Orlando Chamberlain <redecorating@protonmail.com> wrote: > > The LE Read Transmit Power command is Advertised on some Broadcom > controlers, but not supported. Using this command breaks Bluetooth > on the MacBookPro16,1 and iMac20,1. Added a quirk disabling LE Read > Transmit Power for these devices, based off their common chip id 150. > > Link: https://lore.kernel.org/r/4970a940-211b-25d6-edab-21a815313954@protonmail.com > Signed-off-by: Orlando Chamberlain <redecorating@protonmail.com> > --- > v1->v2: Clarified quirk description > > drivers/bluetooth/btbcm.c | 4 ++++ > include/net/bluetooth/hci.h | 11 +++++++++++ > net/bluetooth/hci_core.c | 3 ++- > 3 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c > index e4182acee488..4ecc50d93107 100644 > --- a/drivers/bluetooth/btbcm.c > +++ b/drivers/bluetooth/btbcm.c > @@ -353,6 +353,10 @@ static int btbcm_read_info(struct hci_dev *hdev) > return PTR_ERR(skb); > > bt_dev_info(hdev, "BCM: chip id %u", skb->data[1]); > + > + if (skb->data[1] == 150) > + set_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, &hdev->quirks); > + > kfree_skb(skb); > > /* Read Controller Features */ > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > index b80415011dcd..6da9bd6b7259 100644 > --- a/include/net/bluetooth/hci.h > +++ b/include/net/bluetooth/hci.h > @@ -246,6 +246,17 @@ enum { > * HCI after resume. > */ > HCI_QUIRK_NO_SUSPEND_NOTIFIER, > + > + /* When this quirk is set, LE Read Transmit Power is disabled. > + * This is mainly due to the fact that the HCI LE Read Transmit > + * Power command is advertised, but not supported; these > + * controllers often reply with unknown command and need a hard > + * reset. > + * > + * This quirk can be set before hci_register_dev is called or > + * during the hdev->setup vendor callback. > + */ > + HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, > }; > > /* HCI device flags */ > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > index 8a47a3017d61..9a23fe7c8d67 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -742,7 +742,8 @@ static int hci_init3_req(struct hci_request *req, unsigned long opt) > hci_req_add(req, HCI_OP_LE_READ_ADV_TX_POWER, 0, NULL); > } > > - if (hdev->commands[38] & 0x80) { > + if (hdev->commands[38] & 0x80 && > + !test_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, &hdev->quirks)) { > /* Read LE Min/Max Tx Power*/ > hci_req_add(req, HCI_OP_LE_READ_TRANSMIT_POWER, > 0, NULL); > -- > 2.33.0 Nowadays it is possible to treat errors such like this on a per command basis (assuming it is not essential for the init sequence): diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h index 979da5179ff4..f244f42cc609 100644 --- a/include/net/bluetooth/hci.h +++ b/include/net/bluetooth/hci.h @@ -551,6 +551,7 @@ enum { #define HCI_LK_AUTH_COMBINATION_P256 0x08 /* ---- HCI Error Codes ---- */ +#define HCI_ERROR_UNKNOWN_CMD 0x01 #define HCI_ERROR_UNKNOWN_CONN_ID 0x02 #define HCI_ERROR_AUTH_FAILURE 0x05 #define HCI_ERROR_PIN_OR_KEY_MISSING 0x06 diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c index bb88d31d2212..9c697e058974 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -3325,11 +3325,18 @@ static int hci_le_read_adv_tx_power_sync(struct hci_dev *hdev) /* Read LE Min/Max Tx Power*/ static int hci_le_read_tx_power_sync(struct hci_dev *hdev) { + int status; + if (!(hdev->commands[38] & 0x80)) return 0; - return __hci_cmd_sync_status(hdev, HCI_OP_LE_READ_TRANSMIT_POWER, - 0, NULL, HCI_CMD_TIMEOUT); + status = __hci_cmd_sync_status(hdev, HCI_OP_LE_READ_TRANSMIT_POWER, + 0, NULL, HCI_CMD_TIMEOUT); + /* Ignore if command is not really supported */ + if (status == HCI_ERROR_UNKNOWN_CMD) + return 0; + + return status; } /* Read LE Accept List Size */ Anyway, it would probably be worth pointing out to the vendor they have a broken firmware if they do mark the command as supported but return such error.
> On 06-Nov-2021, at 3:17 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Orlando, > > On Fri, Oct 1, 2021 at 1:56 AM Orlando Chamberlain > <redecorating@protonmail.com> wrote: >> >> The LE Read Transmit Power command is Advertised on some Broadcom >> controlers, but not supported. Using this command breaks Bluetooth >> on the MacBookPro16,1 and iMac20,1. Added a quirk disabling LE Read >> Transmit Power for these devices, based off their common chip id 150. >> >> Link: https://lore.kernel.org/r/4970a940-211b-25d6-edab-21a815313954@protonmail.com >> Signed-off-by: Orlando Chamberlain <redecorating@protonmail.com> >> --- >> v1->v2: Clarified quirk description >> >> drivers/bluetooth/btbcm.c | 4 ++++ >> include/net/bluetooth/hci.h | 11 +++++++++++ >> net/bluetooth/hci_core.c | 3 ++- >> 3 files changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c >> index e4182acee488..4ecc50d93107 100644 >> --- a/drivers/bluetooth/btbcm.c >> +++ b/drivers/bluetooth/btbcm.c >> @@ -353,6 +353,10 @@ static int btbcm_read_info(struct hci_dev *hdev) >> return PTR_ERR(skb); >> >> bt_dev_info(hdev, "BCM: chip id %u", skb->data[1]); >> + >> + if (skb->data[1] == 150) >> + set_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, &hdev->quirks); >> + >> kfree_skb(skb); >> >> /* Read Controller Features */ >> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h >> index b80415011dcd..6da9bd6b7259 100644 >> --- a/include/net/bluetooth/hci.h >> +++ b/include/net/bluetooth/hci.h >> @@ -246,6 +246,17 @@ enum { >> * HCI after resume. >> */ >> HCI_QUIRK_NO_SUSPEND_NOTIFIER, >> + >> + /* When this quirk is set, LE Read Transmit Power is disabled. >> + * This is mainly due to the fact that the HCI LE Read Transmit >> + * Power command is advertised, but not supported; these >> + * controllers often reply with unknown command and need a hard >> + * reset. >> + * >> + * This quirk can be set before hci_register_dev is called or >> + * during the hdev->setup vendor callback. >> + */ >> + HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, >> }; >> >> /* HCI device flags */ >> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c >> index 8a47a3017d61..9a23fe7c8d67 100644 >> --- a/net/bluetooth/hci_core.c >> +++ b/net/bluetooth/hci_core.c >> @@ -742,7 +742,8 @@ static int hci_init3_req(struct hci_request *req, unsigned long opt) >> hci_req_add(req, HCI_OP_LE_READ_ADV_TX_POWER, 0, NULL); >> } >> >> - if (hdev->commands[38] & 0x80) { >> + if (hdev->commands[38] & 0x80 && >> + !test_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, &hdev->quirks)) { >> /* Read LE Min/Max Tx Power*/ >> hci_req_add(req, HCI_OP_LE_READ_TRANSMIT_POWER, >> 0, NULL); >> -- >> 2.33.0 > > Nowadays it is possible to treat errors such like this on a per > command basis (assuming it is not essential for the init sequence): > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > index 979da5179ff4..f244f42cc609 100644 > --- a/include/net/bluetooth/hci.h > +++ b/include/net/bluetooth/hci.h > @@ -551,6 +551,7 @@ enum { > #define HCI_LK_AUTH_COMBINATION_P256 0x08 > > /* ---- HCI Error Codes ---- */ > +#define HCI_ERROR_UNKNOWN_CMD 0x01 > #define HCI_ERROR_UNKNOWN_CONN_ID 0x02 > #define HCI_ERROR_AUTH_FAILURE 0x05 > #define HCI_ERROR_PIN_OR_KEY_MISSING 0x06 > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c This diff cannot be applied to stable 5.15. Could you provide a patch capable of being applied to stable. > index bb88d31d2212..9c697e058974 100644 > --- a/net/bluetooth/hci_sync.c > +++ b/net/bluetooth/hci_sync.c > @@ -3325,11 +3325,18 @@ static int > hci_le_read_adv_tx_power_sync(struct hci_dev *hdev) > /* Read LE Min/Max Tx Power*/ > static int hci_le_read_tx_power_sync(struct hci_dev *hdev) > { > + int status; > + > if (!(hdev->commands[38] & 0x80)) > return 0; > > - return __hci_cmd_sync_status(hdev, HCI_OP_LE_READ_TRANSMIT_POWER, > - 0, NULL, HCI_CMD_TIMEOUT); > + status = __hci_cmd_sync_status(hdev, HCI_OP_LE_READ_TRANSMIT_POWER, > + 0, NULL, HCI_CMD_TIMEOUT); > + /* Ignore if command is not really supported */ > + if (status == HCI_ERROR_UNKNOWN_CMD) > + return 0; > + > + return status; > } > > /* Read LE Accept List Size */ > > Anyway, it would probably be worth pointing out to the vendor they > have a broken firmware if they do mark the command as supported but > return such error. > > -- > Luiz Augusto von Dentz >
On Sat, Nov 06, 2021 at 09:41:28AM +0000, Aditya Garg wrote: > > > > On 06-Nov-2021, at 3:17 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > > > Hi Orlando, > > > > On Fri, Oct 1, 2021 at 1:56 AM Orlando Chamberlain > > <redecorating@protonmail.com> wrote: > >> > >> The LE Read Transmit Power command is Advertised on some Broadcom > >> controlers, but not supported. Using this command breaks Bluetooth > >> on the MacBookPro16,1 and iMac20,1. Added a quirk disabling LE Read > >> Transmit Power for these devices, based off their common chip id 150. > >> > >> Link: https://lore.kernel.org/r/4970a940-211b-25d6-edab-21a815313954@protonmail.com > >> Signed-off-by: Orlando Chamberlain <redecorating@protonmail.com> > >> --- > >> v1->v2: Clarified quirk description > >> > >> drivers/bluetooth/btbcm.c | 4 ++++ > >> include/net/bluetooth/hci.h | 11 +++++++++++ > >> net/bluetooth/hci_core.c | 3 ++- > >> 3 files changed, 17 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c > >> index e4182acee488..4ecc50d93107 100644 > >> --- a/drivers/bluetooth/btbcm.c > >> +++ b/drivers/bluetooth/btbcm.c > >> @@ -353,6 +353,10 @@ static int btbcm_read_info(struct hci_dev *hdev) > >> return PTR_ERR(skb); > >> > >> bt_dev_info(hdev, "BCM: chip id %u", skb->data[1]); > >> + > >> + if (skb->data[1] == 150) > >> + set_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, &hdev->quirks); > >> + > >> kfree_skb(skb); > >> > >> /* Read Controller Features */ > >> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > >> index b80415011dcd..6da9bd6b7259 100644 > >> --- a/include/net/bluetooth/hci.h > >> +++ b/include/net/bluetooth/hci.h > >> @@ -246,6 +246,17 @@ enum { > >> * HCI after resume. > >> */ > >> HCI_QUIRK_NO_SUSPEND_NOTIFIER, > >> + > >> + /* When this quirk is set, LE Read Transmit Power is disabled. > >> + * This is mainly due to the fact that the HCI LE Read Transmit > >> + * Power command is advertised, but not supported; these > >> + * controllers often reply with unknown command and need a hard > >> + * reset. > >> + * > >> + * This quirk can be set before hci_register_dev is called or > >> + * during the hdev->setup vendor callback. > >> + */ > >> + HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, > >> }; > >> > >> /* HCI device flags */ > >> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > >> index 8a47a3017d61..9a23fe7c8d67 100644 > >> --- a/net/bluetooth/hci_core.c > >> +++ b/net/bluetooth/hci_core.c > >> @@ -742,7 +742,8 @@ static int hci_init3_req(struct hci_request *req, unsigned long opt) > >> hci_req_add(req, HCI_OP_LE_READ_ADV_TX_POWER, 0, NULL); > >> } > >> > >> - if (hdev->commands[38] & 0x80) { > >> + if (hdev->commands[38] & 0x80 && > >> + !test_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, &hdev->quirks)) { > >> /* Read LE Min/Max Tx Power*/ > >> hci_req_add(req, HCI_OP_LE_READ_TRANSMIT_POWER, > >> 0, NULL); > >> -- > >> 2.33.0 > > > > Nowadays it is possible to treat errors such like this on a per > > command basis (assuming it is not essential for the init sequence): > > > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > > index 979da5179ff4..f244f42cc609 100644 > > --- a/include/net/bluetooth/hci.h > > +++ b/include/net/bluetooth/hci.h > > @@ -551,6 +551,7 @@ enum { > > #define HCI_LK_AUTH_COMBINATION_P256 0x08 > > > > /* ---- HCI Error Codes ---- */ > > +#define HCI_ERROR_UNKNOWN_CMD 0x01 > > #define HCI_ERROR_UNKNOWN_CONN_ID 0x02 > > #define HCI_ERROR_AUTH_FAILURE 0x05 > > #define HCI_ERROR_PIN_OR_KEY_MISSING 0x06 > > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > This diff cannot be applied to stable 5.15. Could you provide a patch capable of being applied to stable. That is not needed until a patch is in Linus's tree. Why do you need it earlier? thanks, greg k-h
> On 06-Nov-2021, at 5:19 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > > On Sat, Nov 06, 2021 at 09:41:28AM +0000, Aditya Garg wrote: >> >> >>> On 06-Nov-2021, at 3:17 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: >>> >>> Hi Orlando, >>> >>> On Fri, Oct 1, 2021 at 1:56 AM Orlando Chamberlain >>> <redecorating@protonmail.com> wrote: >>>> >>>> The LE Read Transmit Power command is Advertised on some Broadcom >>>> controlers, but not supported. Using this command breaks Bluetooth >>>> on the MacBookPro16,1 and iMac20,1. Added a quirk disabling LE Read >>>> Transmit Power for these devices, based off their common chip id 150. >>>> >>>> Link: https://lore.kernel.org/r/4970a940-211b-25d6-edab-21a815313954@protonmail.com >>>> Signed-off-by: Orlando Chamberlain <redecorating@protonmail.com> >>>> --- >>>> v1->v2: Clarified quirk description >>>> >>>> drivers/bluetooth/btbcm.c | 4 ++++ >>>> include/net/bluetooth/hci.h | 11 +++++++++++ >>>> net/bluetooth/hci_core.c | 3 ++- >>>> 3 files changed, 17 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c >>>> index e4182acee488..4ecc50d93107 100644 >>>> --- a/drivers/bluetooth/btbcm.c >>>> +++ b/drivers/bluetooth/btbcm.c >>>> @@ -353,6 +353,10 @@ static int btbcm_read_info(struct hci_dev *hdev) >>>> return PTR_ERR(skb); >>>> >>>> bt_dev_info(hdev, "BCM: chip id %u", skb->data[1]); >>>> + >>>> + if (skb->data[1] == 150) >>>> + set_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, &hdev->quirks); >>>> + >>>> kfree_skb(skb); >>>> >>>> /* Read Controller Features */ >>>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h >>>> index b80415011dcd..6da9bd6b7259 100644 >>>> --- a/include/net/bluetooth/hci.h >>>> +++ b/include/net/bluetooth/hci.h >>>> @@ -246,6 +246,17 @@ enum { >>>> * HCI after resume. >>>> */ >>>> HCI_QUIRK_NO_SUSPEND_NOTIFIER, >>>> + >>>> + /* When this quirk is set, LE Read Transmit Power is disabled. >>>> + * This is mainly due to the fact that the HCI LE Read Transmit >>>> + * Power command is advertised, but not supported; these >>>> + * controllers often reply with unknown command and need a hard >>>> + * reset. >>>> + * >>>> + * This quirk can be set before hci_register_dev is called or >>>> + * during the hdev->setup vendor callback. >>>> + */ >>>> + HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, >>>> }; >>>> >>>> /* HCI device flags */ >>>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c >>>> index 8a47a3017d61..9a23fe7c8d67 100644 >>>> --- a/net/bluetooth/hci_core.c >>>> +++ b/net/bluetooth/hci_core.c >>>> @@ -742,7 +742,8 @@ static int hci_init3_req(struct hci_request *req, unsigned long opt) >>>> hci_req_add(req, HCI_OP_LE_READ_ADV_TX_POWER, 0, NULL); >>>> } >>>> >>>> - if (hdev->commands[38] & 0x80) { >>>> + if (hdev->commands[38] & 0x80 && >>>> + !test_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, &hdev->quirks)) { >>>> /* Read LE Min/Max Tx Power*/ >>>> hci_req_add(req, HCI_OP_LE_READ_TRANSMIT_POWER, >>>> 0, NULL); >>>> -- >>>> 2.33.0 >>> >>> Nowadays it is possible to treat errors such like this on a per >>> command basis (assuming it is not essential for the init sequence): >>> >>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h >>> index 979da5179ff4..f244f42cc609 100644 >>> --- a/include/net/bluetooth/hci.h >>> +++ b/include/net/bluetooth/hci.h >>> @@ -551,6 +551,7 @@ enum { >>> #define HCI_LK_AUTH_COMBINATION_P256 0x08 >>> >>> /* ---- HCI Error Codes ---- */ >>> +#define HCI_ERROR_UNKNOWN_CMD 0x01 >>> #define HCI_ERROR_UNKNOWN_CONN_ID 0x02 >>> #define HCI_ERROR_AUTH_FAILURE 0x05 >>> #define HCI_ERROR_PIN_OR_KEY_MISSING 0x06 >>> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c >> This diff cannot be applied to stable 5.15. Could you provide a patch capable of being applied to stable. > > That is not needed until a patch is in Linus's tree. Why do you need it > earlier? > Well not an emergency, but the issue of Bluetooth not working on some Apple Macs has been there for a long time. BTW, will this patch be there in Linus’s tree in the coming days? > thanks, > > greg k-h
On Sat, 06 Nov 2021 08:47:39 +1100 "Luiz Augusto von Dentz" <luiz.dentz@gmail.com> wrote: > Nowadays it is possible to treat errors such like this on a per > command basis (assuming it is not essential for the init sequence): > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > index 979da5179ff4..f244f42cc609 100644 > --- a/include/net/bluetooth/hci.h > +++ b/include/net/bluetooth/hci.h > @@ -551,6 +551,7 @@ enum { > #define HCI_LK_AUTH_COMBINATION_P256 0x08 > > /* ---- HCI Error Codes ---- */ > +#define HCI_ERROR_UNKNOWN_CMD 0x01 > #define HCI_ERROR_UNKNOWN_CONN_ID 0x02 > #define HCI_ERROR_AUTH_FAILURE 0x05 > #define HCI_ERROR_PIN_OR_KEY_MISSING 0x06 > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > index bb88d31d2212..9c697e058974 100644 > --- a/net/bluetooth/hci_sync.c > +++ b/net/bluetooth/hci_sync.c > @@ -3325,11 +3325,18 @@ static int > hci_le_read_adv_tx_power_sync(struct hci_dev *hdev) > /* Read LE Min/Max Tx Power*/ > static int hci_le_read_tx_power_sync(struct hci_dev *hdev) > { > + int status; > + > if (!(hdev->commands[38] & 0x80)) > return 0; > > - return __hci_cmd_sync_status(hdev, > HCI_OP_LE_READ_TRANSMIT_POWER, > - 0, NULL, HCI_CMD_TIMEOUT); > + status = __hci_cmd_sync_status(hdev, > HCI_OP_LE_READ_TRANSMIT_POWER, > + 0, NULL, HCI_CMD_TIMEOUT); > + /* Ignore if command is not really supported */ > + if (status == HCI_ERROR_UNKNOWN_CMD) > + return 0; > + > + return status; > } > > /* Read LE Accept List Size */ I've tried this patch, and status seems to be -56, not 0x01, but if I change + if (status == HCI_ERROR_UNKNOWN_CMD) to + if (status == -56) It ignores the error and continues. I seem to have an unrelated problem where although I can connect to my Logitech MX Anywhere 2S mouse (I haven't tried any other devices yet), it doesn't move the cursor or register clicks. I've also noticed that bluetoothctl pair isn't asking for a "yes" when I pair a device, which it was doing on 5.15 (with the patch I sent to get bluetooth working at all). I've put dmesg and btsnoop for both 5.15 and bluetooth-next into a gist here: https://gist.github.com/Redecorating/5620b758d8191418cf19879d09672cf4 but I think this is a separate issue. > > Anyway, it would probably be worth pointing out to the vendor they > have a broken firmware if they do mark the command as supported but > return such error. Do you know if it'd be better to contact Broadcom or Apple for this? > -- > Luiz Augusto von Dentz --
On Sat, Nov 06, 2021 at 05:27:27PM +0000, Aditya Garg wrote: > > > > On 06-Nov-2021, at 5:19 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Sat, Nov 06, 2021 at 09:41:28AM +0000, Aditya Garg wrote: > >> > >> > >>> On 06-Nov-2021, at 3:17 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > >>> > >>> Hi Orlando, > >>> > >>> On Fri, Oct 1, 2021 at 1:56 AM Orlando Chamberlain > >>> <redecorating@protonmail.com> wrote: > >>>> > >>>> The LE Read Transmit Power command is Advertised on some Broadcom > >>>> controlers, but not supported. Using this command breaks Bluetooth > >>>> on the MacBookPro16,1 and iMac20,1. Added a quirk disabling LE Read > >>>> Transmit Power for these devices, based off their common chip id 150. > >>>> > >>>> Link: https://lore.kernel.org/r/4970a940-211b-25d6-edab-21a815313954@protonmail.com > >>>> Signed-off-by: Orlando Chamberlain <redecorating@protonmail.com> > >>>> --- > >>>> v1->v2: Clarified quirk description > >>>> > >>>> drivers/bluetooth/btbcm.c | 4 ++++ > >>>> include/net/bluetooth/hci.h | 11 +++++++++++ > >>>> net/bluetooth/hci_core.c | 3 ++- > >>>> 3 files changed, 17 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c > >>>> index e4182acee488..4ecc50d93107 100644 > >>>> --- a/drivers/bluetooth/btbcm.c > >>>> +++ b/drivers/bluetooth/btbcm.c > >>>> @@ -353,6 +353,10 @@ static int btbcm_read_info(struct hci_dev *hdev) > >>>> return PTR_ERR(skb); > >>>> > >>>> bt_dev_info(hdev, "BCM: chip id %u", skb->data[1]); > >>>> + > >>>> + if (skb->data[1] == 150) > >>>> + set_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, &hdev->quirks); > >>>> + > >>>> kfree_skb(skb); > >>>> > >>>> /* Read Controller Features */ > >>>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > >>>> index b80415011dcd..6da9bd6b7259 100644 > >>>> --- a/include/net/bluetooth/hci.h > >>>> +++ b/include/net/bluetooth/hci.h > >>>> @@ -246,6 +246,17 @@ enum { > >>>> * HCI after resume. > >>>> */ > >>>> HCI_QUIRK_NO_SUSPEND_NOTIFIER, > >>>> + > >>>> + /* When this quirk is set, LE Read Transmit Power is disabled. > >>>> + * This is mainly due to the fact that the HCI LE Read Transmit > >>>> + * Power command is advertised, but not supported; these > >>>> + * controllers often reply with unknown command and need a hard > >>>> + * reset. > >>>> + * > >>>> + * This quirk can be set before hci_register_dev is called or > >>>> + * during the hdev->setup vendor callback. > >>>> + */ > >>>> + HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, > >>>> }; > >>>> > >>>> /* HCI device flags */ > >>>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > >>>> index 8a47a3017d61..9a23fe7c8d67 100644 > >>>> --- a/net/bluetooth/hci_core.c > >>>> +++ b/net/bluetooth/hci_core.c > >>>> @@ -742,7 +742,8 @@ static int hci_init3_req(struct hci_request *req, unsigned long opt) > >>>> hci_req_add(req, HCI_OP_LE_READ_ADV_TX_POWER, 0, NULL); > >>>> } > >>>> > >>>> - if (hdev->commands[38] & 0x80) { > >>>> + if (hdev->commands[38] & 0x80 && > >>>> + !test_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, &hdev->quirks)) { > >>>> /* Read LE Min/Max Tx Power*/ > >>>> hci_req_add(req, HCI_OP_LE_READ_TRANSMIT_POWER, > >>>> 0, NULL); > >>>> -- > >>>> 2.33.0 > >>> > >>> Nowadays it is possible to treat errors such like this on a per > >>> command basis (assuming it is not essential for the init sequence): > >>> > >>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > >>> index 979da5179ff4..f244f42cc609 100644 > >>> --- a/include/net/bluetooth/hci.h > >>> +++ b/include/net/bluetooth/hci.h > >>> @@ -551,6 +551,7 @@ enum { > >>> #define HCI_LK_AUTH_COMBINATION_P256 0x08 > >>> > >>> /* ---- HCI Error Codes ---- */ > >>> +#define HCI_ERROR_UNKNOWN_CMD 0x01 > >>> #define HCI_ERROR_UNKNOWN_CONN_ID 0x02 > >>> #define HCI_ERROR_AUTH_FAILURE 0x05 > >>> #define HCI_ERROR_PIN_OR_KEY_MISSING 0x06 > >>> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > >> This diff cannot be applied to stable 5.15. Could you provide a patch capable of being applied to stable. > > > > That is not needed until a patch is in Linus's tree. Why do you need it > > earlier? > > > Well not an emergency, but the issue of Bluetooth not working on some Apple Macs has been there for a long time. BTW, will this patch be there in Linus’s tree in the coming days? That is up to the bluetooth maintainers, they have to accept it first. thanks, greg k-h
Hi, this is your Linux kernel regression tracker speaking. To make this easy and quick to read for everyone I for once rely on top-posting: Bluetooth maintainers, what's the status here? The proposed patch is fixing a regression. It's not a recent one (it afaics was introduced in v5.11-rc1). Nevertheless it would be good to get this finally resolved. But this thread seems inactive for more than a week now. Or was progress made, but is only visible somewhere else? Ciao, Thorsten (carrying his Linux kernel regression tracker hat) P.S.: As a Linux kernel regression tracker I'm getting a lot of reports on my table. I can only look briefly into most of them. Therefore I unfortunately will get things wrong or miss something important. I hope that's not the case here; if you think it is, don't hesitate to tell me about it in a public reply. That's in everyone's interest, as what I wrote above might be misleading to everyone reading this, which is something I'd like to avoid. BTW, I have no personal interest in this issue, which is tracked using regzbot, my Linux kernel regression tracking bot (https://linux-regtracking.leemhuis.info/regzbot/). I'm only posting this mail to hopefully get things rolling again and hence don't need to be CC on all further activities wrt to this regression. Also feel free to ignore the following lines, they are meant for regzbot: #regzbot poke On 07.11.21 09:35, Greg KH wrote: > On Sat, Nov 06, 2021 at 05:27:27PM +0000, Aditya Garg wrote: >>> On 06-Nov-2021, at 5:19 PM, Greg KH <gregkh@linuxfoundation.org> wrote: >>> On Sat, Nov 06, 2021 at 09:41:28AM +0000, Aditya Garg wrote: >>>>> On 06-Nov-2021, at 3:17 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: >>>>> On Fri, Oct 1, 2021 at 1:56 AM Orlando Chamberlain >>>>> <redecorating@protonmail.com> wrote: >>>>>> >>>>>> The LE Read Transmit Power command is Advertised on some Broadcom >>>>>> controlers, but not supported. Using this command breaks Bluetooth >>>>>> on the MacBookPro16,1 and iMac20,1. Added a quirk disabling LE Read >>>>>> Transmit Power for these devices, based off their common chip id 150. >>>>>> >>>>>> Link: https://lore.kernel.org/r/4970a940-211b-25d6-edab-21a815313954@protonmail.com >>>>>> Signed-off-by: Orlando Chamberlain <redecorating@protonmail.com> >>>>>> --- >>>>>> v1->v2: Clarified quirk description >>>>>> >>>>>> drivers/bluetooth/btbcm.c | 4 ++++ >>>>>> include/net/bluetooth/hci.h | 11 +++++++++++ >>>>>> net/bluetooth/hci_core.c | 3 ++- >>>>>> 3 files changed, 17 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c >>>>>> index e4182acee488..4ecc50d93107 100644 >>>>>> --- a/drivers/bluetooth/btbcm.c >>>>>> +++ b/drivers/bluetooth/btbcm.c >>>>>> @@ -353,6 +353,10 @@ static int btbcm_read_info(struct hci_dev *hdev) >>>>>> return PTR_ERR(skb); >>>>>> >>>>>> bt_dev_info(hdev, "BCM: chip id %u", skb->data[1]); >>>>>> + >>>>>> + if (skb->data[1] == 150) >>>>>> + set_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, &hdev->quirks); >>>>>> + >>>>>> kfree_skb(skb); >>>>>> >>>>>> /* Read Controller Features */ >>>>>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h >>>>>> index b80415011dcd..6da9bd6b7259 100644 >>>>>> --- a/include/net/bluetooth/hci.h >>>>>> +++ b/include/net/bluetooth/hci.h >>>>>> @@ -246,6 +246,17 @@ enum { >>>>>> * HCI after resume. >>>>>> */ >>>>>> HCI_QUIRK_NO_SUSPEND_NOTIFIER, >>>>>> + >>>>>> + /* When this quirk is set, LE Read Transmit Power is disabled. >>>>>> + * This is mainly due to the fact that the HCI LE Read Transmit >>>>>> + * Power command is advertised, but not supported; these >>>>>> + * controllers often reply with unknown command and need a hard >>>>>> + * reset. >>>>>> + * >>>>>> + * This quirk can be set before hci_register_dev is called or >>>>>> + * during the hdev->setup vendor callback. >>>>>> + */ >>>>>> + HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, >>>>>> }; >>>>>> >>>>>> /* HCI device flags */ >>>>>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c >>>>>> index 8a47a3017d61..9a23fe7c8d67 100644 >>>>>> --- a/net/bluetooth/hci_core.c >>>>>> +++ b/net/bluetooth/hci_core.c >>>>>> @@ -742,7 +742,8 @@ static int hci_init3_req(struct hci_request *req, unsigned long opt) >>>>>> hci_req_add(req, HCI_OP_LE_READ_ADV_TX_POWER, 0, NULL); >>>>>> } >>>>>> >>>>>> - if (hdev->commands[38] & 0x80) { >>>>>> + if (hdev->commands[38] & 0x80 && >>>>>> + !test_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, &hdev->quirks)) { >>>>>> /* Read LE Min/Max Tx Power*/ >>>>>> hci_req_add(req, HCI_OP_LE_READ_TRANSMIT_POWER, >>>>>> 0, NULL); >>>>>> -- >>>>>> 2.33.0 >>>>> >>>>> Nowadays it is possible to treat errors such like this on a per >>>>> command basis (assuming it is not essential for the init sequence): >>>>> >>>>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h >>>>> index 979da5179ff4..f244f42cc609 100644 >>>>> --- a/include/net/bluetooth/hci.h >>>>> +++ b/include/net/bluetooth/hci.h >>>>> @@ -551,6 +551,7 @@ enum { >>>>> #define HCI_LK_AUTH_COMBINATION_P256 0x08 >>>>> >>>>> /* ---- HCI Error Codes ---- */ >>>>> +#define HCI_ERROR_UNKNOWN_CMD 0x01 >>>>> #define HCI_ERROR_UNKNOWN_CONN_ID 0x02 >>>>> #define HCI_ERROR_AUTH_FAILURE 0x05 >>>>> #define HCI_ERROR_PIN_OR_KEY_MISSING 0x06 >>>>> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c >>>> This diff cannot be applied to stable 5.15. Could you provide a patch capable of being applied to stable. >>> >>> That is not needed until a patch is in Linus's tree. Why do you need it >>> earlier? >>> >> Well not an emergency, but the issue of Bluetooth not working on some Apple Macs has been there for a long time. BTW, will this patch be there in Linus’s tree in the coming days? > > That is up to the bluetooth maintainers, they have to accept it first. > > thanks, > > greg k-h > >
> Bluetooth maintainers, what's the status here? The proposed patch is > fixing a regression. It's not a recent one (it afaics was introduced in > v5.11-rc1). Nevertheless it would be good to get this finally resolved. > But this thread seems inactive for more than a week now. Or was progress > made, but is only visible somewhere else? I think the best solution is getting broadcom to update their firmware, I've just sent them a message through a form on their website, I couldn't seem to get it to tell me "Your message has been sent", so it's possible that it didn't submit (more likely I've sent the same message several times). If I hear back from them I'll send something here.
On 16.11.21 10:02, Orlando Chamberlain wrote: >> Bluetooth maintainers, what's the status here? The proposed patch is >> fixing a regression. It's not a recent one (it afaics was introduced in >> v5.11-rc1). Nevertheless it would be good to get this finally resolved. >> But this thread seems inactive for more than a week now. Or was progress >> made, but is only visible somewhere else? > > I think the best solution is getting broadcom to update their firmware, > I've just sent them a message through a form on their website, I couldn't > seem to get it to tell me "Your message has been sent", so it's possible > that it didn't submit (more likely I've sent the same message several times). > > If I hear back from them I'll send something here. Thx for that. But FWIW: from the point of the regression tracker that's not the best solution, as according to your report this is a regression. IOW: we deal with something that used to up to a certain kernel version and was broken by a change to the kernel. That is something frown upon in Linux kernel development, hence changes introducing regression are often quickly reverted, if they can't get fixed by follow up change quickly. That sentence has two "quickly", as we want to prevent more people running into the issue, resulting in a loss of trust. But that's what will happen if we wait for a firmware update to get developed, tested, published, and rolled out. And even then we can't expect users to have the latest firmware installed when they switch to a new kernel. Hence the best solution *afaics* might be: fix this in the kernel somehow now with a workaround; once the firmware update is out, change the kernel again to only apply the workaround if the old firmware is in use. At least that's how it looks to me from the outside. But as mentioned earlier already: as a Linux kernel regression tracker I'm getting a lot of reports on my table. I can only look briefly into most of them. Therefore I unfortunately will get things wrong or miss something important. I hope that's not the case here; if you think it is, don't hesitate to tell me about it in a public reply. That's in everyone's interest, as what I wrote above might be misleading to everyone reading this, which is something I'd like to avoid. Ciao, Thorsten (carrying his Linux kernel regression tracker hat)
> On 16-Nov-2021, at 2:56 PM, Thorsten Leemhuis <regressions@leemhuis.info> wrote: > > On 16.11.21 10:02, Orlando Chamberlain wrote: >>> Bluetooth maintainers, what's the status here? The proposed patch is >>> fixing a regression. It's not a recent one (it afaics was introduced in >>> v5.11-rc1). Nevertheless it would be good to get this finally resolved. >>> But this thread seems inactive for more than a week now. Or was progress >>> made, but is only visible somewhere else? >> >> I think the best solution is getting broadcom to update their firmware, >> I've just sent them a message through a form on their website, I couldn't >> seem to get it to tell me "Your message has been sent", so it's possible >> that it didn't submit (more likely I've sent the same message several times). >> >> If I hear back from them I'll send something here. > > Thx for that. But FWIW: from the point of the regression tracker that's > not the best solution, as according to your report this is a regression. > IOW: we deal with something that used to up to a certain kernel version > and was broken by a change to the kernel. That is something frown upon > in Linux kernel development, hence changes introducing regression are > often quickly reverted, if they can't get fixed by follow up change quickly. > > That sentence has two "quickly", as we want to prevent more people > running into the issue, resulting in a loss of trust. But that's what > will happen if we wait for a firmware update to get developed, tested, > published, and rolled out. And even then we can't expect users to have > the latest firmware installed when they switch to a new kernel. > > Hence the best solution *afaics* might be: fix this in the kernel > somehow now with a workaround; once the firmware update is out, change > the kernel again to only apply the workaround if the old firmware is in use. I have an idea. Can we make LE Read Transmit Power as a module parameter and users can turn it off if it is causing trouble. I have a patch for the same but haven't tested it yet. > > At least that's how it looks to me from the outside. But as mentioned > earlier already: as a Linux kernel regression tracker I'm getting a lot > of reports on my table. I can only look briefly into most of them. > Therefore I unfortunately will get things wrong or miss something > important. I hope that's not the case here; if you think it is, don't > hesitate to tell me about it in a public reply. That's in everyone's > interest, as what I wrote above might be misleading to everyone reading > this, which is something I'd like to avoid. > > Ciao, Thorsten (carrying his Linux kernel regression tracker hat)
On Wed, Nov 17, 2021 at 03:28:29AM +0000, Aditya Garg wrote: > > > > On 16-Nov-2021, at 2:56 PM, Thorsten Leemhuis <regressions@leemhuis.info> wrote: > > > > On 16.11.21 10:02, Orlando Chamberlain wrote: > >>> Bluetooth maintainers, what's the status here? The proposed patch is > >>> fixing a regression. It's not a recent one (it afaics was introduced in > >>> v5.11-rc1). Nevertheless it would be good to get this finally resolved. > >>> But this thread seems inactive for more than a week now. Or was progress > >>> made, but is only visible somewhere else? > >> > >> I think the best solution is getting broadcom to update their firmware, > >> I've just sent them a message through a form on their website, I couldn't > >> seem to get it to tell me "Your message has been sent", so it's possible > >> that it didn't submit (more likely I've sent the same message several times). > >> > >> If I hear back from them I'll send something here. > > > > Thx for that. But FWIW: from the point of the regression tracker that's > > not the best solution, as according to your report this is a regression. > > IOW: we deal with something that used to up to a certain kernel version > > and was broken by a change to the kernel. That is something frown upon > > in Linux kernel development, hence changes introducing regression are > > often quickly reverted, if they can't get fixed by follow up change quickly. > > > > That sentence has two "quickly", as we want to prevent more people > > running into the issue, resulting in a loss of trust. But that's what > > will happen if we wait for a firmware update to get developed, tested, > > published, and rolled out. And even then we can't expect users to have > > the latest firmware installed when they switch to a new kernel. > > > > Hence the best solution *afaics* might be: fix this in the kernel > > somehow now with a workaround; once the firmware update is out, change > > the kernel again to only apply the workaround if the old firmware is in use. > I have an idea. Can we make LE Read Transmit Power as a module parameter and users can turn it off if it is causing trouble. I have a patch for the same but haven't tested it yet. Module parameters are for the 1990's, please never add new ones as they modify code, not data, and you want to do something like this on a per-device basis, not on "all devices in the system", right? thanks, greg k-h
> On 17-Nov-2021, at 12:55 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > > On Wed, Nov 17, 2021 at 03:28:29AM +0000, Aditya Garg wrote: >> >> >>> On 16-Nov-2021, at 2:56 PM, Thorsten Leemhuis <regressions@leemhuis.info> wrote: >>> >>> On 16.11.21 10:02, Orlando Chamberlain wrote: >>>>> Bluetooth maintainers, what's the status here? The proposed patch is >>>>> fixing a regression. It's not a recent one (it afaics was introduced in >>>>> v5.11-rc1). Nevertheless it would be good to get this finally resolved. >>>>> But this thread seems inactive for more than a week now. Or was progress >>>>> made, but is only visible somewhere else? >>>> >>>> I think the best solution is getting broadcom to update their firmware, >>>> I've just sent them a message through a form on their website, I couldn't >>>> seem to get it to tell me "Your message has been sent", so it's possible >>>> that it didn't submit (more likely I've sent the same message several times). >>>> >>>> If I hear back from them I'll send something here. >>> >>> Thx for that. But FWIW: from the point of the regression tracker that's >>> not the best solution, as according to your report this is a regression. >>> IOW: we deal with something that used to up to a certain kernel version >>> and was broken by a change to the kernel. That is something frown upon >>> in Linux kernel development, hence changes introducing regression are >>> often quickly reverted, if they can't get fixed by follow up change quickly. >>> >>> That sentence has two "quickly", as we want to prevent more people >>> running into the issue, resulting in a loss of trust. But that's what >>> will happen if we wait for a firmware update to get developed, tested, >>> published, and rolled out. And even then we can't expect users to have >>> the latest firmware installed when they switch to a new kernel. >>> >>> Hence the best solution *afaics* might be: fix this in the kernel >>> somehow now with a workaround; once the firmware update is out, change >>> the kernel again to only apply the workaround if the old firmware is in use. >> I have an idea. Can we make LE Read Transmit Power as a module parameter and users can turn it off if it is causing trouble. I have a patch for the same but haven't tested it yet. > > Module parameters are for the 1990's, please never add new ones as they > modify code, not data, and you want to do something like this on a > per-device basis, not on "all devices in the system", right? Exactly. Since the issue affects only a few Macs and not all devices. In fact I have spotted just 2 Macs yet affected with this issue. > > thanks, > > greg k-h
On 17.11.21 10:26, Aditya Garg wrote: >> On 17-Nov-2021, at 12:55 PM, Greg KH <gregkh@linuxfoundation.org> wrote: >> On Wed, Nov 17, 2021 at 03:28:29AM +0000, Aditya Garg wrote: >>>> On 16-Nov-2021, at 2:56 PM, Thorsten Leemhuis <regressions@leemhuis.info> wrote: >>>> On 16.11.21 10:02, Orlando Chamberlain wrote: >>>>>> Bluetooth maintainers, what's the status here? The proposed patch is >>>>>> fixing a regression. It's not a recent one (it afaics was introduced in >>>>>> v5.11-rc1). Nevertheless it would be good to get this finally resolved. >>>>>> But this thread seems inactive for more than a week now. Or was progress >>>>>> made, but is only visible somewhere else? >>>>> >>>>> I think the best solution is getting broadcom to update their firmware, >>>>> I've just sent them a message through a form on their website, I couldn't >>>>> seem to get it to tell me "Your message has been sent", so it's possible >>>>> that it didn't submit (more likely I've sent the same message several times). >>>>> >>>>> If I hear back from them I'll send something here. >>>> >>>> Thx for that. But FWIW: from the point of the regression tracker that's >>>> not the best solution, as according to your report this is a regression. >>>> IOW: we deal with something that used to up to a certain kernel version >>>> and was broken by a change to the kernel. That is something frown upon >>>> in Linux kernel development, hence changes introducing regression are >>>> often quickly reverted, if they can't get fixed by follow up change quickly. >>>> >>>> That sentence has two "quickly", as we want to prevent more people >>>> running into the issue, resulting in a loss of trust. But that's what >>>> will happen if we wait for a firmware update to get developed, tested, >>>> published, and rolled out. And even then we can't expect users to have >>>> the latest firmware installed when they switch to a new kernel. >>>> >>>> Hence the best solution *afaics* might be: fix this in the kernel >>>> somehow now with a workaround; once the firmware update is out, change >>>> the kernel again to only apply the workaround if the old firmware is in use. >>> I have an idea. Can we make LE Read Transmit Power as a module parameter and users can turn it off if it is causing trouble. I have a patch for the same but haven't tested it yet. >> >> Module parameters are for the 1990's, please never add new ones as they >> modify code, not data, and you want to do something like this on a >> per-device basis, not on "all devices in the system", right? > > Exactly. Since the issue affects only a few Macs and not all devices. > In fact I have spotted just 2 Macs yet affected with this issue. When Greg said "per-device basis", he afaics meant: per-device in a system, as a module parameter would also affect a second bluetooth controller if there was one (say one connected via USB) -- and that shouldn't happen. And FWIW: it's still a regression if something that used to work suddenly requires a module parameter to get working. So if this just affects two macs, why can't the fix be realized as a quirk that is only enabled on those two systems? Or are they impossible to detect clearly via DMI data or something like that? Ciao, Thorsten
> On 17-Nov-2021, at 3:12 PM, Thorsten Leemhuis <regressions@leemhuis.info> wrote: > > On 17.11.21 10:26, Aditya Garg wrote: >>> On 17-Nov-2021, at 12:55 PM, Greg KH <gregkh@linuxfoundation.org> wrote: >>> On Wed, Nov 17, 2021 at 03:28:29AM +0000, Aditya Garg wrote: >>>>> On 16-Nov-2021, at 2:56 PM, Thorsten Leemhuis <regressions@leemhuis.info> wrote: >>>>> On 16.11.21 10:02, Orlando Chamberlain wrote: >>>>>>> Bluetooth maintainers, what's the status here? The proposed patch is >>>>>>> fixing a regression. It's not a recent one (it afaics was introduced in >>>>>>> v5.11-rc1). Nevertheless it would be good to get this finally resolved. >>>>>>> But this thread seems inactive for more than a week now. Or was progress >>>>>>> made, but is only visible somewhere else? >>>>>> >>>>>> I think the best solution is getting broadcom to update their firmware, >>>>>> I've just sent them a message through a form on their website, I couldn't >>>>>> seem to get it to tell me "Your message has been sent", so it's possible >>>>>> that it didn't submit (more likely I've sent the same message several times). >>>>>> >>>>>> If I hear back from them I'll send something here. >>>>> >>>>> Thx for that. But FWIW: from the point of the regression tracker that's >>>>> not the best solution, as according to your report this is a regression. >>>>> IOW: we deal with something that used to up to a certain kernel version >>>>> and was broken by a change to the kernel. That is something frown upon >>>>> in Linux kernel development, hence changes introducing regression are >>>>> often quickly reverted, if they can't get fixed by follow up change quickly. >>>>> >>>>> That sentence has two "quickly", as we want to prevent more people >>>>> running into the issue, resulting in a loss of trust. But that's what >>>>> will happen if we wait for a firmware update to get developed, tested, >>>>> published, and rolled out. And even then we can't expect users to have >>>>> the latest firmware installed when they switch to a new kernel. >>>>> >>>>> Hence the best solution *afaics* might be: fix this in the kernel >>>>> somehow now with a workaround; once the firmware update is out, change >>>>> the kernel again to only apply the workaround if the old firmware is in use. >>>> I have an idea. Can we make LE Read Transmit Power as a module parameter and users can turn it off if it is causing trouble. I have a patch for the same but haven't tested it yet. >>> >>> Module parameters are for the 1990's, please never add new ones as they >>> modify code, not data, and you want to do something like this on a >>> per-device basis, not on "all devices in the system", right? >> >> Exactly. Since the issue affects only a few Macs and not all devices. >> In fact I have spotted just 2 Macs yet affected with this issue. > When Greg said "per-device basis", he afaics meant: per-device in a > system, as a module parameter would also affect a second bluetooth > controller if there was one (say one connected via USB) -- and that > shouldn't happen. > > And FWIW: it's still a regression if something that used to work > suddenly requires a module parameter to get working. > > So if this just affects two macs, why can't the fix be realized as a > quirk that is only enabled on those two systems? Or are they impossible > to detect clearly via DMI data or something like that? <RESENDING AS PLAIN TEXT> A part of the output of dmidecode is below :- Handle 0x0005, DMI type 1, 27 bytes System Information Manufacturer: Apple Inc. Product Name: MacBookPro16,1 Version: 1.0 Serial Number: <Removed for Privacy> UUID: <Removed for Privacy> Wake-up Type: Power Switch SKU Number: Family: MacBook Pro Handle 0x0006, DMI type 2, 17 bytes Base Board Information Manufacturer: Apple Inc. Product Name: Mac-E1008331FDC96864 Version: MacBookPro16,1 Serial Number: <Removed for Privacy> Asset Tag: Features: Board is a hosting board Location In Chassis: Chassis Handle: 0x0007 Type: Motherboard Contained Object Handles: 0 The product name, MacBookPro16,1 in my case, is unique for each model. If possible a quirk to disable LE Read Transmit Power can be made on this basis. > > Ciao, Thorsten
> So if this just affects two macs, why can't the fix be realized as a > quirk that is only enabled on those two systems? Or are they impossible > to detect clearly via DMI data or something like that? I think we should be able to quirk based off the acpi _CID "apple-uart-blth" or _HID "BCM2E7C". Marcel suggested quirking based of the acpi table here https://lore.kernel.org/linux-bluetooth/1D2217A9-EA73-4D93-8D0B-5BC2718D4788@holtmann.org/ This would catch some unaffected Macs, but they don't support the LE Read Transmit Power command anyway (the affected macs were released after it was added to the Bluetooth spec, while the unaffected Macs were released before it was added to the spec, and thus don't support it). I'm not sure how to go about applying a quirk based off this, there are quirks in drivers/bluetooth/hci_bcm.c (no_early_set_baudrate and drive_rts_on_open), but they don't seem to be based off acpi ids. It might be simpler to make it ignore the Unknown Command error, like in this patch https://lore.kernel.org/linux-bluetooth/CABBYNZLjSfcG_KqTEbL6NOSvHhA5-b1t_S=3FQP4=GwW21kuzg@mail.gmail.com/ however that only applies on bluetooth-next and needed the status it checks for to be -56, not 0x01. -- Thanks, Orlando
Hi Orlando, >> So if this just affects two macs, why can't the fix be realized as a >> quirk that is only enabled on those two systems? Or are they impossible >> to detect clearly via DMI data or something like that? > > I think we should be able to quirk based off the acpi _CID "apple-uart-blth" > or _HID "BCM2E7C". Marcel suggested quirking based of the acpi table here > https://lore.kernel.org/linux-bluetooth/1D2217A9-EA73-4D93-8D0B-5BC2718D4788@holtmann.org/ > > This would catch some unaffected Macs, but they don't support the LE Read > Transmit Power command anyway (the affected macs were released after it > was added to the Bluetooth spec, while the unaffected Macs were released > before it was added to the spec, and thus don't support it). > > I'm not sure how to go about applying a quirk based off this, there are > quirks in drivers/bluetooth/hci_bcm.c (no_early_set_baudrate and > drive_rts_on_open), but they don't seem to be based off acpi ids. > > It might be simpler to make it ignore the Unknown Command error, like > in this patch https://lore.kernel.org/linux-bluetooth/CABBYNZLjSfcG_KqTEbL6NOSvHhA5-b1t_S=3FQP4=GwW21kuzg@mail.gmail.com/ > however that only applies on bluetooth-next and needed the status it > checks for to be -56, not 0x01. so we abstain from try-and-error sending of commands. The Bluetooth spec has a list of supported commands that a host can query for a reason. This is really broken behavior of the controller and needs to be pointed out as such. The question is just how we quirk it. Regards Marcel
> On 18-Nov-2021, at 12:31 AM, Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Orlando, > >>> So if this just affects two macs, why can't the fix be realized as a >>> quirk that is only enabled on those two systems? Or are they impossible >>> to detect clearly via DMI data or something like that? >> >> I think we should be able to quirk based off the acpi _CID "apple-uart-blth" >> or _HID "BCM2E7C". Marcel suggested quirking based of the acpi table here >> https://lore.kernel.org/linux-bluetooth/1D2217A9-EA73-4D93-8D0B-5BC2718D4788@holtmann.org/ >> >> This would catch some unaffected Macs, but they don't support the LE Read >> Transmit Power command anyway (the affected macs were released after it >> was added to the Bluetooth spec, while the unaffected Macs were released >> before it was added to the spec, and thus don't support it). >> >> I'm not sure how to go about applying a quirk based off this, there are >> quirks in drivers/bluetooth/hci_bcm.c (no_early_set_baudrate and >> drive_rts_on_open), but they don't seem to be based off acpi ids. >> >> It might be simpler to make it ignore the Unknown Command error, like >> in this patch https://lore.kernel.org/linux-bluetooth/CABBYNZLjSfcG_KqTEbL6NOSvHhA5-b1t_S=3FQP4=GwW21kuzg@mail.gmail.com/ >> however that only applies on bluetooth-next and needed the status it >> checks for to be -56, not 0x01. > > so we abstain from try-and-error sending of commands. The Bluetooth spec > has a list of supported commands that a host can query for a reason. This > is really broken behavior of the controller and needs to be pointed out as > such. Well all I can do is provide you any logs or information I can. But we do really wish to get this regression fixed soon. > > The question is just how we quirk it. > > Regards > > Marcel >
Hi, this is your Linux kernel regression tracker speaking again. On 19.11.21 17:59, Aditya Garg wrote: >> On 18-Nov-2021, at 12:31 AM, Marcel Holtmann <marcel@holtmann.org> wrote: >>>> So if this just affects two macs, why can't the fix be realized as a >>>> quirk that is only enabled on those two systems? Or are they impossible >>>> to detect clearly via DMI data or something like that? >>> >>> I think we should be able to quirk based off the acpi _CID "apple-uart-blth" >>> or _HID "BCM2E7C". Marcel suggested quirking based of the acpi table here >>> https://lore.kernel.org/linux-bluetooth/1D2217A9-EA73-4D93-8D0B-5BC2718D4788@holtmann.org/ >>> >>> This would catch some unaffected Macs, but they don't support the LE Read >>> Transmit Power command anyway (the affected macs were released after it >>> was added to the Bluetooth spec, while the unaffected Macs were released >>> before it was added to the spec, and thus don't support it). >>> >>> I'm not sure how to go about applying a quirk based off this, there are >>> quirks in drivers/bluetooth/hci_bcm.c (no_early_set_baudrate and >>> drive_rts_on_open), but they don't seem to be based off acpi ids. >>> >>> It might be simpler to make it ignore the Unknown Command error, like >>> in this patch https://lore.kernel.org/linux-bluetooth/CABBYNZLjSfcG_KqTEbL6NOSvHhA5-b1t_S=3FQP4=GwW21kuzg@mail.gmail.com/ >>> however that only applies on bluetooth-next and needed the status it >>> checks for to be -56, not 0x01. >> >> so we abstain from try-and-error sending of commands. The Bluetooth spec >> has a list of supported commands that a host can query for a reason. This >> is really broken behavior of the controller and needs to be pointed out as >> such. > Well all I can do is provide you any logs or information I can. But we do really wish to get this regression fixed soon. >> >> The question is just how we quirk it. This thread once again looks stalled and smells a lot like "everyone agrees that his should be fixed, but afaics nobody submitted a fix or committed to work on one". Please speak up if my impression is wrong, as this is a regression and thus needs to be fixed, ideally quickly. Part of my job is to make that happen and thus remind developers and maintainers about this until we have a fix. Ciao, Thorsten #regzbot title bluetooth: "Query LE tx power on startup" broke Bluetooth on MacBookPro16,1 #regzbot poke
> On 25-Nov-2021, at 5:56 PM, Thorsten Leemhuis <regressions@leemhuis.info> wrote: > > Hi, this is your Linux kernel regression tracker speaking again. > > On 19.11.21 17:59, Aditya Garg wrote: >>> On 18-Nov-2021, at 12:31 AM, Marcel Holtmann <marcel@holtmann.org> wrote: >>>>> So if this just affects two macs, why can't the fix be realized as a >>>>> quirk that is only enabled on those two systems? Or are they impossible >>>>> to detect clearly via DMI data or something like that? >>>> >>>> I think we should be able to quirk based off the acpi _CID "apple-uart-blth" >>>> or _HID "BCM2E7C". Marcel suggested quirking based of the acpi table here >>>> https://lore.kernel.org/linux-bluetooth/1D2217A9-EA73-4D93-8D0B-5BC2718D4788@holtmann.org/ >>>> >>>> This would catch some unaffected Macs, but they don't support the LE Read >>>> Transmit Power command anyway (the affected macs were released after it >>>> was added to the Bluetooth spec, while the unaffected Macs were released >>>> before it was added to the spec, and thus don't support it). >>>> >>>> I'm not sure how to go about applying a quirk based off this, there are >>>> quirks in drivers/bluetooth/hci_bcm.c (no_early_set_baudrate and >>>> drive_rts_on_open), but they don't seem to be based off acpi ids. >>>> >>>> It might be simpler to make it ignore the Unknown Command error, like >>>> in this patch https://lore.kernel.org/linux-bluetooth/CABBYNZLjSfcG_KqTEbL6NOSvHhA5-b1t_S=3FQP4=GwW21kuzg@mail.gmail.com/ >>>> however that only applies on bluetooth-next and needed the status it >>>> checks for to be -56, not 0x01. >>> >>> so we abstain from try-and-error sending of commands. The Bluetooth spec >>> has a list of supported commands that a host can query for a reason. This >>> is really broken behavior of the controller and needs to be pointed out as >>> such. >> Well all I can do is provide you any logs or information I can. But we do really wish to get this regression fixed soon. >>> >>> The question is just how we quirk it. > > This thread once again looks stalled and smells a lot like "everyone > agrees that his should be fixed, but afaics nobody submitted a fix or > committed to work on one". Please speak up if my impression is wrong, as > this is a regression and thus needs to be fixed, ideally quickly. Part > of my job is to make that happen and thus remind developers and > maintainers about this until we have a fix. On the basis of DMI data, I have made this patch to disable read transmit power on 16,1. I have tested this on my 16,1 successfully. Still consider this as a draft as more models have to be added. I am sending this to get the approval of the maintainers whether this quirk is acceptable or not. If yes, I shall send the final patch. From 3dab2e1e9e0b266574f5f010efc6680417fb0c61 Mon Sep 17 00:00:00 2001 From: Aditya Garg <gargaditya08@live.com> Date: Fri, 26 Nov 2021 18:28:46 +0530 Subject: [PATCH] Add quirk to disable read transmit power on MacBook Pro 16 inch, 2019 --- net/bluetooth/hci_core.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index 8d33aa64846b1c..d11064cb3666ef 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -32,6 +32,7 @@ #include <linux/property.h> #include <linux/suspend.h> #include <linux/wait.h> +#include <linux/dmi.h> #include <asm/unaligned.h> #include <net/bluetooth/bluetooth.h> @@ -461,9 +462,23 @@ static void hci_set_event_mask_page_2(struct hci_request *req) sizeof(events), events); } +static const struct dmi_system_id fix_up_apple_bluetooth[] = { + { + /* Match for Apple MacBook Pro 16 inch, 2019 which needs + * read transmit power to be disabled + */ + .matches = { + DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."), + DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,1"), + }, + }, + { } +}; + static int hci_init3_req(struct hci_request *req, unsigned long opt) { struct hci_dev *hdev = req->hdev; + const struct dmi_system_id *dmi_id; u8 p; hci_setup_event_mask(req); @@ -619,7 +634,8 @@ static int hci_init3_req(struct hci_request *req, unsigned long opt) hci_req_add(req, HCI_OP_LE_READ_ADV_TX_POWER, 0, NULL); } - if (hdev->commands[38] & 0x80) { + dmi_id = dmi_first_match(fix_up_apple_bluetooth); + if (hdev->commands[38] & 0x80 && (!dmi_id)) { /* Read LE Min/Max Tx Power*/ hci_req_add(req, HCI_OP_LE_READ_TRANSMIT_POWER, 0, NULL); > > Ciao, Thorsten > > #regzbot title bluetooth: "Query LE tx power on startup" broke Bluetooth > on MacBookPro16,1 > #regzbot poke
So I have finally managed to add quirks in btbcm on the basis on DMI data. This shall be advantageous in the situation when the user shall be using a USB adapter so that the quirk gets ineffective for the adapter. > On 26-Nov-2021, at 8:45 PM, Aditya Garg <gargaditya08@live.com> wrote: > > > >> On 25-Nov-2021, at 5:56 PM, Thorsten Leemhuis <regressions@leemhuis.info> wrote: >> >> Hi, this is your Linux kernel regression tracker speaking again. >> >> On 19.11.21 17:59, Aditya Garg wrote: >>>> On 18-Nov-2021, at 12:31 AM, Marcel Holtmann <marcel@holtmann.org> wrote: >>>>>> So if this just affects two macs, why can't the fix be realized as a >>>>>> quirk that is only enabled on those two systems? Or are they impossible >>>>>> to detect clearly via DMI data or something like that? >>>>> >>>>> I think we should be able to quirk based off the acpi _CID "apple-uart-blth" >>>>> or _HID "BCM2E7C". Marcel suggested quirking based of the acpi table here >>>>> https://lore.kernel.org/linux-bluetooth/1D2217A9-EA73-4D93-8D0B-5BC2718D4788@holtmann.org/ >>>>> >>>>> This would catch some unaffected Macs, but they don't support the LE Read >>>>> Transmit Power command anyway (the affected macs were released after it >>>>> was added to the Bluetooth spec, while the unaffected Macs were released >>>>> before it was added to the spec, and thus don't support it). >>>>> >>>>> I'm not sure how to go about applying a quirk based off this, there are >>>>> quirks in drivers/bluetooth/hci_bcm.c (no_early_set_baudrate and >>>>> drive_rts_on_open), but they don't seem to be based off acpi ids. >>>>> >>>>> It might be simpler to make it ignore the Unknown Command error, like >>>>> in this patch https://lore.kernel.org/linux-bluetooth/CABBYNZLjSfcG_KqTEbL6NOSvHhA5-b1t_S=3FQP4=GwW21kuzg@mail.gmail.com/ >>>>> however that only applies on bluetooth-next and needed the status it >>>>> checks for to be -56, not 0x01. >>>> >>>> so we abstain from try-and-error sending of commands. The Bluetooth spec >>>> has a list of supported commands that a host can query for a reason. This >>>> is really broken behavior of the controller and needs to be pointed out as >>>> such. >>> Well all I can do is provide you any logs or information I can. But we do really wish to get this regression fixed soon. >>>> >>>> The question is just how we quirk it. >> >> This thread once again looks stalled and smells a lot like "everyone >> agrees that his should be fixed, but afaics nobody submitted a fix or >> committed to work on one". Please speak up if my impression is wrong, as >> this is a regression and thus needs to be fixed, ideally quickly. Part >> of my job is to make that happen and thus remind developers and >> maintainers about this until we have a fix. > On the basis of DMI data, I have made this patch to disable read transmit power on 16,1. I have tested this on my 16,1 successfully. Still consider this as a draft as more models have to be added. I am sending this to get the approval of the maintainers whether this quirk is acceptable or not. If yes, I shall send the final patch. > > From 3dab2e1e9e0b266574f5f010efc6680417fb0c61 Mon Sep 17 00:00:00 2001 > From: Aditya Garg <gargaditya08@live.com> > Date: Fri, 26 Nov 2021 18:28:46 +0530 > Subject: [PATCH] Add quirk to disable read transmit power on MacBook Pro 16 > inch, 2019 > > --- > net/bluetooth/hci_core.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > index 8d33aa64846b1c..d11064cb3666ef 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -32,6 +32,7 @@ > #include <linux/property.h> > #include <linux/suspend.h> > #include <linux/wait.h> > +#include <linux/dmi.h> > #include <asm/unaligned.h> > > #include <net/bluetooth/bluetooth.h> > @@ -461,9 +462,23 @@ static void hci_set_event_mask_page_2(struct hci_request *req) > sizeof(events), events); > } > > +static const struct dmi_system_id fix_up_apple_bluetooth[] = { > + { > + /* Match for Apple MacBook Pro 16 inch, 2019 which needs > + * read transmit power to be disabled > + */ > + .matches = { > + DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."), > + DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,1"), > + }, > + }, > + { } > +}; > + > static int hci_init3_req(struct hci_request *req, unsigned long opt) > { > struct hci_dev *hdev = req->hdev; > + const struct dmi_system_id *dmi_id; > u8 p; > > hci_setup_event_mask(req); > @@ -619,7 +634,8 @@ static int hci_init3_req(struct hci_request *req, unsigned long opt) > hci_req_add(req, HCI_OP_LE_READ_ADV_TX_POWER, 0, NULL); > } > > - if (hdev->commands[38] & 0x80) { > + dmi_id = dmi_first_match(fix_up_apple_bluetooth); > + if (hdev->commands[38] & 0x80 && (!dmi_id)) { > /* Read LE Min/Max Tx Power*/ > hci_req_add(req, HCI_OP_LE_READ_TRANSMIT_POWER, > 0, NULL); >> >> Ciao, Thorsten >> >> #regzbot title bluetooth: "Query LE tx power on startup" broke Bluetooth >> on MacBookPro16,1 >> #regzbot poke
diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c index e4182acee488..4ecc50d93107 100644 --- a/drivers/bluetooth/btbcm.c +++ b/drivers/bluetooth/btbcm.c @@ -353,6 +353,10 @@ static int btbcm_read_info(struct hci_dev *hdev) return PTR_ERR(skb); bt_dev_info(hdev, "BCM: chip id %u", skb->data[1]); + + if (skb->data[1] == 150) + set_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, &hdev->quirks); + kfree_skb(skb); /* Read Controller Features */ diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h index b80415011dcd..6da9bd6b7259 100644 --- a/include/net/bluetooth/hci.h +++ b/include/net/bluetooth/hci.h @@ -246,6 +246,17 @@ enum { * HCI after resume. */ HCI_QUIRK_NO_SUSPEND_NOTIFIER, + + /* When this quirk is set, LE Read Transmit Power is disabled. + * This is mainly due to the fact that the HCI LE Read Transmit + * Power command is advertised, but not supported; these + * controllers often reply with unknown command and need a hard + * reset. + * + * This quirk can be set before hci_register_dev is called or + * during the hdev->setup vendor callback. + */ + HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, }; /* HCI device flags */ diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index 8a47a3017d61..9a23fe7c8d67 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -742,7 +742,8 @@ static int hci_init3_req(struct hci_request *req, unsigned long opt) hci_req_add(req, HCI_OP_LE_READ_ADV_TX_POWER, 0, NULL); } - if (hdev->commands[38] & 0x80) { + if (hdev->commands[38] & 0x80 && + !test_bit(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER, &hdev->quirks)) { /* Read LE Min/Max Tx Power*/ hci_req_add(req, HCI_OP_LE_READ_TRANSMIT_POWER, 0, NULL);
The LE Read Transmit Power command is Advertised on some Broadcom controlers, but not supported. Using this command breaks Bluetooth on the MacBookPro16,1 and iMac20,1. Added a quirk disabling LE Read Transmit Power for these devices, based off their common chip id 150. Link: https://lore.kernel.org/r/4970a940-211b-25d6-edab-21a815313954@protonmail.com Signed-off-by: Orlando Chamberlain <redecorating@protonmail.com> --- v1->v2: Clarified quirk description drivers/bluetooth/btbcm.c | 4 ++++ include/net/bluetooth/hci.h | 11 +++++++++++ net/bluetooth/hci_core.c | 3 ++- 3 files changed, 17 insertions(+), 1 deletion(-)