linux-kernel - Re: [PATCH 2/2 v4] pinctrl: introduce generic pin config

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:	Thu, 1 Dec 2011 11:11:36 +0100
From:	Linus Walleij <linus.walleij@...aro.org>
To:	Stephen Warren <swarren@...dia.com>
Cc:	Linus Walleij <linus.walleij@...ricsson.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	Barry Song <21cnbao@...il.com>,
	Shawn Guo <shawn.guo@...escale.com>,
	Thomas Abraham <thomas.abraham@...aro.org>,
	Dong Aisheng <dong.aisheng@...aro.org>,
	Rajendra Nayak <rajendra.nayak@...aro.org>
Subject: Re: [PATCH 2/2 v4] pinctrl: introduce generic pin config

On Wed, Nov 30, 2011 at 8:45 PM, Stephen Warren <swarren@...dia.com> wrote:
> Linus Walleij wrote at Thursday, November 24, 2011 11:46 AM:

>> +void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
> ...
>> +     for(i = 0; i < ARRAY_SIZE(conf_items); i++) {
> ...
>> +             /* We want to check out this parameter */
>> +             config = (unsigned long) conf_items[i].param;
>
> Don't you need to use to_config_packed() here?

Yep! Fixed it.

>> +             ret = pin_config_get(pctldev, pin, &config);
>> +             /* These are legal errors */
>> +             if (ret == -EINVAL || ret == -ENOTSUPP)
>> +                     continue;
>
> Ah, I guess I see why you consider -ENOTSUPP an error that you don't want
> to print to syslog. Maybe you should call _pin_config_get() here which
> doesn't spew messages on -ENOTSUPP, but have the public pin_config_get()
> function spew errors.

I just deleted the error check for now since it's not necessary.
Better keep the code compact and simple.

>> +             if (ret) {
>> +                     seq_printf(s, "ERROR READING CONFIG SETTING %d ", i);
>> +                     continue;
>> +             }
>> +             /* Space between multiple configs */
>> +             seq_puts(s, " ");
>> +             seq_puts(s, conf_items[i].display);
>> +             /* Print unit if available */
>> +             if (conf_items[i].format && config != 0)
>> +                     seq_printf(s, " (%lu %s)", config,
>> +                                conf_items[i].format);
>
> Don't you need to use to_config_argument() here?

Yes. I'm too sloppy :-(

>> @@ -169,6 +169,8 @@ static void pinconf_dump_pin(struct pinctrl_dev *pctldev,
>>  {
>>       const struct pinconf_ops *ops = pctldev->desc->confops;
>>
>> +     /* no-op when not using generic pin config */
>> +     pinconf_generic_dump_pin(pctldev, s, pin);
>>       if (ops && ops->pin_config_dbg_show)
>>               ops->pin_config_dbg_show(pctldev, s, pin);
>>  }
>
> This is really a comment on the previous patch, but what about config
> params that only apply to groups?

Oh! I need to create a separate debugfs file for that.
Thanks for this nice catch!

>> diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
>
>> +/*
>> + * You shouldn't even be able to compile with these enums etc unless you're
>> + * using generic pin config. That is why this is defined out.
>> + */
>> +#ifdef CONFIG_GENERIC_PINCONF
>
> Hmm. I'd prefer to have drivers able to use both generic values and
> extend them with custom values. Can't we just use the top bit of the
> param value to indicate 0:standard (from the enum below) 1:custom
> (something meaningful to only the individual pinctrl driver). This
> could then trigger calling pinconf_generic_dump_pin() or not for
> example.

Hm...

That will get messy when if I refactor this stuff, add new enums
and whatever.

>> +#ifdef CONFIG_GENERIC_PINCONF
>> +     bool is_generic;
>> +#endif
>
> ... and get rid of that flag.

This is for the case wher you have both generic and non-generic
config controllers onboard a system.

Like in the generic debugfs dump function:

	if (!ops->is_generic)
		return;

If I take this out, the generic debugfs code will be used for
everything.

And then the generic sematics which you didn't like in the
previous patch:

if (ret == -EINVAL || ret == -ENOTSUPP)

Need to go back in, else the generic debugfs stuff won't
work.

I'm really not sure about that kind of extension, it feels to
me like a hard-to grasp unstable middleground between two
clean-cut solutions "all custom" or "all generic".

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/