On 11/02/2012 02:25 PM, Simon Glass wrote:
Hi Stephen,
On Fri, Nov 2, 2012 at 1:22 PM, Stephen Warren swarren@wwwdotorg.org wrote:
On 04/26/2012 11:29 PM, Mike Frysinger wrote:
On Thursday 26 April 2012 06:34:43 Thierry Reding wrote:
For reference, see sd_change_freq() in drivers/mmc/mmc.c.
This is a follow-up to:
http://lists.denx.de/pipermail/u-boot/2012-April/123080.html
which was referenced from:
http://lists.denx.de/pipermail/u-boot/2012-September/133641.html
yes, that shows what we're talking about. int sd_change_freq(struct mmc *mmc) { ... stack vars ... int err; ALLOC_CACHE_ALIGN_BUFFER(uint, scr, 2); struct mmc_data data; int timeout; ... stack vars ... ... data.dest = (char *)scr; data.blocksize = 8; data.blocks = 1; data.flags = MMC_DATA_READ; err = mmc_send_cmd(mmc, &cmd, &data); ...
static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) { ... if (data->flags & MMC_DATA_READ) { if ((uintptr_t)data->dest & (ARCH_DMA_MINALIGN - 1)) printf("Warning: unaligned read from %p " "may fail\n", data->dest); invalidate_dcache_range((ulong)data->dest, (ulong)data->dest + data->blocks * data->blocksize); } ...
so what invalidate_dcache_range() will invalidate is from &scr and 8 bytes after that. the higher layers make sure &scr is aligned properly, but not that it spans a full cache line. so if the stack layout is: [random stuff] 0x100 [scr[0]] 0x104 [scr[1]] 0x108 [err] 0x10a [timeout] [more stuff]
That's not the stack layout. scr is allocated (as quoted above) using ALLOC_CACHE_ALIGN_BUFFER. Therefore, the storage space allocated for scr is always cache-aligned in address and size, even if the code only uses 8 bytes of it.
this implicitly invalidates [err] and [timeout] and more stuff. by you forcibly rounding up the length, you no longer get a warning, but you still (silently) blew away those variables from cache possibly losing changes that weren't written back to external memory yet.
So, this problem won't actually occur here.
So, Thierry's proposed solution (which I'll re-quote below) would actually work just fine:
[PATCH 2/2] mmc: tegra: invalidate complete cachelines
The MMC core sometimes reads buffers that are smaller than a complete cacheline, for example when reading the SCR. In order to avoid a warning from the ARM v7 cache handling code, this patch makes sure that complete cachelines are flushed.
Signed-off-by: Thierry Reding <thierry.reding at avionic-design.de> --- drivers/mmc/tegra2_mmc.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/tegra2_mmc.c b/drivers/mmc/tegra2_mmc.c index fb8a57d..3615876 100644 --- a/drivers/mmc/tegra2_mmc.c +++ b/drivers/mmc/tegra2_mmc.c @@ -323,12 +323,13 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, } writel(mask, &host->reg->norintsts); if (data->flags & MMC_DATA_READ) { + ulong end = (ulong)data->dest + + roundup(data->blocks * data->blocksize, + ARCH_DMA_MINALIGN); if ((uintptr_t)data->dest & (ARCH_DMA_MINALIGN - 1)) printf("Warning: unaligned read from %p " "may fail\n", data->dest); - invalidate_dcache_range((ulong)data->dest, - (ulong)data->dest
data->blocks * data->blocksize); +invalidate_dcache_range((ulong)data->dest, end); } }
--
... so long as we require any struct mmc_data data's .dest field to point at a buffer that was allocated using ALLOC_CACHE_ALIGN_BUFFER.
So, I'd like to propose that we take Thierry's fix, perhaps having validated (or implemented some way of enforcing) that ALLOC_CACHE_ALIGN_BUFFER() was used to allocate the buffer pointer.
That sounds ok to me.
It is important to avoid silent failure if we can. Are you proposing to pass a 'size' parameter along with any buffer pointer, or something else?
I was wondering about creating some kind of macro to initialize the buffer pointer in struct mmc_data, and writing that macro so that the buffer passed to it had to have been allocated using ALLOC_CACHE_ALIGN_BUFFER (e.g. by referencing the hidden variable it creates, or something like that).
Passing in the buffer size would also work, although it seems like it'd be easier to screw that up and hide the silent errors you mentioned?