Am Wed, 11 Apr 2018 08:03:37 -0400
schrieb Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx>:
> 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@xxxxxxxxxxxxxxxx>
> ---
> 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
--
https://github.com/herrnst
