On Mon, Jan 25, 2021 at 05:32:39PM +0200, Andy Shevchenko wrote:

On Sat, Jan 23, 2021 at 9:29 AM Drew Fustini [off-list ref] wrote:

quoted

This RFC is a change in approach from my previous RFC patch [1]. It adds
"pinnux-select" to debugfs. Function and group on the pin control device
will be activated when 2 integers "<function-selector> <group-selector>"
are written to the file. The debugfs write operation pinmux_select()
handles this by calling ops->set_mux() with fsel and gsel.

...

quoted

RFC notes:

Please, move below to reST formatted document.

Ok, I'll make it a series and include Documentation/driver-api/pinctl.rst

...

quoted

+static ssize_t pinmux_select(struct file *file, const char __user *user_buf,
+                                  size_t cnt, loff_t *ppos)
+{
+       struct seq_file *sfile = file->private_data;
+       struct pinctrl_dev *pctldev = sfile->private;
+       const struct pinmux_ops *ops = pctldev->desc->pmxops;
+       int fsel, gsel, ret;
+       // RFC note: two integers separated by a space should never exceed 16
+       char buf[16];

quoted

+       if (*ppos != 0)
+               return -EINVAL;

But why? Do we really care about it? Moreover, you have no_llseek() below.

Good point, I'll get rid of it.

quoted

+       ret = strncpy_from_user(buf, user_buf, cnt);

Potential buffer overflow.

  cnt -> sizeof(buf)

Thanks, that is a good point.

quoted

+       if (ret < 0)
+               return ret;

quoted

+       buf[cnt] = '\0';

Not sure, shouldn't be

  buf[sizeof(buf) - 1] = '\0';

?

I'll take a look at it.

quoted

+       if (buf[cnt - 1] == '\n')
+               buf[cnt - 1] = '\0';

strstrip() ?

Neat, I wasn't aware of that one.

quoted

+       ret = sscanf(buf, "%d %d", &fsel, &gsel);
+       if (ret != 2) {

quoted

+               dev_err(pctldev->dev, "%s: sscanf() expects '<fsel> <gsel>'", __func__);

__func__ is useless, please drop it. And below as well.

Sorry, I should have removed that.

quoted

+               return -EINVAL;
+       }

quoted

+       ret = ops->set_mux(pctldev, fsel, gsel);
+       if (ret != 0) {

I thought I gave you a comment on this...

if (ret)

Yes, sorry, I should have changed that.

quoted

+               dev_err(pctldev->dev, "%s(): set_mux() failed: %d", __func__, ret);
+               return -EINVAL;
+       }
+
+       return cnt;
+}

...

quoted

        debugfs_create_file("pinmux-pins", S_IFREG | S_IRUGO,
                            devroot, pctldev, &pinmux_pins_fops);
+       debugfs_create_file("pinmux-select", 0200,
+                           devroot, pctldev, &pinmux_set_ops);

Consider to add another (prerequisite) patch to get rid of symbolic permissions.

Ok, I'll do that.

-- 
With Best Regards,
Andy Shevchenko

Thanks for the comments,
Drew