[PATCH] iio: humidity: dht11: Improve error reporting in .probe()

[PATCH] iio: humidity: dht11: Improve error reporting in .probe()

[Uwe Kleine-König]

Don't emit a message for -ENOMEM, the kernel is already loud enough in
this case. Add a message if getting the GPIO or registering the iio
device fails and use dev_err_probe for improved behaviour on
-EPROBE_DEFER.

Signed-off-by: Uwe Kleine-König <ukleinek@debian.org>
---
 drivers/iio/humidity/dht11.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index c97e25448772..d8b2cb3ef81e 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -293,24 +293,23 @@ 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) {
-		dev_err(dev, "Failed to allocate IIO device\n");
+	if (!iio)
 		return -ENOMEM;
-	}
 
 	dht11 = iio_priv(iio);
 	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) {
-		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;
@@ -325,7 +324,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 = {

base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
-- 
2.36.1

Re: [PATCH] iio: humidity: dht11: Improve error reporting in .probe()

[Andy Shevchenko]

On Thu, Jun 30, 2022 at 8:58 AM Uwe Kleine-König <ukleinek@debian.org> wrote:
>
> Don't emit a message for -ENOMEM, the kernel is already loud enough in
> this case. Add a message if getting the GPIO or registering the iio
> device fails and use dev_err_probe for improved behaviour on
> -EPROBE_DEFER.

Why do we need additional messages?

They should be split in a separate patch, perhaps, with the explanation.

Actually the rest I would split to two: converting to dev_err_probe()
in the case where it's not right now, and dropping ENOMEM message.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: humidity: dht11: Improve error reporting in .probe()

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1233 bytes --]

On 6/30/22 09:17, Andy Shevchenko wrote:
> On Thu, Jun 30, 2022 at 8:58 AM Uwe Kleine-König <ukleinek@debian.org> wrote:
>>
>> Don't emit a message for -ENOMEM, the kernel is already loud enough in
>> this case. Add a message if getting the GPIO or registering the iio
>> device fails and use dev_err_probe for improved behaviour on
>> -EPROBE_DEFER.
> 
> Why do we need additional messages?

I don't understand the question. Do you really wonder why there is a 
benefit of an error message if a resource allocation in probe fails?

Current state is that for Yves-Alexis (on Cc:) the driver is silent but 
unbound. I guess that's because gpiod_get fails, but seeing a 
confirmation in the kernel log would be nice.

> They should be split in a separate patch, perhaps, with the explanation.
> 
> Actually the rest I would split to two: converting to dev_err_probe()
> in the case where it's not right now, and dropping ENOMEM message.

So we're at three patches:

  - drop ENOMEM message
  - convert existing messages to dev_err_probe()
  - introduce errors for devm_gpiod_get() and devm_iio_device_register()

I can rework accordingly, but for me this looks a bit over engineered.

Best regards
Uwe

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] iio: humidity: dht11: Improve error reporting in .probe()

[Andy Shevchenko]

On Thu, Jun 30, 2022 at 12:25 PM Uwe Kleine-König <ukleinek@debian.org> wrote:
> On 6/30/22 09:17, Andy Shevchenko wrote:
> > On Thu, Jun 30, 2022 at 8:58 AM Uwe Kleine-König <ukleinek@debian.org> wrote:
> >>
> >> Don't emit a message for -ENOMEM, the kernel is already loud enough in
> >> this case. Add a message if getting the GPIO or registering the iio
> >> device fails and use dev_err_probe for improved behaviour on
> >> -EPROBE_DEFER.
> >
> > Why do we need additional messages?
>
> I don't understand the question. Do you really wonder why there is a
> benefit of an error message if a resource allocation in probe fails?
>
> Current state is that for Yves-Alexis (on Cc:) the driver is silent but
> unbound. I guess that's because gpiod_get fails, but seeing a
> confirmation in the kernel log would be nice.
>
> > They should be split in a separate patch, perhaps, with the explanation.
> >
> > Actually the rest I would split to two: converting to dev_err_probe()
> > in the case where it's not right now, and dropping ENOMEM message.
>
> So we're at three patches:
>
>   - drop ENOMEM message
>   - convert existing messages to dev_err_probe()
>   - introduce errors for devm_gpiod_get() and devm_iio_device_register()
>
> I can rework accordingly, but for me this looks a bit over engineered.

Perhaps, but they three are about different stuff.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: humidity: dht11: Improve error reporting in .probe()

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2001 bytes --]

On 6/30/22 12:26, Andy Shevchenko wrote:
> On Thu, Jun 30, 2022 at 12:25 PM Uwe Kleine-König <ukleinek@debian.org> wrote:
>> On 6/30/22 09:17, Andy Shevchenko wrote:
>>> On Thu, Jun 30, 2022 at 8:58 AM Uwe Kleine-König <ukleinek@debian.org> wrote:
>>>>
>>>> Don't emit a message for -ENOMEM, the kernel is already loud enough in
>>>> this case. Add a message if getting the GPIO or registering the iio
>>>> device fails and use dev_err_probe for improved behaviour on
>>>> -EPROBE_DEFER.
>>>
>>> Why do we need additional messages?
>>
>> I don't understand the question. Do you really wonder why there is a
>> benefit of an error message if a resource allocation in probe fails?
>>
>> Current state is that for Yves-Alexis (on Cc:) the driver is silent but
>> unbound. I guess that's because gpiod_get fails, but seeing a
>> confirmation in the kernel log would be nice.
>>
>>> They should be split in a separate patch, perhaps, with the explanation.
>>>
>>> Actually the rest I would split to two: converting to dev_err_probe()
>>> in the case where it's not right now, and dropping ENOMEM message.
>>
>> So we're at three patches:
>>
>>    - drop ENOMEM message
>>    - convert existing messages to dev_err_probe()
>>    - introduce errors for devm_gpiod_get() and devm_iio_device_register()
>>
>> I can rework accordingly, but for me this looks a bit over engineered.
> 
> Perhaps, but they three are about different stuff.

The different things could be named as follows:

  - Improve error reporting in .probe() for devm_iio_device_alloc()
  - Improve error reporting in .probe() for devm_gpiod_get()
  - Improve error reporting in .probe() for gpiod_to_irq()
  - Improve error reporting in .probe() for devm_iio_device_register()

Even after the discussion I think it's ok to summarize that to a single 
patch "Improve error reporting in .probe()". I'd wait for Jonathan's 
feedback with his opinion before reworking the patch.

Best regards
Uwe

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] iio: humidity: dht11: Improve error reporting in .probe()

[Jonathan Cameron]

On Thu, 30 Jun 2022 16:15:05 +0200
Uwe Kleine-König <ukleinek@debian.org> wrote:

> On 6/30/22 12:26, Andy Shevchenko wrote:
> > On Thu, Jun 30, 2022 at 12:25 PM Uwe Kleine-König <ukleinek@debian.org> wrote:  
> >> On 6/30/22 09:17, Andy Shevchenko wrote:  
> >>> On Thu, Jun 30, 2022 at 8:58 AM Uwe Kleine-König <ukleinek@debian.org> wrote:  
> >>>>
> >>>> Don't emit a message for -ENOMEM, the kernel is already loud enough in
> >>>> this case. Add a message if getting the GPIO or registering the iio
> >>>> device fails and use dev_err_probe for improved behaviour on
> >>>> -EPROBE_DEFER.  
> >>>
> >>> Why do we need additional messages?  
> >>
> >> I don't understand the question. Do you really wonder why there is a
> >> benefit of an error message if a resource allocation in probe fails?
> >>
> >> Current state is that for Yves-Alexis (on Cc:) the driver is silent but
> >> unbound. I guess that's because gpiod_get fails, but seeing a
> >> confirmation in the kernel log would be nice.

Giving that example in the patch description makes for a very strong
reasoning.  Mostly I want this captured so that others who might consider
similar patches and see this, understand the reasoning.  Messages for things
that can happen in real life are definitely a good thing!

> >>  
> >>> They should be split in a separate patch, perhaps, with the explanation.
> >>>
> >>> Actually the rest I would split to two: converting to dev_err_probe()
> >>> in the case where it's not right now, and dropping ENOMEM message.  
> >>
> >> So we're at three patches:
> >>
> >>    - drop ENOMEM message
> >>    - convert existing messages to dev_err_probe()
> >>    - introduce errors for devm_gpiod_get() and devm_iio_device_register()

That lines up with how I'd prefer it split. If I'd not wanted the patch description
additions above anyway I 'might' have just picked it up.  Given we are going
around again, let's set a nice example.  In particularly it will make the
description easier to read as separates -ENOMEM case from the new additions.

Jonathan


> >>
> >> I can rework accordingly, but for me this looks a bit over engineered.  
> > 
> > Perhaps, but they three are about different stuff.  
> 
> The different things could be named as follows:
> 
>   - Improve error reporting in .probe() for devm_iio_device_alloc()
>   - Improve error reporting in .probe() for devm_gpiod_get()
>   - Improve error reporting in .probe() for gpiod_to_irq()
>   - Improve error reporting in .probe() for devm_iio_device_register()
> 
> Even after the discussion I think it's ok to summarize that to a single 
> patch "Improve error reporting in .probe()". I'd wait for Jonathan's 
> feedback with his opinion before reworking the patch.



> 
> Best regards
> Uwe