Em Fri, 6 Apr 2018 14:41:11 -0700
Christopher Li <sparse@xxxxxxxxxxx> escreveu:
> 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.
Hmm... it could try to track the if conditions and check if
"let" statements changing it would be there, but to be frank,
I never studied sparse code (nor did any work with lexical analyzers).
So, I've no idea about how it would track it.
>
>
> >
> > 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;
Actually output->dma is a pointer, but I got the idea.
>
> if (locking)
> spin_lock_irq();
>
> ddbwritel();
>
> if (locking)
> spin_unlock_irq();
>
> That will take care of the output->dma changing in between.
I tried that approach on two ways:
1) Doing:
struct ddb_dma *dma = output->dma;
And replacing all occurrences of output->dma to dma inside the
function (not replacing cause an extra warning from either sparse
or smatch, as it assigns dma then it checks if it is NULL);
2) with the enclosed patch:
diff --git a/drivers/media/pci/ddbridge/ddbridge-core.c b/drivers/media/pci/ddbridge/ddbridge-core.c
index 90687eff5909..02646f0c41ff 100644
--- a/drivers/media/pci/ddbridge/ddbridge-core.c
+++ b/drivers/media/pci/ddbridge/ddbridge-core.c
@@ -448,13 +448,14 @@ static void ddb_output_start(struct ddb_output *output)
static void ddb_output_stop(struct ddb_output *output)
{
struct ddb *dev = output->port->dev;
+ const bool has_dma = output->dma ? true : false;
- if (output->dma)
+ if (has_dma)
spin_lock_irq(&output->dma->lock);
ddbwritel(dev, 0, TS_CONTROL(output));
- if (output->dma) {
+ if (has_dma) {
ddbwritel(dev, 0, DMA_BUFFER_CONTROL(output->dma));
output->dma->running = 0;
spin_unlock_irq(&output->dma->lock);
Even in this case, where has_dma is const, sparse still generates the
warning. Just in case, I also tried changing has_dma to just
"bool" and just "int".
The warning is still there:
drivers/media/pci/ddbridge/ddbridge-core.c:458:9: warning: context imbalance in 'ddb_output_stop' - different lock contexts for basic block
>
> > 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.
IMO, it should be a little smarter, in order to at least work on codes
similar to the one produced with the above patch.
Thanks,
Mauro
--
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
