[U-Boot] [PATCH 2/2] mmc: tegra: invalidate complete cachelines

[Stephen Warren]

On 11/02/2012 02:25 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Fri, Nov 2, 2012 at 1:22 PM, Stephen Warren <swarren at 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?