Re: [PATCH 3/4] pinctrl: disable and free setting in select_state incase of error

[Richard Genoud]

2013/3/28 Stephen Warren <swarren@xxxxxxxxxxxxx>:
> On 03/28/2013 04:55 AM, Richard Genoud wrote:
>> 2013/3/28 Stephen Warren <swarren@xxxxxxxxxxxxx>:
>>> 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.
Ok, I understand now.

>
>> 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.
>
got it.
I'll change that

thanks !


-- 
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/