Re: [PATCH 53/82] usb: dwc3: ep0: simplify dwc3_ep0_handle_feature()

2016-11-01 Thread Felipe Balbi 

Hi,

John Youn  writes:
>> +static int dwc3_ep0_handle_device(struct dwc3 *dwc,
>>  struct usb_ctrlrequest *ctrl, int set)
>>  {
>> -struct dwc3_ep  *dep;
>> -u32 recip;
>> +enum usb_device_state   state;
>>  u32 wValue;
>>  u32 wIndex;
>> -u32 reg;
>> -int ret;
>> -enum usb_device_state   state;
>> +int ret = 0;
>>  
>>  wValue = le16_to_cpu(ctrl->wValue);
>>  wIndex = le16_to_cpu(ctrl->wIndex);
>> -recip = ctrl->bRequestType & USB_RECIP_MASK;
>>  state = dwc->gadget.state;
>>  
>> -switch (recip) {
>> -case USB_RECIP_DEVICE:
>> +switch (wValue) {
>> +case USB_DEVICE_REMOTE_WAKEUP:
>> +break;
>> +/*
>> + * 9.4.1 says only only for SS, in AddressState only for
>> + * default control pipe
>> + */
>> +case USB_DEVICE_U1_ENABLE:
>> +ret = dwc3_ep0_handle_u1(dwc, state, set);
>> +break;
>> +case USB_DEVICE_U2_ENABLE:
>> +ret = dwc3_ep0_handle_u2(dwc, state, set);
>> +break;
>> +case USB_DEVICE_LTM_ENABLE:
>> +ret = -EINVAL;
>> +break;
>> +case USB_DEVICE_TEST_MODE:
>> +ret = dwc3_ep0_handle_test(dwc, state, wIndex, set);
>
> Need a break here.
>
> Found with coverity.

nice!! :-) thanks for letting me know. Here's a new version:

8<--
From a9f78f9dcb6698bee03ec9a8eb0c73f08f49c2ee Mon Sep 17 00:00:00 2001
From: Felipe Balbi 
Date: Mon, 3 Oct 2016 12:55:29 +0300
Subject: [PATCH] usb: dwc3: ep0: simplify dwc3_ep0_handle_feature()

By extracting smaller functions from
dwc3_ep0_handle_feature(), it becomes far easier to
understand what's going on. Cleanup only, no
functional changes.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/ep0.c | 257 +++--
 1 file changed, 164 insertions(+), 93 deletions(-)

diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index c562613ccd1a..a1f7c2b4b000 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -371,126 +371,197 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc,
return __dwc3_gadget_ep0_queue(dep, >ep0_usb_req);
 }
 
-static int dwc3_ep0_handle_feature(struct dwc3 *dwc,
+static int dwc3_ep0_handle_u1(struct dwc3 *dwc, enum usb_device_state state,
+   int set)
+{
+   u32 reg;
+
+   if (state != USB_STATE_CONFIGURED)
+   return -EINVAL;
+   if ((dwc->speed != DWC3_DSTS_SUPERSPEED) &&
+   (dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS))
+   return -EINVAL;
+
+   reg = dwc3_readl(dwc->regs, DWC3_DCTL);
+   if (set)
+   reg |= DWC3_DCTL_INITU1ENA;
+   else
+   reg &= ~DWC3_DCTL_INITU1ENA;
+   dwc3_writel(dwc->regs, DWC3_DCTL, reg);
+
+   return 0;
+}
+
+static int dwc3_ep0_handle_u2(struct dwc3 *dwc, enum usb_device_state state,
+   int set)
+{
+   u32 reg;
+
+
+   if (state != USB_STATE_CONFIGURED)
+   return -EINVAL;
+   if ((dwc->speed != DWC3_DSTS_SUPERSPEED) &&
+   (dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS))
+   return -EINVAL;
+
+   reg = dwc3_readl(dwc->regs, DWC3_DCTL);
+   if (set)
+   reg |= DWC3_DCTL_INITU2ENA;
+   else
+   reg &= ~DWC3_DCTL_INITU2ENA;
+   dwc3_writel(dwc->regs, DWC3_DCTL, reg);
+
+   return 0;
+}
+
+static int dwc3_ep0_handle_test(struct dwc3 *dwc, enum usb_device_state state,
+   u32 wIndex, int set)
+{
+   if ((wIndex & 0xff) != 0)
+   return -EINVAL;
+   if (!set)
+   return -EINVAL;
+
+   switch (wIndex >> 8) {
+   case TEST_J:
+   case TEST_K:
+   case TEST_SE0_NAK:
+   case TEST_PACKET:
+   case TEST_FORCE_EN:
+   dwc->test_mode_nr = wIndex >> 8;
+   dwc->test_mode = true;
+   break;
+   default:
+   return -EINVAL;
+   }
+
+   return 0;
+}
+
+static int dwc3_ep0_handle_device(struct dwc3 *dwc,
struct usb_ctrlrequest *ctrl, int set)
 {
-   struct dwc3_ep  *dep;
-   u32 recip;
+   enum usb_device_state   state;
u32 wValue;
u32 wIndex;
-   u32 reg;
-   int ret;
-   enum usb_device_state   state;
+   int ret = 0;
 
wValue = le16_to_cpu(ctrl->wValue);
wIndex = le16_to_cpu(ctrl->wIndex);
-   recip = ctrl->bRequestType & USB_RECIP_MASK;
state = dwc->gadget.state;
 
-   switch (recip) {
-   case USB_RECIP_DEVICE:
+

Re: [PATCH 53/82] usb: dwc3: ep0: simplify dwc3_ep0_handle_feature()

2016-10-31 Thread John Youn 
On 10/31/2016 3:51 AM, Felipe Balbi wrote:
> By extracting smaller functions from
> dwc3_ep0_handle_feature(), it becomes far easier to
> understand what's going on. Cleanup only, no
> functional changes.
> 
> Signed-off-by: Felipe Balbi 
> ---
>  drivers/usb/dwc3/ep0.c | 256 
> +++--
>  1 file changed, 163 insertions(+), 93 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index c562613ccd1a..5e642d65a3b2 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -371,126 +371,196 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc,
>   return __dwc3_gadget_ep0_queue(dep, >ep0_usb_req);
>  }
>  
> -static int dwc3_ep0_handle_feature(struct dwc3 *dwc,
> +static int dwc3_ep0_handle_u1(struct dwc3 *dwc, enum usb_device_state state,
> + int set)
> +{
> + u32 reg;
> +
> + if (state != USB_STATE_CONFIGURED)
> + return -EINVAL;
> + if ((dwc->speed != DWC3_DSTS_SUPERSPEED) &&
> + (dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS))
> + return -EINVAL;
> +
> + reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> + if (set)
> + reg |= DWC3_DCTL_INITU1ENA;
> + else
> + reg &= ~DWC3_DCTL_INITU1ENA;
> + dwc3_writel(dwc->regs, DWC3_DCTL, reg);
> +
> + return 0;
> +}
> +
> +static int dwc3_ep0_handle_u2(struct dwc3 *dwc, enum usb_device_state state,
> + int set)
> +{
> + u32 reg;
> +
> +
> + if (state != USB_STATE_CONFIGURED)
> + return -EINVAL;
> + if ((dwc->speed != DWC3_DSTS_SUPERSPEED) &&
> + (dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS))
> + return -EINVAL;
> +
> + reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> + if (set)
> + reg |= DWC3_DCTL_INITU2ENA;
> + else
> + reg &= ~DWC3_DCTL_INITU2ENA;
> + dwc3_writel(dwc->regs, DWC3_DCTL, reg);
> +
> + return 0;
> +}
> +
> +static int dwc3_ep0_handle_test(struct dwc3 *dwc, enum usb_device_state 
> state,
> + u32 wIndex, int set)
> +{
> + if ((wIndex & 0xff) != 0)
> + return -EINVAL;
> + if (!set)
> + return -EINVAL;
> +
> + switch (wIndex >> 8) {
> + case TEST_J:
> + case TEST_K:
> + case TEST_SE0_NAK:
> + case TEST_PACKET:
> + case TEST_FORCE_EN:
> + dwc->test_mode_nr = wIndex >> 8;
> + dwc->test_mode = true;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int dwc3_ep0_handle_device(struct dwc3 *dwc,
>   struct usb_ctrlrequest *ctrl, int set)
>  {
> - struct dwc3_ep  *dep;
> - u32 recip;
> + enum usb_device_state   state;
>   u32 wValue;
>   u32 wIndex;
> - u32 reg;
> - int ret;
> - enum usb_device_state   state;
> + int ret = 0;
>  
>   wValue = le16_to_cpu(ctrl->wValue);
>   wIndex = le16_to_cpu(ctrl->wIndex);
> - recip = ctrl->bRequestType & USB_RECIP_MASK;
>   state = dwc->gadget.state;
>  
> - switch (recip) {
> - case USB_RECIP_DEVICE:
> + switch (wValue) {
> + case USB_DEVICE_REMOTE_WAKEUP:
> + break;
> + /*
> +  * 9.4.1 says only only for SS, in AddressState only for
> +  * default control pipe
> +  */
> + case USB_DEVICE_U1_ENABLE:
> + ret = dwc3_ep0_handle_u1(dwc, state, set);
> + break;
> + case USB_DEVICE_U2_ENABLE:
> + ret = dwc3_ep0_handle_u2(dwc, state, set);
> + break;
> + case USB_DEVICE_LTM_ENABLE:
> + ret = -EINVAL;
> + break;
> + case USB_DEVICE_TEST_MODE:
> + ret = dwc3_ep0_handle_test(dwc, state, wIndex, set);

Need a break here.

Found with coverity.

Regards,
John



> + default:
> + ret = -EINVAL;
> + }
>  
> - switch (wValue) {
> - case USB_DEVICE_REMOTE_WAKEUP:
> - break;
> - /*
> -  * 9.4.1 says only only for SS, in AddressState only for
> -  * default control pipe
> -  */
> - case USB_DEVICE_U1_ENABLE:
> - if (state != USB_STATE_CONFIGURED)
> - return -EINVAL;
> - if ((dwc->speed != DWC3_DSTS_SUPERSPEED) &&
> - (dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS))
> - return -EINVAL;
> + return ret;
> +}
>  
> - reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> - if (set)
> - reg |= DWC3_DCTL_INITU1ENA;
> - else
> - reg &= ~DWC3_DCTL_INITU1ENA;
> - dwc3_writel(dwc->regs,

[PATCH 53/82] usb: dwc3: ep0: simplify dwc3_ep0_handle_feature()

2016-10-31 Thread Felipe Balbi 
By extracting smaller functions from
dwc3_ep0_handle_feature(), it becomes far easier to
understand what's going on. Cleanup only, no
functional changes.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/ep0.c | 256 +++--
 1 file changed, 163 insertions(+), 93 deletions(-)

diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index c562613ccd1a..5e642d65a3b2 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -371,126 +371,196 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc,
return __dwc3_gadget_ep0_queue(dep, >ep0_usb_req);
 }
 
-static int dwc3_ep0_handle_feature(struct dwc3 *dwc,
+static int dwc3_ep0_handle_u1(struct dwc3 *dwc, enum usb_device_state state,
+   int set)
+{
+   u32 reg;
+
+   if (state != USB_STATE_CONFIGURED)
+   return -EINVAL;
+   if ((dwc->speed != DWC3_DSTS_SUPERSPEED) &&
+   (dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS))
+   return -EINVAL;
+
+   reg = dwc3_readl(dwc->regs, DWC3_DCTL);
+   if (set)
+   reg |= DWC3_DCTL_INITU1ENA;
+   else
+   reg &= ~DWC3_DCTL_INITU1ENA;
+   dwc3_writel(dwc->regs, DWC3_DCTL, reg);
+
+   return 0;
+}
+
+static int dwc3_ep0_handle_u2(struct dwc3 *dwc, enum usb_device_state state,
+   int set)
+{
+   u32 reg;
+
+
+   if (state != USB_STATE_CONFIGURED)
+   return -EINVAL;
+   if ((dwc->speed != DWC3_DSTS_SUPERSPEED) &&
+   (dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS))
+   return -EINVAL;
+
+   reg = dwc3_readl(dwc->regs, DWC3_DCTL);
+   if (set)
+   reg |= DWC3_DCTL_INITU2ENA;
+   else
+   reg &= ~DWC3_DCTL_INITU2ENA;
+   dwc3_writel(dwc->regs, DWC3_DCTL, reg);
+
+   return 0;
+}
+
+static int dwc3_ep0_handle_test(struct dwc3 *dwc, enum usb_device_state state,
+   u32 wIndex, int set)
+{
+   if ((wIndex & 0xff) != 0)
+   return -EINVAL;
+   if (!set)
+   return -EINVAL;
+
+   switch (wIndex >> 8) {
+   case TEST_J:
+   case TEST_K:
+   case TEST_SE0_NAK:
+   case TEST_PACKET:
+   case TEST_FORCE_EN:
+   dwc->test_mode_nr = wIndex >> 8;
+   dwc->test_mode = true;
+   break;
+   default:
+   return -EINVAL;
+   }
+
+   return 0;
+}
+
+static int dwc3_ep0_handle_device(struct dwc3 *dwc,
struct usb_ctrlrequest *ctrl, int set)
 {
-   struct dwc3_ep  *dep;
-   u32 recip;
+   enum usb_device_state   state;
u32 wValue;
u32 wIndex;
-   u32 reg;
-   int ret;
-   enum usb_device_state   state;
+   int ret = 0;
 
wValue = le16_to_cpu(ctrl->wValue);
wIndex = le16_to_cpu(ctrl->wIndex);
-   recip = ctrl->bRequestType & USB_RECIP_MASK;
state = dwc->gadget.state;
 
-   switch (recip) {
-   case USB_RECIP_DEVICE:
+   switch (wValue) {
+   case USB_DEVICE_REMOTE_WAKEUP:
+   break;
+   /*
+* 9.4.1 says only only for SS, in AddressState only for
+* default control pipe
+*/
+   case USB_DEVICE_U1_ENABLE:
+   ret = dwc3_ep0_handle_u1(dwc, state, set);
+   break;
+   case USB_DEVICE_U2_ENABLE:
+   ret = dwc3_ep0_handle_u2(dwc, state, set);
+   break;
+   case USB_DEVICE_LTM_ENABLE:
+   ret = -EINVAL;
+   break;
+   case USB_DEVICE_TEST_MODE:
+   ret = dwc3_ep0_handle_test(dwc, state, wIndex, set);
+   default:
+   ret = -EINVAL;
+   }
 
-   switch (wValue) {
-   case USB_DEVICE_REMOTE_WAKEUP:
-   break;
-   /*
-* 9.4.1 says only only for SS, in AddressState only for
-* default control pipe
-*/
-   case USB_DEVICE_U1_ENABLE:
-   if (state != USB_STATE_CONFIGURED)
-   return -EINVAL;
-   if ((dwc->speed != DWC3_DSTS_SUPERSPEED) &&
-   (dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS))
-   return -EINVAL;
+   return ret;
+}
 
-   reg = dwc3_readl(dwc->regs, DWC3_DCTL);
-   if (set)
-   reg |= DWC3_DCTL_INITU1ENA;
-   else
-   reg &= ~DWC3_DCTL_INITU1ENA;
-   dwc3_writel(dwc->regs, DWC3_DCTL, reg);
-   break;
+static int dwc3_ep0_handle_intf(struct dwc3 *dwc,
+   struct usb_ctrlrequest *ctrl, int set)
+{
+   enum usb_device_state   state;
+