linux-kernel - Re: [PATCH v2 4/5] i2c: tegra: Add support for SW mutex register

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: <4b9777cc-0bb3-44c1-92f8-209c30837f20@kernel.org>
Date: Thu, 30 Jan 2025 18:49:14 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Kartik Rajput <kkartik@...dia.com>,
 "thierry.reding@...il.com" <thierry.reding@...il.com>,
 Jon Hunter <jonathanh@...dia.com>, Akhil R <akhilrajeev@...dia.com>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "robh@...nel.org" <robh@...nel.org>, Laxman Dewangan <ldewangan@...dia.com>,
 "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
 "andi.shyti@...nel.org" <andi.shyti@...nel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "conor+dt@...nel.org" <conor+dt@...nel.org>,
 "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
 "digetx@...il.com" <digetx@...il.com>,
 "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH v2 4/5] i2c: tegra: Add support for SW mutex register

On 30/01/2025 17:35, Kartik Rajput wrote:
>>>  /**
>>> @@ -372,6 +382,103 @@ 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_poll_register(struct tegra_i2c_dev *i2c_dev,
>>> +                                u32 reg, u32 mask, u32 delay_us,
>>> +                                u32 timeout_us)
>>> +{
>>> +     void __iomem *addr = i2c_dev->base +
>>> tegra_i2c_reg_addr(i2c_dev, reg);
>>> +     u32 val;
>>> +
>>> +     if (!i2c_dev->atomic_mode)
>>> +             return readl_relaxed_poll_timeout(addr, val, !(val &
>>> mask),
>>> +                                               delay_us,
>>> timeout_us);
>>> +
>>> +     return readl_relaxed_poll_timeout_atomic(addr, val, !(val &
>>> mask),
>>> +                                              delay_us,
>>> timeout_us);
>>> +}
>>> +
>>> +static int tegra_i2c_mutex_trylock(struct tegra_i2c_dev *i2c_dev)
>>> +{
>>> +     u32 val, id;
>>> +
>>> +     val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
>>> +     id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
>>> +     if (id != 0 && id != I2C_SW_MUTEX_ID)
>>> +             return 0;
>>> +
>>> +     val = FIELD_PREP(I2C_SW_MUTEX_REQUEST, I2C_SW_MUTEX_ID);
>>> +     i2c_writel(i2c_dev, val, I2C_SW_MUTEX);
>>
>> And how do you exactly prevent concurrent, overwriting write? This
>> looks
>> like pure race.
>>
> 
> The I2C_SW_MUTEX_GRANT field reflects the id of the current mutex
> owner. The I2C_SW_MUTEX_GRANT field does not change with overwrites to
> the I2C_SW_MUTEX_REQUEST field, unless I2C_SW_MUTEX_REQUEST field is
> cleared.


So second concurrent write to I2C_SW_MUTEX_REQUEST will fail silently,
and you rely on below check which ID succeeded to write?

If that is how it works, then should succeed... except the trouble is
that you use here i2c_readl/writel wrappers (which was already a poor
idea, because it hides the implementation for no real gain) and it turns
out they happen to be relaxed making all your assumptions about ordering
inaccurate. You need to switch to non-relaxed API.


Best regards,
Krzysztof