[PATCH v3 1/2] usb: dwc2: gadget: sparse warning of context imbalance

Discussion:

Sudip Mukherjee

2014-10-17 04:44:02 UTC

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

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

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

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

--
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

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

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

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