Hi Marcel,
On Thu, Feb 28, 2013, Marcel Holtmann wrote:
> > +typedef void (*transaction_complete_t)(struct hci_dev *hdev, u16 last_cmd,
> > + int status);
>
> why is status an int and not __u8?
>
> We might want to prefix this with hci_ here. Just in case someone
> includes this header.
The int came from the old hci_req_complete() API, but you're right it
doesn't really make sense to have anything else than u8. I'll fix both
issues for the next iteration of the set.
> > +struct transaction_ctrl {
>
> Same here. hci_ prefix seems like a good idea.
Sure.
> > + __u8 start;
>
> Why is not a bool and we are using true/false. Magic numbers like 1 in
> the code seem like a bad idea.
The main reason is that I wanted to control the amount of data we
consume from the control buffer (which is limited to 40 bytes) and the
size of bool is architecture dependent (could be 1 byte or 4 or
something else). E.g. all other existing variables part of the buffer
are either u8 or u16. We still have plenty of space left though and the
complete callback pointer is anyway not a fixed size, so if you're not
too worried that we'll continue extending the buffer in the future I'll
just change this to bool.
> > +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.
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
