linux-kernel - Re: [PATCH 1/2] i2c: ocores: replace 1ms poll iteration timeout with total transfer timeout

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: <445z46hybgl2mdmhsapiitqccxkkpafqstgksov45sfd5v2c2m@mmiye6kt6itb>
Date: Fri, 10 Oct 2025 00:25:13 +0200
From: Andi Shyti <andi.shyti@...nel.org>
To: Matthias Schiffer <matthias.schiffer@...tq-group.com>
Cc: Peter Korsgaard <peter@...sgaard.com>, Andrew Lunn <andrew@...n.ch>, 
	linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org, linux@...tq-group.com
Subject: Re: [PATCH 1/2] i2c: ocores: replace 1ms poll iteration timeout with
 total transfer timeout

Hi Matthias,

On Tue, Oct 07, 2025 at 02:09:24PM +0200, Matthias Schiffer wrote:
> When a target makes use of clock stretching, a timeout of 1ms may not be
> enough. One extreme example is the NXP PTN3460 eDP to LVDS bridge, which
> takes ~320ms to send its ACK after a flash command has been
> submitted.

besides, the specification doesn't impose any maximum time.

> Replace the per-iteration timeout of 1ms with limiting the total
> transfer time to the timeout set in struct i2c_adapter (defaulting to
> 1s, configurable through the I2C_TIMEOUT ioctl). While we're at it, also
> add a cpu_relax() to the busy poll loop.
> 

...

> @@ -269,17 +269,16 @@ static int ocores_wait(struct ocores_i2c *i2c,
>  		       int reg, u8 mask, u8 val,
>  		       const unsigned long timeout)
>  {
> -	unsigned long j;
> -
> -	j = jiffies + timeout;

Any reason we don't take "jiffies + i2c->adap.timeout" and avoud
all the changes below? It also simplifies the parameters list.

>  	while (1) {
>  		u8 status = oc_getreg(i2c, reg);
>  
>  		if ((status & mask) == val)
>  			break;
>  
> -		if (time_after(jiffies, j))
> +		if (time_after(jiffies, timeout))
>  			return -ETIMEDOUT;
> +
> +		cpu_relax();

Good.

>  	}
>  	return 0;
>  }

...

> -	/*
> -	 * once we are here we expect to get the expected result immediately
> -	 * so if after 1ms we timeout then something is broken.
> -	 */

Why have you deleted this comment completely?

> -	err = ocores_wait(i2c, OCI2C_STATUS, mask, 0, msecs_to_jiffies(1));
> +	err = ocores_wait(i2c, OCI2C_STATUS, mask, 0, timeout);
>  	if (err)
> -		dev_warn(i2c->adap.dev.parent,
> -			 "%s: STATUS timeout, bit 0x%x did not clear in 1ms\n",
> -			 __func__, mask);
> +		dev_dbg(i2c->adap.dev.parent,
> +			"%s: STATUS timeout, bit 0x%x did not clear\n",
> +			__func__, mask);

Why are you changing from warn to dbg? This change is not
mentioned in the commit log.

Andi

>  	return err;
>  }