If rzg2l_map_add_config() fails then the error handling calls
mutex_unlock(&pctrl->mutex) but we're not holding that mutex. Move
the unlocks to before the gotos to avoid this situation.
Fixes: d3aaa7203a17 ("pinctrl: renesas: rzg2l: Add pin configuration support for pinmux groups")
Signed-off-by: Dan Carpenter <[email protected]>
---
(Not tested).
drivers/pinctrl/renesas/pinctrl-rzg2l.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index 80fb5011c7bb..8bbfb0530538 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -453,7 +453,8 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev *pctldev,
gsel = pinctrl_generic_add_group(pctldev, name, pins, num_pinmux, NULL);
if (gsel < 0) {
ret = gsel;
- goto unlock;
+ mutex_unlock(&pctrl->mutex);
+ goto done;
}
/*
@@ -464,6 +465,7 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev *pctldev,
fsel = pinmux_generic_add_function(pctldev, name, pin_fn, 1, psel_val);
if (fsel < 0) {
ret = fsel;
+ mutex_unlock(&pctrl->mutex);
goto remove_group;
}
@@ -490,8 +492,6 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev *pctldev,
remove_group:
pinctrl_generic_remove_group(pctldev, gsel);
-unlock:
- mutex_unlock(&pctrl->mutex);
done:
*index = idx;
kfree(configs);
--
2.43.0
Hi, Dan,
Thanks for your patch!
On 10.01.2024 20:41, Dan Carpenter wrote:
> If rzg2l_map_add_config() fails then the error handling calls
> mutex_unlock(&pctrl->mutex) but we're not holding that mutex. Move
> the unlocks to before the gotos to avoid this situation.
>
> Fixes: d3aaa7203a17 ("pinctrl: renesas: rzg2l: Add pin configuration support for pinmux groups")
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> (Not tested).
I've tested it on RZ/G3S SoC and all is good.
However, I think, to keep the locking scheme unchanged and simpler (FMPOV),
commit d3aaa7203a17 ("pinctrl: renesas: rzg2l: Add pin configuration
support for pinmux groups") should have been call rzg2l_map_add_config()
just before the mutex is locked. That would be the following diff:
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -447,6 +447,16 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev
*pctldev,
name = np->name;
}
+ if (num_configs) {
+ ret = rzg2l_map_add_config(&maps[idx], name,
+ PIN_MAP_TYPE_CONFIGS_GROUP,
+ configs, num_configs);
+ if (ret < 0)
+ goto done;
+
+ idx++;
+ }
+
mutex_lock(&pctrl->mutex);
/* Register a single pin group listing all the pins we read from DT */
@@ -474,16 +484,6 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev
*pctldev,
maps[idx].data.mux.function = name;
idx++;
- if (num_configs) {
- ret = rzg2l_map_add_config(&maps[idx], name,
- PIN_MAP_TYPE_CONFIGS_GROUP,
- configs, num_configs);
- if (ret < 0)
- goto remove_group;
-
- idx++;
- }
-
dev_dbg(pctrl->dev, "Parsed %pOF with %d pins\n", np, num_pinmux);
ret = 0;
goto done;
Would you mind doing it like this?
Please, let me know if you want me to handle it.
Thank you,
Claudiu Beznea
>
> drivers/pinctrl/renesas/pinctrl-rzg2l.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> index 80fb5011c7bb..8bbfb0530538 100644
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -453,7 +453,8 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev *pctldev,
> gsel = pinctrl_generic_add_group(pctldev, name, pins, num_pinmux, NULL);
> if (gsel < 0) {
> ret = gsel;
> - goto unlock;
> + mutex_unlock(&pctrl->mutex);
> + goto done;
> }
>
> /*
> @@ -464,6 +465,7 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev *pctldev,
> fsel = pinmux_generic_add_function(pctldev, name, pin_fn, 1, psel_val);
> if (fsel < 0) {
> ret = fsel;
> + mutex_unlock(&pctrl->mutex);
> goto remove_group;
> }
>
> @@ -490,8 +492,6 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev *pctldev,
>
> remove_group:
> pinctrl_generic_remove_group(pctldev, gsel);
> -unlock:
> - mutex_unlock(&pctrl->mutex);
> done:
> *index = idx;
> kfree(configs);
On Fri, Jan 12, 2024 at 10:55:40AM +0200, claudiu beznea wrote:
> Hi, Dan,
>
> Thanks for your patch!
>
> On 10.01.2024 20:41, Dan Carpenter wrote:
> > If rzg2l_map_add_config() fails then the error handling calls
> > mutex_unlock(&pctrl->mutex) but we're not holding that mutex. Move
> > the unlocks to before the gotos to avoid this situation.
> >
> > Fixes: d3aaa7203a17 ("pinctrl: renesas: rzg2l: Add pin configuration support for pinmux groups")
> > Signed-off-by: Dan Carpenter <[email protected]>
> > ---
> > (Not tested).
>
> I've tested it on RZ/G3S SoC and all is good.
>
> However, I think, to keep the locking scheme unchanged and simpler (FMPOV),
> commit d3aaa7203a17 ("pinctrl: renesas: rzg2l: Add pin configuration
> support for pinmux groups") should have been call rzg2l_map_add_config()
> just before the mutex is locked. That would be the following diff:
>
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -447,6 +447,16 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev
> *pctldev,
> name = np->name;
> }
>
> + if (num_configs) {
> + ret = rzg2l_map_add_config(&maps[idx], name,
> + PIN_MAP_TYPE_CONFIGS_GROUP,
> + configs, num_configs);
> + if (ret < 0)
> + goto done;
> +
> + idx++;
> + }
> +
> mutex_lock(&pctrl->mutex);
>
> /* Register a single pin group listing all the pins we read from DT */
> @@ -474,16 +484,6 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev
> *pctldev,
> maps[idx].data.mux.function = name;
> idx++;
^^^^^
>
> - if (num_configs) {
> - ret = rzg2l_map_add_config(&maps[idx], name,
> - PIN_MAP_TYPE_CONFIGS_GROUP,
> - configs, num_configs);
> - if (ret < 0)
> - goto remove_group;
> -
> - idx++;
> - }
Does the ordering of the maps[] not matter?
> -
> dev_dbg(pctrl->dev, "Parsed %pOF with %d pins\n", np, num_pinmux);
> ret = 0;
> goto done;
>
> Would you mind doing it like this?
>
> Please, let me know if you want me to handle it.
Either way is fine. Whatever is easiest.
regards,
dan carpenter
On 12.01.2024 11:53, Dan Carpenter wrote:
> On Fri, Jan 12, 2024 at 10:55:40AM +0200, claudiu beznea wrote:
>> Hi, Dan,
>>
>> Thanks for your patch!
>>
>> On 10.01.2024 20:41, Dan Carpenter wrote:
>>> If rzg2l_map_add_config() fails then the error handling calls
>>> mutex_unlock(&pctrl->mutex) but we're not holding that mutex. Move
>>> the unlocks to before the gotos to avoid this situation.
>>>
>>> Fixes: d3aaa7203a17 ("pinctrl: renesas: rzg2l: Add pin configuration support for pinmux groups")
>>> Signed-off-by: Dan Carpenter <[email protected]>
>>> ---
>>> (Not tested).
>>
>> I've tested it on RZ/G3S SoC and all is good.
>>
>> However, I think, to keep the locking scheme unchanged and simpler (FMPOV),
>> commit d3aaa7203a17 ("pinctrl: renesas: rzg2l: Add pin configuration
>> support for pinmux groups") should have been call rzg2l_map_add_config()
>> just before the mutex is locked. That would be the following diff:
>>
>> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
>> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
>> @@ -447,6 +447,16 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev
>> *pctldev,
>> name = np->name;
>> }
>>
>> + if (num_configs) {
>> + ret = rzg2l_map_add_config(&maps[idx], name,
>> + PIN_MAP_TYPE_CONFIGS_GROUP,
>> + configs, num_configs);
>> + if (ret < 0)
>> + goto done;
>> +
>> + idx++;
>> + }
>> +
>> mutex_lock(&pctrl->mutex);
>>
>> /* Register a single pin group listing all the pins we read from DT */
>> @@ -474,16 +484,6 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev
>> *pctldev,
>> maps[idx].data.mux.function = name;
>> idx++;
> ^^^^^
This needs to be here for subsequent calls of rzg2l_dt_subnode_to_map() to
know which entry in maps[] to be populated next time.
>
>>
>> - if (num_configs) {
>> - ret = rzg2l_map_add_config(&maps[idx], name,
>> - PIN_MAP_TYPE_CONFIGS_GROUP,
>> - configs, num_configs);
>> - if (ret < 0)
>> - goto remove_group;
>> -
>> - idx++;
>> - }
>
> Does the ordering of the maps[] not matter?
It doesn't matter, AFAIKT. The core code checks for map type (e.g.
PIN_MAP_TYPE_CONFIGS_GROUP) when processes the data from maps[].
>
>> -
>> dev_dbg(pctrl->dev, "Parsed %pOF with %d pins\n", np, num_pinmux);
>> ret = 0;
>> goto done;
>>
>> Would you mind doing it like this?
>>
>> Please, let me know if you want me to handle it.
>
> Either way is fine. Whatever is easiest.
Ok, I'll prepare a patch as I already tested it on my side on multiple
platforms.
Thank you,
Claudiu Beznea
>
> regards,
> dan carpenter
>
On Fri, Jan 12, 2024 at 02:05:17PM +0200, claudiu beznea wrote:
>
> Ok, I'll prepare a patch as I already tested it on my side on multiple
> platforms.
Awesome. Thanks!
regards,
dan carpenter