linux-kernel - Re: [PATCH v6 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: <l4po62bw6672xpaabkbvu6snyg4hrgcdxaijpt6evizortwjok@jwg2asezj3cb>
Date: Fri, 5 Sep 2025 01:28:00 +0200
From: Andi Shyti <andi.shyti@...nel.org>
To: Kartik Rajput <kkartik@...dia.com>
Cc: akhilrajeev@...dia.com, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, thierry.reding@...il.com, 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 v6 4/5] i2c: tegra: Add support for SW mutex register

Hi Kartik,

On Thu, Aug 28, 2025 at 11:29:32AM +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.

You could add a short description on how to use the mutex
register here.

> 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: Kartik Rajput <kkartik@...dia.com>
> Signed-off-by: Akhil R <akhilrajeev@...dia.com>

...

> @@ -381,6 +391,73 @@ 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_mutex_acquired(struct tegra_i2c_dev *i2c_dev)

this is a bool function.

> +{
> +	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 != I2C_SW_MUTEX_ID_CCPLEX)
> +		return 0;
> +
> +	return 1;

return id != I2C_SW_MUTEX_ID_CCPLEX;

> +}
> +
> +static int tegra_i2c_mutex_trylock(struct tegra_i2c_dev *i2c_dev)

I think this can be bool.

> +{
> +	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_CCPLEX)
> +		return 0;
> +
> +	val = FIELD_PREP(I2C_SW_MUTEX_REQUEST, I2C_SW_MUTEX_ID_CCPLEX);
> +	writel(val, i2c_dev->base + reg);
> +
> +	return tegra_i2c_mutex_acquired(i2c_dev);
> +}
> +
> +static int tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev)
> +{
> +	int locked;

I guess this can be bool.

> +	int ret;
> +
> +	if (i2c_dev->atomic_mode)
> +		ret = read_poll_timeout_atomic(tegra_i2c_mutex_trylock, locked, locked,
> +					       USEC_PER_MSEC, I2C_SW_MUTEX_TIMEOUT_US,
> +					       false, i2c_dev);
> +	else
> +		ret = read_poll_timeout(tegra_i2c_mutex_trylock, locked, locked, USEC_PER_MSEC,
> +					I2C_SW_MUTEX_TIMEOUT_US, false, i2c_dev);
> +
> +	if (!tegra_i2c_mutex_acquired(i2c_dev))
> +		dev_warn(i2c_dev->dev, "failed to acquire mutex\n");

I would try a few times before giving up.

Besides, is there a chance where ret is '0' and the mutex is not
acquired? If so, we are not signalling error if the mutex is not
acquired, but I think we should.

I would do:

	if (...)
		ret = ...
	else
		ret = ...
	
	if (ret)
		return ret;

	if (!tegra_i2c_mutex_acquired(i2c_dev)) {
		dev_warn(i2c_dev->dev, "failed to acquire mutex\n");
		return -ESOMETHING;
	}

	return 0;

Makes sense?

> +
> +	return ret;
> +}
> +
> +static int tegra_i2c_mutex_unlock(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 && id != I2C_SW_MUTEX_ID_CCPLEX) {
> +		dev_warn(i2c_dev->dev, "unable to unlock mutex, mutex is owned by: %u\n", id);
> +		return -EPERM;

I would try a few times before giving up.

> +	}
> +
> +	writel(0, i2c_dev->base + reg);
> +
> +	return 0;
> +}
> +
>  static void tegra_i2c_mask_irq(struct tegra_i2c_dev *i2c_dev, u32 mask)
>  {
>  	u32 int_mask;
> @@ -1422,6 +1499,13 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
>  		return ret;
>  	}
>  
> +

no need for this extra blank line.

> +	if (i2c_dev->hw->has_mutex) {

I would put this check in tegra_i2c_mutex_lock() and _unlock() in
order to avoid two level indentation here.

> +		ret = tegra_i2c_mutex_lock(i2c_dev);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	for (i = 0; i < num; i++) {
>  		enum msg_end_type end_type = MSG_END_STOP;
>  
> @@ -1451,6 +1535,12 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
>  			break;
>  	}
>  
> +	if (i2c_dev->hw->has_mutex) {
> +		ret = tegra_i2c_mutex_unlock(i2c_dev);
> +		if (ret)
> +			return ret;

We are skipping pm_runtime_put(), though.

Thanks,
Andi

> +	}
> +
>  	pm_runtime_put(i2c_dev->dev);
>  
>  	return ret ?: i;