Re: [PATCH] staging: unisys: visorbus: visorchannel: Refactor locking code to be statically deterministic.

[Chuckleberryfinn <chuckleberryfinn@gmail.com>]

On Wed, Oct 19, 2016 at 05:00:53PM +0000, Sell, Timothy C wrote:
> On Wednesday, October 19, 2016 7:31 AM, Cathal Mullaney wrote:
> > This patch makes locking in visorchannel_signalempty statically deterministic.
> > As a result this patch fixes the sparse warning:
> > Context imbalance in 'visorchannel_signalempty' - different lock contexts for
> > basic block.
> > 
> > The logic of the locking code doesn't change but the layout of the original
> > code is "frowned upon"
> > according to mails on sparse context checking.
> > Refactoring removes the warning and makes the code more readable.
> > 
> > Signed-off-by: Cathal Mullaney <chuckleberryfinn@gmail.com>
> > ---
> >  drivers/staging/unisys/visorbus/visorchannel.c | 26 +++++++++++++++++---
> > ------
> >  1 file changed, 17 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/staging/unisys/visorbus/visorchannel.c
> > b/drivers/staging/unisys/visorbus/visorchannel.c
> > index a1381eb..1eea5d8 100644
> > --- a/drivers/staging/unisys/visorbus/visorchannel.c
> > +++ b/drivers/staging/unisys/visorbus/visorchannel.c
> > @@ -300,22 +300,30 @@
> > ---
> >  bool
> >  visorchannel_signalempty(struct visorchannel *channel, u32 queue)
> >  {
> >  	unsigned long flags = 0;
> > -	struct signal_queue_header sig_hdr;
> >  	bool rc = false;
> 
> It appears as if you no longer need to initialize 'rc' above.
> 
> Although this is NOT caused by your patch, it also looks like 'flags'
> is being unnecessarily initialized.  You may want to fix that too
> while you're in the neighborhood.
> 
> (Kernel folks seem to frown on unnecessary variable initializations.)
> 
> > 
> > -	if (channel->needs_lock)
> > -		spin_lock_irqsave(&channel->remove_lock, flags);
> > +	if (!channel->needs_lock)
> > +		return queue_empty(channel, queue);
> > 
> > -	if (sig_read_header(channel, queue, &sig_hdr))
> > -		rc = true;
> > -	if (sig_hdr.head == sig_hdr.tail)
> > -		rc = true;
> > -	if (channel->needs_lock)
> > -		spin_unlock_irqrestore(&channel->remove_lock, flags);
> > +	spin_lock_irqsave(&channel->remove_lock, flags);
> > +	rc = queue_empty(channel, queue);
> > +	spin_unlock_irqrestore(&channel->remove_lock, flags);
> > 
> >  	return rc;
> >  }
> > --
> > 2.7.4
> 
> Besides that, your patch looks good to me.  Thanks.
> 
> - Tim Sell
>

Thanks for your feedback Tim.
Submitted v2 with the suggested updates.
Thanks again.

Kind regards,
Cathal