| Message ID | 20220405100756.42920-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [v4] i2c: rcar: add support for I2C_M_RECV_LEN | expand |
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
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
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.
> 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.
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
> I withdraw my proposal then ;-) Sorry for the noise.
Not noise, this was a valid discussion :)
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!
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;