Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1230797 > unrolled thread

[PATCH 2/3] usb: gadget: f_midi: free usb request when done

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/3] usb: gadget: f_midi: free usb request when done "Felipe F. Tonello" <eu@felipetonello.com> - 2015-09-22 21:10 +0200
    Re: [PATCH 2/3] usb: gadget: f_midi: free usb request when done Felipe Balbi <balbi@ti.com> - 2015-09-22 23:20 +0200
    Re: [PATCH 2/3] usb: gadget: f_midi: free usb request when done Felipe Tonello <eu@felipetonello.com> - 2015-09-23 13:50 +0200
      Re: [PATCH 2/3] usb: gadget: f_midi: free usb request when done Alan Stern <stern@rowland.harvard.edu> - 2015-09-23 16:40 +0200
        Re: [PATCH 2/3] usb: gadget: f_midi: free usb request when done Felipe Tonello <eu@felipetonello.com> - 2015-09-23 16:50 +0200

#1230797 — [PATCH 2/3] usb: gadget: f_midi: free usb request when done

req->actual == req->length means that there is no data left to enqueue,
so free the request.

Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 drivers/usb/gadget/function/f_midi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index edb84ca..e92aff5 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -258,7 +258,10 @@ f_midi_complete(struct usb_ep *ep, struct usb_request *req)
 		} else if (ep == midi->in_ep) {
 			/* Our transmit completed. See if there's more to go.
 			 * f_midi_transmit eats req, don't queue it again. */
-			f_midi_transmit(midi, req);
+			if (req->actual < req->length)
+				f_midi_transmit(midi, req);
+			else
+				free_ep_req(ep, req);
 			return;
 		}
 		break;
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1230963

[Multipart message — attachments visible in raw view] — view raw

On Tue, Sep 22, 2015 at 07:59:09PM +0100, Felipe F. Tonello wrote:
> req->actual == req->length means that there is no data left to enqueue,
> so free the request.
> 
> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
> ---
>  drivers/usb/gadget/function/f_midi.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index edb84ca..e92aff5 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -258,7 +258,10 @@ f_midi_complete(struct usb_ep *ep, struct usb_request *req)
>  		} else if (ep == midi->in_ep) {
>  			/* Our transmit completed. See if there's more to go.
>  			 * f_midi_transmit eats req, don't queue it again. */
> -			f_midi_transmit(midi, req);
> +			if (req->actual < req->length)
> +				f_midi_transmit(midi, req);
> +			else
> +				free_ep_req(ep, req);

I'd have to have a deeper look at f_midi, but this doesn't look
correct to me. Why do you think that we should stop queueing
requests when we transfer one in full ?

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1231379

Hi Peter,

On Wed, Sep 23, 2015 at 4:10 AM, Peter Chen <peter.chen@freescale.com> wrote:
> On Tue, Sep 22, 2015 at 07:59:09PM +0100, Felipe F. Tonello wrote:
>> req->actual == req->length means that there is no data left to enqueue,
>> so free the request.
>>
>> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>> index edb84ca..e92aff5 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -258,7 +258,10 @@ f_midi_complete(struct usb_ep *ep, struct usb_request *req)
>>               } else if (ep == midi->in_ep) {
>>                       /* Our transmit completed. See if there's more to go.
>>                        * f_midi_transmit eats req, don't queue it again. */
>> -                     f_midi_transmit(midi, req);
>> +                     if (req->actual < req->length)
>> +                             f_midi_transmit(midi, req);
>> +                     else
>> +                             free_ep_req(ep, req);
>>                       return;
>>               }
>
> It is incorrect, if no reqeust in queue, how device knows when
> the host sends data?

This is the complete function of the IN endpoint.

Actually I believe the proper patch is to enqueue this request again
if req->actual < req->length is true. Because the data is still there,
just not fully completed. Asking to transmit the request again will
cause to read new data from ALSA MIDI module, which it can possibly
steal data from a real ALSA request from f_midi_in_trigger. If that
doesn't happen (req->length == 0), the request will be freed anyway.

Any thoughts?

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231478

On Wed, 23 Sep 2015, Felipe Tonello wrote:

> Hi Peter,
> 
> On Wed, Sep 23, 2015 at 4:10 AM, Peter Chen <peter.chen@freescale.com> wrote:
> > On Tue, Sep 22, 2015 at 07:59:09PM +0100, Felipe F. Tonello wrote:
> >> req->actual == req->length means that there is no data left to enqueue,
> >> so free the request.
> >>
> >> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
> >> ---
> >>  drivers/usb/gadget/function/f_midi.c | 5 ++++-
> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> >> index edb84ca..e92aff5 100644
> >> --- a/drivers/usb/gadget/function/f_midi.c
> >> +++ b/drivers/usb/gadget/function/f_midi.c
> >> @@ -258,7 +258,10 @@ f_midi_complete(struct usb_ep *ep, struct usb_request *req)
> >>               } else if (ep == midi->in_ep) {
> >>                       /* Our transmit completed. See if there's more to go.
> >>                        * f_midi_transmit eats req, don't queue it again. */
> >> -                     f_midi_transmit(midi, req);
> >> +                     if (req->actual < req->length)
> >> +                             f_midi_transmit(midi, req);
> >> +                     else
> >> +                             free_ep_req(ep, req);
> >>                       return;
> >>               }
> >
> > It is incorrect, if no reqeust in queue, how device knows when
> > the host sends data?
> 
> This is the complete function of the IN endpoint.
> 
> Actually I believe the proper patch is to enqueue this request again
> if req->actual < req->length is true. Because the data is still there,
> just not fully completed. Asking to transmit the request again will
> cause to read new data from ALSA MIDI module, which it can possibly
> steal data from a real ALSA request from f_midi_in_trigger. If that
> doesn't happen (req->length == 0), the request will be freed anyway.
> 
> Any thoughts?

Please pardon me for jumping in in the middle of a conversation.  I 
know practically zero about f_midi.  But nevertheless...

How can you ever have req->actual < req->length for a usb_request on an
IN endpoint?  The only way that can happen is if some sort of error or
exceptional event occurred, for example, if the transfer was cancelled
before it could run to completion.  In such cases I doubt that you
really want to retransmit the data.  Particularly since part of it
probably was received by the host -- do you really want to send that
part of the data a second time?

Don't bother to answer if this doesn't make any sense...

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231483

Hi Alan,

On Wed, Sep 23, 2015 at 3:30 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 23 Sep 2015, Felipe Tonello wrote:
>
>> Hi Peter,
>>
>> On Wed, Sep 23, 2015 at 4:10 AM, Peter Chen <peter.chen@freescale.com> wrote:
>> > On Tue, Sep 22, 2015 at 07:59:09PM +0100, Felipe F. Tonello wrote:
>> >> req->actual == req->length means that there is no data left to enqueue,
>> >> so free the request.
>> >>
>> >> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
>> >> ---
>> >>  drivers/usb/gadget/function/f_midi.c | 5 ++++-
>> >>  1 file changed, 4 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>> >> index edb84ca..e92aff5 100644
>> >> --- a/drivers/usb/gadget/function/f_midi.c
>> >> +++ b/drivers/usb/gadget/function/f_midi.c
>> >> @@ -258,7 +258,10 @@ f_midi_complete(struct usb_ep *ep, struct usb_request *req)
>> >>               } else if (ep == midi->in_ep) {
>> >>                       /* Our transmit completed. See if there's more to go.
>> >>                        * f_midi_transmit eats req, don't queue it again. */
>> >> -                     f_midi_transmit(midi, req);
>> >> +                     if (req->actual < req->length)
>> >> +                             f_midi_transmit(midi, req);
>> >> +                     else
>> >> +                             free_ep_req(ep, req);
>> >>                       return;
>> >>               }
>> >
>> > It is incorrect, if no reqeust in queue, how device knows when
>> > the host sends data?
>>
>> This is the complete function of the IN endpoint.
>>
>> Actually I believe the proper patch is to enqueue this request again
>> if req->actual < req->length is true. Because the data is still there,
>> just not fully completed. Asking to transmit the request again will
>> cause to read new data from ALSA MIDI module, which it can possibly
>> steal data from a real ALSA request from f_midi_in_trigger. If that
>> doesn't happen (req->length == 0), the request will be freed anyway.
>>
>> Any thoughts?
>
> Please pardon me for jumping in in the middle of a conversation.  I
> know practically zero about f_midi.  But nevertheless...
>
> How can you ever have req->actual < req->length for a usb_request on an
> IN endpoint?  The only way that can happen is if some sort of error or
> exceptional event occurred, for example, if the transfer was cancelled
> before it could run to completion.  In such cases I doubt that you
> really want to retransmit the data.  Particularly since part of it
> probably was received by the host -- do you really want to send that
> part of the data a second time?

That is a fair point.

IMO we should always free the request upon a completion. Never
retransmit, since ALSA trigger will do that anyway.

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web