2021-02-20 20:30:30

by Drew Fustini

[permalink] [raw]
Subject: [PATCH v8 0/3] pinctrl: pinmux: Add pinmux-select debugfs file

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

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

The final patch in this series documents the debugfs files for pinctrl.

Notes for PATCH v8:
- add 'Reviewed-by:' from Geert Uytterhoeven for pinmux-select patch
- add 'Tested-by:' from Geert Uytterhoeven for pinmux-select patch
- change pinmux-select format to '<group-name function-name>' based on
feedback from Geert
- rephrase parts of documentation per Geert's comments

Notes for PATCH v7:
- add 'Reviewed-by:' from Andy Shevchenko for pinmux-select patch
- add 'Reviewed-by:' from Andy Shevchenko for documentation patch
- add 'Reviewed-by:' from Tony Lindgren to all patches
- change order of '#include <linux/ctype.h>' per Andy's suggestion
- change PINMUX_SELECT_MAX back to 128 as I had accidentally changed it
to 50 and Andy pointed this out.
- grammer fixes as suggested by Andy
- rework assignment of fsel and ret from pinmux_func_name_to_selector()
- rework assignment of gsel and ret from pinctrl_get_group_selector()

Notes for PATCH v6:
- add 'Suggested-by:' for Joe Perches to octal permissions patch
- add 'Reviewed-by:' from Andy and Geert to octal permissions patch
- reword example in the pinmux-select patch per Andy's advice
- indent the example output per Andy's advice
- remove usage error messages as Andy advised it is too verbose
- return -ENOMEM when write is too big for the input buffer per Andy's advice
- handle whitespace before, in between, and after the function name and
group name as suggested by Andy
- rename free_buf to exit_free_buf per Andy's advice
- add documentation patch to series which documents the debugfs files
for the pinctrl subsystem including the new pinmux-select file

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/[email protected]/
[2] https://lore.kernel.org/linux-gpio/20210126050817.GA187797@x1/
[3] https://lore.kernel.org/linux-gpio/[email protected]/

Drew Fustini (3):
pinctrl: use to octal permissions for debugfs files
pinctrl: pinmux: Add pinmux-select debugfs file
docs/pinctrl: document debugfs files

Documentation/driver-api/pinctl.rst | 37 ++++++++++
drivers/pinctrl/core.c | 12 ++--
drivers/pinctrl/pinconf.c | 4 +-
drivers/pinctrl/pinmux.c | 106 +++++++++++++++++++++++++++-
4 files changed, 149 insertions(+), 10 deletions(-)

--
2.25.1


2021-02-20 20:31:25

by Drew Fustini

[permalink] [raw]
Subject: [PATCH v8 1/3] pinctrl: use to octal permissions for debugfs files

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: Joe Perches <[email protected]>
Suggested-by: Andy Shevchenko <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Tony Lindgren <[email protected]>
Signed-off-by: Drew Fustini <[email protected]>
---
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

2021-02-20 20:33:42

by Drew Fustini

[permalink] [raw]
Subject: [PATCH v8 2/3] pinctrl: pinmux: Add pinmux-select debugfs file

Add "pinmux-select" to debugfs which will activate a pin function for a
given pin group:

echo "<group-name function-name>" > pinmux-select

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 on group pinmux-i2c1-pins:

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

Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Tony Lindgren <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Tested-by: Geert Uytterhoeven <[email protected]>
Signed-off-by: Drew Fustini <[email protected]>
---
drivers/pinctrl/pinmux.c | 102 +++++++++++++++++++++++++++++++++++++++
1 file changed, 102 insertions(+)

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index c651b2db0925..f4abfaecfc5c 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -12,6 +12,7 @@
*/
#define pr_fmt(fmt) "pinmux core: " fmt

+#include <linux/ctype.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/init.h>
@@ -673,6 +674,105 @@ 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)
+{
+ 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, *gname, *fname;
+ unsigned int num_groups;
+ int fsel, gsel, ret;
+
+ if (len > PINMUX_SELECT_MAX)
+ return -ENOMEM;
+
+ buf = kzalloc(PINMUX_SELECT_MAX, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = strncpy_from_user(buf, user_buf, PINMUX_SELECT_MAX);
+ if (ret < 0)
+ goto exit_free_buf;
+ buf[len-1] = '\0';
+
+ /* remove leading and trailing spaces of input buffer */
+ gname = strstrip(buf);
+ if (*gname == '\0') {
+ ret = -EINVAL;
+ goto exit_free_buf;
+ }
+
+ /* find a separator which is a spacelike character */
+ for (fname = gname; !isspace(*fname); fname++) {
+ if (*fname == '\0') {
+ ret = -EINVAL;
+ goto exit_free_buf;
+ }
+ }
+ *fname = '\0';
+
+ /* drop extra spaces between function and group names */
+ fname = skip_spaces(fname + 1);
+ if (*fname == '\0') {
+ ret = -EINVAL;
+ goto exit_free_buf;
+ }
+
+ ret = pinmux_func_name_to_selector(pctldev, fname);
+ if (ret < 0) {
+ dev_err(pctldev->dev, "invalid function %s in map table\n", fname);
+ goto exit_free_buf;
+ }
+ fsel = ret;
+
+ 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 exit_free_buf;
+ }
+
+ ret = match_string(groups, num_groups, gname);
+ if (ret < 0) {
+ dev_err(pctldev->dev, "invalid group %s", gname);
+ goto exit_free_buf;
+ }
+
+ ret = pinctrl_get_group_selector(pctldev, gname);
+ if (ret < 0) {
+ dev_err(pctldev->dev, "failed to get group selector for %s", gname);
+ goto exit_free_buf;
+ }
+ gsel = ret;
+
+ ret = pmxops->set_mux(pctldev, fsel, gsel);
+ if (ret) {
+ dev_err(pctldev->dev, "set_mux() failed: %d", ret);
+ goto exit_free_buf;
+ }
+ ret = len;
+
+exit_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 +780,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

2021-02-20 20:34:09

by Drew Fustini

[permalink] [raw]
Subject: [PATCH v8 3/3] docs/pinctrl: document debugfs files

Document debugfs directories and files created for pinctrl subsystem.

Suggested-by: Andy Shevchenko <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Tony Lindgren <[email protected]>
Signed-off-by: Drew Fustini <[email protected]>
---
Documentation/driver-api/pinctl.rst | 37 +++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)

diff --git a/Documentation/driver-api/pinctl.rst b/Documentation/driver-api/pinctl.rst
index 3d2deaf48841..c905b273e833 100644
--- a/Documentation/driver-api/pinctl.rst
+++ b/Documentation/driver-api/pinctl.rst
@@ -1428,3 +1428,40 @@ on the pins defined by group B::
The above has to be done from process context. The reservation of the pins
will be done when the state is activated, so in effect one specific pin
can be used by different functions at different times on a running system.
+
+
+Debugfs files
+=============
+These files are created in ``/sys/kernel/debug/pinctrl``:
+
+- ``pinctrl-devices``: prints each pin controller device along with columns to
+ indicate support for pinmux and pinconf
+
+- ``pinctrl-handles``: prints each configured pin controller handle and the
+ corresponding pinmux maps
+
+- ``pinctrl-maps``: print all pinctrl maps
+
+A sub-directory is created inside of ``/sys/kernel/debug/pinctrl`` for each pin
+controller device containing these files:
+
+- ``pins``: prints a line for each pin registered on the pin controller. The
+ pinctrl driver may add additional information such as register contents.
+
+- ``gpio-ranges``: print ranges that map gpio lines to pins on the controller
+
+- ``pingroups``: print all pin groups registered on the pin controller
+
+- ``pinconf-pins``: print pin config settings for each pin
+
+- ``pinconf-groups``: print pin config settings per pin group
+
+- ``pinmux-functions``: print each pin function along with the pin groups that
+ map to the pin function
+
+- ``pinmux-pins``: iterate through all pins and print mux owner, gpio owner
+ and if the pin is a hog
+
+- ``pinmux-select``: write to this file to activate a pin function for a group::
+
+ echo "<group-name function-name>" > pinmux-select
--
2.25.1

2021-02-26 22:24:05

by Drew Fustini

[permalink] [raw]
Subject: Re: [PATCH v8 0/3] pinctrl: pinmux: Add pinmux-select debugfs file

On Sat, Feb 20, 2021 at 12:27:47PM -0800, Drew Fustini wrote:
> This series first converts the debugfs files in the pinctrl subsystem to
> octal permissions and then adds a new debugfs file "pinmux-select".
>
> Group name and function name can be written to "pinmux-select" which
> will cause the pin function for the specified group to be activated on
> the pin controller.
>
> The final patch in this series documents the debugfs files for pinctrl.
>
> Notes for PATCH v8:
> - add 'Reviewed-by:' from Geert Uytterhoeven for pinmux-select patch
> - add 'Tested-by:' from Geert Uytterhoeven for pinmux-select patch
> - change pinmux-select format to '<group-name function-name>' based on
> feedback from Geert
> - rephrase parts of documentation per Geert's comments
>
> Notes for PATCH v7:
> - add 'Reviewed-by:' from Andy Shevchenko for pinmux-select patch
> - add 'Reviewed-by:' from Andy Shevchenko for documentation patch
> - add 'Reviewed-by:' from Tony Lindgren to all patches
> - change order of '#include <linux/ctype.h>' per Andy's suggestion
> - change PINMUX_SELECT_MAX back to 128 as I had accidentally changed it
> to 50 and Andy pointed this out.
> - grammer fixes as suggested by Andy
> - rework assignment of fsel and ret from pinmux_func_name_to_selector()
> - rework assignment of gsel and ret from pinctrl_get_group_selector()
>
> Notes for PATCH v6:
> - add 'Suggested-by:' for Joe Perches to octal permissions patch
> - add 'Reviewed-by:' from Andy and Geert to octal permissions patch
> - reword example in the pinmux-select patch per Andy's advice
> - indent the example output per Andy's advice
> - remove usage error messages as Andy advised it is too verbose
> - return -ENOMEM when write is too big for the input buffer per Andy's advice
> - handle whitespace before, in between, and after the function name and
> group name as suggested by Andy
> - rename free_buf to exit_free_buf per Andy's advice
> - add documentation patch to series which documents the debugfs files
> for the pinctrl subsystem including the new pinmux-select file
>
> 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/[email protected]/
> [2] https://lore.kernel.org/linux-gpio/20210126050817.GA187797@x1/
> [3] https://lore.kernel.org/linux-gpio/[email protected]/
>
> Drew Fustini (3):
> pinctrl: use to octal permissions for debugfs files
> pinctrl: pinmux: Add pinmux-select debugfs file
> docs/pinctrl: document debugfs files
>
> Documentation/driver-api/pinctl.rst | 37 ++++++++++
> drivers/pinctrl/core.c | 12 ++--
> drivers/pinctrl/pinconf.c | 4 +-
> drivers/pinctrl/pinmux.c | 106 +++++++++++++++++++++++++++-
> 4 files changed, 149 insertions(+), 10 deletions(-)
>
> --
> 2.25.1
>

Does any have any additional feedback for any of the patches in this
series?

Thank you!
DRew