linux-kernel - Re: [RFC PATCH] pinctrl: add helper to expose pinctrl state in debugfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 15 Dec 2020 21:36:33 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Drew Fustini <drew@...gleboard.org>
Cc:     "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Tony Lindgren <tony@...mide.com>
Subject: Re: [RFC PATCH] pinctrl: add helper to expose pinctrl state in debugfs

On Mon, Dec 14, 2020 at 11:44 PM Drew Fustini <drew@...gleboard.org> wrote:
> On Mon, Dec 14, 2020 at 07:55:12PM +0200, Andy Shevchenko wrote:
> > On Sat, Dec 12, 2020 at 1:43 AM Drew Fustini <drew@...gleboard.org> wrote:
> > > On Fri, Dec 11, 2020 at 11:15:21PM +0200, Andy Shevchenko wrote:
> > > > On Fri, Dec 11, 2020 at 1:54 PM Drew Fustini <drew@...gleboard.org> wrote:

...

> > > > But I'm wondering, why it requires this kind of thing and can't be
> > > > simply always part of the kernel based on configuration option?
> > >
> > > Do you mean not having a new CONFIG option for this driver and just have
> > > it be enabled by CONFIG_PINCTRL?
> >
> > No, configuration option stays, but no compatible strings no nothing
> > like that. Just probed always when loaded.
>
> I first started down the route of implementing this inside of
> pinctrl-single.  I found it didn't work because devm_pinctrl_get() would
> fail.  I think was because it was happening too early for pinctrl to be
> ready.
>
> I do think it seems awkward to have to add this to dts and have the
> driver get probed for each entry:
>
>         P1_04_pinmux {
>                 compatible = "pinctrl,state-helper";
>                 status = "okay";
>                 pinctrl-names = "default", "gpio", "gpio_pu", "gpio_pd", "gpio_input", "pruout", "pruin";
>                 pinctrl-0 = <&P1_04_default_pin>;
>                 pinctrl-1 = <&P1_04_gpio_pin>;
>                 pinctrl-2 = <&P1_04_gpio_pu_pin>;
>                 pinctrl-3 = <&P1_04_gpio_pd_pin>;
>                 pinctrl-4 = <&P1_04_gpio_input_pin>;
>                 pinctrl-5 = <&P1_04_pruout_pin>;
>                 pinctrl-6 = <&P1_04_pruin_pin>;
>         };
>
> But I am having a hard time figuring out another way of doing it.

I'm not a DT expert and I have no clue why you need all this. To me it
looks over engineered to engage DT for debugging things. OTOH, you may
add a property to allow debug mux (but it prevent ACPI enabled
platforms to utilize this).

...

> Any ideas as to what would trigger the probe() if there was not a match
> on a compatible like "pinctrl,state-helper"?
>
> > Actually not even sure we want to have it as a module.
>
> And have just be a part of one of the existing pinctrl files like core.c?

Separate file, but in conjunction with core.c and pinmux and so on.

...

> > > > Shouldn't it be rather a part of a certain pin control folder:
> > > > debug/pinctrl/.../mux/...
> > > > ?
> > >
> > > Yes, I think that would make sense, but I was struggling to figure out
> > > how to do that. pinctrl_init_debugfs() in pinctrl/core.c does create the
> > > "pinctrl" directory, but I could not figure out how to use this as the
> > > parent dir when calling debugfs_create_dir() in this driver's probe().
> > >
> > > I thought there might be a way in debugfs API to use existing directory
> > > path as a parent but I couldn't figure anything like that. I would
> > > appreciate any advice.
> >
> > If the option is boolean from the beginning then you just call it from
> > the corresponding pin control instantiation chain.
>
> Sorry, I am not sure I understand what you mean here.  What does
> "option" mean in this context?  I don't think there is any value that is
> boolean invovled.  The pinctrl states are strings.

config PINMUX_DEBUG
 bool "..."
 depends on PINMUX



>
> With regards to parent directory, I did discover there is
> debugfs_lookup(), so I can get the dentry for "pinctrl" and create new
> subdirectory inside of it.  This is the structure now:
>
> /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_35_pinmux/state
> /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_34_pinmux/state
> /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_33_pinmux/state
> /sys/kernel/debug/pinctrl/pinctrl_state/ocp:P2_32_pinmux/state
> etc..


--
With Best Regards,
Andy Shevchenko