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 <tedd.an@xxxxxxxxx>
>>> ---
>>> 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 <linux/module.h>
>>> #include <linux/usb.h>
>>> +#include <linux/firmware.h>
>>>
>>> #include <net/bluetooth/bluetooth.h>
>>> #include <net/bluetooth/hci_core.h>
>>> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
