From: Akhil R <akhilrajeev@nvidia.com>
Add support for SW mutex register introduced in Tegra264 to provide
an option to share the interface between multiple firmwares and/or
VMs.
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: Akhil R <akhilrajeev@nvidia.com>
Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
---
v1 -> v2:
* Fixed typos.
* Fix tegra_i2c_mutex_lock() logic.
* Add a timeout in tegra_i2c_mutex_lock() instead of polling for
mutex indefinitely.
---
drivers/i2c/busses/i2c-tegra.c | 132 +++++++++++++++++++++++++++++----
1 file changed, 117 insertions(+), 15 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 7c8b76406e2e..aa92faa6f5cb 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -135,6 +135,14 @@
#define I2C_MST_FIFO_STATUS_TX GENMASK(23, 16)
#define I2C_MST_FIFO_STATUS_RX GENMASK(7, 0)
+#define I2C_SW_MUTEX 0x0ec
+#define I2C_SW_MUTEX_REQUEST GENMASK(3, 0)
+#define I2C_SW_MUTEX_GRANT GENMASK(7, 4)
+#define I2C_SW_MUTEX_ID 9
+
+/* SW mutex acquire timeout value in milliseconds. */
+#define I2C_SW_MUTEX_TIMEOUT 25
+
/* configuration load timeout in microseconds */
#define I2C_CONFIG_LOAD_TIMEOUT 1000000
@@ -203,6 +211,7 @@ enum msg_end_type {
* @has_interface_timing_reg: Has interface timing register to program the tuned
* timing settings.
* @has_hs_mode_support: Has support for high speed (HS) mode transfers.
+ * @has_mutex: Has mutex register for mutual exclusion with other firmwares or VM.
*/
struct tegra_i2c_hw_feature {
bool has_continue_xfer_support;
@@ -229,6 +238,7 @@ struct tegra_i2c_hw_feature {
u32 setup_hold_time_hs_mode;
bool has_interface_timing_reg;
bool has_hs_mode_support;
+ bool has_mutex;
};
/**
@@ -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);
+
+ val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
+ id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
+
+ if (id != I2C_SW_MUTEX_ID)
+ return 0;
+
+ return 1;
+}
+
+static void tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev)
+{
+ unsigned int num_retries = I2C_SW_MUTEX_TIMEOUT;
+
+ /* Poll until mutex is acquired or timeout. */
+ while (--num_retries && !tegra_i2c_mutex_trylock(i2c_dev))
+ usleep_range(1000, 2000);
+
+ WARN_ON(!num_retries);
+}
+
+static void tegra_i2c_mutex_unlock(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 (WARN_ON(id != I2C_SW_MUTEX_ID))
+ return;
+
+ i2c_writel(i2c_dev, 0, I2C_SW_MUTEX);
+}
+
+static void tegra_i2c_bus_lock(struct i2c_adapter *adapter,
+ unsigned int flags)
+{
+ struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter);
+
+ rt_mutex_lock_nested(&adapter->bus_lock, i2c_adapter_depth(adapter));
+ tegra_i2c_mutex_lock(i2c_dev);
+}
+
+static int tegra_i2c_bus_trylock(struct i2c_adapter *adapter,
+ unsigned int flags)
+{
+ struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter);
+ int ret;
+
+ ret = rt_mutex_trylock(&adapter->bus_lock);
+ if (ret)
+ ret = tegra_i2c_mutex_trylock(i2c_dev);
+
+ return ret;
+}
+
+static void tegra_i2c_bus_unlock(struct i2c_adapter *adapter,
+ unsigned int flags)
+{
+ struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adapter);
+
+ rt_mutex_unlock(&adapter->bus_lock);
+ tegra_i2c_mutex_unlock(i2c_dev);
+}
+
+static const struct i2c_lock_operations tegra_i2c_lock_ops = {
+ .lock_bus = tegra_i2c_bus_lock,
+ .trylock_bus = tegra_i2c_bus_trylock,
+ .unlock_bus = tegra_i2c_bus_unlock,
+};
+
static void tegra_i2c_mask_irq(struct tegra_i2c_dev *i2c_dev, u32 mask)
{
u32 int_mask;
@@ -550,21 +657,6 @@ static void tegra_i2c_vi_init(struct tegra_i2c_dev *i2c_dev)
i2c_writel(i2c_dev, 0x0, I2C_TLOW_SEXT);
}
-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_flush_fifos(struct tegra_i2c_dev *i2c_dev)
{
u32 mask, val, offset;
@@ -1503,6 +1595,7 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
.setup_hold_time_fast_fast_plus_mode = 0x0,
.setup_hold_time_hs_mode = 0x0,
.has_interface_timing_reg = false,
+ .has_mutex = false,
};
static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
@@ -1527,6 +1620,7 @@ static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
.setup_hold_time_fast_fast_plus_mode = 0x0,
.setup_hold_time_hs_mode = 0x0,
.has_interface_timing_reg = false,
+ .has_mutex = false,
};
static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
@@ -1551,6 +1645,7 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
.setup_hold_time_fast_fast_plus_mode = 0x0,
.setup_hold_time_hs_mode = 0x0,
.has_interface_timing_reg = false,
+ .has_mutex = false,
};
static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
@@ -1575,6 +1670,7 @@ static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
.setup_hold_time_fast_fast_plus_mode = 0x0,
.setup_hold_time_hs_mode = 0x0,
.has_interface_timing_reg = true,
+ .has_mutex = false,
};
static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
@@ -1599,6 +1695,7 @@ static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
.setup_hold_time_fast_fast_plus_mode = 0,
.setup_hold_time_hs_mode = 0,
.has_interface_timing_reg = true,
+ .has_mutex = false,
};
static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
@@ -1623,6 +1720,7 @@ static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
.setup_hold_time_fast_fast_plus_mode = 0,
.setup_hold_time_hs_mode = 0,
.has_interface_timing_reg = true,
+ .has_mutex = false,
};
static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
@@ -1650,6 +1748,7 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
.setup_hold_time_hs_mode = 0x090909,
.has_interface_timing_reg = true,
.has_hs_mode_support = true,
+ .has_mutex = false,
};
static const struct of_device_id tegra_i2c_of_match[] = {
@@ -1853,6 +1952,9 @@ static int tegra_i2c_probe(struct platform_device *pdev)
i2c_dev->adapter.nr = pdev->id;
ACPI_COMPANION_SET(&i2c_dev->adapter.dev, ACPI_COMPANION(&pdev->dev));
+ if (i2c_dev->hw->has_mutex)
+ i2c_dev->adapter.lock_ops = &tegra_i2c_lock_ops;
+
if (i2c_dev->hw->supports_bus_clear)
i2c_dev->adapter.bus_recovery_info = &tegra_i2c_recovery_info;
--
2.43.0On 30/01/2025 15:34, Kartik Rajput wrote:
> From: Akhil R <akhilrajeev@nvidia.com>
>
> Add support for SW mutex register introduced in Tegra264 to provide
> an option to share the interface between multiple firmwares and/or
> VMs.
>
> 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: Akhil R <akhilrajeev@nvidia.com>
> Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> ---
> v1 -> v2:
> * Fixed typos.
> * Fix tegra_i2c_mutex_lock() logic.
> * Add a timeout in tegra_i2c_mutex_lock() instead of polling for
> mutex indefinitely.
> ---
> drivers/i2c/busses/i2c-tegra.c | 132 +++++++++++++++++++++++++++++----
> 1 file changed, 117 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 7c8b76406e2e..aa92faa6f5cb 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -135,6 +135,14 @@
> #define I2C_MST_FIFO_STATUS_TX GENMASK(23, 16)
> #define I2C_MST_FIFO_STATUS_RX GENMASK(7, 0)
>
> +#define I2C_SW_MUTEX 0x0ec
> +#define I2C_SW_MUTEX_REQUEST GENMASK(3, 0)
> +#define I2C_SW_MUTEX_GRANT GENMASK(7, 4)
> +#define I2C_SW_MUTEX_ID 9
> +
> +/* SW mutex acquire timeout value in milliseconds. */
> +#define I2C_SW_MUTEX_TIMEOUT 25
> +
> /* configuration load timeout in microseconds */
> #define I2C_CONFIG_LOAD_TIMEOUT 1000000
>
> @@ -203,6 +211,7 @@ enum msg_end_type {
> * @has_interface_timing_reg: Has interface timing register to program the tuned
> * timing settings.
> * @has_hs_mode_support: Has support for high speed (HS) mode transfers.
> + * @has_mutex: Has mutex register for mutual exclusion with other firmwares or VM.
> */
> struct tegra_i2c_hw_feature {
> bool has_continue_xfer_support;
> @@ -229,6 +238,7 @@ struct tegra_i2c_hw_feature {
> u32 setup_hold_time_hs_mode;
> bool has_interface_timing_reg;
> bool has_hs_mode_support;
> + bool has_mutex;
> };
>
> /**
> @@ -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.
> +
> + val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
> + id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
> +
> + if (id != I2C_SW_MUTEX_ID)
> + return 0;
> +
> + return 1;
> +}
> +
> +static void tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev)
> +{
> + unsigned int num_retries = I2C_SW_MUTEX_TIMEOUT;
> +
> + /* Poll until mutex is acquired or timeout. */
> + while (--num_retries && !tegra_i2c_mutex_trylock(i2c_dev))
> + usleep_range(1000, 2000);
> +
> + WARN_ON(!num_retries);
Blocked thread is not a reason to reboot entire system (see panic on
warn). Drop or change to some dev_warn.
> +}
> +
> +static void tegra_i2c_mutex_unlock(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 (WARN_ON(id != I2C_SW_MUTEX_ID))
Same problem here.
Best regards,
Krzysztof
Thanks for reviewing the patch Krzysztof!
On Thu, 2025-01-30 at 17:12 +0100, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
>
>
> On 30/01/2025 15:34, Kartik Rajput wrote:
> > From: Akhil R <akhilrajeev@nvidia.com>
> >
> > Add support for SW mutex register introduced in Tegra264 to provide
> > an option to share the interface between multiple firmwares and/or
> > VMs.
> >
> > 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: Akhil R <akhilrajeev@nvidia.com>
> > Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> > ---
> > v1 -> v2:
> > * Fixed typos.
> > * Fix tegra_i2c_mutex_lock() logic.
> > * Add a timeout in tegra_i2c_mutex_lock() instead of polling
> > for
> > mutex indefinitely.
> > ---
> > drivers/i2c/busses/i2c-tegra.c | 132
> > +++++++++++++++++++++++++++++----
> > 1 file changed, 117 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-tegra.c
> > b/drivers/i2c/busses/i2c-tegra.c
> > index 7c8b76406e2e..aa92faa6f5cb 100644
> > --- a/drivers/i2c/busses/i2c-tegra.c
> > +++ b/drivers/i2c/busses/i2c-tegra.c
> > @@ -135,6 +135,14 @@
> > #define I2C_MST_FIFO_STATUS_TX GENMASK(23,
> > 16)
> > #define I2C_MST_FIFO_STATUS_RX GENMASK(7, 0)
> >
> > +#define I2C_SW_MUTEX 0x0ec
> > +#define I2C_SW_MUTEX_REQUEST GENMASK(3, 0)
> > +#define I2C_SW_MUTEX_GRANT GENMASK(7, 4)
> > +#define I2C_SW_MUTEX_ID 9
> > +
> > +/* SW mutex acquire timeout value in milliseconds. */
> > +#define I2C_SW_MUTEX_TIMEOUT 25
> > +
> > /* configuration load timeout in microseconds */
> > #define I2C_CONFIG_LOAD_TIMEOUT 1000000
> >
> > @@ -203,6 +211,7 @@ enum msg_end_type {
> > * @has_interface_timing_reg: Has interface timing register to
> > program the tuned
> > * timing settings.
> > * @has_hs_mode_support: Has support for high speed (HS) mode
> > transfers.
> > + * @has_mutex: Has mutex register for mutual exclusion with other
> > firmwares or VM.
> > */
> > struct tegra_i2c_hw_feature {
> > bool has_continue_xfer_support;
> > @@ -229,6 +238,7 @@ struct tegra_i2c_hw_feature {
> > u32 setup_hold_time_hs_mode;
> > bool has_interface_timing_reg;
> > bool has_hs_mode_support;
> > + bool has_mutex;
> > };
> >
> > /**
> > @@ -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.
> > +
> > + val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
> > + id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
> > +
> > + if (id != I2C_SW_MUTEX_ID)
> > + return 0;
> > +
> > + return 1;
> > +}
> > +
> > +static void tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev)
> > +{
> > + unsigned int num_retries = I2C_SW_MUTEX_TIMEOUT;
> > +
> > + /* Poll until mutex is acquired or timeout. */
> > + while (--num_retries && !tegra_i2c_mutex_trylock(i2c_dev))
> > + usleep_range(1000, 2000);
> > +
> > + WARN_ON(!num_retries);
>
>
> Blocked thread is not a reason to reboot entire system (see panic on
> warn). Drop or change to some dev_warn.
>
>
Ack. Will change this to dev_warn in v3.
> > +}
> > +
> > +static void tegra_i2c_mutex_unlock(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 (WARN_ON(id != I2C_SW_MUTEX_ID))
>
> Same problem here.
>
Ack.
>
>
> Best regards,
> Krzysztof
Thanks & Regards,
Kartik
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
On Thu, 2025-01-30 at 18:49 +0100, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
>
>
> 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?
>
Correct.
> 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.
>
Ack. I will update the implementation to use non-relaxed APIs instead.
>
> Best regards,
> Krzysztof
Thanks & Regards,
Kartik
© 2016 - 2026 Red Hat, Inc.