From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from osg.samsung.com ([64.30.133.232]:46185 "EHLO osg.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751491AbeDKQdJ (ORCPT ); Wed, 11 Apr 2018 12:33:09 -0400 Date: Wed, 11 Apr 2018 13:33:02 -0300 From: Mauro Carvalho Chehab To: Daniel Scheller Cc: Linux Media Mailing List , Ralph Metzler , Mauro Carvalho Chehab Subject: Re: [PATCH] media: ddbridge: better handle optional spin locks at the code Message-ID: <20180411133302.4382174d@vento.lan> In-Reply-To: <20180411180315.74c3a1bd@perian.wuest.de> References: <5156a3b987ae3698ff4c650a6395997f93ba093e.1523448215.git.mchehab@s-opensource.com> <20180411180315.74c3a1bd@perian.wuest.de> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Em Wed, 11 Apr 2018 18:03:15 +0200 Daniel Scheller escreveu: > Am Wed, 11 Apr 2018 08:03:37 -0400 > schrieb Mauro Carvalho Chehab : > > > Currently, ddbridge produces 4 warnings on sparse: > > drivers/media/pci/ddbridge/ddbridge-core.c:495:9: warning: context imbalance in 'ddb_output_start' - different lock contexts for basic block > > drivers/media/pci/ddbridge/ddbridge-core.c:510:9: warning: context imbalance in 'ddb_output_stop' - different lock contexts for basic block > > drivers/media/pci/ddbridge/ddbridge-core.c:525:9: warning: context imbalance in 'ddb_input_stop' - different lock contexts for basic block > > drivers/media/pci/ddbridge/ddbridge-core.c:560:9: warning: context imbalance in 'ddb_input_start' - different lock contexts for basic block > > > > Those are all false positives, but they result from the fact that > > there could potentially have some troubles at the locking schema, > > because the lock depends on a var (output->dma). > > > > I discussed that in priv with Sparse author and with the current > > maintainer. Both believe that sparse is doing the right thing, and > > that the proper fix would be to change the code to make it clearer > > that, when spin_lock_irq() is called, spin_unlock_irq() will be > > also called. > > > > That help not only static analyzers to better understand the code, > > but also humans that could need to take a look at the code. > > > > It was also pointed that gcc would likely be smart enough to > > optimize the code and produce the same result. I double > > checked: indeed, the size of the driver didn't change after > > this patch. > > > > Signed-off-by: Mauro Carvalho Chehab > > --- > > drivers/media/pci/ddbridge/ddbridge-core.c | 43 ++++++++++++++++++++---------- > > 1 file changed, 29 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/media/pci/ddbridge/ddbridge-core.c b/drivers/media/pci/ddbridge/ddbridge-core.c > > index 4a2819d3e225..080e2189ca7f 100644 > > --- a/drivers/media/pci/ddbridge/ddbridge-core.c > > +++ b/drivers/media/pci/ddbridge/ddbridge-core.c > > @@ -458,13 +458,12 @@ static void calc_con(struct ddb_output *output, u32 *con, u32 *con2, u32 flags) > > *con2 = (nco << 16) | gap; > > } > > > > -static void ddb_output_start(struct ddb_output *output) > > +static void __ddb_output_start(struct ddb_output *output) > > { > > struct ddb *dev = output->port->dev; > > u32 con = 0x11c, con2 = 0; > > > > if (output->dma) { > > - spin_lock_irq(&output->dma->lock); > > output->dma->cbuf = 0; > > output->dma->coff = 0; > > output->dma->stat = 0; > > @@ -492,9 +491,18 @@ static void ddb_output_start(struct ddb_output *output) > > > > ddbwritel(dev, con | 1, TS_CONTROL(output)); > > > > - if (output->dma) { > > + if (output->dma) > > output->dma->running = 1; > > +} > > + > > +static void ddb_output_start(struct ddb_output *output) > > +{ > > + if (output->dma) { > > + spin_lock_irq(&output->dma->lock); > > + __ddb_output_start(output); > > spin_unlock_irq(&output->dma->lock); > > + } else { > > + __ddb_output_start(output); > > } > > } > > This makes things look rather strange (at least to my eyes), especially > when simply trying to satisfy automated checkers, which in this case is > useless since both lock and unlock will always happen based on the same > condition ([input|output]->dma != NULL). Though I agree having the > locking inside a condition in it's current form isn't optimal, too, and > I also already thought about this in the past. > > I'd rather try to fix this by checking for the dma ptrs at the > beginning of the four functions and immediately return if the ptr is > invalid. Though I don't know if this may cause side effects as there's > data written to the regs pointed by the TS_CONTROL() macros even if > there's no dma ptr present. > > I'd like to hear Ralph's opinion on this, and also like to have this > changed (in whatever way) in the upstream (dddvb) repository, too. > > Please refrain from applying this patch until we agreed on a proper > solution that works for everyone. Yeah, sure. Btw, does ddbridge driver works without DMA? On a quick look, it seems that it is enabled all the times. > > Best regards, > Daniel Scheller Thanks, Mauro