diff mbox series

[v3,12/12] gpio: rockchip: replace mutex_lock() with guard()

Message ID 20240903073649.237362-13-ye.zhang@rock-chips.com
State New
Headers show
Series gpio: rockchip: Update the GPIO driver | expand

Commit Message

Ye Zhang Sept. 3, 2024, 7:36 a.m. UTC
Replacing mutex_lock with guard() simplifies the code and helps avoid
deadlocks.

Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
---
 drivers/gpio/gpio-rockchip.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

Comments

Andy Shevchenko Sept. 3, 2024, 4:10 p.m. UTC | #1
On Tue, Sep 03, 2024 at 03:36:49PM +0800, Ye Zhang wrote:
> Replacing mutex_lock with guard() simplifies the code and helps avoid

mutex_lock()

avoiding

> deadlocks.

...

> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c

+ cleanup.h

...

>  	}
> -	mutex_lock(&bank->deferred_lock);
> +	guard(mutex)(&bank->deferred_lock);

Make it surrounded by blank lines.

> +	ret = rockchip_get_bank_data(bank);
> +	if (ret)
> +		goto err_disabled_clk;
Christophe JAILLET Sept. 3, 2024, 4:52 p.m. UTC | #2
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@rock-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);
diff mbox series

Patch

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);
+	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);