| Message ID | 20180406234417.2301bcba@vento.lan (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, Apr 6, 2018 at 7:44 PM, Mauro Carvalho Chehab <mchehab@kernel.org> 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)); 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. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Fri, 6 Apr 2018 19:48:39 -0700 Linus Torvalds <torvalds@linux-foundation.org> escreveu: > On Fri, Apr 6, 2018 at 7:44 PM, Mauro Carvalho Chehab > <mchehab@kernel.org> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 6, 2018 at 8:50 PM, Mauro Carvalho Chehab <mchehab@kernel.org> wrote: > > 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: Yes. That should avoid the warning and share the code. And if the shared code is small, inlining will automatically do the right thing. And if the shared code is large, the small wrapper to get locking right won't be a big deal. Of course, you *can* just choose to say that sparse doesn't understand the code, and ignore the warnings. It's true. But if you are otherwise sparse-clean, maybe just making sparse happy here too is a good idea. It does force you to try to think about locking a bit, and it does tend to make the locking easier to follow even for humans. Having a lock taken in one context and then released in some other context can be confusing even if you aren't sparse. So it's often just nicer if you see "unconditional lock" + some locked code + "unconditional unlock" instead of having conditions that you can get wrong. It's just that sparse really *requires* that pattern in order to understand locks. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);