From: Drew Fustini <drew@beagleboard.org>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
Pantelis Antoniou <pantelis.antoniou@konsulko.com>,
Jason Kridner <jkridner@beagleboard.org>,
Robert Nelson <robertcnelson@beagleboard.org>,
Linus Walleij <linus.walleij@linaro.org>,
Tony Lindgren <tony@atomide.com>,
Alexandre Belloni <alexandre.belloni@bootlin.com>,
Geert Uytterhoeven <geert@linux-m68k.org>
Subject: Re: [RFC PATCH v2] pinctrl: pinmux: Add pinmux-select debugfs file
Date: Mon, 25 Jan 2021 13:30:10 -0800 [thread overview]
Message-ID: <20210125213010.GA57277@x1> (raw)
In-Reply-To: <CAHp75VeOB7ORsq0OWKHCyGy8ouUyQSoC85-F7RCdQ2WK_5R1UA@mail.gmail.com>
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@beagleboard.org> 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
prev parent reply other threads:[~2021-01-25 23:25 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-23 6:49 [RFC PATCH v2] pinctrl: pinmux: Add pinmux-select debugfs file Drew Fustini
2021-01-25 15:32 ` Andy Shevchenko
2021-01-25 21:30 ` Drew Fustini [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210125213010.GA57277@x1 \
--to=drew@beagleboard.org \
--cc=alexandre.belloni@bootlin.com \
--cc=andy.shevchenko@gmail.com \
--cc=geert@linux-m68k.org \
--cc=jkridner@beagleboard.org \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=pantelis.antoniou@konsulko.com \
--cc=robertcnelson@beagleboard.org \
--cc=tony@atomide.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).