[PATCH v2 0/3] i2c: recovery: make sure pulses are not misinterpreted

[PATCH v2 0/3] i2c: recovery: make sure pulses are not misinterpreted

[Wolfram Sang]

As the 'incomplete_write_byte' fault injector has shown, we need to take care
that pulses used for recovery will not be misinterpreted as a regular cycle.
This series implements that. This approach makes sure that STOP signals put on
the bus do not create additional pulses. We still do 9 pulses only, but
generate a STOP signal in case SDA was released again.

The first patch implements it in a minimal way, so it is suitable for stable.
The second patch adds some sanity checking and documentation on top which is
not suitable for stable. The third patch simplifies the logic because of the
new behaviour.

It has been tested with a Renesas Lager board (R-Car H2). I will test it
tomorrow a bit more once I taught the GPIO driver bus recovery.

A branch (with further improvements) can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/recovery/write-byte-fix

Looking forward to comments.

Kind regards,

   Wolfram

Changes since V1:

* we create STOP now always when set_sda is available independent of get_sda
  being available or not. This is much safer and allows for a cleaner logic
  implemented in the new patch 3.
* hopefully improved comments

Wolfram Sang (3):
  i2c: recovery: if possible send STOP with recovery pulses
  i2c: recovery: require either get_sda or set_sda
  i2c: recovery: refactor recovery function

 drivers/i2c/i2c-core-base.c | 36 +++++++++++++++++++-----------------
 include/linux/i2c.h         | 12 ++++++------
 2 files changed, 25 insertions(+), 23 deletions(-)

-- 
2.11.0

[PATCH v2 1/3] i2c: recovery: if possible send STOP with recovery pulses

[Wolfram Sang]

I2C clients may misunderstand recovery pulses if they can't read SDA to
bail out early. In the worst case, as a write operation. To avoid that
and if we can write SDA, try to send STOP to avoid the
misinterpretation.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 31d16ada6e7d..301285c54603 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -198,7 +198,16 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
 
 		val = !val;
 		bri->set_scl(adap, val);
-		ndelay(RECOVERY_NDELAY);
+
+		/*
+		 * If we can set SDA, we will always create STOP here to ensure
+		 * the additional pulses will do no harm. This is achieved by
+		 * letting SDA follow SCL half a cycle later.
+		 */
+		ndelay(RECOVERY_NDELAY / 2);
+		if (bri->set_sda)
+			bri->set_sda(adap, val);
+		ndelay(RECOVERY_NDELAY / 2);
 	}
 
 	/* check if recovery actually succeeded */
-- 
2.11.0

[PATCH v2 2/3] i2c: recovery: require either get_sda or set_sda

[Wolfram Sang]

For bus recovery, we either need to bail out early if we can read SDA or
we need to send STOP after every pulse. Otherwise recovery might be
misinterpreted as an unwanted write. So, require one of those SDA
handling functions to avoid this problem.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c |  7 ++++++-
 include/linux/i2c.h         | 12 ++++++------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 301285c54603..871a9731894f 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -202,7 +202,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
 		/*
 		 * If we can set SDA, we will always create STOP here to ensure
 		 * the additional pulses will do no harm. This is achieved by
-		 * letting SDA follow SCL half a cycle later.
+		 * letting SDA follow SCL half a cycle later. Check the
+		 * 'incomplete_write_byte' fault injector for details.
 		 */
 		ndelay(RECOVERY_NDELAY / 2);
 		if (bri->set_sda)
@@ -274,6 +275,10 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
 			err_str = "no {get|set}_scl() found";
 			goto err;
 		}
+		if (!bri->set_sda && !bri->get_sda) {
+			err_str = "either get_sda() or set_sda() needed";
+			goto err;
+		}
 	}
 
 	return;
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 465afb092fa7..9d1818c56775 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -581,12 +581,12 @@ struct i2c_timings {
  *      recovery. Populated internally for generic GPIO recovery.
  * @set_scl: This sets/clears the SCL line. Mandatory for generic SCL recovery.
  *      Populated internally for generic GPIO recovery.
- * @get_sda: This gets current value of SDA line. Optional for generic SCL
- *      recovery. Populated internally, if sda_gpio is a valid GPIO, for generic
- *      GPIO recovery.
- * @set_sda: This sets/clears the SDA line. Optional for generic SCL recovery.
- *	Populated internally, if sda_gpio is a valid GPIO, for generic GPIO
- *	recovery.
+ * @get_sda: This gets current value of SDA line. This or set_sda() is mandatory
+ *	for generic SCL recovery. Populated internally, if sda_gpio is a valid
+ *	GPIO, for generic GPIO recovery.
+ * @set_sda: This sets/clears the SDA line. This or get_sda() is mandatory for
+ *	generic SCL recovery. Populated internally, if sda_gpio is a valid GPIO,
+ *	for generic GPIO recovery.
  * @prepare_recovery: This will be called before starting recovery. Platform may
  *	configure padmux here for SDA/SCL line or something else they want.
  * @unprepare_recovery: This will be called after completing recovery. Platform
-- 
2.11.0

[PATCH v2 3/3] i2c: recovery: refactor recovery function

[Wolfram Sang]

After exiting the while loop, we checked if recovery was successful and
sent a STOP to the clients. Meanwhile however, we send a STOP after
every pulse, so it is not needed after the loop. If we move the check
for a free bus to the end of the while loop, we can shorten and simplify
the logic. It is still ensured that at least one STOP will be sent to
the wire even if SDA was not stuck low.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 871a9731894f..c7995efd58ea 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -191,9 +191,6 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
 				ret = -EBUSY;
 				break;
 			}
-			/* Break if SDA is high */
-			if (bri->get_sda && bri->get_sda(adap))
-				break;
 		}
 
 		val = !val;
@@ -209,22 +206,13 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
 		if (bri->set_sda)
 			bri->set_sda(adap, val);
 		ndelay(RECOVERY_NDELAY / 2);
-	}
-
-	/* check if recovery actually succeeded */
-	if (bri->get_sda && !bri->get_sda(adap))
-		ret = -EBUSY;
 
-	/* If all went well, send STOP for a sane bus state. */
-	if (ret == 0 && bri->set_sda) {
-		bri->set_scl(adap, 0);
-		ndelay(RECOVERY_NDELAY / 2);
-		bri->set_sda(adap, 0);
-		ndelay(RECOVERY_NDELAY / 2);
-		bri->set_scl(adap, 1);
-		ndelay(RECOVERY_NDELAY / 2);
-		bri->set_sda(adap, 1);
-		ndelay(RECOVERY_NDELAY / 2);
+		/* Break if SDA is high */
+		if (val && bri->get_sda) {
+			ret = bri->get_sda(adap) ? 0 : -EBUSY;
+			if (ret == 0)
+				break;
+		}
 	}
 
 	if (bri->unprepare_recovery)
-- 
2.11.0

Re: [PATCH v2 1/3] i2c: recovery: if possible send STOP with recovery pulses

[Peter Rosin]

On 2018-07-10 23:42, Wolfram Sang wrote:
> I2C clients may misunderstand recovery pulses if they can't read SDA to
> bail out early. In the worst case, as a write operation. To avoid that
> and if we can write SDA, try to send STOP to avoid the
> misinterpretation.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 31d16ada6e7d..301285c54603 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -198,7 +198,16 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>  
>  		val = !val;
>  		bri->set_scl(adap, val);
> -		ndelay(RECOVERY_NDELAY);
> +
> +		/*
> +		 * If we can set SDA, we will always create STOP here to ensure
> +		 * the additional pulses will do no harm. This is achieved by
> +		 * letting SDA follow SCL half a cycle later.
> +		 */
> +		ndelay(RECOVERY_NDELAY / 2);
> +		if (bri->set_sda)
> +			bri->set_sda(adap, val);
> +		ndelay(RECOVERY_NDELAY / 2);
>  	}
>  
>  	/* check if recovery actually succeeded */
> 
Hmmm, should not the ndelay before the loop also be split up in two like
this, with one ndelay(... / 2) on either side of the (possible) set_sda.
Not that it should matter, since SDA is presumably stuck low. But what if
it isn't?

I would also change the while (...) to

	for (i = 0; i < RECOVERY_CLK_CNT * 2; i++)

but both these "issues" are perhaps not related to this patch...

Either way,

Reviewed-by: Peter Rosin <peda@axentia.se>

Cheers,
Peter

Re: [PATCH v2 2/3] i2c: recovery: require either get_sda or set_sda

[Peter Rosin]

On 2018-07-10 23:42, Wolfram Sang wrote:
> For bus recovery, we either need to bail out early if we can read SDA or
> we need to send STOP after every pulse. Otherwise recovery might be
> misinterpreted as an unwanted write. So, require one of those SDA
> handling functions to avoid this problem.

Assuming that all users fulfill the stricter requirement...

Acked-by: Peter Rosin <peda@axentia.se>

> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c |  7 ++++++-
>  include/linux/i2c.h         | 12 ++++++------
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 301285c54603..871a9731894f 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -202,7 +202,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>  		/*
>  		 * If we can set SDA, we will always create STOP here to ensure
>  		 * the additional pulses will do no harm. This is achieved by
> -		 * letting SDA follow SCL half a cycle later.
> +		 * letting SDA follow SCL half a cycle later. Check the
> +		 * 'incomplete_write_byte' fault injector for details.
>  		 */
>  		ndelay(RECOVERY_NDELAY / 2);
>  		if (bri->set_sda)
> @@ -274,6 +275,10 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
>  			err_str = "no {get|set}_scl() found";
>  			goto err;
>  		}
> +		if (!bri->set_sda && !bri->get_sda) {
> +			err_str = "either get_sda() or set_sda() needed";
> +			goto err;
> +		}
>  	}
>  
>  	return;
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 465afb092fa7..9d1818c56775 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -581,12 +581,12 @@ struct i2c_timings {
>   *      recovery. Populated internally for generic GPIO recovery.
>   * @set_scl: This sets/clears the SCL line. Mandatory for generic SCL recovery.
>   *      Populated internally for generic GPIO recovery.
> - * @get_sda: This gets current value of SDA line. Optional for generic SCL
> - *      recovery. Populated internally, if sda_gpio is a valid GPIO, for generic
> - *      GPIO recovery.
> - * @set_sda: This sets/clears the SDA line. Optional for generic SCL recovery.
> - *	Populated internally, if sda_gpio is a valid GPIO, for generic GPIO
> - *	recovery.
> + * @get_sda: This gets current value of SDA line. This or set_sda() is mandatory
> + *	for generic SCL recovery. Populated internally, if sda_gpio is a valid
> + *	GPIO, for generic GPIO recovery.
> + * @set_sda: This sets/clears the SDA line. This or get_sda() is mandatory for
> + *	generic SCL recovery. Populated internally, if sda_gpio is a valid GPIO,
> + *	for generic GPIO recovery.
>   * @prepare_recovery: This will be called before starting recovery. Platform may
>   *	configure padmux here for SDA/SCL line or something else they want.
>   * @unprepare_recovery: This will be called after completing recovery. Platform
> 

Re: [PATCH v2 3/3] i2c: recovery: refactor recovery function

[Peter Rosin]

On 2018-07-10 23:42, Wolfram Sang wrote:
> After exiting the while loop, we checked if recovery was successful and
> sent a STOP to the clients. Meanwhile however, we send a STOP after
> every pulse, so it is not needed after the loop. If we move the check
> for a free bus to the end of the while loop, we can shorten and simplify
> the logic. It is still ensured that at least one STOP will be sent to
> the wire even if SDA was not stuck low.

Well, there will be no STOP if ->set_sda isn't implemented, but that case
is also handled equivalently after this patch AFAICT, so

Reviewed-by: Peter Rosin <peda@axentia.se>

> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c | 24 ++++++------------------
>  1 file changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 871a9731894f..c7995efd58ea 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -191,9 +191,6 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>  				ret = -EBUSY;
>  				break;
>  			}
> -			/* Break if SDA is high */
> -			if (bri->get_sda && bri->get_sda(adap))
> -				break;
>  		}
>  
>  		val = !val;
> @@ -209,22 +206,13 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>  		if (bri->set_sda)
>  			bri->set_sda(adap, val);
>  		ndelay(RECOVERY_NDELAY / 2);
> -	}
> -
> -	/* check if recovery actually succeeded */
> -	if (bri->get_sda && !bri->get_sda(adap))
> -		ret = -EBUSY;
>  
> -	/* If all went well, send STOP for a sane bus state. */
> -	if (ret == 0 && bri->set_sda) {
> -		bri->set_scl(adap, 0);
> -		ndelay(RECOVERY_NDELAY / 2);
> -		bri->set_sda(adap, 0);
> -		ndelay(RECOVERY_NDELAY / 2);
> -		bri->set_scl(adap, 1);
> -		ndelay(RECOVERY_NDELAY / 2);
> -		bri->set_sda(adap, 1);
> -		ndelay(RECOVERY_NDELAY / 2);
> +		/* Break if SDA is high */
> +		if (val && bri->get_sda) {
> +			ret = bri->get_sda(adap) ? 0 : -EBUSY;
> +			if (ret == 0)
> +				break;
> +		}
>  	}
>  
>  	if (bri->unprepare_recovery)
> 

Re: [PATCH v2 1/3] i2c: recovery: if possible send STOP with recovery pulses

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 640 bytes --]

Hi Peter,

> Hmmm, should not the ndelay before the loop also be split up in two like
> this, with one ndelay(... / 2) on either side of the (possible) set_sda.
> Not that it should matter, since SDA is presumably stuck low. But what if
> it isn't?

I agree it would be better.

> I would also change the while (...) to
> 
> 	for (i = 0; i < RECOVERY_CLK_CNT * 2; i++)

Yeah, could be done as well.

> but both these "issues" are perhaps not related to this patch...

I also think these should be handled incrementally.

> Reviewed-by: Peter Rosin <peda@axentia.se>

Thanks for the review!

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/3] i2c: recovery: require either get_sda or set_sda

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 490 bytes --]

On Wed, Jul 11, 2018 at 07:51:02AM +0200, Peter Rosin wrote:
> On 2018-07-10 23:42, Wolfram Sang wrote:
> > For bus recovery, we either need to bail out early if we can read SDA or
> > we need to send STOP after every pulse. Otherwise recovery might be
> > misinterpreted as an unwanted write. So, require one of those SDA
> > handling functions to avoid this problem.
> 
> Assuming that all users fulfill the stricter requirement...

Yes, I checked all in-tree users for that.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/3] i2c: recovery: if possible send STOP with recovery pulses

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 399 bytes --]

On Tue, Jul 10, 2018 at 11:42:15PM +0200, Wolfram Sang wrote:
> I2C clients may misunderstand recovery pulses if they can't read SDA to
> bail out early. In the worst case, as a write operation. To avoid that
> and if we can write SDA, try to send STOP to avoid the
> misinterpretation.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-current, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/3] i2c: recovery: require either get_sda or set_sda

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 428 bytes --]

On Tue, Jul 10, 2018 at 11:42:16PM +0200, Wolfram Sang wrote:
> For bus recovery, we either need to bail out early if we can read SDA or
> we need to send STOP after every pulse. Otherwise recovery might be
> misinterpreted as an unwanted write. So, require one of those SDA
> handling functions to avoid this problem.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/3] i2c: recovery: refactor recovery function

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 578 bytes --]

On Tue, Jul 10, 2018 at 11:42:17PM +0200, Wolfram Sang wrote:
> After exiting the while loop, we checked if recovery was successful and
> sent a STOP to the clients. Meanwhile however, we send a STOP after
> every pulse, so it is not needed after the loop. If we move the check
> for a free bus to the end of the while loop, we can shorten and simplify
> the logic. It is still ensured that at least one STOP will be sent to
> the wire even if SDA was not stuck low.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]