From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754452AbcJSV4r (ORCPT ); Wed, 19 Oct 2016 17:56:47 -0400 Received: from mail-lf0-f68.google.com ([209.85.215.68]:35370 "EHLO mail-lf0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752820AbcJSV4q (ORCPT ); Wed, 19 Oct 2016 17:56:46 -0400 Date: Wed, 19 Oct 2016 22:56:43 +0100 From: Chuckleberryfinn To: "Sell, Timothy C" Cc: "gregkh@linuxfoundation.org" , *S-Par-Maintainer , "devel@driverdev.osuosl.org" , "linux-kernel@vger.kernel.org" , "Kershner, David A" Subject: Re: [PATCH] staging: unisys: visorbus: visorchannel: Refactor locking code to be statically deterministic. Message-ID: <20161019215642.GA4198@Shiva> References: <1476876643-3708-1-git-send-email-chuckleberryfinn@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > --- > > 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