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] [day] [month] [year] [list]

Message-ID: <b34e3ac0-70b4-491c-a807-dc13fac41d06@quicinc.com>
Date: Thu, 26 Dec 2024 17:52:39 +0530
From: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
To: Vinod Koul <vkoul@...nel.org>, Md Sadre Alam <quic_mdalam@...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 12/24/2024 3:28 PM, Vinod Koul wrote:
> On 18-12-24, 18:04, Mukesh Kumar Savaliya wrote:
>> Hi Vinod, Thanks !  I just saw your comments now as somehow it was going in
>> some other folder and didn't realize.
>>
>> On 12/4/2024 5:51 PM, Vinod Koul wrote:
>>> 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
>> Yes, I think so. Please let me clarify.
>> First variable is a feature flag and it's required to be explicitly
>> mentioned by client (i2c/spi/etc) to GSI driver.
>>
>> Second and third, can be optimized to boolean so either first or last can be
>> passed.
>>
>> Please correct me or add simple change where you would like to make, i can
>> add that.
> 
> I though we could do with a single and derive
> 
Sure, so as mentioned in the other crypto BAM patch probably dmaengine.h 
can hold flag and that can add support for lock/unlock similar to that 
patch.
I just realized it from your shared patch. let me work internally with 
Md sadre and review. Thanks for the comment.
> Also, please see 20241212041639.4109039-3-quic_mdalam@...cinc.com, folks
> from same company should talk together on same solutions, please
> converge and come up with a single proposal which works for both drivers
> 
Sure