Return-Path: Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 6.3 \(1503\)) Subject: Re: [PATCH v4] Bluetooth: Add support for Intel Bluetooth device [8087:07dc] From: Marcel Holtmann In-Reply-To: <1466472.WPu0Nr1Sby@tedd-ubuntu> Date: Thu, 18 Apr 2013 09:23:45 -0700 Cc: Gustavo Padovan , linux-bluetooth@vger.kernel.org, johan.hedberg@intel.com, don.fry@intel.com Message-Id: References: <3638763.erUlVvs0at@tedd-ubuntu> <20130418041217.GA28913@joana> <1466472.WPu0Nr1Sby@tedd-ubuntu> To: Tedd Ho-Jeong An Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Tedd, >>> This patch adds support for Intel Bluetooth device by adding >>> btusb_setup_intel() routine that update the device with ROM patch. >>> >>> T: Bus=02 Lev=02 Prnt=02 Port=00 Cnt=01 Dev#= 4 Spd=12 MxCh= 0 >>> D: Ver= 2.00 Cls=e0(wlcon) Sub=01 Prot=01 MxPS=64 #Cfgs= 1 >>> P: Vendor=8087 ProdID=07dc Rev= 0.01 >>> C:* #Ifs= 2 Cfg#= 1 Atr=e0 MxPwr=100mA >>> I:* If#= 0 Alt= 0 #EPs= 3 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb >>> E: Ad=81(I) Atr=03(Int.) MxPS= 64 Ivl=1ms >>> E: Ad=02(O) Atr=02(Bulk) MxPS= 64 Ivl=0ms >>> E: Ad=82(I) Atr=02(Bulk) MxPS= 64 Ivl=0ms >>> I:* If#= 1 Alt= 0 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb >>> E: Ad=03(O) Atr=01(Isoc) MxPS= 0 Ivl=1ms >>> E: Ad=83(I) Atr=01(Isoc) MxPS= 0 Ivl=1ms >>> I: If#= 1 Alt= 1 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb >>> E: Ad=03(O) Atr=01(Isoc) MxPS= 9 Ivl=1ms >>> E: Ad=83(I) Atr=01(Isoc) MxPS= 9 Ivl=1ms >>> I: If#= 1 Alt= 2 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb >>> E: Ad=03(O) Atr=01(Isoc) MxPS= 17 Ivl=1ms >>> E: Ad=83(I) Atr=01(Isoc) MxPS= 17 Ivl=1ms >>> I: If#= 1 Alt= 3 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb >>> E: Ad=03(O) Atr=01(Isoc) MxPS= 25 Ivl=1ms >>> E: Ad=83(I) Atr=01(Isoc) MxPS= 25 Ivl=1ms >>> I: If#= 1 Alt= 4 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb >>> E: Ad=03(O) Atr=01(Isoc) MxPS= 33 Ivl=1ms >>> E: Ad=83(I) Atr=01(Isoc) MxPS= 33 Ivl=1ms >>> I: If#= 1 Alt= 5 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb >>> E: Ad=03(O) Atr=01(Isoc) MxPS= 49 Ivl=1ms >>> E: Ad=83(I) Atr=01(Isoc) MxPS= 49 Ivl=1ms >>> >>> Signed-off-by: Tedd Ho-Jeong An >>> --- >>> v4 changes >>> - refactored patching routin to separate function >>> - changed varaible type for status to u8 >>> - corrected minor format issues >>> >>> drivers/bluetooth/btusb.c | 366 +++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 366 insertions(+) >>> >>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c >>> index 3d684d2..52c0ec0 100644 >>> --- a/drivers/bluetooth/btusb.c >>> +++ b/drivers/bluetooth/btusb.c >>> @@ -23,6 +23,7 @@ >>> >>> #include >>> #include >>> +#include >>> >>> #include >>> #include >>> @@ -47,6 +48,7 @@ static struct usb_driver btusb_driver; >>> #define BTUSB_BROKEN_ISOC 0x20 >>> #define BTUSB_WRONG_SCO_MTU 0x40 >>> #define BTUSB_ATH3012 0x80 >>> +#define BTUSB_INTEL 0x100 >>> >>> static struct usb_device_id btusb_table[] = { >>> /* Generic Bluetooth USB device */ >>> @@ -207,6 +209,9 @@ static struct usb_device_id blacklist_table[] = { >>> /* Frontline ComProbe Bluetooth Sniffer */ >>> { USB_DEVICE(0x16d3, 0x0002), .driver_info = BTUSB_SNIFFER }, >>> >>> + /* Intel Bluetooth device */ >>> + { USB_DEVICE(0x8087, 0x07dc), .driver_info = BTUSB_INTEL }, >>> + >>> { } /* Terminating entry */ >>> }; >>> >>> @@ -943,6 +948,364 @@ static int btusb_setup_bcm92035(struct hci_dev *hdev) >>> return 0; >>> } >>> >>> +struct intel_version { >>> + u8 status; >>> + u8 hw_platform; >>> + u8 hw_variant; >>> + u8 hw_revision; >>> + u8 fw_variant; >>> + u8 fw_revision; >>> + u8 fw_build_num; >>> + u8 fw_build_ww; >>> + u8 fw_build_yy; >>> + u8 fw_patch_num; >>> +} __packed; >>> + >>> +static const struct firmware *btusb_setup_intel_get_fw(struct hci_dev *hdev, >>> + struct intel_version *ver) >>> +{ >>> + const struct firmware *fw; >>> + char fwname[64]; >>> + int ret; >>> + >>> + snprintf(fwname, sizeof(fwname), >>> + "intel/ibt-hw-%x.%x.%x-fw-%x.%x.%x.%x.%x.bseq", >>> + ver->hw_platform, ver->hw_variant, ver->hw_revision, >>> + ver->fw_variant, ver->fw_revision, ver->fw_build_num, >>> + ver->fw_build_ww, ver->fw_build_yy); >>> + >>> + ret = request_firmware(&fw, fwname, &hdev->dev); >>> + if (ret < 0) { >>> + if (ret == -EINVAL) { >>> + BT_ERR("%s Intel firmware file request failed (%d)", >>> + hdev->name, ret); >>> + return NULL; >>> + } >>> + >>> + BT_ERR("%s failed to open Intel firmware file: %s(%d)", >>> + hdev->name, fwname, ret); >>> + >>> + snprintf(fwname, sizeof(fwname), "intel/ibt-default-%x.%x.bseq", >>> + ver->hw_platform, ver->hw_variant); >>> + if (request_firmware(&fw, fwname, &hdev->dev) < 0) { >>> + BT_ERR("%s failed to open default Intel fw file: %s", >>> + hdev->name, fwname); >>> + return NULL; >>> + } >>> + } >>> + >>> + BT_INFO("%s Intel Bluetooth firmware file: %s", hdev->name, fwname); >>> + >>> + return fw; >>> +} >>> + >>> +static int btusb_setup_intel_patching(struct hci_dev *hdev, >>> + const struct firmware *fw, >>> + const u8 **curr_fw_ptr, >>> + int *disable_patch) >>> +{ >>> + struct sk_buff *skb; >>> + struct hci_command_hdr *cmd; >>> + const u8 *cmd_param; >>> + struct hci_event_hdr *evt = NULL; >>> + const u8 *evt_param = NULL; >>> + const u8 *fw_ptr = *curr_fw_ptr; >> >> Why do you need an extra var to keep fw_ptr? can't you just use curr_fw_ptr >> directly? > > Yes. I can. I will make a change. > >> >>> + int remain = fw->size - (fw_ptr - fw->data); >>> + >>> + /* Read command */ >>> + if (remain > HCI_COMMAND_HDR_SIZE && >>> + fw_ptr[0] != 0x01) { >> >> Apparently you don't need to break line on the if. > > ACK. > >> >>> + BT_ERR("%s Intel fw corrupted: invalid cmd read", hdev->name); >>> + return -1; >> >> Please return a proper error code. > > ACK. > >> >>> + } >>> + fw_ptr++; >>> + remain--; >>> + >>> + cmd = (struct hci_command_hdr *)fw_ptr; >>> + fw_ptr += sizeof(*cmd); >>> + remain -= sizeof(*cmd); >>> + >>> + /* checking the length */ >>> + if (remain < cmd->plen) { >>> + BT_ERR("%s Intel fw corrupted: invalid cmd len", hdev->name); >>> + return -1; >> >> Same here and all other places you return -1 > > ACK. > >> >>> + } >>> + >>> + /* If there is a command that loads a patch in the firmware >>> + * file, then enable the patch upon success, otherwise just >>> + * disable the manufacturer mode, for example patch activation >>> + * is not required when the default firmware patch file is used >>> + * because there are no patch data to load. >>> + */ >>> + if (*disable_patch && cmd->opcode == 0xfc8e) >>> + *disable_patch = 0; >>> + >>> + cmd_param = fw_ptr; >>> + fw_ptr += cmd->plen; >>> + remain -= cmd->plen; >>> + >>> + /* Read event */ >>> + while (remain > HCI_EVENT_HDR_SIZE && fw_ptr[0] == 0x02) { >>> + fw_ptr++; >>> + remain--; >>> + >>> + evt = (struct hci_event_hdr *)fw_ptr; >>> + fw_ptr += sizeof(*evt); >>> + remain -= sizeof(*evt); >>> + >>> + if (remain < evt->plen) { >>> + BT_ERR("%s Intel fw corrupted: invalid evt len", >>> + hdev->name); >>> + return -1; >>> + } >>> + >>> + evt_param = fw_ptr; >>> + fw_ptr += evt->plen; >>> + remain -= evt->plen; >>> + } >>> + >>> + if (!evt || !evt_param || remain < 0) { >>> + BT_ERR("%s Intel fw corrupted: invalid evt read", hdev->name); >>> + return -1; >>> + } >>> + >>> + skb = __hci_cmd_sync_ev(hdev, le16_to_cpu(cmd->opcode), >>> + cmd->plen, (void *)cmd_param, evt->evt, >>> + HCI_INIT_TIMEOUT); >>> + if (IS_ERR(skb)) { >>> + BT_ERR("%s sending Intel patch command failed (%ld)", >>> + hdev->name, PTR_ERR(skb)); >>> + return -1; >>> + } >>> + >>> + /* Checking the returned event */ >>> + if (skb->len != evt->plen) { >>> + BT_ERR("%s mismatch event length", hdev->name); >>> + kfree_skb(skb); >>> + return -1; >>> + } >>> + >>> + if (memcmp(skb->data, evt_param, evt->plen)) { >>> + BT_ERR("%s mismatch event parameter", hdev->name); >>> + kfree_skb(skb); >>> + return -1; >>> + } >>> + kfree_skb(skb); >>> + >>> + *curr_fw_ptr = fw_ptr; >>> + >>> + return 0; >>> +} >>> + >>> +static int btusb_setup_intel(struct hci_dev *hdev) >>> +{ >>> + struct sk_buff *skb; >>> + const struct firmware *fw; >>> + const u8 *fw_ptr; >>> + int disable_patch; >>> + struct intel_version *ver; >>> + >>> + u8 mfg_enable[] = { 0x01, 0x00 }; >>> + u8 mfg_disable[] = { 0x00, 0x00 }; >>> + u8 mfg_reset_deactivate[] = { 0x00, 0x01 }; >>> + u8 mfg_reset_activate[] = { 0x00, 0x02 }; >>> + >>> + BT_DBG("%s", hdev->name); >>> + >>> + /* The controller has a bug with the first HCI command send to it >> >> s/send/sent/ > > ACK. > >> >>> + * returning number of completed commands as zero. This would stall the >>> + * command processing in the Bluetooth core >>> + * >>> + * As a workaround, send HCI Reset command first which will reset the >>> + * number of completed commands and allow normal command processing >>> + * from now on >>> + */ >>> + skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT); >>> + if (IS_ERR(skb)) { >>> + BT_ERR("%s sending initial HCI reset command failed (%ld)", >>> + hdev->name, PTR_ERR(skb)); >>> + return -PTR_ERR(skb); >>> + } >>> + kfree_skb(skb); >>> + >>> + /* Read Intel specific controller version first to allow selection of >>> + * which firmware file to load. >>> + * >>> + * The returned information are hardware variant and revision plus >>> + * firmware variant, revision and build number. >>> + */ >>> + skb = __hci_cmd_sync(hdev, 0xfc05, 0, NULL, HCI_INIT_TIMEOUT); >> >> Could you turn 0xfc05 into a macro that we can actually understand > > My older version of patch used macro but I changed it later due to a comments from others. > I will make a change back to use macro. I think that will make the code cleaner. please leave this as 0xfc05. The comment above is enough. That is why I asked for the comment to put above to explain what this command does. There is no real point in creating a macro for this. We will not be releasing the vendor command specification any time soon. So even if you call this HCI_INTEL_FOO it has no extra meaning to anybody reading this. The comment above is way more helpful for anybody who has to review this code then a made up macro. And in this specific cases, the macro forces a line split and makes the code even less readable without adding any extra value for the reader. Gustavo, feel free to check my earlier reviews. Regards Marcel