Re: [PATCH net-next v4 2/5] ice: configure FW logging

[Jakub Kicinski <kuba@kernel.org>]

On Thu,  5 Oct 2023 10:01:07 -0700 Tony Nguyen wrote:
> +static ssize_t ice_debugfs_parse_cmd_line(const char __user *src, size_t len,
> +					  char ***argv, int *argc)
> +{
> +	char *cmd_buf, *cmd_buf_tmp;
> +
> +	cmd_buf = memdup_user(src, len + 1);

memdup() with len + 1 is quite suspicious, the buffer has length 
of len you shouldn't copy more than that

> +	if (IS_ERR(cmd_buf))
> +		return PTR_ERR(cmd_buf);
> +	cmd_buf[len] = '\0';
> +
> +	/* the cmd_buf has a newline at the end of the command so
> +	 * remove it
> +	 */
> +	cmd_buf_tmp = strchr(cmd_buf, '\n');
> +	if (cmd_buf_tmp) {
> +		*cmd_buf_tmp = '\0';
> +		len = (size_t)cmd_buf_tmp - (size_t)cmd_buf + 1;
> +	}
> +
> +	*argv = argv_split(GFP_KERNEL, cmd_buf, argc);
> +	if (!*argv)
> +		return -ENOMEM;
> +
> +	kfree(cmd_buf);
> +	return 0;

> +static ssize_t
> +ice_debugfs_module_write(struct file *filp, const char __user *buf,
> +			 size_t count, loff_t *ppos)
> +{
> +	struct ice_pf *pf = filp->private_data;
> +	struct device *dev = ice_pf_to_dev(pf);
> +	ssize_t ret;
> +	char **argv;
> +	int argc;
> +
> +	/* don't allow commands if the FW doesn't support it */
> +	if (!ice_fwlog_supported(&pf->hw))
> +		return -EOPNOTSUPP;
> +
> +	/* don't allow partial writes */
> +	if (*ppos != 0)
> +		return 0;
> +
> +	ret = ice_debugfs_parse_cmd_line(buf, count, &argv, &argc);
> +	if (ret)
> +		goto err_copy_from_user;
> +
> +	if (argc == 2) {
> +		int module, log_level;
> +
> +		module = sysfs_match_string(ice_fwlog_module_string, argv[0]);
> +		if (module < 0) {
> +			dev_info(dev, "unknown module '%s'\n", argv[0]);
> +			ret = -EINVAL;
> +			goto module_write_error;
> +		}
> +
> +		log_level = sysfs_match_string(ice_fwlog_level_string, argv[1]);
> +		if (log_level < 0) {
> +			dev_info(dev, "unknown log level '%s'\n", argv[1]);
> +			ret = -EINVAL;
> +			goto module_write_error;
> +		}

The parsing looks pretty over-engineered.

You can group the entries into structs like this:

struct something {
	const char *name;
	size_t sz;
	enum whatever value;
};
#define FILL_IN_STH(thing) \
	{ .name = thing, sz = sizeof(thing) - 1, value = ICE_..##thing,}

struct something[] = {
  FILL_IN_STH(ALL),
  FILL_IN_STH(MNG),
  ...
};

but with nicer names

Then just:

for entry in array(..) {
  if !strncmp(input, entry->name, entry->sz) {
    str += entry->sz + 1
    found = entry;
    break
  }
}

> +static ssize_t ice_debugfs_resolution_read(struct file *filp,
> +					   char __user *buffer, size_t count,
> +					   loff_t *ppos)
> +{
> +	struct ice_pf *pf = filp->private_data;
> +	struct ice_hw *hw = &pf->hw;
> +	char buff[32] = {};
> +	int status;
> +
> +	/* don't allow commands if the FW doesn't support it */
> +	if (!ice_fwlog_supported(&pf->hw))
> +		return -EOPNOTSUPP;
> +
> +	snprintf(buff, sizeof(buff), "%d\n",
> +		 hw->fwlog_cfg.log_resolution);
> +
> +	status = simple_read_from_buffer(buffer, count, ppos, buff,
> +					 strlen(buff));
> +
> +	return status;
> +}

> +static ssize_t
> +ice_debugfs_resolution_write(struct file *filp, const char __user *buf,
> +			     size_t count, loff_t *ppos)
> +{
> +	struct ice_pf *pf = filp->private_data;
> +	struct device *dev = ice_pf_to_dev(pf);
> +	struct ice_hw *hw = &pf->hw;
> +	ssize_t ret;
> +	char **argv;
> +	int argc;
> +
> +	/* don't allow commands if the FW doesn't support it */
> +	if (!ice_fwlog_supported(hw))
> +		return -EOPNOTSUPP;
> +
> +	/* don't allow partial writes */
> +	if (*ppos != 0)
> +		return 0;
> +
> +	ret = ice_debugfs_parse_cmd_line(buf, count, &argv, &argc);
> +	if (ret)
> +		goto err_copy_from_user;

And for the simple params can you try to reuse existing debugfs
helpers? They can already read and write scalars, all you need
is to inject yourself on the write path to update the config
in the device.