Hi Tedd,
> 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"?
my bad that I was not clear. This one is of course correct since you don't have a constant for the cmd->opcode value. I meant the one occasion above where you missed the swapping. There you should use the __constant_le16_to_cpu function.
>>> + 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.
That is what I almost figured.
Johan, can you check if we can fix __hci_cmd_sync to take a const array.
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
