linux-kernel - Re: [RFC PATCH 2/5] gnss: ubx: use new helper to remove open coded regulator handling

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: <ZJFn04P7_JhC24ST@hovoldconsulting.com>
Date:   Tue, 20 Jun 2023 10:48:19 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Wolfram Sang <wsa+renesas@...g-engineering.com>
Cc:     linux-renesas-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>
Subject: Re: [RFC PATCH 2/5] gnss: ubx: use new helper to remove open coded
 regulator handling

On Tue, May 23, 2023 at 08:43:07AM +0200, Wolfram Sang wrote:
> v_bckp shall always be enabled as long as the device exists. We now have
> a regulator helper for that, use it.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@...g-engineering.com>
> ---
>  drivers/gnss/ubx.c | 26 ++++----------------------
>  1 file changed, 4 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gnss/ubx.c b/drivers/gnss/ubx.c
> index c01e1e875727..c8d027973b85 100644
> --- a/drivers/gnss/ubx.c
> +++ b/drivers/gnss/ubx.c
> @@ -17,7 +17,6 @@
>  #include "serial.h"
>  
>  struct ubx_data {
> -	struct regulator *v_bckp;
>  	struct regulator *vcc;
>  };
>  
> @@ -106,30 +105,16 @@ static int ubx_probe(struct serdev_device *serdev)
>  		goto err_free_gserial;
>  	}
>  
> -	data->v_bckp = devm_regulator_get_optional(&serdev->dev, info->v_bckp_name);
> -	if (IS_ERR(data->v_bckp)) {
> -		ret = PTR_ERR(data->v_bckp);
> -		if (ret == -ENODEV)
> -			data->v_bckp = NULL;
> -		else
> -			goto err_free_gserial;
> -	}
> -
> -	if (data->v_bckp) {
> -		ret = regulator_enable(data->v_bckp);
> -		if (ret)
> -			goto err_free_gserial;
> -	}
> +	ret = devm_regulator_get_enable_optional(&serdev->dev, info->v_bckp_name);
> +	if (ret < 0 && ret != -ENODEV)
> +		goto err_free_gserial;

I'm a bit torn about this one as I'm generally sceptical of devres and
especially helpers that enable or register resources, which just tends to
lead to subtle bugs.

But if there's one place where this new helper, which importantly does
not return a pointer to the regulator, may be useful I guess it's here.

Care to respin this one after dropping the merge patch?

Johan