linux-kernel - Re: [BUG] ls1046a: eDMA does not transfer data from I2C

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <9b20569d-7855-0031-3552-090ff7880cec@seco.com>
Date:   Tue, 20 Sep 2022 11:24:17 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     Robin Murphy <robin.murphy@....com>,
        Oleksij Rempel <linux@...pel-privat.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        linux-i2c@...r.kernel.org,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Vinod Koul <vkoul@...nel.org>, dmaengine@...r.kernel.org
Cc:     Li Yang <leoyang.li@....com>, Peng Ma <peng.ma@....com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        dri-devel@...ts.freedesktop.org,
        Christian König <christian.koenig@....com>,
        linaro-mm-sig@...ts.linaro.org, Robin Gong <yibin.gong@....com>,
        Shawn Guo <shawnguo@...nel.org>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Joy Zou <joy.zou@....com>, linux-media@...r.kernel.org
Subject: Re: [BUG] ls1046a: eDMA does not transfer data from I2C



On 9/20/22 6:07 AM, Robin Murphy wrote:
> On 2022-09-19 23:24, Sean Anderson wrote:
>> Hi all,
>>
>> I discovered a bug in either imx_i2c or fsl-edma on the LS1046A where no
>> data is read in i2c_imx_dma_read except for the last two bytes (which
>> are not read using DMA). This is perhaps best illustrated with the
>> following example:
>>
>> # hexdump -C /sys/bus/nvmem/devices/0-00540/nvmem
>> [  308.914884] i2c i2c-0: ffff000809380000 0x0000000889380000 0x00000000f5401000 ffff000075401000
>> [  308.923529] src= 2180004 dst=f5401000 attr=   0 soff=   0 nbytes=1 slast=       0
>> [  308.923529] citer=  7e biter=  7e doff=   1 dlast_sga=       0
>> [  308.923529] major_int=1 disable_req=1 enable_sg=0
>> [  308.942113] fsl-edma 2c00000.edma: vchan 000000001b4371fc: txd 00000000d9dd26c5[4]: submitted
>> [  308.974049] fsl-edma 2c00000.edma: txd 00000000d9dd26c5[4]: marked complete
>> [  308.981339] i2c i2c-0: ffff000809380000 = [2e 2e 2f 2e 2e 2f 2e 2e 2f 64 65 76 69 63 65 73 2f 70 6c 61 74 66 6f 72 6d 2f 73 6f 63 2f 32 31 38 30 30 30 30 2e 69 32 63 2f 69 32 63 2d 30 2f 30 2d 30 30 35 34 2f 30 2d 30 30 35 34 30 00 00]
>> [  309.002226] i2c i2c-0: ffff000075401000 = [2e 2e 2f 2e 2e 2f 2e 2e 2f 64 65 76 69 63 65 73 2f 70 6c 61 74 66 6f 72 6d 2f 73 6f 63 2f 32 31 38 30 30 30 30 2e 69 32 63 2f 69 32 63 2d 30 2f 30 2d 30 30 35 34 2f 30 2d 30 30 35 34 30 00 00]
>> [  309.024649] i2c i2c-0: ffff000809380080 0x0000000889380080 0x00000000f5401800 ffff000075401800
>> [  309.033270] src= 2180004 dst=f5401800 attr=   0 soff=   0 nbytes=1 slast=       0
>> [  309.033270] citer=  7e biter=  7e doff=   1 dlast_sga=       0
>> [  309.033270] major_int=1 disable_req=1 enable_sg=0
>> [  309.051633] fsl-edma 2c00000.edma: vchan 000000001b4371fc: txd 00000000d9dd26c5[5]: submitted
>> [  309.083526] fsl-edma 2c00000.edma: txd 00000000d9dd26c5[5]: marked complete
>> [  309.090807] i2c i2c-0: ffff000809380080 = [00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00]
>> [  309.111694] i2c i2c-0: ffff000075401800 = [00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00]
>> 00000000  2e 2e 2f 2e 2e 2f 2e 2e  2f 64 65 76 69 63 65 73  |../../../devices|
>> 00000010  2f 70 6c 61 74 66 6f 72  6d 2f 73 6f 63 2f 32 31  |/platform/soc/21|
>> 00000020  38 30 30 30 30 2e 69 32  63 2f 69 32 63 2d 30 2f  |80000.i2c/i2c-0/|
>> 00000030  30 2d 30 30 35 34 2f 30  2d 30 30 35 34 30 00 00  |0-0054/0-00540..|
>> 00000040  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
>> *
>> 00000070  00 00 00 00 00 00 00 00  00 00 00 00 00 00 ff ff  |................|
>> 00000080  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
>> *
>> 000000f0  00 00 00 00 00 00 00 00  00 00 00 00 00 00 ff 5b  |...............[|
>> 00000100
>>
>> (patch with my debug prints appended below)
>>
>> Despite the DMA completing successfully, no data was copied into the
>> buffer, leaving the original (now junk) contents. I probed the I2C bus
>> with an oscilloscope, and I verified that the transfer did indeed occur.
>> The timing between submission and completion seems reasonable for the
>> bus speed (50 kHz for whatever reason).
>>
>> I had a look over the I2C driver, and nothing looked obviously
>> incorrect. If anyone has ideas on what to try, I'm more than willing.
> 
> Is the DMA controller cache-coherent? I see the mainline LS1046A DT doesn't have a "dma-coherent" property for it, but the behaviour is entirely consistent with that being wrong - dma_map_single() cleans the cache, coherent DMA write hits the still-present cache lines, dma_unmap_single() invalidates the cache, and boom, the data is gone and you read back the previous content of the buffer that was cleaned out to DRAM beforehand.

I've tried both with and without [1] applied. I also tried removing the
call to dma_unmap_single, but to no effect.

--Sean

[1] https://lore.kernel.org/linux-arm-kernel/20220915233432.31660-6-leoyang.li@nxp.com/

>> --Sean
>>
>> diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c
>> index 15896e2413c4..1d9d4a55d2af 100644
>> --- a/drivers/dma/fsl-edma-common.c
>> +++ b/drivers/dma/fsl-edma-common.c
>> @@ -391,6 +391,12 @@ void fsl_edma_fill_tcd(struct fsl_edma_hw_tcd *tcd, u32 src, u32 dst,
>>   {
>>          u16 csr = 0;
>>   +       pr_info("src=%8x dst=%8x attr=%4x soff=%4x nbytes=%u slast=%8x\n"
>> +               "citer=%4x biter=%4x doff=%4x dlast_sga=%8x\n"
>> +               "major_int=%d disable_req=%d enable_sg=%d\n",
>> +               src, dst, attr, soff, nbytes, slast, citer, biter, doff,
>> +               dlast_sga, major_int, disable_req, enable_sg);
>> +
>>          /*
>>           * eDMA hardware SGs require the TCDs to be stored in little
>>           * endian format irrespective of the register endian model.
>> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
>> index 3576b63a6c03..0217f0cb1331 100644
>> --- a/drivers/i2c/busses/i2c-imx.c
>> +++ b/drivers/i2c/busses/i2c-imx.c
>> @@ -402,6 +402,9 @@ static int i2c_imx_dma_xfer(struct imx_i2c_struct *i2c_imx,
>>                  dev_err(dev, "DMA mapping failed\n");
>>                  goto err_map;
>>          }
>> +       phys_addr_t bufp = virt_to_phys(msgs->buf);
>> +       dev_info(dev, "%px %pap %pad %px\n", msgs->buf, &bufp,
>> +                &dma->dma_buf, phys_to_virt(dma->dma_buf));
>>            txdesc = dmaengine_prep_slave_single(dma->chan_using, dma->dma_buf,
>>                                          dma->dma_len, dma->dma_transfer_dir,
>> @@ -965,6 +968,9 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
>>                  }
>>                  schedule();
>>          }
>> +       dev_info(dev, "%px = [%*ph]\n", msgs->buf, msgs->len, msgs->buf);
>> +       dev_info(dev, "%px = [%*ph]\n", phys_to_virt(dma->dma_buf), msgs->len,
>> +                phys_to_virt(dma->dma_buf));
>>            temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>>          temp &= ~I2CR_DMAEN;