From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754005AbbJ0Gl6 (ORCPT ); Tue, 27 Oct 2015 02:41:58 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:61457 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753825AbbJ0Gl4 (ORCPT ); Tue, 27 Oct 2015 02:41:56 -0400 X-AuditID: cbfec7f4-f79c56d0000012ee-cd-562f1cb10125 Subject: Re: [PATCH v4 1/7] usb: gadget: f_midi: Transmit data only when IN ep is enabled To: Felipe Tonello , Robert Baldyga References: <1445878509-12084-1-git-send-email-eu@felipetonello.com> <1445878509-12084-2-git-send-email-eu@felipetonello.com> <562EA570.8000309@hackerion.com> Cc: USB list , Kernel development list , Felipe Balbi , Greg Kroah-Hartman , Andrzej Pietrasiewicz , Clemens Ladisch From: Robert Baldyga Message-id: <562F1CAF.7010905@samsung.com> Date: Tue, 27 Oct 2015 07:41:51 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-version: 1.0 In-reply-to: Content-type: text/plain; charset=utf-8 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrJLMWRmVeSWpSXmKPExsVy+t/xa7obZfTDDCbcYrGY9bKdxeLg/XqL JSceMll0zj/LbtG8eD2bxeVdc9gsFi1rZba4++8TowOHx92vD9k9juw8xubxY0U/s8f+uWvY Pfq2rGL0OH5jO5PH501yAexRXDYpqTmZZalF+nYJXBn7/vezF+yXrnh67xNbA+N20S5GTg4J AROJhmV7WCFsMYkL99azdTFycQgJLGWUWN70hAXCecYo8eHnd3aQKmGBaImPM+cydjFycIgI hErM/6gMUfOSUWL/x5OMIA6zQDeTxP2mE2wgDWwCOhJbvk9gBLF5BbQkriyfzARiswioSmyc /hdstahAhMTECQ2sEDWCEj8m32MBsTkFgiXO/l4GtoxZQF1iypRckDCzgLzE5jVvmScwCsxC 0jELoWoWkqoFjMyrGEVTS5MLipPScw31ihNzi0vz0vWS83M3MUIi4MsOxsXHrA4xCnAwKvHw GlTohQmxJpYVV+YeYpTgYFYS4c3/CRTiTUmsrEotyo8vKs1JLT7EKM3BoiTOO3fX+xAhgfTE ktTs1NSC1CKYLBMHp1QDY6zNJEdHtavTz4TYPiqbxLzMhNlMz4qBhXVRr+f5c2U6G/MkYt5N aPxiJ35fgT/1NLsCp9Umd4ukh3HLdr1WV/FVDd7qHPD0lFNsXtL0n++PtSzS2fpXe+rWs5Yl 5Q8m+fDLWAv8uOqw8PQFA/6ckhXVT7QWHf45W26p2TImNn370/Zm93PDlViKMxINtZiLihMB uusQl3wCAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/26/2015 11:49 PM, Felipe Tonello wrote: > Hi Robert, > > On Mon, Oct 26, 2015 at 10:13 PM, Robert Baldyga > wrote: >> Hi Felipe, >> >> On 10/26/2015 05:55 PM, Felipe F. Tonello wrote: >>> This makes sure f_midi doesn't try to enqueue data when the IN endpoint is >>> disabled, ie, USB cable is disconnected. >>> >>> Signed-off-by: Felipe F. Tonello >>> --- >>> drivers/usb/gadget/function/f_midi.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c >>> index edb84ca..e08f365 100644 >>> --- a/drivers/usb/gadget/function/f_midi.c >>> +++ b/drivers/usb/gadget/function/f_midi.c >>> @@ -87,6 +87,7 @@ struct f_midi { >>> int index; >>> char *id; >>> unsigned int buflen, qlen; >>> + bool in_ep_enabled; >> >> It's not necessary, you can use ep->enabled flag instead. > > There is no such flag in usb_ep struct[1]. > > [1] http://lxr.free-electrons.com/source/include/linux/usb/gadget.h#L170 It's already in next branch of Felipe Balbi's tree. Look here: https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/ Best regards, Robert > >> >>> }; >>> >>> static inline struct f_midi *func_to_midi(struct usb_function *f) >>> @@ -332,6 +333,7 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt) >>> err = f_midi_start_ep(midi, f, midi->in_ep); >>> if (err) >>> return err; >>> + midi->in_ep_enabled = true; >>> >>> err = f_midi_start_ep(midi, f, midi->out_ep); >>> if (err) >>> @@ -387,6 +389,8 @@ static void f_midi_disable(struct usb_function *f) >>> */ >>> usb_ep_disable(midi->in_ep); >>> usb_ep_disable(midi->out_ep); >>> + >>> + midi->in_ep_enabled = false; >>> } >>> >>> static int f_midi_snd_free(struct snd_device *device) >>> @@ -543,7 +547,7 @@ static void f_midi_transmit(struct f_midi *midi, struct usb_request *req) >>> } >>> } >>> >>> - if (req->length > 0) { >>> + if (req->length > 0 && midi->in_ep_enabled) { >> >> You should rather test it at the beginning of this function. Or, even >> better, when tasklet is scheduled, because tasklet is the only way this >> function can be called when endpoints are disabled. > > Not in this case, because this function needs to consume the triggered > data from ALSA, otherwise a timeout will happen (which is worse). > Of course this is not the best solution, but it is an incremental improvement. > > Patch 7 has the proper solution, which checks this flag at the > beginning of the function as expected. Also, it is more optimal > because it drops all substreams buffers, instead of copying it. > >> >>> int err; >>> >>> err = usb_ep_queue(ep, req, GFP_ATOMIC); >>> @@ -1158,6 +1162,7 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi) >>> midi->index = opts->index; >>> midi->buflen = opts->buflen; >>> midi->qlen = opts->qlen; >>> + midi->in_ep_enabled = false; >>> ++opts->refcnt; >>> mutex_unlock(&opts->lock); >>> >>> >> > > Felipe > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >