netdev - Re: [PATCH v1] net: ipa: make use of dev_err_cast

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: <20240828160728.GR1368797@kernel.org>
Date: Wed, 28 Aug 2024 17:07:28 +0100
From: Simon Horman <horms@...nel.org>
To: Yuesong Li <liyuesong@...o.com>, Hongbo Li <lihongbo22@...wei.com>
Cc: elder@...nel.org, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, opensource.kernel@...o.com
Subject: Re: [PATCH v1] net: ipa: make use of dev_err_cast_probe()

On Wed, Aug 28, 2024 at 04:41:15PM +0800, Yuesong Li wrote:
> Using dev_err_cast_probe() to simplify the code.
> 
> Signed-off-by: Yuesong Li <liyuesong@...o.com>
> ---
>  drivers/net/ipa/ipa_power.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
> index 65fd14da0f86..248bcc0b661e 100644
> --- a/drivers/net/ipa/ipa_power.c
> +++ b/drivers/net/ipa/ipa_power.c
> @@ -243,9 +243,8 @@ ipa_power_init(struct device *dev, const struct ipa_power_data *data)
>  
>  	clk = clk_get(dev, "core");
>  	if (IS_ERR(clk)) {
> -		dev_err_probe(dev, PTR_ERR(clk), "error getting core clock\n");
> -
> -		return ERR_CAST(clk);
> +		return dev_err_cast_probe(dev, clk,
> +				"error getting core clock\n");
>  	}
>  
>  	ret = clk_set_rate(clk, data->core_clock_rate);

Hi,

There are lot of clean-up patches floating around at this time.
And I'm unsure if you are both on the same team or not, but in
any case it would be nice if there was some co-ordination.
Because here we have two different versions of the same patch.
Which, from a maintainer and reviewer pov is awkward.

In principle the change(s) look(s) fine to me. But there are some minor
problems.

1. For the patch above, it should be explicitly targeted at net-next.
   (Or net if it was a bug fix, which it is not.)

   Not a huge problem, as this is the default. But please keep this in mind
   for future posts.

	Subject: [PATCH vX net-next]: ...

2. For the patch above, the {} should be dropped, as in the patch below.

3. For both patches. The dev_err_cast_probe should be line wrapped,
   and the indentation should align with the opening (.

		return dev_err_cast_probe(dev, clk,
					  "error getting core clock\n");

I'd like to ask you to please negotiate amongst yourselves and
post _just one_ v2 which addresses the minor problems highlighted above.

Thanks!

On Wed, Aug 28, 2024 at 08:15:51PM +0800, Hongbo Li wrote:
> Using dev_err_cast_probe() to simplify the code.
> 
> Signed-off-by: Hongbo Li <lihongbo22@...wei.com>
> ---
>  drivers/net/ipa/ipa_power.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
> index 65fd14da0f86..c572da9e9bc4 100644
> --- a/drivers/net/ipa/ipa_power.c
> +++ b/drivers/net/ipa/ipa_power.c
> @@ -242,11 +242,8 @@ ipa_power_init(struct device *dev, const struct ipa_power_data *data)
>  	int ret;
>  
>  	clk = clk_get(dev, "core");
> -	if (IS_ERR(clk)) {
> -		dev_err_probe(dev, PTR_ERR(clk), "error getting core clock\n");
> -
> -		return ERR_CAST(clk);
> -	}
> +	if (IS_ERR(clk))
> +		return dev_err_cast_probe(dev, clk, "error getting core clock\n");
>  
>  	ret = clk_set_rate(clk, data->core_clock_rate);
>  	if (ret) {

-- 
pw-bot: cr