| 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
| ||
|
Message-ID: <3a9f8cea-c84f-4e63-bdf1-faa016566a65@quicinc.com>
Date: Wed, 13 Nov 2024 21:39:58 +0530
From: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
To: Bjorn Andersson <andersson@...nel.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>,
<dan.carpenter@...aro.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
Thanks for the review Bjorn !
On 9/30/2024 9:16 AM, Bjorn Andersson wrote:
> On Fri, Sep 27, 2024 at 12:01:08PM GMT, Mukesh Kumar Savaliya wrote:
>> Add support to share I2C SE by two Subsystems 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.
>>
>
> Please start your commit message by describing the problem your patch
> is solving.
>
Done
>> Sharing of SE(Serial engine) is possible only for GSI mode as each
>> subsystem(SS) 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.
>>
>> 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.
>>
>
> This last paragraph describes patch 3, right?
Yes, i think i should i keep it in patch 3.
>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
>> ---
>> drivers/i2c/busses/i2c-qcom-geni.c | 24 ++++++++++++++++++++----
>> 1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>> index 212336f724a6..479fa8e1c33f 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -1,5 +1,6 @@
>> // SPDX-License-Identifier: GPL-2.0
>> // Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>> +// Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>
>> #include <linux/acpi.h>
>> #include <linux/clk.h>
>> @@ -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?)
>
>> peripheral.addr = msgs[i].addr;
>>
>> ret = geni_i2c_gpi(gi2c, &msgs[i], &config,
>> @@ -631,8 +635,11 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>> dma_async_issue_pending(gi2c->tx_c);
>>
>> time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
>> - if (!time_left)
>> + if (!time_left) {
>> + dev_dbg(gi2c->se.dev, "I2C timeout gpi flags:%d addr:0x%x\n",
>> + gi2c->cur->flags, gi2c->cur->addr);
>
> This looks useful, but unrelated to this patch.
Sure, Removed it.
>
>> gi2c->err = -ETIMEDOUT;
>> + }
>>
>> if (gi2c->err) {
>> ret = gi2c->err;
>> @@ -800,6 +807,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
>> gi2c->clk_freq_out = KHZ(100);
>> }
>>
>> + if (of_property_read_bool(pdev->dev.of_node, "qcom,shared-se")) {
>> + gi2c->se.shared_geni_se = true;
>
> gi2c->se.shared_geni_se = of_property_read_bool(dev->of_node, "qcom,shared-se");
>
>> + dev_dbg(&pdev->dev, "I2C is shared between subsystems\n");
>> + }
>> +
>> if (has_acpi_companion(dev))
>> ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
>>
>> @@ -870,8 +882,10 @@ static int geni_i2c_probe(struct platform_device *pdev)
>> else
>> fifo_disable = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
>>
>> - if (fifo_disable) {
>> - /* FIFO is disabled, so we can only use GPI DMA */
>> + if (fifo_disable || gi2c->se.shared_geni_se) {
>> + /* FIFO is disabled, so we can only use GPI DMA.
>> + * SE can be shared in GSI mode between subsystems, each SS owns a GPII.
>> + **/
>
> I think you're trying to document why we're entering the "GPI-only"
> branch. The addition you made was that if the user has requested
> "shared-se", then it's GPI-only.
>
yes, that's right.
> But I'm not able to wrap my head around your addition here. Why does it
> matter that each subsystem own a GPII? Is that a reason for choosing
> GPI-only mode?
Not sure i got your question here.
The feature flag true means it should be in GPI mode.
>
>> gi2c->gpi_mode = true;
>> ret = setup_gpi_dma(gi2c);
>> if (ret) {
>> @@ -883,6 +897,9 @@ static int geni_i2c_probe(struct platform_device *pdev)
>> dev_dbg(dev, "Using GPI DMA mode for I2C\n");
>> } else {
>> gi2c->gpi_mode = false;
>> +
>> + /* Force disable shared SE case for non GSI mode */
>
> GSI or GPI mode?
Changed to GPI.
>
>> + gi2c->se.shared_geni_se = false;
>
> If shared_geni_se was true prior to this assignment, wouldn't we have
> entered the if (fifo_disable ...) branch?
In that case also we should enter the condition of executing in GPI mode.
>
>> tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
>>
>> /* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
>> @@ -964,7 +981,6 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
>> if (ret) {
>> enable_irq(gi2c->irq);
>> return ret;
>> -
>
> Please avoid such unrelated cleanups.
>
Sure
> Regards,
> Bjorn
>
>> } else {
>> gi2c->suspended = 1;
>> }
>> --
>> 2.25.1
>>
Powered by blists - more mailing lists