linux-kernel - Re: [PATCH v5 2/4] dmaengine: gpi: Add Lock and Unlock TRE support to access I2C exclusively

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: <Z1BJSbf+1G8ojTib@vaman>
Date: Wed, 4 Dec 2024 17:51:29 +0530
From: Vinod Koul <vkoul@...nel.org>
To: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
Cc: konrad.dybcio@...aro.org, andersson@...nel.org, andi.shyti@...nel.org,
	linux-arm-msm@...r.kernel.org, dmaengine@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org,
	conor+dt@...nel.org, agross@...nel.org, devicetree@...r.kernel.org,
	linux@...blig.org, dan.carpenter@...aro.org, Frank.Li@....com,
	konradybcio@...nel.org, bryan.odonoghue@...aro.org,
	krzk+dt@...nel.org, robh@...nel.org, quic_vdadhani@...cinc.com
Subject: Re: [PATCH v5 2/4] dmaengine: gpi: Add Lock and Unlock TRE support
 to access I2C exclusively

On 02-12-24, 16:13, Mukesh Kumar Savaliya wrote:
> Thanks for the review comments Vinod !
> 
> On 12/2/2024 12:17 PM, Vinod Koul wrote:
> > On 29-11-24, 20:13, Mukesh Kumar Savaliya wrote:
> > > GSI DMA provides specific TREs(Transfer ring element) namely Lock and
> > > Unlock TRE. It provides mutually exclusive access to I2C controller from
> > > any of the processor(Apps,ADSP). Lock prevents other subsystems from
> > > concurrently performing DMA transfers and avoids disturbance to data path.
> > > Basically for shared I2C usecase, lock the SE(Serial Engine) for one of
> > > the processor, complete the transfer, unlock the SE.
> > > 
> > > Apply Lock TRE for the first transfer of shared SE and Apply Unlock
> > > TRE for the last transfer.
> > > 
> > > Also change MAX_TRE macro to 5 from 3 because of the two additional TREs.
> > > 
> > 
> > ...
> > 
> > > @@ -65,6 +65,9 @@ enum i2c_op {
> > >    * @rx_len: receive length for buffer
> > >    * @op: i2c cmd
> > >    * @muli-msg: is part of multi i2c r-w msgs
> > > + * @shared_se: bus is shared between subsystems
> > > + * @bool first_msg: use it for tracking multimessage xfer
> > > + * @bool last_msg: use it for tracking multimessage xfer
> > >    */
> > >   struct gpi_i2c_config {
> > >   	u8 set_config;
> > > @@ -78,6 +81,9 @@ struct gpi_i2c_config {
> > >   	u32 rx_len;
> > >   	enum i2c_op op;
> > >   	bool multi_msg;
> > > +	bool shared_se;
> > 
> > Looking at this why do you need this field? It can be internal to your
> > i2c driver... Why not just set an enum for lock and use the values as
> > lock/unlock/dont care and make the interface simpler. I see no reason to
> > use three variables to communicate the info which can be handled in
> > simpler way..?
> > 
> Below was earlier reply to [PATCH V3, 2/4], please let me know if you have
> any additional comment and need further clarifications.

Looks like you misunderstood, the question is why do you need three
variables to convey this info..? Use a single variable please

> --
> > Looking at the usage in following patches, why cant this be handled
> > internally as part of prep call?
> >
> As per design, i2c driver iterates over each message and submits to GPI
> where it creates TRE. Since it's per transfer, we need to create Lock and
> Unlock TRE based on first or last message.
> --
> > > +	bool first_msg;
> > > +	bool last_msg;
> > 

-- 
~Vinod