| 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: <wgpcfddybwgdt4ggllooh35iukid24urb7mrbrcd5egs4blqyv@hty6js2piqug>
Date: Tue, 10 Jun 2025 09:49:47 +0200
From: Thierry Reding <thierry.reding@...il.com>
To: Kartik Rajput <kkartik@...dia.com>
Cc: akhilrajeev@...dia.com, andi.shyti@...nel.org, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, jonathanh@...dia.com, ldewangan@...dia.com,
digetx@...il.com, linux-i2c@...r.kernel.org, devicetree@...r.kernel.org,
linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 4/5] i2c: tegra: Add support for SW mutex register
On Mon, Jun 09, 2025 at 03:04:19PM +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.
>
> 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@...dia.com>
> Signed-off-by: Kartik Rajput <kkartik@...dia.com>
> ---
> v2 -> v3:
> * Update tegra_i2c_mutex_trylock and tegra_i2c_mutex_unlock to
> use readl and writel APIs instead of i2c_readl and i2c_writel
> which use relaxed APIs.
> * Use dev_warn instead of WARN_ON if mutex lock/unlock fails.
> 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 | 137 +++++++++++++++++++++++++++++----
> 1 file changed, 122 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index d0b6aa013c96..dae59e9e993b 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -137,6 +137,14 @@
>
> #define I2C_MASTER_RESET_CNTRL 0x0a8
>
> +#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
Maybe this should contain some sort of suffix to denote which ID this
is? Maybe I2C_SW_MUTEX_ID_CCPLEX?
> +
> +/* SW mutex acquire timeout value in milliseconds. */
> +#define I2C_SW_MUTEX_TIMEOUT 25
> +
> /* configuration load timeout in microseconds */
> #define I2C_CONFIG_LOAD_TIMEOUT 1000000
>
> @@ -210,6 +218,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 VMs.
> */
> struct tegra_i2c_hw_feature {
> bool has_continue_xfer_support;
> @@ -237,6 +246,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;
> };
>
> /**
> @@ -380,6 +390,108 @@ 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);
> +}
The move of this function seems unnecessary.
> +
> +static int tegra_i2c_mutex_trylock(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 != 0 && id != I2C_SW_MUTEX_ID)
> + return 0;
> +
> + val = FIELD_PREP(I2C_SW_MUTEX_REQUEST, I2C_SW_MUTEX_ID);
> + writel(val, i2c_dev->base + reg);
> +
> + val = readl(i2c_dev->base + reg);
> + id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
> +
> + if (id != I2C_SW_MUTEX_ID)
> + return 0;
> +
> + return 1;
> +}
Do we need some sort of locking around these? Or is this always
guaranteed to be called from a locked region already?
> +
> +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);
Maybe this can be rewritten to be easier on the eye? Something like:
while (--num_retries) {
if (tegra_i2c_mutex_trylock(i2c_dev))
break;
usleep_range(1000, 2000);
}
looks a bit easier to follow. It may also be better to change this to a
properly timed loop. As it is, the usleep_range() can take anywhere from
1 to 2 ms, so effectively I2C_SW_MUTEX_TIMEOUT 25 can be 50 ms long. I'm
sure that doesn't matter all that much, but it's a bit of an ambiguous
specification. So I think we should either be precise with a timed loop
if we specify the timeout in milliseconds or we should not pretend that
we care about the specific time and rename the variable to something
like I2C_SW_MUTEX_RETRIES instead. I prefer the timed loop variant, and
I think there's ways you can use helpers from linux/iopoll.h to achieve
this (i.e. use the generic read_poll_timeout() with
tegra_i2c_mutex_trylock as op parameter and passing i2c_dev as args).
> +
> + if (!num_retries)
> + dev_warn(i2c_dev->dev, "timeout while acquiring mutex, proceeding anyway\n");
> +}
I take it there's no way to refuse operations since there's no return
value for this function? I wonder if it's the right interface for this,
then. If there's no mechanism to enforce the lock in hardware, then we
must somehow respect it in software. Proceeding even after failing to
acquire the lock seems like a recipe for breaking things.
Also, this looks slightly wrong. What if the trylock succeeds on the
last retry? num_retries will have been decremented to 0 at that point,
so we'll see the warning even if it did succeed.
Thierry
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists