| Message ID | 5156a3b987ae3698ff4c650a6395997f93ba093e.1523448215.git.mchehab@s-opensource.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am Wed, 11 Apr 2018 08:03:37 -0400 schrieb Mauro Carvalho Chehab <mchehab@s-opensource.com>: > 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 <mchehab@s-opensource.com> > --- > 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. Best regards, Daniel Scheller
Em Wed, 11 Apr 2018 18:03:15 +0200 Daniel Scheller <d.scheller.oss@gmail.com> escreveu: > Am Wed, 11 Apr 2018 08:03:37 -0400 > schrieb Mauro Carvalho Chehab <mchehab@s-opensource.com>: > > > 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 <mchehab@s-opensource.com> > > --- > > 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
Am Wed, 11 Apr 2018 13:33:02 -0300 schrieb Mauro Carvalho Chehab <mchehab@s-opensource.com>: > Em Wed, 11 Apr 2018 18:03:15 +0200 > Daniel Scheller <d.scheller.oss@gmail.com> escreveu: > > > Am Wed, 11 Apr 2018 08:03:37 -0400 > > schrieb Mauro Carvalho Chehab <mchehab@s-opensource.com>: > > > > > 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 <mchehab@s-opensource.com> > > > --- > > > 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. DMA (and only this way of transportation) is used for all TS stream input/output from/to demods and CI adapters (and the modulator cards in the upstream driver) when driven by any of the PCIe bridges. After another quick glance, [in|out]put->dma should really always be set. In the end, they are pointers to "struct ddb_dma"'s to the fixed members of struct ddb, which is allocated when the driver is loaded via ddb_probe() in ddbridge-main. Not sure at the moment where the assignment to in/output can fail, but if it did, I believe programming the remaining things in the hardware can rather safely be left out aswell. Best regards, Daniel Scheller
Mauro Carvalho Chehab writes: > Em Wed, 11 Apr 2018 18:03:15 +0200 > Daniel Scheller <d.scheller.oss@gmail.com> escreveu: > > > Am Wed, 11 Apr 2018 08:03:37 -0400 > > schrieb Mauro Carvalho Chehab <mchehab@s-opensource.com>: > > > > > 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). > > > Yeah, there were false positives on other parts of the driver where it was obvious to anybody that the bad case cannot happen ... > > > 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 <mchehab@s-opensource.com> > > > --- > > > 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); > > > } > > > } What does it say if you do: struct ddb_dma *dma = output->dma; if (dma) lock; ... if (dma) unlock; ? If that does not work, what if you make dma const? > > > > 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. TS_CONTROL only controls the output itself and does not need to have DMA. > > > > 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. > DMA is not used with the OctopusNet and only partially on the OctopusNetPro. Both are ARM based network streaming devices. The OctopusNet has no DMA at all and can only stream directly to CI or network. The Pro version has DMA and will support streaming and/or DMA on the each channel. DMA channel assignment was at one point planned to be done dynamically. It also is PCIe based. So, this is part of ddbridge-main.c and not octonet-main.c. But support for those devices and some PCIe cards is not part of the kernel driver anyway. So, you could of course through all of this DMA checking out of the kernel version. It might have to be added back in for future/other cards. But that problems already exists with other features. Regards, Ralph
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); } } @@ -502,15 +510,13 @@ static void ddb_output_stop(struct ddb_output *output) { struct ddb *dev = output->port->dev; - if (output->dma) - spin_lock_irq(&output->dma->lock); - - ddbwritel(dev, 0, TS_CONTROL(output)); - if (output->dma) { + spin_lock_irq(&output->dma->lock); ddbwritel(dev, 0, DMA_BUFFER_CONTROL(output->dma)); output->dma->running = 0; spin_unlock_irq(&output->dma->lock); + } else { + ddbwritel(dev, 0, TS_CONTROL(output)); } } @@ -519,22 +525,21 @@ static void ddb_input_stop(struct ddb_input *input) struct ddb *dev = input->port->dev; u32 tag = DDB_LINK_TAG(input->port->lnr); - if (input->dma) - spin_lock_irq(&input->dma->lock); - ddbwritel(dev, 0, tag | TS_CONTROL(input)); if (input->dma) { + spin_lock_irq(&input->dma->lock); ddbwritel(dev, 0, DMA_BUFFER_CONTROL(input->dma)); input->dma->running = 0; spin_unlock_irq(&input->dma->lock); + } else { + ddbwritel(dev, 0, tag | TS_CONTROL(input)); } } -static void ddb_input_start(struct ddb_input *input) +static void __ddb_input_start(struct ddb_input *input) { struct ddb *dev = input->port->dev; if (input->dma) { - spin_lock_irq(&input->dma->lock); input->dma->cbuf = 0; input->dma->coff = 0; input->dma->stat = 0; @@ -557,9 +562,19 @@ static void ddb_input_start(struct ddb_input *input) if (input->port->type == DDB_TUNER_DUMMY) ddbwritel(dev, 0x000fff01, TS_CONTROL2(input)); - if (input->dma) { + if (input->dma) input->dma->running = 1; +} + +static void ddb_input_start(struct ddb_input *input) +{ + + if (input->dma) { + spin_lock_irq(&input->dma->lock); + __ddb_input_start(input); spin_unlock_irq(&input->dma->lock); + } else { + __ddb_input_start(input); } }
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 <mchehab@s-opensource.com> --- drivers/media/pci/ddbridge/ddbridge-core.c | 43 ++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 14 deletions(-)