Discussion:
[PATCH v3 1/2] usb: dwc2: gadget: sparse warning of context imbalance
(too old to reply)
Sudip Mukherjee
2014-10-17 04:44:02 UTC
Permalink
Raw Message
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.

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

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 7b5856f..7f25527 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2561,8 +2561,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 error;
+ }
}

/* for non control endpoints, set PID to D0 */
@@ -2579,6 +2581,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
/* enable the endpoint interrupt */
s3c_hsotg_ctrl_epint(hsotg, index, dir_in, 1);

+error:
spin_unlock_irqrestore(&hsotg->lock, flags);
return ret;
}
--
1.8.1.2
Sudip Mukherjee
2014-10-17 04:44:03 UTC
Permalink
Raw Message
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.

This patch depends on the previous patch of the series.

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

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 7f25527..e8a8fc7 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);
@@ -2583,6 +2584,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,

error:
spin_unlock_irqrestore(&hsotg->lock, flags);
+error1:
return ret;
}
--
1.8.1.2
David Laight
2014-10-17 09:02:00 UTC
Permalink
Raw Message
From: Of Sudip Mukherjee
Post by Sudip Mukherjee
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.
Many of us would disagree with you there.
Early returns actually make the code easier to read, certainly
better than a goto 'end of function'.

David
Post by Sudip Mukherjee
This patch depends on the previous patch of the series.
---
drivers/usb/dwc2/gadget.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 7f25527..e8a8fc7 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);
@@ -2583,6 +2584,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
spin_unlock_irqrestore(&hsotg->lock, flags);
return ret;
}
--
1.8.1.2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sudip Mukherjee
2014-10-17 10:03:25 UTC
Permalink
Raw Message
Post by David Laight
From: Of Sudip Mukherjee
Post by Sudip Mukherjee
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.
Many of us would disagree with you there.
Early returns actually make the code easier to read, certainly
better than a goto 'end of function'.
For a long function like this, I'd rather keep a single return point at
the end.
so I thought he meant all the return statements in the function.

thanks
sudip
Post by David Laight
David
Post by Sudip Mukherjee
This patch depends on the previous patch of the series.
---
drivers/usb/dwc2/gadget.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 7f25527..e8a8fc7 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);
@@ -2583,6 +2584,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
spin_unlock_irqrestore(&hsotg->lock, flags);
return ret;
}
--
1.8.1.2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
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
Paul Zimmerman
2014-10-17 18:10:54 UTC
Permalink
Raw Message
Sent: Friday, October 17, 2014 3:03 AM
Post by David Laight
From: Of Sudip Mukherjee
Post by Sudip Mukherjee
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.
Many of us would disagree with you there.
Early returns actually make the code easier to read, certainly
better than a goto 'end of function'.
actually , frankly speaking, this first return statement was also easier for me to understand. But in
Post by David Laight
For a long function like this, I'd rather keep a single return point at
the end.
so I thought he meant all the return statements in the function.
What I didn't like about your first patch was that there were two
places where the spinlock was released. I think that is error-prone,
as can be seen by the original bug. But I am OK with leaving the
first return statement as-is, since the spinlock is not held there.

So I think we should apply patch 1, and drop patch 2.
--
Paul
Paul Zimmerman
2014-10-17 18:52:37 UTC
Permalink
Raw Message
Sent: Thursday, October 16, 2014 9:44 PM
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.
This patch depends on the previous patch of the series.
---
drivers/usb/dwc2/gadget.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 7f25527..e8a8fc7 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);
@@ -2583,6 +2584,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
spin_unlock_irqrestore(&hsotg->lock, flags);
return ret;
}
According to the discussion in another thread, let's drop this patch.
--
Paul
Paul Zimmerman
2014-10-17 18:50:19 UTC
Permalink
Raw Message
Sent: Thursday, October 16, 2014 9:44 PM
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.
---
drivers/usb/dwc2/gadget.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 7b5856f..7f25527 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2561,8 +2561,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 error;
+ }
}
/* for non control endpoints, set PID to D0 */
@@ -2579,6 +2581,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
/* enable the endpoint interrupt */
s3c_hsotg_ctrl_epint(hsotg, index, dir_in, 1);
spin_unlock_irqrestore(&hsotg->lock, flags);
return ret;
}
Acked-by: Paul Zimmerman <***@synopsys.com>
Felipe Balbi
2014-10-17 18:52:09 UTC
Permalink
Raw Message
Sent: Thursday, October 16, 2014 9:44 PM
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.
---
drivers/usb/dwc2/gadget.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 7b5856f..7f25527 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2561,8 +2561,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 error;
+ }
}
/* for non control endpoints, set PID to D0 */
@@ -2579,6 +2581,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
/* enable the endpoint interrupt */
s3c_hsotg_ctrl_epint(hsotg, index, dir_in, 1);
spin_unlock_irqrestore(&hsotg->lock, flags);
return ret;
}
v3.18-rc or v3.19 ?
--
balbi
Paul Zimmerman
2014-10-17 19:05:19 UTC
Permalink
Raw Message
Sent: Friday, October 17, 2014 11:52 AM
Sent: Thursday, October 16, 2014 9:44 PM
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.
---
drivers/usb/dwc2/gadget.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 7b5856f..7f25527 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2561,8 +2561,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 error;
+ }
}
/* for non control endpoints, set PID to D0 */
@@ -2579,6 +2581,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
/* enable the endpoint interrupt */
s3c_hsotg_ctrl_epint(hsotg, index, dir_in, 1);
spin_unlock_irqrestore(&hsotg->lock, flags);
return ret;
}
v3.18-rc or v3.19 ?
v3.18-rc, since it's a bugfix. And I forgot, this should be marked for
3.17 stable too.
--
Paul
Felipe Balbi
2014-10-17 19:12:22 UTC
Permalink
Raw Message
Post by Paul Zimmerman
Sent: Friday, October 17, 2014 11:52 AM
Sent: Thursday, October 16, 2014 9:44 PM
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.
---
drivers/usb/dwc2/gadget.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 7b5856f..7f25527 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2561,8 +2561,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 error;
+ }
}
/* for non control endpoints, set PID to D0 */
@@ -2579,6 +2581,7 @@ static int s3c_hsotg_ep_enable(struct usb_ep *ep,
/* enable the endpoint interrupt */
s3c_hsotg_ctrl_epint(hsotg, index, dir_in, 1);
spin_unlock_irqrestore(&hsotg->lock, flags);
return ret;
}
v3.18-rc or v3.19 ?
v3.18-rc, since it's a bugfix. And I forgot, this should be marked for
3.17 stable too.
Alright, I'll add that.
--
balbi
Loading...