| Message ID | 20240830081402.21716-1-shenlichuan@vivo.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v1] ieee802154: at86rf230: Simplify with dev_err_probe() | expand |
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.
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
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?
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
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.
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
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; }
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(-)