Discussion:
[PATCH v2 1/2] usb: dwc2: gadget: modify return statement
Sudip Mukherjee
2014-10-16 07:41:09 UTC
Permalink
modified the function to have a single return statement at the end
instead of multiple return statement in the middle of the function
to improve the readability of the code.

Signed-off-by: Sudip Mukherjee <***@vectorindia.org>
---
drivers/usb/dwc2/gadget.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 7b5856f..af5517f 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2471,7 +2471,8 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
dir_in = (desc->bEndpointAddress & USB_ENDPOINT_DIR_MASK) ? 1 : 0;
if (dir_in != hs_ep->dir_in) {
dev_err(hsotg->dev, "%s: direction mismatch!\n", __func__);
- return -EINVAL;
+ ret = -EINVAL;
+ goto error1;
}

mps = usb_endpoint_maxp(desc);
@@ -2561,8 +2562,10 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
hs_ep->fifo_size = val;
break;
}
- if (i == 8)
- return -ENOMEM;
+ if (i == 8) {
+ ret = -ENOMEM;
+ goto error1;
+ }
}

/* for non control endpoints, set PID to D0 */
@@ -2580,6 +2583,8 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
s3c_hsotg_ctrl_epint(hsotg, index, dir_in, 1);

spin_unlock_irqrestore(&hsotg->lock, flags);
+
+error1:
return ret;
}
--
1.8.1.2
Sudip Mukherjee
2014-10-16 07:41:10 UTC
Permalink
sparse was giving the following warning:
warning: context imbalance in 's3c_hsotg_ep_enable'
- different lock contexts for basic block

we were returning ENOMEM while still holding the spinlock.
The sparse warning was fixed by releasing the spinlock before return.

This patch depends on the previous patch of the series.

Signed-off-by: Sudip Mukherjee <***@vectorindia.org>
---
drivers/usb/dwc2/gadget.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index af5517f..05a9a23 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2564,7 +2564,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
}
if (i == 8) {
ret = -ENOMEM;
- goto error1;
+ goto error2;
}
}

@@ -2582,6 +2582,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
/* enable the endpoint interrupt */
s3c_hsotg_ctrl_epint(hsotg, index, dir_in, 1);

+error2:
spin_unlock_irqrestore(&hsotg->lock, flags);

error1:
--
1.8.1.2
Felipe Balbi
2014-10-16 13:21:55 UTC
Permalink
HI,
Post by Sudip Mukherjee
warning: context imbalance in 's3c_hsotg_ep_enable'
- different lock contexts for basic block
we were returning ENOMEM while still holding the spinlock.
The sparse warning was fixed by releasing the spinlock before return.
This patch depends on the previous patch of the series.
this should be patch one so it can be backported to stable kernels.
--
balbi
Sudip Mukherjee
2014-10-16 14:52:08 UTC
Permalink
Post by Felipe Balbi
HI,
Post by Sudip Mukherjee
warning: context imbalance in 's3c_hsotg_ep_enable'
- different lock contexts for basic block
we were returning ENOMEM while still holding the spinlock.
The sparse warning was fixed by releasing the spinlock before return.
This patch depends on the previous patch of the series.
this should be patch one so it can be backported to stable kernels.
my v1 patch fixed only this , while reviewing that one Paul Zimmerman suggested to rewrite the return statements.
so this v2 series had the rewrite and the spinlock error fix.
now if this is to be made the patch one then it will be a duplicate of my v1 followed by another patch for return statements.
should i do that ?

thanks
sudip
Post by Felipe Balbi
--
balbi
Felipe Balbi
2014-10-16 14:57:52 UTC
Permalink
Post by Sudip Mukherjee
Post by Felipe Balbi
HI,
Post by Sudip Mukherjee
warning: context imbalance in 's3c_hsotg_ep_enable'
- different lock contexts for basic block
we were returning ENOMEM while still holding the spinlock.
The sparse warning was fixed by releasing the spinlock before return.
This patch depends on the previous patch of the series.
this should be patch one so it can be backported to stable kernels.
my v1 patch fixed only this , while reviewing that one Paul Zimmerman
suggested to rewrite the return statements.
so this v2 series had the rewrite and the spinlock error fix.
now if this is to be made the patch one then it will be a duplicate of
my v1 followed by another patch for return statements.
should i do that ?
Paul has the final word on this driver, if he already asked you to
change, I withdraw my comment ;-)

cheers
--
balbi
Paul Zimmerman
2014-10-16 18:25:35 UTC
Permalink
Sent: Thursday, October 16, 2014 7:52 AM
Post by Felipe Balbi
HI,
Post by Sudip Mukherjee
warning: context imbalance in 's3c_hsotg_ep_enable'
- different lock contexts for basic block
we were returning ENOMEM while still holding the spinlock.
The sparse warning was fixed by releasing the spinlock before return.
This patch depends on the previous patch of the series.
this should be patch one so it can be backported to stable kernels.
my v1 patch fixed only this , while reviewing that one Paul Zimmerman suggested to rewrite the return
statements.
so this v2 series had the rewrite and the spinlock error fix.
now if this is to be made the patch one then it will be a duplicate of my v1 followed by another patch
for return statements.
should i do that ?
Hi Sudip,

Please make the first patch like I showed in my previous reply. Then we
can mark that one for stable to fix the bug. Then make a second patch to
change the other error path.
--
Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Loading...