linux-kernel - RE: [PATCH v2] i2c: rcar: add SMBus block read support

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] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 18 Feb 2022 11:02:45 +0000
From:   "Gabbasov, Andrew" <Andrew_Gabbasov@...tor.com>
To:     Wolfram Sang <wsa+renesas@...g-engineering.com>
CC:     "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Geert Uytterhoeven" <geert+renesas@...der.be>,
        "Surachari, Bhuvanesh" <Bhuvanesh_Surachari@...tor.com>
Subject: RE: [PATCH v2] i2c: rcar: add SMBus block read support

Hi Wolfram!

Thank you for your feedback!
See my responses below.

> -----Original Message-----
> From: Wolfram Sang <wsa+renesas@...g-engineering.com>
> Sent: Thursday, February 17, 2022 10:45 PM
> To: Gabbasov, Andrew <Andrew_Gabbasov@...tor.com>
> Cc: linux-renesas-soc@...r.kernel.org; linux-i2c@...r.kernel.org; linux-kernel@...r.kernel.org; Geert
> Uytterhoeven <geert+renesas@...der.be>; Surachari, Bhuvanesh <Bhuvanesh_Surachari@...tor.com>
> Subject: Re: [PATCH v2] i2c: rcar: add SMBus block read support
> 
[skipped]
> 
> > This patch (adapted) was tested with v4.14, but due to lack of real
> > hardware with SMBus block read operations support, using "simulation",
> > that is manual analysis of data, read from plain I2C devices with
> > SMBus block read request.
> 
> You could wire up two R-Car I2C instances, set up one as an I2C slave
> handled by the I2C testunit and then use the other instance with
> SMBUS_BLOCK_PROC_CALL which also needs RECV_LEN. Check
> Documentation/i2c/slave-testunit-backend.rst for details.

You mean physical connection of two R-Car boards via I2C bus,
or physical connection of I2C bus wires on the single board, right?
It looks like all the boards, that I have access to, do not have
I2C bus wires exposed to some connectors, so both variants would
require hardware re-wiring modification of the boards, which is
not an option for me. Or do I understand you incorrectly and you
mean something different?

> I wonder a bit about the complexity of your patch. In my WIP-branch for
> 256-byte transfers, I have the following patch. It is only missing the
> range check for the received byte, but that it easy to add. Do you see
> anything else missing? If not, I prefer this simpler version because it
> is less intrusive and the state machine is a bit fragile (due to HW
> issues with old HW).

Most of complexity in my patch is related to DMA transfers support,
that I'm trying to retain for SMBus block data transfers too (for the rest
of bytes after the first "length" byte). Your simple patch makes
the driver perform all M_RECV_LEN transfers in PIO mode only (with no DMA at all),
which is probably not quite good (it's a pity to loose existing HW capability,
already supported by the driver).

Also, see a couple of comments below.

> From: Wolfram Sang <wsa+renesas@...g-engineering.com>
> Date: Sun, 2 Aug 2020 00:24:52 +0200
> Subject: [PATCH] i2c: rcar: add support for I2C_M_RECV_LEN
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@...g-engineering.com>
> ---
>  drivers/i2c/busses/i2c-rcar.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index 217def2d7cb4..e473f5c0a708 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -528,6 +528,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))
> @@ -542,11 +543,13 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
>  	} else if (priv->pos < msg->len) {
>  		/* get received data */
>  		msg->buf[priv->pos] = rcar_i2c_read(priv, ICRXTX);
> +		if (recv_len_init)
> +			msg->len += msg->buf[0];
>  		priv->pos++;
>  	}
> 
>  	/* If next received data is the _LAST_, go to new phase. */
> -	if (priv->pos + 1 == msg->len) {
> +	if (priv->pos + 1 == msg->len && !recv_len_init) {

If a message contains a single byte after the length byte,
when we come here after processing the length (in the same function call),
"pos" is 1, "len" is 2, and we indeed are going to process the last byte.
However, "recv_len_init" is still "true", and we skip these corresponding
register writes, which is probably incorrect.
The flag in this case should be re-set back to "false" after length
processing and "pos" moving, but I think the variant in my patch
(leaving this "if" unchanged, but skipping it on the first pass with "goto")
may be even simpler.

>  		if (priv->flags & ID_LAST_MSG) {
>  			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
>  		} else {
> @@ -889,7 +892,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);

This flags setting adds also I2C_FUNC_SMBUS_BLOCK_PROC_CALL flag,
which is missed in my patch. My patch should probably be updated
to include it too (if you'll agree to take my variant ;-) ).

> 
>  	if (priv->flags & ID_P_HOST_NOTIFY)
>  		func |= I2C_FUNC_SMBUS_HOST_NOTIFY;
> 
> Happy hacking,
> 
>    Wolfram

Thanks!

Best regards,
Andrew