[PATCH v1] ieee802154: at86rf230: Simplify with dev_err_probe()

[PATCH v1] ieee802154: at86rf230: Simplify with dev_err_probe()

[Shen Lichuan]

Use dev_err_probe() to simplify the error path and unify a message
template.

Using this helper is totally fine even if err is known to never
be -EPROBE_DEFER.

The benefit compared to a normal dev_err() is the standardized format
of the error code, it being emitted symbolically and the fact that
the error code is returned which allows more compact error paths.

Signed-off-by: Shen Lichuan <shenlichuan@vivo.com>
---
 drivers/net/ieee802154/at86rf230.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index f632b0cfd5ae..3fb536734034 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -1532,11 +1532,9 @@ static int at86rf230_probe(struct spi_device *spi)
 
 	rc = device_property_read_u8(&spi->dev, "xtal-trim", &xtal_trim);
 	if (rc < 0) {
-		if (rc != -EINVAL) {
-			dev_err(&spi->dev,
-				"failed to parse xtal-trim: %d\n", rc);
-			return rc;
-		}
+		if (rc != -EINVAL)
+			return dev_err_probe(&spi->dev, rc,
+					     "failed to parse xtal-trim\n");
 		xtal_trim = 0;
 	}
 
@@ -1576,9 +1574,8 @@ static int at86rf230_probe(struct spi_device *spi)
 
 	lp->regmap = devm_regmap_init_spi(spi, &at86rf230_regmap_spi_config);
 	if (IS_ERR(lp->regmap)) {
-		rc = PTR_ERR(lp->regmap);
-		dev_err(&spi->dev, "Failed to allocate register map: %d\n",
-			rc);
+		dev_err_probe(&spi->dev, PTR_ERR(lp->regmap),
+			      "Failed to allocate register map\n");
 		goto free_dev;
 	}
 
-- 
2.17.1

Re: [PATCH v1] ieee802154: at86rf230: Simplify with dev_err_probe()

[Simon Horman]

On Fri, Aug 30, 2024 at 04:14:02PM +0800, Shen Lichuan wrote:
> Use dev_err_probe() to simplify the error path and unify a message
> template.
> 
> Using this helper is totally fine even if err is known to never
> be -EPROBE_DEFER.
> 
> The benefit compared to a normal dev_err() is the standardized format
> of the error code, it being emitted symbolically and the fact that
> the error code is returned which allows more compact error paths.
> 
> Signed-off-by: Shen Lichuan <shenlichuan@vivo.com>

...

> @@ -1576,9 +1574,8 @@ static int at86rf230_probe(struct spi_device *spi)
>  
>  	lp->regmap = devm_regmap_init_spi(spi, &at86rf230_regmap_spi_config);
>  	if (IS_ERR(lp->regmap)) {
> -		rc = PTR_ERR(lp->regmap);
> -		dev_err(&spi->dev, "Failed to allocate register map: %d\n",
> -			rc);
> +		dev_err_probe(&spi->dev, PTR_ERR(lp->regmap),
> +			      "Failed to allocate register map\n");
>  		goto free_dev;

After branching to dev_free the function will return rc.
So I think it still needs to be set a in this error path.

-- 
pw-bot: cr

Re: [PATCH v1] ieee802154: at86rf230: Simplify with dev_err_probe()

[Krzysztof Kozlowski]

On 30/08/2024 18:02, Simon Horman wrote:
> On Fri, Aug 30, 2024 at 04:14:02PM +0800, Shen Lichuan wrote:
>> Use dev_err_probe() to simplify the error path and unify a message
>> template.
>>
>> Using this helper is totally fine even if err is known to never
>> be -EPROBE_DEFER.
>>
>> The benefit compared to a normal dev_err() is the standardized format
>> of the error code, it being emitted symbolically and the fact that
>> the error code is returned which allows more compact error paths.
>>
>> Signed-off-by: Shen Lichuan <shenlichuan@vivo.com>
> 
> ...
> 
>> @@ -1576,9 +1574,8 @@ static int at86rf230_probe(struct spi_device *spi)
>>  
>>  	lp->regmap = devm_regmap_init_spi(spi, &at86rf230_regmap_spi_config);
>>  	if (IS_ERR(lp->regmap)) {
>> -		rc = PTR_ERR(lp->regmap);
>> -		dev_err(&spi->dev, "Failed to allocate register map: %d\n",
>> -			rc);
>> +		dev_err_probe(&spi->dev, PTR_ERR(lp->regmap),
>> +			      "Failed to allocate register map\n");
>>  		goto free_dev;
> 
> After branching to dev_free the function will return rc.
> So I think it still needs to be set a in this error path.

Another bug introduced by @vivo.com.

Since ~2 weeks there is tremendous amount of trivial patches coming from
vivo.com. I identified at least 5 buggy, where the contributor did not
understand the code.

All these "trivial" improvements should be really double-checked.

Best regards,
Krzysztof

Re: [PATCH v1] ieee802154: at86rf230: Simplify with dev_err_probe()

[Simon Horman]

On Fri, Aug 30, 2024 at 07:43:30PM +0200, Krzysztof Kozlowski wrote:
> On 30/08/2024 18:02, Simon Horman wrote:
> > On Fri, Aug 30, 2024 at 04:14:02PM +0800, Shen Lichuan wrote:
> >> Use dev_err_probe() to simplify the error path and unify a message
> >> template.
> >>
> >> Using this helper is totally fine even if err is known to never
> >> be -EPROBE_DEFER.
> >>
> >> The benefit compared to a normal dev_err() is the standardized format
> >> of the error code, it being emitted symbolically and the fact that
> >> the error code is returned which allows more compact error paths.
> >>
> >> Signed-off-by: Shen Lichuan <shenlichuan@vivo.com>
> > 
> > ...
> > 
> >> @@ -1576,9 +1574,8 @@ static int at86rf230_probe(struct spi_device *spi)
> >>  
> >>  	lp->regmap = devm_regmap_init_spi(spi, &at86rf230_regmap_spi_config);
> >>  	if (IS_ERR(lp->regmap)) {
> >> -		rc = PTR_ERR(lp->regmap);
> >> -		dev_err(&spi->dev, "Failed to allocate register map: %d\n",
> >> -			rc);
> >> +		dev_err_probe(&spi->dev, PTR_ERR(lp->regmap),
> >> +			      "Failed to allocate register map\n");
> >>  		goto free_dev;
> > 
> > After branching to dev_free the function will return rc.
> > So I think it still needs to be set a in this error path.
> 
> Another bug introduced by @vivo.com.
> 
> Since ~2 weeks there is tremendous amount of trivial patches coming from
> vivo.com. I identified at least 5 buggy, where the contributor did not
> understand the code.
> 
> All these "trivial" improvements should be really double-checked.

Are you concerned about those that have been accepted?

Re: [PATCH v1] ieee802154: at86rf230: Simplify with dev_err_probe()

[Krzysztof Kozlowski]

On 30/08/2024 20:16, Simon Horman wrote:
> On Fri, Aug 30, 2024 at 07:43:30PM +0200, Krzysztof Kozlowski wrote:
>> On 30/08/2024 18:02, Simon Horman wrote:
>>> On Fri, Aug 30, 2024 at 04:14:02PM +0800, Shen Lichuan wrote:
>>>> Use dev_err_probe() to simplify the error path and unify a message
>>>> template.
>>>>
>>>> Using this helper is totally fine even if err is known to never
>>>> be -EPROBE_DEFER.
>>>>
>>>> The benefit compared to a normal dev_err() is the standardized format
>>>> of the error code, it being emitted symbolically and the fact that
>>>> the error code is returned which allows more compact error paths.
>>>>
>>>> Signed-off-by: Shen Lichuan <shenlichuan@vivo.com>
>>>
>>> ...
>>>
>>>> @@ -1576,9 +1574,8 @@ static int at86rf230_probe(struct spi_device *spi)
>>>>  
>>>>  	lp->regmap = devm_regmap_init_spi(spi, &at86rf230_regmap_spi_config);
>>>>  	if (IS_ERR(lp->regmap)) {
>>>> -		rc = PTR_ERR(lp->regmap);
>>>> -		dev_err(&spi->dev, "Failed to allocate register map: %d\n",
>>>> -			rc);
>>>> +		dev_err_probe(&spi->dev, PTR_ERR(lp->regmap),
>>>> +			      "Failed to allocate register map\n");
>>>>  		goto free_dev;
>>>
>>> After branching to dev_free the function will return rc.
>>> So I think it still needs to be set a in this error path.
>>
>> Another bug introduced by @vivo.com.
>>
>> Since ~2 weeks there is tremendous amount of trivial patches coming from
>> vivo.com. I identified at least 5 buggy, where the contributor did not
>> understand the code.
>>
>> All these "trivial" improvements should be really double-checked.
> 
> Are you concerned about those that have been accepted?

Yes, both posted and accepted. I was doing brief review (amazingly
useless 2 hours...) what's on the list and so far I think there are 6
cases of wrong/malicious dev_err_probe(). One got accepted, I sent a revert:
https://lore.kernel.org/all/20240830170014.15389-1-krzysztof.kozlowski@linaro.org/

But the amount of flood from vivo.com started somehow around 20th of
August (weirdly after I posted set of cleanups and got review from
Jonathan...), is just over-whelming. And many are just ridiculously
split, like converting one dev_err->dev_err_probe in the driver, leaving
rest untouched.

I think this was some sort of trivial automation, thus none of the
patches were actually reviewed before posting.

Best regards,
Krzysztof

Re: [PATCH v1] ieee802154: at86rf230: Simplify with dev_err_probe()

[Simon Horman]

On Fri, Aug 30, 2024 at 08:27:02PM +0200, Krzysztof Kozlowski wrote:
> On 30/08/2024 20:16, Simon Horman wrote:
> > On Fri, Aug 30, 2024 at 07:43:30PM +0200, Krzysztof Kozlowski wrote:
> >> On 30/08/2024 18:02, Simon Horman wrote:
> >>> On Fri, Aug 30, 2024 at 04:14:02PM +0800, Shen Lichuan wrote:
> >>>> Use dev_err_probe() to simplify the error path and unify a message
> >>>> template.
> >>>>
> >>>> Using this helper is totally fine even if err is known to never
> >>>> be -EPROBE_DEFER.
> >>>>
> >>>> The benefit compared to a normal dev_err() is the standardized format
> >>>> of the error code, it being emitted symbolically and the fact that
> >>>> the error code is returned which allows more compact error paths.
> >>>>
> >>>> Signed-off-by: Shen Lichuan <shenlichuan@vivo.com>
> >>>
> >>> ...
> >>>
> >>>> @@ -1576,9 +1574,8 @@ static int at86rf230_probe(struct spi_device *spi)
> >>>>  
> >>>>  	lp->regmap = devm_regmap_init_spi(spi, &at86rf230_regmap_spi_config);
> >>>>  	if (IS_ERR(lp->regmap)) {
> >>>> -		rc = PTR_ERR(lp->regmap);
> >>>> -		dev_err(&spi->dev, "Failed to allocate register map: %d\n",
> >>>> -			rc);
> >>>> +		dev_err_probe(&spi->dev, PTR_ERR(lp->regmap),
> >>>> +			      "Failed to allocate register map\n");
> >>>>  		goto free_dev;
> >>>
> >>> After branching to dev_free the function will return rc.
> >>> So I think it still needs to be set a in this error path.
> >>
> >> Another bug introduced by @vivo.com.
> >>
> >> Since ~2 weeks there is tremendous amount of trivial patches coming from
> >> vivo.com. I identified at least 5 buggy, where the contributor did not
> >> understand the code.
> >>
> >> All these "trivial" improvements should be really double-checked.
> > 
> > Are you concerned about those that have been accepted?
> 
> Yes, both posted and accepted. I was doing brief review (amazingly
> useless 2 hours...) what's on the list and so far I think there are 6
> cases of wrong/malicious dev_err_probe(). One got accepted, I sent a revert:
> https://lore.kernel.org/all/20240830170014.15389-1-krzysztof.kozlowski@linaro.org/

Thanks, I see that.

> But the amount of flood from vivo.com started somehow around 20th of
> August (weirdly after I posted set of cleanups and got review from
> Jonathan...), is just over-whelming. And many are just ridiculously
> split, like converting one dev_err->dev_err_probe in the driver, leaving
> rest untouched.

Yes, I have also noticed a significant number of patches.

> I think this was some sort of trivial automation, thus none of the
> patches were actually reviewed before posting.

Interesting theory.

Re: [PATCH v1] ieee802154: at86rf230: Simplify with dev_err_probe()

[Stefan Schmidt]

Hello Shen,

On 8/30/24 10:14 AM, Shen Lichuan wrote:
> Use dev_err_probe() to simplify the error path and unify a message
> template.
> 
> Using this helper is totally fine even if err is known to never
> be -EPROBE_DEFER.
> 
> The benefit compared to a normal dev_err() is the standardized format
> of the error code, it being emitted symbolically and the fact that
> the error code is returned which allows more compact error paths.
> 
> Signed-off-by: Shen Lichuan <shenlichuan@vivo.com>
> ---
>   drivers/net/ieee802154/at86rf230.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> index f632b0cfd5ae..3fb536734034 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -1532,11 +1532,9 @@ static int at86rf230_probe(struct spi_device *spi)
>   
>   	rc = device_property_read_u8(&spi->dev, "xtal-trim", &xtal_trim);
>   	if (rc < 0) {
> -		if (rc != -EINVAL) {
> -			dev_err(&spi->dev,
> -				"failed to parse xtal-trim: %d\n", rc);
> -			return rc;
> -		}
> +		if (rc != -EINVAL)
> +			return dev_err_probe(&spi->dev, rc,
> +					     "failed to parse xtal-trim\n");
>   		xtal_trim = 0;
>   	}
>   
> @@ -1576,9 +1574,8 @@ static int at86rf230_probe(struct spi_device *spi)
>   
>   	lp->regmap = devm_regmap_init_spi(spi, &at86rf230_regmap_spi_config);
>   	if (IS_ERR(lp->regmap)) {
> -		rc = PTR_ERR(lp->regmap);
> -		dev_err(&spi->dev, "Failed to allocate register map: %d\n",
> -			rc);
> +		dev_err_probe(&spi->dev, PTR_ERR(lp->regmap),
> +			      "Failed to allocate register map\n");
>   		goto free_dev;
>   	}
>   

Please take the review from Simon into account here. Dropped this 
version of the patch from the wpan patchwork queue.

regards
Stefan Schmidt