diff mbox series

usb: gadget: f_uac1: add set requests support

Message ID 20220216094301.2448-1-3090101217@zju.edu.cn
State Superseded
Headers show
Series usb: gadget: f_uac1: add set requests support | expand

Commit Message

Jing Leng Feb. 16, 2022, 9:43 a.m. UTC
From: Jing Leng <jleng@ambarella.com>

Currently the f_uac1 driver only supports UAC_SET_CUR request.

But when uac1 device is plugged to Ubuntu 20.04 PC, at the stage
of setup, the PC will send UAC_SET_RES request, If the device
doesn't respond to the request, the PC will abort the setup process
and uac1 device can't be recognized on Ubuntu 20.04 PC.

So f_uac1 driver should handle other set requests.

Signed-off-by: Jing Leng <jleng@ambarella.com>
---
 drivers/usb/gadget/function/f_uac1.c | 44 +++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 8 deletions(-)

Comments

Greg KH Feb. 16, 2022, 10:36 a.m. UTC | #1
On Wed, Feb 16, 2022 at 05:43:01PM +0800, 3090101217@zju.edu.cn wrote:
> From: Jing Leng <jleng@ambarella.com>
> 
> Currently the f_uac1 driver only supports UAC_SET_CUR request.
> 
> But when uac1 device is plugged to Ubuntu 20.04 PC, at the stage
> of setup, the PC will send UAC_SET_RES request, If the device
> doesn't respond to the request, the PC will abort the setup process
> and uac1 device can't be recognized on Ubuntu 20.04 PC.

So is this a bug in the Host side to not do stuff like this?  Why not
fix it there instead?

Where is the requirement that this command must be handled by the
device?

thanks,

greg k-h
Jing Leng Feb. 17, 2022, 1:42 a.m. UTC | #2
Hi Greg KH,

> So is this a bug in the Host side to not do stuff like this?  Why not
> fix it there instead?
> 
> Where is the requirement that this command must be handled by the
> device?
> 

First we need to clarify two issues.

1. Does the Ubuntu go beyond the UAC1 specification?
No. 
On page 66 of the UAC1 specification (
https://www.usb.org/sites/default/files/audio10.pdf):
The bRequest can be SET_CUR, SET_MIN, SET_MAX, SET_RES or SET_MEM.
In most cases, only the CUR and MEM attribute will be supported for
the Set request. However, this specification does not prevent a
designer from making other attributes programmable.
Supplement: Windows 10 only sends SET_CUR request.

2. Does the old version kernel have the problem on the Ubuntu?
NO. (e.g. linux-5.10)
The problem is introduced by the following modification:
    commit 0356e6283c7177391d144612f4b12986ed5c4f6e
    Author: Ruslan Bilovol <ruslan.bilovol@gmail.com>
    Date:   Mon Jul 12 14:55:29 2021 +0200

        usb: gadget: f_uac1: add volume and mute support

Since Ubuntu doesn't go beyond the UAC1 specification and the problem
is introduced by new version kernel, Why don't we perfect it on 
kernel side?

Thanks
Jing Leng
Greg KH Feb. 17, 2022, 3:28 p.m. UTC | #3
On Thu, Feb 17, 2022 at 09:42:00AM +0800, Jing Leng wrote:
> Hi Greg KH,
> 
> > So is this a bug in the Host side to not do stuff like this?  Why not
> > fix it there instead?
> > 
> > Where is the requirement that this command must be handled by the
> > device?
> > 
> 
> First we need to clarify two issues.
> 
> 1. Does the Ubuntu go beyond the UAC1 specification?
> No. 
> On page 66 of the UAC1 specification (
> https://www.usb.org/sites/default/files/audio10.pdf):
> The bRequest can be SET_CUR, SET_MIN, SET_MAX, SET_RES or SET_MEM.
> In most cases, only the CUR and MEM attribute will be supported for
> the Set request. However, this specification does not prevent a
> designer from making other attributes programmable.
> Supplement: Windows 10 only sends SET_CUR request.
> 
> 2. Does the old version kernel have the problem on the Ubuntu?
> NO. (e.g. linux-5.10)
> The problem is introduced by the following modification:
>     commit 0356e6283c7177391d144612f4b12986ed5c4f6e
>     Author: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>     Date:   Mon Jul 12 14:55:29 2021 +0200
> 
>         usb: gadget: f_uac1: add volume and mute support

Then please add this commit id as a "Fixes:" tag in the changelog area.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/f_uac1.c b/drivers/usb/gadget/function/f_uac1.c
index 03f50643fbba..c9d8ec4fdf22 100644
--- a/drivers/usb/gadget/function/f_uac1.c
+++ b/drivers/usb/gadget/function/f_uac1.c
@@ -589,7 +589,7 @@  in_rq_res(struct usb_function *fn, const struct usb_ctrlrequest *cr)
 }
 
 static void
-out_rq_cur_complete(struct usb_ep *ep, struct usb_request *req)
+out_rq_complete(struct usb_ep *ep, struct usb_request *req)
 {
 	struct g_audio *audio = req->context;
 	struct usb_composite_dev *cdev = audio->func.config->cdev;
@@ -614,9 +614,11 @@  out_rq_cur_complete(struct usb_ep *ep, struct usb_request *req)
 			is_playback = 1;
 
 		if (control_selector == UAC_FU_MUTE) {
-			u8 mute = *(u8 *)req->buf;
+			if (cr->bRequest == UAC_SET_CUR) {
+				u8 mute = *(u8 *)req->buf;
 
-			u_audio_set_mute(audio, is_playback, mute);
+				u_audio_set_mute(audio, is_playback, mute);
+			}
 
 			return;
 		} else if (control_selector == UAC_FU_VOLUME) {
@@ -624,7 +626,34 @@  out_rq_cur_complete(struct usb_ep *ep, struct usb_request *req)
 			s16 volume;
 
 			volume = le16_to_cpu(*c);
-			u_audio_set_volume(audio, is_playback, volume);
+
+			switch (cr->bRequest) {
+			case UAC_SET_CUR:
+				u_audio_set_volume(audio, is_playback, volume);
+				break;
+			case UAC_SET_MIN:
+				if (is_playback)
+					opts->p_volume_min = volume;
+				else
+					opts->c_volume_min = volume;
+				break;
+			case UAC_SET_MAX:
+				if (is_playback)
+					opts->p_volume_max = volume;
+				else
+					opts->c_volume_max = volume;
+				break;
+			case UAC_SET_RES:
+				if (is_playback)
+					opts->p_volume_res = volume;
+				else
+					opts->c_volume_res = volume;
+				break;
+			case UAC_SET_MEM:
+				break;
+			default:
+				break;
+			}
 
 			return;
 		} else {
@@ -643,7 +672,7 @@  out_rq_cur_complete(struct usb_ep *ep, struct usb_request *req)
 }
 
 static int
-out_rq_cur(struct usb_function *fn, const struct usb_ctrlrequest *cr)
+ac_rq_out(struct usb_function *fn, const struct usb_ctrlrequest *cr)
 {
 	struct usb_request *req = fn->config->cdev->req;
 	struct g_audio *audio = func_to_g_audio(fn);
@@ -659,7 +688,7 @@  out_rq_cur(struct usb_function *fn, const struct usb_ctrlrequest *cr)
 			(FUOUT_EN(opts) && (entity_id == USB_OUT_FU_ID))) {
 		memcpy(&uac1->setup_cr, cr, sizeof(*cr));
 		req->context = audio;
-		req->complete = out_rq_cur_complete;
+		req->complete = out_rq_complete;
 
 		return w_length;
 	} else {
@@ -789,8 +818,7 @@  f_audio_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 		value = audio_get_endpoint_req(f, ctrl);
 		break;
 	case USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE:
-		if (ctrl->bRequest == UAC_SET_CUR)
-			value = out_rq_cur(f, ctrl);
+		value = ac_rq_out(f, ctrl);
 		break;
 	case USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE:
 		value = ac_rq_in(f, ctrl);