linux-kernel - Re: [PATCH v4 1/2] pinctrl: single: parse #pinctrl-cells = 2

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]

Message-ID: <20200913194233.GA1955808@x1>
Date:   Sun, 13 Sep 2020 21:42:33 +0200
From:   Drew Fustini <drew@...gleboard.org>
To:     Trent Piepho <tpiepho@...il.com>
Cc:     linux-arm-kernel@...ts.infradead.org,
        Tony Lindgren <tony@...mide.com>,
        Rob Herring <robh+dt@...nel.org>, linux-omap@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
        Linus Walleij <linus.walleij@...aro.org>,
        Haojian Zhuang <haojian.zhuang@...aro.org>,
        devicetree@...r.kernel.org, bcousson@...libre.com,
        Jason Kridner <jkridner@...gleboard.org>,
        Robert Nelson <robertcnelson@...il.com>
Subject: Re: [PATCH v4 1/2] pinctrl: single: parse #pinctrl-cells = 2

On Tue, Sep 08, 2020 at 04:52:58PM -0700, Trent Piepho wrote:
> On Tuesday, June 30, 2020 6:33:19 PM PDT Drew Fustini wrote:
> > If "pinctrl-single,pins" has 3 arguments (offset, conf, mux), then
> > pcs_parse_one_pinctrl_entry() does an OR operation on conf and mux to
> > get the value to store in the register.
> 
> 
> > -		vals[found].val = pinctrl_spec.args[1];
> > +
> > +		switch (pinctrl_spec.args_count) {
> > +		case 2:
> > +			vals[found].val = pinctrl_spec.args[1];
> > +			break;
> > +		case 3:
> > +			vals[found].val = (pinctrl_spec.args[1] | 
> pinctrl_spec.args[2]);
> > +			break;
> > +		}
> > 
> >  		dev_dbg(pcs->dev, "%pOFn index: 0x%x value: 0x%x\n",
> >  			pinctrl_spec.np, offset, 
> pinctrl_spec.args[1]);
> 
> If #pinctrl-cells value is greater than 2, nothing will set vals[found].val to 
> anything other than zero (from when it's calloc'ed) and the pinctrl will 
> silently be programmed to zero.

If #pinctrl-cells is 3, then it will be:

	vals[found].val = (pinctrl_spec.args[1] | pinctrl_spec.args[2]);

Do you mean if #pinctrl-cells is great than 3 then it will just have a
default value of zero?

That does appear to be the case and is probably not the behavior we
want.  Thank you for pointing this out.  Earlier, there is a check to
make sure there are at least 2 arguments:

	if (pinctrl_spec.args_count < 2) {
		dev_err(pcs->dev, "invalid args_count for spec: %i\n",
			pinctrl_spec.args_count); 
		break;
	}

I'll submit a patch where the upper bound is also checked:

	if (pinctrl_spec.args_count < 2 || pinctrl_spec.args_count > 3) {
		dev_err(pcs->dev, "invalid args_count for spec: %i\n",
			pinctrl_spec.args_count); 
		break;
	}

> The debug printout was not change to print vals[found].val, so it will 
> continue to print the value of the 2nd cell.

Thank you for pointing this out.  Yes, this is an oversight and I will
submit a patch.

> The result is that a #pinctrl-cells of 3 will produce no warning or error, 
> program the pinctrl to zero, whilst at the same time emit debug log messages 
> that it is programming the expected values.
>
> The device tree documentation still states that #pinctrl-cells must be 1 when 
> using pinctrl-single,pins.  This new special case of ORing two values is not 
> documented.

This is a good point, too.  I will make a patch to update the
documentation.


-Drew