Hi Johan,
>>>>> +int hci_transaction_run(struct hci_transaction *transaction)
>>>>> +{
>>>>> + struct hci_dev *hdev = transaction->hdev;
>>>>> +
>>>>> + BT_DBG("length %u", skb_queue_len(&transaction->cmd_q));
>>>>> +
>>>>> + /* Do not allow empty transactions */
>>>>> + if (skb_queue_empty(&transaction->cmd_q))
>>>>> + return -EINVAL;
>>>>> +
>>>>> + spin_lock(&hdev->cmd_q.lock);
>>>>> + skb_queue_splice_tail(&transaction->cmd_q, &hdev->cmd_q);
>>>>> + spin_unlock(&hdev->cmd_q.lock);
>>>>> +
>>>>> + queue_work(hdev->workqueue, &hdev->cmd_work);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>
>>>> I am wondering why we are not giving the complete handler when
>>>> finishing the transaction. Having a copy of the handler in hci_dev
>>>> structure seems a bit pointless. What is the reason here?
>>>
>>> The hdev->transaction_complete is not related to building transactions
>>> but for running them. I can move giving the callback to the
>>> transaction_run function and thereby remove the need to store it in
>>> struct hci_transaction, but the fact remains that it still needs to be
>>> copied to the first skb of the transaction (since the moment we start
>>> processing the transaction the callback needs to be copied to
>>> hdev->transaction_complete). Storing the callback in struct
>>> hci_transaction made it easy to copy it to the first skb when
>>> hci_transaction_cmd() gets called for the first time on the transaction.
>>>
>>> So let me repeat, we need the hdev->complete_transaction since the
>>> callback could be needed for any individual HCI command that's part of
>>> the transaction in case that command fails (since then we stop
>>> processing the transaction and call the callback). Because of this the
>>> callback needs to be part of the first skb of the transaction.
>>
>> The way I see this is that if one command of the transaction fails, we
>> need to not continue and just discard the rest of the transaction. Why
>> not just go through the queue and find the complete callback attached
>> with the last skb of transaction. Either that is the last successful
>> command, or we had to go and remove the rest of the queue.
>
> A command that completes it not part of hdev->cmd_q anymore but can be
> found in hdev->sent_cmd instead. This means that to find the complete
> callback we first need to check for
> bt_cb(hdev->sent_cmd)->transaction.complete and if it's NULL start going
> through hdev->cmd_q. This is more complicated than just checking for
> hdev->transaction_complete, but if you think it's worth not having to
> add another hdev member then I'll go with it.
>
> Another problem is that the control buffer gets lost when doing
> skb_clone. This means we have to add a memcpy of the skb->cb after doing
> the hdev->sent_cmd = skb_clone(skb) in hci_cmd_work.
maybe we are trying to hard to make this fit with our existing model and we just need to redo it. The feeling that I am getting is that a complete callback function "temporary" storage inside hci_dev is a pretty bad idea. It should be part of the context of transaction itself and this mean the skb.
Not that we have been really great with this in the past, we clearly were not. But since we are touching tis code now, I might make sense to have a look on how HCI command handling can be done better.
Same goes for the sent_cmd data. As I said, in userspace we either keep the current packet at the head of the queue until it gets a response or we create a separate pending queue for the packets in fly. Since we only allow one HCI command at a time anyway, maybe just hanging on to cmd skb would be a good idea and in conjunction with your other comment, give a clone to the driver.
>> This reminds me. Have you tested this with random commands in the
>> transaction failing?
>
> I *think* I tested it, but I'll make sure to do it at least before
> sending the next patch set iteration.
Please test this. Since if any comment of the transaction fails, we need to remove all pending commands of that said transaction.
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
