Re: dw_mmc: IDMAC Invalidate cache after read

From: JABLONSKY Jan
Date: Sat Dec 01 2018 - 09:00:58 EST


Sorry for late answer,


The problem appears during calculation of md5sum of eMMC (16GB) in the
loop.
After some time (e.g. 1-2 hours), md5sum may be different

For example:
5c2b3c7a6d69a2f6c4c1ddfdd3bf1ed5 /dev/mmcblk0 time 746 [s]
5c2b3c7a6d69a2f6c4c1ddfdd3bf1ed5 /dev/mmcblk0 time 738 [s]
c5f2bb8e9d83744d4087450d6274208e /dev/mmcblk0 time 691 [s]
...



> It looks very dubious whether this is actually the right thing to do.
> > Just considering this driver, edma has an complementary sync_sg call in
> > its .start method, so if idma needed this one, logically shouldn't it
> > also need the other one as well?


If there is some coherency issue on Cortex-A9 - Probably yes,
I have to test it


> However, from a DMA API point of view, these syncs make no sense either
> > way - the very next thing we do here is call host->dma_ops->cleanup(),
> > which calls dma_unmap_sg(), which will perform the appropriate cache
> > maintenance anyway. Thus I can't see why this code is even here to begin
> > with. Similarly on the request path - the sg list really shouldn't have
> > been touched since being mapped in dw_mci_pre_dma_transfer(), so that
> > sync should also be an effective no-op unless it's papering over some
> > race condition elsewhere.

I agree,

But if there is some coherency issue with RK31xx SoCs (Cortex-A9, EDMA)
and Altera SoC FPGA (Cortex-A9, IDMA) ?
Because I see positive results, after applying mentioned patch
(Altera SoC FPGA - Cortex-A9, IDMA).
Probably using sync-ops functions before DMA transaction and after DMA
transaction in this case make sense



Shawn
Could you please also share some details, that you remember
(sync-ops and RK31xx SoCs) ?
I would really appreciate it.

Thanks



On Di, 2018-11-27 at 08:43 +0800, Shawn Lin wrote:
> On 2018/11/23 23:29, Robin Murphy wrote:
> > Hi Jan,
> >
> > [repeating some of the discussion from your other thread for the benefit
> > of the MMC audience]
> >
> > On 21/11/2018 07:42, JABLONSKY Jan wrote:
> >> CPU may not see most up-to-date and correct copy of DMA buffer, when
> >> internal DMA controller is in use.
> >> Problem appears on The Altera SoC FPGA (uses integrated DMA controller),
> >> during higher CPU and system memory load
> >>
> >> Signed-off-by: Jan Jablonsky <jan.jablonsky@xxxxxxxxxxxxxxx>
> >> ---
> >> drivers/mmc/host/dw_mmc.c | 3 +--
> >> 1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> >> index 80dc2fd..63873d9 100644
> >> --- a/drivers/mmc/host/dw_mmc.c
> >> +++ b/drivers/mmc/host/dw_mmc.c
> >> @@ -499,8 +499,7 @@ static void dw_mci_dmac_complete_dma(void *arg)
> >> dev_vdbg(host->dev, "DMA complete\n");
> >> - if ((host->use_dma == TRANS_MODE_EDMAC) &&
> >> - data && (data->flags & MMC_DATA_READ))
> >> + if (data && (data->flags & MMC_DATA_READ))
> >> /* Invalidate cache after read */
> >> dma_sync_sg_for_cpu(mmc_dev(host->slot->mmc),
> >> data->sg,
> >
> > It looks very dubious whether this is actually the right thing to do.
> > Just considering this driver, edma has an complementary sync_sg call in
> > its .start method, so if idma needed this one, logically shouldn't it
> > also need the other one as well?
> >
> > However, from a DMA API point of view, these syncs make no sense either
> > way - the very next thing we do here is call host->dma_ops->cleanup(),
> > which calls dma_unmap_sg(), which will perform the appropriate cache
> > maintenance anyway. Thus I can't see why this code is even here to begin
> > with. Similarly on the request path - the sg list really shouldn't have
> > been touched since being mapped in dw_mci_pre_dma_transfer(), so that
> > sync should also be an effective no-op unless it's papering over some
> > race condition elsewhere.
> >
> > Shawn - do you remember why these syncs were added in 3fc7eaef44dbc?
> > Were you seeing actual coherency issues on RK31xx SoCs, or was it
> > perhaps just some leftover or misunderstanding which missed getting
> > cleaned up?
>
> I can't remember too much details but looking at the dma-mapping code
> again, it seems the complemetary sync-op here is useless.
>
> >
> > Robin.
> >
> >
> >
>
>