2021-04-19 09:04:40

by Pawel Laszczak

Subject: [PATCH 1/2] usb: gadget: f_uac2: Stop endpoint before enabling it.

From: Pawel Laszczak <[email protected]>

Patch adds disabling endpoint before enabling it during changing
alternate setting. Lack of this functionality causes that in some
cases uac2 queue the same request multiple time.
Such situation can occur when host send set interface with
alternate setting 1 twice.

Signed-off-by: Pawel Laszczak <[email protected]>
---
drivers/usb/gadget/function/f_uac2.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
index 9cc5c512a5cd..7d20a9d8a1b4 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -890,17 +890,17 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
if (intf == uac2->as_out_intf) {
uac2->as_out_alt = alt;

+ u_audio_stop_capture(&uac2->g_audio);
+
if (alt)
ret = u_audio_start_capture(&uac2->g_audio);
- else
- u_audio_stop_capture(&uac2->g_audio);
} else if (intf == uac2->as_in_intf) {
uac2->as_in_alt = alt;

+ u_audio_stop_playback(&uac2->g_audio);
+
if (alt)
ret = u_audio_start_playback(&uac2->g_audio);
- else
- u_audio_stop_playback(&uac2->g_audio);
} else {
dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
return -EINVAL;
--
2.25.1

2021-04-20 01:09:39

by Peter Chen

Subject: Re: [PATCH 1/2] usb: gadget: f_uac2: Stop endpoint before enabling it.

On 21-04-19 09:50:53, Pawel Laszczak wrote:
> From: Pawel Laszczak <[email protected]>
>
> Patch adds disabling endpoint before enabling it during changing
> alternate setting. Lack of this functionality causes that in some
> cases uac2 queue the same request multiple time.
> Such situation can occur when host send set interface with
> alternate setting 1 twice.
>
> Signed-off-by: Pawel Laszczak <[email protected]>
> ---
> drivers/usb/gadget/function/f_uac2.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
> index 9cc5c512a5cd..7d20a9d8a1b4 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
> @@ -890,17 +890,17 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
> if (intf == uac2->as_out_intf) {
> uac2->as_out_alt = alt;
>
> + u_audio_stop_capture(&uac2->g_audio);
> +
> if (alt)
> ret = u_audio_start_capture(&uac2->g_audio);
> - else
> - u_audio_stop_capture(&uac2->g_audio);
> } else if (intf == uac2->as_in_intf) {
> uac2->as_in_alt = alt;
>
> + u_audio_stop_playback(&uac2->g_audio);
> +
> if (alt)
> ret = u_audio_start_playback(&uac2->g_audio);
> - else
> - u_audio_stop_playback(&uac2->g_audio);
> } else {
> dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
> return -EINVAL;

To avoid this, you may use prm->ep_enabled to judge if the endpoint has
already enabled.

--

Thanks,
Peter Chen

2021-04-20 03:57:50

by Pawel Laszczak

Subject: RE: [PATCH 1/2] usb: gadget: f_uac2: Stop endpoint before enabling it.

>On 21-04-19 09:50:53, Pawel Laszczak wrote:
>> From: Pawel Laszczak <[email protected]>
>>
>> Patch adds disabling endpoint before enabling it during changing
>> alternate setting. Lack of this functionality causes that in some
>> cases uac2 queue the same request multiple time.
>> Such situation can occur when host send set interface with
>> alternate setting 1 twice.
>>
>> Signed-off-by: Pawel Laszczak <[email protected]>
>> ---
>> drivers/usb/gadget/function/f_uac2.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
>> index 9cc5c512a5cd..7d20a9d8a1b4 100644
>> --- a/drivers/usb/gadget/function/f_uac2.c
>> +++ b/drivers/usb/gadget/function/f_uac2.c
>> @@ -890,17 +890,17 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
>> if (intf == uac2->as_out_intf) {
>> uac2->as_out_alt = alt;
>>
>> + u_audio_stop_capture(&uac2->g_audio);
>> +
>> if (alt)
>> ret = u_audio_start_capture(&uac2->g_audio);
>> - else
>> - u_audio_stop_capture(&uac2->g_audio);
>> } else if (intf == uac2->as_in_intf) {
>> uac2->as_in_alt = alt;
>>
>> + u_audio_stop_playback(&uac2->g_audio);
>> +
>> if (alt)
>> ret = u_audio_start_playback(&uac2->g_audio);
>> - else
>> - u_audio_stop_playback(&uac2->g_audio);
>> } else {
>> dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
>> return -EINVAL;
>
>To avoid this, you may use prm->ep_enabled to judge if the endpoint has
>already enabled.

Such condition is as first instruction inside u_audio_stop_playback->free_ep function,
so we don't need duplicate it here.

>
>--
>
>Thanks,
>Peter Chen

--

Regards,
Pawe Laszczak

2021-04-23 10:51:01

by Peter Chen

Subject: Re: [PATCH 1/2] usb: gadget: f_uac2: Stop endpoint before enabling it.

On 21-04-20 03:56:25, Pawel Laszczak wrote:
> >On 21-04-19 09:50:53, Pawel Laszczak wrote:
> >> From: Pawel Laszczak <[email protected]>
> >>
> >> Patch adds disabling endpoint before enabling it during changing
> >> alternate setting. Lack of this functionality causes that in some
> >> cases uac2 queue the same request multiple time.
> >> Such situation can occur when host send set interface with
> >> alternate setting 1 twice.
> >>
> >> Signed-off-by: Pawel Laszczak <[email protected]>
> >> ---
> >> drivers/usb/gadget/function/f_uac2.c | 8 ++++----
> >> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
> >> index 9cc5c512a5cd..7d20a9d8a1b4 100644
> >> --- a/drivers/usb/gadget/function/f_uac2.c
> >> +++ b/drivers/usb/gadget/function/f_uac2.c
> >> @@ -890,17 +890,17 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
> >> if (intf == uac2->as_out_intf) {
> >> uac2->as_out_alt = alt;
> >>
> >> + u_audio_stop_capture(&uac2->g_audio);
> >> +
> >> if (alt)
> >> ret = u_audio_start_capture(&uac2->g_audio);
> >> - else
> >> - u_audio_stop_capture(&uac2->g_audio);
> >> } else if (intf == uac2->as_in_intf) {
> >> uac2->as_in_alt = alt;
> >>
> >> + u_audio_stop_playback(&uac2->g_audio);
> >> +
> >> if (alt)
> >> ret = u_audio_start_playback(&uac2->g_audio);
> >> - else
> >> - u_audio_stop_playback(&uac2->g_audio);
> >> } else {
> >> dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
> >> return -EINVAL;
> >
> >To avoid this, you may use prm->ep_enabled to judge if the endpoint has
> >already enabled.
>
> Such condition is as first instruction inside u_audio_stop_playback->free_ep function,
> so we don't need duplicate it here.
>

Get your points, you may add more explanations both the code your change
and the commit log.

--

Thanks,
Peter Chen

2021-04-25 03:31:15

by Peter Chen

Subject: Re: [PATCH 1/2] usb: gadget: f_uac2: Stop endpoint before enabling it.

On 21-04-23 18:49:14, Peter Chen wrote:
> On 21-04-20 03:56:25, Pawel Laszczak wrote:
> > >On 21-04-19 09:50:53, Pawel Laszczak wrote:
> > >> From: Pawel Laszczak <[email protected]>
> > >>
> > >> Patch adds disabling endpoint before enabling it during changing
> > >> alternate setting. Lack of this functionality causes that in some
> > >> cases uac2 queue the same request multiple time.
> > >> Such situation can occur when host send set interface with
> > >> alternate setting 1 twice.
> > >>
> > >> Signed-off-by: Pawel Laszczak <[email protected]>
> > >> ---
> > >> drivers/usb/gadget/function/f_uac2.c | 8 ++++----
> > >> 1 file changed, 4 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
> > >> index 9cc5c512a5cd..7d20a9d8a1b4 100644
> > >> --- a/drivers/usb/gadget/function/f_uac2.c
> > >> +++ b/drivers/usb/gadget/function/f_uac2.c
> > >> @@ -890,17 +890,17 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
> > >> if (intf == uac2->as_out_intf) {
> > >> uac2->as_out_alt = alt;
> > >>
> > >> + u_audio_stop_capture(&uac2->g_audio);
> > >> +
> > >> if (alt)
> > >> ret = u_audio_start_capture(&uac2->g_audio);
> > >> - else
> > >> - u_audio_stop_capture(&uac2->g_audio);
> > >> } else if (intf == uac2->as_in_intf) {
> > >> uac2->as_in_alt = alt;
> > >>
> > >> + u_audio_stop_playback(&uac2->g_audio);
> > >> +
> > >> if (alt)
> > >> ret = u_audio_start_playback(&uac2->g_audio);
> > >> - else
> > >> - u_audio_stop_playback(&uac2->g_audio);
> > >> } else {
> > >> dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
> > >> return -EINVAL;
> > >
> > >To avoid this, you may use prm->ep_enabled to judge if the endpoint has
> > >already enabled.
> >
> > Such condition is as first instruction inside u_audio_stop_playback->free_ep function,
> > so we don't need duplicate it here.
> >
>
> Get your points, you may add more explanations both the code your change
> and the commit log.
>

After thinking more, I think it is better add judge code at
u_audio_start_playback/capture, but not change code here. The reader may
not understand the code your change at the first glance.

--

Thanks,
Peter Chen