On Fri, Apr 6, 2018 at 11:59 AM, Mauro Carvalho Chehab
<mchehab@xxxxxxxxxx> wrote:
> drivers/media/pci/ddbridge/ddbridge-core.c:442:9: warning: context imbalance in 'ddb_output_start' - different lock contexts for basic block
> drivers/media/pci/ddbridge/ddbridge-core.c:457:9: warning: context imbalance in 'ddb_output_stop' - different lock contexts for basic block
> drivers/media/pci/ddbridge/ddbridge-core.c:472:9: warning: context imbalance in 'ddb_input_stop' - different lock contexts for basic block
> drivers/media/pci/ddbridge/ddbridge-core.c:504:9: warning: context imbalance in 'ddb_input_start' - different lock contexts for basic block
>
> IMO, the problem here is at the sparce logic. Basically, this driver can
> work with output->dma filled or with NULL. when it is filled, there's a
> lock to protect the DMA. Otherwise, no lock is needed.
There is possible risk there. What if output->dma get changed
before you try to unlock?
Of course, from the code context there is no reason to see output->dma
can be changed, from developer point of view. However, for sparse,
there is not way of knowing that.
>
> Patches like the one below would make sparse to stop complaining:
>
> diff --git a/drivers/media/pci/ddbridge/ddbridge-core.c b/drivers/media/pci/ddbridge/ddbridge-core.c
> index 90687eff5909..69a29d4bf86d 100644
> --- a/drivers/media/pci/ddbridge/ddbridge-core.c
> +++ b/drivers/media/pci/ddbridge/ddbridge-core.c
> @@ -449,15 +449,16 @@ static void ddb_output_stop(struct ddb_output *output)
> {
> struct ddb *dev = output->port->dev;
>
> - if (output->dma)
> + if (output->dma) {
> spin_lock_irq(&output->dma->lock);
>
> - ddbwritel(dev, 0, TS_CONTROL(output));
> + ddbwritel(dev, 0, TS_CONTROL(output));
>
> - if (output->dma) {
> 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));
> }
> }
if you change that as:
int locking = output->dma;
if (locking)
spin_lock_irq();
ddbwritel();
if (locking)
spin_unlock_irq();
That will take care of the output->dma changing in between.
> The problem here is that sparse is not considering the conditional code
> for output->dma.
>
> I'd appreciate if you could look into it and add a patch fixing it
> there, if possible.
The current balance checking is simple and dumb.
There is possibility that using symbolic execution
to eliminate some of false positive like the above example.
Unfortunately, there is not much you can do in the more general
complicate case.
Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
