Messages in this thread | | | Date | Wed, 12 Jul 2017 10:27:04 +0800 | From | jeffy <> | Subject | Re: [RFC PATCH] Bluetooth: btusb: Fix memory leak in play_deferred |
| |
Hi Oliver,
Thanx for your comments, and sorry for reply late.
On 07/04/2017 07:38 PM, Oliver Neukum wrote: > Am Freitag, den 23.06.2017, 11:46 +0800 schrieb jeffy: >> >>>> --- >>>> >>>> drivers/bluetooth/btusb.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c >>>> index 278e811..b469f9b 100644 >>>> --- a/drivers/bluetooth/btusb.c >>>> +++ b/drivers/bluetooth/btusb.c >>>> @@ -3254,11 +3254,12 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message) >>>> >>>> static void play_deferred(struct btusb_data *data) >>>> { >>>> + struct hci_dev *hdev = data->hdev; >>>> struct urb *urb; >>>> int err; >>>> >>>> while ((urb = usb_get_from_anchor(&data->deferred))) { >>>> - err = usb_submit_urb(urb, GFP_ATOMIC); >>>> + err = submit_tx_urb(hdev, urb); > > If you do that you have to change submit_tx_urb() to be called under a > spinlock.
sorry, why we need that? since submit_tx_urb is basically usb_anchor_urb/usb_submit_urb/usb_free_urb
> >>>> if (err < 0) >>>> break; >>> >>> so why not just fix the memory leak here and instead call submit_tx_urb. I am not sure that is actually the right approach. Why anchor this URB now to the TX anchor now? Is that actually safe? >>> >> the current flow is: >> submit_or_queue_tx_urb >> if (!suspending) >> submit_tx_urb >> else >> put into deferred anchor >> wake btusb >> >> retry the deferred urbs in deferred anchor(using usb_submit_urb) >> after resumed >> >> so i think there are 2 problems here: >> 1/ error handling, compare submit_tx_urb to usb_submit_urb, it freed >> urb->setup_packet when failed to submit > > In theory yes. If we ever put control URBs on the deferred anchor. > >> 2/ memory leak: >> in usb_submit_urb, we ref that urb >> in __usb_hcd_giveback_urb, we unanchor it, and then unref it. >> >> so i think the usb_submit_urb expected the urb not just be referenced, >> but also anchored? > > It expects that in the sense that it reacts to anchorings, but they are > not required. > >> or referenced, but the caller would unref it himself >> later? > > The caller is responsible for its own references. hmm, maybe unref it in the complete callback(btusb_tx_complete?), and if we do so, we may need to detect which urb came from here... > >> and for tx_anchor, we put urb in it, and kill them all during suspending >> to prevent transfer. so i guess it would be safe to put deferred urb in >> to it after resume too? >> but i don't know much about usb/btusb, so i could be wrong all about that :) > > IIRC the reason for directly submitting them was the spinlock. sorry, i'm not clear about this, could you help to explain more? do you mean txlock?
the current play_deferred is called under txlock locked, and submit_tx_urb not:
spin_lock_irq(&data->txlock); play_deferred(data); clear_bit(BTUSB_SUSPENDING, &data->flags); spin_unlock_irq(&data->txlock);
spin_unlock_irqrestore(&data->txlock, flags);
if (!suspending) return submit_tx_urb(hdev, urb);
> > Regards > Oliver > > > >
| |