LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
To: Ferry Toth <fntoth@gmail.com>,
	Jerome Brunet <jbrunet@baylibre.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>,
	Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Subject: Re: [PATCH] usb: gadget: f_uac2: fixup feedback endpoint stop
Date: Wed, 25 Aug 2021 21:42:52 +0000	[thread overview]
Message-ID: <9709a13f-46f5-ad53-3d19-86b4954bb0f8@synopsys.com> (raw)
In-Reply-To: <79bd686d-5c1f-982c-344a-17c10b64a231@gmail.com>

Ferry Toth wrote:
> Hi,
> 
> Op 25-08-2021 om 11:21 schreef Jerome Brunet:
>> When the uac2 function is stopped, there seems to be an issue with some
>> platforms (Intel Merrifield at least)
>>

The issue isn't hardware specific.

>> BUG: kernel NULL pointer dereference, address: 0000000000000008
>> ...
>> RIP: 0010:dwc3_gadget_del_and_unmap_request+0x19/0xe0
>> ...
>> Call Trace:
>>   dwc3_remove_requests.constprop.0+0x12f/0x170
>>   __dwc3_gadget_ep_disable+0x7a/0x160
>>   dwc3_gadget_ep_disable+0x3d/0xd0
>>   usb_ep_disable+0x1c/0x70
>>   u_audio_stop_capture+0x79/0x120 [u_audio]
>>   afunc_set_alt+0x73/0x80 [usb_f_uac2]
>>   composite_setup+0x224/0x1b90 [libcomposite]
>>
>> The issue happens only when the gadget is using the sync type "async",
>> not
>> "adaptive". This indicates that problem is likely coming from the
>> feedback
>> endpoint, which is only used with async synchronization mode.

This does not describe the actual problem. The problem is that the
usb_ep_dequeue() can be an asynchronous call, and we can't free the
request until its completion (from cancellation).

>>
>> Update the feedback endpoint free function to release the endpoint the
>> same
>> way it is done for the data endpoint.
>>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>
>>   Hi Ferry,
>>
>>   Would you mind trying this before reverting the whole thing ?
>>   The HW I have did not show the issue so far so I can't really check
>>   if it helps. Hopefully, it does ...
> 
> Tested this evening and confirming that this resolves my issue. I can't
> say much about the code itself, maybe Thinh?

Sure. I can take a look.

> 
> Would be great if we could get this in instead of reverting the series.
> 
> Tested-by:  Ferry Toth <ftoth@exalondelft.nl> (dwc3 / Intel Merrifield)
> 
>>   drivers/usb/gadget/function/u_audio.c | 15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> 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?

>> +        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.

BR,
Thinh

  reply	other threads:[~2021-08-25 21:43 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 [this message]
2021-08-26  7:25       ` Jerome Brunet
2021-08-27  0:50         ` Thinh Nguyen
2021-08-27  8:38           ` Jerome Brunet
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=9709a13f-46f5-ad53-3d19-86b4954bb0f8@synopsys.com \
    --to=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=jbrunet@baylibre.com \
    --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).