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: <evtz6sp74wp2eiefmiefwwo7nffvj3aiz3o3gyrwojtxh4u7bv@hrbrfgedtmzk>
Date: Wed, 8 Jan 2025 18:01:00 +0100
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 4/5] i2c: tegra: Add support for SW Mutex register

On Wed, Jan 08, 2025 at 04:36:19PM +0530, Kartik Rajput wrote:
> From: Akhil R <akhilrajeev@...dia.com>
> 
> Add support for SW Mutex register introduced in Tegra264 to provide

The spelling is a bit inconsistent. Earlier you referred to this as SW
MUTEX register, which makes sense if that's what it's called. But then
you call it "SW Mutex" register here. If you don't want to refer to it
by the documented name, it should probably be "SW mutex" instead.

> an option to share the interface between multiple firmware and/or

"firmwares"

> Virtual Machines.

"virtual machines" 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>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 126 +++++++++++++++++++++++++++++----
>  1 file changed, 111 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index cf05937cb826..a5974af5b1af 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -135,6 +135,11 @@
>  #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
> +
>  /* configuration load timeout in microseconds */
>  #define I2C_CONFIG_LOAD_TIMEOUT			1000000
>  
> @@ -202,6 +207,7 @@ enum msg_end_type {
>   *		in HS mode.
>   * @has_interface_timing_reg: Has interface timing register to program the tuned
>   *		timing settings.
> + * @has_mutex: Has Mutex register for mutual exclusion with other firmwares or VM.

"mutex"

>   */
>  struct tegra_i2c_hw_feature {
>  	bool has_continue_xfer_support;
> @@ -228,6 +234,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;
>  };
>  
>  /**
> @@ -371,6 +378,99 @@ 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)
> +		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)
> +{
> +	/* Poll until mutex is acquired. */
> +	while (tegra_i2c_mutex_trylock(i2c_dev))
> +		cpu_relax();
> +}

Don't we want to use a timeout here? Otherwise we risk blocking the
thread that this runs on if some firmware decides not to release the
mutex.

Also, is the logic not the wrong way around? I.e. trylock returns true
if the hardware mutex was successfully locked, in which case it doesn't
make sense to keep spinning, right? Or do I misunderstand how this
works?

Thierry

> +
> +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;
> @@ -546,21 +646,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;
> @@ -1497,6 +1582,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 = {
> @@ -1521,6 +1607,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 = {
> @@ -1545,6 +1632,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 = {
> @@ -1569,6 +1657,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 = {
> @@ -1593,6 +1682,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 = {
> @@ -1617,6 +1707,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 = {
> @@ -1644,6 +1735,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 tegra_i2c_hw_feature tegra264_i2c_hw = {
> @@ -1671,6 +1763,7 @@ static const struct tegra_i2c_hw_feature tegra264_i2c_hw = {
>  	.setup_hold_time_hs_mode = 0x090909,
>  	.has_interface_timing_reg = true,
>  	.has_hs_mode_support = true,
> +	.has_mutex = true,
>  };
>  
>  static const struct of_device_id tegra_i2c_of_match[] = {
> @@ -1875,6 +1968,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.0
> 

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ