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

[Drew Fustini <drew@xxxxxxxxxxxxxxx>]

On Fri, Jan 22, 2021 at 11:50:52AM +0200, Andy Shevchenko wrote:
> On Fri, Jan 22, 2021 at 1:26 AM Drew Fustini <drew@xxxxxxxxxxxxxxx> wrote:
> > On Thu, Jan 21, 2021 at 01:18:58PM +0200, Andy Shevchenko wrote:
> > > On Thu, Jan 21, 2021 at 7:18 AM Drew Fustini <drew@xxxxxxxxxxxxxxx> wrote:
> 
> ...
> 
> > > > RFC question: should pinmux-set take function name and group name
> > > > instead of the selector numbers?
> > >
> > > I would prefer names and integers (but from user p.o.v. names are
> > > easier to understand, while numbers are good for scripting).
> >
> > I don't actually see any example of looking up the function name in the
> > existing pinctrl code. There is pin_function_tree in struct pinctrl_dev.
> > pinmux_generic_get_function_name() does radix_tree_lookup() with the
> > selector integer as the key, but there is no corresponding "get function
> > selector by name" function.
> >
> > I think I would need to go through all the nodes in the radix tree to
> > find the name that matches. Although, I am just learning now about the
> > radix implementation in Linux so there might be a simpler way that I am
> > missing.
> 
> I probably have to revive my work towards gluing ACPI with pin control
> where AFAIR I have created some kind of radix / rbtree for something
> (not sure it's exactly what you need here, so consider this just as a
> side note).
> 
> ...
> 
> > > The following is better to include in documentation and remove from
> > > the commit message.
> 
> > > Shorter is better, what about simply
> > >
> > > # cat /sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single/pinmux-functions
> > > ?
> > >
> > > Btw  in reST format you may create a nice citation of this. And yes,
> > > this should also go to the documentation.
> >
> > Good point, I'll shorten the example lines in v2.
> 
> Even better to tell that we operate on the level of mount point of
> debugfs and use
> 
>  # cat pinctrl/44e10800.pinmux-pinctrl-single/pinmux-functions
> 
> > > This and above is still part of documentation, and not a commit message thingy.
> >
> > Is something I should add to Documentation/driver-api/pinctl.rst in a
> > seperate patch?
> 
> Not sure, I think more about as a part of this very path you change
> code and documentation. But usually it's a preference of the certain
> subsystem.
> 
> ...
> 
> > > > +       if (cnt == 0)
> > > > +               return 0;
> > >
> > > Has it ever happened here?
> >
> > Good point, I guess there is no reason for userspace to write 0 bytes.
> 
> My point is that this check is done somewhere in the guts of kernfs.
> When in doubt I recommend to look around in the kernel and check most
> recent code with similar code pieces.
> 
> ...
> 
> > > > +       buf = memdup_user_nul(user_buf, cnt);
> > > > +       if (IS_ERR(buf))
> > > > +               return PTR_ERR(buf);
> > > > +
> > > > +       if (buf[cnt - 1] == '\n')
> > > > +               buf[cnt - 1] = '\0';
> > >
> > > Shouldn't you rather use strndup_from_user() (or how is it called?)
> 
> Any comments?

Sorry, I had meant to comment on that.  I tried strndup_user() but had
difficulty in using it as 'length > n' was always true and thus returned
an error.  There are not that many users of it either.

I've switched to this based on how armada_debugfs_crtc_reg_write() in 
armada_debugfs.c handles the user writing multiple integers:

        char buf[16];

        if (cnt > sizeof(buf) - 1)    
                cnt = sizeof(buf) - 1;
        ret = strncpy_from_user(buf, user_buf, cnt);
        if (ret < 0)
                return ret;
        buf[cnt] = '\0';
        if (buf[cnt - 1] == '\n')
                buf[cnt - 1] = '\0';  
	// the parse with sscanf()

I choose 16 for buf as two integer numbers seperated by a space should
never be more than 16.  I suppose the downside is that it is allocated
on the stack but it is a small buffer.

I'll post v2 so it can be evaluated in the full patch context.

> 
> ...
> 
> > > Can be simply
> > >
> > >  err_freebuf:
> > >         kfree(buf);
> > >         return err ?: cnt;
> >
> > Thanks, I didn't really like the duplication but was having trouble
> > thinking of a cleaner way to write it.  That is good to know it is ok to
> > use the ternary operator in a return statement.
> 
> Again, depends on certain subsystem maintainer's preferences.
> 
> 
> > > > +       debugfs_create_file("pinmux-set", S_IFREG | S_IWUSR,
> > > > +                           devroot, pctldev, &pinmux_set_ops);
> 
> One more thing, as a preparatory patch please move from S_I* to plain
> octal numbers as it's preferable.
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko