Hi Marcel,
On 12/8/14, 10:11 PM, "Marcel Holtmann" <marcel@xxxxxxxxxxxx> wrote:
>Hi Tedd,
>
>> + if (ret < 0) {
>> + BT_ERR("%s: sending DATA section failed", hdev->name);
>> + return ret;
>> + }
>> +
>> + fw_ptr += read_len;
>> + remain -= read_len;
>> + }
>
>And here you already have the fragmentation as a more generic approach.
>As I said, lets put that all into btusb_setup_intel_send_sec. Then this
>becomes nice and simple code.
>
>I even get the feeling that btusb_setup_intel_sec_patching can then be
>removed and the handling of the 4 sections folded into
>btusb_setup_intel_boot2 directly.
Got it. I will try above approach first then try to see if I can remove
btusb_setup_intel_sec_patching.
>
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * Send Intel Reset command to the device to enable the operational
>>firmware.
>> + * Because the device will not send CC event for Intel Reset command,
>> + * the command is sent with btusb_send_frame() instead of
>>__hci_cmd_sync() so
>> + * the host won't wait for event.
>> + */
>
>Why :(
>
>These are the things that does frustrate me sometimes. Is it too much to
>ask from the bootloader to acknowledge that it received the command and
>only then go and reset everything in the device.
>
>I mean you start using a well defined transport protocol and then you
>start violating it whenever you please because it is some tiny shortcut.
>It means that the host stack now has to do extra work and we can never
>tell when the device actually does a reset or if something fails. I mean
>how would we know if some of the firmware verification fails?
I knew that you won¹t like it. :)
This is currently on discussion and this implementation is based on the
current spec and firmware design.
I will update you in a few days.
>
>> +static int btusb_setup_intel_send_reset(struct hci_dev *hdev)
>> +{
>> + int ret;
>
>Use int err and normally we have these last in the variable list.
>
>> + int len, plen;
>> + struct hci_command_hdr *hdr;
>> + struct sk_buff *skb;
>> +
>> + u8 intel_reset[] = { 0x00, 0x00, 0x00, 0x01, 0x00, 0x08, 0x04, 0x00 };
>
>Make these const at least.
>
>> +
>> + plen = sizeof(intel_reset);
>> +
>> + len = HCI_COMMAND_HDR_SIZE + plen;
>> + skb = bt_skb_alloc(len, GFP_ATOMIC);
>> + if (!skb) {
>> + BT_ERR("%s: failed to allocate sk_buff", hdev->name);
>> + return -ENOMEM;
>> + }
>> +
>> + hdr = (struct hci_command_hdr *) skb_put(skb, HCI_COMMAND_HDR_SIZE);
>> + hdr->opcode = cpu_to_le16(0xfc01);
>> + hdr->plen = plen;
>> +
>> + memcpy(skb_put(skb, plen), intel_reset, plen);
>> +
>> + bt_cb(skb)->pkt_type = HCI_COMMAND_PKT;
>> + bt_cb(skb)->opcode = 0xfc01;
>> +
>> + ret = btusb_send_frame(hdev, skb);
>> + if (ret < 0) {
>> + BT_ERR("%s: sending Intel Reset failed (%d)", hdev->name, ret);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int btusb_setup_intel_boot2(struct hci_dev *hdev)
>> +{
>> + struct btusb_data *data = hci_get_drvdata(hdev);
>> + const struct firmware *fw;
>> + int err;
>> +
>> + BT_DBG("%s", hdev->name);
>> +
>> + /*
>> + * During the setup, the specific HCI command (INTE_SEC_SEND) and its
>> + * correspond event are sending/receiving via bulk endpoint instead of
>> + * control ep and interrupt in order to improve the FW downlaoding
>> + * speed. So, it sending and receiving routines needs to be set to
>> + * custom ones during this time and it will set back to generic ones
>> + * after.
>> + */
>> + hdev->send = btusb_send_frame_intel;
>> + data->recv_bulk = btusb_recv_bulk_intel;
>
>I actually prefer that we do not mess with these pointers in a registered
>device. These should be set in the probe function and then stay there.
>
>> +
>> + fw = btusb_setup_intel_get_fw(hdev, INTEL_FW_MODE_BL);
>> + if (!fw) {
>> + err = 0;
>> + goto exit_error;
>> + }
>> +
>> + /* Start to download the firmware */
>> + err = btusb_setup_intel_sec_patching(hdev, fw);
>> + if (err < 0) {
>> + BT_ERR("%s: downloading firmware failed (%d)", hdev->name, err);
>> + goto exit_release;
>> + }
>> +
>> + /* Send Intel Reset to enable the downloaded operational firmware */
>> + err = btusb_setup_intel_send_reset(hdev);
>> + if (err < 0) {
>> + BT_ERR("%s: sending Intel Reset failed (%d)", hdev->name, err);
>> + goto exit_release;
>> + }
>> +
>> + BT_INFO("%s: Intel firmware downloading is completed", hdev->name);
>> +
>> +exit_release:
>> + release_firmware(fw);
>> +
>> + /* extra time after resetting the device */
>> + msleep(200);
>
>So this now gets execute as generic error path. I think this timeout
>should be actually in the btusb_setup_intel_send_reset function. Since
>that is the only time it needs to wait for some magic time to turn the
>device back.
>
>> +
>> + /*
>> + * Once the device is running with operational firmware, send device
>> + * specific configuration parameter with legacy way.
>> + */
>> + err = btusb_setup_intel(hdev);
>> + if (err < 0) {
>> + BT_ERR("%s: configuring Intel BT device failed (%d)",
>> + hdev->name, err);
>> + goto exit_error;
>> + }
>> +
>> + BT_INFO("%s: Intel Bluetooth device configuration is completed",
>> + hdev->name);
>> +
>> +exit_error:
>> + /*
>> + * After this point, All events will come from interrupt endpoint.
>> + * So, put back the bulk_recv routine to generic one
>> + */
>> + BT_INFO("%s: Reset the send and receive routines", hdev->name);
>> +
>> + data->recv_bulk = btusb_recv_bulk;
>> + hdev->send = btusb_send_frame;
>> +
>> + return err;
>> +}
>> +
>> +
>> static int btusb_set_bdaddr_intel(struct hci_dev *hdev, const bdaddr_t
>>*bdaddr)
>> {
>> struct sk_buff *skb;
>> @@ -2079,6 +2432,9 @@ static int btusb_probe(struct usb_interface *intf,
>> hdev->set_bdaddr = btusb_set_bdaddr_intel;
>> }
>>
>> + if (id->driver_info & BTUSB_INTEL_BOOT2)
>> + hdev->setup = btusb_setup_intel_boot2;
>> +
>
>This should include btusb_set_bdaddr_intel as well.
>
>And you also want to set the special btusb_send_frame_intel here as well.
>And same for the internal bulk handler.
>
>> if (id->driver_info & BTUSB_MARVELL)
>> hdev->set_bdaddr = btusb_set_bdaddr_marvell;
>
>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
--
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
