Hi, Marcel.
I was able to update most of the code with your comments except a couple of things.
Please see below.
On Thursday, April 18, 2013 11:47:58 AM Marcel Holtmann wrote:
> Hi Tedd,
>
> > + /* 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;
>
> The opcodes are 16 bit. You need to swap between host endian and the endian that is in the file here. I assume the file is in little endian.
>
> > + 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 -EFAULT;
> > + }
> > +
> > + 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 -EFAULT;
> > + }
> > +
> > + skb = __hci_cmd_sync_ev(hdev, le16_to_cpu(cmd->opcode),
> > + cmd->plen, (void *)cmd_param, evt->evt,
> > + HCI_INIT_TIMEOUT);
>
> Here you do the swapping. So I propose that you fix it above with a constant version of the helper.
constant version means "__constant_le16_to_cpu"?
>
> > + if (IS_ERR(skb)) {
> > + BT_ERR("%s sending Intel patch command failed (%ld)",
> > + hdev->name, PTR_ERR(skb));
> > + return -PTR_ERR(skb);
> > + }
> > +
> > + /* Checking the returned event */
>
> Please also be a bit more verbose here. You are checking first the length of the event matches and then also the content with what the firmware provides.
>
> > + if (skb->len != evt->plen) {
> > + BT_ERR("%s mismatch event length", hdev->name);
> > + kfree_skb(skb);
> > + return -EFAULT;
> > + }
> > +
> > + if (memcmp(skb->data, evt_param, evt->plen)) {
> > + BT_ERR("%s mismatch event parameter", hdev->name);
>
> Personally I would print out the opcode in both cases. You want to know which command failed, right?
>
> If you want to go the extra mile, you could also hexdump the whole command that failed with the event result. However that can be done in an add-on patch as well.
>
> > + kfree_skb(skb);
> > + return -EFAULT;
> > + }
> > + kfree_skb(skb);
> > +
> > + 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 };
>
> Check if you can make these const.
If these becomes const, the compiler throws a warning at __hci_cmd_sync() below.
>
> > +
> > + BT_DBG("%s", hdev->name);
> > +
> > + /* The controller has a bug with the first HCI command sent to it
> > + * returning number of completed commands as zero. This would stall the
> > + * command processing in the Bluetooth core
>
> Add a . at end of a paragraph.
>
> > + *
> > + * As a workaround, send HCI Reset command first which will reset the
> > + * number of completed commands and allow normal command processing
> > + * from now on
>
> Add a . at the end.
>
> > + */
>
> > + 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);
> > + 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 -EIO;
> > + }
> > +
>
> 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
--
Regards
Tedd Ho-Jeong An
Intel Corporation
--
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
