[PATCH v4] i2c: rcar: add support for I2C_M_RECV_LEN

[PATCH v4] i2c: rcar: add support for I2C_M_RECV_LEN

[Wolfram Sang]

With this feature added, SMBus Block reads and Proc calls are now
supported. This patch is the best of two independent developments by
Wolfram and Bhuvanesh + Andrew, refactored again by Wolfram.

Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com>
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

For testing, I wired a Lager board (R-Car H2) and a Salvator-XS (R-Car
H3 ES2.0) together. The Lager board ran the testunit and provided SMBus
Proc Calls. The Salvator-XS board was requesting the data.

Compared to my previous version: sending 1 byte works now, sending with
DMA as well. Invalid sizes are detected, too. This is as much as I can
test, I'd think.

Compared to Bhuvanesh + Andrew's last version: less intrusive and more
self contained (no goto), Proc Calls are covered as well

I tried some other refactoring as well (like one single place where
rcar_i2c_dma() is called) but IMHO this is the most readable solution.

Thank you everyone for working on this. I am very interested in your
comments and test results!

 drivers/i2c/busses/i2c-rcar.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index f71c730f9838..f45991252993 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -105,6 +105,7 @@
 #define ID_DONE		(1 << 2)
 #define ID_ARBLOST	(1 << 3)
 #define ID_NACK		(1 << 4)
+#define ID_EPROTO	(1 << 5)
 /* persistent flags */
 #define ID_P_HOST_NOTIFY	BIT(28)
 #define ID_P_REP_AFTER_RD	BIT(29)
@@ -522,6 +523,7 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
 static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 {
 	struct i2c_msg *msg = priv->msg;
+	bool recv_len_init = priv->pos == 0 && msg->flags & I2C_M_RECV_LEN;
 
 	/* FIXME: sometimes, unknown interrupt happened. Do nothing */
 	if (!(msr & MDR))
@@ -535,12 +537,29 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 		rcar_i2c_dma(priv);
 	} else if (priv->pos < msg->len) {
 		/* get received data */
-		msg->buf[priv->pos] = rcar_i2c_read(priv, ICRXTX);
+		u8 data = rcar_i2c_read(priv, ICRXTX);
+
+		msg->buf[priv->pos] = data;
+		if (recv_len_init) {
+			if (data == 0 || data > I2C_SMBUS_BLOCK_MAX) {
+				priv->flags |= ID_DONE | ID_EPROTO;
+				return;
+			}
+			msg->len += msg->buf[0];
+			/* Enough data for DMA? */
+			if (rcar_i2c_dma(priv))
+				return;
+			/* new length after RECV_LEN now properly initialized */
+			recv_len_init = false;
+		}
 		priv->pos++;
 	}
 
-	/* If next received data is the _LAST_, go to new phase. */
-	if (priv->pos + 1 == msg->len) {
+	/*
+	 * If next received data is the _LAST_ and we are not waiting for a new
+	 * length because of RECV_LEN, then go to a new phase.
+	 */
+	if (priv->pos + 1 == msg->len && !recv_len_init) {
 		if (priv->flags & ID_LAST_MSG) {
 			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
 		} else {
@@ -847,6 +866,8 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 		ret = -ENXIO;
 	} else if (priv->flags & ID_ARBLOST) {
 		ret = -EAGAIN;
+	} else if (priv->flags & ID_EPROTO) {
+		ret = -EPROTO;
 	} else {
 		ret = num - priv->msgs_left; /* The number of transfer */
 	}
@@ -909,6 +930,8 @@ static int rcar_i2c_master_xfer_atomic(struct i2c_adapter *adap,
 		ret = -ENXIO;
 	} else if (priv->flags & ID_ARBLOST) {
 		ret = -EAGAIN;
+	} else if (priv->flags & ID_EPROTO) {
+		ret = -EPROTO;
 	} else {
 		ret = num - priv->msgs_left; /* The number of transfer */
 	}
@@ -975,7 +998,7 @@ static u32 rcar_i2c_func(struct i2c_adapter *adap)
 	 * I2C_M_IGNORE_NAK (automatically sends STOP after NAK)
 	 */
 	u32 func = I2C_FUNC_I2C | I2C_FUNC_SLAVE |
-		   (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
+		   (I2C_FUNC_SMBUS_EMUL_ALL & ~I2C_FUNC_SMBUS_QUICK);
 
 	if (priv->flags & ID_P_HOST_NOTIFY)
 		func |= I2C_FUNC_SMBUS_HOST_NOTIFY;
-- 
2.30.2

Re: [PATCH v4] i2c: rcar: add support for I2C_M_RECV_LEN

[Eugeniu Rosca]

Hello Wolfram,

On Di, Apr 05, 2022 at 12:07:56 +0200, Wolfram Sang wrote:
> With this feature added, SMBus Block reads and Proc calls are now
> supported. This patch is the best of two independent developments by
> Wolfram and Bhuvanesh + Andrew, refactored again by Wolfram.
> 
> Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com>
> Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> For testing, I wired a Lager board (R-Car H2) and a Salvator-XS (R-Car
> H3 ES2.0) together. The Lager board ran the testunit and provided SMBus
> Proc Calls. The Salvator-XS board was requesting the data.
> 
> Compared to my previous version: sending 1 byte works now, sending with
> DMA as well. Invalid sizes are detected, too. This is as much as I can
> test, I'd think.
> 
> Compared to Bhuvanesh + Andrew's last version: less intrusive and more
> self contained (no goto), Proc Calls are covered as well
> 
> I tried some other refactoring as well (like one single place where
> rcar_i2c_dma() is called) but IMHO this is the most readable solution.
> 
> Thank you everyone for working on this. I am very interested in your
> comments and test results!
> 
>  drivers/i2c/busses/i2c-rcar.c | 31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)

I am not an i2c/SMBus expert, but I genuinely tried to attack the patch
from multiple angles, including static analysis (smatch, cppcheck, sparse,
PVS Studio, coccicheck, make W=123), code review, dynamic testing
(KASAN, UBSAN and friends enabled) and couldn't spot any misbehavior or
any obvious opportunities for optimization.

We've tested this patch on vanilla and on 4.14 using reference and
non-reference boards and the behavior matched our expectations.

I've also briefly glanced at the i2c fault injection possibilities, as
described in https://elinux.org/Tests:I2C-fault-injection, but soon
realized this will require HW/board modifications, which are not
straightforward in my personal environment.

Looking forward to any other review comments.
Thank you for your friendly support and cooperation.

Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>

Best regards,
Eugeniu

Re: [PATCH v4] i2c: rcar: add support for I2C_M_RECV_LEN

[Wolfram Sang]

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

Hi Eugeniu,

> Thank you for your friendly support and cooperation.

Same to you, thank you for the extensive testing! Much appreciated.

All the best,

   Wolfram


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

Re: [PATCH v4] i2c: rcar: add support for I2C_M_RECV_LEN

[Gabbasov, Andrew]

Hello Wolfram,

Sorry for late response, I had some problems accessing my e-mail.

Thank you for your efforts on moving forward on this topic.
Indeed, this version of the patch combines all features/suggestions,
being discussed earlier, and looks quite compact and clear.

If you don't mind, I would propose one small "polishing" modification
(re-ordering of statements), that doesn't affect the functionality, see below.

________________________________________
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Sent: Tuesday, April 5, 2022 13:07
> To: linux-i2c@vger.kernel.org
> Cc: linux-renesas-soc@vger.kernel.org; Eugeniu Rosca; Wolfram Sang; Surachari, Bhuvanesh; Gabbasov, Andrew
> Subject: [PATCH v4] i2c: rcar: add support for I2C_M_RECV_LEN
>
> [skipped]
>
> @@ -535,12 +537,29 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
>                 rcar_i2c_dma(priv);
>         } else if (priv->pos < msg->len) {
>                 /* get received data */
> -               msg->buf[priv->pos] = rcar_i2c_read(priv, ICRXTX);
> +               u8 data = rcar_i2c_read(priv, ICRXTX);
> +
> +               msg->buf[priv->pos] = data;
> +               if (recv_len_init) {
> +                       if (data == 0 || data > I2C_SMBUS_BLOCK_MAX) {
> +                               priv->flags |= ID_DONE | ID_EPROTO;
> +                               return;
> +                       }
> +                       msg->len += msg->buf[0];
> +                       /* Enough data for DMA? */
> +                       if (rcar_i2c_dma(priv))
> +                               return;
> +                       /* new length after RECV_LEN now properly initialized */
> +                       recv_len_init = false;
> +               }

It looks like rcar_i2c_dma() starts DMA transfer at once, including to the transfer
the byte, currently residing in ICRXD register. (Because ICMSR.MDR bit is set.
That was the issue in my previous v2 patch: it copied the "current" byte to the
"next" buffer position too. To fix that, it would probably be necessary to clear
that bit before starting DMA, so that it starts with the next/pending bytes).

It means that this very first "length" byte will be copied to the buffer
by DMA transfer too (in addition to explicit assignment above).

So, I would propose to move the statement, placing the data byte to the buffer
("msg->buf[priv->pos] = data;"), to this place, after the "if (recv_len_init)" clause,
just before incrementing priv->pos.
Accordingly, adjustment of msg->len inside that "if" should be done using "data"
instead of "msg->buf[0]" ("msg->len += data;").

Besides avoiding of double assignment of that "length" byte to the buffer,
this move will avoid pollution of the buffer in case of an error (invalid length).
And will place filling in the data and position incrementation closer to each
other, that looks more "natural".

Again, this is just a "stylish" change, the patch looks good without it too ;-)

Thanks!

Best regards,
Andrew

>                 priv->pos++;
>         }
>
> -       /* If next received data is the _LAST_, go to new phase. */
> -       if (priv->pos + 1 == msg->len) {
> +       /*
> +        * If next received data is the _LAST_ and we are not waiting for a new
> +        * length because of RECV_LEN, then go to a new phase.
> +        */
> +       if (priv->pos + 1 == msg->len && !recv_len_init) {
>                 if (priv->flags & ID_LAST_MSG) {
>                         rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
>                 } else {
> @@ -847,6 +866,8 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
>                 ret = -ENXIO;
>         } else if (priv->flags & ID_ARBLOST) {
>                 ret = -EAGAIN;
> +       } else if (priv->flags & ID_EPROTO) {
> +               ret = -EPROTO;
>         } else {
>                 ret = num - priv->msgs_left; /* The number of transfer */
>         }
> @@ -909,6 +930,8 @@ static int rcar_i2c_master_xfer_atomic(struct i2c_adapter *adap,
>                 ret = -ENXIO;
>         } else if (priv->flags & ID_ARBLOST) {
>                 ret = -EAGAIN;
> +       } else if (priv->flags & ID_EPROTO) {
> +               ret = -EPROTO;
>         } else {
>                 ret = num - priv->msgs_left; /* The number of transfer */
>         }
> @@ -975,7 +998,7 @@ static u32 rcar_i2c_func(struct i2c_adapter *adap)
>          * I2C_M_IGNORE_NAK (automatically sends STOP after NAK)
>          */
>         u32 func = I2C_FUNC_I2C | I2C_FUNC_SLAVE |
> -                  (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> +                  (I2C_FUNC_SMBUS_EMUL_ALL & ~I2C_FUNC_SMBUS_QUICK);
>
>         if (priv->flags & ID_P_HOST_NOTIFY)
>                 func |= I2C_FUNC_SMBUS_HOST_NOTIFY;
> --
> 2.30.2
>

Re: [PATCH v4] i2c: rcar: add support for I2C_M_RECV_LEN

[Wolfram Sang]

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


> Besides avoiding of double assignment of that "length" byte to the buffer,
> this move will avoid pollution of the buffer in case of an error (invalid length).

This was intendend as a feature not a bug ;) This was the reason I put
the data to the buffer at the beginning, so people could see the wrong
data length. But yes, it can be argued if it should be logged somewhere
instead of being in the buffer. I'll check what other drivers do.

Sidenote: It is planned to add SMBus3 features somewhen. Then, there
won't be an invalid length anymore because it allows 255 byte transfers.


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

Re: [PATCH v4] i2c: rcar: add support for I2C_M_RECV_LEN

[Gabbasov, Andrew]

Hi Wolfram,

> > Besides avoiding of double assignment of that "length" byte to the buffer,
> > this move will avoid pollution of the buffer in case of an error (invalid length).
> 
> This was intendend as a feature not a bug ;) This was the reason I put
> the data to the buffer at the beginning, so people could see the wrong
> data length. But yes, it can be argued if it should be logged somewhere
> instead of being in the buffer. I'll check what other drivers do.
> 
> Sidenote: It is planned to add SMBus3 features somewhen. Then, there
> won't be an invalid length anymore because it allows 255 byte transfers.

Fair enough. That makes sense.
I withdraw my proposal then ;-) Sorry for the noise.

Thanks.

Best regards,
Andrew

Re: [PATCH v4] i2c: rcar: add support for I2C_M_RECV_LEN

[Wolfram Sang]

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


> I withdraw my proposal then ;-) Sorry for the noise.

Not noise, this was a valid discussion :)


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

Re: [PATCH v4] i2c: rcar: add support for I2C_M_RECV_LEN

[Wolfram Sang]

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

On Tue, Apr 05, 2022 at 12:07:56PM +0200, Wolfram Sang wrote:
> With this feature added, SMBus Block reads and Proc calls are now
> supported. This patch is the best of two independent developments by
> Wolfram and Bhuvanesh + Andrew, refactored again by Wolfram.
> 
> Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com>
> Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> 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 --]