Hi Marcel, On Wed, Oct 2, 2013 at 2:23 AM, Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote: > > Hi Andre, > > > This patch moves hci_dev_lock and hci_dev_unlock calls to where they > > are really required, reducing the critical region in hci_le_conn_ > > complete_evt function. hdev->lock is required only in hci_conn_del > > and hci_conn_add call to protect concurrent add and remove operations > > in hci_conn_hash list. > > is this statement actually true? Because we have done this for so many HCI event, that I highly doubt that your statement tis correct. And if it is correct, you need to fix all other users first. Well, if we take a look at each function called inside le_conn_complete_evt(), we'll find: * hci_conn_hash_lookup_state which traverses hdev->conn_hash (protected by RCU). So it doesn't require hdev->lock. * mgmt_connect_failed which accesses hdev->id. hdev->id is written only at hci_register_dev(). So it doesn't require hdev->lock. * hci_proto_connect_cfm: it handles connection layer stuff, So it doesn't require hdev->lock. * mgmt_device_connected which access hdev->name. hdev->name is written only at hci_register_dev(). So it doesn't require hdev->lock. * hci_conn_add_sysfs which access only hdev->name. So it doesn't require hdev->lock. * hci_conn_del which removes a element from hdev->conn_hash. Since hdev->conn_hash is protected by RCU, we have to guarantee updaters mutual exclusion. So it requires hdev->lock. * hci_conn_add which adds a new element to hdev->conn_hash. Since we have to guarantee updaters mutual exclusion, it requires hdev->lock. That being said, I'm not sure the same applies for all others HCI handlers. We have to analyze each handler to make sure we can safely move the locking. Regards, Andre -- 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
