| Message ID | 20210419075053.28467-1-pawell@gli-login.cadence.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] usb: gadget: f_uac2: Stop endpoint before enabling it. | expand |
On 21-04-19 09:50:53, Pawel Laszczak wrote: > From: Pawel Laszczak <pawell@cadence.com> > > 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 <pawell@cadence.com> > --- > 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.
>On 21-04-19 09:50:53, Pawel Laszczak wrote: >> From: Pawel Laszczak <pawell@cadence.com> >> >> 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 <pawell@cadence.com> >> --- >> 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
On 21-04-20 03:56:25, Pawel Laszczak wrote: > >On 21-04-19 09:50:53, Pawel Laszczak wrote: > >> From: Pawel Laszczak <pawell@cadence.com> > >> > >> 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 <pawell@cadence.com> > >> --- > >> 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.
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 <pawell@cadence.com> > > >> > > >> 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 <pawell@cadence.com> > > >> --- > > >> 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.
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;