From: Tedd Ho-Jeong An <[email protected]>
This patch adds support for Intel Bluetooth device by adding
btusb_setup_intel() routine that update the device with ROM patch.
This is v2 after running checkpatch script with "strict" option.
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 <[email protected]>
---
drivers/bluetooth/btusb.c | 333 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 333 insertions(+)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 3d684d2..c8c816b 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,331 @@ static int btusb_setup_bcm92035(struct hci_dev *hdev)
return 0;
}
+static const struct firmware *btusb_setup_intel_get_fw(struct hci_dev *hdev,
+ struct sk_buff *skb,
+ int *use_default)
+{
+ struct intel_fw_version {
+ 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;
+
+ struct intel_fw_version *ver;
+ const struct firmware *fw;
+ char fwname[64];
+
+ if (skb->len != sizeof(*ver)) {
+ BT_ERR("%s invalid length of Intel firmware version %d",
+ hdev->name, skb->len);
+ return NULL;
+ }
+
+ ver = (void *)skb->data;
+ snprintf(fwname, sizeof(fwname),
+ "intel/ibt-hw-%x.%x.%x-fw-%x.%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, ver->fw_patch_num);
+ kfree_skb(skb);
+
+ *use_default = 0;
+
+ if (request_firmware(&fw, fwname, &hdev->dev) < 0) {
+ BT_ERR("%s failed to open Intel firmware file: %s",
+ hdev->name, fwname);
+
+ snprintf(fwname, sizeof(fwname), "intel/ibt-default-%x.bseq",
+ 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;
+ }
+
+ *use_default = 1;
+ }
+
+ BT_INFO("%s Intel Bluetooth firmware file: %s", hdev->name, fwname);
+
+ return fw;
+}
+
+#define INTEL_HCI_MFG_MODE 0xfc11
+#define INTEL_HCI_GET_VER 0xfc05
+#define INTEL_HCI_CMD 0x01
+#define INTEL_HCI_EVT 0x02
+
+static int btusb_setup_intel(struct hci_dev *hdev)
+{
+ struct sk_buff *skb;
+ const struct firmware *fw;
+ const u8 *fw_ptr;
+ int use_default;
+
+ u8 mfg_enable[] = { 0x01, 0x00 };
+ u8 mfg_no_reset[] = { 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
+ * 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, INTEL_HCI_GET_VER, 0, NULL,
+ HCI_INIT_TIMEOUT);
+ if (IS_ERR(skb)) {
+ BT_ERR("%s reading Intel fw version command failed (%ld)",
+ hdev->name, PTR_ERR(skb));
+ return -PTR_ERR(skb);
+ }
+
+ /* was command successful */
+ if (skb->data[0]) {
+ BT_ERR("%s reading Intel fw version event failed (%02x)",
+ hdev->name, skb->data[0]);
+ kfree_skb(skb);
+ return 0;
+ }
+ skb_pull(skb, 1);
+
+ /* 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, skb, &use_default);
+ if (!fw)
+ 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, INTEL_HCI_MFG_MODE, 2, mfg_enable,
+ HCI_INIT_TIMEOUT);
+ if (IS_ERR(skb)) {
+ BT_ERR("%s entering Intel manufacturer mode failed (%ld)",
+ hdev->name, PTR_ERR(skb));
+ release_firmware(fw);
+ return -PTR_ERR(skb);
+ }
+
+ if (skb->data[0]) {
+ int evt_status = skb->data[0];
+ BT_ERR("%s enable Intel manufacturer mode event failed (%d)",
+ hdev->name, evt_status);
+ kfree_skb(skb);
+ release_firmware(fw);
+ return -bt_to_errno(evt_status);
+ }
+ kfree_skb(skb);
+
+ /* 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
+ * disabled with reset and deactivating the patch.
+ *
+ * If the default patch file is used, no reset is done when disabling
+ * the manufacturer.
+ */
+ while (1) {
+ struct hci_command_hdr *cmd;
+ const u8 *cmd_param;
+ struct hci_event_hdr *evt = NULL;
+ const u8 *evt_param = NULL;
+ int remain = fw->size - (fw_ptr - fw->data);
+
+ /* Read command */
+ if (remain > HCI_COMMAND_HDR_SIZE &&
+ fw_ptr[0] != INTEL_HCI_CMD) {
+ BT_ERR("%s Intel fw corrupted: invalid cmd read",
+ hdev->name);
+ goto exit_mfg_deactivate;
+ }
+ 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);
+ goto exit_mfg_deactivate;
+ }
+
+ cmd_param = fw_ptr;
+ fw_ptr += cmd->plen;
+ remain -= cmd->plen;
+
+ /* Read event */
+ while (remain > HCI_EVENT_HDR_SIZE &&
+ fw_ptr[0] == INTEL_HCI_EVT) {
+ 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);
+ goto exit_mfg_deactivate;
+ }
+
+ 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);
+ goto exit_mfg_deactivate;
+ }
+
+ 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));
+ goto exit_mfg_deactivate;
+ }
+
+ /* Checking the returned event */
+ if (skb->len != evt->plen) {
+ BT_ERR("%s mismatch event length", hdev->name);
+ kfree_skb(skb);
+ goto exit_mfg_deactivate;
+ }
+
+ if (memcmp(skb->data, evt_param, evt->plen)) {
+ BT_ERR("%s mismatch event parameter", hdev->name);
+ kfree_skb(skb);
+ goto exit_mfg_deactivate;
+ }
+ kfree_skb(skb);
+
+ /* Checking if EOF */
+ if (fw->size == fw_ptr - fw->data) {
+ BT_DBG("%s Intel firmware patching completed",
+ hdev->name);
+
+ if (use_default)
+ goto exit_mfg_no_reset;
+ 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;
+ }
+ }
+
+exit_mfg_activate:
+ release_firmware(fw);
+
+ /* Reset is done when disabling the manufacturer mode and activate
+ * the downloaded firmware patches
+ */
+ skb = __hci_cmd_sync(hdev, INTEL_HCI_MFG_MODE, 2, mfg_reset_activate,
+ HCI_INIT_TIMEOUT);
+ if (IS_ERR(skb)) {
+ BT_ERR("%s exiting Intel manufacturer mode failed (%ld)",
+ hdev->name, PTR_ERR(skb));
+ return -PTR_ERR(skb);
+ }
+ kfree_skb(skb);
+
+ BT_INFO("%s Intel Bluetooth firmware patch completed and activated",
+ hdev->name);
+
+ return 0;
+
+exit_mfg_deactivate:
+ release_firmware(fw);
+
+ /* Reset is done when disabling the manufacturer mode and deactivate
+ * the downloaded firmware patches.
+ */
+ skb = __hci_cmd_sync(hdev, INTEL_HCI_MFG_MODE, 2, mfg_reset_deactivate,
+ HCI_INIT_TIMEOUT);
+ if (IS_ERR(skb)) {
+ BT_ERR("%s exiting Intel manufacturer mode failed (%ld)",
+ hdev->name, PTR_ERR(skb));
+ return -PTR_ERR(skb);
+ }
+ kfree_skb(skb);
+
+ BT_INFO("%s Intel Bluetooth firmware patch completed and deactivated",
+ hdev->name);
+
+ return 0;
+
+exit_mfg_no_reset:
+ release_firmware(fw);
+
+ /* No reset is done when disabling the manufacturer mode */
+ skb = __hci_cmd_sync(hdev, INTEL_HCI_MFG_MODE, 2, mfg_no_reset,
+ HCI_INIT_TIMEOUT);
+ if (IS_ERR(skb)) {
+ BT_ERR("%s exiting Intel manufacturer mode failed (%ld)",
+ hdev->name, PTR_ERR(skb));
+ return -PTR_ERR(skb);
+ }
+ kfree_skb(skb);
+
+ BT_INFO("%s Intel Bluetooth firmware patch completed", hdev->name);
+
+ return 0;
+}
+
static int btusb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
@@ -1048,6 +1378,9 @@ static int btusb_probe(struct usb_interface *intf,
if (id->driver_info & BTUSB_BCM92035)
hdev->setup = btusb_setup_bcm92035;
+ if (id->driver_info & BTUSB_INTEL)
+ hdev->setup = btusb_setup_intel;
+
/* Interface numbers are hardcoded in the specification */
data->isoc = usb_ifnum_to_if(data->udev, 1);
--
1.7.9.5
Hi Tedd,
>>> drivers/bluetooth/btusb.c | 333
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 333 insertions(+)
>>>
>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>> index 3d684d2..c8c816b 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>
>>
>> I just remembered that you most likely need also change the Kconfig to
>> depend on the firmware loader support.
>>
>> Actually you do not. You will get an -EINVAL error. Might want to print an
>> error in that case. As a general principle, it would be a good idea if btusb.ko
>> itself does not depend on the firmware loader.
>>
>
> Could you explain more about this?
> Anyway, this header file is needed to read the firmware patch file (request_firmware).
this part is a bit tricky. If you look at the BlueFritz! driver, then you see in Kconfig that it does a select FW_LOADER. So it ensures that the firmware loader support is build. Either as module or built into the kernel.
Of course that driver requires the firmware module since otherwise it won't work. When you look at the generic Bluetooth USB driver, then it can work nicely without the firmware loader. Only the Intel module would need it. And even that would work without, just don't have the right patches or configuration loaded.
Luckily the linux/firmware.h is written in a way that it works with or without the firmware loader being enabled. However in case it is not enabled request_firmware() will return -EINVAL and in that case we should print a proper error message in dmesg that makes it clear why the firmware loading has not been attempted.
Regards
Marcel
Hi Marcel.
> -----Original Message-----
> Hi Tedd,
>=20
> > This patch adds support for Intel Bluetooth device by adding
> > btusb_setup_intel() routine that update the device with ROM patch.
> >
> > This is v2 after running checkpatch script with "strict" option.
>=20
> this comment does not belong here.
>=20
> >
> > T: Bus=3D02 Lev=3D02 Prnt=3D02 Port=3D00 Cnt=3D01 Dev#=3D 4 Spd=3D12 =
MxCh=3D 0
> > D: Ver=3D 2.00 Cls=3De0(wlcon) Sub=3D01 Prot=3D01 MxPS=3D64 #Cfgs=3D =
1
> > P: Vendor=3D8087 ProdID=3D07dc Rev=3D 0.01
> > C:* #Ifs=3D 2 Cfg#=3D 1 Atr=3De0 MxPwr=3D100mA
> > I:* If#=3D 0 Alt=3D 0 #EPs=3D 3 Cls=3De0(wlcon) Sub=3D01 Prot=3D01 Driv=
er=3Dbtusb
> > E: Ad=3D81(I) Atr=3D03(Int.) MxPS=3D 64 Ivl=3D1ms
> > E: Ad=3D02(O) Atr=3D02(Bulk) MxPS=3D 64 Ivl=3D0ms
> > E: Ad=3D82(I) Atr=3D02(Bulk) MxPS=3D 64 Ivl=3D0ms
> > I:* If#=3D 1 Alt=3D 0 #EPs=3D 2 Cls=3De0(wlcon) Sub=3D01 Prot=3D01 Driv=
er=3Dbtusb
> > E: Ad=3D03(O) Atr=3D01(Isoc) MxPS=3D 0 Ivl=3D1ms
> > E: Ad=3D83(I) Atr=3D01(Isoc) MxPS=3D 0 Ivl=3D1ms
> > I: If#=3D 1 Alt=3D 1 #EPs=3D 2 Cls=3De0(wlcon) Sub=3D01 Prot=3D01 Driv=
er=3Dbtusb
> > E: Ad=3D03(O) Atr=3D01(Isoc) MxPS=3D 9 Ivl=3D1ms
> > E: Ad=3D83(I) Atr=3D01(Isoc) MxPS=3D 9 Ivl=3D1ms
> > I: If#=3D 1 Alt=3D 2 #EPs=3D 2 Cls=3De0(wlcon) Sub=3D01 Prot=3D01 Driv=
er=3Dbtusb
> > E: Ad=3D03(O) Atr=3D01(Isoc) MxPS=3D 17 Ivl=3D1ms
> > E: Ad=3D83(I) Atr=3D01(Isoc) MxPS=3D 17 Ivl=3D1ms
> > I: If#=3D 1 Alt=3D 3 #EPs=3D 2 Cls=3De0(wlcon) Sub=3D01 Prot=3D01 Driv=
er=3Dbtusb
> > E: Ad=3D03(O) Atr=3D01(Isoc) MxPS=3D 25 Ivl=3D1ms
> > E: Ad=3D83(I) Atr=3D01(Isoc) MxPS=3D 25 Ivl=3D1ms
> > I: If#=3D 1 Alt=3D 4 #EPs=3D 2 Cls=3De0(wlcon) Sub=3D01 Prot=3D01 Driv=
er=3Dbtusb
> > E: Ad=3D03(O) Atr=3D01(Isoc) MxPS=3D 33 Ivl=3D1ms
> > E: Ad=3D83(I) Atr=3D01(Isoc) MxPS=3D 33 Ivl=3D1ms
> > I: If#=3D 1 Alt=3D 5 #EPs=3D 2 Cls=3De0(wlcon) Sub=3D01 Prot=3D01 Driv=
er=3Dbtusb
> > E: Ad=3D03(O) Atr=3D01(Isoc) MxPS=3D 49 Ivl=3D1ms
> > E: Ad=3D83(I) Atr=3D01(Isoc) MxPS=3D 49 Ivl=3D1ms
> >
> > Signed-off-by: Tedd Ho-Jeong An <[email protected]>
> > ---
>=20
> The v2 changelog should go here. Otherwise git am turns it into the commi=
t
> message.
ACK.
>=20
> > drivers/bluetooth/btusb.c | 333
> > +++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 333 insertions(+)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 3d684d2..c8c816b 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>
>=20
> I just remembered that you most likely need also change the Kconfig to
> depend on the firmware loader support.
>=20
> Actually you do not. You will get an -EINVAL error. Might want to print a=
n
> error in that case. As a general principle, it would be a good idea if bt=
usb.ko
> itself does not depend on the firmware loader.
>=20
Could you explain more about this?
Anyway, this header file is needed to read the firmware patch file (request=
_firmware).
> > #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[] =3D {
> > /* Generic Bluetooth USB device */
> > @@ -207,6 +209,9 @@ static struct usb_device_id blacklist_table[] =3D {
> > /* Frontline ComProbe Bluetooth Sniffer */
> > { USB_DEVICE(0x16d3, 0x0002), .driver_info =3D BTUSB_SNIFFER },
> >
> > + /* Intel Bluetooth device */
> > + { USB_DEVICE(0x8087, 0x07dc), .driver_info =3D BTUSB_INTEL },
> > +
> > { } /* Terminating entry */
> > };
> >
> > @@ -943,6 +948,331 @@ static int btusb_setup_bcm92035(struct hci_dev
> *hdev)
> > return 0;
> > }
> >
> > +static const struct firmware *btusb_setup_intel_get_fw(struct hci_dev
> *hdev,
> > + struct sk_buff *skb,
> > + int *use_default)
>=20
> > +{
> > + struct intel_fw_version {
>=20
> Make it intel_version since it is more than just the firmware version.
ACK.
>=20
> > + 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;
> > +
> > + struct intel_fw_version *ver;
> > + const struct firmware *fw;
> > + char fwname[64];
>=20
> Don't bother aligning these with white spaces. We only do that in structs=
.
ACK.
>=20
> > +
> > + if (skb->len !=3D sizeof(*ver)) {
> > + BT_ERR("%s invalid length of Intel firmware version %d",
> > + hdev->name, skb->len);
> > + return NULL;
> > + }
> > +
> > + ver =3D (void *)skb->data;
> > + snprintf(fwname, sizeof(fwname),
> > + "intel/ibt-hw-%x.%x.%x-fw-%x.%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, ver-
> >fw_patch_num);
> > + kfree_skb(skb);
>=20
> So I am not so sure that this is what we should be doing for the firmware
> versions.
>=20
> Lets brainstorm this a little bit. My idea is that we check give an easy =
way for
> configuration and firmware handling.
>=20
> I have a few questions with this. Can we have firmware ROM masks with a
> pre-patched version already. Meaning can fw_patch_num be something else
> than 0. If we can not incremental patch anyway, then we should leave that
> part out and not continue here further.
No and Yes. The firmware ROM mask cannot have pre-patched version, which me=
ans the fw_patch_num is always 0.
However, once the patches are activated after patching, fw_patch_num will b=
e updated to reflect the version of the activated patch and this will not b=
e changed until power recycled the device.
The fw_patch_num can be used as an indicator whether device is patched or n=
ot.
If the fw_patch_num is ignored, the driver will try to patch the device eac=
h time the device is enumerated or enter the setup stage even if the device=
is patched.
Anyway, I just realized that, in the current implementation, if the device =
is already patched and if the setup routine is being called again, it will =
try to use the default patch (in the current implementation), which is not =
correct.
I will make a change to check the fw_patch_num and only patch if this is 0.=
If it is non-zero, it will just exit with success - device is already patc=
hed.
>=20
> Also it might be a good idea to look for default settings with hw_platfor=
m and
> hw_variant, but where revision is not relevant so much.
ACK.
>=20
> > +
> > + *use_default =3D 0;
>=20
> About this one. Lets figure out if we have to activate a patch or not, bu=
t doing
> that in the patching routine and not by a firmware file name. If there is=
a
> command that loads a patch in the firmware file, then enable the activate
> otherwise just disable.
ACK.
>=20
> > +
> > + if (request_firmware(&fw, fwname, &hdev->dev) < 0) {
> > + BT_ERR("%s failed to open Intel firmware file: %s",
> > + hdev->name, fwname);
> > +
> > + snprintf(fwname, sizeof(fwname), "intel/ibt-default-
> %x.bseq",
> > + 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;
> > + }
> > +
> > + *use_default =3D 1;
> > + }
> > +
> > + BT_INFO("%s Intel Bluetooth firmware file: %s", hdev->name,
> fwname);
> > +
> > + return fw;
> > +}
> > +
> > +#define INTEL_HCI_MFG_MODE 0xfc11
> > +#define INTEL_HCI_GET_VER 0xfc05
> > +#define INTEL_HCI_CMD 0x01
> > +#define INTEL_HCI_EVT 0x02
>=20
> I am fine if you use this as hex codes in the code. The comment above giv=
es
> enough explanation and it makes the lines shorter.
ACK.=20
>=20
> > +static int btusb_setup_intel(struct hci_dev *hdev) {
> > + struct sk_buff *skb;
> > + const struct firmware *fw;
> > + const u8 *fw_ptr;
> > + int use_default;
> > +
> > + u8 mfg_enable[] =3D { 0x01, 0x00 };
> > + u8 mfg_no_reset[] =3D { 0x00, 0x00 };
>=20
> I would call this one mfg_disable. Previous patch was not needing it, so =
I did
> not mention it.
ACK
>=20
> > + u8 mfg_reset_deactivate[] =3D { 0x00, 0x01 };
> > + u8 mfg_reset_activate[] =3D { 0x00, 0x02 };
> > +
> > + BT_DBG("%s", hdev->name);
> > +
> > + /* The controller has a bug with the first HCI command send to it
> > + * 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 =3D __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 =3D __hci_cmd_sync(hdev, INTEL_HCI_GET_VER, 0, NULL,
> > + HCI_INIT_TIMEOUT);
> > + if (IS_ERR(skb)) {
> > + BT_ERR("%s reading Intel fw version command failed (%ld)",
> > + hdev->name, PTR_ERR(skb));
> > + return -PTR_ERR(skb);
> > + }
> > +
>=20
> Maybe add just a u8 status to the intel_version struct and make sure to
> check the length here. I would still print the version details with BT_IN=
FO so
> we have them in dmesg logging.
ACK.
>=20
> Regards
>=20
> Marcel
>=20
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to [email protected] More majordomo
> info at http://vger.kernel.org/majordomo-info.html
Hi Tedd,
> This patch adds support for Intel Bluetooth device by adding
> btusb_setup_intel() routine that update the device with ROM patch.
>
> This is v2 after running checkpatch script with "strict" option.
this comment does not belong here.
>
> 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 <[email protected]>
> ---
The v2 changelog should go here. Otherwise git am turns it into the commit message.
> drivers/bluetooth/btusb.c | 333 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 333 insertions(+)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 3d684d2..c8c816b 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>
I just remembered that you most likely need also change the Kconfig to depend on the firmware loader support.
Actually you do not. You will get an -EINVAL error. Might want to print an error in that case. As a general principle, it would be a good idea if btusb.ko itself does not depend on the firmware loader.
> #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,331 @@ static int btusb_setup_bcm92035(struct hci_dev *hdev)
> return 0;
> }
>
> +static const struct firmware *btusb_setup_intel_get_fw(struct hci_dev *hdev,
> + struct sk_buff *skb,
> + int *use_default)
> +{
> + struct intel_fw_version {
Make it intel_version since it is more than just the firmware version.
> + 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;
> +
> + struct intel_fw_version *ver;
> + const struct firmware *fw;
> + char fwname[64];
Don't bother aligning these with white spaces. We only do that in structs.
> +
> + if (skb->len != sizeof(*ver)) {
> + BT_ERR("%s invalid length of Intel firmware version %d",
> + hdev->name, skb->len);
> + return NULL;
> + }
> +
> + ver = (void *)skb->data;
> + snprintf(fwname, sizeof(fwname),
> + "intel/ibt-hw-%x.%x.%x-fw-%x.%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, ver->fw_patch_num);
> + kfree_skb(skb);
So I am not so sure that this is what we should be doing for the firmware versions.
Lets brainstorm this a little bit. My idea is that we check give an easy way for configuration and firmware handling.
I have a few questions with this. Can we have firmware ROM masks with a pre-patched version already. Meaning can fw_patch_num be something else than 0. If we can not incremental patch anyway, then we should leave that part out and not continue here further.
Also it might be a good idea to look for default settings with hw_platform and hw_variant, but where revision is not relevant so much.
> +
> + *use_default = 0;
About this one. Lets figure out if we have to activate a patch or not, but doing that in the patching routine and not by a firmware file name. If there is a command that loads a patch in the firmware file, then enable the activate otherwise just disable.
> +
> + if (request_firmware(&fw, fwname, &hdev->dev) < 0) {
> + BT_ERR("%s failed to open Intel firmware file: %s",
> + hdev->name, fwname);
> +
> + snprintf(fwname, sizeof(fwname), "intel/ibt-default-%x.bseq",
> + 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;
> + }
> +
> + *use_default = 1;
> + }
> +
> + BT_INFO("%s Intel Bluetooth firmware file: %s", hdev->name, fwname);
> +
> + return fw;
> +}
> +
> +#define INTEL_HCI_MFG_MODE 0xfc11
> +#define INTEL_HCI_GET_VER 0xfc05
> +#define INTEL_HCI_CMD 0x01
> +#define INTEL_HCI_EVT 0x02
I am fine if you use this as hex codes in the code. The comment above gives enough explanation and it makes the lines shorter.
> +static int btusb_setup_intel(struct hci_dev *hdev)
> +{
> + struct sk_buff *skb;
> + const struct firmware *fw;
> + const u8 *fw_ptr;
> + int use_default;
> +
> + u8 mfg_enable[] = { 0x01, 0x00 };
> + u8 mfg_no_reset[] = { 0x00, 0x00 };
I would call this one mfg_disable. Previous patch was not needing it, so I did not mention it.
> + 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
> + * 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, INTEL_HCI_GET_VER, 0, NULL,
> + HCI_INIT_TIMEOUT);
> + if (IS_ERR(skb)) {
> + BT_ERR("%s reading Intel fw version command failed (%ld)",
> + hdev->name, PTR_ERR(skb));
> + return -PTR_ERR(skb);
> + }
> +
Maybe add just a u8 status to the intel_version struct and make sure to check the length here. I would still print the version details with BT_INFO so we have them in dmesg logging.
Regards
Marcel