Hi Marcel,
On Fri, Feb 17, 2012, Marcel Holtmann wrote:
> > >> +int mgmt_interleaved_discovery(struct hci_dev *hdev)
> > >> +{
> > >> + int err;
> > >> +
> > >> + BT_DBG("%s", hdev->name);
> > >> +
> > >> + err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR_LE);
> > >> + if (err < 0) {
> > >> + hci_dev_lock(hdev);
> > >> + hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> > >> + hci_dev_unlock(hdev);
> > >> + }
> > >> +
> > >> + return err;
> > >> +}
> > >
> > > The locking doesn't look right to me above. hci_do_inquiry should be
> > > called with the lock held. I think it might be simpler if you make
> > > mgmt_interleaved_discovery() require the caller to hold the lock.
> >
> > Yes, you're right. I just realized hci_do_inquiry now calls inquiry_
> > cache_flush which requires hdev->lock held. I'll fix this too. Thanks.
>
> please keep the lock inside mgmt_interleaved_discovery() for now. We
> have enough locking crazy. I don't wanna add to it by making the caller
> deal with it right now.
The calling code looks like this:
+
+ if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED) {
+ mgmt_interleaved_discovery(hdev);
+ } else {
+ hci_dev_lock(hdev);
+ hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+ hci_dev_unlock(hdev);
+ }
I was thinking that if mgmt_interleaved_discovery required the lock to
be held this code would become simpler:
hci_dev_lock(hdev);
if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED)
mgmt_interleaved_discovery(hdev);
else
hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
hci_dev_unlock(hdev);
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
