Hi Johan, >>>> 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. > > Thinking a bit more about this we should probably be passing the clone > to the HCI driver and not the original skb. The skb API documentation > talks about doing skb_clone() to retain ownership of the control buffer > between different layers and it's quite natural to consider the HCI > driver one layer and the HCI core another. I.e. the original skb > "belongs" to the HCI core and only the clone should be passed to the HCI > driver. This however means that we'll need to pass at least the pkt_type > as a separate parameter to hci_send_frame(). or we just set the pkt_type in the cb of the cloned skb. Do we also have to set the dev value for hci_dev with it? Sounds like a good idea, but I think the reason why we were never doing it is that it has an impact on ACL and SCO packets where we would be doing a clone for no reason. 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
