Hi Tedd,
* Tedd Ho-Jeong An <tedd.an@xxxxxxxxx> [2013-04-17 11:47:17 -0700]:
> From: Tedd Ho-Jeong An <tedd.an@xxxxxxxxx>
>
> 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?
> + 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.
> + BT_ERR("%s Intel fw corrupted: invalid cmd read", hdev->name);
> + return -1;
Please return a proper error code.
> + }
> + 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
> + }
> +
> + /* 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/
> + * 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
> + if (IS_ERR(skb)) {
> + BT_ERR("%s reading Intel fw version command failed (%ld)",
> + hdev->name, PTR_ERR(skb));
> + return -PTR_ERR(skb);
> + }
> +
> + if (skb->len != sizeof(*ver)) {
> + BT_ERR("%s Intel version event length mismatch", hdev->name);
> + kfree_skb(skb);
> + return 0;
So why you return 0 here?
> + }
> +
> + ver = (struct intel_version *)skb->data;
Is the cast really necessary?
> + /* was command successful */
I would just drop this comment. The 'if' below already tell you that.
> + if (ver->status) {
> + BT_ERR("%s Intel fw version event failed (%02x)", hdev->name,
> + ver->status);
> + kfree_skb(skb);
> + return 0;
Here again. Why return 0? Check other places in the function.
> + }
> +
> + BT_INFO("%s read Intel version: %02x%02x%02x%02x%02x%02x%02x%02x%02x",
Add a ":" after the %s for hdev->name
> + hdev->name, 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,
> + ver->fw_patch_num);
> +
> + /* is the device already patched */
> + if (ver->fw_patch_num) {
> + BT_INFO("%s Intel device is already patched. patch num: %02x",
> + hdev->name, ver->fw_patch_num);
> + kfree_skb(skb);
> + return 0;
> + }
> +
> + /* Opens the firmware patch file based on the firmware version read
> + * from the controller. If it failed to open the matching firmware
> + * patch file, it tries to open the default firmware patch file.
> + * If no patch file, allow the device to operate without a patch.
> + */
> + fw = btusb_setup_intel_get_fw(hdev, ver);
> + if (!fw) {
> + kfree_skb(skb);
> + return 0;
> + }
> + fw_ptr = fw->data;
> +
> + /* This Intel specific command enables the manufacturer mode of the
> + * controller.
> + *
> + * Only while this mode is enabled, the driver can download the
> + * firmware patch data and configuration parameters.
> + */
> + skb = __hci_cmd_sync(hdev, 0xfc11, 2, mfg_enable, HCI_INIT_TIMEOUT);
> + if (IS_ERR(skb)) {
> + BT_ERR("%s entering Intel manufacturer mode failed (%ld)",
> + hdev->name, PTR_ERR(skb));
Same here ":" after %s.
> + release_firmware(fw);
> + return -PTR_ERR(skb);
> + }
> +
> + if (skb->data[0]) {
> + u8 evt_status = skb->data[0];
> + BT_ERR("%s enable Intel manufacturer mode event failed (%02x)",
> + hdev->name, evt_status);
> + kfree_skb(skb);
> + release_firmware(fw);
> + return -bt_to_errno(evt_status);
> + }
> + kfree_skb(skb);
> +
> + disable_patch = 1;
> +
> + /* The firmware data file consists of list of Intel specific HCI
> + * commands and its expected events. The first byte indicates the
> + * type of the message, either HCI command or HCI event.
> + *
> + * It reads the command and its expected event from the firmware file,
> + * and send to the controller. Once __hci_cmd_sync_ev() returns,
> + * the returned event is compared with the event read from the firmware
> + * file and it will continue until all the messages are downloaded to
> + * the controller.
> + *
> + * Once the firmware patching is completed successfully,
> + * the manufacturer mode is disabled with reset and activating the
> + * downloaded patch.
> + *
> + * If the firmware patching is failed, the manufacturer mode is
s/is failed/fails/
> + * disabled with reset and deactivating the patch.
> + *
> + * If the default patch file is used, no reset is done when disabling
> + * the manufacturer.
> + */
> + while (1) {
> + int ret;
> +
> + ret = btusb_setup_intel_patching(hdev, fw, &fw_ptr,
> + &disable_patch);
> + if (ret < 0)
> + goto exit_mfg_deactivate;
> +
> + /* Checking if EOF */
> + if (fw->size == fw_ptr - fw->data) {
> + if (disable_patch)
> + goto exit_mfg_disable;
> + else
> + goto exit_mfg_activate;
> + } else if (fw->size < fw_ptr - fw->data) {
> + BT_ERR("%s inconsistent patch read size", hdev->name);
> + goto exit_mfg_deactivate;
It seems that to make this if true 'remain' has to be < 0 in the other
function and if remain is < 0 the we actually never see this if since the
return of btusb_setup_intel_patching() is < 0 and we goto exit_mfg_deactivate
above.
Gustavo
--
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
