linux-kernel - Re: dw_mmc: IDMAC Invalidate cache after read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 1 Dec 2018 13:56:39 +0000
From:   JABLONSKY Jan <Jan.JABLONSKY@...lesgroup.com>
To:     Shawn Lin <shawn.lin@...k-chips.com>
CC:     Robin Murphy <robin.murphy@....com>,
        Jaehoon Chung <jh80.chung@...sung.com>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: dw_mmc: IDMAC Invalidate cache after read

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@...lesgroup.com>
> >> ---
> >>   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.
> > 
> > 
> > 
> 
>