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

[PATCH v5 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 v5:
- convert permissions from symbolic to octal for debugfs_create_file()
  calls in core.c that Joe Perches pointed out I had missed
- Linus W: please let me know if I should break this series apart as you
  already applied an earlier version of octal conversion patch today [1]
- switch from sscanf() to just pointing to function name and group name
  inside of the buffer. This also avoids having to allocate additional
  buffers for fname and gname. Geert and Andy highlighted this security
  issue and Andy suggested code to use instead of sscanf().
- switch from devm_kfree() to kfree() after Dan Carpenter warned me
- remove .read from pinmux_select_ops per Geert since it is write only
- add usage format to error when unable find fname or gname in buffer

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
- 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    |  12 ++---
 drivers/pinctrl/pinconf.c |   4 +-
 drivers/pinctrl/pinmux.c  | 103 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 109 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH v5 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    | 12 ++++++------
 drivers/pinctrl/pinconf.c |  4 ++--
 drivers/pinctrl/pinmux.c  |  4 ++--
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 3663d87f51a0..07458742bc0f 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1888,11 +1888,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", 0444,
 			    device_root, pctldev, &pinctrl_pins_fops);
-	debugfs_create_file("pingroups", S_IFREG | S_IRUGO,
+	debugfs_create_file("pingroups", 0444,
 			    device_root, pctldev, &pinctrl_groups_fops);
-	debugfs_create_file("gpio-ranges", S_IFREG | S_IRUGO,
+	debugfs_create_file("gpio-ranges", 0444,
 			    device_root, pctldev, &pinctrl_gpioranges_fops);
 	if (pctldev->desc->pmxops)
 		pinmux_init_device_debugfs(device_root, pctldev);
@@ -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

[PATCH v5 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 | 99 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index c651b2db0925..c16a6167ac3a 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -673,6 +673,103 @@ void pinmux_show_setting(struct seq_file *s,
 DEFINE_SHOW_ATTRIBUTE(pinmux_functions);
 DEFINE_SHOW_ATTRIBUTE(pinmux_pins);
 
+#define PINMUX_SELECT_MAX 128
+static ssize_t pinmux_select(struct file *file, const char __user *user_buf,
+				   size_t len, loff_t *ppos)
+{
+	const char *usage =
+		"usage: echo '<function-name> <group-name>' > pinmux-select";
+	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_SELECT_MAX) {
+		dev_err(pctldev->dev, "write too big for buffer");
+		return -EINVAL;
+	}
+
+	buf = kzalloc(PINMUX_SELECT_MAX, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = strncpy_from_user(buf, user_buf, PINMUX_SELECT_MAX);
+	if (ret < 0) {
+		dev_err(pctldev->dev, "failed to copy buffer from userspace");
+		goto free_buf;
+	}
+	buf[len-1] = '\0';
+
+	fname = strstrip(buf);
+	if (*fname == '\0') {
+		dev_err(pctldev->dev, usage);
+		ret = -EINVAL;
+		goto free_buf;
+	}
+
+	gname = strchr(fname, ' ');
+	if (!gname) {
+		dev_err(pctldev->dev, usage);
+		ret = -EINVAL;
+		goto free_buf;
+	}
+	*gname++ = '\0';
+
+	fsel = pinmux_func_name_to_selector(pctldev, fname);
+	if (fsel < 0) {
+		dev_err(pctldev->dev, "invalid function %s in map table\n", fname);
+		ret = fsel;
+		goto free_buf;
+	}
+
+	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_buf;
+	}
+
+	ret = match_string(groups, num_groups, gname);
+	if (ret < 0) {
+		dev_err(pctldev->dev, "invalid group %s", gname);
+		goto free_buf;
+	}
+
+	gsel = pinctrl_get_group_selector(pctldev, gname);
+	if (gsel < 0) {
+		dev_err(pctldev->dev, "failed to get group selector for %s", gname);
+		ret = gsel;
+		goto free_buf;
+	}
+
+	ret = pmxops->set_mux(pctldev, fsel, gsel);
+	if (ret) {
+		dev_err(pctldev->dev, "set_mux() failed: %d", ret);
+		goto free_buf;
+	}
+	ret = len;
+
+free_buf:
+	kfree(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,
+	.write = pinmux_select,
+	.llseek = no_llseek,
+	.release = single_release,
+};
+
 void pinmux_init_device_debugfs(struct dentry *devroot,
 			 struct pinctrl_dev *pctldev)
 {
@@ -680,6 +777,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 v5 1/2] pinctrl: use to octal permissions for debugfs files

[Andy Shevchenko]

On Sat, Feb 13, 2021 at 12:30 AM Drew Fustini <drew@beagleboard.org> 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

I guess it also needs Suggested-by: Joe (IIRC he proposed to convert the rest).
Nevertheless,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Thanks!


> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Drew Fustini <drew@beagleboard.org>
> ---
>  drivers/pinctrl/core.c    | 12 ++++++------
>  drivers/pinctrl/pinconf.c |  4 ++--
>  drivers/pinctrl/pinmux.c  |  4 ++--
>  3 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index 3663d87f51a0..07458742bc0f 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -1888,11 +1888,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", 0444,
>                             device_root, pctldev, &pinctrl_pins_fops);
> -       debugfs_create_file("pingroups", S_IFREG | S_IRUGO,
> +       debugfs_create_file("pingroups", 0444,
>                             device_root, pctldev, &pinctrl_groups_fops);
> -       debugfs_create_file("gpio-ranges", S_IFREG | S_IRUGO,
> +       debugfs_create_file("gpio-ranges", 0444,
>                             device_root, pctldev, &pinctrl_gpioranges_fops);
>         if (pctldev->desc->pmxops)
>                 pinmux_init_device_debugfs(device_root, pctldev);
> @@ -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
>


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sat, Feb 13, 2021 at 12:30 AM 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

The non-standard way of showing parameters, I would write that as
 "<function-name> <group-name>".

> 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 ]

Format this...

> To activate function pinmux-i2c1 and group pinmux-i2c1-pins:
>
> echo "pinmux-i2c1 pinmux-i2c1-pins" > pinmux-select

...and this with two leading spaces (for example) to make sure that
people will understand that these lines are part of the example.

...

>  drivers/pinctrl/pinmux.c | 99 ++++++++++++++++++++++++++++++++++++++++

Still needs a documentation update.

...

> +       const char *usage =
> +               "usage: echo '<function-name> <group-name>' > pinmux-select";

This is quite unusual to have in the kernel. Just return an error
code, everything else should be simply documented.

...

> +       if (len > PINMUX_SELECT_MAX) {

> +               dev_err(pctldev->dev, "write too big for buffer");

Noisy, the user will get an error code and interpret it properly.
So, please drop them all. Otherwise it would be quite easy to exhaust
kernel buffer with this noise and lost the important messages.

> +               return -EINVAL;

To achieve the above, this rather should be -ENOMEM.

> +       }

...

> +       gname = strchr(fname, ' ');
> +       if (!gname) {
> +               dev_err(pctldev->dev, usage);
> +               ret = -EINVAL;
> +               goto free_buf;
> +       }
> +       *gname++ = '\0';

I was thinking about this again and I guess we may allow any amount of
spaces in between and any kind of  (like newline or TAB).
So, taking above into consideration the code may look like this:

/* Take the input and remove leading and trailing spaces of entire buffer */
fname = strstrip(buf);
/* Find a separator, i.e. a space character */
for (gname = fname; !isspace(gname); gname++)
  if (*gname == '\0')
    return -EINVAL;
/* Replace separator with %NUL to terminate first word */
*gname = '\0';
/* Drop space characters between first and second words */
gname = skip_spaces(gname + 1);
if (*gname == '\0')
  return -EINVAL;

But please double check the logic.

...

> +free_buf:

exit_free_buf:

> +       kfree(buf);
> +
> +       return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Drew Fustini]

On Mon, Feb 15, 2021 at 09:04:20PM +0200, Andy Shevchenko wrote:
> On Sat, Feb 13, 2021 at 12:30 AM 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
> 
> The non-standard way of showing parameters, I would write that as
>  "<function-name> <group-name>".

Sorry for your comments, but I don't understand what you mean by this
one.  I think we wrote ""<function-name> <group-name>" the same way, no?

> 
> > 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 ]
> 
> Format this...
> 
> > To activate function pinmux-i2c1 and group pinmux-i2c1-pins:
> >
> > echo "pinmux-i2c1 pinmux-i2c1-pins" > pinmux-select
> 
> ...and this with two leading spaces (for example) to make sure that
> people will understand that these lines are part of the example.

Ok, thanks.

> 
> ...
> 
> >  drivers/pinctrl/pinmux.c | 99 ++++++++++++++++++++++++++++++++++++++++
> 
> Still needs a documentation update.

There is no documentation for any of the existing pinctrl debugfs files.
I was planning to do this as part of a seperate patch, but I can make it
part of this series instead.

> 
> ...
> 
> > +       const char *usage =
> > +               "usage: echo '<function-name> <group-name>' > pinmux-select";
> 
> This is quite unusual to have in the kernel. Just return an error
> code, everything else should be simply documented.
> 
> ...
> 
> > +       if (len > PINMUX_SELECT_MAX) {
> 
> > +               dev_err(pctldev->dev, "write too big for buffer");
> 
> Noisy, the user will get an error code and interpret it properly.
> So, please drop them all. Otherwise it would be quite easy to exhaust
> kernel buffer with this noise and lost the important messages.
> 
> > +               return -EINVAL;
> 
> To achieve the above, this rather should be -ENOMEM.
> 
> > +       }

Thanks, I will remove the usage message and change the return value.

> 
> ...
> 
> > +       gname = strchr(fname, ' ');
> > +       if (!gname) {
> > +               dev_err(pctldev->dev, usage);
> > +               ret = -EINVAL;
> > +               goto free_buf;
> > +       }
> > +       *gname++ = '\0';
> 
> I was thinking about this again and I guess we may allow any amount of
> spaces in between and any kind of  (like newline or TAB).
> So, taking above into consideration the code may look like this:
> 
> /* Take the input and remove leading and trailing spaces of entire buffer */
> fname = strstrip(buf);
> /* Find a separator, i.e. a space character */
> for (gname = fname; !isspace(gname); gname++)
>   if (*gname == '\0')
>     return -EINVAL;
> /* Replace separator with %NUL to terminate first word */
> *gname = '\0';
> /* Drop space characters between first and second words */
> gname = skip_spaces(gname + 1);
> if (*gname == '\0')
>   return -EINVAL;
> 
> But please double check the logic.
> 
> ...


Thanks for the example code.  I'll test it out.


> 
> > +free_buf:
> 
> exit_free_buf:
> 

Ok, thanks.

> > +       kfree(buf);
> > +
> > +       return ret;
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko

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

[Geert Uytterhoeven]

On Fri, Feb 12, 2021 at 11:30 PM Drew Fustini <drew@beagleboard.org> 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>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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 v5 1/2] pinctrl: use to octal permissions for debugfs files

[Linus Walleij]

On Fri, Feb 12, 2021 at 11:30 PM Drew Fustini <drew@beagleboard.org> 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>

Patch applied, thanks for fixing this!

Yours,
Linus Walleij

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

[Andy Shevchenko]

On Tue, Mar 2, 2021 at 10:36 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Feb 12, 2021 at 11:30 PM Drew Fustini <drew@beagleboard.org> 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>
>
> Patch applied, thanks for fixing this!

I guess we are at v9 of this.

-- 
With Best Regards,
Andy Shevchenko

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

[Linus Walleij]

On Tue, Mar 2, 2021 at 11:23 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Mar 2, 2021 at 10:36 AM Linus Walleij <linus.walleij@linaro.org> wrote:

> > Patch applied, thanks for fixing this!
>
> I guess we are at v9 of this.

Yeah I took it out again waiting for the waters to settle.

Yours,
Linus Walleij

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

[Drew Fustini]

On Tue, Mar 02, 2021 at 05:22:37PM +0100, Linus Walleij wrote:
> On Tue, Mar 2, 2021 at 11:23 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Tue, Mar 2, 2021 at 10:36 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> > > Patch applied, thanks for fixing this!
> >
> > I guess we are at v9 of this.
> 
> Yeah I took it out again waiting for the waters to settle.
> 
> Yours,
> Linus Walleij

Sorry for the confusion.

"[PATCH v8 0/3] pinctrl: pinmux: Add pinmux-select debugfs file" [1]
sent Feb. 20th was in my opinion ready to merge.

However, it occured to me yesterday since there had been no replies to
that thread, then it might be a good idea to add a 4th patch to rename
pinctl.rst to pin-control.rst. I sent that yesterday as v9 [2] but I am
fine with that being dropped as renaming pinctl.rst is unrelated to my 
actual goal of adding pinmux-select to debugfs.

thanks,
drew

[1] https://lore.kernel.org/linux-gpio/20210220202750.117421-1-drew@beagleboard.org/
[2] https://lore.kernel.org/lkml/20210302053059.1049035-1-drew@beagleboard.org/