[PATCH 0/4] pintrcl: restore old state when pinctrl_select_state fails

[PATCH 0/4] pintrcl: restore old state when pinctrl_select_state fails

[Richard Genoud]

If there's an error when applying a new state (for example, if a pin is already muxed), old state is not restored.
This patch set corrects this behaviour.
The first patch is just a small typo fix.
The second doesn't introduce any functionnal change, but prepares for the third patch.
The third patch undoes what have been done to enable the new state.
The last patch re-applies the old state.

Richard Genoud (4):
  pinctrl: fix typo in header
  pinctrl: create pinctrl_free_setting function
  pinctrl: disable and free setting in select_state in case of error
  pinctrl: re-enable old state in case of error in pinctrl_select_state

 drivers/pinctrl/core.c |   78 ++++++++++++++++++++++++++++++++++++++----------
 drivers/pinctrl/core.h |    2 +-
 2 files changed, 63 insertions(+), 17 deletions(-)

-- 
1.7.2.5

[PATCH 1/4] pinctrl: fix typo in header

[Richard Genoud]

Clearly, "node" was meant instead of "not"

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/pinctrl/core.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index ee72f1f..6d3d400 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -72,7 +72,7 @@ struct pinctrl {
 
 /**
  * struct pinctrl_state - a pinctrl state for a device
- * @node: list not for struct pinctrl's @states field
+ * @node: list node for struct pinctrl's @states field
  * @name: the name of this state
  * @settings: a list of settings for this state
  */
-- 
1.7.2.5

[PATCH 2/4] pinctrl: create pinctrl_free_setting function

[Richard Genoud]

This prepares the implementation of pinctrl_select_state_locked() free code.

No functionnal change.

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/pinctrl/core.c |   32 +++++++++++++++++++-------------
 1 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index b0de6e7..a28dcca 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -758,6 +758,24 @@ struct pinctrl *pinctrl_get(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(pinctrl_get);
 
+static void pinctrl_free_setting(bool disable_setting,
+				 struct pinctrl_setting *setting)
+{
+	switch (setting->type) {
+	case PIN_MAP_TYPE_MUX_GROUP:
+		if (disable_setting)
+			pinmux_disable_setting(setting);
+		pinmux_free_setting(setting);
+		break;
+	case PIN_MAP_TYPE_CONFIGS_PIN:
+	case PIN_MAP_TYPE_CONFIGS_GROUP:
+		pinconf_free_setting(setting);
+		break;
+	default:
+		break;
+	}
+}
+
 static void pinctrl_put_locked(struct pinctrl *p, bool inlist)
 {
 	struct pinctrl_state *state, *n1;
@@ -765,19 +783,7 @@ static void pinctrl_put_locked(struct pinctrl *p, bool inlist)
 
 	list_for_each_entry_safe(state, n1, &p->states, node) {
 		list_for_each_entry_safe(setting, n2, &state->settings, node) {
-			switch (setting->type) {
-			case PIN_MAP_TYPE_MUX_GROUP:
-				if (state == p->state)
-					pinmux_disable_setting(setting);
-				pinmux_free_setting(setting);
-				break;
-			case PIN_MAP_TYPE_CONFIGS_PIN:
-			case PIN_MAP_TYPE_CONFIGS_GROUP:
-				pinconf_free_setting(setting);
-				break;
-			default:
-				break;
-			}
+			pinctrl_free_setting(state == p->state, setting);
 			list_del(&setting->node);
 			kfree(setting);
 		}
-- 
1.7.2.5

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

[Richard Genoud]

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.

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/pinctrl/core.c |   28 +++++++++++++++++++++++++---
 1 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index a28dcca..350d5f8 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -894,7 +894,7 @@ static int pinctrl_select_state_locked(struct pinctrl *p,
 		}
 	}
 
-	p->state = state;
+	p->state = NULL;
 
 	/* Apply all the settings for the new state */
 	list_for_each_entry(setting, &state->settings, node) {
@@ -910,13 +910,35 @@ static int pinctrl_select_state_locked(struct pinctrl *p,
 			ret = -EINVAL;
 			break;
 		}
+
 		if (ret < 0) {
-			/* FIXME: Difficult to return to prev state */
-			return ret;
+			goto unapply_new_state;
 		}
 	}
 
+	p->state = state;
+
 	return 0;
+
+unapply_new_state:
+	pr_info("Error applying setting, reverse things back\n");
+
+	/*
+	 * 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;
+
+	list_for_each_entry(setting2, &state->settings, node) {
+		if (&setting2->node == &setting->node)
+			break;
+		pinctrl_free_setting(true, setting2);
+	}
+reapply_old_state:
+	/* FIXME: re-enable old setting */
+	return ret;
 }
 
 /**
-- 
1.7.2.5

[PATCH 4/4] pinctrl: re-enable old state in case of error in pinctrl_select_state

[Richard Genoud]

If a new state is applied, the groups configured in the old state but
not in the new state are disabled.
If something goes wrong and the new state can't be applied, we have to
re-enable those groups.

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/pinctrl/core.c |   20 +++++++++++++++++++-
 1 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 350d5f8..9b505acc 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -861,6 +861,7 @@ static int pinctrl_select_state_locked(struct pinctrl *p,
 				       struct pinctrl_state *state)
 {
 	struct pinctrl_setting *setting, *setting2;
+	struct pinctrl_state *old_state = p->state;
 	int ret;
 
 	if (p->state == state)
@@ -937,7 +938,24 @@ unapply_new_state:
 		pinctrl_free_setting(true, setting2);
 	}
 reapply_old_state:
-	/* FIXME: re-enable old setting */
+	if (old_state) {
+		list_for_each_entry(setting, &old_state->settings, node) {
+			bool found = false;
+			if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
+				continue;
+			list_for_each_entry(setting2, &state->settings, node) {
+				if (setting2->type != PIN_MAP_TYPE_MUX_GROUP)
+					continue;
+				if (setting2->data.mux.group ==
+						setting->data.mux.group) {
+					found = true;
+					break;
+				}
+			}
+			if (!found)
+				pinmux_enable_setting(setting);
+		}
+	}
 	return ret;
 }
 
-- 
1.7.2.5

Re: [PATCH 1/4] pinctrl: fix typo in header

[Linus Walleij]

On Mon, Mar 25, 2013 at 3:47 PM, Richard Genoud
<richard.genoud@gmail.com> wrote:

> Clearly, "node" was meant instead of "not"
>
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>

Patch applied.

Thanks!
Linus Walleij

Re: [PATCH 2/4] pinctrl: create pinctrl_free_setting function

[Linus Walleij]

On Mon, Mar 25, 2013 at 3:47 PM, Richard Genoud
<richard.genoud@gmail.com> wrote:

> This prepares the implementation of pinctrl_select_state_locked() free code.
>
> No functionnal change.
>
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>

Makes code more readable so patch applied!

Thanks,
Linus Walleij

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

[Linus Walleij]

On Mon, Mar 25, 2013 at 3:47 PM, Richard Genoud
<richard.genoud@gmail.com> 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.
>
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>

Thanks for fixing this! :-D

Patch applied.

Yours,
Linus Walleij

Re: [PATCH 4/4] pinctrl: re-enable old state in case of error in pinctrl_select_state

[Linus Walleij]

On Mon, Mar 25, 2013 at 3:47 PM, Richard Genoud
<richard.genoud@gmail.com> wrote:

> If a new state is applied, the groups configured in the old state but
> not in the new state are disabled.
> If something goes wrong and the new state can't be applied, we have to
> re-enable those groups.
>
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>

Another very tasty patch, applied!

Thanks!
Linus Walleij

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

[Stephen Warren]

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.

> +unapply_new_state:
> +	pr_info("Error applying setting, reverse things back\n");

That should be dev_err() on the client device.

> +	/*
> +	 * 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.

> +	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.

> +	}
> +reapply_old_state:
> +	/* FIXME: re-enable old setting */
> +	return ret;
>  }

Re: [PATCH 4/4] pinctrl: re-enable old state in case of error in pinctrl_select_state

[Stephen Warren]

On 03/25/2013 08:47 AM, Richard Genoud wrote:
> If a new state is applied, the groups configured in the old state but
> not in the new state are disabled.
> If something goes wrong and the new state can't be applied, we have to
> re-enable those groups.

What is the use-case for this? I wonder if it isn't better to simply
undo the partial selection of the new state (as patch 3/4 attempts to
do) and then leave p->state==NULL, indicating that no state is actively
selected. IIRC, this would be the same as right after the initial
pinctrl_select().

I wonder if it's likely that attempting to re-apply the old state would
actually work, given that applying something just failed.

Finally, this recovery code doesn't:

a) Process anything except MUX_GROUP; any pin config settings in the old
state aren't restored.

b) (I think) Apply any mux settings that don't involve groups that are
referenced by both the old and new states; given that patch 3/4 attempts
to undo everything in the failed application of the new state, I think
this "re-apply the old state" code should simple run through everything
in the old state any apply it unconditionally.

c) Set p->state = oldstate, so it's left at NULL, which would confuse
any future pinctrl_select().

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

[Richard Genoud]

2013/3/28 Stephen Warren <swarren@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.

Re: [PATCH 4/4] pinctrl: re-enable old state in case of error in pinctrl_select_state

[Richard Genoud]

2013/3/28 Stephen Warren <swarren@wwwdotorg.org>:
> On 03/25/2013 08:47 AM, Richard Genoud wrote:
>> If a new state is applied, the groups configured in the old state but
>> not in the new state are disabled.
>> If something goes wrong and the new state can't be applied, we have to
>> re-enable those groups.
>
> What is the use-case for this? I wonder if it isn't better to simply
> undo the partial selection of the new state (as patch 3/4 attempts to
> do) and then leave p->state==NULL, indicating that no state is actively
> selected. IIRC, this would be the same as right after the initial
> pinctrl_select().
>
> I wonder if it's likely that attempting to re-apply the old state would
> actually work, given that applying something just failed.
>
> Finally, this recovery code doesn't:
>
> a) Process anything except MUX_GROUP; any pin config settings in the old
> state aren't restored.
>
> b) (I think) Apply any mux settings that don't involve groups that are
> referenced by both the old and new states; given that patch 3/4 attempts
> to undo everything in the failed application of the new state, I think
> this "re-apply the old state" code should simple run through everything
> in the old state any apply it unconditionally.
Well, I'm not comfortable enough with this code to argue on that...

> c) Set p->state = oldstate, so it's left at NULL, which would confuse
> any future pinctrl_select().
Arrrggg !
I forgot it !
I'll send a fix on that


-- 
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?

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

[Stephen Warren]

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

>>> +     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.

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

[Richard Genoud]

2013/3/28 Stephen Warren <swarren@wwwdotorg.org>:
> On 03/28/2013 04:55 AM, Richard Genoud wrote:
>> 2013/3/28 Stephen Warren <swarren@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
>
>>>> +     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 ?

Re: [PATCH 4/4] pinctrl: re-enable old state in case of error in pinctrl_select_state

[Richard GENOUD]

On [mer., 27.03.2013 17:55:45], Stephen Warren wrote:
> On 03/25/2013 08:47 AM, Richard Genoud wrote:
> > If a new state is applied, the groups configured in the old state but
> > not in the new state are disabled.
> > If something goes wrong and the new state can't be applied, we have to
> > re-enable those groups.
> 
> What is the use-case for this? I wonder if it isn't better to simply
> undo the partial selection of the new state (as patch 3/4 attempts to
> do) and then leave p->state==NULL, indicating that no state is actively
> selected. IIRC, this would be the same as right after the initial
> pinctrl_select().
> 
> I wonder if it's likely that attempting to re-apply the old state would
> actually work, given that applying something just failed.
> 
> Finally, this recovery code doesn't:
> 
> a) Process anything except MUX_GROUP; any pin config settings in the old
> state aren't restored.
> 
> b) (I think) Apply any mux settings that don't involve groups that are
> referenced by both the old and new states; given that patch 3/4 attempts
> to undo everything in the failed application of the new state, I think
> this "re-apply the old state" code should simple run through everything
> in the old state any apply it unconditionally.
So, if I understand correctly, it could be as simple as that:
 	}
 
-	if (old_state) {
-		list_for_each_entry(setting, &old_state->settings, node) {
-			bool found = false;
-			if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
-				continue;
-			list_for_each_entry(setting2, &state->settings, node) {
-				if (setting2->type != PIN_MAP_TYPE_MUX_GROUP)
-					continue;
-				if (setting2->data.mux.group ==
-						setting->data.mux.group) {
-					found = true;
-					break;
-				}
-			}
-			if (!found)
-				pinmux_enable_setting(setting);
-		}
-	}
-
 	p->state = old_state;
+
+	if (old_state)
+		pinctrl_select_state_locked(p, NULL);
+
 	return ret;
 }

> 
> c) Set p->state = oldstate, so it's left at NULL, which would confuse
> any future pinctrl_select().

Re: [PATCH 4/4] pinctrl: re-enable old state in case of error in pinctrl_select_state

[Stephen Warren]

On 03/28/2013 11:34 AM, Richard GENOUD wrote:
> On [mer., 27.03.2013 17:55:45], Stephen Warren wrote:
>> On 03/25/2013 08:47 AM, Richard Genoud wrote:
>>> If a new state is applied, the groups configured in the old state but
>>> not in the new state are disabled.
>>> If something goes wrong and the new state can't be applied, we have to
>>> re-enable those groups.
>>
>> What is the use-case for this? I wonder if it isn't better to simply
>> undo the partial selection of the new state (as patch 3/4 attempts to
>> do) and then leave p->state==NULL, indicating that no state is actively
>> selected. IIRC, this would be the same as right after the initial
>> pinctrl_select().
>>
>> I wonder if it's likely that attempting to re-apply the old state would
>> actually work, given that applying something just failed.
>>
>> Finally, this recovery code doesn't:
>>
>> a) Process anything except MUX_GROUP; any pin config settings in the old
>> state aren't restored.
>>
>> b) (I think) Apply any mux settings that don't involve groups that are
>> referenced by both the old and new states; given that patch 3/4 attempts
>> to undo everything in the failed application of the new state, I think
>> this "re-apply the old state" code should simple run through everything
>> in the old state any apply it unconditionally.
>
> So, if I understand correctly, it could be as simple as that:
>  	}
>  
> -	if (old_state) {
> -		list_for_each_entry(setting, &old_state->settings, node) {
> -			bool found = false;
> -			if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
> -				continue;
> -			list_for_each_entry(setting2, &state->settings, node) {
> -				if (setting2->type != PIN_MAP_TYPE_MUX_GROUP)
> -					continue;
> -				if (setting2->data.mux.group ==
> -						setting->data.mux.group) {
> -					found = true;
> -					break;
> -				}
> -			}
> -			if (!found)
> -				pinmux_enable_setting(setting);
> -		}
> -	}
> -
>  	p->state = old_state;

I think you want to remove that line too, so that p->state == NULL in
the error case, so that if the pinctrl_select_state_locked() call below
also fails to restore the old state, then (!old_state) will be true
inside the recursive call, so it doesn't recurse into itself forever.

> +	if (old_state)
> +		pinctrl_select_state_locked(p, NULL);

You want to pass old_state rather than NULL there, I think.

Re: [PATCH 4/4] pinctrl: re-enable old state in case of error in pinctrl_select_state

[Richard GENOUD]

On [jeu., 28.03.2013 11:38:23], Stephen Warren wrote:
> On 03/28/2013 11:34 AM, Richard GENOUD wrote:
> > On [mer., 27.03.2013 17:55:45], Stephen Warren wrote:
> >> On 03/25/2013 08:47 AM, Richard Genoud wrote:
> >>> If a new state is applied, the groups configured in the old state but
> >>> not in the new state are disabled.
> >>> If something goes wrong and the new state can't be applied, we have to
> >>> re-enable those groups.
> >>
> >> What is the use-case for this? I wonder if it isn't better to simply
> >> undo the partial selection of the new state (as patch 3/4 attempts to
> >> do) and then leave p->state==NULL, indicating that no state is actively
> >> selected. IIRC, this would be the same as right after the initial
> >> pinctrl_select().
> >>
> >> I wonder if it's likely that attempting to re-apply the old state would
> >> actually work, given that applying something just failed.
> >>
> >> Finally, this recovery code doesn't:
> >>
> >> a) Process anything except MUX_GROUP; any pin config settings in the old
> >> state aren't restored.
> >>
> >> b) (I think) Apply any mux settings that don't involve groups that are
> >> referenced by both the old and new states; given that patch 3/4 attempts
> >> to undo everything in the failed application of the new state, I think
> >> this "re-apply the old state" code should simple run through everything
> >> in the old state any apply it unconditionally.
> >
> > So, if I understand correctly, it could be as simple as that:
> >  	}
> >  
> > -	if (old_state) {
> > -		list_for_each_entry(setting, &old_state->settings, node) {
> > -			bool found = false;
> > -			if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
> > -				continue;
> > -			list_for_each_entry(setting2, &state->settings, node) {
> > -				if (setting2->type != PIN_MAP_TYPE_MUX_GROUP)
> > -					continue;
> > -				if (setting2->data.mux.group ==
> > -						setting->data.mux.group) {
> > -					found = true;
> > -					break;
> > -				}
> > -			}
> > -			if (!found)
> > -				pinmux_enable_setting(setting);
> > -		}
> > -	}
> > -
> >  	p->state = old_state;
> 
> I think you want to remove that line too, so that p->state == NULL in
> the error case, so that if the pinctrl_select_state_locked() call below
> also fails to restore the old state, then (!old_state) will be true
> inside the recursive call, so it doesn't recurse into itself forever.
> 
> > +	if (old_state)
> > +		pinctrl_select_state_locked(p, NULL);
> 
> You want to pass old_state rather than NULL there, I think.
Hum, I'm starting to code nonsense. It's time to go home !

But yes, we clearly don't want to have p->state != NULL in the recursive
call (I've seen that, and an evil force made me wrote the contrary :))

So, just:
+	if (old_state)
+		pinctrl_select_state_locked(p, old_state);
Will do the trick.
It can't loop forever as p->state is null.
If it fails, p->state will be still null.
If it succeeds, p->state will be == old_state, and that's what we want.

Got it !(finally)

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

[Linus Walleij]

On Thu, Mar 28, 2013 at 4:47 PM, Richard Genoud
<richard.genoud@gmail.com> wrote:
> 2013/3/28 Stephen Warren <swarren@wwwdotorg.org>:

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

I guess the incremental patches have fixed most issues pointed out
in this thread? Elese keep the add-on patches coming, as long as the
end result looks OK to both of you I'm happy too.

Yours,
Linus Walleij

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

[Richard Genoud]

2013/4/3 Linus Walleij <linus.walleij@linaro.org>:
> On Thu, Mar 28, 2013 at 4:47 PM, Richard Genoud
> <richard.genoud@gmail.com> wrote:
>> 2013/3/28 Stephen Warren <swarren@wwwdotorg.org>:
>
>>> 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
>
> I guess the incremental patches have fixed most issues pointed out
> in this thread? Elese keep the add-on patches coming, as long as the
> end result looks OK to both of you I'm happy too.
Yes, I think we've covered it all.
Tthe last patches are:
[PATCH 1/2] pinctrl: select_state: don't call pinctrl_free_setting on error
[PATCH 2/2] pinctrl: simplify the re-enable old state code in
pinctrl_select_state
(sent and reviewed on 29 march.)

Best regards,
Richard.