linux-kernel - RE: [PATCH V5 3/5] i2c: tegra: Add DMA Support

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:   Wed, 30 Jan 2019 05:04:10 +0000
From:   Sowjanya Komatineni <skomatineni@...dia.com>
To:     Dmitry Osipenko <digetx@...il.com>
CC:     "thierry.reding@...il.com" <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Mantravadi Karthik <mkarthik@...dia.com>,
        "Shardar Mohammed" <smohammed@...dia.com>,
        Timo Alho <talho@...dia.com>,
        "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>
Subject: RE: [PATCH V5 3/5] i2c: tegra: Add DMA Support

> > [Correction]
> > 
> > > > Could you please tell whether you missed my comments to V3 [0] or 
> > > > chose to ignore them? If the former, then I'd want to get answers 
> > > > to those questions and comments. I'll stop here for now.
> > > > 
> > > > [0] https://patchwork.ozlabs.org/patch/1031379/
> > >
> > > Somehow missed those from multiple comments. Will go thru and 
> > > respond back.
> > 
> > V6 includes feedback changes. Want to clarify on few feedback points
> > 
> > - ALIGN is used for 4 byte boundary to use with DMA but extra bytes 
> > doesn’t get transferred over I2C as I2C controller transfer bytes 
> > based on size specified in the packet header. DMA length and memory 
> > address need to be 4 byte boundary.
>
> Okay, I see now. Thanks.
>
> > - RX channel releasing when TX init fails?
> >   For I2C both TX and RX doesn’t happen in same transaction and no 
> > dependency. So if RX channel request & buffer allocation succeeds but 
> > TX channel request fails, then RX DMA can still be used for Msg reads 
> > and transmits can happen on PIO mode
>
> That's not what my point is about. In a case of TX initing failure, the RX channel and DMA buffer are not getting released in the probe function. Please be more careful about managing allocated resources.

OK, this comment from you was for V3. In V5 or V6, I changed init and allocation to happen only when DMA is needed. So my response here was referring to latest changes. 
I should not have mentioned this comment here as its already address in the latest changes.
In latest changes, DMA Channel request and allocation happens only when DMA is needed and If channel + buf is not already allocated.
During RX dma init, buffer is allocated only on successful RX channel request. If buf allocation fails, then RX channel is released.
During TX dma init, buf is allocated only if its not already allocated during RX Init and incase of allocation failure it releases TX channel.
But if TX channel request fails but dma buffer allocation with rx channel during RX DMA init is successful it will use RX DMA for i2c reads and uses PIO for i2c writes 

> > - dma_burst < 8 negatively affects transfer efficiency? Performance 
> > stats for DMA Vs PIO mode. Tested with 256 bytes of transfer and DMA 
> > Vs PIO mode transfer rate is almost same. But the main reason for 
> > adding DMA mode is to address couple of cases mentioned earlier and 
> > not mainly from the transfer performance perspective.
>
> Could you please add a clarifying comment to the code, saying that the whole purpose of the DMA transfer is solely to avoid delaying of the transactions?

Thought that’s too much info to add as comments to code, but added in commit message.

> Thanks for the replies! I'll take a look at V6 later today.