Date: Tue, 10 Oct 2023 16:26:15 -0700
From: Paul M Stillwell Jr <paul.m.stillwell.jr@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, Tony Nguyen <anthony.l.nguyen@...el.com>
CC: <davem@...emloft.net>, <pabeni@...hat.com>, <edumazet@...gle.com>,
	<netdev@...r.kernel.org>, <jacob.e.keller@...el.com>,
	<vaishnavi.tipireddy@...el.com>, <horms@...nel.org>, <leon@...nel.org>, Pucha
 Himasekhar Reddy <himasekharx.reddy.pucha@...el.com>
Subject: Re: [PATCH net-next v4 2/5] ice: configure FW logging

On 10/6/2023 5:02 PM, Jakub Kicinski wrote:
> 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
> 

Yeah, you're right. I'll double check that... IIRC there was a reason I 
did it; for some reason I think it had to do with the CRLF from the user 
input, but that may have been an issue in a previous iteration.

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

I'm probably missing something here, but I don't know if this will do 
what I need or not. What I have is a user passing a module name and a 
log level and I'm trying to match those strings and create integer 
values from them so I can configure the FW log for that module. I'm not 
seeing how the above gets me there...

I was trying to not use strncmp and instead use the built in kernel 
string matching functions so that's how I ended up with the code I have

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

I would use existing debugfs helpers, but the ones that exist look like 
they are really set up for variables you want to read only. We need to 
do some validation on the input when the user is writing and I don't see 
how we could do that with the existing debugfs_create_* helpers. I've 
looked through the kernel code at uses of debugfs_create_* and couldn't 
find one that was setting up a writable file, but I could have missed it.

If you can point me to some code that does what you want then I'm happy 
to try it, I just couldn't find anything.

Powered by blists - more mailing lists