| Message ID | f8c3a3a0-7c48-4e40-8af0-ed4e9d9b049f@moroto.mountain |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: renesas: rzg2l: Fix double unlock in rzg2l_dt_subnode_to_map() | expand |
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@linaro.org> > --- > (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 <dan.carpenter@linaro.org> > > --- > > (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 <dan.carpenter@linaro.org> >>> --- >>> (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
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);
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@linaro.org> --- (Not tested). drivers/pinctrl/renesas/pinctrl-rzg2l.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)