Re: [PATCH v2 08/24] mmc: sdhci: avoid unnecessary mapping/unmapping of align buffer

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

On Tue, Dec 29, 2015 at 03:44:38PM +0200, Adrian Hunter wrote:
> On 21/12/15 13:41, Russell King wrote:
> > Unnecessarily mapping and unmapping the align buffer for SD cards is
> > expensive: performance measurements on iMX6 show that this gives a hit
> > of 10% on hdparm buffered disk reads.
> > 
> > MMC/SD card IO comes from the mm/vfs which gives us page based IO, so
> > for this case, the align buffer is not going to be used.  However, we
> > still map and unmap this buffer.
> > 
> > Eliminate this by switching the align buffer to be a DMA coherent
> > buffer, which needs no DMA maintenance to access the buffer.
> 
> Did you consider putting the align buffer in the same allocation
> as the adma_table?

It's not clear whether host->adma_table_sz would be appropriately
aligned.

> > @@ -3003,6 +2972,10 @@ int sdhci_add_host(struct sdhci_host *host)
> >  						      host->adma_table_sz,
> >  						      &host->adma_addr,
> >  						      GFP_KERNEL);
> > +		host->align_buffer = dma_alloc_coherent(mmc_dev(mmc),
> > +							host->align_buffer_sz,
> > +							&host->align_addr,
> > +							GFP_KERNEL);
> >  		host->align_buffer = kmalloc(host->align_buffer_sz, GFP_KERNEL);
> 
> kmalloc line is still there

Good catch, thanks.

> > +
> > +		/* dma_alloc_coherent returns page aligned and sized buffers */
> > +		BUG_ON(host->align_addr & host->align_mask);
> 
> It would be nicer not to have any BUG_ON()

This is a situation that should _never_ occur (if it does, then the
dma_alloc_coherent() implementation is violating the API requirements,
which are to return a page-sized page-aligned buffer.)  I guess it
could be a WARN_ON(), but if it fails we're likely to cause data
corruption.  So, a WARN_ON() and failing the probe seems more
appropriate - but then that means yet more messy cleanup in an already
messy part of the driver.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.