Re: [PATCH 02/12 v3] Bluetooth: Add basic start/complete HCI transaction functions

[Johan Hedberg <johan.hedberg@gmail.com>]

Hi Marcel,

On Sat, Feb 16, 2013, Marcel Holtmann wrote:
> > +int hci_start_transaction(struct hci_dev *hdev)
> > +{
> > +	struct hci_transaction *transaction;
> > +	int err;
> > +
> > +	hci_transaction_lock(hdev);
> > +
> > +	/* We can't start a new transaction if there's another one in
> > +	 * progress of being built.
> > +	 */
> > +	if (hdev->build_transaction) {
> > +		err = -EBUSY;
> > +		goto unlock;
> > +	}
> 
> I do not get this part. Why not use a common mutex on
> hci_start_transaction and have it close with hci_complete_transaction.
> 
> This sounds more like a double protection.
> 
> > +
> > +	transaction = kzalloc(sizeof(*transaction), GFP_KERNEL);
> > +	if (!transaction) {
> > +		err = -ENOMEM;
> > +		goto unlock;
> > +	}
> > +
> > +	skb_queue_head_init(&transaction->cmd_q);
> > +
> > +	hdev->build_transaction = transaction;
> > +
> 
> I am bit confused with this build_transaction concept. So we are
> expecting to build transaction inside the same function. Meaning that
> start and complete functions will be called in the same context.
> 
> Maybe some concept of DECLARE_TRANSACTION declaring a local variable and
> then start and complete transaction would be better.

Both of the above two issues are related to the desire to not have to
introduce a new function next to hci_send_cmd() (that would take this
local variable "context") and to be able to keep all current
hci_send_cmd() calls as they are.

In my initial iteration of the patch set I did have the kind of locking
you describe, but keeping the requirement for hci_send_cmd() like I
stated means that you'll then either end up having a potential race
or a deadlock inside the function since sometimes you're entering with
the transaction lock held (when using begin/finish transaction) and
sometimes without the lock held (all existing calls to hci_send_cmd().

So what we're dealing with here is trading less complexity in all our
hci_send_cmd() callers (not having to pass around this extra context)
for slightly more complexity inside the hci_send_cmd() function, which I
maintain is less complexity than removing it would introduce elsewhere,
basically this:

       hci_transaction_lock(hdev);

        /* If this is part of a multi-command transaction (i.e.
         * hci_start_transaction() has been called) just add the skb to
         * the end of the transaction being built.
         */
        if (hdev->build_transaction) {
                skb_queue_tail(&hdev->build_transaction->cmd_q, skb);
                goto unlock;
        }

        /* If we're in the middle of a hci_request the req lock will be
         * held and our only choice is to append to the request
         * transaction.
         */
        if (hdev->req_status && hdev->current_transaction) {
                skb_queue_tail(&hdev->current_transaction->cmd_q, skb);
                goto unlock;
        }

        /* This is neither a multi-command transaction nor a hci_request
         * situation, but simply hci_send_cmd being called without any
         * existing context. Create a simple one-command transaction out
         * of the skb
         */
        err = __transaction_from_skb(hdev, skb);
        if (err < 0)
                kfree_skb(skb);

unlock:
        hci_transaction_unlock(hdev);


> On that topic using begin_transaction and finish_transaction sounds a
> bit more appealing. Or even run_transaction instead of finish.

begin/run sounds fine to me.

Johan