Re: [PATCH v4 13/13] mmc: tmio: add barriers to IO operations

[Guennadi Liakhovetski <g.liakhovetski@gmx.de>]

On Mon, 18 Feb 2013, Arnd Bergmann wrote:

> On Monday 18 February 2013, Guennadi Liakhovetski wrote:
> > > Also, should the barrier not be after the MMIO read, rather than before it?
> > > Typically the barrier should ensure that any read from memory after an
> > > MMIO read reflects the memory contents after any DMA is complete that
> > > the MMIO read has already claimed to be done.
> > 
> > Errors, that I've been observing were happening with no DMA, in pure PIO 
> > mode.
> > 
> > Unfortunately, I don't have a good explanation, why the barriers have to 
> > be there, where I put them. At some point during my testing, I had 
> > printk()s in the code and SDIO worked. Then the classical - remove 
> > printk()s - stops working. Delays didn't halp, but barriers did. The 
> > motivation to put a write barrier before a (repeated) read was to wait for 
> > completion of any write operations before starting a read. And indeed, 
> > normal write operations, like writew() / iowrite16() have a write barrier 
> > before the write. So, isn't it possible, that the last write hasn't 
> > completed yet, while we begin with reading? But reads / writes should, 
> > probably, anyway be serialised on the bus...
> 
> What kind of bus is this?

Sorry, I'm not sure how best to describe it, and I don't have sufficient 
information myself. In any case on a block-diagram of sh73a0 SDHI devices 
aren't directly connected to a common super-highway bus, instead they are 
on a bus-splitter. One more thing I forgot to mention - this error has 
been observed on SMP.

> All buses I have looked at do serialize reads
> and writes to the same address at the minimum, and all sane buses
> serialize them when they happen to the same device, but it's harder
> to do when you need to serialize e.g. a read with a previous write
> to another device or another bus connected to the same device.
> 
> Let me try to say what I understand from reading the code: These
> accessors are only used on one function, and there is no DMA
> involved here. The function does (simplified):
> 
> void tmio_mmc_pio_irq(struct page *, void __iomem *io, size_t count, bool read)
> {
> 	void *virt;
> 
> 	/* called from interrupt handler, but let's disable interrupts some more ;) */
> 	local_irq_disable();
> 
> 	/* if highmem, map the page */
> 	virt = kmap_atomic(page);
> 	if (read) {
> 		wmb();
> 		readsw(io, virt, count);
> 	} else {
> 		writesw(io, virt, count);
> 		rmb();
> 	}
> 
> 	kunmap_atomic(virt);
> }
> 
> I don't think there is any other I/O involved, so the barriers are probably
> needed to synchronize with whoever is accessing the page on the other
> side of the kmap/kunmap. Did the bug happen on highmem?

No, don't think highmem was involved.

> The barriers here should probably be outside of the I/O accessors
> and in the tmio_mmc_pio_irq() function, but I still can't explain
> why they are needed here.
> 
> > It's also possible, that these errors are related to runtime 
> > power-management, which would involve IO to other SoC peripherals. But 
> > they all should also contain barriers, so, this doesn't explain it 
> > immediately either.
> 
> Maybe the runtime-pm code uses __raw_writel or writel_relaxed where it
> should be using writel?

I checked clock code under drivers/sh/clk, I don't see any relevant 
__raw_* operations there. One I do see, however, GIC code does use 
*_relaxed() accessors, but under raw spinlocks... Do those provide 
sufficient barriers?

Thanks
Guennadi

> Or maybe there some effect with disabling the caches and not flushing
> them properly in advance here?
> 
> In any case, I would expect a much more specific changeset text for a
> patch like this.
> 
> 	Arnd
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/