Received-SPF: pass (google.com: domain of linux-kernel+bounces-24527-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 12 Jan 2024 12:53:10 +0300
From: Dan Carpenter <dan.carpenter@linaro.org>
To: claudiu beznea <claudiu.beznea@tuxon.dev>
Cc: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Linus Walleij <linus.walleij@linaro.org>,
	linux-renesas-soc@vger.kernel.org, linux-gpio@vger.kernel.org,
	linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] pinctrl: renesas: rzg2l: Fix double unlock in
 rzg2l_dt_subnode_to_map()
Message-ID: <300d204b-8151-45f3-9977-7ceb3a5c5eb0@moroto.mountain>
References: <f8c3a3a0-7c48-4e40-8af0-ed4e9d9b049f@moroto.mountain>
 <185128ab-f229-4b40-91cd-ebdc138e11ac@tuxon.dev>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <185128ab-f229-4b40-91cd-ebdc138e11ac@tuxon.dev>

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