Re: [PATCH v2 1/2] hwmon: (pmbus/isl68137) add debugfs config and black box endpoints

[Guenter Roeck]

On Wed, Apr 22, 2020 at 12:27:14PM -0500, Grant Peltier wrote:
> Add debugfs endpoints to support features of 2nd generation Renesas
> digital multiphase voltage regulators that are not compatible with
> sysfs.
> 
> The read_black_box endpoint allows users to read the contents of a
> RAM segment used to record fault conditions within Gen 2 devices.
> 
> The write_config endpoint allows users to write configuration hex
> files to Gen 2 devices which modify how they operate.
> 
> Signed-off-by: Grant Peltier <grantpeltier93@xxxxxxxxx>

Comments inline.

However, the more I look into this, the more concerns I have.
I think we should limit debugfs functions, if they are needed,
to reporting detailed device status. Can you consider handling
configuration from userspace using i2cget / i2cset commands ?

Thanks,
Guenter

> ---
>  drivers/hwmon/pmbus/isl68137.c | 460 ++++++++++++++++++++++++++++++++-
>  1 file changed, 457 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/isl68137.c b/drivers/hwmon/pmbus/isl68137.c
> index 0c622711ef7e..455235a7a56b 100644
> --- a/drivers/hwmon/pmbus/isl68137.c
> +++ b/drivers/hwmon/pmbus/isl68137.c
> @@ -7,10 +7,12 @@
>   *
>   */
>  
> +#include <linux/debugfs.h>
>  #include <linux/err.h>
>  #include <linux/hwmon-sysfs.h>
>  #include <linux/i2c.h>
>  #include <linux/init.h>
> +#include <linux/jiffies.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/string.h>
> @@ -18,8 +20,22 @@
>  
>  #include "pmbus.h"
>  
> -#define ISL68137_VOUT_AVS	0x30
> -#define RAA_DMPVR2_READ_VMON	0xc8
> +#define ISL68137_VOUT_AVS		0x30
> +#define RAA_DMPVR2_DMA_FIX		0xc5
> +#define RAA_DMPVR2_DMA_SEQ		0xc6
> +#define RAA_DMPVR2_DMA_ADDR		0xc7
> +#define RAA_DMPVR2_READ_VMON		0xc8
> +#define RAA_DMPVR2_BB_BASE_ADDR		0xef80
> +#define RAA_DMPVR2_BB_WSIZE		4
> +#define RAA_DMPVR2_BB_WCNT		32
> +#define RAA_DMPVR2_BB_BUF_SIZE		288
> +#define RAA_DMPVR2_NVM_CNT_ADDR		0x00c2
> +#define RAA_DMPVR2_PRGM_STATUS_ADDR	0x0707
> +#define RAA_DMPVR2_BANK0_STATUS_ADDR	0x0709
> +#define RAA_DMPVR2_BANK1_STATUS_ADDR	0x070a
> +#define RAA_DMPVR2_CFG_MAX_SLOT		16
> +#define RAA_DMPVR2_CFG_HEAD_LEN		290
> +#define RAA_DMPVR2_CFG_SLOT_LEN		358
>  
>  enum chips {
>  	isl68137,
> @@ -71,6 +87,21 @@ enum variants {
>  	raa_dmpvr2_hv,
>  };
>  
> +enum {
> +	RAA_DMPVR2_DEBUGFS_CFG_W = 0,
> +	RAA_DMPVR2_DEBUGFS_BB_R,
> +	RAA_DMPVR2_DEBUGFS_NUM_ENTRIES,
> +};
> +
> +struct raa_dmpvr2_ctrl {
> +	enum chips part;
> +	struct i2c_client *client;
> +	int debugfs_entries[RAA_DMPVR2_DEBUGFS_NUM_ENTRIES];
> +};
> +
> +#define to_ctrl(x, y) container_of((x), struct raa_dmpvr2_ctrl, \
> +				   debugfs_entries[(y)])
> +
>  static ssize_t isl68137_avs_enable_show_page(struct i2c_client *client,
>  					     int page,
>  					     char *buf)
> @@ -157,6 +188,395 @@ static const struct attribute_group *isl68137_attribute_groups[] = {
>  	NULL,
>  };
>  
> +/**
> + * Helper function required since linux SMBus implementation does not currently
> + * (v5.6) support the SMBus 3.0 "Read 32" protocol
> + */
> +static s32 raa_dmpvr2_smbus_read32(const struct i2c_client *client, u8 command,
> +				   unsigned char *data)
> +{
> +	int status;
> +	unsigned char msgbuf[1];
> +	struct i2c_msg msg[2] = {
> +		{
> +			.addr = client->addr,
> +			.flags = client->flags,
> +			.len = 1,
> +			.buf = msgbuf,
> +		},
> +		{
> +			.addr = client->addr,
> +			.flags = client->flags | I2C_M_RD,
> +			.len = 4,
> +			.buf = data,
> +		},
> +	};
> +
> +	msgbuf[0] = command;
> +	status = i2c_transfer(client->adapter, msg, 2);
> +	if (status != 2)
> +		return status;
> +	return 0;

The return value from this function is not checked. Either check it,
or don't return a value.

> +}
> +
> +/**
> + * Helper function required since linux SMBus implementation does not currently
> + * (v5.6) support the SMBus 3.0 "Write 32" protocol
> + */
> +static s32 raa_dmpvr2_smbus_write32(const struct i2c_client *client,
> +				    u8 command, u32 value)
> +{
> +	int status;
> +	unsigned char msgbuf[5];
> +	struct i2c_msg msg[1] = {
> +		{
> +			.addr = client->addr,
> +			.flags = client->flags,
> +			.len = 5,
> +			.buf = msgbuf,
> +		},
> +	};
> +	msgbuf[0] = command;
> +	msgbuf[1] = value & 0x0FF;
> +	msgbuf[2] = (value >> 8) & 0x0FF;
> +	msgbuf[3] = (value >> 16) & 0x0FF;
> +	msgbuf[4] = (value >> 24) &  0x0FF;
> +	status = i2c_transfer(client->adapter, msg, 1);
> +	if (status != 1)
> +		return status;
> +	return 0;
> +}
> +
> +static ssize_t raa_dmpvr2_read_black_box(struct raa_dmpvr2_ctrl *ctrl,
> +					 char __user *buf, size_t len,
> +					 loff_t *ppos)
> +{
> +	int i, j;
> +	u16 addr = RAA_DMPVR2_BB_BASE_ADDR;
> +	unsigned char data[RAA_DMPVR2_BB_WSIZE] = { 0 };
> +	char out[RAA_DMPVR2_BB_BUF_SIZE] = { 0 };
> +	char *optr = out;
> +
> +	i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR, addr);
> +	for (i = 0; i < RAA_DMPVR2_BB_WCNT; i++) {
> +		raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_SEQ,
> +					data);
> +		for (j = 0; j < RAA_DMPVR2_BB_WSIZE; j++)
> +			optr += snprintf(optr, sizeof(out), "%02X", data[j]);

The length check here only ensures that a single call to snprintf() doesn't add
more data to the buffer than available. However, since optr is advanced, there
is no practical limit on the total data written.

I understand that the maximum amount of data written in practice
is RAA_DMPVR2_BB_WCNT * ((RAA_DMPVR2_BB_WSIZE * 2) + 1), or 
32 * ((4 * 2) + 1) = 288, meaning the buffer is large enough, but
using snprintf() with sizeof(out) to limit the length of data written
is pretty pointless. Somewhat cleaner code might keep track of the length
written, write to out + length, and pass sizeof(out) - length as parameter.

> +		*optr = '\n';
> +		optr++;
> +	}
> +
> +	return simple_read_from_buffer(buf, len, ppos, &out, sizeof(out));
> +}
> +
> +struct raa_dmpvr_cfg_cmd {
> +	u8 cmd;
> +	u8 len;
> +	u8 data[4];
> +};
> +
> +struct raa_dmpvr2_cfg {
> +	u8 dev_id[4];
> +	u8 dev_rev[4];
> +	int slot_cnt;
> +	int cmd_cnt;
> +	struct raa_dmpvr_cfg_cmd *cmds;
> +	u8 crc[4];
> +};
> +
> +/**
> + * Helper function to handle hex string to byte conversions
> + */
> +static int raa_dmpvr2_hextou8(char *buf, u8 *res)
> +{
> +	char s[3];
> +
> +	s[0] = buf[0];
> +	s[1] = buf[1];
> +	s[2] = '\0';
> +	return kstrtou8(s, 16, res);
> +}
> +
> +static int raa_dmpvr2_parse_cfg(char *buf, struct raa_dmpvr2_cfg *cfg)
> +{
> +	const int lsta = 2;
> +	const int csta = 6;
> +	const int dsta = 8;
> +	char *cptr, *line;
> +	int lcnt = 1;
> +	int ccnt = 0;
> +	int i, j, ret;
> +	u8 b;
> +
> +	// Ensure there is enough memory for the file

Please don't mix c++ comments into a driver using c style comments.

> +	for (i = 0; i < strlen(buf); i++) {
> +		if (buf[i] == '\n' && i < strlen(buf)-2) {
> +			lcnt++;
> +			if (buf[i+1] != '4' || buf[i+2] != '9')

Space around operators, please.

> +				ccnt++;
> +		}
> +	}
> +	cfg->slot_cnt = (lcnt-RAA_DMPVR2_CFG_HEAD_LEN)/RAA_DMPVR2_CFG_SLOT_LEN;
> +	if (cfg->slot_cnt < 1 || cfg->slot_cnt > RAA_DMPVR2_CFG_MAX_SLOT)
> +		return -EINVAL;
> +	cfg->cmd_cnt = ccnt;
> +	cfg->cmds = kmalloc_array(ccnt, sizeof(struct raa_dmpvr_cfg_cmd),
> +				  GFP_KERNEL);
> +	if (!cfg->cmds)
> +		return -ENOMEM;
> +
> +	// Parse header
> +	for (i = 0; i < 2; i++) {
> +		line = strsep(&buf, "\n");
> +		cptr = line + dsta;
> +		for (j = 3; j >= 0; j--) {
> +			ret = raa_dmpvr2_hextou8(cptr, &b);
> +			if (ret)
> +				goto parse_err;
> +			if (i == 0)
> +				cfg->dev_id[j] = b;
> +			else
> +				cfg->dev_rev[j] = b;
> +			cptr += 2;
> +		}
> +	}
> +
> +	// Parse cmds
> +	i = 0;
> +	while (line != NULL && strlen(line) > (dsta + 2)) {
> +		if (strncmp(line, "49", 2) != 0) {
> +			ret = raa_dmpvr2_hextou8(line+lsta, &b);
> +			if (ret)
> +				goto parse_err;
> +			cfg->cmds[i].len = b - 3;
> +			ret = raa_dmpvr2_hextou8(line+csta, &b);
> +			if (ret)
> +				goto parse_err;
> +			cfg->cmds[i].cmd = b;
> +			for (j = 0; j < cfg->cmds[i].len; j++) {
> +				cptr = line + dsta + (2 * j);
> +				ret = raa_dmpvr2_hextou8(cptr, &b);
> +				if (ret)
> +					goto parse_err;
> +				cfg->cmds[i].data[j] = b;
> +			}
> +			i++;
> +		}
> +		line = strsep(&buf, "\n");
> +	}
> +	return 0;
> +
> +parse_err:
> +	kfree(cfg->cmds);
> +	return ret;
> +}
> +
> +static int raa_dmpvr2_verify_device(struct raa_dmpvr2_ctrl *ctrl,
> +				    struct raa_dmpvr2_cfg *cfg)
> +{
> +	u8 dev_id[5];
> +	u8 dev_rev[5];
> +	int res;
> +
> +	res = i2c_smbus_read_i2c_block_data(ctrl->client, PMBUS_IC_DEVICE_ID,
> +					    5, dev_id);
> +	if (res != 5)
> +		return -EIO;

The calling code returns this as error code.

> +
> +	res = i2c_smbus_read_i2c_block_data(ctrl->client, PMBUS_IC_DEVICE_REV,
> +					    5, dev_rev);

It still puzzles me, quite frankly, why i2c_smbus_read_block_data()
would not work here.

> +	if (res != 5)
> +		return -EIO;
> +
> +	return memcmp(cfg->dev_id, dev_id+1, 4)
> +		| memcmp(cfg->dev_rev+3, dev_rev+4, 1);

Please no arithmetic or of return codes. Besides, this is dangerous:
A mismatch returns a value <0 or >0 depending on the match, and this
is then returned as valid (and pretty much random) error code.
This function should return -ENODEV on error.

> +}
> +
> +static int raa_dmpvr2_check_cfg(struct raa_dmpvr2_ctrl *ctrl,
> +				struct raa_dmpvr2_cfg *cfg)
> +{
> +	u8 data[4];
> +
> +	i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
> +				  RAA_DMPVR2_NVM_CNT_ADDR);
> +	raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_SEQ, data);
> +	if (cfg->slot_cnt > data[0])

If raa_dmpvr2_smbus_read32 returns an error, the content of data[] is
random. In that case, this code is checking a random value.

> +		return -EINVAL;
> +	return 0;
> +}
> +
> +static int raa_dmpvr2_send_cfg(struct raa_dmpvr2_ctrl *ctrl,
> +			       struct raa_dmpvr2_cfg *cfg)
> +{
> +	int i, status;
> +	u16 word;
> +	u32 dbl_word;
> +
> +	for (i = 0; i < cfg->cmd_cnt; i++) {
> +		if (cfg->cmds[i].len == 2) {
> +			word = cfg->cmds[i].data[0]
> +				| (cfg->cmds[i].data[1] << 8);
> +			status = i2c_smbus_write_word_data(ctrl->client,
> +							   cfg->cmds[i].cmd,
> +							   word);
> +			if (status < 0)
> +				return status;
> +		} else if (cfg->cmds[i].len == 4) {
> +			dbl_word = cfg->cmds[i].data[0]
> +				| (cfg->cmds[i].data[1] << 8)
> +				| (cfg->cmds[i].data[2] << 16)
> +				| (cfg->cmds[i].data[3] << 24);
> +			status = raa_dmpvr2_smbus_write32(ctrl->client,
> +							  cfg->cmds[i].cmd,
> +							  dbl_word);
> +			if (status < 0)
> +				return status;
> +		} else {
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int raa_dmpvr2_cfg_write_result(struct raa_dmpvr2_ctrl *ctrl,
> +				       struct raa_dmpvr2_cfg *cfg)
> +{
> +	u8 data[4] = {0};
> +	u8 data1[4];
> +	u8 *dptr;
> +	unsigned long start;
> +	int i, j, status;
> +
> +	// Check programmer status
> +	start = jiffies;
> +	i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
> +				  RAA_DMPVR2_PRGM_STATUS_ADDR);
> +	while (data[0] == 0 && !time_after(jiffies, start + HZ + HZ)) {
> +		raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_FIX,
> +					data);
> +	}
> +	if (data[0] != 1)
> +		return -ETIME;

Are you sure ? Normally I would expect ETIMEDOUT.

> +
> +	// Check bank statuses
> +	i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
> +				  RAA_DMPVR2_BANK0_STATUS_ADDR);
> +	raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_FIX, data);
> +	i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
> +				  RAA_DMPVR2_BANK1_STATUS_ADDR);
> +	raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_FIX, data1);
> +
> +	for (i = 0; i < cfg->slot_cnt; i++) {
> +		if (i < 8) {
> +			j = i;
> +			dptr = data;
> +		} else {
> +			j = i - 8;
> +			dptr = data1;
> +		}
> +		status = (dptr[j/2] >> (4 * (j % 2))) & 0x0F;
> +		if (status != 1)
> +			return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t raa_dmpvr2_write_cfg(struct raa_dmpvr2_ctrl *ctrl,
> +				    const char __user *buf, size_t len,
> +				    loff_t *ppos)
> +{
> +	char *cbuf;
> +	int ret, status;
> +	struct raa_dmpvr2_cfg *cfg;
> +
> +	cfg = kmalloc(sizeof(*cfg), GFP_KERNEL);
> +	if (!cfg)
> +		return -ENOMEM;
> +
> +	cbuf = kmalloc(len, GFP_KERNEL);
> +	if (!cbuf) {
> +		status = -ENOMEM;
> +		goto buf_err;
> +	}
> +	ret = simple_write_to_buffer(cbuf, len, ppos, buf, len);
> +
> +	// Parse file
> +	status = raa_dmpvr2_parse_cfg(cbuf, cfg);
> +	if (status)
> +		goto parse_err;
> +	// Verify device and file IDs/revs match
> +	status = raa_dmpvr2_verify_device(ctrl, cfg);
> +	if (status)
> +		goto dev_err;
> +	// Verify enough NVM slots available
> +	status = raa_dmpvr2_check_cfg(ctrl, cfg);
> +	if (status)
> +		goto dev_err;
> +	// Write CFG to device
> +	status = raa_dmpvr2_send_cfg(ctrl, cfg);
> +	if (status)
> +		goto dev_err;
> +	// Verify programming success
> +	status = raa_dmpvr2_cfg_write_result(ctrl, cfg);
> +	if (status)
> +		goto dev_err;
> +	// Free memory

That comment is a bit useless.

> +	kfree(cbuf);
> +	kfree(cfg->cmds);
> +	kfree(cfg);
> +	return ret;
> +
> +	// Handle Errors

Another useless comment.

> +dev_err:
> +	kfree(cfg->cmds);
> +parse_err:
> +	kfree(cbuf);
> +buf_err:
> +	kfree(cfg);
> +	return status;
> +}
> +
> +static ssize_t raa_dmpvr2_debugfs_read(struct file *file, char __user *buf,
> +				       size_t len, loff_t *ppos)
> +{
> +	int *idxp = file->private_data;
> +	int idx = *idxp;
> +	struct raa_dmpvr2_ctrl *ctrl = to_ctrl(idxp, idx);
> +
> +	switch (idx) {
> +	case RAA_DMPVR2_DEBUGFS_BB_R:
> +		return raa_dmpvr2_read_black_box(ctrl, buf, len, ppos);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static ssize_t raa_dmpvr2_debugfs_write(struct file *file,
> +					const char __user *buf, size_t len,
> +					loff_t *ppos)
> +{
> +	int *idxp = file->private_data;
> +	int idx = *idxp;
> +	struct raa_dmpvr2_ctrl *ctrl = to_ctrl(idxp, idx);
> +
> +	switch (idx) {
> +	case RAA_DMPVR2_DEBUGFS_CFG_W:
> +		return raa_dmpvr2_write_cfg(ctrl, buf, len, ppos);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct file_operations raa_dmpvr2_debugfs_fops = {
> +	.llseek = noop_llseek,
> +	.read = raa_dmpvr2_debugfs_read,
> +	.write = raa_dmpvr2_debugfs_write,
> +	.open = simple_open,
> +};
> +
>  static int raa_dmpvr2_read_word_data(struct i2c_client *client, int page,
>  				     int phase, int reg)
>  {
> @@ -220,7 +640,11 @@ static struct pmbus_driver_info raa_dmpvr_info = {
>  static int isl68137_probe(struct i2c_client *client,
>  			  const struct i2c_device_id *id)
>  {
> +	int i, ret;
>  	struct pmbus_driver_info *info;
> +	struct dentry *debugfs;
> +	struct dentry *debug_dir;
> +	struct raa_dmpvr2_ctrl *ctrl;
>  
>  	info = devm_kzalloc(&client->dev, sizeof(*info), GFP_KERNEL);
>  	if (!info)
> @@ -262,7 +686,37 @@ static int isl68137_probe(struct i2c_client *client,
>  		return -ENODEV;
>  	}
>  
> -	return pmbus_do_probe(client, id, info);
> +	// No debugfs features for Gen 1
> +	if (id->driver_data == raa_dmpvr1_2rail)
> +		return pmbus_do_probe(client, id, info);
> +
> +	ret = pmbus_do_probe(client, id, info);
> +	if (ret != 0)
> +		return ret;

Why not do the version check and return here ?

	if (ret || id->driver_data == raa_dmpvr1_2rail)
		return ret;

> +
> +	ctrl = devm_kzalloc(&client->dev, sizeof(*ctrl), GFP_KERNEL);
> +	if (!ctrl)
> +		return 0;
> +
> +	ctrl->client = client;
> +	debugfs = pmbus_get_debugfs_dir(client);
> +	if (!debugfs)
> +		return 0;
> +
> +	debug_dir = debugfs_create_dir(client->name, debugfs);
> +	if (!debug_dir)
> +		return 0;
> +
> +	for (i = 0; i < RAA_DMPVR2_DEBUGFS_NUM_ENTRIES; i++)
> +		ctrl->debugfs_entries[i] = i;
> +
> +	debugfs_create_file("write_config", 0220, debug_dir,
> +			    &ctrl->debugfs_entries[RAA_DMPVR2_DEBUGFS_CFG_W],
> +			    &raa_dmpvr2_debugfs_fops);
> +	debugfs_create_file("read_black_box", 0440, debug_dir,
> +			    &ctrl->debugfs_entries[RAA_DMPVR2_DEBUGFS_BB_R],
> +			    &raa_dmpvr2_debugfs_fops);
> +	return 0;
>  }
>  
>  static const struct i2c_device_id raa_dmpvr_id[] = {