Re: [PATCH v3 2/6] hwmon: (pmbus/core) Add Block WR

[Guenter Roeck <linux@roeck-us.net>]

On 5/29/20 6:05 AM, alexandru.tachici@analog.com wrote:
> From: Alexandru Tachici <alexandru.tachici@analog.com>
> 
> PmBus devices support Block Write-Block Read Process
> Call described in SMBus specification v 2.0 with the
> exception that Block writes and reads are permitted to
> have up 255 data bytes instead of max 32 bytes (SMBus).
> 
> This patch adds Block WR process call support for PMBus.
> 

I don't think I want to have this code in the PMBus core.
We can move it there if needed at some point in the future,
but for the time being I'd rather have it in the driver
needing it.

> Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
> ---
>  drivers/hwmon/pmbus/Kconfig      |  2 +-
>  drivers/hwmon/pmbus/pmbus.h      |  4 ++
>  drivers/hwmon/pmbus/pmbus_core.c | 88 ++++++++++++++++++++++++++++++++
>  3 files changed, 93 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 6949483aa732..f11712fdcea8 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -5,7 +5,7 @@
>  
>  menuconfig PMBUS
>  	tristate "PMBus support"
> -	depends on I2C
> +	depends on I2C && CRC8

This should be select CRC8, not depends.

>  	help
>  	  Say yes here if you want to enable PMBus support.
>  
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index 18e06fc6c53f..ae4e15c5dff2 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -392,6 +392,8 @@ enum pmbus_sensor_classes {
>  #define PMBUS_PHASE_VIRTUAL	BIT(30)	/* Phases on this page are virtual */
>  #define PMBUS_PAGE_VIRTUAL	BIT(31)	/* Page is virtual */
>  
> +#define PMBUS_BLOCK_MAX		255
> +
>  enum pmbus_data_format { linear = 0, direct, vid };
>  enum vrm_version { vr11 = 0, vr12, vr13, imvp9, amd625mv };
>  
> @@ -467,6 +469,8 @@ int pmbus_read_word_data(struct i2c_client *client, int page, int phase,
>  			 u8 reg);
>  int pmbus_write_word_data(struct i2c_client *client, int page, u8 reg,
>  			  u16 word);
> +int pmbus_block_wr(struct i2c_client *client, u8 cmd, u8 w_len, u8 *data_w,
> +		   u8 *data_r);
>  int pmbus_read_byte_data(struct i2c_client *client, int page, u8 reg);
>  int pmbus_write_byte(struct i2c_client *client, int page, u8 value);
>  int pmbus_write_byte_data(struct i2c_client *client, int page, u8 reg,
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 8d321bf7d15b..ef63468da3b5 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -6,6 +6,7 @@
>   * Copyright (c) 2012 Guenter Roeck
>   */
>  
> +#include <linux/crc8.h>
>  #include <linux/debugfs.h>
>  #include <linux/kernel.h>
>  #include <linux/math64.h>
> @@ -44,6 +45,8 @@
>  
>  #define PMBUS_NAME_SIZE		24
>  
> +DECLARE_CRC8_TABLE(pmbus_crc_table);
> +
>  struct pmbus_sensor {
>  	struct pmbus_sensor *next;
>  	char name[PMBUS_NAME_SIZE];	/* sysfs sensor name */
> @@ -184,6 +187,89 @@ int pmbus_set_page(struct i2c_client *client, int page, int phase)
>  }
>  EXPORT_SYMBOL_GPL(pmbus_set_page);
>  
> +/* Block Write/Read command.

I won't accept network-style multi-line comments.

> + * @client: Handle to slave device
> + * @cmd: Byte interpreted by slave
> + * @w_len: Size of write data block; PMBus allows at most 255 bytes
> + * @data_w: byte array which will be written.
> + * @data_r: Byte array into which data will be read; big enough to hold
> + *	the data returned by the slave. PMBus allows at most 255 bytes.
> + *
> + * Different from Block Read as it sends data and waits for the slave to
> + * return a value dependent on that data. The protocol is simply a Write Block
> + * followed by a Read Block without the Read-Block command field and the
> + * Write-Block STOP bit.
> + *
> + * Returns number of bytes read or negative errno.
> + */
> +int pmbus_block_wr(struct i2c_client *client, u8 cmd, u8 w_len,

_wr is misleading, since it suggests an abbreviated _write.
Better use something like _transfer or _xfer.

> +		   u8 *data_w, u8 *data_r)
> +{
> +	u8 write_buf[PMBUS_BLOCK_MAX + 1];
> +	struct i2c_msg msgs[2] = {
> +		{
> +			.addr = client->addr,
> +			.flags = 0,
> +			.buf = write_buf,
> +			.len = w_len + 2,
> +		},
> +		{
> +			.addr = client->addr,
> +			.flags = I2C_M_RD,
> +			.len = PMBUS_BLOCK_MAX,
> +		}
> +	};
> +	u8 addr = 0;
> +	u8 crc = 0;
> +	int ret;
> +
> +	msgs[0].buf[0] = cmd;
> +	msgs[0].buf[1] = w_len;
> +	memcpy(&msgs[0].buf[2], data_w, w_len);
> +

w_len can be up to 255, meaning up to 255 + 2 bytes can be written
into write_buf. Yet, write_buf is only 256 bytes long.

> +	msgs[0].buf = i2c_get_dma_safe_msg_buf(&msgs[0], 1);
> +	if (!msgs[0].buf)
> +		return -ENOMEM;
> +
> +	msgs[1].buf = i2c_get_dma_safe_msg_buf(&msgs[1], 1);
> +	if (!msgs[1].buf) {
> +		i2c_put_dma_safe_msg_buf(msgs[0].buf, &msgs[0], false);
> +		return -ENOMEM;
> +	}
> +
> +	ret = i2c_transfer(client->adapter, msgs, 2);
> +	if (ret != 2) {
> +		dev_err(&client->dev, "I2C transfer error.");

ret may be 1, which would suggest to the caller that one byte
of data was returned. Also, I am not in favor of logging noise.

> +		goto cleanup;
> +	}
> +
> +	if (client->flags & I2C_CLIENT_PEC) {
> +		addr = i2c_8bit_addr_from_msg(&msgs[0]);
> +		crc = crc8(pmbus_crc_table, &addr, 1, crc);
> +		crc = crc8(pmbus_crc_table, msgs[0].buf,  msgs[0].len, crc);
> +
> +		addr = i2c_8bit_addr_from_msg(&msgs[1]);
> +		crc = crc8(pmbus_crc_table, &addr, 1, crc);
> +		crc = crc8(pmbus_crc_table, msgs[1].buf,  msgs[1].buf[0] + 1,
> +			   crc);
> +
> +		if (crc != msgs[1].buf[msgs[1].buf[0] + 1]) {
> +			ret = -EBADMSG;
> +			goto cleanup;
> +		}
> +	}
> +
> +	memcpy(data_r, &msgs[1].buf[1], msgs[1].buf[0]);

The length of the read buffer is 255 bytes, yet the code suggests
that up to 256 bytes can actually be received.

> +	ret = msgs[1].buf[0];
> +
> +cleanup:
> +	i2c_put_dma_safe_msg_buf(msgs[0].buf, &msgs[0], true);
> +	i2c_put_dma_safe_msg_buf(msgs[1].buf, &msgs[1], true);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pmbus_block_wr);
> +
>  int pmbus_write_byte(struct i2c_client *client, int page, u8 value)
>  {
>  	int rv;
> @@ -2522,6 +2608,8 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
>  	if (!data->groups)
>  		return -ENOMEM;
>  
> +	crc8_populate_msb(pmbus_crc_table, 0x7);
> +
>  	i2c_set_clientdata(client, data);
>  	mutex_init(&data->update_lock);
>  	data->dev = dev;
>