Hi Marcel, > -----Original Message----- > From: Marcel Holtmann [mailto:marcel@xxxxxxxxxxxx] > Sent: Sunday, October 25, 2015 7:57 PM > To: Pawlak, KubaX T > Cc: linux-bluetooth@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] Bluetooth: remove btusb_work related workqueue > > Hi Kuba, > > >>>>>> Workqueue handling btusb_work is no longer needed as btusb_notify > >>>>>> can sleep. > >>>>>> > >>>>>> Signed-off-by: Kuba Pawlak <kubax.t.pawlak@xxxxxxxxx> > >>>>>> --- > >>>>>> drivers/bluetooth/btusb.c | 14 +++++--------- > >>>>>> 1 file changed, 5 insertions(+), 9 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > >>>>>> index > >>>>> > >> > 247b1062cb9affc67d9e7f7a0234c167f90f4b79..98c616ddd7bac53b2d5d5f8b3dcc > >>>>> 2dca2b201e63 100644 > >>>>>> --- a/drivers/bluetooth/btusb.c > >>>>>> +++ b/drivers/bluetooth/btusb.c > >>>>>> @@ -350,7 +350,6 @@ struct btusb_data { > >>>>>> > >>>>>> unsigned long flags; > >>>>>> > >>>>>> - struct work_struct work; > >>>>>> struct work_struct waker; > >>>>>> > >>>>>> struct usb_anchor deferred; > >>>>>> @@ -980,7 +979,6 @@ static int btusb_close(struct hci_dev *hdev) > >>>>>> > >>>>>> BT_DBG("%s", hdev->name); > >>>>>> > >>>>>> - cancel_work_sync(&data->work); > >>>>>> cancel_work_sync(&data->waker); > >>>>>> > >>>>>> clear_bit(BTUSB_ISOC_RUNNING, &data->flags); > >>>>>> @@ -1181,6 +1179,8 @@ static int btusb_send_frame(struct hci_dev > >> *hdev, > >>>>> struct sk_buff *skb) > >>>>>> return -EILSEQ; > >>>>>> } > >>>>>> > >>>>>> +static void btusb_sco_change(struct btusb_data *data); > >>>>>> + > >>>>> > >>>>> do we need this forward declaration or can we just move code around > to > >> avoid > >>>>> it? > >>>>> > >>>>>> static void btusb_notify(struct hci_dev *hdev, unsigned int evt) > >>>>>> { > >>>>>> struct btusb_data *data = hci_get_drvdata(hdev); > >>>>>> @@ -1189,7 +1189,7 @@ static void btusb_notify(struct hci_dev > *hdev, > >>>>> unsigned int evt) > >>>>>> > >>>>>> if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) { > >>>>>> data->sco_num = hci_conn_num(hdev, SCO_LINK); > >>>>>> - schedule_work(&data->work); > >>>>>> + btusb_sco_change(data); > >>>>>> } > >>>>>> } > >>>>>> > >>>>>> @@ -1236,9 +1236,8 @@ static inline int __set_isoc_interface(struct > >> hci_dev > >>>>> *hdev, int altsetting) > >>>>>> return 0; > >>>>>> } > >>>>>> > >>>>>> -static void btusb_work(struct work_struct *work) > >>>>>> +static void btusb_sco_change(struct btusb_data *data) > >>>>>> { > >>>>>> - struct btusb_data *data = container_of(work, struct > btusb_data, > >>>>> work); > >>>>>> struct hci_dev *hdev = data->hdev; > >>>>>> int new_alts; > >>>>>> int err; > >>>>>> @@ -2618,7 +2617,6 @@ static int btusb_probe(struct usb_interface > >> *intf, > >>>>>> data->udev = interface_to_usbdev(intf); > >>>>>> data->intf = intf; > >>>>>> > >>>>>> - INIT_WORK(&data->work, btusb_work); > >>>>>> INIT_WORK(&data->waker, btusb_waker); > >>>>>> init_usb_anchor(&data->deferred); > >>>>>> init_usb_anchor(&data->tx_anchor); > >>>>>> @@ -2848,8 +2846,6 @@ static int btusb_suspend(struct usb_interface > >> *intf, > >>>>> pm_message_t message) > >>>>>> return -EBUSY; > >>>>>> } > >>>>>> > >>>>>> - cancel_work_sync(&data->work); > >>>>>> - > >>>>>> btusb_stop_traffic(data); > >>>>>> usb_kill_anchored_urbs(&data->tx_anchor); > >>>>>> > >>>>>> @@ -2922,7 +2918,7 @@ static int btusb_resume(struct usb_interface > >> *intf) > >>>>>> play_deferred(data); > >>>>>> clear_bit(BTUSB_SUSPENDING, &data->flags); > >>>>>> spin_unlock_irq(&data->txlock); > >>>>>> - schedule_work(&data->work); > >>>>>> + btusb_sco_change(data); > >>>>> > >>>>> Can we actually sleep in the resume callbacks? I think so, but I have not > >> testing > >>>>> runtime power management in a while. > >>>> > >>>> Apparently we can't, so workqueue needs to stay and this patch is invalid. > >>> > >>> I think that removing the usage of the workqueue in btusb_notify is still a > >> good thing since it reduces the latency when establishing SCO connections. > >> > >> actually I wonder if the call to usb_set_interface is needed on resume in the > >> first place. Maybe this is actually not needed and we just need to restart the > >> URBs. > >> > >> Do you have the chance to test and see if the alternate setting actually > changed > >> over resume? > > > > Our platform does not support suspend/resume. I have an Ellisys analyser > with external WP module connected through it over USB, so I can trace what > altsettings are set and when. I will look into a possibility of testing this using a > laptop, but I don’t know how to easily make phone open SCO channel, so I > don't need to port our PulseAudio. > > any update here? I would really get of this workqueue usage in btusb driver. Sorry for the delay, but I'm stuck in some internal project stuff. I used Ellisys and a laptop running Fedora 21 as our system does not support any type of suspend. To create bogus SCO link I use 'scotest -n <phone MAC>'. There is no data flowing, but at least stream is set up. On stream creation AltSettings change to 2 as expected. On laptop suspend there is nothing there, looks like we don't change settings. Controller remains active. On resume we get: GetDescriptor(Device), SetAddress (2), GetDescriptor(Device), GetDescriptor(Configuration),SetConfiguration(1),SetInterface (1, AltSet2), GetStatus(Device). So it looks like we are changing altsetting in resume to whatever was set before suspend. At btusb_work can sleep and resume does not, it looks like we can't remove this workqueue from there. I don’t know USB at all so I can’t say what to do with that. Does active SCO link over suspend/resume makes sense? There is also an annoying Ellisys bug in here. When creating SCO link with scotest, there is no update to overview, no matter what I do to grouping. Only after resume, all messages are pushed to the overview and I can see what happened. Kuba Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Prof. Dr. Hermann Eul Chairperson of the Supervisory Board: Tiffany Doon Silva Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 ��.n��������+%������w��{.n�����{��� �^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
