Return-Path: Date: Fri, 1 Mar 2013 09:04:25 +0200 From: Johan Hedberg To: Marcel Holtmann Cc: linux-bluetooth@vger.kernel.org Subject: Re: [PATCH v3 03/16] Bluetooth: Add initial skeleton for HCI transaction framework Message-ID: <20130301070425.GB14811@x220.P-661HNU-F1> References: <1361951844-13719-1-git-send-email-johan.hedberg@gmail.com> <1361951844-13719-4-git-send-email-johan.hedberg@gmail.com> <4FF31B76-4E16-4481-9158-4874B3D67349@holtmann.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <4FF31B76-4E16-4481-9158-4874B3D67349@holtmann.org> List-ID: 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