| Message ID | 20240626092801.2343844-1-kiran.k@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] Bluetooth: btintel: Allow lowering of drive strength of BRI | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | fail | 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 31: B1 Line exceeds max length (84>80): "[17.368942] Bluetooth: hci0: Found Intel DDC parameters: intel/ibt-0190-0291-pci.ddc" |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | success | CheckSparse PASS |
| tedd_an/CheckSmatch | success | CheckSparse PASS |
| tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
| tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
Dear Kiran, Thank you for the patch. Am 26.06.24 um 11:28 schrieb Kiran K: > BRI (Bluetooth Radio Interface) traffic from CNVr to CNVi was found > causing cross talk step errors to WiFi. As a workaround, driver needs to > reduce the drive strength of BRI. During *setup*, driver reads the drive > strength value from efi variable and passes it to the controller via vendor > specific command with opcode 0xfc0a. I am still surprised this is done via an EFI variable. Could you please add a reference to section in the UEFI(?) specification? Hopefully that explains who is supposed to set the variable. […] Kind regards, Paul
Hi Paul, >-----Original Message----- >From: Paul Menzel <pmenzel@molgen.mpg.de> >Sent: Wednesday, June 26, 2024 5:56 PM >To: K, Kiran <kiran.k@intel.com> >Cc: Srivatsa, Ravishankar <ravishankar.srivatsa@intel.com>; Tumkur Narayan, >Chethan <chethan.tumkur.narayan@intel.com>; Devegowda, Chandrashekar ><chandrashekar.devegowda@intel.com>; Satija, Vijay <vijay.satija@intel.com>; >linux-bluetooth@vger.kernel.org >Subject: Re: [PATCH v3] Bluetooth: btintel: Allow lowering of drive strength of >BRI > >Dear Kiran, > > >Thank you for the patch. > >Am 26.06.24 um 11:28 schrieb Kiran K: >> BRI (Bluetooth Radio Interface) traffic from CNVr to CNVi was found >> causing cross talk step errors to WiFi. As a workaround, driver needs >> to reduce the drive strength of BRI. During *setup*, driver reads the >> drive strength value from efi variable and passes it to the controller >> via vendor specific command with opcode 0xfc0a. > >I am still surprised this is done via an EFI variable. Could you please add a >reference to section in the UEFI(?) specification? Hopefully that explains who is >supposed to set the variable. "UefiCnvCommonDSBR" efi variable would be created by OEMs. > >[…] > > >Kind regards, > >Paul Thanks, Kiran
Dear Kiran, Thank you for your reply. Am 27.06.24 um 03:03 schrieb K, Kiran: >> -----Original Message----- >> From: Paul Menzel <pmenzel@molgen.mpg.de> >> Sent: Wednesday, June 26, 2024 5:56 PM […] >> Am 26.06.24 um 11:28 schrieb Kiran K: >>> BRI (Bluetooth Radio Interface) traffic from CNVr to CNVi was found >>> causing cross talk step errors to WiFi. As a workaround, driver needs >>> to reduce the drive strength of BRI. During *setup*, driver reads the >>> drive strength value from efi variable and passes it to the controller >>> via vendor specific command with opcode 0xfc0a. >> >> I am still surprised this is done via an EFI variable. Could you please add a >> reference to section in the UEFI(?) specification? Hopefully that explains who is >> supposed to set the variable. > > "UefiCnvCommonDSBR" efi variable would be created by OEMs. Isn’t that approach fundamentally broken? How do the OEMs know, what variable to set. It needs to be standardized somewhere (name and allowed values). Also, there are non-UEFI firmwares, like coreboot based, used, for example, with Google Chromebooks. Lastly, users can manipulate UEFI variables to my knowledge. Kind regards, Paul
Hi Paul, >Subject: Re: [PATCH v3] Bluetooth: btintel: Allow lowering of drive strength of >BRI > >Dear Kiran, > > >Thank you for your reply. > >Am 27.06.24 um 03:03 schrieb K, Kiran: > >>> -----Original Message----- >>> From: Paul Menzel <pmenzel@molgen.mpg.de> >>> Sent: Wednesday, June 26, 2024 5:56 PM > >[…] > >>> Am 26.06.24 um 11:28 schrieb Kiran K: >>>> BRI (Bluetooth Radio Interface) traffic from CNVr to CNVi was found >>>> causing cross talk step errors to WiFi. As a workaround, driver >>>> needs to reduce the drive strength of BRI. During *setup*, driver >>>> reads the drive strength value from efi variable and passes it to >>>> the controller via vendor specific command with opcode 0xfc0a. >>> >>> I am still surprised this is done via an EFI variable. Could you >>> please add a reference to section in the UEFI(?) specification? >>> Hopefully that explains who is supposed to set the variable. >> >> "UefiCnvCommonDSBR" efi variable would be created by OEMs. > >Isn’t that approach fundamentally broken? How do the OEMs know, what >variable to set. It needs to be standardized somewhere (name and allowed >values). Also, there are non-UEFI firmwares, like coreboot based, used, for >example, with Google Chromebooks. Lastly, users can manipulate UEFI >variables to my knowledge. Intel shares the information about the UEFI variables to customers (via confidential documents). OEMs may modify these variables based on the platform / re-work / BT NIC etc. Also these variables are locked in BIOS, so I believe its not possible to modify these values later. For non-UEFI firmwares, driver would fetch the data from BIOS from ACPI table. Currently we don’t have requirement to support core boot. I would submit patches for the same if it comes in future :) > > >Kind regards, > >Paul Thanks, Kiran
[Cc: +Guenter to get a Chromium person in the loop] Dear Kiran, Thank you for your reply. Am 28.06.24 um 11:37 schrieb K, Kiran: […] >> Am 27.06.24 um 03:03 schrieb K, Kiran: >> >>>> -----Original Message----- >>>> From: Paul Menzel <pmenzel@molgen.mpg.de> >>>> Sent: Wednesday, June 26, 2024 5:56 PM >> >> […] >> >>>> Am 26.06.24 um 11:28 schrieb Kiran K: >>>>> BRI (Bluetooth Radio Interface) traffic from CNVr to CNVi was found >>>>> causing cross talk step errors to WiFi. As a workaround, driver >>>>> needs to reduce the drive strength of BRI. During *setup*, driver >>>>> reads the drive strength value from efi variable and passes it to >>>>> the controller via vendor specific command with opcode 0xfc0a. >>>> >>>> I am still surprised this is done via an EFI variable. Could you >>>> please add a reference to section in the UEFI(?) specification? >>>> Hopefully that explains who is supposed to set the variable. >>> >>> "UefiCnvCommonDSBR" efi variable would be created by OEMs. >> >> Isn’t that approach fundamentally broken? How do the OEMs know, what >> variable to set. It needs to be standardized somewhere (name and allowed >> values). Also, there are non-UEFI firmwares, like coreboot based, used, for >> example, with Google Chromebooks. Lastly, users can manipulate UEFI >> variables to my knowledge. > > Intel shares the information about the UEFI variables to customers > (via confidential documents). OEMs may modify these variables based > on the platform / re-work / BT NIC etc. Also these variables are > locked in BIOS, so I believe its not possible to modify these values > later. > > For non-UEFI firmwares, driver would fetch the data from BIOS from > ACPI table. Why can’t ACPI tables be used for all? I strongly oppose to add such an undocumented feature to the Linux kernel. Intel should oppose the current practice. > Currently we don’t have requirement to support coreboot. > I would submit patches for the same if it comes in future :) Kind regards, Paul
Hi Kiran, > BRI (Bluetooth Radio Interface) traffic from CNVr to CNVi was found > causing cross talk step errors to WiFi. As a workaround, driver needs to > reduce the drive strength of BRI. During *setup*, driver reads the drive > strength value from efi variable and passes it to the controller via vendor > specific command with opcode 0xfc0a. > > dmesg: > ..... > [16.767459] Bluetooth: hci0: Found device firmware: intel/ibt-0190-0291-iml.sfi > [16.767464] Bluetooth: hci0: Boot Address: 0x30099000 > [16.767464] Bluetooth: hci0: Firmware Version: 9-25.24 > [16.825418] Bluetooth: hci0: Waiting for firmware download to complete > [16.825421] Bluetooth: hci0: Firmware loaded in 56600 usecs > [16.825463] Bluetooth: hci0: Waiting for device to boot > [16.827510] Bluetooth: hci0: Malformed MSFT vendor event: 0x02 > [16.827529] Bluetooth: hci0: Device booted in 2053 usecs > [16.827707] Bluetooth: hci0: dsbr: enabled: 0x01 value: 0x0f > [16.830179] Bluetooth: hci0: Found device firmware: intel/ibt-0190-0291-pci.sfi > [16.830188] Bluetooth: hci0: Boot Address: 0x10000800 > [16.830189] Bluetooth: hci0: Firmware Version: 9-25.24 > [16.928308] Bluetooth: BNEP (Ethernet Emulation) ver 1.3 > [16.928311] Bluetooth: BNEP filters: protocol multicast > [16.928315] Bluetooth: BNEP socket layer initialized > [17.333292] Bluetooth: hci0: Waiting for firmware download to complete > [17.333313] Bluetooth: hci0: Firmware loaded in 491339 usecs > [17.333353] Bluetooth: hci0: Waiting for device to boot > [17.368741] Bluetooth: hci0: Device booted in 34585 usecs > [17.368742] Bluetooth: hci0: Malformed MSFT vendor event: 0x02 > [17.368942] Bluetooth: hci0: Found Intel DDC parameters: intel/ibt-0190-0291-pci.ddc > [17.369199] Bluetooth: hci0: Applying Intel DDC parameters completed > [17.369447] Bluetooth: hci0: Firmware timestamp 2024.25 buildtype 3 build 64777 > [17.369448] Bluetooth: hci0: Firmware SHA1: 0xc33eb15f > [17.369648] Bluetooth: hci0: Fseq status: Success (0x00) > [17.369649] Bluetooth: hci0: Fseq executed: 00.00.04.183 > [17.369650] Bluetooth: hci0: Fseq BT Top: 00.00.04.183 > [17.408366] Bluetooth: MGMT ver 1.23 > [17.408415] Bluetooth: ISO socket layer initialized > [17.434375] Bluetooth: RFCOMM TTY layer initialized > [17.434385] Bluetooth: RFCOMM socket layer initialized > [17.434389] Bluetooth: RFCOMM ver 1.11 > > Signed-off-by: Kiran K <kiran.k@intel.com> > --- > drivers/bluetooth/btintel.c | 117 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 117 insertions(+) > > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c > index 5d735391545a..fb9d4221ccd6 100644 > --- a/drivers/bluetooth/btintel.c > +++ b/drivers/bluetooth/btintel.c > @@ -12,6 +12,8 @@ > #include <linux/acpi.h> > #include <acpi/acpi_bus.h> > #include <asm/unaligned.h> > +#include <linux/efi.h> > + > introducing extra empty line is not acceptable. Please carefully review patches before sending them to the mailing list. These things are easily caught by just looking at the patch. > #include <net/bluetooth/bluetooth.h> > #include <net/bluetooth/hci_core.h> > @@ -26,6 +28,8 @@ > #define ECDSA_OFFSET 644 > #define ECDSA_HEADER_LEN 320 > > +#define BTINTEL_EFI_DRBR L"UefiCnvCommonDSBR" > + I don’t get the naming. Is it DRBR or DSBR? > enum { > DSM_SET_WDISABLE2_DELAY = 1, > DSM_SET_RESET_METHOD = 3, > @@ -49,6 +53,38 @@ static const guid_t btintel_guid_dsm = > GUID_INIT(0xaa10f4e0, 0x81ac, 0x4233, > 0xab, 0xf6, 0x3b, 0x2a, 0xc5, 0x0e, 0x28, 0xd9); > > +static void *btintel_uefi_get_variable(efi_char16_t *name, efi_guid_t *guid) > +{ > + void *data; > + efi_status_t status; > + unsigned long data_size = 0; > + > + if (!IS_ENABLED(CONFIG_EFI)) > + return ERR_PTR(-EOPNOTSUPP); > + > + if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) > + return ERR_PTR(-EOPNOTSUPP); > + > + status = efi.get_variable(name, guid, NULL, &data_size, NULL); > + > + if (status != EFI_BUFFER_TOO_SMALL || !data_size) > + return ERR_PTR(-EIO); > + > + data = kmalloc(data_size, GFP_KERNEL); > + > + if (!data) > + return ERR_PTR(-ENOMEM); > + > + status = efi.get_variable(name, guid, NULL, &data_size, data); > + > + if (status != EFI_SUCCESS) { > + kfree(data); > + return ERR_PTR(-ENXIO); > + } > + > + return data; > +} > + This is something I don’t like at all. You are reading a single EFI variable, but putting an abstraction in place as if we would be reading at least a dozen of them. Please don’t do that since they only thing you are doing in the end is hiding compiler warnings by casting everything to void and making the code a memory allocation hard to follow. This could be just like this: static u32 btintel_uefi_get_dsbr(void). If at any point in the future more abstraction is needed, this is internal code and changes are easy. No ABI breakage will happen. For now keep it simple. > int btintel_check_bdaddr(struct hci_dev *hdev) > { > struct hci_rp_read_bd_addr *bda; > @@ -2615,6 +2651,80 @@ static u8 btintel_classify_pkt_type(struct hci_dev *hdev, struct sk_buff *skb) > return hci_skb_pkt_type(skb); > } > > +static int btintel_set_dsbr(struct hci_dev *hdev, struct intel_version_tlv *ver) > +{ > + struct btintel_dsbr_cmd { > + u8 enable; > + u8 dsbr; > + } __packed; > + > + struct btintel_dsbr { > + u8 header; > + u32 dsbr; > + } __packed; > + > + struct btintel_dsbr *dsbr; > + struct btintel_dsbr_cmd cmd; > + struct sk_buff *skb; > + u8 status; > + efi_guid_t guid = EFI_GUID(0xe65d8884, 0xd4af, 0x4b20, 0x8d, 0x03, > + 0x77, 0x2e, 0xcc, 0x3d, 0xa5, 0x31); > + > + memset(&cmd, 0, sizeof(cmd)); > + dsbr = btintel_uefi_get_variable(BTINTEL_EFI_DRBR, &guid); > + if (IS_ERR(dsbr)) { > + /* If efi variable is not present, driver still needs to send > + * 0xfc0a command with default values > + */ > + bt_dev_dbg(hdev, "Error reading efi: %ls DSBR (%ld)", > + BTINTEL_EFI_DRBR, PTR_ERR(dsbr)); > + dsbr = NULL; > + } > + > + if (dsbr) { > + /* bit0: 0 - Use firmware default value > + * 1 - Override firmware value > + * bit3:1 - Reserved > + * bit7:4 - DSBR override values > + * bt31:7 - Reserved > + */ > + cmd.enable = dsbr->dsbr & BIT(0); > + if (cmd.enable) > + cmd.dsbr = dsbr->dsbr >> 4 & 0xF; > + kfree(dsbr); > + } > + > + bt_dev_info(hdev, "dsbr: enabled: 0x%2.2x value: 0x%2.2x", cmd.enable, > + cmd.dsbr); > + > + skb = __hci_cmd_sync(hdev, 0xfc0a, sizeof(cmd), &cmd, HCI_CMD_TIMEOUT); > + if (IS_ERR(skb)) { > + bt_dev_err(hdev, "Failed to send Intel DSBR command (%ld)", > + PTR_ERR(skb)); > + return -bt_to_errno(PTR_ERR(skb)); > + } > + > + status = skb->data[0]; > + kfree_skb(skb); > + > + if (status) { > + bt_dev_err(hdev, "Set DSBR failed 0x%2.2x", status); > + return -bt_to_errno(status); > + } > + return 0; > +} From the comments, I gather the command to set DSBR is mandatory to be run on a given set of products, no matter if the EFI variable is set or not. So all the testing is overly complicated and clutters the code. My problem is the fact that I don’t understand the combination of enable and dsbr value in the HCI command parameter structure. It would be really beneficial if the EFI variable and the HCI command is actually documented in the code. > + > +static int btintel_apply_dsbr(struct hci_dev *hdev, > + struct intel_version_tlv *ver) > +{ > + /* For BlazarI + B0 step, DSBR command needs to be sent just after > + * downloading IML firmware > + */ > + return ver->img_type == BTINTEL_IMG_IML && > + ((ver->cnvi_top & 0xfff) == BTINTEL_CNVI_BLAZARI) && > + INTEL_CNVX_TOP_STEP(ver->cnvi_top) == 0x01; > +} > + > int btintel_bootloader_setup_tlv(struct hci_dev *hdev, > struct intel_version_tlv *ver) > { > @@ -2649,6 +2759,13 @@ int btintel_bootloader_setup_tlv(struct hci_dev *hdev, > if (err) > return err; > > + if (btintel_apply_dsbr(hdev, ver)) { > + /* set drive strength BRI response */ > + err = btintel_set_dsbr(hdev, ver); > + if (err) > + return err; > + } > + Here I would really just call the command: err = btintel_set_dsbr(hdev); And then let the command figure out what value needs to be read and if it is applicable to the hardware. The function btintel_apply_dsbr() is not acceptable. That will get out of control with the next hardware release. It is already for this hardware version unreadable. In addition the wording "drive strength of BRI” make little sense to me. I don’t know what “drive strength” actually means. When I google it then it talks about current and values in mA. So I really echo Pauls comments here. Please add documentation and comments in the code. Regards Marcel
Hi Marcel, Thanks for your comments. >-----Original Message----- >From: Marcel Holtmann <marcel@holtmann.org> >Sent: Friday, July 5, 2024 7:24 PM >To: K, Kiran <kiran.k@intel.com> >Cc: BlueZ <linux-bluetooth@vger.kernel.org>; Srivatsa, Ravishankar ><ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan ><chethan.tumkur.narayan@intel.com>; Devegowda, Chandrashekar ><chandrashekar.devegowda@intel.com>; Satija, Vijay <vijay.satija@intel.com> >Subject: Re: [PATCH v3] Bluetooth: btintel: Allow lowering of drive strength of >BRI > >Hi Kiran, > >> BRI (Bluetooth Radio Interface) traffic from CNVr to CNVi was found >> causing cross talk step errors to WiFi. As a workaround, driver needs >> to reduce the drive strength of BRI. During *setup*, driver reads the >> drive strength value from efi variable and passes it to the controller >> via vendor specific command with opcode 0xfc0a. >> >> dmesg: >> ..... >> [16.767459] Bluetooth: hci0: Found device firmware: >> intel/ibt-0190-0291-iml.sfi [16.767464] Bluetooth: hci0: Boot Address: >> 0x30099000 [16.767464] Bluetooth: hci0: Firmware Version: 9-25.24 >> [16.825418] Bluetooth: hci0: Waiting for firmware download to complete >> [16.825421] Bluetooth: hci0: Firmware loaded in 56600 usecs >> [16.825463] Bluetooth: hci0: Waiting for device to boot [16.827510] >> Bluetooth: hci0: Malformed MSFT vendor event: 0x02 [16.827529] >> Bluetooth: hci0: Device booted in 2053 usecs [16.827707] Bluetooth: >> hci0: dsbr: enabled: 0x01 value: 0x0f [16.830179] Bluetooth: hci0: >> Found device firmware: intel/ibt-0190-0291-pci.sfi [16.830188] >> Bluetooth: hci0: Boot Address: 0x10000800 [16.830189] Bluetooth: hci0: >> Firmware Version: 9-25.24 [16.928308] Bluetooth: BNEP (Ethernet >> Emulation) ver 1.3 [16.928311] Bluetooth: BNEP filters: protocol >> multicast [16.928315] Bluetooth: BNEP socket layer initialized >> [17.333292] Bluetooth: hci0: Waiting for firmware download to complete >> [17.333313] Bluetooth: hci0: Firmware loaded in 491339 usecs >> [17.333353] Bluetooth: hci0: Waiting for device to boot [17.368741] >> Bluetooth: hci0: Device booted in 34585 usecs [17.368742] Bluetooth: >> hci0: Malformed MSFT vendor event: 0x02 [17.368942] Bluetooth: hci0: >> Found Intel DDC parameters: intel/ibt-0190-0291-pci.ddc [17.369199] >> Bluetooth: hci0: Applying Intel DDC parameters completed [17.369447] >> Bluetooth: hci0: Firmware timestamp 2024.25 buildtype 3 build 64777 >> [17.369448] Bluetooth: hci0: Firmware SHA1: 0xc33eb15f [17.369648] >> Bluetooth: hci0: Fseq status: Success (0x00) [17.369649] Bluetooth: >> hci0: Fseq executed: 00.00.04.183 [17.369650] Bluetooth: hci0: Fseq BT >> Top: 00.00.04.183 [17.408366] Bluetooth: MGMT ver 1.23 [17.408415] >> Bluetooth: ISO socket layer initialized [17.434375] Bluetooth: RFCOMM >> TTY layer initialized [17.434385] Bluetooth: RFCOMM socket layer >> initialized [17.434389] Bluetooth: RFCOMM ver 1.11 >> >> Signed-off-by: Kiran K <kiran.k@intel.com> >> --- >> drivers/bluetooth/btintel.c | 117 ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 117 insertions(+) >> >> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c >> index 5d735391545a..fb9d4221ccd6 100644 >> --- a/drivers/bluetooth/btintel.c >> +++ b/drivers/bluetooth/btintel.c >> @@ -12,6 +12,8 @@ >> #include <linux/acpi.h> >> #include <acpi/acpi_bus.h> >> #include <asm/unaligned.h> >> +#include <linux/efi.h> >> + >> > >introducing extra empty line is not acceptable. Please carefully review patches >before sending them to the mailing list. These things are easily caught by just >looking at the patch. Ack. > >> #include <net/bluetooth/bluetooth.h> >> #include <net/bluetooth/hci_core.h> >> @@ -26,6 +28,8 @@ >> #define ECDSA_OFFSET 644 >> #define ECDSA_HEADER_LEN 320 >> >> +#define BTINTEL_EFI_DRBR L"UefiCnvCommonDSBR" >> + > >I don’t get the naming. Is it DRBR or DSBR? It's DSBR. I missed this typo error. I will fix in the next patch. > >> enum { >> DSM_SET_WDISABLE2_DELAY = 1, >> DSM_SET_RESET_METHOD = 3, >> @@ -49,6 +53,38 @@ static const guid_t btintel_guid_dsm = >> GUID_INIT(0xaa10f4e0, 0x81ac, 0x4233, 0xab, 0xf6, 0x3b, 0x2a, 0xc5, >> 0x0e, 0x28, 0xd9); >> >> +static void *btintel_uefi_get_variable(efi_char16_t *name, efi_guid_t >> +*guid) { void *data; efi_status_t status; unsigned long data_size >> += 0; >> + >> + if (!IS_ENABLED(CONFIG_EFI)) >> + return ERR_PTR(-EOPNOTSUPP); >> + >> + if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) >> + return ERR_PTR(-EOPNOTSUPP); >> + >> + status = efi.get_variable(name, guid, NULL, &data_size, NULL); >> + >> + if (status != EFI_BUFFER_TOO_SMALL || !data_size) return >> + ERR_PTR(-EIO); >> + >> + data = kmalloc(data_size, GFP_KERNEL); >> + >> + if (!data) >> + return ERR_PTR(-ENOMEM); >> + >> + status = efi.get_variable(name, guid, NULL, &data_size, data); >> + >> + if (status != EFI_SUCCESS) { >> + kfree(data); >> + return ERR_PTR(-ENXIO); >> + } >> + >> + return data; >> +} >> + > >This is something I don’t like at all. You are reading a single EFI variable, but >putting an abstraction in place as if we would be reading at least a dozen of >them. Please don’t do that since they only thing you are doing in the end is >hiding compiler warnings by casting everything to void and making the code a >memory allocation hard to follow. > >This could be just like this: > > static u32 btintel_uefi_get_dsbr(void). > >If at any point in the future more abstraction is needed, this is internal code >and changes are easy. No ABI breakage will happen. For now keep it simple. There are few more features I am working on which involves reading configuration data from UEFI variables. Hence abstraction was introduced. I will keep it simple now as suggested. > >> int btintel_check_bdaddr(struct hci_dev *hdev) { struct >> hci_rp_read_bd_addr *bda; @@ -2615,6 +2651,80 @@ static u8 >> btintel_classify_pkt_type(struct hci_dev *hdev, struct sk_buff *skb) >> return hci_skb_pkt_type(skb); } >> >> +static int btintel_set_dsbr(struct hci_dev *hdev, struct >> +intel_version_tlv *ver) { struct btintel_dsbr_cmd { >> + u8 enable; >> + u8 dsbr; >> + } __packed; >> + >> + struct btintel_dsbr { >> + u8 header; >> + u32 dsbr; >> + } __packed; >> + >> + struct btintel_dsbr *dsbr; >> + struct btintel_dsbr_cmd cmd; >> + struct sk_buff *skb; >> + u8 status; >> + efi_guid_t guid = EFI_GUID(0xe65d8884, 0xd4af, 0x4b20, 0x8d, 0x03, >> + 0x77, 0x2e, 0xcc, 0x3d, 0xa5, 0x31); >> + >> + memset(&cmd, 0, sizeof(cmd)); >> + dsbr = btintel_uefi_get_variable(BTINTEL_EFI_DRBR, &guid); if >> + (IS_ERR(dsbr)) { >> + /* If efi variable is not present, driver still needs to send >> + * 0xfc0a command with default values */ bt_dev_dbg(hdev, "Error >> + reading efi: %ls DSBR (%ld)", >> + BTINTEL_EFI_DRBR, PTR_ERR(dsbr)); >> + dsbr = NULL; >> + } >> + >> + if (dsbr) { >> + /* bit0: 0 - Use firmware default value >> + * 1 - Override firmware value >> + * bit3:1 - Reserved >> + * bit7:4 - DSBR override values >> + * bt31:7 - Reserved >> + */ >> + cmd.enable = dsbr->dsbr & BIT(0); >> + if (cmd.enable) >> + cmd.dsbr = dsbr->dsbr >> 4 & 0xF; >> + kfree(dsbr); >> + } >> + >> + bt_dev_info(hdev, "dsbr: enabled: 0x%2.2x value: 0x%2.2x", cmd.enable, >> + cmd.dsbr); >> + >> + skb = __hci_cmd_sync(hdev, 0xfc0a, sizeof(cmd), &cmd, >> + HCI_CMD_TIMEOUT); if (IS_ERR(skb)) { bt_dev_err(hdev, "Failed to >> + send Intel DSBR command (%ld)", >> + PTR_ERR(skb)); >> + return -bt_to_errno(PTR_ERR(skb)); >> + } >> + >> + status = skb->data[0]; >> + kfree_skb(skb); >> + >> + if (status) { >> + bt_dev_err(hdev, "Set DSBR failed 0x%2.2x", status); return >> +-bt_to_errno(status); } return 0; } > >From the comments, I gather the command to set DSBR is mandatory to be run >on a given set of products, no matter if the EFI variable is set or not. So all the >testing is overly complicated and clutters the code. > >My problem is the fact that I don’t understand the combination of enable and >dsbr value in the HCI command parameter structure. It would be really >beneficial if the EFI variable and the HCI command is actually documented in >the code. Ack. I will add more information about the command and parameters. > >> + >> +static int btintel_apply_dsbr(struct hci_dev *hdev, >> + struct intel_version_tlv *ver) >> +{ >> + /* For BlazarI + B0 step, DSBR command needs to be sent just after >> + * downloading IML firmware >> + */ >> + return ver->img_type == BTINTEL_IMG_IML && ((ver->cnvi_top & 0xfff) >> +== BTINTEL_CNVI_BLAZARI) && >> + INTEL_CNVX_TOP_STEP(ver->cnvi_top) == 0x01; } >> + >> int btintel_bootloader_setup_tlv(struct hci_dev *hdev, struct >> intel_version_tlv *ver) { @@ -2649,6 +2759,13 @@ int >> btintel_bootloader_setup_tlv(struct hci_dev *hdev, if (err) return >> err; >> >> + if (btintel_apply_dsbr(hdev, ver)) { >> + /* set drive strength BRI response */ err = btintel_set_dsbr(hdev, >> + ver); if (err) return err; } >> + > >Here I would really just call the command: > > err = btintel_set_dsbr(hdev); > >And then let the command figure out what value needs to be read and if it is >applicable to the hardware. > >The function btintel_apply_dsbr() is not acceptable. That will get out of control >with the next hardware release. It is already for this hardware version >unreadable. Ack. I will have the logic built into "btintel_set_dsbr". > >In addition the wording "drive strength of BRI” make little sense to me. I don’t >know what “drive strength” actually means. When I google it then it talks >about current and values in mA. So I really echo Pauls comments here. Please >add documentation and comments in the code. Ack. > >Regards > >Marcel Thanks, Kiran
diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c index 5d735391545a..fb9d4221ccd6 100644 --- a/drivers/bluetooth/btintel.c +++ b/drivers/bluetooth/btintel.c @@ -12,6 +12,8 @@ #include <linux/acpi.h> #include <acpi/acpi_bus.h> #include <asm/unaligned.h> +#include <linux/efi.h> + #include <net/bluetooth/bluetooth.h> #include <net/bluetooth/hci_core.h> @@ -26,6 +28,8 @@ #define ECDSA_OFFSET 644 #define ECDSA_HEADER_LEN 320 +#define BTINTEL_EFI_DRBR L"UefiCnvCommonDSBR" + enum { DSM_SET_WDISABLE2_DELAY = 1, DSM_SET_RESET_METHOD = 3, @@ -49,6 +53,38 @@ static const guid_t btintel_guid_dsm = GUID_INIT(0xaa10f4e0, 0x81ac, 0x4233, 0xab, 0xf6, 0x3b, 0x2a, 0xc5, 0x0e, 0x28, 0xd9); +static void *btintel_uefi_get_variable(efi_char16_t *name, efi_guid_t *guid) +{ + void *data; + efi_status_t status; + unsigned long data_size = 0; + + if (!IS_ENABLED(CONFIG_EFI)) + return ERR_PTR(-EOPNOTSUPP); + + if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) + return ERR_PTR(-EOPNOTSUPP); + + status = efi.get_variable(name, guid, NULL, &data_size, NULL); + + if (status != EFI_BUFFER_TOO_SMALL || !data_size) + return ERR_PTR(-EIO); + + data = kmalloc(data_size, GFP_KERNEL); + + if (!data) + return ERR_PTR(-ENOMEM); + + status = efi.get_variable(name, guid, NULL, &data_size, data); + + if (status != EFI_SUCCESS) { + kfree(data); + return ERR_PTR(-ENXIO); + } + + return data; +} + int btintel_check_bdaddr(struct hci_dev *hdev) { struct hci_rp_read_bd_addr *bda; @@ -2615,6 +2651,80 @@ static u8 btintel_classify_pkt_type(struct hci_dev *hdev, struct sk_buff *skb) return hci_skb_pkt_type(skb); } +static int btintel_set_dsbr(struct hci_dev *hdev, struct intel_version_tlv *ver) +{ + struct btintel_dsbr_cmd { + u8 enable; + u8 dsbr; + } __packed; + + struct btintel_dsbr { + u8 header; + u32 dsbr; + } __packed; + + struct btintel_dsbr *dsbr; + struct btintel_dsbr_cmd cmd; + struct sk_buff *skb; + u8 status; + efi_guid_t guid = EFI_GUID(0xe65d8884, 0xd4af, 0x4b20, 0x8d, 0x03, + 0x77, 0x2e, 0xcc, 0x3d, 0xa5, 0x31); + + memset(&cmd, 0, sizeof(cmd)); + dsbr = btintel_uefi_get_variable(BTINTEL_EFI_DRBR, &guid); + if (IS_ERR(dsbr)) { + /* If efi variable is not present, driver still needs to send + * 0xfc0a command with default values + */ + bt_dev_dbg(hdev, "Error reading efi: %ls DSBR (%ld)", + BTINTEL_EFI_DRBR, PTR_ERR(dsbr)); + dsbr = NULL; + } + + if (dsbr) { + /* bit0: 0 - Use firmware default value + * 1 - Override firmware value + * bit3:1 - Reserved + * bit7:4 - DSBR override values + * bt31:7 - Reserved + */ + cmd.enable = dsbr->dsbr & BIT(0); + if (cmd.enable) + cmd.dsbr = dsbr->dsbr >> 4 & 0xF; + kfree(dsbr); + } + + bt_dev_info(hdev, "dsbr: enabled: 0x%2.2x value: 0x%2.2x", cmd.enable, + cmd.dsbr); + + skb = __hci_cmd_sync(hdev, 0xfc0a, sizeof(cmd), &cmd, HCI_CMD_TIMEOUT); + if (IS_ERR(skb)) { + bt_dev_err(hdev, "Failed to send Intel DSBR command (%ld)", + PTR_ERR(skb)); + return -bt_to_errno(PTR_ERR(skb)); + } + + status = skb->data[0]; + kfree_skb(skb); + + if (status) { + bt_dev_err(hdev, "Set DSBR failed 0x%2.2x", status); + return -bt_to_errno(status); + } + return 0; +} + +static int btintel_apply_dsbr(struct hci_dev *hdev, + struct intel_version_tlv *ver) +{ + /* For BlazarI + B0 step, DSBR command needs to be sent just after + * downloading IML firmware + */ + return ver->img_type == BTINTEL_IMG_IML && + ((ver->cnvi_top & 0xfff) == BTINTEL_CNVI_BLAZARI) && + INTEL_CNVX_TOP_STEP(ver->cnvi_top) == 0x01; +} + int btintel_bootloader_setup_tlv(struct hci_dev *hdev, struct intel_version_tlv *ver) { @@ -2649,6 +2759,13 @@ int btintel_bootloader_setup_tlv(struct hci_dev *hdev, if (err) return err; + if (btintel_apply_dsbr(hdev, ver)) { + /* set drive strength BRI response */ + err = btintel_set_dsbr(hdev, ver); + if (err) + return err; + } + /* If image type returned is BTINTEL_IMG_IML, then controller supports * intermediae loader image */
BRI (Bluetooth Radio Interface) traffic from CNVr to CNVi was found causing cross talk step errors to WiFi. As a workaround, driver needs to reduce the drive strength of BRI. During *setup*, driver reads the drive strength value from efi variable and passes it to the controller via vendor specific command with opcode 0xfc0a. dmesg: ..... [16.767459] Bluetooth: hci0: Found device firmware: intel/ibt-0190-0291-iml.sfi [16.767464] Bluetooth: hci0: Boot Address: 0x30099000 [16.767464] Bluetooth: hci0: Firmware Version: 9-25.24 [16.825418] Bluetooth: hci0: Waiting for firmware download to complete [16.825421] Bluetooth: hci0: Firmware loaded in 56600 usecs [16.825463] Bluetooth: hci0: Waiting for device to boot [16.827510] Bluetooth: hci0: Malformed MSFT vendor event: 0x02 [16.827529] Bluetooth: hci0: Device booted in 2053 usecs [16.827707] Bluetooth: hci0: dsbr: enabled: 0x01 value: 0x0f [16.830179] Bluetooth: hci0: Found device firmware: intel/ibt-0190-0291-pci.sfi [16.830188] Bluetooth: hci0: Boot Address: 0x10000800 [16.830189] Bluetooth: hci0: Firmware Version: 9-25.24 [16.928308] Bluetooth: BNEP (Ethernet Emulation) ver 1.3 [16.928311] Bluetooth: BNEP filters: protocol multicast [16.928315] Bluetooth: BNEP socket layer initialized [17.333292] Bluetooth: hci0: Waiting for firmware download to complete [17.333313] Bluetooth: hci0: Firmware loaded in 491339 usecs [17.333353] Bluetooth: hci0: Waiting for device to boot [17.368741] Bluetooth: hci0: Device booted in 34585 usecs [17.368742] Bluetooth: hci0: Malformed MSFT vendor event: 0x02 [17.368942] Bluetooth: hci0: Found Intel DDC parameters: intel/ibt-0190-0291-pci.ddc [17.369199] Bluetooth: hci0: Applying Intel DDC parameters completed [17.369447] Bluetooth: hci0: Firmware timestamp 2024.25 buildtype 3 build 64777 [17.369448] Bluetooth: hci0: Firmware SHA1: 0xc33eb15f [17.369648] Bluetooth: hci0: Fseq status: Success (0x00) [17.369649] Bluetooth: hci0: Fseq executed: 00.00.04.183 [17.369650] Bluetooth: hci0: Fseq BT Top: 00.00.04.183 [17.408366] Bluetooth: MGMT ver 1.23 [17.408415] Bluetooth: ISO socket layer initialized [17.434375] Bluetooth: RFCOMM TTY layer initialized [17.434385] Bluetooth: RFCOMM socket layer initialized [17.434389] Bluetooth: RFCOMM ver 1.11 Signed-off-by: Kiran K <kiran.k@intel.com> --- drivers/bluetooth/btintel.c | 117 ++++++++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+)