| 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: <34a91470-ad7a-4a57-bf9d-33b68b0a4473@nvidia.com>
Date: Tue, 16 Sep 2025 21:49:53 +0530
From: Kartik Rajput <kkartik@...dia.com>
To: Andi Shyti <andi.shyti@...nel.org>
Cc: Akhil R <akhilrajeev@...dia.com>, "robh@...nel.org" <robh@...nel.org>,
"krzk+dt@...nel.org" <krzk+dt@...nel.org>,
"conor+dt@...nel.org" <conor+dt@...nel.org>,
"thierry.reding@...il.com" <thierry.reding@...il.com>,
Jon Hunter <jonathanh@...dia.com>, Laxman Dewangan <ldewangan@...dia.com>,
"digetx@...il.com" <digetx@...il.com>,
"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 4/5] i2c: tegra: Add support for SW mutex register
Hi Andi,
Thanks for reviewing the patch!
On 05/09/25 04:58, Andi Shyti wrote:
> External email: Use caution opening links or attachments
>
>
> Hi Kartik,
>
> On Thu, Aug 28, 2025 at 11:29:32AM +0530, Kartik Rajput wrote:
>> Add support for SW mutex register introduced in Tegra264 to provide
>> an option to share the interface between multiple firmwares and/or
>> VMs.
>
> You could add a short description on how to use the mutex
> register here.
>
Ack.
>> However, the hardware does not ensure any protection based on the
>> values. The driver/firmware should honor the peer who already holds
>> the mutex.
>>
>> Signed-off-by: Kartik Rajput <kkartik@...dia.com>
>> Signed-off-by: Akhil R <akhilrajeev@...dia.com>
>
> ...
>
>> @@ -381,6 +391,73 @@ static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
>> readsl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
>> }
>>
>> +static int tegra_i2c_mutex_acquired(struct tegra_i2c_dev *i2c_dev)
>
> this is a bool function.
>
Ack.
>> +{
>> + unsigned int reg = tegra_i2c_reg_addr(i2c_dev, I2C_SW_MUTEX);
>> + u32 val, id;
>> +
>> + val = readl(i2c_dev->base + reg);
>> + id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
>> +
>> + if (id != I2C_SW_MUTEX_ID_CCPLEX)
>> + return 0;
>> +
>> + return 1;
>
> return id != I2C_SW_MUTEX_ID_CCPLEX;
>
Ack.
>> +}
>> +
>> +static int tegra_i2c_mutex_trylock(struct tegra_i2c_dev *i2c_dev)
>
> I think this can be bool.
>
Ack.
>> +{
>> + unsigned int reg = tegra_i2c_reg_addr(i2c_dev, I2C_SW_MUTEX);
>> + u32 val, id;
>> +
>> + val = readl(i2c_dev->base + reg);
>> + id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
>> + if (id != 0 && id != I2C_SW_MUTEX_ID_CCPLEX)
>> + return 0;
>> +
>> + val = FIELD_PREP(I2C_SW_MUTEX_REQUEST, I2C_SW_MUTEX_ID_CCPLEX);
>> + writel(val, i2c_dev->base + reg);
>> +
>> + return tegra_i2c_mutex_acquired(i2c_dev);
>> +}
>> +
>> +static int tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev)
>> +{
>> + int locked;
>
> I guess this can be bool.
>
Ack.
>> + int ret;
>> +
>> + if (i2c_dev->atomic_mode)
>> + ret = read_poll_timeout_atomic(tegra_i2c_mutex_trylock, locked, locked,
>> + USEC_PER_MSEC, I2C_SW_MUTEX_TIMEOUT_US,
>> + false, i2c_dev);
>> + else
>> + ret = read_poll_timeout(tegra_i2c_mutex_trylock, locked, locked, USEC_PER_MSEC,
>> + I2C_SW_MUTEX_TIMEOUT_US, false, i2c_dev);
>> +
>> + if (!tegra_i2c_mutex_acquired(i2c_dev))
>> + dev_warn(i2c_dev->dev, "failed to acquire mutex\n");
>
> I would try a few times before giving up.
>
Yes, we are retrying with read_poll_timeout_* calls.
> Besides, is there a chance where ret is '0' and the mutex is not
In case of failure ret should always be set to -ETIMEDOUT.
> acquired? If so, we are not signalling error if the mutex is not
> acquired, but I think we should.
>
> I would do:
>
> if (...)
> ret = ...
> else
> ret = ...
>
> if (ret)
> return ret;
>
> if (!tegra_i2c_mutex_acquired(i2c_dev)) {
> dev_warn(i2c_dev->dev, "failed to acquire mutex\n");
> return -ESOMETHING;
> }
>
> return 0;
>
> Makes sense?
I agree, we can simplify this logic by just checking the value of ret instead
of calling tegra_i2c_mutex_acquired() again as tegra_i2c_mutex_trylock() already
checks if we have acquired the mutex or not.
...
if (ret)
dev_warn(i2c_dev->dev, "failed to acquire mutex\n");
return ret;
}
>
>> +
>> + return ret;
>> +}
>> +
>> +static int tegra_i2c_mutex_unlock(struct tegra_i2c_dev *i2c_dev)
>> +{
>> + unsigned int reg = tegra_i2c_reg_addr(i2c_dev, I2C_SW_MUTEX);
>> + u32 val, id;
>> +
>> + val = readl(i2c_dev->base + reg);
>> +
>> + id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
>> + if (id && id != I2C_SW_MUTEX_ID_CCPLEX) {
>> + dev_warn(i2c_dev->dev, "unable to unlock mutex, mutex is owned by: %u\n", id);
>> + return -EPERM;
>
> I would try a few times before giving up.
>
Unlocking the mutex should fail if unlock is called and CCPLEX is not the owner.
In that case no need to retry.
>> + }
>> +
>> + writel(0, i2c_dev->base + reg);
>> +
>> + return 0;
>> +}
>> +
>> static void tegra_i2c_mask_irq(struct tegra_i2c_dev *i2c_dev, u32 mask)
>> {
>> u32 int_mask;
>> @@ -1422,6 +1499,13 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
>> return ret;
>> }
>>
>> +
>
> no need for this extra blank line.
>
Ack.
>> + if (i2c_dev->hw->has_mutex) {
>
> I would put this check in tegra_i2c_mutex_lock() and _unlock() in
> order to avoid two level indentation here.
>
Ack. It will look cleaner if the check is moved inside the lock and unlock functions.
>> + ret = tegra_i2c_mutex_lock(i2c_dev);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> for (i = 0; i < num; i++) {
>> enum msg_end_type end_type = MSG_END_STOP;
>>
>> @@ -1451,6 +1535,12 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
>> break;
>> }
>>
>> + if (i2c_dev->hw->has_mutex) {
>> + ret = tegra_i2c_mutex_unlock(i2c_dev);
>> + if (ret)
>> + return ret;
>
> We are skipping pm_runtime_put(), though.
>
> Thanks,
> Andi
>
Yes, we skip calling pm_runtime_put() if unlocking fails. I will fix this in the next revision.
>> + }
>> +
>> pm_runtime_put(i2c_dev->dev);
>>
>> return ret ?: i;
Thanks & Regards,
Kartik
Powered by blists - more mailing lists