From: Tedd Ho-Jeong An <[email protected]>
This patch fixes the command length alignment issue for Intel Bluetooth
8260.
The length of parameters in the firmware downloading command must be
multiplication of 4. If not, the command must append Intel_NOP command
with extra parameters, zeros, at the end, and the firmware file is
already included Intel_NOP command for alignment.
This patch checks the next command and if the next command is Intel_NOP
command, it reads the Intel_NOP command and send them together.
For example, if the data from the firmware file looks like this:
8E FC 03 11 22 33 02 FC 03 00 00 00
Previously, btusb sends two commands:
09 FC 06 8E FC 03 11 22 33
09 FC 06 02 FC 03 00 00 00
This won't work because the length of parameters are 6 which violates
the 4 byte alignment.
This patch will append them together and send as one command:
09 FC 0B 8E FC 03 11 22 33 02 FC 03 00 00 00
Signed-off-by: Tedd Ho-Jeong An <[email protected]>
---
drivers/bluetooth/btusb.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index d21f3b4..13b9969 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2541,6 +2541,22 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
cmd_len = sizeof(*cmd) + cmd->plen;
+ /* The parameter length of secure send command should be a
+ * multiplication of 4. If it is not, it needs to append
+ * Intel_NOP command with arbitrary number of parameters (zeros)
+ * to meet the 4 byte alignment.
+ * The FW file has already formatted with this. So if the next
+ * command is Intel_NOP then send them together.
+ */
+ cmd = (void *)(fw_ptr + cmd_len);
+ if (le16_to_cpu(cmd->opcode) == 0xfc02) {
+ BT_DBG("%s: Updated cmd to include Intel_NOP",
+ hdev->name);
+ /* Update cmd_len to include the Intel_NOP command
+ */
+ cmd_len += sizeof(*cmd) + cmd->plen;
+ }
+
/* Send each command from the firmware data buffer as
* a single Data fragment.
*/
--
2.1.0
Hi Marcel,
On Mon, 18 May 2015 15:52:07 -0700
Tedd Ho-Jeong An <[email protected]> wrote:
> From: Tedd Ho-Jeong An <[email protected]>
>
> This patch fixes the command length alignment issue for Intel Bluetooth
> 8260.
>
> The length of parameters in the firmware downloading command must be
> multiplication of 4. If not, the command must append Intel_NOP command
> with extra parameters, zeros, at the end, and the firmware file is
> already included Intel_NOP command for alignment.
>
> This patch checks the next command and if the next command is Intel_NOP
> command, it reads the Intel_NOP command and send them together.
>
> For example, if the data from the firmware file looks like this:
> 8E FC 03 11 22 33 02 FC 03 00 00 00
>
> Previously, btusb sends two commands:
> 09 FC 06 8E FC 03 11 22 33
> 09 FC 06 02 FC 03 00 00 00
>
> This won't work because the length of parameters are 6 which violates
> the 4 byte alignment.
>
> This patch will append them together and send as one command:
> 09 FC 0B 8E FC 03 11 22 33 02 FC 03 00 00 00
>
> Signed-off-by: Tedd Ho-Jeong An <[email protected]>
> ---
> drivers/bluetooth/btusb.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index d21f3b4..13b9969 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -2541,6 +2541,22 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
>
> cmd_len = sizeof(*cmd) + cmd->plen;
>
> + /* The parameter length of secure send command should be a
> + * multiplication of 4. If it is not, it needs to append
> + * Intel_NOP command with arbitrary number of parameters (zeros)
> + * to meet the 4 byte alignment.
> + * The FW file has already formatted with this. So if the next
> + * command is Intel_NOP then send them together.
> + */
> + cmd = (void *)(fw_ptr + cmd_len);
> + if (le16_to_cpu(cmd->opcode) == 0xfc02) {
> + BT_DBG("%s: Updated cmd to include Intel_NOP",
> + hdev->name);
> + /* Update cmd_len to include the Intel_NOP command
> + */
> + cmd_len += sizeof(*cmd) + cmd->plen;
> + }
> +
> /* Send each command from the firmware data buffer as
> * a single Data fragment.
> */
I just wonder if you had a chance to review this patch.
Regards,
Tedd
Hi Marcel,
On Sat, 6 Jun 2015 08:15:01 +0200
Marcel Holtmann <[email protected]> wrote:
> Hi Tedd,
>
> > This patch fixes the command length alignment issue for Intel Bluetooth
> > 8260.
> >
> > The length of parameters in the firmware downloading command must be
> > multiplication of 4. If not, the command must append Intel_NOP command
> > with extra parameters, zeros, at the end, and the firmware file is
> > already included Intel_NOP command for alignment.
> >
> > This patch checks the next command and if the next command is Intel_NOP
> > command, it reads the Intel_NOP command and send them together.
> >
> > For example, if the data from the firmware file looks like this:
> > 8E FC 03 11 22 33 02 FC 03 00 00 00
> >
> > Previously, btusb sends two commands:
> > 09 FC 06 8E FC 03 11 22 33
> > 09 FC 06 02 FC 03 00 00 00
> >
> > This won't work because the length of parameters are 6 which violates
> > the 4 byte alignment.
> >
> > This patch will append them together and send as one command:
> > 09 FC 0B 8E FC 03 11 22 33 02 FC 03 00 00 00
>
> I assume this should be 09 FC 0C to indicate 12 bytes of payload.
>
> > Signed-off-by: Tedd Ho-Jeong An <[email protected]>
> > ---
> > drivers/bluetooth/btusb.c | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index d21f3b4..13b9969 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -2541,6 +2541,22 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
> >
> > cmd_len = sizeof(*cmd) + cmd->plen;
> >
> > + /* The parameter length of secure send command should be a
> > + * multiplication of 4. If it is not, it needs to append
> > + * Intel_NOP command with arbitrary number of parameters (zeros)
> > + * to meet the 4 byte alignment.
> > + * The FW file has already formatted with this. So if the next
> > + * command is Intel_NOP then send them together.
> > + */
> > + cmd = (void *)(fw_ptr + cmd_len);
> > + if (le16_to_cpu(cmd->opcode) == 0xfc02) {
> > + BT_DBG("%s: Updated cmd to include Intel_NOP",
> > + hdev->name);
> > + /* Update cmd_len to include the Intel_NOP command
> > + */
> > + cmd_len += sizeof(*cmd) + cmd->plen;
> > + }
> > +
>
> I might have sent you down the wrong patch when looking for the Intel_NOP as next command. After starring at this problem for a bit longer, I think this might be the smarter approach to this.
>
> fw_ptr = fw->data + 644;
> frag_len = 0;
>
> while (fw_ptr - fw->data < fw->size) {
> struct hci_command_hdr *cmd = (void *)(fw_ptr + frag_len);
>
> frag_len += sizeof(*cmd) + cmd->plen;
>
> /* The paramter length of the secure send command requires
> * a 4 byte alignment. It happens so that the firmware file
> * contains proper Intel_NOP commands to align the fragments
> * as needed.
> *
> * Send set of commands with 4 byte alignment from the
> * firmware data buffer as a single Data fragement.
> */
> if (!(frag_len % 4)) {
> err = btusb_intel_secure_send(hdev, 0x01, frag_len,
> fw_ptr);
> if (err < 0) {
> BT_ERR("%s: Failed to send firmware data (%d)",
> hdev->name, err);
> goto done;
> }
>
> fw_ptr += frag_len;
> frag_len = 0;
> }
> }
>
> I know this compiles, but I have not had a chance to actually test it with real hardware. So if this works, then I would prefer to go this route instead. Please give a try and update your patch. Thanks.
I found your RFC and tested with REL182 fw which had alignment issue. Please see my response to that email. Bottom line is that it worked well.
>
> Regards
>
> Marcel
>
Hi Tedd,
> This patch fixes the command length alignment issue for Intel Bluetooth
> 8260.
>
> The length of parameters in the firmware downloading command must be
> multiplication of 4. If not, the command must append Intel_NOP command
> with extra parameters, zeros, at the end, and the firmware file is
> already included Intel_NOP command for alignment.
>
> This patch checks the next command and if the next command is Intel_NOP
> command, it reads the Intel_NOP command and send them together.
>
> For example, if the data from the firmware file looks like this:
> 8E FC 03 11 22 33 02 FC 03 00 00 00
>
> Previously, btusb sends two commands:
> 09 FC 06 8E FC 03 11 22 33
> 09 FC 06 02 FC 03 00 00 00
>
> This won't work because the length of parameters are 6 which violates
> the 4 byte alignment.
>
> This patch will append them together and send as one command:
> 09 FC 0B 8E FC 03 11 22 33 02 FC 03 00 00 00
I assume this should be 09 FC 0C to indicate 12 bytes of payload.
> Signed-off-by: Tedd Ho-Jeong An <[email protected]>
> ---
> drivers/bluetooth/btusb.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index d21f3b4..13b9969 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -2541,6 +2541,22 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
>
> cmd_len = sizeof(*cmd) + cmd->plen;
>
> + /* The parameter length of secure send command should be a
> + * multiplication of 4. If it is not, it needs to append
> + * Intel_NOP command with arbitrary number of parameters (zeros)
> + * to meet the 4 byte alignment.
> + * The FW file has already formatted with this. So if the next
> + * command is Intel_NOP then send them together.
> + */
> + cmd = (void *)(fw_ptr + cmd_len);
> + if (le16_to_cpu(cmd->opcode) == 0xfc02) {
> + BT_DBG("%s: Updated cmd to include Intel_NOP",
> + hdev->name);
> + /* Update cmd_len to include the Intel_NOP command
> + */
> + cmd_len += sizeof(*cmd) + cmd->plen;
> + }
> +
I might have sent you down the wrong patch when looking for the Intel_NOP as next command. After starring at this problem for a bit longer, I think this might be the smarter approach to this.
fw_ptr = fw->data + 644;
frag_len = 0;
while (fw_ptr - fw->data < fw->size) {
struct hci_command_hdr *cmd = (void *)(fw_ptr + frag_len);
frag_len += sizeof(*cmd) + cmd->plen;
/* The paramter length of the secure send command requires
* a 4 byte alignment. It happens so that the firmware file
* contains proper Intel_NOP commands to align the fragments
* as needed.
*
* Send set of commands with 4 byte alignment from the
* firmware data buffer as a single Data fragement.
*/
if (!(frag_len % 4)) {
err = btusb_intel_secure_send(hdev, 0x01, frag_len,
fw_ptr);
if (err < 0) {
BT_ERR("%s: Failed to send firmware data (%d)",
hdev->name, err);
goto done;
}
fw_ptr += frag_len;
frag_len = 0;
}
}
I know this compiles, but I have not had a chance to actually test it with real hardware. So if this works, then I would prefer to go this route instead. Please give a try and update your patch. Thanks.
Regards
Marcel