Re: Sparse false-positive warnings

[Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>]

Em Fri, 6 Apr 2018 19:48:39 -0700
Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> escreveu:

> On Fri, Apr 6, 2018 at 7:44 PM, Mauro Carvalho Chehab
> <mchehab@xxxxxxxxxx> wrote:
> >
> > -       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".  
> 
> Sparse will always warn for this, for the simple reason that the basic
> block there in the middle that does that
> 
>           ddbwritel(dev, 0, TS_CONTROL(output));

In theory, yes. In practice, however, the device either has dma (and
needs locking) or not.

> has two different locking contexts.
> 
> So the only way to get sparse to not warn is to have something like
> 
>     if (output->dma) {
>         spin_lock_irq(&output->dma->lock);
>         ddbwritel(dev, 0, TS_CONTROL(output));
>         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));
>     }
> 
> which probably generates better code too when that "shared" region is
> smaller than the rest of the code.

In the specific case of this function, the above is not bad. However,
at the other 3 places where the warnings are generated, the code is
more complex, like on this one:

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;
                ddbwritel(dev, 0, DMA_BUFFER_CONTROL(output->dma));
        }

	if (output->port->input[0]->port->class == DDB_PORT_LOOP)
                con = (1UL << 13) | 0x14;
        else
            	calc_con(output, &con, &con2, 0);

        ddbwritel(dev, 0, TS_CONTROL(output));
        ddbwritel(dev, 2, TS_CONTROL(output));
        ddbwritel(dev, 0, TS_CONTROL(output));
        ddbwritel(dev, con, TS_CONTROL(output));
        ddbwritel(dev, con2, TS_CONTROL2(output));

        if (output->dma) {
                ddbwritel(dev, output->dma->bufval,
                          DMA_BUFFER_SIZE(output->dma));
                ddbwritel(dev, 0, DMA_BUFFER_ACK(output->dma));
                ddbwritel(dev, 1, DMA_BASE_READ);
                ddbwritel(dev, 7, DMA_BUFFER_CONTROL(output->dma));
        }

        ddbwritel(dev, con | 1, TS_CONTROL(output));

        if (output->dma) {
                output->dma->running = 1;
                spin_unlock_irq(&output->dma->lock);
        }
}

duplicating everything doesn't sound right. Ok, we might implement
a __ddb_output_start() function with everything but the lock,
then a new ddb_output_start() with just:

static void ddb_output_start(struct ddb_output *output)
{
        struct ddb *dev = output->port->dev;

        if (output->dma) {
                spin_lock_irq(&output->dma->lock);
		__ddb_output_start(output);
                spin_unlock_irq(&output->dma->lock);
	} else {
		__ddb_output_start(output);
	}
}

for the 4 functions where this warning is produced, but that seems a 
dirty hack made just to shut up the false positive warnings.

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