2013/3/28 Stephen Warren <swar...@wwwdotorg.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
>
>> @@ -910,13 +910,35 @@ static int pinctrl_select_state_locked(struct pinctrl
>> *p,
>
>> if (ret < 0) {
>> - /* FIXME: Difficult to return to prev state */
>> - return ret;
>> + goto unapply_new_state;
>> }
>
> Those { } should really be removed there, since there's only 1 line left
> in the block.
yes, I missed that one.
>> +unapply_new_state:
>> + pr_info("Error applying setting, reverse things back\n");
>
> That should be dev_err() on the client device.
ok, I'll do that
>> + /*
>> + * If the loop stopped on the 1st entry, nothing has been enabled,
>> + * so jump directly to the 2nd phase
>> + */
>> + if (list_entry(&setting->node, typeof(*setting), node) ==
>> + list_first_entry(&state->settings, typeof(*setting), node))
>> + goto reapply_old_state;
>
> That's just an optimization, not a correctness issue isn't it?
>
> I think it'd be simpler to just always run the list_for_each_entry()
> below and let it bail out on the first loop if that's where the failure
> happened. It's a lot simpler than understanding the conditional above,
> which I didn't really try to do.
That's right, I'll wipe that out.
>> + 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;
}
And maybe I'm wrong, but I understood that to "undo" pinmux_enable_setting,
we call pinmux_disable_setting() and pinmux_free_setting() (which is
empty for now).
And to undo pinconf_apply_setting() we call pinconf_free_setting()
And that's what pinctrl_free_setting() does.
>> + }
>> +reapply_old_state:
>> + /* FIXME: re-enable old setting */
>> + return ret;
>> }
>
Thanks for your comments!
Richard.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
