Re: [RFC PATCH v2] pinctrl: pinmux: Add pinmux-select debugfs file

[Drew Fustini <drew@xxxxxxxxxxxxxxx>]

On Mon, Jan 25, 2021 at 05:32:39PM +0200, Andy Shevchenko wrote:
> On Sat, Jan 23, 2021 at 9:29 AM Drew Fustini <drew@xxxxxxxxxxxxxxx> wrote:
> >
> > 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.
> 
> ...
> 
> > RFC notes:
> 
> Please, move below to reST formatted document.

Ok, I'll make it a series and include Documentation/driver-api/pinctl.rst
> 
> ...
> 
> > +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];
> 
> > +       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.

> 
> > +       ret = strncpy_from_user(buf, user_buf, cnt);
> 
> Potential buffer overflow.
> 
>   cnt -> sizeof(buf)
> 
Thanks, that is a good point.

> > +       if (ret < 0)
> > +               return ret;
> 
> > +       buf[cnt] = '\0';
> 
> Not sure, shouldn't be
> 
>   buf[sizeof(buf) - 1] = '\0';
> 
> ?

I'll take a look at it.
> 
> > +       if (buf[cnt - 1] == '\n')
> > +               buf[cnt - 1] = '\0';
> 
> strstrip() ?
> 

Neat, I wasn't aware of that one.

> > +       ret = sscanf(buf, "%d %d", &fsel, &gsel);
> > +       if (ret != 2) {
> 
> > +               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.
> 
> > +               return -EINVAL;
> > +       }
> 
> > +       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.
> 
> > +               dev_err(pctldev->dev, "%s(): set_mux() failed: %d", __func__, ret);
> > +               return -EINVAL;
> > +       }
> > +
> > +       return cnt;
> > +}
> 
> ...
> 
> >         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