linux-kernel - Re: [PATCH 2/2] i2c: designware: Implement atomic transfer suppot

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] [day] [month] [year] [list]

Message-ID: <aKX_PwQWoe9S0QjP@smile.fi.intel.com>
Date: Wed, 20 Aug 2025 20:00:47 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Jisheng Zhang <jszhang@...nel.org>
Cc: Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Jan Dabros <jsd@...ihalf.com>, Andi Shyti <andi.shyti@...nel.org>,
	linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org
Subject: Re: [PATCH 2/2] i2c: designware: Implement atomic transfer suppot

On Wed, Aug 20, 2025 at 11:31:25PM +0800, Jisheng Zhang wrote:
> Rework the read and write code paths in the driver to support operation
> in atomic contexts. To achieve this, the driver must not rely on IRQs
> or perform any scheduling, e.g., via a sleep or schedule routine.
> 
> Implement atomic, sleep-free, and IRQ-less operation. This increases
> complexity but is necessary for atomic I2C transfers required by some
> hardware configurations, e.g., to trigger reboots on an external PMIC chip.

...

> -		usleep_range(25, 250);
> +		if (dev->atomic)
> +			udelay(25);
> +		else
> +			usleep_range(25, 250);

Wondering why this delay is not being properly calculated. Why in atomic case
is okay to use the shortest one?

...

> -	if (!dev->acquire_lock)
> +	if (dev->atomic || !dev->acquire_lock)

I think basically we should no allow atomic transfers at all when the lock is
in use. Otherwise it will be interesting case if HW (FW) wants to have an
exclusive access while OS wants to perform an atomic transfer.

...

> +	if (dev->atomic)
> +		return regmap_read_poll_timeout_atomic(dev->map, DW_IC_STATUS, status,
> +						       !(status & DW_IC_STATUS_MASTER_ACTIVITY),
> +						       1100, 20000) != 0;
> +	else
> +		return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> +					       !(status & DW_IC_STATUS_MASTER_ACTIVITY),
> +					       1100, 20000) != 0;

Please, drop ' != 0' parts at the same time, they are redundant.

...

> +static int i2c_dw_wait_transfer_atomic(struct dw_i2c_dev *dev)
> +{
> +	ktime_t timeout = ktime_add_us(ktime_get(), jiffies_to_usecs(dev->adapter.timeout));
> +	unsigned int stat;
> +	int ret;
> +
> +	do {
> +		ret = try_wait_for_completion(&dev->cmd_complete);
> +		if (ret)
> +			break;
> +
> +		stat = i2c_dw_read_clear_intrbits(dev);
> +		if (stat)
> +			i2c_dw_process_transfer(dev, stat);
> +		else
> +			udelay(15);

No explanation about this value.

> +	} while (ktime_compare(ktime_get(), timeout) < 0);

Whe have _before() and _after() APIs, use them.

> +	return ret ? 0 : -ETIMEDOUT;
> +}

...

>  	switch (dev->flags & MODEL_MASK) {
>  	case MODEL_AMD_NAVI_GPU:
> +		if (dev->atomic) {
> +			ret = -EOPNOTSUPP;
> +			goto done_nolock;
> +		}

Why only AMD case?

>  		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
>  		goto done_nolock;
>  	default:
>  		break;
>  	}

-- 
With Best Regards,
Andy Shevchenko