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

[Krzysztof Kozlowski <krzk@kernel.org>]

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