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 <dan.carpenter@xxxxxxxxxx>
> > ---
> > (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
