linux-kernel - Re: [PATCH 3/4] pinctrl: disable and free setting in select

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: <5154594D.6000103@wwwdotorg.org>
Date:	Thu, 28 Mar 2013 08:53:01 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Richard Genoud <richard.genoud@...il.com>
CC:	Linus Walleij <linus.walleij@...aro.org>,
	Stephen Warren <swarren@...dia.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] pinctrl: disable and free setting in select_state
 in case of error

On 03/28/2013 04:55 AM, Richard Genoud wrote:
> 2013/3/28 Stephen Warren <swarren@...dotorg.org>:
>> On 03/25/2013 08:47 AM, Richard Genoud wrote:
>>> If enabling a pin fails in pinctrl_select_state_locked(), all the
>>> previous enabled pins have to be disabled to get back to the previous
>>> state.
>>
>>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c

>>> +     list_for_each_entry(setting2, &state->settings, node) {
>>> +             if (&setting2->node == &setting->node)
>>> +                     break;
>>> +             pinctrl_free_setting(true, setting2);
>>
>> That's clearly wrong.
>>
>> pinctrl_free_setting() is supposed to free any memory associated with
>> the setting; the storage that holds the representation of that setting.
>>
>> It's only appropriate to do that in pinctrl_put(), when actually
>> destroying the whole struct pinctrl object. If pinctrl_select() fails,
>> we don't want to destroy/invalidate the struct pinctrl content, but
>> rather keep it around in case the driver uses it again even if the face
>> of previous errors.
>>
>> In other words, what you should be doing inside this loop body is
>> exactly what the body of the first loop inside pinctrl_select_state()
>> does to "undo" any previously selected state, which is to call
>> pinmux_disable_setting() for each entry, or something similar to that.
>
> The code here tries to undo what have been done in the *second* loop
> of pinctrl_select_state().
>
> The "do" loop is:
>
> 	list_for_each_entry(setting, &state->settings, node) {
> 		switch (setting->type) {
> 		case PIN_MAP_TYPE_MUX_GROUP:
> 			ret = pinmux_enable_setting(setting);
> 			break;
> 		case PIN_MAP_TYPE_CONFIGS_PIN:
> 		case PIN_MAP_TYPE_CONFIGS_GROUP:
> 			ret = pinconf_apply_setting(setting);
> 			break;
> 		default:
> 			ret = -EINVAL;
> 			break;
> 		}
> 
> 		if (ret < 0)
> 			goto unapply_new_state;
> 	}

Right, I understand that.

> And maybe I'm wrong, but I understood that to "undo" pinmux_enable_setting,
> we call pinmux_disable_setting()

Yes.

> and pinmux_free_setting() (which is empty for now).

No. pinmux_free_setting() free's the storage for a setting. Right now,
nothing is dynamically allocated for the setting, so there's nothing to
do. However, it's still semantically wrong to try to free it at this point.

> And to undo pinconf_apply_setting() we call pinconf_free_setting()
> And that's what pinctrl_free_setting() does.

There's no way to undo the application of a setting. The only way to
undo it is to apply a new setting that over-writes it. Hopefully,
re-applying a different state would do that, but it's not guaranteed.

Again, pinconf_free_setting() is all about freeing any dynamically
allocated storage required to represent the setting itself; it's not
about (un-)programming HW.

--
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/