LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jerome Brunet <jbrunet@baylibre.com>
To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	Ferry Toth <fntoth@gmail.com>, Ferry Toth <ftoth@exalondelft.nl>,
	Ruslan Bilovol <ruslan.bilovol@gmail.com>,
	Felipe Balbi <balbi@kernel.org>,
	Pavel Hofman <pavel.hofman@ivitera.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jack Pham <jackp@codeaurora.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>
Subject: Re: [PATCH] usb: gadget: f_uac2: fixup feedback endpoint stop
Date: Fri, 27 Aug 2021 10:38:12 +0200	[thread overview]
Message-ID: <1jk0k7w853.fsf@starbuckisacylon.baylibre.com> (raw)
In-Reply-To: <a2bbb7eb-f952-bc35-6a58-68b15515c716@synopsys.com>


On Fri 27 Aug 2021 at 00:50, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:

>>>>> diff --git a/drivers/usb/gadget/function/u_audio.c
>>>>> b/drivers/usb/gadget/function/u_audio.c
>>>>> index 018dd0978995..63d9340f008e 100644
>>>>> --- a/drivers/usb/gadget/function/u_audio.c
>>>>> +++ b/drivers/usb/gadget/function/u_audio.c
>>>>> @@ -230,7 +230,13 @@ static void u_audio_iso_fback_complete(struct
>>>>> usb_ep *ep,
>>>>>       int status = req->status;
>>>>>         /* i/f shutting down */
>>>>> -    if (!prm->fb_ep_enabled || req->status == -ESHUTDOWN)
>>>>> +    if (!prm->fb_ep_enabled) {
>>>
>>> prm->fb_ep_enabled is not protected. Potential race problem here?
>> 
>> Given how the variable is used, I don't think so.
>> Could you please detail ?
>> 
>
> I'm thinking of this scenario:
>
> Since free_ep_fback() is called in a separate thread,
> u_audio_iso_fback_complete() can come in the middle after
> prm->fb_ep_enabled is cleared but before the usb_ep_dequeue(). So the
> request may be freed before being accessed again in usb_ep_dequeue().
>

You are right - there is short window of opportunity.
Looks like a 2nd separate issue affecting all u_audio endpoints. (Would
be solved by reverting the feedback series)

Fix seems fairly simple.

>> (I don't think this is really related to the current problem though)
>
> It's related to how we go about to solve the problem right?
>
> Can we just check for the req->status for -ECONNRESET and -ESHUTDOWN and
> free the request? Those 2 statuses should be for when the request is
> cancelled and when the endpoint is disabled.
>

The same problem happened on the data endpoint while feedback was being
reviewed. This is why data got fixed, and the feedback endpoint did not.

As explained in the description, the fix is merely aligning what is
done for the different endpoints here.

Some of the people who have worked on this are no USB expert (myself
included) so I'm sure things can be improved. I'd suggest to take the
rework u_audio as a 2nd step.

>> 
>>>
>>>>> +        kfree(req->buf);
>>>>> +        usb_ep_free_request(ep, req);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    if (req->status == -ESHUTDOWN)
>>>>>           return;
>>>>>         /*
>>>>> @@ -421,9 +427,10 @@ static inline void free_ep_fback(struct
>>>>> uac_rtd_params *prm, struct usb_ep *ep)
>>>>>       prm->fb_ep_enabled = false;
>>>>>         if (prm->req_fback) {
>>>>> -        usb_ep_dequeue(ep, prm->req_fback);
>>>>> -        kfree(prm->req_fback->buf);
>>>>> -        usb_ep_free_request(ep, prm->req_fback);
>>>>> +        if (usb_ep_dequeue(ep, prm->req_fback)) {
>>>>> +            kfree(prm->req_fback->buf);
>>>>> +            usb_ep_free_request(ep, prm->req_fback);
>>>>> +        }
>>>>>           prm->req_fback = NULL;
>>>>>       }
>>>>>   
>>>
>>> On a separate note, I notice that f_uac2 only queues a single feedback
>>> request at a time for isoc endpoint? Even though the interval is 1ms,
>>> this will easily cause data drop.
>>>
>>> Also, you're ignoring other request error status and still processing
>>> bogus data on request completion? That doesn't seem right.
>> 
>> Gadget is sendind the feedback data, not processing it. Every data sent
>
> Ah ok.. I missed that it's IN request.
>
>> is OK.  Yes, packets can be missed with the current implementation,
>> meaning the feedback value is not reported as often as initially
>> intended. On slower HW, packets are also missed with 2 requests queued,
>
> 2 is still too low. From our HW testing, minimum should be 4 to minimize
> data loss at 1ms interval.

Maybe 4 should be the default then ?

>
>> not only on the feedback endpoint but also on the playback
>> endpoint. Picking the approriate value is not straight forward. For the
>> feedback endpoint it isn't big deal because, according to the spec, if
>> the feedback is not sent, the host shall assume the value hasn't
>> changed. This why the whole thing works as it is.
>
> Why do this when we can avoid it.

Because it was not known at the time

> We know that there will be data drop
> with the way it's implemented now. There's no option to adjust the
> number of feedback requests either.
>
>> 
>> I admit things still aren't perfect, but there is progress ...
>> 
> Thanks for the updates to audio/f_uac2. It's been a long time since
> anyone working on it.
>
> BR,
> Thinh


  reply	other threads:[~2021-08-27  9:26 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-24 20:14 [PATCH v1 1/3] Revert "usb: gadget: u_audio: add real feedback implementation" Ferry Toth
2021-08-24 20:14 ` [PATCH v1 2/3] Revert "usb: gadget: f_uac2: add adaptive sync support for capture" Ferry Toth
2021-08-25  5:53   ` Felipe Balbi
2021-08-24 20:14 ` [PATCH v1 3/3] Revert "usb: gadget: f_uac2/u_audio: add feedback endpoint support" Ferry Toth
2021-08-25  5:54   ` Felipe Balbi
2021-08-25  5:53 ` [PATCH v1 1/3] Revert "usb: gadget: u_audio: add real feedback implementation" Felipe Balbi
2021-08-25  7:16   ` Ferry Toth
2021-08-25  9:21 ` [PATCH] usb: gadget: f_uac2: fixup feedback endpoint stop Jerome Brunet
2021-08-25 20:07   ` Ferry Toth
2021-08-25 21:42     ` Thinh Nguyen
2021-08-26  7:25       ` Jerome Brunet
2021-08-27  0:50         ` Thinh Nguyen
2021-08-27  8:38           ` Jerome Brunet [this message]
2021-08-27 23:30             ` Thinh Nguyen
2021-08-26  7:23     ` Jerome Brunet
2021-08-27  7:58 Jerome Brunet
2021-08-27  8:05 ` Felipe Balbi
2021-08-27  9:26   ` Jerome Brunet

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1jk0k7w853.fsf@starbuckisacylon.baylibre.com \
    --to=jbrunet@baylibre.com \
    --cc=Thinh.Nguyen@synopsys.com \
    --cc=balbi@kernel.org \
    --cc=fntoth@gmail.com \
    --cc=ftoth@exalondelft.nl \
    --cc=gregkh@linuxfoundation.org \
    --cc=jackp@codeaurora.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=pavel.hofman@ivitera.com \
    --cc=ruslan.bilovol@gmail.com \
    --subject='Re: [PATCH] usb: gadget: f_uac2: fixup feedback endpoint stop' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).