[PATCH] staging: unisys: uislib: uisqueue.c: fixed sparse warning of context imbalance

[PATCH] staging: unisys: uislib: uisqueue.c: fixed sparse warning of context imbalance

[Sudip Mukherjee]

fixed sparse warning : context imbalance in 'do_locked_client_insert'
			 different lock contexts for basic block

spin_unlock_irqrestore is called at a later stage before returning 
from the function if locked is 1.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/staging/unisys/uislib/uisqueue.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/unisys/uislib/uisqueue.c b/drivers/staging/unisys/uislib/uisqueue.c
index f52bca1..63dbe7c 100644
--- a/drivers/staging/unisys/uislib/uisqueue.c
+++ b/drivers/staging/unisys/uislib/uisqueue.c
@@ -96,8 +96,6 @@ do_locked_client_insert(struct uisqueue_info *queueinfo,
 		goto Away;
 	ULTRA_CHANNEL_CLIENT_RELEASE_OS(queueinfo->chan, channelId, NULL);
 	acquired = 0;
-	spin_unlock_irqrestore(lock, flags);
-	locked = 0;
 
 	queueinfo->packets_sent++;
 
-- 
1.8.1.2

Re: [PATCH] staging: unisys: uislib: uisqueue.c: fixed sparse warning of context imbalance

[Dan Carpenter]

On Tue, Sep 02, 2014 at 04:39:47PM +0530, Sudip Mukherjee wrote:
> fixed sparse warning : context imbalance in 'do_locked_client_insert'
> 			 different lock contexts for basic block
> 
> spin_unlock_irqrestore is called at a later stage before returning 
> from the function if locked is 1.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>

This doesn't match the email address you are using.

Really, your patch isn't bad but I would prefer if you re-wrote this
entire function because currently it is garbage.

static u8
do_locked_client_insert(struct uisqueue_info *queueinfo,
                        unsigned int whichqueue,
                        void *pSignal,
                        spinlock_t *lock,
                        unsigned char issueInterruptIfEmpty,
                        u64 interruptHandle, u8 *channelId)
{
        unsigned long flags;
        unsigned char queueWasEmpty;

        spin_lock_irqsave(lock, flags);

        if (!ULTRA_CHANNEL_CLIENT_ACQUIRE_OS(queueinfo->chan, channelId, NULL))
                goto unlock;

        queueWasEmpty = visor_signalqueue_empty(queueinfo->chan, whichqueue);
        if (!visor_signal_insert(queueinfo->chan, whichqueue, pSignal))
                goto release;
        ULTRA_CHANNEL_CLIENT_RELEASE_OS(queueinfo->chan, channelId, NULL);
        spin_unlock_irqrestore(lock, flags);

        queueinfo->packets_sent++;

        return 1;

release:
        ULTRA_CHANNEL_CLIENT_RELEASE_OS(queueinfo->chan, channelId, NULL);
unlock:
        spin_unlock_irqrestore(lock, flags);

        return 0;
}

The queueWasEmpty variable is kind of silly.  It should just be an int
or maybe a bool if you are being pedantic but instead we very
specifically set it to be an unsigned variable of the incorrect type.
Also we don't use queueWasEmpty at all.  I think we could delete it...

The problem with the original code was that the error paths and the
success paths were mixed together like spaghetti.  If you separate them
out and unwind in the proper order with normal label names then the
code is easy to understand.

regards,
dan carpenter

Re: [PATCH] staging: unisys: uislib: uisqueue.c: fixed sparse warning of context imbalance

[Sudip Mukherjee]

On Tue, Sep 02, 2014 at 05:05:32PM +0300, Dan Carpenter wrote:
> On Tue, Sep 02, 2014 at 04:39:47PM +0530, Sudip Mukherjee wrote:
> > fixed sparse warning : context imbalance in 'do_locked_client_insert'
> > 			 different lock contexts for basic block
> > 
> > spin_unlock_irqrestore is called at a later stage before returning 
> > from the function if locked is 1.
> > 
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> 
> This doesn't match the email address you are using.
> 

for all my patch the Signed-off email address is different from the From email address.
I am not sure if I should do that, but since no one pointed it out so I guessed it is ok.

> Really, your patch isn't bad but I would prefer if you re-wrote this
> entire function because currently it is garbage.
> 
> static u8
> do_locked_client_insert(struct uisqueue_info *queueinfo,
>                         unsigned int whichqueue,
>                         void *pSignal,
>                         spinlock_t *lock,
>                         unsigned char issueInterruptIfEmpty,
>                         u64 interruptHandle, u8 *channelId)
> {
>         unsigned long flags;
>         unsigned char queueWasEmpty;
> 
>         spin_lock_irqsave(lock, flags);
> 
>         if (!ULTRA_CHANNEL_CLIENT_ACQUIRE_OS(queueinfo->chan, channelId, NULL))
>                 goto unlock;
> 
>         queueWasEmpty = visor_signalqueue_empty(queueinfo->chan, whichqueue);
>         if (!visor_signal_insert(queueinfo->chan, whichqueue, pSignal))
>                 goto release;
>         ULTRA_CHANNEL_CLIENT_RELEASE_OS(queueinfo->chan, channelId, NULL);
>         spin_unlock_irqrestore(lock, flags);
> 
>         queueinfo->packets_sent++;
> 
>         return 1;
> 
> release:
>         ULTRA_CHANNEL_CLIENT_RELEASE_OS(queueinfo->chan, channelId, NULL);
> unlock:
>         spin_unlock_irqrestore(lock, flags);
> 
>         return 0;
> }
> 
> The queueWasEmpty variable is kind of silly.  It should just be an int
> or maybe a bool if you are being pedantic but instead we very
> specifically set it to be an unsigned variable of the incorrect type.
> Also we don't use queueWasEmpty at all.  I think we could delete it...
> 
> The problem with the original code was that the error paths and the
> success paths were mixed together like spaghetti.  If you separate them
> out and unwind in the proper order with normal label names then the
> code is easy to understand.
> 
> regards,
> dan carpenter
> 
I will send a modified patch with the required changes.

thanks
sudip

Re: [PATCH] staging: unisys: uislib: uisqueue.c: fixed sparse warning of context imbalance

[Dan Carpenter]

On Tue, Sep 02, 2014 at 08:44:27PM +0530, Sudip Mukherjee wrote:
> On Tue, Sep 02, 2014 at 05:05:32PM +0300, Dan Carpenter wrote:
> > On Tue, Sep 02, 2014 at 04:39:47PM +0530, Sudip Mukherjee wrote:
> > > fixed sparse warning : context imbalance in 'do_locked_client_insert'
> > > 			 different lock contexts for basic block
> > > 
> > > spin_unlock_irqrestore is called at a later stage before returning 
> > > from the function if locked is 1.
> > > 
> > > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > 
> > This doesn't match the email address you are using.
> > 
> 
> for all my patch the Signed-off email address is different from the From email address.
> I am not sure if I should do that, but since no one pointed it out so I guessed it is ok.

Sign-off-by are like signing a legal document that you didn't steal
copyright code.  We like to check that they match up with the email
sending the patch.

regards,
dan carpenter

Re: [PATCH] staging: unisys: uislib: uisqueue.c: fixed sparse warning of context imbalance

[Sudip Mukherjee]

On Tue, Sep 02, 2014 at 06:23:27PM +0300, Dan Carpenter wrote:
> On Tue, Sep 02, 2014 at 08:44:27PM +0530, Sudip Mukherjee wrote:
> > On Tue, Sep 02, 2014 at 05:05:32PM +0300, Dan Carpenter wrote:
> > > On Tue, Sep 02, 2014 at 04:39:47PM +0530, Sudip Mukherjee wrote:
> > > > fixed sparse warning : context imbalance in 'do_locked_client_insert'
> > > > 			 different lock contexts for basic block
> > > > 
> > > > spin_unlock_irqrestore is called at a later stage before returning 
> > > > from the function if locked is 1.
> > > > 
> > > > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > > 
> > > This doesn't match the email address you are using.
> > > 
> > 
> > for all my patch the Signed-off email address is different from the From email address.
> > I am not sure if I should do that, but since no one pointed it out so I guessed it is ok.
> 
> Sign-off-by are like signing a legal document that you didn't steal
> copyright code.  We like to check that they match up with the email
> sending the patch.
> 
> regards,
> dan carpenter
> 
one last question regarding it . can i add both the email addresses as two Signed-off ?

thanks
sudip