linux-kernel - Re: [PATCH v3 4/4] i2c: i2c-qcom-geni: Enable i2c controller sharing between two subsystems

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: <71b5a108-a38b-4260-b683-d1a70178afc8@quicinc.com>
Date: Wed, 13 Nov 2024 21:40:37 +0530
From: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
To: Bjorn Andersson <andersson@...nel.org>,
        Dan Carpenter
	<dan.carpenter@...aro.org>
CC: <konrad.dybcio@...aro.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>, <vkoul@...nel.org>, <linux@...blig.org>,
        <Frank.Li@....com>, <konradybcio@...nel.org>,
        <bryan.odonoghue@...aro.org>, <krzk+dt@...nel.org>, <robh@...nel.org>
Subject: Re: [PATCH v3 4/4] i2c: i2c-qcom-geni: Enable i2c controller sharing
 between two subsystems

Hi Bjorn,

On 10/1/2024 8:09 AM, Bjorn Andersson wrote:
> On Mon, Sep 30, 2024 at 11:21:23AM GMT, Dan Carpenter wrote:
>> On Sun, Sep 29, 2024 at 10:46:37PM -0500, Bjorn Andersson wrote:
>>>> @@ -602,6 +603,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>>>>   	peripheral.clk_div = itr->clk_div;
>>>>   	peripheral.set_config = 1;
>>>>   	peripheral.multi_msg = false;
>>>> +	peripheral.shared_se = gi2c->se.shared_geni_se;
>>>>   
>>>>   	for (i = 0; i < num; i++) {
>>>>   		gi2c->cur = &msgs[i];
>>>> @@ -612,6 +614,8 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>>>>   		if (i < num - 1)
>>>>   			peripheral.stretch = 1;
>>>>   
>>>> +		peripheral.first_msg = (i == 0);
>>>> +		peripheral.last_msg = (i == num - 1);
>>>
>>> There are multiple error paths in this loop, which would result in us
>>> never issuing the unlock TRE - effectively blocking other subsystems
>>> from accessing the serial engine until we perform our next access
>>> (assuming that APSS issuing a lock TRE when APSS already has the channel
>>> locked isn't a problem?)
>>>
>>
>> Hi Bjorn,
>>
>> I saw the words "error paths" and "unlock" and I thought there was maybe
>> something we could do here with static analysis.
> 
> Appreciate you picking up on those topics :)
> 
>> But I don't know what TRE or APSS mean.
>>
> 
> The "APSS" is "the application processor sub system", which is where
> we typically find Linux running. TRE is, if I understand correctly, a
> request on the DMA engine queue.
Yes, Thats right. Transfer ring element, it's like a command to the 
engine. Can be configuration TRE, DMA xfer request TRE etc.
> 
>> The one thing I do see is that this uses "one err" style error handling where
>> there is one err label and it calls dmaengine_terminate_sync(gi2c->rx_c)
>> regardless of whether or not geni_i2c_gpi() was called or failed/succeeded.
>>
> 
> The scheme presented in this series injects a "LOCK" request before the
> DMA request marked first_msg, and an "UNLOCK" after the ones marked
> last_msg. This is needed because the controller is also concurrently by
> some DSP (or similar), so the LOCK/UNLOCK scheme forms mutual exclusion
> of the operations between the Linux and DSP systems.
> 
> I'm not sure if it's possible to tie the unlock operation to
> dmaengine_terminate_sync() in some way.
> 
I think terminate_sync() should clean up all xfers and will continue for 
the next request as a fresh start.
> Giving this some more thought, it feels like the current scheme serves
> the purpose of providing mutual exclusion both for the controller and
> to some degree for the device. But I'd like to understand why we can't
> inject the lock/unlock implicitly for each transfer...
> 
Explicitly adding lock/unlock per transfer adds execution load. Lock is 
meant for taking an ownership of the bus which is better to manage per 
session.
> Regards,
> Bjorn
> 
>> regards,
>> dan carpenter
>>