Re: dw_mmc: IDMAC Invalidate cache after read

[Shawn Lin]

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.