Hi Marcel,
On Thu, Feb 28, 2013, Marcel Holtmann wrote:
> >>> +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.
> 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.
Johan
--
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
