[PATCH v2 1/3] iio: humidity: dht11: Don't warn on memory allocation failure

[PATCH v2 1/3] iio: humidity: dht11: Don't warn on memory allocation failure

[Uwe Kleine-König]

The kernel is already quite noisy if a memory allocation fails. So it's
the usual policy to not add another message on the driver level.

Drop the error message accordingly.

Signed-off-by: Uwe Kleine-König <ukleinek@debian.org>
---
Unchanged since (implicit) v1

 drivers/iio/humidity/dht11.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index c97e25448772..891b6bf0b4ca 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -295,10 +295,8 @@ static int dht11_probe(struct platform_device *pdev)
 	struct iio_dev *iio;
 
 	iio = devm_iio_device_alloc(dev, sizeof(*dht11));
-	if (!iio) {
-		dev_err(dev, "Failed to allocate IIO device\n");
+	if (!iio)
 		return -ENOMEM;
-	}
 
 	dht11 = iio_priv(iio);
 	dht11->dev = dev;

base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
-- 
2.36.1

[PATCH v2 2/3] iio: humidity: dht11: Emit error messages for probe failures

[Uwe Kleine-König]

There are two exit points in the driver's probe function that fail
silently. From a user perspective this is unsatisfactory because the
device is unusable but there is no hint in the kernel log about the
actual problem which makes it unnecessarily hard to fix the problem.

Make use of dev_err_probe() to emit a problem indication which also does
the right thing if requesting the gpio return -EPROBE_DEFER.

Signed-off-by: Uwe Kleine-König <ukleinek@debian.org>
---
Changes since (implicit) v1:

 - Make it actually compile. (It helps quite a lot to have the driver to
   be tested enabled in the config when doing compile tests *sigh*)
 - Fix a typo I added when manually splitting the original patch

 drivers/iio/humidity/dht11.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 891b6bf0b4ca..0db4f7471319 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -293,6 +293,7 @@ static int dht11_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct dht11 *dht11;
 	struct iio_dev *iio;
+	int ret;
 
 	iio = devm_iio_device_alloc(dev, sizeof(*dht11));
 	if (!iio)
@@ -302,7 +303,8 @@ static int dht11_probe(struct platform_device *pdev)
 	dht11->dev = dev;
 	dht11->gpiod = devm_gpiod_get(dev, NULL, GPIOD_IN);
 	if (IS_ERR(dht11->gpiod))
-		return PTR_ERR(dht11->gpiod);
+		return dev_err_probe(dev, PTR_ERR(dht11->gpiod),
+				     "Failed to acquire GPIO\n");
 
 	dht11->irq = gpiod_to_irq(dht11->gpiod);
 	if (dht11->irq < 0) {
@@ -323,7 +325,11 @@ static int dht11_probe(struct platform_device *pdev)
 	iio->channels = dht11_chan_spec;
 	iio->num_channels = ARRAY_SIZE(dht11_chan_spec);
 
-	return devm_iio_device_register(dev, iio);
+	ret = devm_iio_device_register(dev, iio);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to register iio device\n");
+
+	return 0;
 }
 
 static struct platform_driver dht11_driver = {
-- 
2.36.1

[PATCH v2 3/3] iio: humidity: dht11: Use dev_err_probe consistently

[Uwe Kleine-König]

All but one error path use dev_err_probe() to emit a message. Use the same
incarnation for the remaining exit point for consistency.

Signed-off-by: Uwe Kleine-König <ukleinek@debian.org>
---
No changes since (implicit) v1

 drivers/iio/humidity/dht11.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 0db4f7471319..d8b2cb3ef81e 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -307,10 +307,9 @@ static int dht11_probe(struct platform_device *pdev)
 				     "Failed to acquire GPIO\n");
 
 	dht11->irq = gpiod_to_irq(dht11->gpiod);
-	if (dht11->irq < 0) {
-		dev_err(dev, "GPIO %d has no interrupt\n", desc_to_gpio(dht11->gpiod));
-		return -EINVAL;
-	}
+	if (dht11->irq < 0)
+		return dev_err_probe(dev, -EINVAL, "GPIO %d has no interrupt\n",
+				     desc_to_gpio(dht11->gpiod));
 
 	dht11->timestamp = ktime_get_boottime_ns() - DHT11_DATA_VALID_TIME - 1;
 	dht11->num_edges = -1;
-- 
2.36.1

Re: [PATCH v2 1/3] iio: humidity: dht11: Don't warn on memory allocation failure

[Andy Shevchenko]

On Mon, Jul 18, 2022 at 9:50 PM Uwe Kleine-König <ukleinek@debian.org> wrote:
>
> The kernel is already quite noisy if a memory allocation fails. So it's
> the usual policy to not add another message on the driver level.
>
> Drop the error message accordingly.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Uwe Kleine-König <ukleinek@debian.org>
> ---
> Unchanged since (implicit) v1
>
>  drivers/iio/humidity/dht11.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index c97e25448772..891b6bf0b4ca 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -295,10 +295,8 @@ static int dht11_probe(struct platform_device *pdev)
>         struct iio_dev *iio;
>
>         iio = devm_iio_device_alloc(dev, sizeof(*dht11));
> -       if (!iio) {
> -               dev_err(dev, "Failed to allocate IIO device\n");
> +       if (!iio)
>                 return -ENOMEM;
> -       }
>
>         dht11 = iio_priv(iio);
>         dht11->dev = dev;
>
> base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
> --
> 2.36.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] iio: humidity: dht11: Emit error messages for probe failures

[Andy Shevchenko]

On Mon, Jul 18, 2022 at 9:50 PM Uwe Kleine-König <ukleinek@debian.org> wrote:
>
> There are two exit points in the driver's probe function that fail
> silently. From a user perspective this is unsatisfactory because the
> device is unusable but there is no hint in the kernel log about the
> actual problem which makes it unnecessarily hard to fix the problem.
>
> Make use of dev_err_probe() to emit a problem indication which also does
> the right thing if requesting the gpio return -EPROBE_DEFER.

...

> +       ret = devm_iio_device_register(dev, iio);
> +       if (ret < 0)

Do we really need this ' < 0' part?

> +               return dev_err_probe(dev, ret, "Failed to register iio device\n");

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] iio: humidity: dht11: Use dev_err_probe consistently

[Andy Shevchenko]

On Mon, Jul 18, 2022 at 9:51 PM Uwe Kleine-König <ukleinek@debian.org> wrote:
>
> All but one error path use dev_err_probe() to emit a message. Use the same
> incarnation for the remaining exit point for consistency.

...

> +       if (dht11->irq < 0)
> +               return dev_err_probe(dev, -EINVAL, "GPIO %d has no interrupt\n",
> +                                    desc_to_gpio(dht11->gpiod));

Oh, what I missed is the error code shadowing. Can't we propagate the
real error code?
-EINVAL --> dht11->irq

And to be honest I don't like this desc_to_gpio() usage. It's not for
board files, it will bring confusing information to the user. What is
important is the name of GPIO, i.o.w. "connection id".

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] iio: humidity: dht11: Use dev_err_probe consistently

[Andy Shevchenko]

On Mon, Jul 18, 2022 at 10:29 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Jul 18, 2022 at 9:51 PM Uwe Kleine-König <ukleinek@debian.org> wrote:

...

> And to be honest I don't like this desc_to_gpio() usage. It's not for
> board files, it will bring confusing information to the user. What is
> important is the name of GPIO, i.o.w. "connection id".

Yes, I have noticed that this is in the original code, just if you
want to make it better at the same time. Not sure if Jonathan wants
all these in the single patch again, however it will touch almost the
same line in all (three ?).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] iio: humidity: dht11: Emit error messages for probe failures

[Uwe Kleine-König]

Hello Andy,

On 7/18/22 22:27, Andy Shevchenko wrote:
> On Mon, Jul 18, 2022 at 9:50 PM Uwe Kleine-König <ukleinek@debian.org> wrote:
>>
>> There are two exit points in the driver's probe function that fail
>> silently. From a user perspective this is unsatisfactory because the
>> device is unusable but there is no hint in the kernel log about the
>> actual problem which makes it unnecessarily hard to fix the problem.
>>
>> Make use of dev_err_probe() to emit a problem indication which also does
>> the right thing if requesting the gpio return -EPROBE_DEFER.
> 
> ...
> 
>> +       ret = devm_iio_device_register(dev, iio);
>> +       if (ret < 0)
> 
> Do we really need this ' < 0' part?

Not sure, I stumbled about that when I split the patch, too. 
devm_iio_device_register only returns a value <= 0 and I don't expect 
dev_err_probe to behave when the error is >= 0. So adding the check at 
least documents the expectations. But I don't have hard feelings and 
dropping " < 0" would be fine for me, too.

>> +               return dev_err_probe(dev, ret, "Failed to register iio device\n");

Best regards
Uwe

Re: [PATCH v2 3/3] iio: humidity: dht11: Use dev_err_probe consistently

[Jonathan Cameron]

On Mon, 18 Jul 2022 22:32:17 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Jul 18, 2022 at 10:29 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Mon, Jul 18, 2022 at 9:51 PM Uwe Kleine-König <ukleinek@debian.org> wrote:  
> 
> ...
> 
> > And to be honest I don't like this desc_to_gpio() usage. It's not for
> > board files, it will bring confusing information to the user. What is
> > important is the name of GPIO, i.o.w. "connection id".  
> 
> Yes, I have noticed that this is in the original code, just if you
> want to make it better at the same time. Not sure if Jonathan wants
> all these in the single patch again, however it will touch almost the
> same line in all (three ?).
> 
So ideal would be separate patches for the different change types, but
meh, that's a pain as you say so given it's just a few lines I'm
fine with whatever combining you want to do.

Re: [PATCH v2 2/3] iio: humidity: dht11: Emit error messages for probe failures

[Jonathan Cameron]

On Mon, 18 Jul 2022 22:35:31 +0200
Uwe Kleine-König <ukleinek@debian.org> wrote:

> Hello Andy,
> 
> On 7/18/22 22:27, Andy Shevchenko wrote:
> > On Mon, Jul 18, 2022 at 9:50 PM Uwe Kleine-König <ukleinek@debian.org> wrote:  
> >>
> >> There are two exit points in the driver's probe function that fail
> >> silently. From a user perspective this is unsatisfactory because the
> >> device is unusable but there is no hint in the kernel log about the
> >> actual problem which makes it unnecessarily hard to fix the problem.
> >>
> >> Make use of dev_err_probe() to emit a problem indication which also does
> >> the right thing if requesting the gpio return -EPROBE_DEFER.  
> > 
> > ...
> >   
> >> +       ret = devm_iio_device_register(dev, iio);
> >> +       if (ret < 0)  
> > 
> > Do we really need this ' < 0' part?  
> 
> Not sure, I stumbled about that when I split the patch, too. 
> devm_iio_device_register only returns a value <= 0 and I don't expect 
> dev_err_probe to behave when the error is >= 0. So adding the check at 
> least documents the expectations. But I don't have hard feelings and 
> dropping " < 0" would be fine for me, too.

Slight preference for if (ret)

J
> 
> >> +               return dev_err_probe(dev, ret, "Failed to register iio device\n");  
> 
> Best regards
> Uwe

Re: [PATCH v2 2/3] iio: humidity: dht11: Emit error messages for probe failures

[Andy Shevchenko]

On Mon, Jul 18, 2022 at 9:50 PM Uwe Kleine-König <ukleinek@debian.org> wrote:
>
> There are two exit points in the driver's probe function that fail
> silently. From a user perspective this is unsatisfactory because the
> device is unusable but there is no hint in the kernel log about the
> actual problem which makes it unnecessarily hard to fix the problem.
>
> Make use of dev_err_probe() to emit a problem indication which also does
> the right thing if requesting the gpio return -EPROBE_DEFER.

After addressing the comment,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>


> Signed-off-by: Uwe Kleine-König <ukleinek@debian.org>
> ---
> Changes since (implicit) v1:
>
>  - Make it actually compile. (It helps quite a lot to have the driver to
>    be tested enabled in the config when doing compile tests *sigh*)
>  - Fix a typo I added when manually splitting the original patch
>
>  drivers/iio/humidity/dht11.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index 891b6bf0b4ca..0db4f7471319 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -293,6 +293,7 @@ static int dht11_probe(struct platform_device *pdev)
>         struct device *dev = &pdev->dev;
>         struct dht11 *dht11;
>         struct iio_dev *iio;
> +       int ret;
>
>         iio = devm_iio_device_alloc(dev, sizeof(*dht11));
>         if (!iio)
> @@ -302,7 +303,8 @@ static int dht11_probe(struct platform_device *pdev)
>         dht11->dev = dev;
>         dht11->gpiod = devm_gpiod_get(dev, NULL, GPIOD_IN);
>         if (IS_ERR(dht11->gpiod))
> -               return PTR_ERR(dht11->gpiod);
> +               return dev_err_probe(dev, PTR_ERR(dht11->gpiod),
> +                                    "Failed to acquire GPIO\n");
>
>         dht11->irq = gpiod_to_irq(dht11->gpiod);
>         if (dht11->irq < 0) {
> @@ -323,7 +325,11 @@ static int dht11_probe(struct platform_device *pdev)
>         iio->channels = dht11_chan_spec;
>         iio->num_channels = ARRAY_SIZE(dht11_chan_spec);
>
> -       return devm_iio_device_register(dev, iio);
> +       ret = devm_iio_device_register(dev, iio);
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "Failed to register iio device\n");
> +
> +       return 0;
>  }
>
>  static struct platform_driver dht11_driver = {
> --
> 2.36.1
>


-- 
With Best Regards,
Andy Shevchenko