| Message ID | 20200829064726.26268-8-krzk@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,01/18] iio: accel: bma180: Simplify with dev_err_probe() | expand |
On Sat, 29 Aug 2020 08:47:16 +0200 Krzysztof Kozlowski <krzk@kernel.org> wrote: > Common pattern of handling deferred probe can be simplified with > dev_err_probe(). Less code and also it prints the error value. > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > I don't have the thread to hand, but this tripped a warning next and the patch was dropped as a result. See below. Jonathan > --- > > Changes since v2: > 1. Wrap dev_err_probe() lines at 80 character > > Changes since v1: > 1. Convert to devm_clk_get_optional > 2. Update also stm32-dfsdm-core and stm32-dac-core. > 3. Wrap around 100 characters (accepted by checkpatch). > --- > drivers/iio/adc/stm32-adc-core.c | 75 ++++++++++-------------------- > drivers/iio/adc/stm32-adc.c | 10 ++-- > drivers/iio/adc/stm32-dfsdm-adc.c | 10 ++-- > drivers/iio/adc/stm32-dfsdm-core.c | 9 ++-- > drivers/iio/dac/stm32-dac-core.c | 5 +- > 5 files changed, 35 insertions(+), 74 deletions(-) > > diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c > index 0e2068ec068b..3f27b4817a42 100644 > --- a/drivers/iio/adc/stm32-adc-core.c > +++ b/drivers/iio/adc/stm32-adc-core.c > @@ -582,11 +582,9 @@ static int stm32_adc_core_switches_probe(struct device *dev, > priv->syscfg = syscon_regmap_lookup_by_phandle(np, "st,syscfg"); > if (IS_ERR(priv->syscfg)) { > ret = PTR_ERR(priv->syscfg); > - if (ret != -ENODEV) { > - if (ret != -EPROBE_DEFER) > - dev_err(dev, "Can't probe syscfg: %d\n", ret); > - return ret; > - } > + if (ret != -ENODEV) > + return dev_err_probe(dev, ret, "Can't probe syscfg\n"); > + > priv->syscfg = NULL; > } > > @@ -596,12 +594,9 @@ static int stm32_adc_core_switches_probe(struct device *dev, > priv->booster = devm_regulator_get_optional(dev, "booster"); > if (IS_ERR(priv->booster)) { > ret = PTR_ERR(priv->booster); > - if (ret != -ENODEV) { > - if (ret != -EPROBE_DEFER) > - dev_err(dev, "can't get booster %d\n", > - ret); > - return ret; > - } > + if (ret != -ENODEV) > + dev_err_probe(dev, ret, "can't get booster\n"); This tripped a warning and got the patch dropped because we no longer return on error. > + > priv->booster = NULL; > } > } > @@ -612,11 +607,9 @@ static int stm32_adc_core_switches_probe(struct device *dev, > priv->vdd = devm_regulator_get_optional(dev, "vdd"); > if (IS_ERR(priv->vdd)) { > ret = PTR_ERR(priv->vdd); > - if (ret != -ENODEV) { > - if (ret != -EPROBE_DEFER) > - dev_err(dev, "can't get vdd %d\n", ret); > - return ret; > - } > + if (ret != -ENODEV) > + return dev_err_probe(dev, ret, "can't get vdd\n"); > + > priv->vdd = NULL; > } > } > @@ -669,42 +662,24 @@ static int stm32_adc_probe(struct platform_device *pdev) > priv->common.phys_base = res->start; > > priv->vdda = devm_regulator_get(&pdev->dev, "vdda"); > - if (IS_ERR(priv->vdda)) { > - ret = PTR_ERR(priv->vdda); > - if (ret != -EPROBE_DEFER) > - dev_err(&pdev->dev, "vdda get failed, %d\n", ret); > - return ret; > - } > + if (IS_ERR(priv->vdda)) > + return dev_err_probe(&pdev->dev, PTR_ERR(priv->vdda), > + "vdda get failed\n"); > > priv->vref = devm_regulator_get(&pdev->dev, "vref"); > - if (IS_ERR(priv->vref)) { > - ret = PTR_ERR(priv->vref); > - if (ret != -EPROBE_DEFER) > - dev_err(&pdev->dev, "vref get failed, %d\n", ret); > - return ret; > - } > - > - priv->aclk = devm_clk_get(&pdev->dev, "adc"); > - if (IS_ERR(priv->aclk)) { > - ret = PTR_ERR(priv->aclk); > - if (ret != -ENOENT) { > - if (ret != -EPROBE_DEFER) > - dev_err(&pdev->dev, "Can't get 'adc' clock\n"); > - return ret; > - } > - priv->aclk = NULL; > - } > - > - priv->bclk = devm_clk_get(&pdev->dev, "bus"); > - if (IS_ERR(priv->bclk)) { > - ret = PTR_ERR(priv->bclk); > - if (ret != -ENOENT) { > - if (ret != -EPROBE_DEFER) > - dev_err(&pdev->dev, "Can't get 'bus' clock\n"); > - return ret; > - } > - priv->bclk = NULL; > - } > + if (IS_ERR(priv->vref)) > + return dev_err_probe(&pdev->dev, PTR_ERR(priv->vref), > + "vref get failed\n"); > + > + priv->aclk = devm_clk_get_optional(&pdev->dev, "adc"); > + if (IS_ERR(priv->aclk)) > + return dev_err_probe(&pdev->dev, PTR_ERR(priv->aclk), > + "Can't get 'adc' clock\n"); > + > + priv->bclk = devm_clk_get_optional(&pdev->dev, "bus"); > + if (IS_ERR(priv->bclk)) > + return dev_err_probe(&pdev->dev, PTR_ERR(priv->bclk), > + "Can't get 'bus' clock\n"); > > ret = stm32_adc_core_switches_probe(dev, priv); > if (ret) > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c > index 3eb9ebe8372f..b3f31f147347 100644 > --- a/drivers/iio/adc/stm32-adc.c > +++ b/drivers/iio/adc/stm32-adc.c > @@ -1805,13 +1805,9 @@ static int stm32_adc_dma_request(struct device *dev, struct iio_dev *indio_dev) > adc->dma_chan = dma_request_chan(dev, "rx"); > if (IS_ERR(adc->dma_chan)) { > ret = PTR_ERR(adc->dma_chan); > - if (ret != -ENODEV) { > - if (ret != -EPROBE_DEFER) > - dev_err(dev, > - "DMA channel request failed with %d\n", > - ret); > - return ret; > - } > + if (ret != -ENODEV) > + return dev_err_probe(dev, ret, > + "DMA channel request failed with\n"); > > /* DMA is optional: fall back to IRQ mode */ > adc->dma_chan = NULL; > diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c > index 5e10fb4f3704..c7e0109315f8 100644 > --- a/drivers/iio/adc/stm32-dfsdm-adc.c > +++ b/drivers/iio/adc/stm32-dfsdm-adc.c > @@ -1473,13 +1473,9 @@ static int stm32_dfsdm_adc_init(struct device *dev, struct iio_dev *indio_dev) > /* Optionally request DMA */ > ret = stm32_dfsdm_dma_request(dev, indio_dev); > if (ret) { > - if (ret != -ENODEV) { > - if (ret != -EPROBE_DEFER) > - dev_err(dev, > - "DMA channel request failed with %d\n", > - ret); > - return ret; > - } > + if (ret != -ENODEV) > + return dev_err_probe(dev, ret, > + "DMA channel request failed with\n"); > > dev_dbg(dev, "No DMA support\n"); > return 0; > diff --git a/drivers/iio/adc/stm32-dfsdm-core.c b/drivers/iio/adc/stm32-dfsdm-core.c > index 26e2011c5868..0b8bea88b011 100644 > --- a/drivers/iio/adc/stm32-dfsdm-core.c > +++ b/drivers/iio/adc/stm32-dfsdm-core.c > @@ -243,12 +243,9 @@ static int stm32_dfsdm_parse_of(struct platform_device *pdev, > * on use case. > */ > priv->clk = devm_clk_get(&pdev->dev, "dfsdm"); > - if (IS_ERR(priv->clk)) { > - ret = PTR_ERR(priv->clk); > - if (ret != -EPROBE_DEFER) > - dev_err(&pdev->dev, "Failed to get clock (%d)\n", ret); > - return ret; > - } > + if (IS_ERR(priv->clk)) > + return dev_err_probe(&pdev->dev, PTR_ERR(priv->clk), > + "Failed to get clock\n"); > > priv->aclk = devm_clk_get(&pdev->dev, "audio"); > if (IS_ERR(priv->aclk)) > diff --git a/drivers/iio/dac/stm32-dac-core.c b/drivers/iio/dac/stm32-dac-core.c > index 7e5809ba0dee..906436780347 100644 > --- a/drivers/iio/dac/stm32-dac-core.c > +++ b/drivers/iio/dac/stm32-dac-core.c > @@ -150,10 +150,7 @@ static int stm32_dac_probe(struct platform_device *pdev) > rst = devm_reset_control_get_optional_exclusive(dev, NULL); > if (rst) { > if (IS_ERR(rst)) { > - ret = PTR_ERR(rst); > - if (ret != -EPROBE_DEFER) > - dev_err(dev, "reset get failed, %d\n", ret); > - > + ret = dev_err_probe(dev, PTR_ERR(rst), "reset get failed\n"); > goto err_hw_stop; > } >
On Wed, 9 Sep 2020 at 20:36, Jonathan Cameron <jic23@kernel.org> wrote: > > On Sat, 29 Aug 2020 08:47:16 +0200 > Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > Common pattern of handling deferred probe can be simplified with > > dev_err_probe(). Less code and also it prints the error value. > > > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > I don't have the thread to hand, but this tripped a warning next > and the patch was dropped as a result. See below. Thanks for letting me know. If you mean the warning caused by: https://lore.kernel.org/lkml/20200909073716.GA560912@kroah.com/ then the driver-core patch was dropped, not the iio one: https://lore.kernel.org/linux-next/20200909074130.GB561485@kroah.com/T/#t So we are good here :) Best regards, Krzysztof > Jonathan > > --- > > > > Changes since v2: > > 1. Wrap dev_err_probe() lines at 80 character > > > > Changes since v1: > > 1. Convert to devm_clk_get_optional > > 2. Update also stm32-dfsdm-core and stm32-dac-core. > > 3. Wrap around 100 characters (accepted by checkpatch). > > --- > > drivers/iio/adc/stm32-adc-core.c | 75 ++++++++++-------------------- > > drivers/iio/adc/stm32-adc.c | 10 ++-- > > drivers/iio/adc/stm32-dfsdm-adc.c | 10 ++-- > > drivers/iio/adc/stm32-dfsdm-core.c | 9 ++-- > > drivers/iio/dac/stm32-dac-core.c | 5 +- > > 5 files changed, 35 insertions(+), 74 deletions(-) > > > > diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c > > index 0e2068ec068b..3f27b4817a42 100644 > > --- a/drivers/iio/adc/stm32-adc-core.c > > +++ b/drivers/iio/adc/stm32-adc-core.c > > @@ -582,11 +582,9 @@ static int stm32_adc_core_switches_probe(struct device *dev, > > priv->syscfg = syscon_regmap_lookup_by_phandle(np, "st,syscfg"); > > if (IS_ERR(priv->syscfg)) { > > ret = PTR_ERR(priv->syscfg); > > - if (ret != -ENODEV) { > > - if (ret != -EPROBE_DEFER) > > - dev_err(dev, "Can't probe syscfg: %d\n", ret); > > - return ret; > > - } > > + if (ret != -ENODEV) > > + return dev_err_probe(dev, ret, "Can't probe syscfg\n"); > > + > > priv->syscfg = NULL; > > } > > > > @@ -596,12 +594,9 @@ static int stm32_adc_core_switches_probe(struct device *dev, > > priv->booster = devm_regulator_get_optional(dev, "booster"); > > if (IS_ERR(priv->booster)) { > > ret = PTR_ERR(priv->booster); > > - if (ret != -ENODEV) { > > - if (ret != -EPROBE_DEFER) > > - dev_err(dev, "can't get booster %d\n", > > - ret); > > - return ret; > > - } > > + if (ret != -ENODEV) > > + dev_err_probe(dev, ret, "can't get booster\n"); > > This tripped a warning and got the patch dropped because we no longer > return on error.
Hi! On 2020-09-09 21:57, Krzysztof Kozlowski wrote: > On Wed, 9 Sep 2020 at 20:36, Jonathan Cameron <jic23@kernel.org> wrote: >> >> On Sat, 29 Aug 2020 08:47:16 +0200 >> Krzysztof Kozlowski <krzk@kernel.org> wrote: >> >>> Common pattern of handling deferred probe can be simplified with >>> dev_err_probe(). Less code and also it prints the error value. >>> >>> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> >>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >>> >> I don't have the thread to hand, but this tripped a warning next >> and the patch was dropped as a result. See below. > > Thanks for letting me know. If you mean the warning caused by: > https://lore.kernel.org/lkml/20200909073716.GA560912@kroah.com/ > then the driver-core patch was dropped, not the iio one: > https://lore.kernel.org/linux-next/20200909074130.GB561485@kroah.com/T/#t > > So we are good here :) No, we are definitely not good. See below. That means "See below", and not "Please take a guess at what is being talking about". > Best regards, > Krzysztof > >> Jonathan >>> --- >>> >>> Changes since v2: >>> 1. Wrap dev_err_probe() lines at 80 character >>> >>> Changes since v1: >>> 1. Convert to devm_clk_get_optional >>> 2. Update also stm32-dfsdm-core and stm32-dac-core. >>> 3. Wrap around 100 characters (accepted by checkpatch). >>> --- >>> drivers/iio/adc/stm32-adc-core.c | 75 ++++++++++-------------------- >>> drivers/iio/adc/stm32-adc.c | 10 ++-- >>> drivers/iio/adc/stm32-dfsdm-adc.c | 10 ++-- >>> drivers/iio/adc/stm32-dfsdm-core.c | 9 ++-- >>> drivers/iio/dac/stm32-dac-core.c | 5 +- >>> 5 files changed, 35 insertions(+), 74 deletions(-) >>> >>> diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c >>> index 0e2068ec068b..3f27b4817a42 100644 >>> --- a/drivers/iio/adc/stm32-adc-core.c >>> +++ b/drivers/iio/adc/stm32-adc-core.c >>> @@ -582,11 +582,9 @@ static int stm32_adc_core_switches_probe(struct device *dev, >>> priv->syscfg = syscon_regmap_lookup_by_phandle(np, "st,syscfg"); >>> if (IS_ERR(priv->syscfg)) { >>> ret = PTR_ERR(priv->syscfg); >>> - if (ret != -ENODEV) { >>> - if (ret != -EPROBE_DEFER) >>> - dev_err(dev, "Can't probe syscfg: %d\n", ret); >>> - return ret; >>> - } >>> + if (ret != -ENODEV) >>> + return dev_err_probe(dev, ret, "Can't probe syscfg\n"); >>> + >>> priv->syscfg = NULL; >>> } >>> >>> @@ -596,12 +594,9 @@ static int stm32_adc_core_switches_probe(struct device *dev, >>> priv->booster = devm_regulator_get_optional(dev, "booster"); >>> if (IS_ERR(priv->booster)) { >>> ret = PTR_ERR(priv->booster); >>> - if (ret != -ENODEV) { >>> - if (ret != -EPROBE_DEFER) >>> - dev_err(dev, "can't get booster %d\n", >>> - ret); >>> - return ret; >>> - } >>> + if (ret != -ENODEV) >>> + dev_err_probe(dev, ret, "can't get booster\n"); >> >> This tripped a warning and got the patch dropped because we no longer >> return on error. As Jonathan already said, we no longer return in this hunk. I.e., you have clobbered the error path. Cheers, Peter
On Thu, 10 Sep 2020 at 08:52, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Thursday, September 10, 2020, Peter Rosin <peda@axentia.se> wrote: >> >> Hi! >> >> On 2020-09-09 21:57, Krzysztof Kozlowski wrote: >> > On Wed, 9 Sep 2020 at 20:36, Jonathan Cameron <jic23@kernel.org> wrote: >> >> >> >> On Sat, 29 Aug 2020 08:47:16 +0200 >> >> Krzysztof Kozlowski <krzk@kernel.org> wrote: >> >> >> >>> Common pattern of handling deferred probe can be simplified with >> >>> dev_err_probe(). Less code and also it prints the error value. >> >>> >> >>> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> >> >>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >> >>> >> >> I don't have the thread to hand, but this tripped a warning next >> >> and the patch was dropped as a result. See below. >> > >> > Thanks for letting me know. If you mean the warning caused by: >> > https://lore.kernel.org/lkml/20200909073716.GA560912@kroah.com/ >> > then the driver-core patch was dropped, not the iio one: >> > https://lore.kernel.org/linux-next/20200909074130.GB561485@kroah.com/T/#t >> > >> > So we are good here :) >> >> No, we are definitely not good. See below. That means "See below", and >> not "Please take a guess at what is being talking about". > > > >> >> >>> @@ -596,12 +594,9 @@ static int stm32_adc_core_switches_probe(struct device *dev, >> >>> priv->booster = devm_regulator_get_optional(dev, "booster"); >> >>> if (IS_ERR(priv->booster)) { >> >>> ret = PTR_ERR(priv->booster); >> >>> - if (ret != -ENODEV) { >> >>> - if (ret != -EPROBE_DEFER) >> >>> - dev_err(dev, "can't get booster %d\n", >> >>> - ret); >> >>> - return ret; >> >>> - } >> >>> + if (ret != -ENODEV) >> >>> + dev_err_probe(dev, ret, "can't get booster\n"); >> >> >> >> This tripped a warning and got the patch dropped because we no longer >> >> return on error. >> >> As Jonathan already said, we no longer return in this hunk. I.e., you have >> clobbered the error path. > > > Exactly my point why I proposed _must_check in the first place. That was not exactly that point as you did not mention possible errors but only "miss the opportunity to optimize". Optimization is different things than a mistake. Best regards, Krzysztof
On Thu, 10 Sep 2020 08:58:57 +0200 Krzysztof Kozlowski <krzk@kernel.org> wrote: > On Thu, 10 Sep 2020 at 08:52, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > > On Thursday, September 10, 2020, Peter Rosin <peda@axentia.se> wrote: > >> > >> Hi! > >> > >> On 2020-09-09 21:57, Krzysztof Kozlowski wrote: > >> > On Wed, 9 Sep 2020 at 20:36, Jonathan Cameron <jic23@kernel.org> wrote: > >> >> > >> >> On Sat, 29 Aug 2020 08:47:16 +0200 > >> >> Krzysztof Kozlowski <krzk@kernel.org> wrote: > >> >> > >> >>> Common pattern of handling deferred probe can be simplified with > >> >>> dev_err_probe(). Less code and also it prints the error value. > >> >>> > >> >>> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > >> >>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > >> >>> > >> >> I don't have the thread to hand, but this tripped a warning next > >> >> and the patch was dropped as a result. See below. oops. That is what I get for reading an email very quickly then looking at the code a few hours later. Still a problem here we need to fix unless I'm missing something. > >> > > >> > Thanks for letting me know. If you mean the warning caused by: > >> > https://lore.kernel.org/lkml/20200909073716.GA560912@kroah.com/ > >> > then the driver-core patch was dropped, not the iio one: > >> > https://lore.kernel.org/linux-next/20200909074130.GB561485@kroah.com/T/#t > >> > > >> > So we are good here :) > >> > >> No, we are definitely not good. See below. That means "See below", and > >> not "Please take a guess at what is being talking about". > > > > > > > >> > >> >>> @@ -596,12 +594,9 @@ static int stm32_adc_core_switches_probe(struct device *dev, > >> >>> priv->booster = devm_regulator_get_optional(dev, "booster"); > >> >>> if (IS_ERR(priv->booster)) { > >> >>> ret = PTR_ERR(priv->booster); > >> >>> - if (ret != -ENODEV) { > >> >>> - if (ret != -EPROBE_DEFER) > >> >>> - dev_err(dev, "can't get booster %d\n", > >> >>> - ret); > >> >>> - return ret; > >> >>> - } > >> >>> + if (ret != -ENODEV) > >> >>> + dev_err_probe(dev, ret, "can't get booster\n"); > >> >> > >> >> This tripped a warning and got the patch dropped because we no longer > >> >> return on error. > >> > >> As Jonathan already said, we no longer return in this hunk. I.e., you have > >> clobbered the error path. > > > > > > Exactly my point why I proposed _must_check in the first place. > > That was not exactly that point as you did not mention possible errors > but only "miss the opportunity to optimize". Optimization is different > things than a mistake. In this particular case we have introduced a bug. If the regulator returns an error other than -ENODEV we will carry on when really should error out. This includes deferred probe route in which it won't print a message but also won't actually defer. Jonathan > > Best regards, > Krzysztof > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, 10 Sep 2020 at 10:13, Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Thu, 10 Sep 2020 08:58:57 +0200 > Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > On Thu, 10 Sep 2020 at 08:52, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > > > > > > On Thursday, September 10, 2020, Peter Rosin <peda@axentia.se> wrote: > > >> > > >> Hi! > > >> > > >> On 2020-09-09 21:57, Krzysztof Kozlowski wrote: > > >> > On Wed, 9 Sep 2020 at 20:36, Jonathan Cameron <jic23@kernel.org> wrote: > > >> >> > > >> >> On Sat, 29 Aug 2020 08:47:16 +0200 > > >> >> Krzysztof Kozlowski <krzk@kernel.org> wrote: > > >> >> > > >> >>> Common pattern of handling deferred probe can be simplified with > > >> >>> dev_err_probe(). Less code and also it prints the error value. > > >> >>> > > >> >>> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > >> >>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > >> >>> > > >> >> I don't have the thread to hand, but this tripped a warning next > > >> >> and the patch was dropped as a result. See below. > > oops. That is what I get for reading an email very quickly then looking > at the code a few hours later. Still a problem here we need to fix > unless I'm missing something. > > > >> > > > >> > Thanks for letting me know. If you mean the warning caused by: > > >> > https://lore.kernel.org/lkml/20200909073716.GA560912@kroah.com/ > > >> > then the driver-core patch was dropped, not the iio one: > > >> > https://lore.kernel.org/linux-next/20200909074130.GB561485@kroah.com/T/#t > > >> > > > >> > So we are good here :) > > >> > > >> No, we are definitely not good. See below. That means "See below", and > > >> not "Please take a guess at what is being talking about". > > > > > > > > > > > >> > > >> >>> @@ -596,12 +594,9 @@ static int stm32_adc_core_switches_probe(struct device *dev, > > >> >>> priv->booster = devm_regulator_get_optional(dev, "booster"); > > >> >>> if (IS_ERR(priv->booster)) { > > >> >>> ret = PTR_ERR(priv->booster); > > >> >>> - if (ret != -ENODEV) { > > >> >>> - if (ret != -EPROBE_DEFER) > > >> >>> - dev_err(dev, "can't get booster %d\n", > > >> >>> - ret); > > >> >>> - return ret; > > >> >>> - } > > >> >>> + if (ret != -ENODEV) > > >> >>> + dev_err_probe(dev, ret, "can't get booster\n"); > > >> >> > > >> >> This tripped a warning and got the patch dropped because we no longer > > >> >> return on error. > > >> > > >> As Jonathan already said, we no longer return in this hunk. I.e., you have > > >> clobbered the error path. > > > > > > > > > Exactly my point why I proposed _must_check in the first place. > > > > That was not exactly that point as you did not mention possible errors > > but only "miss the opportunity to optimize". Optimization is different > > things than a mistake. > > In this particular case we have introduced a bug. If the regulator returns > an error other than -ENODEV we will carry on when really should error out. > This includes deferred probe route in which it won't print a message but also won't > actually defer. Yes, I see, Peter pointed this out. The commit was actually not dropped from next so I will send a fixup. Best regards, Krzysztof
On Thu, Sep 10, 2020 at 9:59 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > On Thu, 10 Sep 2020 at 08:52, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thursday, September 10, 2020, Peter Rosin <peda@axentia.se> wrote: > >> On 2020-09-09 21:57, Krzysztof Kozlowski wrote: > >> > On Wed, 9 Sep 2020 at 20:36, Jonathan Cameron <jic23@kernel.org> wrote: > >> >> On Sat, 29 Aug 2020 08:47:16 +0200 > >> >> Krzysztof Kozlowski <krzk@kernel.org> wrote: ... > >> >>> @@ -596,12 +594,9 @@ static int stm32_adc_core_switches_probe(struct device *dev, > >> >>> priv->booster = devm_regulator_get_optional(dev, "booster"); > >> >>> if (IS_ERR(priv->booster)) { > >> >>> ret = PTR_ERR(priv->booster); > >> >>> - if (ret != -ENODEV) { > >> >>> - if (ret != -EPROBE_DEFER) > >> >>> - dev_err(dev, "can't get booster %d\n", > >> >>> - ret); > >> >>> - return ret; > >> >>> - } > >> >>> + if (ret != -ENODEV) > >> >>> + dev_err_probe(dev, ret, "can't get booster\n"); > >> >> > >> >> This tripped a warning and got the patch dropped because we no longer > >> >> return on error. > >> > >> As Jonathan already said, we no longer return in this hunk. I.e., you have > >> clobbered the error path. > > > > > > Exactly my point why I proposed _must_check in the first place. > > That was not exactly that point as you did not mention possible errors > but only "miss the opportunity to optimize". Optimization is different > things than a mistake. Yes, and that's what happened here. You missed optimization which led to an error. And this is a good showcase to see how dev_err_probe() may be misused because of overlooking subtle details. Perhaps we can do static inline __must_check dev_err_probe_ret(...) { return dev_err_probe(...); } (or other way around, introduce dev_err_probe_noret(), yes, name sucks)
diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c index 0e2068ec068b..3f27b4817a42 100644 --- a/drivers/iio/adc/stm32-adc-core.c +++ b/drivers/iio/adc/stm32-adc-core.c @@ -582,11 +582,9 @@ static int stm32_adc_core_switches_probe(struct device *dev, priv->syscfg = syscon_regmap_lookup_by_phandle(np, "st,syscfg"); if (IS_ERR(priv->syscfg)) { ret = PTR_ERR(priv->syscfg); - if (ret != -ENODEV) { - if (ret != -EPROBE_DEFER) - dev_err(dev, "Can't probe syscfg: %d\n", ret); - return ret; - } + if (ret != -ENODEV) + return dev_err_probe(dev, ret, "Can't probe syscfg\n"); + priv->syscfg = NULL; } @@ -596,12 +594,9 @@ static int stm32_adc_core_switches_probe(struct device *dev, priv->booster = devm_regulator_get_optional(dev, "booster"); if (IS_ERR(priv->booster)) { ret = PTR_ERR(priv->booster); - if (ret != -ENODEV) { - if (ret != -EPROBE_DEFER) - dev_err(dev, "can't get booster %d\n", - ret); - return ret; - } + if (ret != -ENODEV) + dev_err_probe(dev, ret, "can't get booster\n"); + priv->booster = NULL; } } @@ -612,11 +607,9 @@ static int stm32_adc_core_switches_probe(struct device *dev, priv->vdd = devm_regulator_get_optional(dev, "vdd"); if (IS_ERR(priv->vdd)) { ret = PTR_ERR(priv->vdd); - if (ret != -ENODEV) { - if (ret != -EPROBE_DEFER) - dev_err(dev, "can't get vdd %d\n", ret); - return ret; - } + if (ret != -ENODEV) + return dev_err_probe(dev, ret, "can't get vdd\n"); + priv->vdd = NULL; } } @@ -669,42 +662,24 @@ static int stm32_adc_probe(struct platform_device *pdev) priv->common.phys_base = res->start; priv->vdda = devm_regulator_get(&pdev->dev, "vdda"); - if (IS_ERR(priv->vdda)) { - ret = PTR_ERR(priv->vdda); - if (ret != -EPROBE_DEFER) - dev_err(&pdev->dev, "vdda get failed, %d\n", ret); - return ret; - } + if (IS_ERR(priv->vdda)) + return dev_err_probe(&pdev->dev, PTR_ERR(priv->vdda), + "vdda get failed\n"); priv->vref = devm_regulator_get(&pdev->dev, "vref"); - if (IS_ERR(priv->vref)) { - ret = PTR_ERR(priv->vref); - if (ret != -EPROBE_DEFER) - dev_err(&pdev->dev, "vref get failed, %d\n", ret); - return ret; - } - - priv->aclk = devm_clk_get(&pdev->dev, "adc"); - if (IS_ERR(priv->aclk)) { - ret = PTR_ERR(priv->aclk); - if (ret != -ENOENT) { - if (ret != -EPROBE_DEFER) - dev_err(&pdev->dev, "Can't get 'adc' clock\n"); - return ret; - } - priv->aclk = NULL; - } - - priv->bclk = devm_clk_get(&pdev->dev, "bus"); - if (IS_ERR(priv->bclk)) { - ret = PTR_ERR(priv->bclk); - if (ret != -ENOENT) { - if (ret != -EPROBE_DEFER) - dev_err(&pdev->dev, "Can't get 'bus' clock\n"); - return ret; - } - priv->bclk = NULL; - } + if (IS_ERR(priv->vref)) + return dev_err_probe(&pdev->dev, PTR_ERR(priv->vref), + "vref get failed\n"); + + priv->aclk = devm_clk_get_optional(&pdev->dev, "adc"); + if (IS_ERR(priv->aclk)) + return dev_err_probe(&pdev->dev, PTR_ERR(priv->aclk), + "Can't get 'adc' clock\n"); + + priv->bclk = devm_clk_get_optional(&pdev->dev, "bus"); + if (IS_ERR(priv->bclk)) + return dev_err_probe(&pdev->dev, PTR_ERR(priv->bclk), + "Can't get 'bus' clock\n"); ret = stm32_adc_core_switches_probe(dev, priv); if (ret) diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c index 3eb9ebe8372f..b3f31f147347 100644 --- a/drivers/iio/adc/stm32-adc.c +++ b/drivers/iio/adc/stm32-adc.c @@ -1805,13 +1805,9 @@ static int stm32_adc_dma_request(struct device *dev, struct iio_dev *indio_dev) adc->dma_chan = dma_request_chan(dev, "rx"); if (IS_ERR(adc->dma_chan)) { ret = PTR_ERR(adc->dma_chan); - if (ret != -ENODEV) { - if (ret != -EPROBE_DEFER) - dev_err(dev, - "DMA channel request failed with %d\n", - ret); - return ret; - } + if (ret != -ENODEV) + return dev_err_probe(dev, ret, + "DMA channel request failed with\n"); /* DMA is optional: fall back to IRQ mode */ adc->dma_chan = NULL; diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c index 5e10fb4f3704..c7e0109315f8 100644 --- a/drivers/iio/adc/stm32-dfsdm-adc.c +++ b/drivers/iio/adc/stm32-dfsdm-adc.c @@ -1473,13 +1473,9 @@ static int stm32_dfsdm_adc_init(struct device *dev, struct iio_dev *indio_dev) /* Optionally request DMA */ ret = stm32_dfsdm_dma_request(dev, indio_dev); if (ret) { - if (ret != -ENODEV) { - if (ret != -EPROBE_DEFER) - dev_err(dev, - "DMA channel request failed with %d\n", - ret); - return ret; - } + if (ret != -ENODEV) + return dev_err_probe(dev, ret, + "DMA channel request failed with\n"); dev_dbg(dev, "No DMA support\n"); return 0; diff --git a/drivers/iio/adc/stm32-dfsdm-core.c b/drivers/iio/adc/stm32-dfsdm-core.c index 26e2011c5868..0b8bea88b011 100644 --- a/drivers/iio/adc/stm32-dfsdm-core.c +++ b/drivers/iio/adc/stm32-dfsdm-core.c @@ -243,12 +243,9 @@ static int stm32_dfsdm_parse_of(struct platform_device *pdev, * on use case. */ priv->clk = devm_clk_get(&pdev->dev, "dfsdm"); - if (IS_ERR(priv->clk)) { - ret = PTR_ERR(priv->clk); - if (ret != -EPROBE_DEFER) - dev_err(&pdev->dev, "Failed to get clock (%d)\n", ret); - return ret; - } + if (IS_ERR(priv->clk)) + return dev_err_probe(&pdev->dev, PTR_ERR(priv->clk), + "Failed to get clock\n"); priv->aclk = devm_clk_get(&pdev->dev, "audio"); if (IS_ERR(priv->aclk)) diff --git a/drivers/iio/dac/stm32-dac-core.c b/drivers/iio/dac/stm32-dac-core.c index 7e5809ba0dee..906436780347 100644 --- a/drivers/iio/dac/stm32-dac-core.c +++ b/drivers/iio/dac/stm32-dac-core.c @@ -150,10 +150,7 @@ static int stm32_dac_probe(struct platform_device *pdev) rst = devm_reset_control_get_optional_exclusive(dev, NULL); if (rst) { if (IS_ERR(rst)) { - ret = PTR_ERR(rst); - if (ret != -EPROBE_DEFER) - dev_err(dev, "reset get failed, %d\n", ret); - + ret = dev_err_probe(dev, PTR_ERR(rst), "reset get failed\n"); goto err_hw_stop; }