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 <[email protected]>
---
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
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 <[email protected]>
---
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
From: Of 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
> This patch depends on the previous patch of the series.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
> 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 17, 2014 at 09:02:00AM +0000, David Laight wrote:
> From: Of 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 my v1 patch , Paul mentioned :
>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
> David
>
> > This patch depends on the previous patch of the series.
> >
> > Signed-off-by: Sudip Mukherjee <[email protected]>
> > ---
> > 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
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
> From: Sudip Mukherjee [mailto:[email protected]]
> Sent: Friday, October 17, 2014 3:03 AM
>
> On Fri, Oct 17, 2014 at 09:02:00AM +0000, David Laight wrote:
> > From: Of 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
> my v1 patch , Paul mentioned :
> >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
> From: Sudip Mukherjee [mailto:[email protected]]
> Sent: Thursday, October 16, 2014 9:44 PM
>
> 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 <[email protected]>
> ---
> 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;
> }
Acked-by: Paul Zimmerman <[email protected]>
On Fri, Oct 17, 2014 at 06:50:19PM +0000, Paul Zimmerman wrote:
> > From: Sudip Mukherjee [mailto:[email protected]]
> > Sent: Thursday, October 16, 2014 9:44 PM
> >
> > 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 <[email protected]>
> > ---
> > 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;
> > }
>
> Acked-by: Paul Zimmerman <[email protected]>
v3.18-rc or v3.19 ?
--
balbi
> From: Sudip Mukherjee [mailto:[email protected]]
> 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.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
> 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;
> }
According to the discussion in another thread, let's drop this patch.
--
Paul
> From: Felipe Balbi [mailto:[email protected]]
> Sent: Friday, October 17, 2014 11:52 AM
>
> On Fri, Oct 17, 2014 at 06:50:19PM +0000, Paul Zimmerman wrote:
> > > From: Sudip Mukherjee [mailto:[email protected]]
> > > Sent: Thursday, October 16, 2014 9:44 PM
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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;
> > > }
> >
> > Acked-by: Paul Zimmerman <[email protected]>
>
> 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
On Fri, Oct 17, 2014 at 07:05:19PM +0000, Paul Zimmerman wrote:
> > From: Felipe Balbi [mailto:[email protected]]
> > Sent: Friday, October 17, 2014 11:52 AM
> >
> > On Fri, Oct 17, 2014 at 06:50:19PM +0000, Paul Zimmerman wrote:
> > > > From: Sudip Mukherjee [mailto:[email protected]]
> > > > Sent: Thursday, October 16, 2014 9:44 PM
> > > >
> > > > 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 <[email protected]>
> > > > ---
> > > > 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;
> > > > }
> > >
> > > Acked-by: Paul Zimmerman <[email protected]>
> >
> > 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
On Fri, Oct 17, 2014 at 07:05:19PM +0000, Paul Zimmerman wrote:
> > From: Felipe Balbi [mailto:[email protected]]
> > Sent: Friday, October 17, 2014 11:52 AM
> >
> > On Fri, Oct 17, 2014 at 06:50:19PM +0000, Paul Zimmerman wrote:
> > > > From: Sudip Mukherjee [mailto:[email protected]]
> > > > Sent: Thursday, October 16, 2014 9:44 PM
> > > >
> > > > 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 <[email protected]>
> > > > ---
> > > > 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;
> > > > }
> > >
> > > Acked-by: Paul Zimmerman <[email protected]>
> >
> > 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.
>
hi,
this is not yet added to linux-next , is something pending from my side?
thanks
sudip
> --
> Paul
>
On Thu, Oct 23, 2014 at 03:37:14PM +0530, Sudip Mukherjee wrote:
> On Fri, Oct 17, 2014 at 07:05:19PM +0000, Paul Zimmerman wrote:
> > > From: Felipe Balbi [mailto:[email protected]]
> > > Sent: Friday, October 17, 2014 11:52 AM
> > >
> > > On Fri, Oct 17, 2014 at 06:50:19PM +0000, Paul Zimmerman wrote:
> > > > > From: Sudip Mukherjee [mailto:[email protected]]
> > > > > Sent: Thursday, October 16, 2014 9:44 PM
> > > > >
> > > > > 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 <[email protected]>
> > > > > ---
> > > > > 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;
> > > > > }
> > > >
> > > > Acked-by: Paul Zimmerman <[email protected]>
> > >
> > > 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.
> >
> hi,
> this is not yet added to linux-next , is something pending from my side?
Patience.