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

STALE2053d

Revision rfc of 2 in this series.

12 messages, 3 authors, 2021-01-18 · open the first message on its own page

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

From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date: 2021-01-12 16:42:36

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

From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date: 2021-01-12 16:42:36

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 3/3] ipmi: remove open coded version of SMBus block write

From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date: 2021-01-12 16:42:36

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

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

From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date: 2021-01-12 16:42:37

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

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

From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date: 2021-01-12 16:45:22

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!

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

From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date: 2021-01-12 19:42:31

+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.

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

From: Corey Minyard <hidden>
Date: 2021-01-14 01:46:16

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

quoted hunk

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

From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date: 2021-01-14 14:05:05

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!

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

From: Hans Verkuil <hidden>
Date: 2021-01-18 09:39:03

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

quoted hunk

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

From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date: 2021-01-18 09:47:31

quoted

+#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!

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

From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Date: 2021-01-18 09:47:31

Hello Hans,

I hope you are well!

quoted

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

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

From: Hans Verkuil <hidden>
Date: 2021-01-18 10:20:11

On 12/01/2021 17:41, Wolfram Sang wrote:

quoted hunk

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

 
 /* ----------------------------------------------------------------------- */

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help