linux-kernel - Re: [PATCH v3 12/12] gpio: rockchip: replace mutex

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6555c378-f230-480e-9dd4-625ea550074f@wanadoo.fr>
Date: Tue, 3 Sep 2024 18:52:16 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Ye Zhang <ye.zhang@...k-chips.com>, linus.walleij@...aro.org,
 brgl@...ev.pl, heiko@...ech.de, linux-gpio@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org
Cc: linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
 mika.westerberg@...ux.intel.com, andriy.shevchenko@...ux.intel.com,
 tao.huang@...k-chips.com, finley.xiao@...k-chips.com,
 tim.chen@...k-chips.com, elaine.zhang@...k-chips.com
Subject: Re: [PATCH v3 12/12] gpio: rockchip: replace mutex_lock() with
 guard()

Le 03/09/2024 à 09:36, Ye Zhang a écrit :
> Replacing mutex_lock with guard() simplifies the code and helps avoid
> deadlocks.
> 
> Signed-off-by: Ye Zhang <ye.zhang@...k-chips.com>
> ---
>   drivers/gpio/gpio-rockchip.c | 14 +++++---------
>   1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index 73e57efb46fc..d5c57617fc86 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -765,20 +765,19 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
>   		}
>   	}
>   
> -	ret = rockchip_get_bank_data(bank);
> -	if (ret)
> -		goto err_disabled_clk;
> -
>   	/*
>   	 * Prevent clashes with a deferred output setting
>   	 * being added right at this moment.
>   	 */
> -	mutex_lock(&bank->deferred_lock);
> +	guard(mutex)(&bank->deferred_lock);
> +	ret = rockchip_get_bank_data(bank);

rockchip_get_bank_data() was out of the lock before, now it is inside.

It looks ok, but is it on purpose? If so, maybe it could be mentioned or 
explained why in the changelog ?

CJ

> +	if (ret)
> +		goto err_disabled_clk;
>   
>   	ret = rockchip_gpiolib_register(bank);
>   	if (ret) {
>   		dev_err(bank->dev, "Failed to register gpio %d\n", ret);
> -		goto err_unlock;
> +		goto err_disabled_clk;
>   	}
>   
>   	while (!list_empty(&bank->deferred_pins)) {
> @@ -805,14 +804,11 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
>   		kfree(cfg);
>   	}
>   
> -	mutex_unlock(&bank->deferred_lock);
>   
>   	platform_set_drvdata(pdev, bank);
>   	dev_info(dev, "probed %pOF\n", np);
>   
>   	return 0;
> -err_unlock:
> -	mutex_unlock(&bank->deferred_lock);
>   err_disabled_clk:
>   	if (bank->manual_clk_release)
>   		clk_disable_unprepare(bank->clk);