[PATCH RFC 0/3] treewide: remove open coded SMBus block transfers

[PATCH RFC 0/3] treewide: remove open coded SMBus block transfers

[Wolfram Sang]

The bigger picture is that I want to extend the maximum block length for
SMBus block transfers from 32 (SMBus2) to 255 (SMBus3). That needs some
cleanups and refactoring first. To make that easier, it would be helpful
if all in-kernel users would call the helper functions of the I2C core
for SMBus block transfers and not open code it via the generic
smbus_xfer.

This series converts the three users doing that. I don't have the
hardware, so these patches are only build tested. Please let me know
what you think.


Wolfram Sang (3):
  media: i2c: adv7842: remove open coded version of SMBus block write
  media: i2c: adv7842: remove open coded version of SMBus block read
  ipmi: remove open coded version of SMBus block write

 drivers/char/ipmi/ipmb_dev_int.c | 21 +++++++----------
 drivers/media/i2c/adv7511-v4l2.c | 40 +++++++++++---------------------
 drivers/media/i2c/adv7842.c      | 14 +----------
 3 files changed, 23 insertions(+), 52 deletions(-)

-- 
2.29.2

[PATCH RFC 1/3] media: i2c: adv7842: remove open coded version of SMBus block write

[Wolfram Sang]

The version here is identical to the one in the I2C core, so use a
define to keep the original name within the driver but call the I2C core
function instead.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/media/i2c/adv7842.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/media/i2c/adv7842.c b/drivers/media/i2c/adv7842.c
index 0855f648416d..6ed6bcd1d64d 100644
--- a/drivers/media/i2c/adv7842.c
+++ b/drivers/media/i2c/adv7842.c
@@ -343,19 +343,7 @@ static void adv_smbus_write_byte_no_check(struct i2c_client *client,
 		       I2C_SMBUS_BYTE_DATA, &data);
 }
 
-static s32 adv_smbus_write_i2c_block_data(struct i2c_client *client,
-				  u8 command, unsigned length, const u8 *values)
-{
-	union i2c_smbus_data data;
-
-	if (length > I2C_SMBUS_BLOCK_MAX)
-		length = I2C_SMBUS_BLOCK_MAX;
-	data.block[0] = length;
-	memcpy(data.block + 1, values, length);
-	return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-			      I2C_SMBUS_WRITE, command,
-			      I2C_SMBUS_I2C_BLOCK_DATA, &data);
-}
+#define adv_smbus_write_i2c_block_data i2c_smbus_write_i2c_block_data
 
 /* ----------------------------------------------------------------------- */
 
-- 
2.29.2

[PATCH RFC 2/3] media: i2c: adv7842: remove open coded version of SMBus block read

[Wolfram Sang]

The open coded version differs from the one in the core in one way: the
buffer will be always copied back, even when the transfer failed. It
looks like it is expected that the sanity check for a correct CRC and
header will bail out later.

Use the block read from the I2C core and propagate a potential errno
further to the sanity check.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Note: we could now make the error checking even stronger by checking if
the number of received bytes is I2C_SMBUS_BLOCK_MAX. But to avoid
regressions, I kept the logic as is, i.e. only check for errno.

 drivers/media/i2c/adv7511-v4l2.c | 40 +++++++++++---------------------
 1 file changed, 14 insertions(+), 26 deletions(-)

diff --git a/drivers/media/i2c/adv7511-v4l2.c b/drivers/media/i2c/adv7511-v4l2.c
index a3161d709015..0150f76dc6a6 100644
--- a/drivers/media/i2c/adv7511-v4l2.c
+++ b/drivers/media/i2c/adv7511-v4l2.c
@@ -214,36 +214,24 @@ static inline void adv7511_wr_and_or(struct v4l2_subdev *sd, u8 reg, u8 clr_mask
 	adv7511_wr(sd, reg, (adv7511_rd(sd, reg) & clr_mask) | val_mask);
 }
 
-static int adv_smbus_read_i2c_block_data(struct i2c_client *client,
-					 u8 command, unsigned length, u8 *values)
-{
-	union i2c_smbus_data data;
-	int ret;
-
-	if (length > I2C_SMBUS_BLOCK_MAX)
-		length = I2C_SMBUS_BLOCK_MAX;
-	data.block[0] = length;
-
-	ret = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-			     I2C_SMBUS_READ, command,
-			     I2C_SMBUS_I2C_BLOCK_DATA, &data);
-	memcpy(values, data.block + 1, length);
-	return ret;
-}
-
-static void adv7511_edid_rd(struct v4l2_subdev *sd, uint16_t len, uint8_t *buf)
+static int adv7511_edid_rd(struct v4l2_subdev *sd, uint16_t len, uint8_t *buf)
 {
 	struct adv7511_state *state = get_adv7511_state(sd);
+	s32 len;
 	int i;
-	int err = 0;
 
 	v4l2_dbg(1, debug, sd, "%s:\n", __func__);
 
-	for (i = 0; !err && i < len; i += I2C_SMBUS_BLOCK_MAX)
-		err = adv_smbus_read_i2c_block_data(state->i2c_edid, i,
+	for (i = 0; i < len; i += I2C_SMBUS_BLOCK_MAX) {
+		len = i2c_smbus_read_i2c_block_data(state->i2c_edid, i,
 						    I2C_SMBUS_BLOCK_MAX, buf + i);
-	if (err)
-		v4l2_err(sd, "%s: i2c read error\n", __func__);
+		if (len < 0) {
+			v4l2_err(sd, "%s: i2c read error\n", __func__);
+			return len;
+		}
+	}
+
+	return 0;
 }
 
 static inline int adv7511_cec_read(struct v4l2_subdev *sd, u8 reg)
@@ -1668,20 +1656,20 @@ static bool adv7511_check_edid_status(struct v4l2_subdev *sd)
 	if (edidRdy & MASK_ADV7511_EDID_RDY) {
 		int segment = adv7511_rd(sd, 0xc4);
 		struct adv7511_edid_detect ed;
+		int err;
 
 		if (segment >= EDID_MAX_SEGM) {
 			v4l2_err(sd, "edid segment number too big\n");
 			return false;
 		}
 		v4l2_dbg(1, debug, sd, "%s: got segment %d\n", __func__, segment);
-		adv7511_edid_rd(sd, 256, &state->edid.data[segment * 256]);
+		err = adv7511_edid_rd(sd, 256, &state->edid.data[segment * 256]);
 		adv7511_dbg_dump_edid(2, debug, sd, segment, &state->edid.data[segment * 256]);
 		if (segment == 0) {
 			state->edid.blocks = state->edid.data[0x7e] + 1;
 			v4l2_dbg(1, debug, sd, "%s: %d blocks in total\n", __func__, state->edid.blocks);
 		}
-		if (!edid_verify_crc(sd, segment) ||
-		    !edid_verify_header(sd, segment)) {
+		if (err < 0 || !edid_verify_crc(sd, segment) || !edid_verify_header(sd, segment)) {
 			/* edid crc error, force reread of edid segment */
 			v4l2_err(sd, "%s: edid crc or header error\n", __func__);
 			state->have_monitor = false;
-- 
2.29.2

[PATCH RFC 3/3] ipmi: remove open coded version of SMBus block write

[Wolfram Sang]

The block-write function of the core was not used because there was no
client-struct to use. However, in this case it seems apropriate to use a
temporary client struct. Because we are answering a request we recieved
when being a client ourselves. So, convert the code to use a temporary
client and use the block-write function of the I2C core.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/char/ipmi/ipmb_dev_int.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c
index 382b28f1cf2f..10d89886e5f3 100644
--- a/drivers/char/ipmi/ipmb_dev_int.c
+++ b/drivers/char/ipmi/ipmb_dev_int.c
@@ -137,7 +137,7 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
 {
 	struct ipmb_dev *ipmb_dev = to_ipmb_dev(file);
 	u8 rq_sa, netf_rq_lun, msg_len;
-	union i2c_smbus_data data;
+	struct i2c_client temp_client;
 	u8 msg[MAX_MSG_LEN];
 	ssize_t ret;
 
@@ -160,21 +160,16 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
 	}
 
 	/*
-	 * subtract rq_sa and netf_rq_lun from the length of the msg passed to
-	 * i2c_smbus_xfer
+	 * subtract rq_sa and netf_rq_lun from the length of the msg. Fill the
+	 * temporary client. Note that its use is an exception for IPMI.
 	 */
 	msg_len = msg[IPMB_MSG_LEN_IDX] - SMBUS_MSG_HEADER_LENGTH;
-	if (msg_len > I2C_SMBUS_BLOCK_MAX)
-		msg_len = I2C_SMBUS_BLOCK_MAX;
+	memcpy(&temp_client, ipmb_dev->client, sizeof(temp_client));
+	temp_client.addr = rq_sa;
 
-	data.block[0] = msg_len;
-	memcpy(&data.block[1], msg + SMBUS_MSG_IDX_OFFSET, msg_len);
-	ret = i2c_smbus_xfer(ipmb_dev->client->adapter, rq_sa,
-			     ipmb_dev->client->flags,
-			     I2C_SMBUS_WRITE, netf_rq_lun,
-			     I2C_SMBUS_BLOCK_DATA, &data);
-
-	return ret ? : count;
+	ret = i2c_smbus_write_block_data(&temp_client, netf_rq_lun, msg_len,
+					 msg + SMBUS_MSG_IDX_OFFSET);
+	return ret < 0 ? ret : count;
 }
 
 static __poll_t ipmb_poll(struct file *file, poll_table *wait)
-- 
2.29.2

Re: [PATCH RFC 2/3] media: i2c: adv7842: remove open coded version of SMBus block read

[Wolfram Sang]

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

On Tue, Jan 12, 2021 at 05:41:28PM +0100, Wolfram Sang wrote:
> The open coded version differs from the one in the core in one way: the
> buffer will be always copied back, even when the transfer failed. It
> looks like it is expected that the sanity check for a correct CRC and
> header will bail out later.
> 
> Use the block read from the I2C core and propagate a potential errno
> further to the sanity check.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

$subject should say "adv7511", sorry!


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

Re: [PATCH RFC 2/3] media: i2c: adv7842: remove open coded version of SMBus block read

[Wolfram Sang]

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


> +static int adv7511_edid_rd(struct v4l2_subdev *sd, uint16_t len, uint8_t *buf)
>  {
>  	struct adv7511_state *state = get_adv7511_state(sd);
> +	s32 len;

And 'len' here shadows the function argument :( Ok, I need to resend
this patch. Still, looking forward to a comment if an approach like this
is acceptable.


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

Re: [PATCH RFC 3/3] ipmi: remove open coded version of SMBus block write

[Corey Minyard]

On Tue, Jan 12, 2021 at 05:41:29PM +0100, Wolfram Sang wrote:
> The block-write function of the core was not used because there was no
> client-struct to use. However, in this case it seems apropriate to use a
> temporary client struct. Because we are answering a request we recieved
> when being a client ourselves. So, convert the code to use a temporary
> client and use the block-write function of the I2C core.

I asked the original authors of this about the change, and apparently is
results in a stack size warning.  Arnd Bergmann ask for it to be changed
from what you are suggesting to what it currently is.  See:

https://www.lkml.org/lkml/2019/6/19/440

So apparently this change will cause compile warnings due to the size of
struct i2c_client.

-corey

> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/char/ipmi/ipmb_dev_int.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c
> index 382b28f1cf2f..10d89886e5f3 100644
> --- a/drivers/char/ipmi/ipmb_dev_int.c
> +++ b/drivers/char/ipmi/ipmb_dev_int.c
> @@ -137,7 +137,7 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
>  {
>  	struct ipmb_dev *ipmb_dev = to_ipmb_dev(file);
>  	u8 rq_sa, netf_rq_lun, msg_len;
> -	union i2c_smbus_data data;
> +	struct i2c_client temp_client;
>  	u8 msg[MAX_MSG_LEN];
>  	ssize_t ret;
>  
> @@ -160,21 +160,16 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
>  	}
>  
>  	/*
> -	 * subtract rq_sa and netf_rq_lun from the length of the msg passed to
> -	 * i2c_smbus_xfer
> +	 * subtract rq_sa and netf_rq_lun from the length of the msg. Fill the
> +	 * temporary client. Note that its use is an exception for IPMI.
>  	 */
>  	msg_len = msg[IPMB_MSG_LEN_IDX] - SMBUS_MSG_HEADER_LENGTH;
> -	if (msg_len > I2C_SMBUS_BLOCK_MAX)
> -		msg_len = I2C_SMBUS_BLOCK_MAX;
> +	memcpy(&temp_client, ipmb_dev->client, sizeof(temp_client));
> +	temp_client.addr = rq_sa;
>  
> -	data.block[0] = msg_len;
> -	memcpy(&data.block[1], msg + SMBUS_MSG_IDX_OFFSET, msg_len);
> -	ret = i2c_smbus_xfer(ipmb_dev->client->adapter, rq_sa,
> -			     ipmb_dev->client->flags,
> -			     I2C_SMBUS_WRITE, netf_rq_lun,
> -			     I2C_SMBUS_BLOCK_DATA, &data);
> -
> -	return ret ? : count;
> +	ret = i2c_smbus_write_block_data(&temp_client, netf_rq_lun, msg_len,
> +					 msg + SMBUS_MSG_IDX_OFFSET);
> +	return ret < 0 ? ret : count;
>  }
>  
>  static __poll_t ipmb_poll(struct file *file, poll_table *wait)
> -- 
> 2.29.2
> 

Re: [PATCH RFC 3/3] ipmi: remove open coded version of SMBus block write

[Wolfram Sang]

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


> I asked the original authors of this about the change, and apparently is
> results in a stack size warning.  Arnd Bergmann ask for it to be changed
> from what you are suggesting to what it currently is.  See:
> 
> https://www.lkml.org/lkml/2019/6/19/440
> 
> So apparently this change will cause compile warnings due to the size of
> struct i2c_client.

Wow, didn't know that my patch was actually a revert. I replaced now the
stack usage with kmemdup and will have a second thought about this
patch. Thanks for the heads up!


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

Re: [PATCH RFC 2/3] media: i2c: adv7842: remove open coded version of SMBus block read

[Hans Verkuil]

Hi Wolfram,

On 12/01/2021 17:41, Wolfram Sang wrote:
> The open coded version differs from the one in the core in one way: the
> buffer will be always copied back, even when the transfer failed. It
> looks like it is expected that the sanity check for a correct CRC and
> header will bail out later.

Nah, it's just a bug. It should have returned and checked the error code,
so your patch does the right thing.

Regards,

	Hans

> 
> Use the block read from the I2C core and propagate a potential errno
> further to the sanity check.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Note: we could now make the error checking even stronger by checking if
> the number of received bytes is I2C_SMBUS_BLOCK_MAX. But to avoid
> regressions, I kept the logic as is, i.e. only check for errno.
> 
>  drivers/media/i2c/adv7511-v4l2.c | 40 +++++++++++---------------------
>  1 file changed, 14 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7511-v4l2.c b/drivers/media/i2c/adv7511-v4l2.c
> index a3161d709015..0150f76dc6a6 100644
> --- a/drivers/media/i2c/adv7511-v4l2.c
> +++ b/drivers/media/i2c/adv7511-v4l2.c
> @@ -214,36 +214,24 @@ static inline void adv7511_wr_and_or(struct v4l2_subdev *sd, u8 reg, u8 clr_mask
>  	adv7511_wr(sd, reg, (adv7511_rd(sd, reg) & clr_mask) | val_mask);
>  }
>  
> -static int adv_smbus_read_i2c_block_data(struct i2c_client *client,
> -					 u8 command, unsigned length, u8 *values)
> -{
> -	union i2c_smbus_data data;
> -	int ret;
> -
> -	if (length > I2C_SMBUS_BLOCK_MAX)
> -		length = I2C_SMBUS_BLOCK_MAX;
> -	data.block[0] = length;
> -
> -	ret = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> -			     I2C_SMBUS_READ, command,
> -			     I2C_SMBUS_I2C_BLOCK_DATA, &data);
> -	memcpy(values, data.block + 1, length);
> -	return ret;
> -}
> -
> -static void adv7511_edid_rd(struct v4l2_subdev *sd, uint16_t len, uint8_t *buf)
> +static int adv7511_edid_rd(struct v4l2_subdev *sd, uint16_t len, uint8_t *buf)
>  {
>  	struct adv7511_state *state = get_adv7511_state(sd);
> +	s32 len;
>  	int i;
> -	int err = 0;
>  
>  	v4l2_dbg(1, debug, sd, "%s:\n", __func__);
>  
> -	for (i = 0; !err && i < len; i += I2C_SMBUS_BLOCK_MAX)
> -		err = adv_smbus_read_i2c_block_data(state->i2c_edid, i,
> +	for (i = 0; i < len; i += I2C_SMBUS_BLOCK_MAX) {
> +		len = i2c_smbus_read_i2c_block_data(state->i2c_edid, i,
>  						    I2C_SMBUS_BLOCK_MAX, buf + i);
> -	if (err)
> -		v4l2_err(sd, "%s: i2c read error\n", __func__);
> +		if (len < 0) {
> +			v4l2_err(sd, "%s: i2c read error\n", __func__);
> +			return len;
> +		}
> +	}
> +
> +	return 0;
>  }
>  
>  static inline int adv7511_cec_read(struct v4l2_subdev *sd, u8 reg)
> @@ -1668,20 +1656,20 @@ static bool adv7511_check_edid_status(struct v4l2_subdev *sd)
>  	if (edidRdy & MASK_ADV7511_EDID_RDY) {
>  		int segment = adv7511_rd(sd, 0xc4);
>  		struct adv7511_edid_detect ed;
> +		int err;
>  
>  		if (segment >= EDID_MAX_SEGM) {
>  			v4l2_err(sd, "edid segment number too big\n");
>  			return false;
>  		}
>  		v4l2_dbg(1, debug, sd, "%s: got segment %d\n", __func__, segment);
> -		adv7511_edid_rd(sd, 256, &state->edid.data[segment * 256]);
> +		err = adv7511_edid_rd(sd, 256, &state->edid.data[segment * 256]);
>  		adv7511_dbg_dump_edid(2, debug, sd, segment, &state->edid.data[segment * 256]);
>  		if (segment == 0) {
>  			state->edid.blocks = state->edid.data[0x7e] + 1;
>  			v4l2_dbg(1, debug, sd, "%s: %d blocks in total\n", __func__, state->edid.blocks);
>  		}
> -		if (!edid_verify_crc(sd, segment) ||
> -		    !edid_verify_header(sd, segment)) {
> +		if (err < 0 || !edid_verify_crc(sd, segment) || !edid_verify_header(sd, segment)) {
>  			/* edid crc error, force reread of edid segment */
>  			v4l2_err(sd, "%s: edid crc or header error\n", __func__);
>  			state->have_monitor = false;
> 

Re: [PATCH RFC 1/3] media: i2c: adv7842: remove open coded version of SMBus block write

[Hans Verkuil]

On 12/01/2021 17:41, Wolfram Sang wrote:
> The version here is identical to the one in the I2C core, so use a
> define to keep the original name within the driver but call the I2C core
> function instead.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/media/i2c/adv7842.c | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7842.c b/drivers/media/i2c/adv7842.c
> index 0855f648416d..6ed6bcd1d64d 100644
> --- a/drivers/media/i2c/adv7842.c
> +++ b/drivers/media/i2c/adv7842.c
> @@ -343,19 +343,7 @@ static void adv_smbus_write_byte_no_check(struct i2c_client *client,
>  		       I2C_SMBUS_BYTE_DATA, &data);
>  }
>  
> -static s32 adv_smbus_write_i2c_block_data(struct i2c_client *client,
> -				  u8 command, unsigned length, const u8 *values)
> -{
> -	union i2c_smbus_data data;
> -
> -	if (length > I2C_SMBUS_BLOCK_MAX)
> -		length = I2C_SMBUS_BLOCK_MAX;
> -	data.block[0] = length;
> -	memcpy(data.block + 1, values, length);
> -	return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> -			      I2C_SMBUS_WRITE, command,
> -			      I2C_SMBUS_I2C_BLOCK_DATA, &data);
> -}
> +#define adv_smbus_write_i2c_block_data i2c_smbus_write_i2c_block_data

I would prefer it if the driver just uses i2c_smbus_write_i2c_block_data
directly instead of relying on this define. It's used only 5 times, so
it should be a trivial change.

Regards,

	Hans

>  
>  /* ----------------------------------------------------------------------- */
>  
> 

Re: [PATCH RFC 1/3] media: i2c: adv7842: remove open coded version of SMBus block write

[Wolfram Sang]

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


> > +#define adv_smbus_write_i2c_block_data i2c_smbus_write_i2c_block_data
> 
> I would prefer it if the driver just uses i2c_smbus_write_i2c_block_data
> directly instead of relying on this define. It's used only 5 times, so
> it should be a trivial change.

Okay, will change it!


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

Re: [PATCH RFC 2/3] media: i2c: adv7842: remove open coded version of SMBus block read

[Wolfram Sang]

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

Hello Hans,

I hope you are well!

> > The open coded version differs from the one in the core in one way: the
> > buffer will be always copied back, even when the transfer failed. It
> > looks like it is expected that the sanity check for a correct CRC and
> > header will bail out later.
> 
> Nah, it's just a bug. It should have returned and checked the error code,
> so your patch does the right thing.

I see. So, I will only update the commit message.

Thanks for the reviews!

All the best,

   Wolfram

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