[PATCH v4 0/2] pinctrl: pinmux: Add pinmux-select debugfs file

[PATCH v4 0/2] pinctrl: pinmux: Add pinmux-select debugfs file

[Drew Fustini]

This series first converts the debugfs files in the pinctrl subsystem to
octal permissions and then adds a new debugfs file "pinmux-select".

Function name and group name can be written to "pinmux-select" which
will cause the function and group to be activated on the pin controller.

Notes for PATCH v4:
- correct the commit message in the second patch to reference function
  and group name instead of integer selectors. Apologies for not fixing
  that in v3
- fix typos in cover letter

Notes for PATCH v3:
- add Suggested-by: Andy Shevchenko to the "pinctrl: use to octal
  permissions for debugfs files" patch
- change the octal permissions from 0400 to 0444 to correctly match the
  symbolic permissions (thanks to Joe Perches and Geert Uytterhoeven)
- note that S_IFREG flag is added to the mode in __debugfs_create_file()
  (thanks to Andy for highlighting this and Joe for suggesting I should
  add a note to the commit message)
- fix order of the goto labels so that the buffers are freed correctly
  as suggested by Dan Carpenter
- move from devm_kzalloc() to kzalloc() as the buffers are only used
  inside the pinmux_select() function and not related to the lifetime
  of the pin controller device (thanks to Andy for pointing this out)
- correct the pinmux-select example in commit message to use the
  function and group name instead of selector (thanks to Geert)

Notes for PATCH v2:
- create patch series that includes patch to switch all the debugfs
  files in pinctrl subsystem over to octal permission
- write function name and group name, instead of error-prone selector
  numbers, to the 'pinmux-select' file
- switch from static to dynamic allocation for the kernel buffer filled
  by strncpy_from_user()
- look up function selector from function name using
  pinmux_func_name_to_selector()
- validate group name with get_function_groups() and match_string()
- look up selector for group name with pinctrl_get_group_selector()

Notes for PATCH v1:
- posted seperate patch to switch all the debugfs files in pinctrl
  subsystem over to octal permission [1]
- there is no existing documentation for any of the debugfs enteries for
  pinctrl, so it seemed to have a bigger scope than just this patch. I
  also noticed that rst documentation is confusingly named "pinctl" (no
  'r') and started thread about that [2]. Linus suggested chaning that
  to 'pin-control'. Thus I am planning a seperate documentation patch
  series where the file is renamed, references changed and a section on
  the pinctrl debugfs files is added.

Notes for RFC v2 [3]:
- rename debugfs file "pinmux-set" to "pinmux-select"
- renmae pinmux_set_write() to pinmux_select()
- switch from memdup_user_nul() to strncpy_from_user()
- switch from pr_warn() to dev_err()

[1] https://lore.kernel.org/linux-gpio/20210126044742.87602-1-drew@beagleboard.org/
[2] https://lore.kernel.org/linux-gpio/20210126050817.GA187797@x1/
[3] https://lore.kernel.org/linux-gpio/20210123064909.466225-1-drew@beagleboard.org/

Drew Fustini (2):
  pinctrl: use to octal permissions for debugfs files
  pinctrl: pinmux: Add pinmux-select debugfs file

 drivers/pinctrl/core.c    |   6 +--
 drivers/pinctrl/pinconf.c |   4 +-
 drivers/pinctrl/pinmux.c  | 111 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 114 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH v4 1/2] pinctrl: use to octal permissions for debugfs files

[Drew Fustini]

Switch over pinctrl debugfs files to use octal permissions as they are
preferred over symbolic permissions. Refer to commit f90774e1fd27
("checkpatch: look for symbolic permissions and suggest octal instead").

Note: S_IFREG flag is added to the mode by __debugfs_create_file()
in fs/debugfs/inode.c

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Drew Fustini <drew@beagleboard.org>
---
 drivers/pinctrl/core.c    | 6 +++---
 drivers/pinctrl/pinconf.c | 4 ++--
 drivers/pinctrl/pinmux.c  | 4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 3663d87f51a0..02f8710afb9c 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1914,11 +1914,11 @@ static void pinctrl_init_debugfs(void)
 		return;
 	}
 
-	debugfs_create_file("pinctrl-devices", S_IFREG | S_IRUGO,
+	debugfs_create_file("pinctrl-devices", 0444,
 			    debugfs_root, NULL, &pinctrl_devices_fops);
-	debugfs_create_file("pinctrl-maps", S_IFREG | S_IRUGO,
+	debugfs_create_file("pinctrl-maps", 0444,
 			    debugfs_root, NULL, &pinctrl_maps_fops);
-	debugfs_create_file("pinctrl-handles", S_IFREG | S_IRUGO,
+	debugfs_create_file("pinctrl-handles", 0444,
 			    debugfs_root, NULL, &pinctrl_fops);
 }
 
diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
index 02c075cc010b..d9d54065472e 100644
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -370,9 +370,9 @@ DEFINE_SHOW_ATTRIBUTE(pinconf_groups);
 void pinconf_init_device_debugfs(struct dentry *devroot,
 			 struct pinctrl_dev *pctldev)
 {
-	debugfs_create_file("pinconf-pins", S_IFREG | S_IRUGO,
+	debugfs_create_file("pinconf-pins", 0444,
 			    devroot, pctldev, &pinconf_pins_fops);
-	debugfs_create_file("pinconf-groups", S_IFREG | S_IRUGO,
+	debugfs_create_file("pinconf-groups", 0444,
 			    devroot, pctldev, &pinconf_groups_fops);
 }
 
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index bab888fe3f8e..c651b2db0925 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -676,9 +676,9 @@ DEFINE_SHOW_ATTRIBUTE(pinmux_pins);
 void pinmux_init_device_debugfs(struct dentry *devroot,
 			 struct pinctrl_dev *pctldev)
 {
-	debugfs_create_file("pinmux-functions", S_IFREG | S_IRUGO,
+	debugfs_create_file("pinmux-functions", 0444,
 			    devroot, pctldev, &pinmux_functions_fops);
-	debugfs_create_file("pinmux-pins", S_IFREG | S_IRUGO,
+	debugfs_create_file("pinmux-pins", 0444,
 			    devroot, pctldev, &pinmux_pins_fops);
 }
 
-- 
2.25.1

Re: [PATCH v4 1/2] pinctrl: use to octal permissions for debugfs files

[Joe Perches]

On Wed, 2021-02-10 at 14:28 -0800, Drew Fustini wrote:
> Switch over pinctrl debugfs files to use octal permissions as they are
> preferred over symbolic permissions. Refer to commit f90774e1fd27
> ("checkpatch: look for symbolic permissions and suggest octal instead").
> 
> Note: S_IFREG flag is added to the mode by __debugfs_create_file()
> in fs/debugfs/inode.c
> 
> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Drew Fustini <drew@beagleboard.org>
> ---
>  drivers/pinctrl/core.c    | 6 +++---
>  drivers/pinctrl/pinconf.c | 4 ++--
>  drivers/pinctrl/pinmux.c  | 4 ++--
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index 3663d87f51a0..02f8710afb9c 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -1914,11 +1914,11 @@ static void pinctrl_init_debugfs(void)
>  		return;
>  	}
>  
> 
> -	debugfs_create_file("pinctrl-devices", S_IFREG | S_IRUGO,
> +	debugfs_create_file("pinctrl-devices", 0444,
>  			    debugfs_root, NULL, &pinctrl_devices_fops);
> -	debugfs_create_file("pinctrl-maps", S_IFREG | S_IRUGO,
> +	debugfs_create_file("pinctrl-maps", 0444,
>  			    debugfs_root, NULL, &pinctrl_maps_fops);
> -	debugfs_create_file("pinctrl-handles", S_IFREG | S_IRUGO,
> +	debugfs_create_file("pinctrl-handles", 0444,
>  			    debugfs_root, NULL, &pinctrl_fops);
>  }

Why aren't you also converting this block in the same file?

@@ -1890,11 +1890,11 @@ static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev)
                        dev_name(pctldev->dev));
                return;
        }
-       debugfs_create_file("pins", S_IFREG | S_IRUGO,
+       debugfs_create_file("pins", S_IFREG | 0444,
                            device_root, pctldev, &pinctrl_pins_fops);
-       debugfs_create_file("pingroups", S_IFREG | S_IRUGO,
+       debugfs_create_file("pingroups", S_IFREG | 0444,
                            device_root, pctldev, &pinctrl_groups_fops);
-       debugfs_create_file("gpio-ranges", S_IFREG | S_IRUGO,
+       debugfs_create_file("gpio-ranges", S_IFREG | 0444,
                            device_root, pctldev, &pinctrl_gpioranges_fops);
        if (pctldev->desc->pmxops)
                pinmux_init_device_debugfs(device_root, pctldev);

Re: [PATCH v4 1/2] pinctrl: use to octal permissions for debugfs files

[Drew Fustini]

On Wed, Feb 10, 2021 at 11:36:39PM -0800, Joe Perches wrote:
> On Wed, 2021-02-10 at 14:28 -0800, Drew Fustini wrote:
> > Switch over pinctrl debugfs files to use octal permissions as they are
> > preferred over symbolic permissions. Refer to commit f90774e1fd27
> > ("checkpatch: look for symbolic permissions and suggest octal instead").
> > 
> > Note: S_IFREG flag is added to the mode by __debugfs_create_file()
> > in fs/debugfs/inode.c
> > 
> > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Signed-off-by: Drew Fustini <drew@beagleboard.org>
> > ---
> >  drivers/pinctrl/core.c    | 6 +++---
> >  drivers/pinctrl/pinconf.c | 4 ++--
> >  drivers/pinctrl/pinmux.c  | 4 ++--
> >  3 files changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> > index 3663d87f51a0..02f8710afb9c 100644
> > --- a/drivers/pinctrl/core.c
> > +++ b/drivers/pinctrl/core.c
> > @@ -1914,11 +1914,11 @@ static void pinctrl_init_debugfs(void)
> >  		return;
> >  	}
> >  
> > 
> > -	debugfs_create_file("pinctrl-devices", S_IFREG | S_IRUGO,
> > +	debugfs_create_file("pinctrl-devices", 0444,
> >  			    debugfs_root, NULL, &pinctrl_devices_fops);
> > -	debugfs_create_file("pinctrl-maps", S_IFREG | S_IRUGO,
> > +	debugfs_create_file("pinctrl-maps", 0444,
> >  			    debugfs_root, NULL, &pinctrl_maps_fops);
> > -	debugfs_create_file("pinctrl-handles", S_IFREG | S_IRUGO,
> > +	debugfs_create_file("pinctrl-handles", 0444,
> >  			    debugfs_root, NULL, &pinctrl_fops);
> >  }
> 
> Why aren't you also converting this block in the same file?
> 
> @@ -1890,11 +1890,11 @@ static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev)
>                         dev_name(pctldev->dev));
>                 return;
>         }
> -       debugfs_create_file("pins", S_IFREG | S_IRUGO,
> +       debugfs_create_file("pins", S_IFREG | 0444,
>                             device_root, pctldev, &pinctrl_pins_fops);
> -       debugfs_create_file("pingroups", S_IFREG | S_IRUGO,
> +       debugfs_create_file("pingroups", S_IFREG | 0444,
>                             device_root, pctldev, &pinctrl_groups_fops);
> -       debugfs_create_file("gpio-ranges", S_IFREG | S_IRUGO,
> +       debugfs_create_file("gpio-ranges", S_IFREG | 0444,
>                             device_root, pctldev, &pinctrl_gpioranges_fops);
>         if (pctldev->desc->pmxops)
>                 pinmux_init_device_debugfs(device_root, pctldev);
> 
> 
> 

Thank you, that is a very good point.  I should have included those
calls to debugfs_create_file() in the patch as well.  I will fix that
in the next revision.  It looks like I also need to change how sscanf()
is being handle per the other thread of discussion.

Thanks,
Drew

[PATCH v4 2/2] pinctrl: pinmux: Add pinmux-select debugfs file

[Drew Fustini]

Add "pinmux-select" to debugfs which will activate a function and group
when "<function-name group-name>" are written to the file. The write
operation pinmux_select() handles this by checking that the names map to
valid selectors and then calling ops->set_mux().

The existing "pinmux-functions" debugfs file lists the pin functions
registered for the pin controller. For example:

function: pinmux-uart0, groups = [ pinmux-uart0-pins ]
function: pinmux-mmc0, groups = [ pinmux-mmc0-pins ]
function: pinmux-mmc1, groups = [ pinmux-mmc1-pins ]
function: pinmux-i2c0, groups = [ pinmux-i2c0-pins ]
function: pinmux-i2c1, groups = [ pinmux-i2c1-pins ]
function: pinmux-spi1, groups = [ pinmux-spi1-pins ]

To activate function pinmux-i2c1 and group pinmux-i2c1-pins:

echo "pinmux-i2c1 pinmux-i2c1-pins" > pinmux-select

Signed-off-by: Drew Fustini <drew@beagleboard.org>
---
 drivers/pinctrl/pinmux.c | 107 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index c651b2db0925..23fa32f0a067 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -673,6 +673,111 @@ void pinmux_show_setting(struct seq_file *s,
 DEFINE_SHOW_ATTRIBUTE(pinmux_functions);
 DEFINE_SHOW_ATTRIBUTE(pinmux_pins);
 
+#define PINMUX_MAX_NAME 64
+static ssize_t pinmux_select(struct file *file, const char __user *user_buf,
+				   size_t len, loff_t *ppos)
+{
+	struct seq_file *sfile = file->private_data;
+	struct pinctrl_dev *pctldev = sfile->private;
+	const struct pinmux_ops *pmxops = pctldev->desc->pmxops;
+	const char *const *groups;
+	char *buf, *fname, *gname;
+	unsigned int num_groups;
+	int fsel, gsel, ret;
+
+	if (len > (PINMUX_MAX_NAME * 2)) {
+		dev_err(pctldev->dev, "write too big for buffer");
+		return -EINVAL;
+	}
+
+	buf = kzalloc(PINMUX_MAX_NAME * 2, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	fname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
+	if (!fname) {
+		ret = -ENOMEM;
+		goto free_buf;
+	}
+
+	gname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
+	if (!buf) {
+		ret = -ENOMEM;
+		goto free_fname;
+	}
+
+	ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
+	if (ret < 0) {
+		dev_err(pctldev->dev, "failed to copy buffer from userspace");
+		goto free_gname;
+	}
+	buf[len-1] = '\0';
+
+	ret = sscanf(buf, "%s %s", fname, gname);
+	if (ret != 2) {
+		dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
+		goto free_gname;
+	}
+
+	fsel = pinmux_func_name_to_selector(pctldev, fname);
+	if (fsel < 0) {
+		dev_err(pctldev->dev, "invalid function %s in map table\n", fname);
+		ret = -EINVAL;
+		goto free_gname;
+	}
+
+	ret = pmxops->get_function_groups(pctldev, fsel, &groups, &num_groups);
+	if (ret) {
+		dev_err(pctldev->dev, "no groups for function %d (%s)", fsel, fname);
+		goto free_gname;
+
+	}
+
+	ret = match_string(groups, num_groups, gname);
+	if (ret < 0) {
+		dev_err(pctldev->dev, "invalid group %s", gname);
+		goto free_gname;
+	}
+
+	ret = pinctrl_get_group_selector(pctldev, gname);
+	if (ret < 0) {
+		dev_err(pctldev->dev, "failed to get group selectorL %s", gname);
+		goto free_gname;
+	}
+	gsel = ret;
+
+	ret = pmxops->set_mux(pctldev, fsel, gsel);
+	if (ret) {
+		dev_err(pctldev->dev, "set_mux() failed: %d", ret);
+		goto free_gname;
+	}
+
+	return len;
+
+free_gname:
+	devm_kfree(pctldev->dev, gname);
+free_fname:
+	devm_kfree(pctldev->dev, fname);
+free_buf:
+	devm_kfree(pctldev->dev, buf);
+
+	return ret;
+}
+
+static int pinmux_select_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, NULL, inode->i_private);
+}
+
+static const struct file_operations pinmux_select_ops = {
+	.owner = THIS_MODULE,
+	.open = pinmux_select_open,
+	.read = seq_read,
+	.write = pinmux_select,
+	.llseek = no_llseek,
+	.release = single_release,
+};
+
 void pinmux_init_device_debugfs(struct dentry *devroot,
 			 struct pinctrl_dev *pctldev)
 {
@@ -680,6 +785,8 @@ void pinmux_init_device_debugfs(struct dentry *devroot,
 			    devroot, pctldev, &pinmux_functions_fops);
 	debugfs_create_file("pinmux-pins", 0444,
 			    devroot, pctldev, &pinmux_pins_fops);
+	debugfs_create_file("pinmux-select", 0200,
+			    devroot, pctldev, &pinmux_select_ops);
 }
 
 #endif /* CONFIG_DEBUG_FS */
-- 
2.25.1

Re: [PATCH v4 2/2] pinctrl: pinmux: Add pinmux-select debugfs file

[Dan Carpenter]

On Wed, Feb 10, 2021 at 02:28:54PM -0800, Drew Fustini wrote:
> +	ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
> +	if (ret < 0) {
> +		dev_err(pctldev->dev, "failed to copy buffer from userspace");
> +		goto free_gname;
> +	}
> +	buf[len-1] = '\0';
> +
> +	ret = sscanf(buf, "%s %s", fname, gname);
> +	if (ret != 2) {
> +		dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> +		goto free_gname;

We need a "ret = -EINVAL;" before the goto.  sscanf doesn't return error
codes.  Normally we would write it like so:

	if (sscanf(buf, "%s %s", fname, gname) != 2) {
		dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
		ret = -EINVAL;
		goto free_gname;
	}

I'm going to write a Smatch check for this today.

> +	}
> +
> +	fsel = pinmux_func_name_to_selector(pctldev, fname);
> +	if (fsel < 0) {
> +		dev_err(pctldev->dev, "invalid function %s in map table\n", fname);
> +		ret = -EINVAL;

ret = fsel;

> +		goto free_gname;
> +	}
> +

regards,
dan carpenter

Re: [PATCH v4 2/2] pinctrl: pinmux: Add pinmux-select debugfs file

[Joe Perches]

On Thu, 2021-02-11 at 10:11 +0300, Dan Carpenter wrote:
> On Wed, Feb 10, 2021 at 02:28:54PM -0800, Drew Fustini wrote:
> > +	ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
> > +	if (ret < 0) {
> > +		dev_err(pctldev->dev, "failed to copy buffer from userspace");
> > +		goto free_gname;
> > +	}
> > +	buf[len-1] = '\0';
> > +
> > +	ret = sscanf(buf, "%s %s", fname, gname);
> > +	if (ret != 2) {
> > +		dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > +		goto free_gname;
> 
> We need a "ret = -EINVAL;" before the goto.  sscanf doesn't return error
> codes.  Normally we would write it like so:
> 
> 	if (sscanf(buf, "%s %s", fname, gname) != 2) {
> 		dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> 		ret = -EINVAL;
> 		goto free_gname;
> 	}
> 
> I'm going to write a Smatch check for this today.

It's a pretty frequently used style:

$ git grep -P '\w+\s*=\s+sscanf\b' | wc -l
327

A grep with -A5 seems to show most use some additional error assignment
when checking the return value.

$ git grep -P -A5 '\w+\s*=\s+sscanf\b' | grep -P '(?:return|=)\s*\-E' | wc -l
174

Re: [PATCH v4 2/2] pinctrl: pinmux: Add pinmux-select debugfs file

[Dan Carpenter]

On Wed, Feb 10, 2021 at 11:24:23PM -0800, Joe Perches wrote:
> On Thu, 2021-02-11 at 10:11 +0300, Dan Carpenter wrote:
> > On Wed, Feb 10, 2021 at 02:28:54PM -0800, Drew Fustini wrote:
> > > +	ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
> > > +	if (ret < 0) {
> > > +		dev_err(pctldev->dev, "failed to copy buffer from userspace");
> > > +		goto free_gname;
> > > +	}
> > > +	buf[len-1] = '\0';
> > > +
> > > +	ret = sscanf(buf, "%s %s", fname, gname);
> > > +	if (ret != 2) {
> > > +		dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > > +		goto free_gname;
> > 
> > We need a "ret = -EINVAL;" before the goto.  sscanf doesn't return error
> > codes.  Normally we would write it like so:
> > 
> > 	if (sscanf(buf, "%s %s", fname, gname) != 2) {
> > 		dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > 		ret = -EINVAL;
> > 		goto free_gname;
> > 	}
> > 
> > I'm going to write a Smatch check for this today.
> 
> It's a pretty frequently used style:
> 
> $ git grep -P '\w+\s*=\s+sscanf\b' | wc -l
> 327

Yeah.  That's true.  I looked through a couple of those and they were
fine.  (Sample size 2)  But the other format is more common.

$ git grep sscanf | grep = | wc -l
803

I have written a Smatch check to complain whenever we propogate the
return value from sscanf.  I'll let you know tomorrow how that goes.

I should write another check which says "On this error path, we know
sscanf was not equal to the value we wanted but we are still returning
success".

regards,
dan carpenter

Re: [PATCH v4 2/2] pinctrl: pinmux: Add pinmux-select debugfs file

[Drew Fustini]

On Thu, Feb 11, 2021 at 10:39:38AM +0300, Dan Carpenter wrote:
> On Wed, Feb 10, 2021 at 11:24:23PM -0800, Joe Perches wrote:
> > On Thu, 2021-02-11 at 10:11 +0300, Dan Carpenter wrote:
> > > On Wed, Feb 10, 2021 at 02:28:54PM -0800, Drew Fustini wrote:
> > > > +	ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
> > > > +	if (ret < 0) {
> > > > +		dev_err(pctldev->dev, "failed to copy buffer from userspace");
> > > > +		goto free_gname;
> > > > +	}
> > > > +	buf[len-1] = '\0';
> > > > +
> > > > +	ret = sscanf(buf, "%s %s", fname, gname);
> > > > +	if (ret != 2) {
> > > > +		dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > > > +		goto free_gname;
> > > 
> > > We need a "ret = -EINVAL;" before the goto.  sscanf doesn't return error
> > > codes.  Normally we would write it like so:
> > > 
> > > 	if (sscanf(buf, "%s %s", fname, gname) != 2) {
> > > 		dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > > 		ret = -EINVAL;
> > > 		goto free_gname;
> > > 	}
> > > 
> > > I'm going to write a Smatch check for this today.
> > 
> > It's a pretty frequently used style:
> > 
> > $ git grep -P '\w+\s*=\s+sscanf\b' | wc -l
> > 327
> 
> Yeah.  That's true.  I looked through a couple of those and they were
> fine.  (Sample size 2)  But the other format is more common.
> 
> $ git grep sscanf | grep = | wc -l
> 803
> 
> I have written a Smatch check to complain whenever we propogate the
> return value from sscanf.  I'll let you know tomorrow how that goes.
> 
> I should write another check which says "On this error path, we know
> sscanf was not equal to the value we wanted but we are still returning
> success".
> 
> regards,
> dan carpenter
> 

Thank you for comments regarding sscanf().  And also thank you for the
LF mentorship session on smatch this morning.  It helped me understand
it much better.

Based on further comments, it seems there are better ways for me to pull
the strings out of the write buffer, but I will keep this in mind if I
need to use sscanf() in the future.

-Drew

Re: [PATCH v4 2/2] pinctrl: pinmux: Add pinmux-select debugfs file

[Dan Carpenter]

On Thu, Feb 11, 2021 at 07:35:33PM -0800, Drew Fustini wrote:
> On Thu, Feb 11, 2021 at 10:39:38AM +0300, Dan Carpenter wrote:
> > On Wed, Feb 10, 2021 at 11:24:23PM -0800, Joe Perches wrote:
> > > On Thu, 2021-02-11 at 10:11 +0300, Dan Carpenter wrote:
> > > > On Wed, Feb 10, 2021 at 02:28:54PM -0800, Drew Fustini wrote:
> > > > > +	ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
> > > > > +	if (ret < 0) {
> > > > > +		dev_err(pctldev->dev, "failed to copy buffer from userspace");
> > > > > +		goto free_gname;
> > > > > +	}
> > > > > +	buf[len-1] = '\0';
> > > > > +
> > > > > +	ret = sscanf(buf, "%s %s", fname, gname);
> > > > > +	if (ret != 2) {
> > > > > +		dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > > > > +		goto free_gname;
> > > > 
> > > > We need a "ret = -EINVAL;" before the goto.  sscanf doesn't return error
> > > > codes.  Normally we would write it like so:
> > > > 
> > > > 	if (sscanf(buf, "%s %s", fname, gname) != 2) {
> > > > 		dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > > > 		ret = -EINVAL;
> > > > 		goto free_gname;
> > > > 	}
> > > > 
> > > > I'm going to write a Smatch check for this today.
> > > 
> > > It's a pretty frequently used style:
> > > 
> > > $ git grep -P '\w+\s*=\s+sscanf\b' | wc -l
> > > 327
> > 
> > Yeah.  That's true.  I looked through a couple of those and they were
> > fine.  (Sample size 2)  But the other format is more common.
> > 
> > $ git grep sscanf | grep = | wc -l
> > 803
> > 
> > I have written a Smatch check to complain whenever we propogate the
> > return value from sscanf.  I'll let you know tomorrow how that goes.
> > 
> > I should write another check which says "On this error path, we know
> > sscanf was not equal to the value we wanted but we are still returning
> > success".
> > 
> > regards,
> > dan carpenter
> > 
> 
> Thank you for comments regarding sscanf().  And also thank you for the
> LF mentorship session on smatch this morning.  It helped me understand
> it much better.

Good deal!

The warning about propagating errors from sscanf caught a couple bugs.
The one about returning success if sscanf failed didn't catch anything.

The sscanf overflow patch didn't find anything either, but I think we've
had those bugs in the past and so I expect some in the future so I will
keep that one in my private tests without pushing it.

regards,
dan carpenter

Re: [PATCH v4 2/2] pinctrl: pinmux: Add pinmux-select debugfs file

[Geert Uytterhoeven]

Hi Drew,

On Wed, Feb 10, 2021 at 11:33 PM Drew Fustini <drew@beagleboard.org> wrote:
> Add "pinmux-select" to debugfs which will activate a function and group
> when "<function-name group-name>" are written to the file. The write
> operation pinmux_select() handles this by checking that the names map to
> valid selectors and then calling ops->set_mux().
>
> The existing "pinmux-functions" debugfs file lists the pin functions
> registered for the pin controller. For example:
>
> function: pinmux-uart0, groups = [ pinmux-uart0-pins ]
> function: pinmux-mmc0, groups = [ pinmux-mmc0-pins ]
> function: pinmux-mmc1, groups = [ pinmux-mmc1-pins ]
> function: pinmux-i2c0, groups = [ pinmux-i2c0-pins ]
> function: pinmux-i2c1, groups = [ pinmux-i2c1-pins ]
> function: pinmux-spi1, groups = [ pinmux-spi1-pins ]
>
> To activate function pinmux-i2c1 and group pinmux-i2c1-pins:
>
> echo "pinmux-i2c1 pinmux-i2c1-pins" > pinmux-select
>
> Signed-off-by: Drew Fustini <drew@beagleboard.org>

Thanks for your patch!

> --- a/drivers/pinctrl/pinmux.c
> +++ b/drivers/pinctrl/pinmux.c
> @@ -673,6 +673,111 @@ void pinmux_show_setting(struct seq_file *s,
>  DEFINE_SHOW_ATTRIBUTE(pinmux_functions);
>  DEFINE_SHOW_ATTRIBUTE(pinmux_pins);
>
> +#define PINMUX_MAX_NAME 64
> +static ssize_t pinmux_select(struct file *file, const char __user *user_buf,
> +                                  size_t len, loff_t *ppos)
> +{
> +       struct seq_file *sfile = file->private_data;
> +       struct pinctrl_dev *pctldev = sfile->private;
> +       const struct pinmux_ops *pmxops = pctldev->desc->pmxops;
> +       const char *const *groups;
> +       char *buf, *fname, *gname;
> +       unsigned int num_groups;
> +       int fsel, gsel, ret;
> +
> +       if (len > (PINMUX_MAX_NAME * 2)) {
> +               dev_err(pctldev->dev, "write too big for buffer");
> +               return -EINVAL;
> +       }
> +
> +       buf = kzalloc(PINMUX_MAX_NAME * 2, GFP_KERNEL);
> +       if (!buf)
> +               return -ENOMEM;
> +
> +       fname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
> +       if (!fname) {
> +               ret = -ENOMEM;
> +               goto free_buf;
> +       }
> +
> +       gname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
> +       if (!buf) {
> +               ret = -ENOMEM;
> +               goto free_fname;
> +       }
> +
> +       ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);

While this guarantees buf is not overflowed...

> +       if (ret < 0) {
> +               dev_err(pctldev->dev, "failed to copy buffer from userspace");
> +               goto free_gname;
> +       }
> +       buf[len-1] = '\0';
> +
> +       ret = sscanf(buf, "%s %s", fname, gname);

... one of the two strings can still be longer than PINMUX_MAX_NAME,
thus overflowing fname or gname.

As buf is already a copy, it may be easier to just find the strings in
buf, write the NUL terminators into buf, and set up fname and gname
to point to the strings inside buf.

> +       if (ret != 2) {
> +               dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> +               goto free_gname;
> +       }

> +static const struct file_operations pinmux_select_ops = {
> +       .owner = THIS_MODULE,
> +       .open = pinmux_select_open,
> +       .read = seq_read,

I don't think you need to fill in .read for a write-only file.

> +       .write = pinmux_select,
> +       .llseek = no_llseek,
> +       .release = single_release,
> +};
> +
>  void pinmux_init_device_debugfs(struct dentry *devroot,
>                          struct pinctrl_dev *pctldev)
>  {

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 2/2] pinctrl: pinmux: Add pinmux-select debugfs file

[Andy Shevchenko]

On Thu, Feb 11, 2021 at 10:09 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Feb 10, 2021 at 11:33 PM Drew Fustini <drew@beagleboard.org> wrote:


> > +#define PINMUX_MAX_NAME 64

> > +       if (len > (PINMUX_MAX_NAME * 2)) {
> > +               dev_err(pctldev->dev, "write too big for buffer");
> > +               return -EINVAL;
> > +       }
> > +
> > +       buf = kzalloc(PINMUX_MAX_NAME * 2, GFP_KERNEL);

> > +       ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
>
> While this guarantees buf is not overflowed...
>
> > +       if (ret < 0) {
> > +               dev_err(pctldev->dev, "failed to copy buffer from userspace");
> > +               goto free_gname;
> > +       }
> > +       buf[len-1] = '\0';
> > +
> > +       ret = sscanf(buf, "%s %s", fname, gname);
>
> ... one of the two strings can still be longer than PINMUX_MAX_NAME,
> thus overflowing fname or gname.
>
> As buf is already a copy, it may be easier to just find the strings in
> buf, write the NUL terminators into buf, and set up fname and gname
> to point to the strings inside buf.

You beat me up to it. I was about to comment the same.

So, indeed, instead of sscanf it's simply better and faster to do just
something like

fname = strstrip(buf) ;
if (*fname == '\0') {
  ...
  return -EINVAL;
}

gname = strchr(fname, ' ');
if (!gname) {
 ...
  return -EINVAL;
}
*gname++ = '\0';

on top of the buf pointer.

> > +       if (ret != 2) {
> > +               dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > +               goto free_gname;
> > +       }



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/2] pinctrl: pinmux: Add pinmux-select debugfs file

[Drew Fustini]

On Thu, Feb 11, 2021 at 11:53:24AM +0200, Andy Shevchenko wrote:
> On Thu, Feb 11, 2021 at 10:09 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Wed, Feb 10, 2021 at 11:33 PM Drew Fustini <drew@beagleboard.org> wrote:
> 
> 
> > > +#define PINMUX_MAX_NAME 64
> 
> > > +       if (len > (PINMUX_MAX_NAME * 2)) {
> > > +               dev_err(pctldev->dev, "write too big for buffer");
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       buf = kzalloc(PINMUX_MAX_NAME * 2, GFP_KERNEL);
> 
> > > +       ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
> >
> > While this guarantees buf is not overflowed...
> >
> > > +       if (ret < 0) {
> > > +               dev_err(pctldev->dev, "failed to copy buffer from userspace");
> > > +               goto free_gname;
> > > +       }
> > > +       buf[len-1] = '\0';
> > > +
> > > +       ret = sscanf(buf, "%s %s", fname, gname);
> >
> > ... one of the two strings can still be longer than PINMUX_MAX_NAME,
> > thus overflowing fname or gname.
> >
> > As buf is already a copy, it may be easier to just find the strings in
> > buf, write the NUL terminators into buf, and set up fname and gname
> > to point to the strings inside buf.
> 
> You beat me up to it. I was about to comment the same.
> 
> So, indeed, instead of sscanf it's simply better and faster to do just
> something like
> 
> fname = strstrip(buf) ;
> if (*fname == '\0') {
>   ...
>   return -EINVAL;
> }
> 
> gname = strchr(fname, ' ');
> if (!gname) {
>  ...
>   return -EINVAL;
> }
> *gname++ = '\0';
> 
> on top of the buf pointer.
> 

Thank you for the suggestion about how to implement this.  I'll use that
in the next revision.

-Drew

Re: [PATCH v4 2/2] pinctrl: pinmux: Add pinmux-select debugfs file

[Drew Fustini]

On Thu, Feb 11, 2021 at 09:09:03AM +0100, Geert Uytterhoeven wrote:
> Hi Drew,
> 
> On Wed, Feb 10, 2021 at 11:33 PM Drew Fustini <drew@beagleboard.org> wrote:
> > Add "pinmux-select" to debugfs which will activate a function and group
> > when "<function-name group-name>" are written to the file. The write
> > operation pinmux_select() handles this by checking that the names map to
> > valid selectors and then calling ops->set_mux().
> >
> > The existing "pinmux-functions" debugfs file lists the pin functions
> > registered for the pin controller. For example:
> >
> > function: pinmux-uart0, groups = [ pinmux-uart0-pins ]
> > function: pinmux-mmc0, groups = [ pinmux-mmc0-pins ]
> > function: pinmux-mmc1, groups = [ pinmux-mmc1-pins ]
> > function: pinmux-i2c0, groups = [ pinmux-i2c0-pins ]
> > function: pinmux-i2c1, groups = [ pinmux-i2c1-pins ]
> > function: pinmux-spi1, groups = [ pinmux-spi1-pins ]
> >
> > To activate function pinmux-i2c1 and group pinmux-i2c1-pins:
> >
> > echo "pinmux-i2c1 pinmux-i2c1-pins" > pinmux-select
> >
> > Signed-off-by: Drew Fustini <drew@beagleboard.org>
> 
> Thanks for your patch!
> 
> > --- a/drivers/pinctrl/pinmux.c
> > +++ b/drivers/pinctrl/pinmux.c
> > @@ -673,6 +673,111 @@ void pinmux_show_setting(struct seq_file *s,
> >  DEFINE_SHOW_ATTRIBUTE(pinmux_functions);
> >  DEFINE_SHOW_ATTRIBUTE(pinmux_pins);
> >
> > +#define PINMUX_MAX_NAME 64
> > +static ssize_t pinmux_select(struct file *file, const char __user *user_buf,
> > +                                  size_t len, loff_t *ppos)
> > +{
> > +       struct seq_file *sfile = file->private_data;
> > +       struct pinctrl_dev *pctldev = sfile->private;
> > +       const struct pinmux_ops *pmxops = pctldev->desc->pmxops;
> > +       const char *const *groups;
> > +       char *buf, *fname, *gname;
> > +       unsigned int num_groups;
> > +       int fsel, gsel, ret;
> > +
> > +       if (len > (PINMUX_MAX_NAME * 2)) {
> > +               dev_err(pctldev->dev, "write too big for buffer");
> > +               return -EINVAL;
> > +       }
> > +
> > +       buf = kzalloc(PINMUX_MAX_NAME * 2, GFP_KERNEL);
> > +       if (!buf)
> > +               return -ENOMEM;
> > +
> > +       fname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
> > +       if (!fname) {
> > +               ret = -ENOMEM;
> > +               goto free_buf;
> > +       }
> > +
> > +       gname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
> > +       if (!buf) {
> > +               ret = -ENOMEM;
> > +               goto free_fname;
> > +       }
> > +
> > +       ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
> 
> While this guarantees buf is not overflowed...
> 
> > +       if (ret < 0) {
> > +               dev_err(pctldev->dev, "failed to copy buffer from userspace");
> > +               goto free_gname;
> > +       }
> > +       buf[len-1] = '\0';
> > +
> > +       ret = sscanf(buf, "%s %s", fname, gname);
> 
> ... one of the two strings can still be longer than PINMUX_MAX_NAME,
> thus overflowing fname or gname.
> 
> As buf is already a copy, it may be easier to just find the strings in
> buf, write the NUL terminators into buf, and set up fname and gname
> to point to the strings inside buf.

Thank you for pointing this out.  I should have considered that one name
could be much larger than the other name.  I see Andy suggested
alternative to sscanf() that gets around having to allocate seperate
buffers for each name.

> > +       if (ret != 2) {
> > +               dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > +               goto free_gname;
> > +       }
> 
> > +static const struct file_operations pinmux_select_ops = {
> > +       .owner = THIS_MODULE,
> > +       .open = pinmux_select_open,
> > +       .read = seq_read,
> 
> I don't think you need to fill in .read for a write-only file.

Thanks, I'll remove it.
> 
> > +       .write = pinmux_select,
> > +       .llseek = no_llseek,
> > +       .release = single_release,
> > +};
> > +
> >  void pinmux_init_device_debugfs(struct dentry *devroot,
> >                          struct pinctrl_dev *pctldev)
> >  {
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH v4 2/2] pinctrl: pinmux: Add pinmux-select debugfs file

[Dan Carpenter]

On Wed, Feb 10, 2021 at 02:28:54PM -0800, Drew Fustini wrote:
> Add "pinmux-select" to debugfs which will activate a function and group
> when "<function-name group-name>" are written to the file. The write
> operation pinmux_select() handles this by checking that the names map to
> valid selectors and then calling ops->set_mux().
> 
> The existing "pinmux-functions" debugfs file lists the pin functions
> registered for the pin controller. For example:
> 
> function: pinmux-uart0, groups = [ pinmux-uart0-pins ]
> function: pinmux-mmc0, groups = [ pinmux-mmc0-pins ]
> function: pinmux-mmc1, groups = [ pinmux-mmc1-pins ]
> function: pinmux-i2c0, groups = [ pinmux-i2c0-pins ]
> function: pinmux-i2c1, groups = [ pinmux-i2c1-pins ]
> function: pinmux-spi1, groups = [ pinmux-spi1-pins ]
> 
> To activate function pinmux-i2c1 and group pinmux-i2c1-pins:
> 
> echo "pinmux-i2c1 pinmux-i2c1-pins" > pinmux-select
> 
> Signed-off-by: Drew Fustini <drew@beagleboard.org>
> ---
>  drivers/pinctrl/pinmux.c | 107 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 107 insertions(+)
> 
> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
> index c651b2db0925..23fa32f0a067 100644
> --- a/drivers/pinctrl/pinmux.c
> +++ b/drivers/pinctrl/pinmux.c
> @@ -673,6 +673,111 @@ void pinmux_show_setting(struct seq_file *s,
>  DEFINE_SHOW_ATTRIBUTE(pinmux_functions);
>  DEFINE_SHOW_ATTRIBUTE(pinmux_pins);
>  
> +#define PINMUX_MAX_NAME 64
> +static ssize_t pinmux_select(struct file *file, const char __user *user_buf,
> +				   size_t len, loff_t *ppos)
> +{
> +	struct seq_file *sfile = file->private_data;
> +	struct pinctrl_dev *pctldev = sfile->private;
> +	const struct pinmux_ops *pmxops = pctldev->desc->pmxops;
> +	const char *const *groups;
> +	char *buf, *fname, *gname;
> +	unsigned int num_groups;
> +	int fsel, gsel, ret;
> +
> +	if (len > (PINMUX_MAX_NAME * 2)) {
> +		dev_err(pctldev->dev, "write too big for buffer");
> +		return -EINVAL;
> +	}
> +
> +	buf = kzalloc(PINMUX_MAX_NAME * 2, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	fname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
> +	if (!fname) {
> +		ret = -ENOMEM;
> +		goto free_buf;
> +	}
> +
> +	gname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
> +	if (!buf) {
> +		ret = -ENOMEM;
> +		goto free_fname;
> +	}
> +
> +	ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
> +	if (ret < 0) {
> +		dev_err(pctldev->dev, "failed to copy buffer from userspace");
> +		goto free_gname;
> +	}
> +	buf[len-1] = '\0';
> +
> +	ret = sscanf(buf, "%s %s", fname, gname);
> +	if (ret != 2) {
> +		dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> +		goto free_gname;
> +	}
> +
> +	fsel = pinmux_func_name_to_selector(pctldev, fname);
> +	if (fsel < 0) {
> +		dev_err(pctldev->dev, "invalid function %s in map table\n", fname);
> +		ret = -EINVAL;
> +		goto free_gname;
> +	}
> +
> +	ret = pmxops->get_function_groups(pctldev, fsel, &groups, &num_groups);
> +	if (ret) {
> +		dev_err(pctldev->dev, "no groups for function %d (%s)", fsel, fname);
> +		goto free_gname;
> +
> +	}
> +
> +	ret = match_string(groups, num_groups, gname);
> +	if (ret < 0) {
> +		dev_err(pctldev->dev, "invalid group %s", gname);
> +		goto free_gname;
> +	}
> +
> +	ret = pinctrl_get_group_selector(pctldev, gname);
> +	if (ret < 0) {
> +		dev_err(pctldev->dev, "failed to get group selectorL %s", gname);
> +		goto free_gname;
> +	}
> +	gsel = ret;
> +
> +	ret = pmxops->set_mux(pctldev, fsel, gsel);
> +	if (ret) {
> +		dev_err(pctldev->dev, "set_mux() failed: %d", ret);
> +		goto free_gname;
> +	}
> +
> +	return len;
> +
> +free_gname:
> +	devm_kfree(pctldev->dev, gname);
> +free_fname:
> +	devm_kfree(pctldev->dev, fname);
> +free_buf:
> +	devm_kfree(pctldev->dev, buf);

Ugh...  I honestly thought Smatch was supposed to print a warning when
you used devm_kfree() on kzalloc()ed memory, but I guess the warning is
only the other way around.

Smatch does complain about it as a leak because it was expecting a
regular free.

drivers/pinctrl/pinmux.c:330 pinmux_func_name_to_selector() warn: potential NULL parameter dereference 'fname'
drivers/pinctrl/pinmux.c:764 pinmux_select() warn: possible memory leak of 'gname'
drivers/pinctrl/pinmux.c:764 pinmux_select() warn: sscanf doesn't return error codes
drivers/pinctrl/pinmux.c:764 pinmux_select() warn: returning success when sscanf failed

And what about the success path?  Shouldn't we free these on the success
path as well?

regards,
dan carpenter

Re: [PATCH v4 2/2] pinctrl: pinmux: Add pinmux-select debugfs file

[Drew Fustini]

On Thu, Feb 11, 2021 at 03:00:51PM +0300, Dan Carpenter wrote:
> On Wed, Feb 10, 2021 at 02:28:54PM -0800, Drew Fustini wrote:
> > Add "pinmux-select" to debugfs which will activate a function and group
> > when "<function-name group-name>" are written to the file. The write
> > operation pinmux_select() handles this by checking that the names map to
> > valid selectors and then calling ops->set_mux().
> > 
> > The existing "pinmux-functions" debugfs file lists the pin functions
> > registered for the pin controller. For example:
> > 
> > function: pinmux-uart0, groups = [ pinmux-uart0-pins ]
> > function: pinmux-mmc0, groups = [ pinmux-mmc0-pins ]
> > function: pinmux-mmc1, groups = [ pinmux-mmc1-pins ]
> > function: pinmux-i2c0, groups = [ pinmux-i2c0-pins ]
> > function: pinmux-i2c1, groups = [ pinmux-i2c1-pins ]
> > function: pinmux-spi1, groups = [ pinmux-spi1-pins ]
> > 
> > To activate function pinmux-i2c1 and group pinmux-i2c1-pins:
> > 
> > echo "pinmux-i2c1 pinmux-i2c1-pins" > pinmux-select
> > 
> > Signed-off-by: Drew Fustini <drew@beagleboard.org>
> > ---
> >  drivers/pinctrl/pinmux.c | 107 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 107 insertions(+)
> > 
> > diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
> > index c651b2db0925..23fa32f0a067 100644
> > --- a/drivers/pinctrl/pinmux.c
> > +++ b/drivers/pinctrl/pinmux.c
> > @@ -673,6 +673,111 @@ void pinmux_show_setting(struct seq_file *s,
> >  DEFINE_SHOW_ATTRIBUTE(pinmux_functions);
> >  DEFINE_SHOW_ATTRIBUTE(pinmux_pins);
> >  
> > +#define PINMUX_MAX_NAME 64
> > +static ssize_t pinmux_select(struct file *file, const char __user *user_buf,
> > +				   size_t len, loff_t *ppos)
> > +{
> > +	struct seq_file *sfile = file->private_data;
> > +	struct pinctrl_dev *pctldev = sfile->private;
> > +	const struct pinmux_ops *pmxops = pctldev->desc->pmxops;
> > +	const char *const *groups;
> > +	char *buf, *fname, *gname;
> > +	unsigned int num_groups;
> > +	int fsel, gsel, ret;
> > +
> > +	if (len > (PINMUX_MAX_NAME * 2)) {
> > +		dev_err(pctldev->dev, "write too big for buffer");
> > +		return -EINVAL;
> > +	}
> > +
> > +	buf = kzalloc(PINMUX_MAX_NAME * 2, GFP_KERNEL);
> > +	if (!buf)
> > +		return -ENOMEM;
> > +
> > +	fname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
> > +	if (!fname) {
> > +		ret = -ENOMEM;
> > +		goto free_buf;
> > +	}
> > +
> > +	gname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
> > +	if (!buf) {
> > +		ret = -ENOMEM;
> > +		goto free_fname;
> > +	}
> > +
> > +	ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
> > +	if (ret < 0) {
> > +		dev_err(pctldev->dev, "failed to copy buffer from userspace");
> > +		goto free_gname;
> > +	}
> > +	buf[len-1] = '\0';
> > +
> > +	ret = sscanf(buf, "%s %s", fname, gname);
> > +	if (ret != 2) {
> > +		dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > +		goto free_gname;
> > +	}
> > +
> > +	fsel = pinmux_func_name_to_selector(pctldev, fname);
> > +	if (fsel < 0) {
> > +		dev_err(pctldev->dev, "invalid function %s in map table\n", fname);
> > +		ret = -EINVAL;
> > +		goto free_gname;
> > +	}
> > +
> > +	ret = pmxops->get_function_groups(pctldev, fsel, &groups, &num_groups);
> > +	if (ret) {
> > +		dev_err(pctldev->dev, "no groups for function %d (%s)", fsel, fname);
> > +		goto free_gname;
> > +
> > +	}
> > +
> > +	ret = match_string(groups, num_groups, gname);
> > +	if (ret < 0) {
> > +		dev_err(pctldev->dev, "invalid group %s", gname);
> > +		goto free_gname;
> > +	}
> > +
> > +	ret = pinctrl_get_group_selector(pctldev, gname);
> > +	if (ret < 0) {
> > +		dev_err(pctldev->dev, "failed to get group selectorL %s", gname);
> > +		goto free_gname;
> > +	}
> > +	gsel = ret;
> > +
> > +	ret = pmxops->set_mux(pctldev, fsel, gsel);
> > +	if (ret) {
> > +		dev_err(pctldev->dev, "set_mux() failed: %d", ret);
> > +		goto free_gname;
> > +	}
> > +
> > +	return len;
> > +
> > +free_gname:
> > +	devm_kfree(pctldev->dev, gname);
> > +free_fname:
> > +	devm_kfree(pctldev->dev, fname);
> > +free_buf:
> > +	devm_kfree(pctldev->dev, buf);
> 
> Ugh...  I honestly thought Smatch was supposed to print a warning when
> you used devm_kfree() on kzalloc()ed memory, but I guess the warning is
> only the other way around.
> 
> Smatch does complain about it as a leak because it was expecting a
> regular free.
> 
> drivers/pinctrl/pinmux.c:330 pinmux_func_name_to_selector() warn: potential NULL parameter dereference 'fname'
> drivers/pinctrl/pinmux.c:764 pinmux_select() warn: possible memory leak of 'gname'
> drivers/pinctrl/pinmux.c:764 pinmux_select() warn: sscanf doesn't return error codes
> drivers/pinctrl/pinmux.c:764 pinmux_select() warn: returning success when sscanf failed

Thank you pointing this out.  I should have switched back to normal free
now that I no longer using a device managed buffer.

> 
> And what about the success path?  Shouldn't we free these on the success
> path as well?

Good point.  That is my fault.  I will update it so that it correctly
free's buffer no matter what happens in the function.


thanks,
drew

Re: [PATCH v4 2/2] pinctrl: pinmux: Add pinmux-select debugfs file

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 8258 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210210222851.232374-3-drew@beagleboard.org>
References: <20210210222851.232374-3-drew@beagleboard.org>
TO: Drew Fustini <drew@beagleboard.org>
TO: Linus Walleij <linus.walleij@linaro.org>
TO: linux-gpio(a)vger.kernel.org
TO: linux-kernel(a)vger.kernel.org
TO: Tony Lindgren <tony@atomide.com>
TO: Andy Shevchenko <andy.shevchenko@gmail.com>
TO: Alexandre Belloni <alexandre.belloni@bootlin.com>
TO: Geert Uytterhoeven <geert@linux-m68k.org>
TO: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
TO: Jason Kridner <jkridner@beagleboard.org>
TO: Robert Nelson <robertcnelson@beagleboard.org>

Hi Drew,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pinctrl/devel]
[also build test WARNING on v5.11-rc7 next-20210125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Drew-Fustini/pinctrl-pinmux-Add-pinmux-select-debugfs-file/20210211-063617
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
:::::: branch date: 6 hours ago
:::::: commit date: 6 hours ago
config: x86_64-randconfig-m001-20210211 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/pinctrl/pinmux.c:764 pinmux_select() warn: possible memory leak of 'gname'

vim +/gname +764 drivers/pinctrl/pinmux.c

2744e8afb3b763 Linus Walleij 2011-05-02  675  
f878efc17642ce Drew Fustini  2021-02-10  676  #define PINMUX_MAX_NAME 64
f878efc17642ce Drew Fustini  2021-02-10  677  static ssize_t pinmux_select(struct file *file, const char __user *user_buf,
f878efc17642ce Drew Fustini  2021-02-10  678  				   size_t len, loff_t *ppos)
f878efc17642ce Drew Fustini  2021-02-10  679  {
f878efc17642ce Drew Fustini  2021-02-10  680  	struct seq_file *sfile = file->private_data;
f878efc17642ce Drew Fustini  2021-02-10  681  	struct pinctrl_dev *pctldev = sfile->private;
f878efc17642ce Drew Fustini  2021-02-10  682  	const struct pinmux_ops *pmxops = pctldev->desc->pmxops;
f878efc17642ce Drew Fustini  2021-02-10  683  	const char *const *groups;
f878efc17642ce Drew Fustini  2021-02-10  684  	char *buf, *fname, *gname;
f878efc17642ce Drew Fustini  2021-02-10  685  	unsigned int num_groups;
f878efc17642ce Drew Fustini  2021-02-10  686  	int fsel, gsel, ret;
f878efc17642ce Drew Fustini  2021-02-10  687  
f878efc17642ce Drew Fustini  2021-02-10  688  	if (len > (PINMUX_MAX_NAME * 2)) {
f878efc17642ce Drew Fustini  2021-02-10  689  		dev_err(pctldev->dev, "write too big for buffer");
f878efc17642ce Drew Fustini  2021-02-10  690  		return -EINVAL;
f878efc17642ce Drew Fustini  2021-02-10  691  	}
f878efc17642ce Drew Fustini  2021-02-10  692  
f878efc17642ce Drew Fustini  2021-02-10  693  	buf = kzalloc(PINMUX_MAX_NAME * 2, GFP_KERNEL);
f878efc17642ce Drew Fustini  2021-02-10  694  	if (!buf)
f878efc17642ce Drew Fustini  2021-02-10  695  		return -ENOMEM;
f878efc17642ce Drew Fustini  2021-02-10  696  
f878efc17642ce Drew Fustini  2021-02-10  697  	fname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
f878efc17642ce Drew Fustini  2021-02-10  698  	if (!fname) {
f878efc17642ce Drew Fustini  2021-02-10  699  		ret = -ENOMEM;
f878efc17642ce Drew Fustini  2021-02-10  700  		goto free_buf;
f878efc17642ce Drew Fustini  2021-02-10  701  	}
f878efc17642ce Drew Fustini  2021-02-10  702  
f878efc17642ce Drew Fustini  2021-02-10  703  	gname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
f878efc17642ce Drew Fustini  2021-02-10  704  	if (!buf) {
f878efc17642ce Drew Fustini  2021-02-10  705  		ret = -ENOMEM;
f878efc17642ce Drew Fustini  2021-02-10  706  		goto free_fname;
f878efc17642ce Drew Fustini  2021-02-10  707  	}
f878efc17642ce Drew Fustini  2021-02-10  708  
f878efc17642ce Drew Fustini  2021-02-10  709  	ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
f878efc17642ce Drew Fustini  2021-02-10  710  	if (ret < 0) {
f878efc17642ce Drew Fustini  2021-02-10  711  		dev_err(pctldev->dev, "failed to copy buffer from userspace");
f878efc17642ce Drew Fustini  2021-02-10  712  		goto free_gname;
f878efc17642ce Drew Fustini  2021-02-10  713  	}
f878efc17642ce Drew Fustini  2021-02-10  714  	buf[len-1] = '\0';
f878efc17642ce Drew Fustini  2021-02-10  715  
f878efc17642ce Drew Fustini  2021-02-10  716  	ret = sscanf(buf, "%s %s", fname, gname);
f878efc17642ce Drew Fustini  2021-02-10  717  	if (ret != 2) {
f878efc17642ce Drew Fustini  2021-02-10  718  		dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
f878efc17642ce Drew Fustini  2021-02-10  719  		goto free_gname;
f878efc17642ce Drew Fustini  2021-02-10  720  	}
f878efc17642ce Drew Fustini  2021-02-10  721  
f878efc17642ce Drew Fustini  2021-02-10  722  	fsel = pinmux_func_name_to_selector(pctldev, fname);
f878efc17642ce Drew Fustini  2021-02-10  723  	if (fsel < 0) {
f878efc17642ce Drew Fustini  2021-02-10  724  		dev_err(pctldev->dev, "invalid function %s in map table\n", fname);
f878efc17642ce Drew Fustini  2021-02-10  725  		ret = -EINVAL;
f878efc17642ce Drew Fustini  2021-02-10  726  		goto free_gname;
f878efc17642ce Drew Fustini  2021-02-10  727  	}
f878efc17642ce Drew Fustini  2021-02-10  728  
f878efc17642ce Drew Fustini  2021-02-10  729  	ret = pmxops->get_function_groups(pctldev, fsel, &groups, &num_groups);
f878efc17642ce Drew Fustini  2021-02-10  730  	if (ret) {
f878efc17642ce Drew Fustini  2021-02-10  731  		dev_err(pctldev->dev, "no groups for function %d (%s)", fsel, fname);
f878efc17642ce Drew Fustini  2021-02-10  732  		goto free_gname;
f878efc17642ce Drew Fustini  2021-02-10  733  
f878efc17642ce Drew Fustini  2021-02-10  734  	}
f878efc17642ce Drew Fustini  2021-02-10  735  
f878efc17642ce Drew Fustini  2021-02-10  736  	ret = match_string(groups, num_groups, gname);
f878efc17642ce Drew Fustini  2021-02-10  737  	if (ret < 0) {
f878efc17642ce Drew Fustini  2021-02-10  738  		dev_err(pctldev->dev, "invalid group %s", gname);
f878efc17642ce Drew Fustini  2021-02-10  739  		goto free_gname;
f878efc17642ce Drew Fustini  2021-02-10  740  	}
f878efc17642ce Drew Fustini  2021-02-10  741  
f878efc17642ce Drew Fustini  2021-02-10  742  	ret = pinctrl_get_group_selector(pctldev, gname);
f878efc17642ce Drew Fustini  2021-02-10  743  	if (ret < 0) {
f878efc17642ce Drew Fustini  2021-02-10  744  		dev_err(pctldev->dev, "failed to get group selectorL %s", gname);
f878efc17642ce Drew Fustini  2021-02-10  745  		goto free_gname;
f878efc17642ce Drew Fustini  2021-02-10  746  	}
f878efc17642ce Drew Fustini  2021-02-10  747  	gsel = ret;
f878efc17642ce Drew Fustini  2021-02-10  748  
f878efc17642ce Drew Fustini  2021-02-10  749  	ret = pmxops->set_mux(pctldev, fsel, gsel);
f878efc17642ce Drew Fustini  2021-02-10  750  	if (ret) {
f878efc17642ce Drew Fustini  2021-02-10  751  		dev_err(pctldev->dev, "set_mux() failed: %d", ret);
f878efc17642ce Drew Fustini  2021-02-10  752  		goto free_gname;
f878efc17642ce Drew Fustini  2021-02-10  753  	}
f878efc17642ce Drew Fustini  2021-02-10  754  
f878efc17642ce Drew Fustini  2021-02-10  755  	return len;
f878efc17642ce Drew Fustini  2021-02-10  756  
f878efc17642ce Drew Fustini  2021-02-10  757  free_gname:
f878efc17642ce Drew Fustini  2021-02-10  758  	devm_kfree(pctldev->dev, gname);
f878efc17642ce Drew Fustini  2021-02-10  759  free_fname:
f878efc17642ce Drew Fustini  2021-02-10  760  	devm_kfree(pctldev->dev, fname);
f878efc17642ce Drew Fustini  2021-02-10  761  free_buf:
f878efc17642ce Drew Fustini  2021-02-10  762  	devm_kfree(pctldev->dev, buf);
f878efc17642ce Drew Fustini  2021-02-10  763  
f878efc17642ce Drew Fustini  2021-02-10 @764  	return ret;
f878efc17642ce Drew Fustini  2021-02-10  765  }
f878efc17642ce Drew Fustini  2021-02-10  766  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 37886 bytes --]