linux-kernel - Re: [PATCH v4 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: <8e0ffc82-e205-4ea9-8b84-7437564e28cd@quicinc.com>
Date: Mon, 25 Nov 2024 10:56:23 +0530
From: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>, <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>, <vkoul@...nel.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>
CC: <quic_vdadhani@...cinc.com>
Subject: Re: [PATCH v4 4/4] i2c: i2c-qcom-geni: Enable i2c controller sharing
 between two subsystems

Thanks Konrad !

On 11/22/2024 7:12 PM, Konrad Dybcio wrote:
> On 18.11.2024 6:45 AM, Mukesh Kumar Savaliya wrote:
>> Thanks for the review konrad !
>>
>> On 11/16/2024 12:58 AM, Konrad Dybcio wrote:
>>> On 13.11.2024 5:14 PM, Mukesh Kumar Savaliya wrote:
>>>> Add support to share I2C controller in multiprocessor system in a mutually
>>>> exclusive way. Use "qcom,shared-se" flag in a particular i2c instance node
>>>> if the usecase requires i2c controller to be shared.
>>>
>>> Can we read back some value from the registers to know whether such sharing
>>> takes place?
>> Actually, HW register doesn't provide such mechanism, it's add on feature if SE is programmed for GSI mode.
> 
> So it's more of an unwritten contract between subsystems.. okay
> 
yes, purely SW flag based decision.
>>>
>>>> Sharing of I2C SE(Serial engine) is possible only for GSI mode as client
>>>> from each processor can queue transfers over its own GPII Channel. For
>>>> non GSI mode, we should force disable this feature even if set by user
>>>> from DT by mistake.
>>>
>>> The DT is to be taken authoritatively
>>>
>> To clarify - Does it mean i should not have SW disable this feature OR override  ? And let it continue in non GSI mode even it's not going to work ?
> 
> If a configuration is invalid, you should return -EINVAL from probe,
> with an appropriate error message.
> 
Sure, i agree. I will make a change here and will bail out with an error 
and print logs  if (gi2c->se.shared_geni_se == true). Thanks for this 
suggestion.

>>>>
>>>> I2C driver just need to mark first_msg and last_msg flag to help indicate
>>>> GPI driver to take lock and unlock TRE there by protecting from concurrent
>>>> access from other EE or Subsystem.
>>>>
>>>> gpi_create_i2c_tre() function at gpi.c will take care of adding Lock and
>>>> Unlock TRE for the respective transfer operations.
>>>>
>>>> Since the GPIOs are also shared between two SS, do not unconfigure them
>>>> during runtime suspend. This will allow other SS to continue to transfer
>>>> the data without any disturbance over the IO lines.
>>>>
>>>> For example, Assume an I2C EEPROM device connected with an I2C controller.
>>>> Each client from ADSP and APPS processor can perform i2c transactions
>>>> without any disturbance from each other.
>>>>
>>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
>>>> ---
>>>
>>> [...]
>>>
>>>>        } else {
>>>>            gi2c->gpi_mode = false;
>>>> +
>>>> +        /* Force disable shared SE case for non GSI mode */
>>>> +        gi2c->se.shared_geni_se = false;
>>>
>>> Doing this silently sounds rather odd..
>> we can have Some SW logging added ?
> 
> Normally such overrides mandate a warning/notice, but as I said above,
> we should disallow such combinations altogether for sanity
> 
Sure, taken cared with above suggestion.
> Konrad