diff mbox series

[v2] usb: dwc2: Fix SET/CLEAR_FEATURE and GET_STATUS flows

Message ID 38a93a8deedd76dbc22bb1e41b4fc998f3750c95.1576516371.git.hminas@synopsys.com (mailing list archive)
State Superseded
Headers show
Series [v2] usb: dwc2: Fix SET/CLEAR_FEATURE and GET_STATUS flows | expand

Commit Message

Minas Harutyunyan Dec. 16, 2019, 5:14 p.m. UTC
SET/CLEAR_FEATURE for Remote Wakeup allowance not handled correctly.
GET_STATUS handling provided not correct data on DATA Stage.
Issue seen when gadget's dr_mode set to "otg" mode and connected
to MacOS.
Both are fixed and tested using USBCV Ch.9 tests.

Signed-off-by: Minas Harutyunyan <hminas@synopsys.com>
Fixes: fa389a6d7726 ("usb: dwc2: gadget: Add remote_wakeup_allowed flag")
Tested-by: Jack Mitchell <ml@embed.me.uk>
Cc: stable@vger.kernel.org
---
Changes in v2:
- Add Fixes tag
- Add Tested-by tag

Signed-off-by: Minas Harutyunyan <hminas@synopsys.com>
---
 drivers/usb/dwc2/gadget.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

Comments

Minas Harutyunyan Jan. 15, 2020, 1:21 p.m. UTC | #1
Hi Felipe,

On 12/16/2019 9:14 PM, Minas Harutyunyan wrote:
> SET/CLEAR_FEATURE for Remote Wakeup allowance not handled correctly.
> GET_STATUS handling provided not correct data on DATA Stage.
> Issue seen when gadget's dr_mode set to "otg" mode and connected
> to MacOS.
> Both are fixed and tested using USBCV Ch.9 tests.
> 
> Signed-off-by: Minas Harutyunyan <hminas@synopsys.com>
> Fixes: fa389a6d7726 ("usb: dwc2: gadget: Add remote_wakeup_allowed flag")
> Tested-by: Jack Mitchell <ml@embed.me.uk>
> Cc: stable@vger.kernel.org
> ---
> Changes in v2:
> - Add Fixes tag
> - Add Tested-by tag
> 
> Signed-off-by: Minas Harutyunyan <hminas@synopsys.com>
> ---
>   drivers/usb/dwc2/gadget.c | 28 ++++++++++++++++------------
>   1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 6be10e496e10..3a6176c22371 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -1632,6 +1632,7 @@ static int dwc2_hsotg_process_req_status(struct dwc2_hsotg *hsotg,
>   	struct dwc2_hsotg_ep *ep0 = hsotg->eps_out[0];
>   	struct dwc2_hsotg_ep *ep;
>   	__le16 reply;
> +	u16 status;
>   	int ret;
>   
>   	dev_dbg(hsotg->dev, "%s: USB_REQ_GET_STATUS\n", __func__);
> @@ -1643,11 +1644,10 @@ static int dwc2_hsotg_process_req_status(struct dwc2_hsotg *hsotg,
>   
>   	switch (ctrl->bRequestType & USB_RECIP_MASK) {
>   	case USB_RECIP_DEVICE:
> -		/*
> -		 * bit 0 => self powered
> -		 * bit 1 => remote wakeup
> -		 */
> -		reply = cpu_to_le16(0);
> +		status = 1 << USB_DEVICE_SELF_POWERED;
> +		status |= hsotg->remote_wakeup_allowed <<
> +			  USB_DEVICE_REMOTE_WAKEUP;
> +		reply = cpu_to_le16(status);
>   		break;
>   
>   	case USB_RECIP_INTERFACE:
> @@ -1758,7 +1758,10 @@ static int dwc2_hsotg_process_req_feature(struct dwc2_hsotg *hsotg,
>   	case USB_RECIP_DEVICE:
>   		switch (wValue) {
>   		case USB_DEVICE_REMOTE_WAKEUP:
> -			hsotg->remote_wakeup_allowed = 1;
> +			if (set)
> +				hsotg->remote_wakeup_allowed = 1;
> +			else
> +				hsotg->remote_wakeup_allowed = 0;
>   			break;
>   
>   		case USB_DEVICE_TEST_MODE:
> @@ -1768,16 +1771,17 @@ static int dwc2_hsotg_process_req_feature(struct dwc2_hsotg *hsotg,
>   				return -EINVAL;
>   
>   			hsotg->test_mode = wIndex >> 8;
> -			ret = dwc2_hsotg_send_reply(hsotg, ep0, NULL, 0);
> -			if (ret) {
> -				dev_err(hsotg->dev,
> -					"%s: failed to send reply\n", __func__);
> -				return ret;
> -			}
>   			break;
>   		default:
>   			return -ENOENT;
>   		}
> +
> +		ret = dwc2_hsotg_send_reply(hsotg, ep0, NULL, 0);
> +		if (ret) {
> +			dev_err(hsotg->dev,
> +				"%s: failed to send reply\n", __func__);
> +			return ret;
> +		}
>   		break;
>   
>   	case USB_RECIP_ENDPOINT:
> 

What about this patch?



Thanks,
Minas
Felipe Balbi Jan. 15, 2020, 1:27 p.m. UTC | #2
Hi,

Minas Harutyunyan <Minas.Harutyunyan@synopsys.com> writes:
>> @@ -1768,16 +1771,17 @@ static int dwc2_hsotg_process_req_feature(struct dwc2_hsotg *hsotg,
>>   				return -EINVAL;
>>   
>>   			hsotg->test_mode = wIndex >> 8;
>> -			ret = dwc2_hsotg_send_reply(hsotg, ep0, NULL, 0);
>> -			if (ret) {
>> -				dev_err(hsotg->dev,
>> -					"%s: failed to send reply\n", __func__);
>> -				return ret;
>> -			}
>>   			break;
>>   		default:
>>   			return -ENOENT;
>>   		}
>> +
>> +		ret = dwc2_hsotg_send_reply(hsotg, ep0, NULL, 0);
>> +		if (ret) {
>> +			dev_err(hsotg->dev,
>> +				"%s: failed to send reply\n", __func__);
>> +			return ret;
>> +		}
>>   		break;
>>   
>>   	case USB_RECIP_ENDPOINT:
>> 
>
> What about this patch?

See:

https://lore.kernel.org/linux-usb/875zhd6pw0.fsf@kernel.org/T/#u
diff mbox series

Patch

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 6be10e496e10..3a6176c22371 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -1632,6 +1632,7 @@  static int dwc2_hsotg_process_req_status(struct dwc2_hsotg *hsotg,
 	struct dwc2_hsotg_ep *ep0 = hsotg->eps_out[0];
 	struct dwc2_hsotg_ep *ep;
 	__le16 reply;
+	u16 status;
 	int ret;
 
 	dev_dbg(hsotg->dev, "%s: USB_REQ_GET_STATUS\n", __func__);
@@ -1643,11 +1644,10 @@  static int dwc2_hsotg_process_req_status(struct dwc2_hsotg *hsotg,
 
 	switch (ctrl->bRequestType & USB_RECIP_MASK) {
 	case USB_RECIP_DEVICE:
-		/*
-		 * bit 0 => self powered
-		 * bit 1 => remote wakeup
-		 */
-		reply = cpu_to_le16(0);
+		status = 1 << USB_DEVICE_SELF_POWERED;
+		status |= hsotg->remote_wakeup_allowed <<
+			  USB_DEVICE_REMOTE_WAKEUP;
+		reply = cpu_to_le16(status);
 		break;
 
 	case USB_RECIP_INTERFACE:
@@ -1758,7 +1758,10 @@  static int dwc2_hsotg_process_req_feature(struct dwc2_hsotg *hsotg,
 	case USB_RECIP_DEVICE:
 		switch (wValue) {
 		case USB_DEVICE_REMOTE_WAKEUP:
-			hsotg->remote_wakeup_allowed = 1;
+			if (set)
+				hsotg->remote_wakeup_allowed = 1;
+			else
+				hsotg->remote_wakeup_allowed = 0;
 			break;
 
 		case USB_DEVICE_TEST_MODE:
@@ -1768,16 +1771,17 @@  static int dwc2_hsotg_process_req_feature(struct dwc2_hsotg *hsotg,
 				return -EINVAL;
 
 			hsotg->test_mode = wIndex >> 8;
-			ret = dwc2_hsotg_send_reply(hsotg, ep0, NULL, 0);
-			if (ret) {
-				dev_err(hsotg->dev,
-					"%s: failed to send reply\n", __func__);
-				return ret;
-			}
 			break;
 		default:
 			return -ENOENT;
 		}
+
+		ret = dwc2_hsotg_send_reply(hsotg, ep0, NULL, 0);
+		if (ret) {
+			dev_err(hsotg->dev,
+				"%s: failed to send reply\n", __func__);
+			return ret;
+		}
 		break;
 
 	case USB_RECIP_ENDPOINT: