[PATCH v4 0/3] driver core: add probe error check helper

[PATCH v4 0/3] driver core: add probe error check helper

[Andrzej Hajda]

Hi Greg, Rafael,

This patchset proposes probe helper function which should simplify little bit
resource acquisition error handling, it also extend it with adding defer probe
reason to devices_deferred property:

This patchset is actually resend of the most important 1st and 2nd patch.

I have also attached patch adding probe_err_ptr - it will allow to replace
quite frequent calls:
    probe_err(dev, PTR_ERR(ptr), ...)
with
    probe_err_ptr(dev, ptr, ...)

I have dropped the last patch showing usage of probe_err(_ptr)? as it is
very big, should be split per subsystem, and should be applied after merge
of patches introducing probe_err(_ptr) helpers.

Just for the record - my dirty cocci script generates patch which replaces
code with probe_err* helpers with following stats (on linux_next branch):
   1585 probe_err
   1194 probe_err_ptr
   1638 files changed, 6487 insertions(+), 9163 deletions(-).
Of course there are much more places where probe_err* can be applied, the script
tries to catch the most obvious ones.
More importantly probe_err should handle probe errors more correctly
and uniformly than it is done now.

If this patchset will be accepted I will try to send patches introducing probe_err*
per subsystem.

Regards
Andrzej


Andrzej Hajda (3):
  driver core: add probe_err log helper
  driver core: add deferring probe reason to devices_deferred property
  driver core: add probe_err_ptr helper

 drivers/base/base.h    |  3 +++
 drivers/base/core.c    | 40 ++++++++++++++++++++++++++++++++++++++++
 drivers/base/dd.c      | 21 ++++++++++++++++++++-
 include/linux/device.h |  5 +++++
 4 files changed, 68 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH v4 1/3] driver core: add probe_err log helper

[Andrzej Hajda]

During probe every time driver gets resource it should usually check for error
printk some message if it is not -EPROBE_DEFER and return the error. This
pattern is simple but requires adding few lines after any resource acquisition
code, as a result it is often omited or implemented only partially.
probe_err helps to replace such code sequences with simple call, so code:
	if (err != -EPROBE_DEFER)
		dev_err(dev, ...);
	return err;
becomes:
	return probe_err(dev, err, ...);

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
v3:
  - added 'extern __printf(3, 4)' decorators to probe_err,
  - changed error logging format - newline at the end,
  - added empty lines in probe_err around dev_err,
  - added R-b by Mark and Andy.
v2:
  - added error value to log message,
  - fixed code style,
  - added EXPORT_SYMBOL_GPL,
  - Added R-B by Javier (I hope the changes did not invalidate it).
---
 drivers/base/core.c    | 39 +++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |  3 +++
 2 files changed, 42 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 0073b09bb99f..7f644f3c41d3 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3099,6 +3099,45 @@ define_dev_printk_level(_dev_info, KERN_INFO);
 
 #endif
 
+/**
+ * probe_err - probe error check and log helper
+ * @dev: the pointer to the struct device
+ * @err: error value to test
+ * @fmt: printf-style format string
+ * @...: arguments as specified in the format string
+ *
+ * This helper implements common pattern present in probe functions for error
+ * checking: print message if the error is not -EPROBE_DEFER and propagate it.
+ * It replaces code sequence:
+ * 	if (err != -EPROBE_DEFER)
+ * 		dev_err(dev, ...);
+ * 	return err;
+ * with
+ * 	return probe_err(dev, err, ...);
+ *
+ * Returns @err.
+ *
+ */
+int probe_err(const struct device *dev, int err, const char *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+
+	if (err == -EPROBE_DEFER)
+		return err;
+
+	va_start(args, fmt);
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	dev_err(dev, "error %d: %pV", err, &vaf);
+
+	va_end(args);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(probe_err);
+
 static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
 {
 	return fwnode && !IS_ERR(fwnode->secondary);
diff --git a/include/linux/device.h b/include/linux/device.h
index 6cb4640b6160..2d3a1cc6f5da 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1591,6 +1591,9 @@ do {									\
 	WARN_ONCE(condition, "%s %s: " format, \
 			dev_driver_string(dev), dev_name(dev), ## arg)
 
+extern __printf(3, 4)
+int probe_err(const struct device *dev, int err, const char *fmt, ...);
+
 /* Create alias, so I can be autoloaded. */
 #define MODULE_ALIAS_CHARDEV(major,minor) \
 	MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
-- 
2.17.1

Re: [PATCH v4 1/3] driver core: add probe_err log helper

[Rafael J. Wysocki]

On Thu, Dec 20, 2018 at 11:23 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> During probe every time driver gets resource it should usually check for error
> printk some message if it is not -EPROBE_DEFER and return the error. This
> pattern is simple but requires adding few lines after any resource acquisition
> code, as a result it is often omited or implemented only partially.
> probe_err helps to replace such code sequences with simple call, so code:
>         if (err != -EPROBE_DEFER)
>                 dev_err(dev, ...);
>         return err;
> becomes:
>         return probe_err(dev, err, ...);
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Mark Brown <broonie@kernel.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

I'm not entirely convinced that the dev_err() log level is adequate
for this purpose, but anyway it looks like this may reduce code
duplication quite a bit, so

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
> v3:
>   - added 'extern __printf(3, 4)' decorators to probe_err,
>   - changed error logging format - newline at the end,
>   - added empty lines in probe_err around dev_err,
>   - added R-b by Mark and Andy.
> v2:
>   - added error value to log message,
>   - fixed code style,
>   - added EXPORT_SYMBOL_GPL,
>   - Added R-B by Javier (I hope the changes did not invalidate it).
> ---
>  drivers/base/core.c    | 39 +++++++++++++++++++++++++++++++++++++++
>  include/linux/device.h |  3 +++
>  2 files changed, 42 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 0073b09bb99f..7f644f3c41d3 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3099,6 +3099,45 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>
>  #endif
>
> +/**
> + * probe_err - probe error check and log helper
> + * @dev: the pointer to the struct device
> + * @err: error value to test
> + * @fmt: printf-style format string
> + * @...: arguments as specified in the format string
> + *
> + * This helper implements common pattern present in probe functions for error
> + * checking: print message if the error is not -EPROBE_DEFER and propagate it.
> + * It replaces code sequence:
> + *     if (err != -EPROBE_DEFER)
> + *             dev_err(dev, ...);
> + *     return err;
> + * with
> + *     return probe_err(dev, err, ...);
> + *
> + * Returns @err.
> + *
> + */
> +int probe_err(const struct device *dev, int err, const char *fmt, ...)
> +{
> +       struct va_format vaf;
> +       va_list args;
> +
> +       if (err == -EPROBE_DEFER)
> +               return err;
> +
> +       va_start(args, fmt);
> +       vaf.fmt = fmt;
> +       vaf.va = &args;
> +
> +       dev_err(dev, "error %d: %pV", err, &vaf);
> +
> +       va_end(args);
> +
> +       return err;
> +}
> +EXPORT_SYMBOL_GPL(probe_err);
> +
>  static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>  {
>         return fwnode && !IS_ERR(fwnode->secondary);
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 6cb4640b6160..2d3a1cc6f5da 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1591,6 +1591,9 @@ do {                                                                      \
>         WARN_ONCE(condition, "%s %s: " format, \
>                         dev_driver_string(dev), dev_name(dev), ## arg)
>
> +extern __printf(3, 4)
> +int probe_err(const struct device *dev, int err, const char *fmt, ...);
> +
>  /* Create alias, so I can be autoloaded. */
>  #define MODULE_ALIAS_CHARDEV(major,minor) \
>         MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
> --
> 2.17.1
>

Re: [PATCH v4 1/3] driver core: add probe_err log helper

[Greg Kroah-Hartman]

On Thu, Dec 20, 2018 at 11:22:45AM +0100, Andrzej Hajda wrote:
> During probe every time driver gets resource it should usually check for error
> printk some message if it is not -EPROBE_DEFER and return the error. This
> pattern is simple but requires adding few lines after any resource acquisition
> code, as a result it is often omited or implemented only partially.
> probe_err helps to replace such code sequences with simple call, so code:
> 	if (err != -EPROBE_DEFER)
> 		dev_err(dev, ...);
> 	return err;
> becomes:
> 	return probe_err(dev, err, ...);

Can you show a driver being converted to use this to show if it really
will save a bunch of lines and make things simpler?  Usually you are
requesting lots of resources so you need to do more than just return,
you need to clean stuff up first.

thanks,

greg k-h

Re: [PATCH v4 1/3] driver core: add probe_err log helper

[Andrzej Hajda]

On 20.12.2018 12:14, Greg Kroah-Hartman wrote:
> On Thu, Dec 20, 2018 at 11:22:45AM +0100, Andrzej Hajda wrote:
>> During probe every time driver gets resource it should usually check for error
>> printk some message if it is not -EPROBE_DEFER and return the error. This
>> pattern is simple but requires adding few lines after any resource acquisition
>> code, as a result it is often omited or implemented only partially.
>> probe_err helps to replace such code sequences with simple call, so code:
>> 	if (err != -EPROBE_DEFER)
>> 		dev_err(dev, ...);
>> 	return err;
>> becomes:
>> 	return probe_err(dev, err, ...);
> Can you show a driver being converted to use this to show if it really
> will save a bunch of lines and make things simpler?  Usually you are
> requesting lots of resources so you need to do more than just return,
> you need to clean stuff up first.


I have posted sample conversion patch (generated by cocci) in previous
version of this patchset [1].

I did not re-posted it again as it is quite big patch and it will not be
applied without prior splitting it per subsystem.

Regarding stuff cleaning: devm_* usually makes it unnecessary, but also
even with necessary cleaning you can profit from probe_err, you just
calls it without leaving probe - you have still handled correctly probe
deferring.

Here is sample usage (taken from beginning of the mentioned patch):

---
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 4b900fc659f7..52e891fe1586 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -581,11 +581,8 @@ int ahci_platform_init_host(struct platform_device *pdev,
 	int i, irq, n_ports, rc;
 
 	irq = platform_get_irq(pdev, 0);
-	if (irq <= 0) {
-		if (irq != -EPROBE_DEFER)
-			dev_err(dev, "no irq\n");
-		return irq;
-	}
+	if (irq <= 0)
+		return probe_err(dev, irq, "no irq\n");

---

And there is plenty of such places, with new version of cocci script I can locate about 2700 such cases, and it is still far from completeness.

[1]:
https://lore.kernel.org/lkml/20181016072244.1216-4-a.hajda@samsung.com/T/#u


Regards

Andrzej

Re: [PATCH v4 1/3] driver core: add probe_err log helper

[Rob Herring]

On Thu, Dec 20, 2018 at 5:38 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> On 20.12.2018 12:14, Greg Kroah-Hartman wrote:
> > On Thu, Dec 20, 2018 at 11:22:45AM +0100, Andrzej Hajda wrote:
> >> During probe every time driver gets resource it should usually check for error
> >> printk some message if it is not -EPROBE_DEFER and return the error. This
> >> pattern is simple but requires adding few lines after any resource acquisition
> >> code, as a result it is often omited or implemented only partially.
> >> probe_err helps to replace such code sequences with simple call, so code:
> >>      if (err != -EPROBE_DEFER)
> >>              dev_err(dev, ...);
> >>      return err;
> >> becomes:
> >>      return probe_err(dev, err, ...);
> > Can you show a driver being converted to use this to show if it really
> > will save a bunch of lines and make things simpler?  Usually you are
> > requesting lots of resources so you need to do more than just return,
> > you need to clean stuff up first.
>
>
> I have posted sample conversion patch (generated by cocci) in previous
> version of this patchset [1].
>
> I did not re-posted it again as it is quite big patch and it will not be
> applied without prior splitting it per subsystem.
>
> Regarding stuff cleaning: devm_* usually makes it unnecessary, but also
> even with necessary cleaning you can profit from probe_err, you just
> calls it without leaving probe - you have still handled correctly probe
> deferring.
>
> Here is sample usage (taken from beginning of the mentioned patch):
>
> ---
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index 4b900fc659f7..52e891fe1586 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -581,11 +581,8 @@ int ahci_platform_init_host(struct platform_device *pdev,
>         int i, irq, n_ports, rc;
>
>         irq = platform_get_irq(pdev, 0);
> -       if (irq <= 0) {
> -               if (irq != -EPROBE_DEFER)
> -                       dev_err(dev, "no irq\n");
> -               return irq;
> -       }
> +       if (irq <= 0)
> +               return probe_err(dev, irq, "no irq\n");

Shouldn't platform_get_irq (or what it calls) print the error message
(like we do for kmalloc), rather than every driver? We could get rid
of lots of error strings that way. I guess there are cases where no
irq is not an error and we wouldn't want to always print an error. In
some cases like that, we have 2 versions of the function.

Not what you're addressing here exactly, but what I'd like to see is
the ability to print the exact locations generating errors in the
first place. That would require wrapping all the error code
assignments and returns (or at least the common sources). If we're
going to make tree wide changes, then that might be the better place
to put the effort. If we had that, then maybe we'd need a lot fewer
error messages in drivers. I did a prototype implementation and
coccinelle script a while back that I could dust off if there's
interest. It was helpful in finding the source of errors, but did have
some false positives printed.

Rob

[PATCH] driver core: platform: Add an error message to platform_get_irq*()

[Stephen Boyd]

A grep of the kernel shows that many drivers print an error message if
they fail to get the irq they're looking for. Furthermore, those drivers
all decide to print the device name, or not, and the irq they were
requesting, or not, etc. Let's consolidate all these error messages into
the API itself, allowing us to get rid of the error messages in each
driver.

Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---

Rob Herring wrote:
> Shouldn't platform_get_irq (or what it calls) print the error message
> (like we do for kmalloc), rather than every driver? We could get rid
> of lots of error strings that way. I guess there are cases where no
> irq is not an error and we wouldn't want to always print an error. In
> some cases like that, we have 2 versions of the function.

Yes it should. Just by coincidence, I got around to implementing this
patch yesterday after I bemoaned this on the list a few months ago[1].

> Not what you're addressing here exactly, but what I'd like to see is
> the ability to print the exact locations generating errors in the
> first place. That would require wrapping all the error code
> assignments and returns (or at least the common sources). 

That could be done with some macro magic to add __line__ and __FILE__
into a definition of the "important" functions. In that sense nothing
would really need to change besides the implementation, right?

I also started working on a cocci script to fix up the call sites to
drop the extra prints, but I'm really bad at writing semantic patches so
please help improve the below. My simple grep shows that we can remove
~500 error prints out of the 1500 places the platform_get_irq()
functions are called.

	@@
	expression ret;
	struct platform_device *E;
	@@

	ret =
	(
	platform_get_irq(E, ...)
	|
	platform_get_irq_byname(E, ...)
	);

	if ( \( ret < 0 \| ret <= 0 \) )
	{
	(
	-if (ret != -EPROBE_DEFER)
	-{ ...
	-dev_err(...);
	-... }
	|
	...
	-dev_err(...);
	)
	...
	}

[1] https://lkml.kernel.org/r/153911433511.119890.17831207059115471972@swboyd.mtv.corp.google.com

 drivers/base/platform.c | 52 +++++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 17 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 1c958eb33ef4..75ceda9f452a 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -79,23 +79,18 @@ struct resource *platform_get_resource(struct platform_device *dev,
 }
 EXPORT_SYMBOL_GPL(platform_get_resource);
 
-/**
- * platform_get_irq - get an IRQ for a device
- * @dev: platform device
- * @num: IRQ number index
- */
-int platform_get_irq(struct platform_device *dev, unsigned int num)
+static int __platform_get_irq(struct platform_device *dev, unsigned int num, bool warn)
 {
+	int ret = -ENXIO;
+
 #ifdef CONFIG_SPARC
 	/* sparc does not have irqs represented as IORESOURCE_IRQ resources */
 	if (!dev || num >= dev->archdata.num_irqs)
-		return -ENXIO;
+		goto error;
 	return dev->archdata.irqs[num];
 #else
 	struct resource *r;
 	if (IS_ENABLED(CONFIG_OF_IRQ) && dev->dev.of_node) {
-		int ret;
-
 		ret = of_irq_get(dev->dev.of_node, num);
 		if (ret > 0 || ret == -EPROBE_DEFER)
 			return ret;
@@ -104,11 +99,11 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
 	r = platform_get_resource(dev, IORESOURCE_IRQ, num);
 	if (has_acpi_companion(&dev->dev)) {
 		if (r && r->flags & IORESOURCE_DISABLED) {
-			int ret;
-
 			ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
-			if (ret)
+			if (ret > 0 || ret == -EPROBE_DEFER)
 				return ret;
+			if (ret)
+				goto error;
 		}
 	}
 
@@ -122,13 +117,32 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
 		struct irq_data *irqd;
 
 		irqd = irq_get_irq_data(r->start);
-		if (!irqd)
-			return -ENXIO;
+		if (!irqd) {
+			ret = -ENXIO;
+			goto error;
+		}
+
 		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
 	}
 
-	return r ? r->start : -ENXIO;
+	if (r)
+		return r->start;
 #endif
+error:
+	if (warn)
+		dev_err(&dev->dev, "IRQ%d not found\n", num);
+
+	return ret;
+}
+
+/**
+ * platform_get_irq - get an IRQ for a device
+ * @dev: platform device
+ * @num: IRQ number index
+ */
+int platform_get_irq(struct platform_device *dev, unsigned int num)
+{
+	return __platform_get_irq(dev, num, true);
 }
 EXPORT_SYMBOL_GPL(platform_get_irq);
 
@@ -142,7 +156,7 @@ int platform_irq_count(struct platform_device *dev)
 {
 	int ret, nr = 0;
 
-	while ((ret = platform_get_irq(dev, nr)) >= 0)
+	while ((ret = __platform_get_irq(dev, nr, false)) >= 0)
 		nr++;
 
 	if (ret == -EPROBE_DEFER)
@@ -195,7 +209,11 @@ int platform_get_irq_byname(struct platform_device *dev, const char *name)
 	}
 
 	r = platform_get_resource_byname(dev, IORESOURCE_IRQ, name);
-	return r ? r->start : -ENXIO;
+	if (r)
+		return r->start;
+
+	dev_err(&dev->dev, "IRQ %s not found\n", name);
+	return -ENXIO;
 }
 EXPORT_SYMBOL_GPL(platform_get_irq_byname);
 
-- 
Sent by a computer through tubes

Re: [PATCH] driver core: platform: Add an error message to platform_get_irq*()

[Russell King - ARM Linux]

On Fri, Dec 21, 2018 at 11:24:52PM -0800, Stephen Boyd wrote:
> A grep of the kernel shows that many drivers print an error message if
> they fail to get the irq they're looking for. Furthermore, those drivers
> all decide to print the device name, or not, and the irq they were
> requesting, or not, etc. Let's consolidate all these error messages into
> the API itself, allowing us to get rid of the error messages in each
> driver.
...
> +error:
> +	if (warn)
> +		dev_err(&dev->dev, "IRQ%d not found\n", num);

Please don't use the notation IRQn - this is normally used when
referring to interrupt numbers (such as those seen in
/proc/interrupts) rather than a per-device interrupt index.
Grep for IRQ% in drivers/ for many examples.

dev_err(&dev->dev, "IRQ index %u not found: %d\n", num, ret);

would be better - note also the use of %u for unsigned integers.
Using %d for them is IMHO sloppy coding.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] driver core: platform: Add an error message to platform_get_irq*()

[Stephen Boyd]

Quoting Russell King - ARM Linux (2018-12-22 02:33:20)
> On Fri, Dec 21, 2018 at 11:24:52PM -0800, Stephen Boyd wrote:
> > A grep of the kernel shows that many drivers print an error message if
> > they fail to get the irq they're looking for. Furthermore, those drivers
> > all decide to print the device name, or not, and the irq they were
> > requesting, or not, etc. Let's consolidate all these error messages into
> > the API itself, allowing us to get rid of the error messages in each
> > driver.
> ...
> > +error:
> > +     if (warn)
> > +             dev_err(&dev->dev, "IRQ%d not found\n", num);
> 
> Please don't use the notation IRQn - this is normally used when
> referring to interrupt numbers (such as those seen in
> /proc/interrupts) rather than a per-device interrupt index.
> Grep for IRQ% in drivers/ for many examples.
> 
> dev_err(&dev->dev, "IRQ index %u not found: %d\n", num, ret);

Sure! I'll use that one.

> 
> would be better - note also the use of %u for unsigned integers.
> Using %d for them is IMHO sloppy coding.
> 

Ok.

Re: [PATCH v4 1/3] driver core: add probe_err log helper

[Andrzej Hajda]

On 21.12.2018 23:47, Rob Herring wrote:
> On Thu, Dec 20, 2018 at 5:38 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>> On 20.12.2018 12:14, Greg Kroah-Hartman wrote:
>>> On Thu, Dec 20, 2018 at 11:22:45AM +0100, Andrzej Hajda wrote:
>>>> During probe every time driver gets resource it should usually check for error
>>>> printk some message if it is not -EPROBE_DEFER and return the error. This
>>>> pattern is simple but requires adding few lines after any resource acquisition
>>>> code, as a result it is often omited or implemented only partially.
>>>> probe_err helps to replace such code sequences with simple call, so code:
>>>>      if (err != -EPROBE_DEFER)
>>>>              dev_err(dev, ...);
>>>>      return err;
>>>> becomes:
>>>>      return probe_err(dev, err, ...);
>>> Can you show a driver being converted to use this to show if it really
>>> will save a bunch of lines and make things simpler?  Usually you are
>>> requesting lots of resources so you need to do more than just return,
>>> you need to clean stuff up first.
>>
>> I have posted sample conversion patch (generated by cocci) in previous
>> version of this patchset [1].
>>
>> I did not re-posted it again as it is quite big patch and it will not be
>> applied without prior splitting it per subsystem.
>>
>> Regarding stuff cleaning: devm_* usually makes it unnecessary, but also
>> even with necessary cleaning you can profit from probe_err, you just
>> calls it without leaving probe - you have still handled correctly probe
>> deferring.
>>
>> Here is sample usage (taken from beginning of the mentioned patch):
>>
>> ---
>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
>> index 4b900fc659f7..52e891fe1586 100644
>> --- a/drivers/ata/libahci_platform.c
>> +++ b/drivers/ata/libahci_platform.c
>> @@ -581,11 +581,8 @@ int ahci_platform_init_host(struct platform_device *pdev,
>>         int i, irq, n_ports, rc;
>>
>>         irq = platform_get_irq(pdev, 0);
>> -       if (irq <= 0) {
>> -               if (irq != -EPROBE_DEFER)
>> -                       dev_err(dev, "no irq\n");
>> -               return irq;
>> -       }
>> +       if (irq <= 0)
>> +               return probe_err(dev, irq, "no irq\n");
> Shouldn't platform_get_irq (or what it calls) print the error message
> (like we do for kmalloc), rather than every driver? We could get rid
> of lots of error strings that way. I guess there are cases where no
> irq is not an error and we wouldn't want to always print an error. In
> some cases like that, we have 2 versions of the function.


kmalloc prints error and stack trace because it shows shortage of common
resource used by everyone, quite different thing than irq specific for
given device. Usually only device driver knows if error in irq acquiring
should be reported to user, and how it should be reported.

The example is for irq, but the question about the best way of reporting
error stands for all other resource acquisitions: gpios, regulators,
clocks,....

Alternative ways I see for now:

1. Do it in the consumer, like it is done now - in such case probe_err
seems to be a good helper.

2. Do it in the provider's framework, in such case framework should know
if the error should be printed:

  a) by calling special versions of all allocators,

  b) by adding extra argument to all allocators,

  c) adding extra flag to struct device (it is passed to most allocators)

3. By creating generic allocator for multiple resources, something
similar to what I have proposed few years ago in "resource tracking"
framework [1]. For example:

  ret = devm_resources_get(dev,

    res_irq_desc(&ctx->irq, 0),

    res_clk_desc(&ctx->clk, "bus_clk"),

    res_gpio_desc(&ctx->enable_gpio, "enable", GPIOD_OUT_HIGH),

    ...

  );

  Error reporting would be performed in this universal allocator.


If we want to perform brave changes I would opt for 3 - it is very
common to allocate multiple resources in probe, compacting it into one
helper should significantly simplify the code.

Option 1 is the simplest one - we do not change existing practices - so
it is the best in case of conservative approach.

I have mixed feelings about 2c, practically it looks quite tempting - we
get what we want with minimal effort, but I am not sure if polluting
struct device with 'presentation' layer is a good solution.

I do not like 2a neither 2b - alternatives between function namespace
pollution and argument list pollution.


[1]: https://lwn.net/Articles/625454/


> Not what you're addressing here exactly, but what I'd like to see is
> the ability to print the exact locations generating errors in the
> first place. That would require wrapping all the error code
> assignments and returns (or at least the common sources). If we're
> going to make tree wide changes, then that might be the better place
> to put the effort. If we had that, then maybe we'd need a lot fewer
> error messages in drivers. I did a prototype implementation and
> coccinelle script a while back that I could dust off if there's
> interest. It was helpful in finding the source of errors, but did have
> some false positives printed.


I guess that in case of resource acquisition it is usually easy to
locate place the error was reported, if the error message is informative
enough, exact line number/function name seems to me overkill.


Regards

Andrzej



>
> Rob
>
>

[PATCH v2] driver core: platform: Add an error message to platform_get_irq*()

[Stephen Boyd]

A grep of the kernel shows that many drivers print an error message if
they fail to get the irq they're looking for. Furthermore, those drivers
all decide to print the device name, or not, and the irq they were
requesting, or not, etc. Let's consolidate all these error messages into
the API itself, allowing us to get rid of the error messages in each
driver.

Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---

Changes from v1:
 * Update error text to indicate irq index instead of IRQn, use %u

 drivers/base/platform.c | 52 +++++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 17 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 1c958eb33ef4..e7af7cd63d0b 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -79,23 +79,18 @@ struct resource *platform_get_resource(struct platform_device *dev,
 }
 EXPORT_SYMBOL_GPL(platform_get_resource);
 
-/**
- * platform_get_irq - get an IRQ for a device
- * @dev: platform device
- * @num: IRQ number index
- */
-int platform_get_irq(struct platform_device *dev, unsigned int num)
+static int __platform_get_irq(struct platform_device *dev, unsigned int num, bool warn)
 {
+	int ret = -ENXIO;
+
 #ifdef CONFIG_SPARC
 	/* sparc does not have irqs represented as IORESOURCE_IRQ resources */
 	if (!dev || num >= dev->archdata.num_irqs)
-		return -ENXIO;
+		goto error;
 	return dev->archdata.irqs[num];
 #else
 	struct resource *r;
 	if (IS_ENABLED(CONFIG_OF_IRQ) && dev->dev.of_node) {
-		int ret;
-
 		ret = of_irq_get(dev->dev.of_node, num);
 		if (ret > 0 || ret == -EPROBE_DEFER)
 			return ret;
@@ -104,11 +99,11 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
 	r = platform_get_resource(dev, IORESOURCE_IRQ, num);
 	if (has_acpi_companion(&dev->dev)) {
 		if (r && r->flags & IORESOURCE_DISABLED) {
-			int ret;
-
 			ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
-			if (ret)
+			if (ret > 0 || ret == -EPROBE_DEFER)
 				return ret;
+			if (ret)
+				goto error;
 		}
 	}
 
@@ -122,13 +117,32 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
 		struct irq_data *irqd;
 
 		irqd = irq_get_irq_data(r->start);
-		if (!irqd)
-			return -ENXIO;
+		if (!irqd) {
+			ret = -ENXIO;
+			goto error;
+		}
+
 		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
 	}
 
-	return r ? r->start : -ENXIO;
+	if (r)
+		return r->start;
 #endif
+error:
+	if (warn)
+		dev_err(&dev->dev, "IRQ index %u not found\n", num);
+
+	return ret;
+}
+
+/**
+ * platform_get_irq - get an IRQ for a device
+ * @dev: platform device
+ * @num: IRQ number index
+ */
+int platform_get_irq(struct platform_device *dev, unsigned int num)
+{
+	return __platform_get_irq(dev, num, true);
 }
 EXPORT_SYMBOL_GPL(platform_get_irq);
 
@@ -142,7 +156,7 @@ int platform_irq_count(struct platform_device *dev)
 {
 	int ret, nr = 0;
 
-	while ((ret = platform_get_irq(dev, nr)) >= 0)
+	while ((ret = __platform_get_irq(dev, nr, false)) >= 0)
 		nr++;
 
 	if (ret == -EPROBE_DEFER)
@@ -195,7 +209,11 @@ int platform_get_irq_byname(struct platform_device *dev, const char *name)
 	}
 
 	r = platform_get_resource_byname(dev, IORESOURCE_IRQ, name);
-	return r ? r->start : -ENXIO;
+	if (r)
+		return r->start;
+
+	dev_err(&dev->dev, "IRQ %s not found\n", name);
+	return -ENXIO;
 }
 EXPORT_SYMBOL_GPL(platform_get_irq_byname);
 
-- 
Sent by a computer through tubes

Re: [PATCH v2] driver core: platform: Add an error message to platform_get_irq*()

[Andy Shevchenko]

On Fri, Dec 28, 2018 at 11:56 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> A grep of the kernel shows that many drivers print an error message if
> they fail to get the irq they're looking for. Furthermore, those drivers
> all decide to print the device name, or not, and the irq they were
> requesting, or not, etc. Let's consolidate all these error messages into
> the API itself, allowing us to get rid of the error messages in each
> driver.

> +static int __platform_get_irq(struct platform_device *dev, unsigned int num, bool warn)
>  {

> +error:
> +       if (warn)
> +               dev_err(&dev->dev, "IRQ index %u not found\n", num);
> +
> +       return ret;
> +}
> +
> +/**
> + * platform_get_irq - get an IRQ for a device
> + * @dev: platform device
> + * @num: IRQ number index
> + */
> +int platform_get_irq(struct platform_device *dev, unsigned int num)
> +{
> +       return __platform_get_irq(dev, num, true);

Hmm... Why not just  do
int  ret = __plaform_get_irq();
if (ret)
 dev_err();
return ret;

instead of big refactoring of platform_get_irq()?

>  }



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] driver core: platform: Add an error message to platform_get_irq*()

[Stephen Boyd]

Quoting Andy Shevchenko (2018-12-30 02:42:39)
> On Fri, Dec 28, 2018 at 11:56 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > A grep of the kernel shows that many drivers print an error message if
> > they fail to get the irq they're looking for. Furthermore, those drivers
> > all decide to print the device name, or not, and the irq they were
> > requesting, or not, etc. Let's consolidate all these error messages into
> > the API itself, allowing us to get rid of the error messages in each
> > driver.
> 
> > +static int __platform_get_irq(struct platform_device *dev, unsigned int num, bool warn)
> >  {
> 
> > +error:
> > +       if (warn)
> > +               dev_err(&dev->dev, "IRQ index %u not found\n", num);
> > +
> > +       return ret;
> > +}
> > +
> > +/**
> > + * platform_get_irq - get an IRQ for a device
> > + * @dev: platform device
> > + * @num: IRQ number index
> > + */
> > +int platform_get_irq(struct platform_device *dev, unsigned int num)
> > +{
> > +       return __platform_get_irq(dev, num, true);
> 
> Hmm... Why not just  do
> int  ret = __plaform_get_irq();
> if (ret)
>  dev_err();
> return ret;
> 
> instead of big refactoring of platform_get_irq()?

Sure. Thanks for the suggestion. But we need to check for ret < 0 I
suppose so that we don't print an error message for all the irq numbers.
I'll send the updated patch as a v3.

[PATCH v3] driver core: platform: Add an error message to platform_get_irq*()

[Stephen Boyd]

A grep of the kernel shows that many drivers print an error message if
they fail to get the irq they're looking for. Furthermore, those drivers
all decide to print the device name, or not, and the irq they were
requesting, or not, etc. Let's consolidate all these error messages into
the API itself, allowing us to get rid of the error messages in each
driver.

Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---

Changes from v2:
 * Don't refactor platform_get_irq(), just wrap it

Changes from v1:
 * Update error text to indicate irq index instead of IRQn, use %u

 drivers/base/platform.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 1c958eb33ef4..388461306dd4 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -79,12 +79,7 @@ struct resource *platform_get_resource(struct platform_device *dev,
 }
 EXPORT_SYMBOL_GPL(platform_get_resource);
 
-/**
- * platform_get_irq - get an IRQ for a device
- * @dev: platform device
- * @num: IRQ number index
- */
-int platform_get_irq(struct platform_device *dev, unsigned int num)
+static int __platform_get_irq(struct platform_device *dev, unsigned int num)
 {
 #ifdef CONFIG_SPARC
 	/* sparc does not have irqs represented as IORESOURCE_IRQ resources */
@@ -130,6 +125,22 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
 	return r ? r->start : -ENXIO;
 #endif
 }
+
+/**
+ * platform_get_irq - get an IRQ for a device
+ * @dev: platform device
+ * @num: IRQ number index
+ */
+int platform_get_irq(struct platform_device *dev, unsigned int num)
+{
+	int ret;
+
+	ret = __platform_get_irq(dev, num);
+	if (ret < 0 && ret != -EPROBE_DEFER)
+		dev_err(&dev->dev, "IRQ index %u not found\n", num);
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(platform_get_irq);
 
 /**
@@ -142,7 +153,7 @@ int platform_irq_count(struct platform_device *dev)
 {
 	int ret, nr = 0;
 
-	while ((ret = platform_get_irq(dev, nr)) >= 0)
+	while ((ret = __platform_get_irq(dev, nr)) >= 0)
 		nr++;
 
 	if (ret == -EPROBE_DEFER)
@@ -195,7 +206,11 @@ int platform_get_irq_byname(struct platform_device *dev, const char *name)
 	}
 
 	r = platform_get_resource_byname(dev, IORESOURCE_IRQ, name);
-	return r ? r->start : -ENXIO;
+	if (r)
+		return r->start;
+
+	dev_err(&dev->dev, "IRQ %s not found\n", name);
+	return -ENXIO;
 }
 EXPORT_SYMBOL_GPL(platform_get_irq_byname);
 
-- 
Sent by a computer through tubes

Re: [PATCH v3] driver core: platform: Add an error message to platform_get_irq*()

[Rafael J. Wysocki]

On Wed, Jan 2, 2019 at 7:51 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> A grep of the kernel shows that many drivers print an error message if
> they fail to get the irq they're looking for. Furthermore, those drivers
> all decide to print the device name, or not, and the irq they were
> requesting, or not, etc. Let's consolidate all these error messages into
> the API itself, allowing us to get rid of the error messages in each
> driver.
>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>
> Changes from v2:
>  * Don't refactor platform_get_irq(), just wrap it
>
> Changes from v1:
>  * Update error text to indicate irq index instead of IRQn, use %u
>
>  drivers/base/platform.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 1c958eb33ef4..388461306dd4 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -79,12 +79,7 @@ struct resource *platform_get_resource(struct platform_device *dev,
>  }
>  EXPORT_SYMBOL_GPL(platform_get_resource);
>
> -/**
> - * platform_get_irq - get an IRQ for a device
> - * @dev: platform device
> - * @num: IRQ number index
> - */
> -int platform_get_irq(struct platform_device *dev, unsigned int num)
> +static int __platform_get_irq(struct platform_device *dev, unsigned int num)
>  {
>  #ifdef CONFIG_SPARC
>         /* sparc does not have irqs represented as IORESOURCE_IRQ resources */
> @@ -130,6 +125,22 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
>         return r ? r->start : -ENXIO;
>  #endif
>  }
> +
> +/**
> + * platform_get_irq - get an IRQ for a device
> + * @dev: platform device
> + * @num: IRQ number index
> + */
> +int platform_get_irq(struct platform_device *dev, unsigned int num)
> +{
> +       int ret;
> +
> +       ret = __platform_get_irq(dev, num);
> +       if (ret < 0 && ret != -EPROBE_DEFER)
> +               dev_err(&dev->dev, "IRQ index %u not found\n", num);

Why don't you log the error code too?

> +
> +       return ret;
> +}
>  EXPORT_SYMBOL_GPL(platform_get_irq);
>
>  /**
> @@ -142,7 +153,7 @@ int platform_irq_count(struct platform_device *dev)
>  {
>         int ret, nr = 0;
>
> -       while ((ret = platform_get_irq(dev, nr)) >= 0)
> +       while ((ret = __platform_get_irq(dev, nr)) >= 0)
>                 nr++;
>
>         if (ret == -EPROBE_DEFER)
> @@ -195,7 +206,11 @@ int platform_get_irq_byname(struct platform_device *dev, const char *name)
>         }
>
>         r = platform_get_resource_byname(dev, IORESOURCE_IRQ, name);
> -       return r ? r->start : -ENXIO;
> +       if (r)
> +               return r->start;
> +
> +       dev_err(&dev->dev, "IRQ %s not found\n", name);
> +       return -ENXIO;
>  }
>  EXPORT_SYMBOL_GPL(platform_get_irq_byname);
>
> --
> Sent by a computer through tubes
>

Re: [PATCH v3] driver core: platform: Add an error message to platform_get_irq*()

[Stephen Boyd]

Quoting Rafael J. Wysocki (2019-01-03 01:40:26)
> On Wed, Jan 2, 2019 at 7:51 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > @@ -130,6 +125,22 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
> >         return r ? r->start : -ENXIO;
> >  #endif
> >  }
> > +
> > +/**
> > + * platform_get_irq - get an IRQ for a device
> > + * @dev: platform device
> > + * @num: IRQ number index
> > + */
> > +int platform_get_irq(struct platform_device *dev, unsigned int num)
> > +{
> > +       int ret;
> > +
> > +       ret = __platform_get_irq(dev, num);
> > +       if (ret < 0 && ret != -EPROBE_DEFER)
> > +               dev_err(&dev->dev, "IRQ index %u not found\n", num);
> 
> Why don't you log the error code too?
> 

I don't see much benefit to seeing -ENXIO or -EINVAL printed here, so I
left out the error code.

Re: [PATCH v3] driver core: platform: Add an error message to platform_get_irq*()

[Rafael J. Wysocki]

On Thu, Jan 3, 2019 at 5:12 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Rafael J. Wysocki (2019-01-03 01:40:26)
> > On Wed, Jan 2, 2019 at 7:51 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > @@ -130,6 +125,22 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
> > >         return r ? r->start : -ENXIO;
> > >  #endif
> > >  }
> > > +
> > > +/**
> > > + * platform_get_irq - get an IRQ for a device
> > > + * @dev: platform device
> > > + * @num: IRQ number index
> > > + */
> > > +int platform_get_irq(struct platform_device *dev, unsigned int num)
> > > +{
> > > +       int ret;
> > > +
> > > +       ret = __platform_get_irq(dev, num);
> > > +       if (ret < 0 && ret != -EPROBE_DEFER)
> > > +               dev_err(&dev->dev, "IRQ index %u not found\n", num);
> >
> > Why don't you log the error code too?
> >
>
> I don't see much benefit to seeing -ENXIO or -EINVAL printed here, so I
> left out the error code.

OK, so the value of the message is to tell the user that some driver
asked for an invalid IRQ, right?

Re: [PATCH v3] driver core: platform: Add an error message to platform_get_irq*()

[Stephen Boyd]

Quoting Rafael J. Wysocki (2019-01-03 09:22:56)
> On Thu, Jan 3, 2019 at 5:12 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > I don't see much benefit to seeing -ENXIO or -EINVAL printed here, so I
> > left out the error code.
> 
> OK, so the value of the message is to tell the user that some driver
> asked for an invalid IRQ, right?

Yes.

Re: [PATCH v3] driver core: platform: Add an error message to platform_get_irq*()

[Rafael J. Wysocki]

On Thu, Jan 3, 2019 at 6:25 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Rafael J. Wysocki (2019-01-03 09:22:56)
> > On Thu, Jan 3, 2019 at 5:12 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > I don't see much benefit to seeing -ENXIO or -EINVAL printed here, so I
> > > left out the error code.
> >
> > OK, so the value of the message is to tell the user that some driver
> > asked for an invalid IRQ, right?
>
> Yes.

OK

This can help to reduce code duplication a bit, so

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

for the v3.

Re: [PATCH v3] driver core: platform: Add an error message to platform_get_irq*()

[Andy Shevchenko]

On Wed, Jan 2, 2019 at 8:51 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> A grep of the kernel shows that many drivers print an error message if
> they fail to get the irq they're looking for. Furthermore, those drivers
> all decide to print the device name, or not, and the irq they were
> requesting, or not, etc. Let's consolidate all these error messages into
> the API itself, allowing us to get rid of the error messages in each
> driver.
>

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

> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>
> Changes from v2:
>  * Don't refactor platform_get_irq(), just wrap it
>
> Changes from v1:
>  * Update error text to indicate irq index instead of IRQn, use %u
>
>  drivers/base/platform.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 1c958eb33ef4..388461306dd4 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -79,12 +79,7 @@ struct resource *platform_get_resource(struct platform_device *dev,
>  }
>  EXPORT_SYMBOL_GPL(platform_get_resource);
>
> -/**
> - * platform_get_irq - get an IRQ for a device
> - * @dev: platform device
> - * @num: IRQ number index
> - */
> -int platform_get_irq(struct platform_device *dev, unsigned int num)
> +static int __platform_get_irq(struct platform_device *dev, unsigned int num)
>  {
>  #ifdef CONFIG_SPARC
>         /* sparc does not have irqs represented as IORESOURCE_IRQ resources */
> @@ -130,6 +125,22 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
>         return r ? r->start : -ENXIO;
>  #endif
>  }
> +
> +/**
> + * platform_get_irq - get an IRQ for a device
> + * @dev: platform device
> + * @num: IRQ number index
> + */
> +int platform_get_irq(struct platform_device *dev, unsigned int num)
> +{
> +       int ret;
> +
> +       ret = __platform_get_irq(dev, num);
> +       if (ret < 0 && ret != -EPROBE_DEFER)
> +               dev_err(&dev->dev, "IRQ index %u not found\n", num);
> +
> +       return ret;
> +}
>  EXPORT_SYMBOL_GPL(platform_get_irq);
>
>  /**
> @@ -142,7 +153,7 @@ int platform_irq_count(struct platform_device *dev)
>  {
>         int ret, nr = 0;
>
> -       while ((ret = platform_get_irq(dev, nr)) >= 0)
> +       while ((ret = __platform_get_irq(dev, nr)) >= 0)
>                 nr++;
>
>         if (ret == -EPROBE_DEFER)
> @@ -195,7 +206,11 @@ int platform_get_irq_byname(struct platform_device *dev, const char *name)
>         }
>
>         r = platform_get_resource_byname(dev, IORESOURCE_IRQ, name);
> -       return r ? r->start : -ENXIO;
> +       if (r)
> +               return r->start;
> +
> +       dev_err(&dev->dev, "IRQ %s not found\n", name);
> +       return -ENXIO;
>  }
>  EXPORT_SYMBOL_GPL(platform_get_irq_byname);
>
> --
> Sent by a computer through tubes
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 2/3] driver core: add deferring probe reason to devices_deferred property

[Andrzej Hajda]

/sys/kernel/debug/devices_deferred property contains list of deferred devices.
This list does not contain reason why the driver deferred probe, the patch
improves it.
The natural place to set the reason is probe_err function introduced recently,
ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message
will be attached to deferred device and printed when user read devices_deferred
property.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
v4:
  - removed NULL check before kfree,
  - coding style tweaking.
v3:
  - adjusted deferred_devs_show, to accept newline ended messages,
  - changed conditonal check to positive,
  - added R-b by Andy.
v2:
  - changed __deferred_probe_set_msg args - like in __dev_printk, fits better,
  - use kasprintf instead of bunch of code,
  - keep consistent format of devices_deferred lines,
  - added R-Bs (again I hope changes above are not against it).
---
---
 drivers/base/base.h |  3 +++
 drivers/base/core.c |  9 +++++----
 drivers/base/dd.c   | 21 ++++++++++++++++++++-
 3 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 7a419a7a6235..effbd5e7f9f1 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -75,6 +75,7 @@ struct device_private {
 	struct klist_node knode_driver;
 	struct klist_node knode_bus;
 	struct list_head deferred_probe;
+	char *deferred_probe_msg;
 	struct device *device;
 };
 #define to_device_private_parent(obj)	\
@@ -113,6 +114,8 @@ extern void device_release_driver_internal(struct device *dev,
 extern void driver_detach(struct device_driver *drv);
 extern int driver_probe_device(struct device_driver *drv, struct device *dev);
 extern void driver_deferred_probe_del(struct device *dev);
+extern void __deferred_probe_set_msg(const struct device *dev,
+				     struct va_format *vaf);
 static inline int driver_match_device(struct device_driver *drv,
 				      struct device *dev)
 {
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 7f644f3c41d3..d3eb5aeeaa28 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3108,6 +3108,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
  *
  * This helper implements common pattern present in probe functions for error
  * checking: print message if the error is not -EPROBE_DEFER and propagate it.
+ * In case of -EPROBE_DEFER it sets defer probe reason.
  * It replaces code sequence:
  * 	if (err != -EPROBE_DEFER)
  * 		dev_err(dev, ...);
@@ -3123,14 +3124,14 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...)
 	struct va_format vaf;
 	va_list args;
 
-	if (err == -EPROBE_DEFER)
-		return err;
-
 	va_start(args, fmt);
 	vaf.fmt = fmt;
 	vaf.va = &args;
 
-	dev_err(dev, "error %d: %pV", err, &vaf);
+	if (err == -EPROBE_DEFER)
+		__deferred_probe_set_msg(dev, &vaf);
+	else
+		dev_err(dev, "error %d: %pV", err, &vaf);
 
 	va_end(args);
 
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 88713f182086..857aa4d1d45d 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -27,6 +27,7 @@
 #include <linux/async.h>
 #include <linux/pm_runtime.h>
 #include <linux/pinctrl/devinfo.h>
+#include <linux/slab.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -132,6 +133,8 @@ void driver_deferred_probe_del(struct device *dev)
 	if (!list_empty(&dev->p->deferred_probe)) {
 		dev_dbg(dev, "Removed from deferred list\n");
 		list_del_init(&dev->p->deferred_probe);
+		kfree(dev->p->deferred_probe_msg);
+		dev->p->deferred_probe_msg = NULL;
 	}
 	mutex_unlock(&deferred_probe_mutex);
 }
@@ -202,6 +205,21 @@ void device_unblock_probing(void)
 	driver_deferred_probe_trigger();
 }
 
+/*
+ * __deferred_probe_set_msg() - Set defer probe reason message for device
+ */
+void __deferred_probe_set_msg(const struct device *dev, struct va_format *vaf)
+{
+	const char *drv = dev_driver_string(dev);
+
+	mutex_lock(&deferred_probe_mutex);
+
+	kfree(dev->p->deferred_probe_msg);
+	dev->p->deferred_probe_msg = kasprintf(GFP_KERNEL, "%s: %pV", drv, vaf);
+
+	mutex_unlock(&deferred_probe_mutex);
+}
+
 /*
  * deferred_devs_show() - Show the devices in the deferred probe pending list.
  */
@@ -212,7 +230,8 @@ static int deferred_devs_show(struct seq_file *s, void *data)
 	mutex_lock(&deferred_probe_mutex);
 
 	list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe)
-		seq_printf(s, "%s\n", dev_name(curr->device));
+		seq_printf(s, "%s\t%s", dev_name(curr->device),
+			   curr->device->p->deferred_probe_msg ?: "\n");
 
 	mutex_unlock(&deferred_probe_mutex);
 
-- 
2.17.1

Re: [PATCH v4 2/3] driver core: add deferring probe reason to devices_deferred property

[Rafael J. Wysocki]

On Thu, Dec 20, 2018 at 11:23 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> /sys/kernel/debug/devices_deferred property contains list of deferred devices.
> This list does not contain reason why the driver deferred probe, the patch
> improves it.
> The natural place to set the reason is probe_err function introduced recently,
> ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message
> will be attached to deferred device and printed when user read devices_deferred
> property.
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Mark Brown <broonie@kernel.org>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
> v4:
>   - removed NULL check before kfree,
>   - coding style tweaking.
> v3:
>   - adjusted deferred_devs_show, to accept newline ended messages,
>   - changed conditonal check to positive,
>   - added R-b by Andy.
> v2:
>   - changed __deferred_probe_set_msg args - like in __dev_printk, fits better,
>   - use kasprintf instead of bunch of code,
>   - keep consistent format of devices_deferred lines,
>   - added R-Bs (again I hope changes above are not against it).
> ---
> ---
>  drivers/base/base.h |  3 +++
>  drivers/base/core.c |  9 +++++----
>  drivers/base/dd.c   | 21 ++++++++++++++++++++-
>  3 files changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 7a419a7a6235..effbd5e7f9f1 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -75,6 +75,7 @@ struct device_private {
>         struct klist_node knode_driver;
>         struct klist_node knode_bus;
>         struct list_head deferred_probe;
> +       char *deferred_probe_msg;

Many drivers will never use this, so is the memory overhead justified?

>         struct device *device;
>  };

Re: [PATCH v4 2/3] driver core: add deferring probe reason to devices_deferred property

[Andrzej Hajda]

On 20.12.2018 12:04, Rafael J. Wysocki wrote:
> On Thu, Dec 20, 2018 at 11:23 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>> /sys/kernel/debug/devices_deferred property contains list of deferred devices.
>> This list does not contain reason why the driver deferred probe, the patch
>> improves it.
>> The natural place to set the reason is probe_err function introduced recently,
>> ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message
>> will be attached to deferred device and printed when user read devices_deferred
>> property.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> Reviewed-by: Mark Brown <broonie@kernel.org>
>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> ---
>> v4:
>>   - removed NULL check before kfree,
>>   - coding style tweaking.
>> v3:
>>   - adjusted deferred_devs_show, to accept newline ended messages,
>>   - changed conditonal check to positive,
>>   - added R-b by Andy.
>> v2:
>>   - changed __deferred_probe_set_msg args - like in __dev_printk, fits better,
>>   - use kasprintf instead of bunch of code,
>>   - keep consistent format of devices_deferred lines,
>>   - added R-Bs (again I hope changes above are not against it).
>> ---
>> ---
>>  drivers/base/base.h |  3 +++
>>  drivers/base/core.c |  9 +++++----
>>  drivers/base/dd.c   | 21 ++++++++++++++++++++-
>>  3 files changed, 28 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/base/base.h b/drivers/base/base.h
>> index 7a419a7a6235..effbd5e7f9f1 100644
>> --- a/drivers/base/base.h
>> +++ b/drivers/base/base.h
>> @@ -75,6 +75,7 @@ struct device_private {
>>         struct klist_node knode_driver;
>>         struct klist_node knode_bus;
>>         struct list_head deferred_probe;
>> +       char *deferred_probe_msg;
> Many drivers will never use this, so is the memory overhead justified?


I can try to move it somewhere else if it is a problem.

Putting it here seems quite natural - near deferred_probe field which
should have similar number of users.


Regards

Andrzej


>
>>         struct device *device;
>>  };
>

Re: [PATCH v4 2/3] driver core: add deferring probe reason to devices_deferred property

[Greg Kroah-Hartman]

On Thu, Dec 20, 2018 at 11:22:46AM +0100, Andrzej Hajda wrote:
> /sys/kernel/debug/devices_deferred property contains list of deferred devices.
> This list does not contain reason why the driver deferred probe, the patch
> improves it.
> The natural place to set the reason is probe_err function introduced recently,
> ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message
> will be attached to deferred device and printed when user read devices_deferred
> property.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Mark Brown <broonie@kernel.org>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
> v4:
>   - removed NULL check before kfree,
>   - coding style tweaking.
> v3:
>   - adjusted deferred_devs_show, to accept newline ended messages,
>   - changed conditonal check to positive,
>   - added R-b by Andy.
> v2:
>   - changed __deferred_probe_set_msg args - like in __dev_printk, fits better,
>   - use kasprintf instead of bunch of code,
>   - keep consistent format of devices_deferred lines,
>   - added R-Bs (again I hope changes above are not against it).
> ---
> ---
>  drivers/base/base.h |  3 +++
>  drivers/base/core.c |  9 +++++----
>  drivers/base/dd.c   | 21 ++++++++++++++++++++-
>  3 files changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 7a419a7a6235..effbd5e7f9f1 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -75,6 +75,7 @@ struct device_private {
>  	struct klist_node knode_driver;
>  	struct klist_node knode_bus;
>  	struct list_head deferred_probe;
> +	char *deferred_probe_msg;
>  	struct device *device;
>  };
>  #define to_device_private_parent(obj)	\
> @@ -113,6 +114,8 @@ extern void device_release_driver_internal(struct device *dev,
>  extern void driver_detach(struct device_driver *drv);
>  extern int driver_probe_device(struct device_driver *drv, struct device *dev);
>  extern void driver_deferred_probe_del(struct device *dev);
> +extern void __deferred_probe_set_msg(const struct device *dev,
> +				     struct va_format *vaf);
>  static inline int driver_match_device(struct device_driver *drv,
>  				      struct device *dev)
>  {
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 7f644f3c41d3..d3eb5aeeaa28 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3108,6 +3108,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>   *
>   * This helper implements common pattern present in probe functions for error
>   * checking: print message if the error is not -EPROBE_DEFER and propagate it.
> + * In case of -EPROBE_DEFER it sets defer probe reason.
>   * It replaces code sequence:
>   * 	if (err != -EPROBE_DEFER)
>   * 		dev_err(dev, ...);
> @@ -3123,14 +3124,14 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...)
>  	struct va_format vaf;
>  	va_list args;
>  
> -	if (err == -EPROBE_DEFER)
> -		return err;
> -
>  	va_start(args, fmt);
>  	vaf.fmt = fmt;
>  	vaf.va = &args;
>  
> -	dev_err(dev, "error %d: %pV", err, &vaf);
> +	if (err == -EPROBE_DEFER)
> +		__deferred_probe_set_msg(dev, &vaf);
> +	else
> +		dev_err(dev, "error %d: %pV", err, &vaf);
>  
>  	va_end(args);
>  
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 88713f182086..857aa4d1d45d 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -27,6 +27,7 @@
>  #include <linux/async.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pinctrl/devinfo.h>
> +#include <linux/slab.h>
>  
>  #include "base.h"
>  #include "power/power.h"
> @@ -132,6 +133,8 @@ void driver_deferred_probe_del(struct device *dev)
>  	if (!list_empty(&dev->p->deferred_probe)) {
>  		dev_dbg(dev, "Removed from deferred list\n");
>  		list_del_init(&dev->p->deferred_probe);
> +		kfree(dev->p->deferred_probe_msg);
> +		dev->p->deferred_probe_msg = NULL;
>  	}
>  	mutex_unlock(&deferred_probe_mutex);
>  }
> @@ -202,6 +205,21 @@ void device_unblock_probing(void)
>  	driver_deferred_probe_trigger();
>  }
>  
> +/*
> + * __deferred_probe_set_msg() - Set defer probe reason message for device
> + */
> +void __deferred_probe_set_msg(const struct device *dev, struct va_format *vaf)
> +{
> +	const char *drv = dev_driver_string(dev);
> +
> +	mutex_lock(&deferred_probe_mutex);
> +
> +	kfree(dev->p->deferred_probe_msg);
> +	dev->p->deferred_probe_msg = kasprintf(GFP_KERNEL, "%s: %pV", drv, vaf);

Why do you also need the dev driver string here?  Don't you get it
automatically when you print out the list in deferred_devs_show()?

How about we just wait on this patch, it seems very unneeded.  Or at
least I can't see who would need this, what can a user do with this
information?

thanks,

greg k-h

Re: [PATCH v4 2/3] driver core: add deferring probe reason to devices_deferred property

[Andrzej Hajda]

On 20.12.2018 12:12, Greg Kroah-Hartman wrote:
> On Thu, Dec 20, 2018 at 11:22:46AM +0100, Andrzej Hajda wrote:
>> /sys/kernel/debug/devices_deferred property contains list of deferred devices.
>> This list does not contain reason why the driver deferred probe, the patch
>> improves it.
>> The natural place to set the reason is probe_err function introduced recently,
>> ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message
>> will be attached to deferred device and printed when user read devices_deferred
>> property.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> Reviewed-by: Mark Brown <broonie@kernel.org>
>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> ---
>> v4:
>>   - removed NULL check before kfree,
>>   - coding style tweaking.
>> v3:
>>   - adjusted deferred_devs_show, to accept newline ended messages,
>>   - changed conditonal check to positive,
>>   - added R-b by Andy.
>> v2:
>>   - changed __deferred_probe_set_msg args - like in __dev_printk, fits better,
>>   - use kasprintf instead of bunch of code,
>>   - keep consistent format of devices_deferred lines,
>>   - added R-Bs (again I hope changes above are not against it).
>> ---
>> ---
>>  drivers/base/base.h |  3 +++
>>  drivers/base/core.c |  9 +++++----
>>  drivers/base/dd.c   | 21 ++++++++++++++++++++-
>>  3 files changed, 28 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/base/base.h b/drivers/base/base.h
>> index 7a419a7a6235..effbd5e7f9f1 100644
>> --- a/drivers/base/base.h
>> +++ b/drivers/base/base.h
>> @@ -75,6 +75,7 @@ struct device_private {
>>  	struct klist_node knode_driver;
>>  	struct klist_node knode_bus;
>>  	struct list_head deferred_probe;
>> +	char *deferred_probe_msg;
>>  	struct device *device;
>>  };
>>  #define to_device_private_parent(obj)	\
>> @@ -113,6 +114,8 @@ extern void device_release_driver_internal(struct device *dev,
>>  extern void driver_detach(struct device_driver *drv);
>>  extern int driver_probe_device(struct device_driver *drv, struct device *dev);
>>  extern void driver_deferred_probe_del(struct device *dev);
>> +extern void __deferred_probe_set_msg(const struct device *dev,
>> +				     struct va_format *vaf);
>>  static inline int driver_match_device(struct device_driver *drv,
>>  				      struct device *dev)
>>  {
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 7f644f3c41d3..d3eb5aeeaa28 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -3108,6 +3108,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>>   *
>>   * This helper implements common pattern present in probe functions for error
>>   * checking: print message if the error is not -EPROBE_DEFER and propagate it.
>> + * In case of -EPROBE_DEFER it sets defer probe reason.
>>   * It replaces code sequence:
>>   * 	if (err != -EPROBE_DEFER)
>>   * 		dev_err(dev, ...);
>> @@ -3123,14 +3124,14 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...)
>>  	struct va_format vaf;
>>  	va_list args;
>>  
>> -	if (err == -EPROBE_DEFER)
>> -		return err;
>> -
>>  	va_start(args, fmt);
>>  	vaf.fmt = fmt;
>>  	vaf.va = &args;
>>  
>> -	dev_err(dev, "error %d: %pV", err, &vaf);
>> +	if (err == -EPROBE_DEFER)
>> +		__deferred_probe_set_msg(dev, &vaf);
>> +	else
>> +		dev_err(dev, "error %d: %pV", err, &vaf);
>>  
>>  	va_end(args);
>>  
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index 88713f182086..857aa4d1d45d 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -27,6 +27,7 @@
>>  #include <linux/async.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/pinctrl/devinfo.h>
>> +#include <linux/slab.h>
>>  
>>  #include "base.h"
>>  #include "power/power.h"
>> @@ -132,6 +133,8 @@ void driver_deferred_probe_del(struct device *dev)
>>  	if (!list_empty(&dev->p->deferred_probe)) {
>>  		dev_dbg(dev, "Removed from deferred list\n");
>>  		list_del_init(&dev->p->deferred_probe);
>> +		kfree(dev->p->deferred_probe_msg);
>> +		dev->p->deferred_probe_msg = NULL;
>>  	}
>>  	mutex_unlock(&deferred_probe_mutex);
>>  }
>> @@ -202,6 +205,21 @@ void device_unblock_probing(void)
>>  	driver_deferred_probe_trigger();
>>  }
>>  
>> +/*
>> + * __deferred_probe_set_msg() - Set defer probe reason message for device
>> + */
>> +void __deferred_probe_set_msg(const struct device *dev, struct va_format *vaf)
>> +{
>> +	const char *drv = dev_driver_string(dev);
>> +
>> +	mutex_lock(&deferred_probe_mutex);
>> +
>> +	kfree(dev->p->deferred_probe_msg);
>> +	dev->p->deferred_probe_msg = kasprintf(GFP_KERNEL, "%s: %pV", drv, vaf);
> Why do you also need the dev driver string here?  Don't you get it
> automatically when you print out the list in deferred_devs_show()?


Deferred device is not bound to driver.


>
> How about we just wait on this patch, it seems very unneeded.  Or at
> least I can't see who would need this, what can a user do with this
> information?


It should be quite helpful in case of deferred probing, solving
mysteries why given device is not bound to the driver is very common in
mobile world.

And since deferal should not be treated as an error grepping dmesg often
does not help.


Regards

Andrzej



>
> thanks,
>
> greg k-h
>
>

[PATCH v4 3/3] driver core: add probe_err_ptr helper

[Andrzej Hajda]

probe_err is useful in multiple contexts where error is encoded in pointer.
Adding helper performing conversion to error value should simplify code
further.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 include/linux/device.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index 2d3a1cc6f5da..50632414c363 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1594,6 +1594,8 @@ do {									\
 extern __printf(3, 4)
 int probe_err(const struct device *dev, int err, const char *fmt, ...);
 
+#define probe_err_ptr(dev, ptr, args...) probe_err(dev, PTR_ERR(ptr), args)
+
 /* Create alias, so I can be autoloaded. */
 #define MODULE_ALIAS_CHARDEV(major,minor) \
 	MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
-- 
2.17.1

Re: [PATCH v4 3/3] driver core: add probe_err_ptr helper

[Rafael J. Wysocki]

On Thu, Dec 20, 2018 at 11:23 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> probe_err is useful in multiple contexts where error is encoded in pointer.
> Adding helper performing conversion to error value should simplify code
> further.
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  include/linux/device.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 2d3a1cc6f5da..50632414c363 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1594,6 +1594,8 @@ do {                                                                      \
>  extern __printf(3, 4)
>  int probe_err(const struct device *dev, int err, const char *fmt, ...);
>
> +#define probe_err_ptr(dev, ptr, args...) probe_err(dev, PTR_ERR(ptr), args)
> +
>  /* Create alias, so I can be autoloaded. */
>  #define MODULE_ALIAS_CHARDEV(major,minor) \
>         MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
> --
> 2.17.1
>

Re: [PATCH v4 3/3] driver core: add probe_err_ptr helper

[Greg Kroah-Hartman]

On Thu, Dec 20, 2018 at 11:22:47AM +0100, Andrzej Hajda wrote:
> probe_err is useful in multiple contexts where error is encoded in pointer.
> Adding helper performing conversion to error value should simplify code
> further.

Same question as on patch 1/3, please some someone using this.

Also, it's too late for 4.21, so no rush on this.  My trees are now
closed.

thanks,

greg k-h

[PATCH] PCI: pcie-rockchip: use probe_err helpers instead of open coding

[Andrzej Hajda]

probe_err helpers makes probe error handling easier and less error prone.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
Hi all,

This is sample conversion of one of drivers to proposed probe_err* helpers.
It was created to convince Greg that these helpers are useful.
With this helper we gain:
- corect error handling (deferral does not spam dmesg, other errors are logged),
- uniform error logging,
- simplified code,
- possibilty to check why some devices are deferred,
- shorter code.
Here are links to patchsets adding helpers v1[1], v4[2], in v1 I have added
big patch doing conversion to probe_err to show scale of the 'issue'.
If the helpers will be accepted I plan to send patches converting other drivers
as well.

[1]: https://lkml.org/lkml/2018/10/16/601
[2]: https://lkml.org/lkml/2018/12/20/438

Regards
Andrzej
---

 drivers/pci/controller/pcie-rockchip.c | 100 ++++++++++---------------
 1 file changed, 39 insertions(+), 61 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
index c53d1322a3d6..0d4f012c02ba 100644
--- a/drivers/pci/controller/pcie-rockchip.c
+++ b/drivers/pci/controller/pcie-rockchip.c
@@ -69,86 +69,67 @@ int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
 		rockchip->link_gen = 2;
 
 	rockchip->core_rst = devm_reset_control_get_exclusive(dev, "core");
-	if (IS_ERR(rockchip->core_rst)) {
-		if (PTR_ERR(rockchip->core_rst) != -EPROBE_DEFER)
-			dev_err(dev, "missing core reset property in node\n");
-		return PTR_ERR(rockchip->core_rst);
-	}
+	if (IS_ERR(rockchip->core_rst))
+		return probe_err_ptr(dev, rockchip->core_rst,
+				     "missing core reset property in node\n");
 
 	rockchip->mgmt_rst = devm_reset_control_get_exclusive(dev, "mgmt");
-	if (IS_ERR(rockchip->mgmt_rst)) {
-		if (PTR_ERR(rockchip->mgmt_rst) != -EPROBE_DEFER)
-			dev_err(dev, "missing mgmt reset property in node\n");
-		return PTR_ERR(rockchip->mgmt_rst);
-	}
+	if (IS_ERR(rockchip->mgmt_rst))
+		return probe_err_ptr(dev, rockchip->mgmt_rst,
+				     "missing mgmt reset property in node\n");
 
 	rockchip->mgmt_sticky_rst = devm_reset_control_get_exclusive(dev,
 								     "mgmt-sticky");
-	if (IS_ERR(rockchip->mgmt_sticky_rst)) {
-		if (PTR_ERR(rockchip->mgmt_sticky_rst) != -EPROBE_DEFER)
-			dev_err(dev, "missing mgmt-sticky reset property in node\n");
-		return PTR_ERR(rockchip->mgmt_sticky_rst);
-	}
+	if (IS_ERR(rockchip->mgmt_sticky_rst))
+		return probe_err_ptr(dev, rockchip->mgmt_sticky_rst,
+				     "missing mgmt-sticky reset property in node\n");
 
 	rockchip->pipe_rst = devm_reset_control_get_exclusive(dev, "pipe");
-	if (IS_ERR(rockchip->pipe_rst)) {
-		if (PTR_ERR(rockchip->pipe_rst) != -EPROBE_DEFER)
-			dev_err(dev, "missing pipe reset property in node\n");
-		return PTR_ERR(rockchip->pipe_rst);
-	}
+	if (IS_ERR(rockchip->pipe_rst))
+		return probe_err_ptr(dev, rockchip->pipe_rst,
+				     "missing pipe reset property in node\n");
 
 	rockchip->pm_rst = devm_reset_control_get_exclusive(dev, "pm");
-	if (IS_ERR(rockchip->pm_rst)) {
-		if (PTR_ERR(rockchip->pm_rst) != -EPROBE_DEFER)
-			dev_err(dev, "missing pm reset property in node\n");
-		return PTR_ERR(rockchip->pm_rst);
-	}
+	if (IS_ERR(rockchip->pm_rst))
+		return probe_err_ptr(dev, rockchip->pm_rst,
+				     "missing pm reset property in node\n");
 
 	rockchip->pclk_rst = devm_reset_control_get_exclusive(dev, "pclk");
-	if (IS_ERR(rockchip->pclk_rst)) {
-		if (PTR_ERR(rockchip->pclk_rst) != -EPROBE_DEFER)
-			dev_err(dev, "missing pclk reset property in node\n");
-		return PTR_ERR(rockchip->pclk_rst);
-	}
+	if (IS_ERR(rockchip->pclk_rst))
+		return probe_err_ptr(dev, rockchip->pclk_rst,
+				     "missing pclk reset property in node\n");
 
 	rockchip->aclk_rst = devm_reset_control_get_exclusive(dev, "aclk");
-	if (IS_ERR(rockchip->aclk_rst)) {
-		if (PTR_ERR(rockchip->aclk_rst) != -EPROBE_DEFER)
-			dev_err(dev, "missing aclk reset property in node\n");
-		return PTR_ERR(rockchip->aclk_rst);
-	}
+	if (IS_ERR(rockchip->aclk_rst))
+		return probe_err_ptr(dev, rockchip->aclk_rst,
+				     "missing aclk reset property in node\n");
 
 	if (rockchip->is_rc) {
 		rockchip->ep_gpio = devm_gpiod_get(dev, "ep", GPIOD_OUT_HIGH);
-		if (IS_ERR(rockchip->ep_gpio)) {
-			dev_err(dev, "missing ep-gpios property in node\n");
-			return PTR_ERR(rockchip->ep_gpio);
-		}
+		if (IS_ERR(rockchip->ep_gpio))
+			return probe_err_ptr(dev, rockchip->ep_gpio,
+					     "missing ep-gpios property in node\n");
 	}
 
 	rockchip->aclk_pcie = devm_clk_get(dev, "aclk");
-	if (IS_ERR(rockchip->aclk_pcie)) {
-		dev_err(dev, "aclk clock not found\n");
-		return PTR_ERR(rockchip->aclk_pcie);
-	}
+	if (IS_ERR(rockchip->aclk_pcie))
+		return probe_err_ptr(dev, rockchip->aclk_pcie,
+				     "aclk clock not found\n");
 
 	rockchip->aclk_perf_pcie = devm_clk_get(dev, "aclk-perf");
-	if (IS_ERR(rockchip->aclk_perf_pcie)) {
-		dev_err(dev, "aclk_perf clock not found\n");
-		return PTR_ERR(rockchip->aclk_perf_pcie);
-	}
+	if (IS_ERR(rockchip->aclk_perf_pcie))
+		return probe_err_ptr(dev, rockchip->aclk_perf_pcie,
+				     "aclk_perf clock not found\n");
 
 	rockchip->hclk_pcie = devm_clk_get(dev, "hclk");
-	if (IS_ERR(rockchip->hclk_pcie)) {
-		dev_err(dev, "hclk clock not found\n");
-		return PTR_ERR(rockchip->hclk_pcie);
-	}
+	if (IS_ERR(rockchip->hclk_pcie))
+		return probe_err_ptr(dev, rockchip->hclk_pcie,
+				     "hclk clock not found\n");
 
 	rockchip->clk_pcie_pm = devm_clk_get(dev, "pm");
-	if (IS_ERR(rockchip->clk_pcie_pm)) {
-		dev_err(dev, "pm clock not found\n");
-		return PTR_ERR(rockchip->clk_pcie_pm);
-	}
+	if (IS_ERR(rockchip->clk_pcie_pm))
+		return probe_err_ptr(dev, rockchip->clk_pcie_pm,
+				     "pm clock not found\n");
 
 	return 0;
 }
@@ -323,12 +304,9 @@ int rockchip_pcie_get_phys(struct rockchip_pcie *rockchip)
 		phy = devm_of_phy_get(dev, dev->of_node, name);
 		kfree(name);
 
-		if (IS_ERR(phy)) {
-			if (PTR_ERR(phy) != -EPROBE_DEFER)
-				dev_err(dev, "missing phy for lane %d: %ld\n",
-					i, PTR_ERR(phy));
-			return PTR_ERR(phy);
-		}
+		if (IS_ERR(phy))
+			return probe_err_ptr(dev, phy,
+					     "missing phy for lane %d\n", i);
 
 		rockchip->phys[i] = phy;
 	}
-- 
2.17.1

Re: [PATCH] PCI: pcie-rockchip: use probe_err helpers instead of open coding

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 6047 bytes --]

Hi Andrzej,

I love your patch! Yet something to improve:

[auto build test ERROR on rockchip/for-next]
[also build test ERROR on v4.20-rc7]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Andrzej-Hajda/PCI-pcie-rockchip-use-probe_err-helpers-instead-of-open-coding/20181222-044838
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git for-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 8.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.1.0 make.cross ARCH=ia64 

All errors (new ones prefixed by >>):

   drivers/pci/controller/pcie-rockchip.c: In function 'rockchip_pcie_parse_dt':
>> drivers/pci/controller/pcie-rockchip.c:73:10: error: implicit declaration of function 'probe_err_ptr'; did you mean 'probe_irq_off'? [-Werror=implicit-function-declaration]
      return probe_err_ptr(dev, rockchip->core_rst,
             ^~~~~~~~~~~~~
             probe_irq_off
   cc1: some warnings being treated as errors

vim +73 drivers/pci/controller/pcie-rockchip.c

    24	
    25	int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
    26	{
    27		struct device *dev = rockchip->dev;
    28		struct platform_device *pdev = to_platform_device(dev);
    29		struct device_node *node = dev->of_node;
    30		struct resource *regs;
    31		int err;
    32	
    33		if (rockchip->is_rc) {
    34			regs = platform_get_resource_byname(pdev,
    35							    IORESOURCE_MEM,
    36							    "axi-base");
    37			rockchip->reg_base = devm_pci_remap_cfg_resource(dev, regs);
    38			if (IS_ERR(rockchip->reg_base))
    39				return PTR_ERR(rockchip->reg_base);
    40		} else {
    41			rockchip->mem_res =
    42				platform_get_resource_byname(pdev, IORESOURCE_MEM,
    43							     "mem-base");
    44			if (!rockchip->mem_res)
    45				return -EINVAL;
    46		}
    47	
    48		regs = platform_get_resource_byname(pdev, IORESOURCE_MEM,
    49						    "apb-base");
    50		rockchip->apb_base = devm_ioremap_resource(dev, regs);
    51		if (IS_ERR(rockchip->apb_base))
    52			return PTR_ERR(rockchip->apb_base);
    53	
    54		err = rockchip_pcie_get_phys(rockchip);
    55		if (err)
    56			return err;
    57	
    58		rockchip->lanes = 1;
    59		err = of_property_read_u32(node, "num-lanes", &rockchip->lanes);
    60		if (!err && (rockchip->lanes == 0 ||
    61			     rockchip->lanes == 3 ||
    62			     rockchip->lanes > 4)) {
    63			dev_warn(dev, "invalid num-lanes, default to use one lane\n");
    64			rockchip->lanes = 1;
    65		}
    66	
    67		rockchip->link_gen = of_pci_get_max_link_speed(node);
    68		if (rockchip->link_gen < 0 || rockchip->link_gen > 2)
    69			rockchip->link_gen = 2;
    70	
    71		rockchip->core_rst = devm_reset_control_get_exclusive(dev, "core");
    72		if (IS_ERR(rockchip->core_rst))
  > 73			return probe_err_ptr(dev, rockchip->core_rst,
    74					     "missing core reset property in node\n");
    75	
    76		rockchip->mgmt_rst = devm_reset_control_get_exclusive(dev, "mgmt");
    77		if (IS_ERR(rockchip->mgmt_rst))
    78			return probe_err_ptr(dev, rockchip->mgmt_rst,
    79					     "missing mgmt reset property in node\n");
    80	
    81		rockchip->mgmt_sticky_rst = devm_reset_control_get_exclusive(dev,
    82									     "mgmt-sticky");
    83		if (IS_ERR(rockchip->mgmt_sticky_rst))
    84			return probe_err_ptr(dev, rockchip->mgmt_sticky_rst,
    85					     "missing mgmt-sticky reset property in node\n");
    86	
    87		rockchip->pipe_rst = devm_reset_control_get_exclusive(dev, "pipe");
    88		if (IS_ERR(rockchip->pipe_rst))
    89			return probe_err_ptr(dev, rockchip->pipe_rst,
    90					     "missing pipe reset property in node\n");
    91	
    92		rockchip->pm_rst = devm_reset_control_get_exclusive(dev, "pm");
    93		if (IS_ERR(rockchip->pm_rst))
    94			return probe_err_ptr(dev, rockchip->pm_rst,
    95					     "missing pm reset property in node\n");
    96	
    97		rockchip->pclk_rst = devm_reset_control_get_exclusive(dev, "pclk");
    98		if (IS_ERR(rockchip->pclk_rst))
    99			return probe_err_ptr(dev, rockchip->pclk_rst,
   100					     "missing pclk reset property in node\n");
   101	
   102		rockchip->aclk_rst = devm_reset_control_get_exclusive(dev, "aclk");
   103		if (IS_ERR(rockchip->aclk_rst))
   104			return probe_err_ptr(dev, rockchip->aclk_rst,
   105					     "missing aclk reset property in node\n");
   106	
   107		if (rockchip->is_rc) {
   108			rockchip->ep_gpio = devm_gpiod_get(dev, "ep", GPIOD_OUT_HIGH);
   109			if (IS_ERR(rockchip->ep_gpio))
   110				return probe_err_ptr(dev, rockchip->ep_gpio,
   111						     "missing ep-gpios property in node\n");
   112		}
   113	
   114		rockchip->aclk_pcie = devm_clk_get(dev, "aclk");
   115		if (IS_ERR(rockchip->aclk_pcie))
   116			return probe_err_ptr(dev, rockchip->aclk_pcie,
   117					     "aclk clock not found\n");
   118	
   119		rockchip->aclk_perf_pcie = devm_clk_get(dev, "aclk-perf");
   120		if (IS_ERR(rockchip->aclk_perf_pcie))
   121			return probe_err_ptr(dev, rockchip->aclk_perf_pcie,
   122					     "aclk_perf clock not found\n");
   123	
   124		rockchip->hclk_pcie = devm_clk_get(dev, "hclk");
   125		if (IS_ERR(rockchip->hclk_pcie))
   126			return probe_err_ptr(dev, rockchip->hclk_pcie,
   127					     "hclk clock not found\n");
   128	
   129		rockchip->clk_pcie_pm = devm_clk_get(dev, "pm");
   130		if (IS_ERR(rockchip->clk_pcie_pm))
   131			return probe_err_ptr(dev, rockchip->clk_pcie_pm,
   132					     "pm clock not found\n");
   133	
   134		return 0;
   135	}
   136	EXPORT_SYMBOL_GPL(rockchip_pcie_parse_dt);
   137	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 52333 bytes --]