[RESEND PATCH v5 0/5] driver core: add probe error check helper

[RESEND PATCH v5 0/5] driver core: add probe error check helper

[Andrzej Hajda]

Hi All,

Recently I took some time to re-check error handling in drivers probe code,
and I have noticed that number of incorrect resource acquisition error handling
increased and there are no other propositions which can cure the situation.

So I have decided to resend my old proposition of probe_err helper which should
simplify resource acquisition error handling, it also extend it with adding defer
probe reason to devices_deferred debugfs property, which should improve debugging
experience for developers/testers.

In v5 I have also attached patch adding macro to replace quite frequent calls:
    probe_err(dev, PTR_ERR(ptr), ...)
with
    probe_err(dev, ptr, ...)

I have also added two patches showing usage and benefits of the helper.

My dirty/ad-hoc cocci scripts shows that this helper can be used in at least 2700 places
saving about 3500 lines of code.

Regards
Andrzej


Andrzej Hajda (5):
  driver core: add probe_err log helper
  driver core: add deferring probe reason to devices_deferred property
  drivers core: allow probe_err accept integer and pointer types
  drm/bridge/sii8620: fix resource acquisition error handling
  drm/bridge: lvds-codec: simplify error handling code

 drivers/base/base.h                  |  3 +++
 drivers/base/core.c                  | 20 ++++++++++++++++++++
 drivers/base/dd.c                    | 21 ++++++++++++++++++++-
 drivers/gpu/drm/bridge/lvds-codec.c  |  9 ++-------
 drivers/gpu/drm/bridge/sil-sii8620.c | 18 ++++++------------
 include/linux/device.h               | 26 ++++++++++++++++++++++++++
 6 files changed, 77 insertions(+), 20 deletions(-)

-- 
2.17.1

[RESEND PATCH v5 1/5] 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>
---
 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 67d39a90b45c..ee9da66bff1b 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3953,6 +3953,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 15460a5ac024..40a90d9bf799 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -964,6 +964,9 @@ void device_link_remove(void *consumer, struct device *supplier);
 void device_links_supplier_sync_state_pause(void);
 void device_links_supplier_sync_state_resume(void);
 
+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

[RESEND PATCH v5 2/5] 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>
---
 drivers/base/base.h |  3 +++
 drivers/base/core.c | 10 ++++++----
 drivers/base/dd.c   | 21 ++++++++++++++++++++-
 3 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 95c22c0f9036..93ef1c2f4c1f 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -93,6 +93,7 @@ struct device_private {
 	struct klist_node knode_class;
 	struct list_head deferred_probe;
 	struct device_driver *async_driver;
+	char *deferred_probe_msg;
 	struct device *device;
 	u8 dead:1;
 };
@@ -134,6 +135,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 ee9da66bff1b..2a96954d5460 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3962,6 +3962,8 @@ 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, which can be checked
+ * later by reading devices_deferred debugfs attribute.
  * It replaces code sequence:
  * 	if (err != -EPROBE_DEFER)
  * 		dev_err(dev, ...);
@@ -3977,14 +3979,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 9a1d940342ac..f44d26454b6a 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"
@@ -136,6 +137,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);
 }
@@ -211,6 +214,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.
  */
@@ -221,7 +239,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

[RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types

[Andrzej Hajda]

Many resource acquisition functions return error value encapsulated in
pointer instead of integer value. To simplify coding we can use macro
which will accept both types of error.
With this patch user can use:
	probe_err(dev, ptr, ...)
instead of:
	probe_err(dev, PTR_ERR(ptr), ...)
Without loosing old functionality:
	probe_err(dev, err, ...)

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/base/core.c    | 25 ++-----------------------
 include/linux/device.h | 25 ++++++++++++++++++++++++-
 2 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2a96954d5460..df283c62d9c0 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3953,28 +3953,7 @@ 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.
- * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
- * later by reading devices_deferred debugfs attribute.
- * 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, ...)
+int __probe_err(const struct device *dev, int err, const char *fmt, ...)
 {
 	struct va_format vaf;
 	va_list args;
@@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...)
 
 	return err;
 }
-EXPORT_SYMBOL_GPL(probe_err);
+EXPORT_SYMBOL_GPL(__probe_err);
 
 static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
 {
diff --git a/include/linux/device.h b/include/linux/device.h
index 40a90d9bf799..22d3c3d4f461 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void);
 void device_links_supplier_sync_state_resume(void);
 
 extern __printf(3, 4)
-int probe_err(const struct device *dev, int err, const char *fmt, ...);
+int __probe_err(const struct device *dev, int err, const char *fmt, ...);
+
+/**
+ * probe_err - probe error check and log helper
+ * @dev: the pointer to the struct device
+ * @err: error value to test, can be integer or pointer type
+ * @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.
+ * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
+ * later by reading devices_deferred debugfs attribute.
+ * It replaces code sequence:
+ * 	if (err != -EPROBE_DEFER)
+ * 		dev_err(dev, ...);
+ * 	return err;
+ * with
+ * 	return probe_err(dev, err, ...);
+ *
+ * Returns @err.
+ *
+ */
+#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args)
 
 /* Create alias, so I can be autoloaded. */
 #define MODULE_ALIAS_CHARDEV(major,minor) \
-- 
2.17.1

[RESEND PATCH v5 4/5] drm/bridge/sii8620: fix resource acquisition error handling

[Andrzej Hajda]

In case of error during resource acquisition driver should print error
message only in case it is not deferred probe, using probe_err helper
solves the issue. Moreover it records defer probe reason for debugging.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/bridge/sil-sii8620.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
index 92acd336aa89..2f825b2d0098 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -2299,10 +2299,8 @@ static int sii8620_probe(struct i2c_client *client,
 	INIT_LIST_HEAD(&ctx->mt_queue);
 
 	ctx->clk_xtal = devm_clk_get(dev, "xtal");
-	if (IS_ERR(ctx->clk_xtal)) {
-		dev_err(dev, "failed to get xtal clock from DT\n");
-		return PTR_ERR(ctx->clk_xtal);
-	}
+	if (IS_ERR(ctx->clk_xtal))
+		return probe_err(dev, ctx->clk_xtal, "failed to get xtal clock from DT\n");
 
 	if (!client->irq) {
 		dev_err(dev, "no irq provided\n");
@@ -2313,16 +2311,12 @@ static int sii8620_probe(struct i2c_client *client,
 					sii8620_irq_thread,
 					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
 					"sii8620", ctx);
-	if (ret < 0) {
-		dev_err(dev, "failed to install IRQ handler\n");
-		return ret;
-	}
+	if (ret < 0)
+		return probe_err(dev, ret, "failed to install IRQ handler\n");
 
 	ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
-	if (IS_ERR(ctx->gpio_reset)) {
-		dev_err(dev, "failed to get reset gpio from DT\n");
-		return PTR_ERR(ctx->gpio_reset);
-	}
+	if (IS_ERR(ctx->gpio_reset))
+		return probe_err(dev, ctx->gpio_reset, "failed to get reset gpio from DT\n");
 
 	ctx->supplies[0].supply = "cvcc10";
 	ctx->supplies[1].supply = "iovcc18";
-- 
2.17.1

[RESEND PATCH v5 5/5] drm/bridge: lvds-codec: simplify error handling code

[Andrzej Hajda]

Using probe_err code has following advantages:
- shorter code,
- recorded defer probe reason for debugging,
- uniform error code logging.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/bridge/lvds-codec.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
index 24fb1befdfa2..c76fa0239e39 100644
--- a/drivers/gpu/drm/bridge/lvds-codec.c
+++ b/drivers/gpu/drm/bridge/lvds-codec.c
@@ -71,13 +71,8 @@ static int lvds_codec_probe(struct platform_device *pdev)
 	lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
 	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
 							     GPIOD_OUT_HIGH);
-	if (IS_ERR(lvds_codec->powerdown_gpio)) {
-		int err = PTR_ERR(lvds_codec->powerdown_gpio);
-
-		if (err != -EPROBE_DEFER)
-			dev_err(dev, "powerdown GPIO failure: %d\n", err);
-		return err;
-	}
+	if (IS_ERR(lvds_codec->powerdown_gpio))
+		return probe_err(dev, lvds_codec->powerdown_gpio, "powerdown GPIO failure\n");
 
 	/* Locate the panel DT node. */
 	panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
-- 
2.17.1

Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper

[Rafael J. Wysocki]

On Wed, Jun 24, 2020 at 1:41 PM 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>

Reviewed-by Rafael J. Wysocki <rafael@kernel.org>

> ---
>  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 67d39a90b45c..ee9da66bff1b 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3953,6 +3953,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 15460a5ac024..40a90d9bf799 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -964,6 +964,9 @@ void device_link_remove(void *consumer, struct device *supplier);
>  void device_links_supplier_sync_state_pause(void);
>  void device_links_supplier_sync_state_resume(void);
>
> +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: [RESEND PATCH v5 2/5] driver core: add deferring probe reason to devices_deferred property

[Rafael J. Wysocki]

On Wed, Jun 24, 2020 at 1:41 PM 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>
> ---
>  drivers/base/base.h |  3 +++
>  drivers/base/core.c | 10 ++++++----
>  drivers/base/dd.c   | 21 ++++++++++++++++++++-
>  3 files changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 95c22c0f9036..93ef1c2f4c1f 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -93,6 +93,7 @@ struct device_private {
>         struct klist_node knode_class;
>         struct list_head deferred_probe;
>         struct device_driver *async_driver;
> +       char *deferred_probe_msg;

What about calling this deferred_probe_reason?

>         struct device *device;
>         u8 dead:1;
>  };
> @@ -134,6 +135,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);

I'd call this device_set_deferred_probe_reson() to follow the naming
convention for the function names in this header file.

>  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 ee9da66bff1b..2a96954d5460 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3962,6 +3962,8 @@ 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, which can be checked
> + * later by reading devices_deferred debugfs attribute.
>   * It replaces code sequence:
>   *     if (err != -EPROBE_DEFER)
>   *             dev_err(dev, ...);
> @@ -3977,14 +3979,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 9a1d940342ac..f44d26454b6a 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"
> @@ -136,6 +137,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);
>  }
> @@ -211,6 +214,21 @@ void device_unblock_probing(void)
>         driver_deferred_probe_trigger();
>  }
>
> +/*
> + * __deferred_probe_set_msg() - Set defer probe reason message for device

I'd change this into a full kerneldoc comment.

> + */
> +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.
>   */
> @@ -221,7 +239,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: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types

[Rafael J. Wysocki]

On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> Many resource acquisition functions return error value encapsulated in
> pointer instead of integer value. To simplify coding we can use macro
> which will accept both types of error.
> With this patch user can use:
>         probe_err(dev, ptr, ...)
> instead of:
>         probe_err(dev, PTR_ERR(ptr), ...)
> Without loosing old functionality:
>         probe_err(dev, err, ...)
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>

The separation of this change from patch [1/5] looks kind of artificial to me.

You are introducing a new function anyway, so why not to make it what
you want right away?

> ---
>  drivers/base/core.c    | 25 ++-----------------------
>  include/linux/device.h | 25 ++++++++++++++++++++++++-
>  2 files changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 2a96954d5460..df283c62d9c0 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3953,28 +3953,7 @@ 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.
> - * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
> - * later by reading devices_deferred debugfs attribute.
> - * 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, ...)
> +int __probe_err(const struct device *dev, int err, const char *fmt, ...)
>  {
>         struct va_format vaf;
>         va_list args;
> @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...)
>
>         return err;
>  }
> -EXPORT_SYMBOL_GPL(probe_err);
> +EXPORT_SYMBOL_GPL(__probe_err);
>
>  static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>  {
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 40a90d9bf799..22d3c3d4f461 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void);
>  void device_links_supplier_sync_state_resume(void);
>
>  extern __printf(3, 4)
> -int probe_err(const struct device *dev, int err, const char *fmt, ...);
> +int __probe_err(const struct device *dev, int err, const char *fmt, ...);
> +
> +/**
> + * probe_err - probe error check and log helper
> + * @dev: the pointer to the struct device
> + * @err: error value to test, can be integer or pointer type
> + * @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.
> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
> + * later by reading devices_deferred debugfs attribute.
> + * It replaces code sequence:
> + *     if (err != -EPROBE_DEFER)
> + *             dev_err(dev, ...);
> + *     return err;
> + * with
> + *     return probe_err(dev, err, ...);
> + *
> + * Returns @err.
> + *
> + */
> +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args)
>
>  /* Create alias, so I can be autoloaded. */
>  #define MODULE_ALIAS_CHARDEV(major,minor) \
> --
> 2.17.1
>

Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types

[Greg Kroah-Hartman]

On Wed, Jun 24, 2020 at 01:41:25PM +0200, Andrzej Hajda wrote:
> Many resource acquisition functions return error value encapsulated in
> pointer instead of integer value. To simplify coding we can use macro
> which will accept both types of error.
> With this patch user can use:
> 	probe_err(dev, ptr, ...)
> instead of:
> 	probe_err(dev, PTR_ERR(ptr), ...)
> Without loosing old functionality:
> 	probe_err(dev, err, ...)
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/base/core.c    | 25 ++-----------------------
>  include/linux/device.h | 25 ++++++++++++++++++++++++-
>  2 files changed, 26 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 2a96954d5460..df283c62d9c0 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3953,28 +3953,7 @@ 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.
> - * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
> - * later by reading devices_deferred debugfs attribute.
> - * 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, ...)
> +int __probe_err(const struct device *dev, int err, const char *fmt, ...)
>  {
>  	struct va_format vaf;
>  	va_list args;
> @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...)
>  
>  	return err;
>  }
> -EXPORT_SYMBOL_GPL(probe_err);
> +EXPORT_SYMBOL_GPL(__probe_err);
>  
>  static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>  {
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 40a90d9bf799..22d3c3d4f461 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void);
>  void device_links_supplier_sync_state_resume(void);
>  
>  extern __printf(3, 4)
> -int probe_err(const struct device *dev, int err, const char *fmt, ...);
> +int __probe_err(const struct device *dev, int err, const char *fmt, ...);
> +
> +/**
> + * probe_err - probe error check and log helper
> + * @dev: the pointer to the struct device
> + * @err: error value to test, can be integer or pointer type
> + * @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.
> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
> + * later by reading devices_deferred debugfs attribute.
> + * It replaces code sequence:
> + * 	if (err != -EPROBE_DEFER)
> + * 		dev_err(dev, ...);
> + * 	return err;
> + * with
> + * 	return probe_err(dev, err, ...);
> + *
> + * Returns @err.
> + *
> + */
> +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args)

Shouldn't that be "unsigned long" instead of "long"?  That's what we put
pointers in last I looked...

thanks,

greg k-h

Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper

[Greg Kroah-Hartman]

On Wed, Jun 24, 2020 at 01:41:23PM +0200, 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, ...);
> 
> 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>
> ---
>  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 67d39a90b45c..ee9da66bff1b 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3953,6 +3953,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);

Please be specific in global symbols, how about "driver_probe_error()"?

And merge the other patch into this one, as Raphael said, otherwise this
just looks odd to add something and then fix it up later.

thanks,

greg k-h

Re: [RESEND PATCH v5 2/5] driver core: add deferring probe reason to devices_deferred property

[Greg Kroah-Hartman]

On Wed, Jun 24, 2020 at 01:41:24PM +0200, 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>
> ---
>  drivers/base/base.h |  3 +++
>  drivers/base/core.c | 10 ++++++----
>  drivers/base/dd.c   | 21 ++++++++++++++++++++-
>  3 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 95c22c0f9036..93ef1c2f4c1f 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -93,6 +93,7 @@ struct device_private {
>  	struct klist_node knode_class;
>  	struct list_head deferred_probe;
>  	struct device_driver *async_driver;
> +	char *deferred_probe_msg;
>  	struct device *device;
>  	u8 dead:1;
>  };
> @@ -134,6 +135,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 ee9da66bff1b..2a96954d5460 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3962,6 +3962,8 @@ 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, which can be checked
> + * later by reading devices_deferred debugfs attribute.
>   * It replaces code sequence:
>   * 	if (err != -EPROBE_DEFER)
>   * 		dev_err(dev, ...);
> @@ -3977,14 +3979,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 9a1d940342ac..f44d26454b6a 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"
> @@ -136,6 +137,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);
>  }
> @@ -211,6 +214,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);

What about the device name?  Don't you also want that?

You want the same format that __dev_printk() outputs, please use that
to be consistant with all other messages that drivers are spitting out.

thanks,

greg k-h

Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types

[Robin Murphy]

On 2020-06-24 12:41, Andrzej Hajda wrote:
> Many resource acquisition functions return error value encapsulated in
> pointer instead of integer value. To simplify coding we can use macro
> which will accept both types of error.
> With this patch user can use:
> 	probe_err(dev, ptr, ...)
> instead of:
> 	probe_err(dev, PTR_ERR(ptr), ...)
> Without loosing old functionality:
> 	probe_err(dev, err, ...)

Personally I'm not convinced that simplification has much value, and I'd 
say it *does* have a significant downside. This:

	if (IS_ERR(x))
		do_something_with(PTR_ERR(x));

is a familiar and expected pattern when reading/reviewing code, and at a 
glance is almost certainly doing the right thing. If I see this, on the 
other hand:

	if (IS_ERR(x))
		do_something_with(x);

my immediate instinct is to be suspicious, and now I've got to go off 
and double-check that if do_something_with() really expects a pointer 
it's also robust against PTR_ERR values. Off-hand I can't think of any 
APIs that work that way in the areas with which I'm familiar, so it 
would be a pretty unusual and non-obvious thing.

Furthermore, an error helper that explicitly claims to accept "pointer 
type" values seems like it could easily lead to misunderstandings like this:

	int init_my_buffer(struct my_device *d)
	{
		d->buffer = kzalloc(d->buffer_size, GFP_KERNEL);
		return probe_err(d->dev, d->buffer, "failed to init buffer\n");
	}

and allowing that to compile without any hint of an error seems a 
little... unfair.

Robin.

> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>   drivers/base/core.c    | 25 ++-----------------------
>   include/linux/device.h | 25 ++++++++++++++++++++++++-
>   2 files changed, 26 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 2a96954d5460..df283c62d9c0 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3953,28 +3953,7 @@ 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.
> - * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
> - * later by reading devices_deferred debugfs attribute.
> - * 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, ...)
> +int __probe_err(const struct device *dev, int err, const char *fmt, ...)
>   {
>   	struct va_format vaf;
>   	va_list args;
> @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...)
>   
>   	return err;
>   }
> -EXPORT_SYMBOL_GPL(probe_err);
> +EXPORT_SYMBOL_GPL(__probe_err);
>   
>   static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>   {
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 40a90d9bf799..22d3c3d4f461 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void);
>   void device_links_supplier_sync_state_resume(void);
>   
>   extern __printf(3, 4)
> -int probe_err(const struct device *dev, int err, const char *fmt, ...);
> +int __probe_err(const struct device *dev, int err, const char *fmt, ...);
> +
> +/**
> + * probe_err - probe error check and log helper
> + * @dev: the pointer to the struct device
> + * @err: error value to test, can be integer or pointer type
> + * @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.
> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
> + * later by reading devices_deferred debugfs attribute.
> + * It replaces code sequence:
> + * 	if (err != -EPROBE_DEFER)
> + * 		dev_err(dev, ...);
> + * 	return err;
> + * with
> + * 	return probe_err(dev, err, ...);
> + *
> + * Returns @err.
> + *
> + */
> +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args)
>   
>   /* Create alias, so I can be autoloaded. */
>   #define MODULE_ALIAS_CHARDEV(major,minor) \
> 

Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types

[Andy Shevchenko]

On Wed, Jun 24, 2020 at 2:41 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> Many resource acquisition functions return error value encapsulated in
> pointer instead of integer value. To simplify coding we can use macro
> which will accept both types of error.
> With this patch user can use:
>         probe_err(dev, ptr, ...)
> instead of:
>         probe_err(dev, PTR_ERR(ptr), ...)
> Without loosing old functionality:
>         probe_err(dev, err, ...)

...

> + * 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, which can be checked
> + * later by reading devices_deferred debugfs attribute.
> + * It replaces code sequence:
> + *     if (err != -EPROBE_DEFER)
> + *             dev_err(dev, ...);

Btw, we have now %pe. Can you consider to use it?

> + *     return err;
> + * with
> + *     return probe_err(dev, err, ...);
> + *
> + * Returns @err.
> + *
> + */
> +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args)

Can't we use PTR_ERR() here?

-- 
With Best Regards,
Andy Shevchenko

Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types

[Andy Shevchenko]

On Wed, Jun 24, 2020 at 3:37 PM Robin Murphy <robin.murphy@arm.com> wrote:
> On 2020-06-24 12:41, Andrzej Hajda wrote:
> > Many resource acquisition functions return error value encapsulated in
> > pointer instead of integer value. To simplify coding we can use macro
> > which will accept both types of error.
> > With this patch user can use:
> >       probe_err(dev, ptr, ...)
> > instead of:
> >       probe_err(dev, PTR_ERR(ptr), ...)
> > Without loosing old functionality:
> >       probe_err(dev, err, ...)
>
> Personally I'm not convinced that simplification has much value, and I'd
> say it *does* have a significant downside. This:
>
>         if (IS_ERR(x))
>                 do_something_with(PTR_ERR(x));
>
> is a familiar and expected pattern when reading/reviewing code, and at a
> glance is almost certainly doing the right thing. If I see this, on the
> other hand:
>
>         if (IS_ERR(x))
>                 do_something_with(x);

I don't consider your arguments strong enough. You are appealing to
one pattern vs. new coming *pattern* just with a different name and
actually much less characters to parse. We have a lot of clean ups
like this, have you protested against them? JFYI: they are now
*established patterns* and saved a ton of LOCs in some of which even
were typos.

> my immediate instinct is to be suspicious, and now I've got to go off
> and double-check that if do_something_with() really expects a pointer
> it's also robust against PTR_ERR values. Off-hand I can't think of any
> APIs that work that way in the areas with which I'm familiar, so it
> would be a pretty unusual and non-obvious thing.

-- 
With Best Regards,
Andy Shevchenko

Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types

[Andrzej Hajda]

On 24.06.2020 14:53, Andy Shevchenko wrote:
> On Wed, Jun 24, 2020 at 2:41 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>> Many resource acquisition functions return error value encapsulated in
>> pointer instead of integer value. To simplify coding we can use macro
>> which will accept both types of error.
>> With this patch user can use:
>>          probe_err(dev, ptr, ...)
>> instead of:
>>          probe_err(dev, PTR_ERR(ptr), ...)
>> Without loosing old functionality:
>>          probe_err(dev, err, ...)
> ...
>
>> + * 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, which can be checked
>> + * later by reading devices_deferred debugfs attribute.
>> + * It replaces code sequence:
>> + *     if (err != -EPROBE_DEFER)
>> + *             dev_err(dev, ...);
> Btw, we have now %pe. Can you consider to use it?


OK, I haven't noticed it finally appeared.


>
>> + *     return err;
>> + * with
>> + *     return probe_err(dev, err, ...);
>> + *
>> + * Returns @err.
>> + *
>> + */
>> +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args)
> Can't we use PTR_ERR() here?


Nope, I want to accept both types: int and pointer.


Regards

Andrzej


>

Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper

[Laurent Pinchart]

On Wed, Jun 24, 2020 at 02:31:40PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jun 24, 2020 at 01:41:23PM +0200, 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, ...);
> > 
> > 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>
> > ---
> >  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 67d39a90b45c..ee9da66bff1b 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -3953,6 +3953,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);
> 
> Please be specific in global symbols, how about "driver_probe_error()"?

Or dev_err_probe() to match the existing dev_* functions ?

> And merge the other patch into this one, as Raphael said, otherwise this
> just looks odd to add something and then fix it up later.

-- 
Regards,

Laurent Pinchart

Re: [RESEND PATCH v5 4/5] drm/bridge/sii8620: fix resource acquisition error handling

[Mark Brown]

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

On Wed, Jun 24, 2020 at 01:41:26PM +0200, Andrzej Hajda wrote:
> In case of error during resource acquisition driver should print error
> message only in case it is not deferred probe, using probe_err helper
> solves the issue. Moreover it records defer probe reason for debugging.

If we silently ignore all deferred probe errors we make it hard for
anyone who is experiencing issues with deferred probe to figure out what
they're missing.  We should at least be logging problems at debug level
so there's something for people to go on without having to hack the
kernel source.

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

Re: [RESEND PATCH v5 2/5] driver core: add deferring probe reason to devices_deferred property

[Andrzej Hajda]

On 24.06.2020 14:34, Greg Kroah-Hartman wrote:
> On Wed, Jun 24, 2020 at 01:41:24PM +0200, 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>
>> ---
>>   drivers/base/base.h |  3 +++
>>   drivers/base/core.c | 10 ++++++----
>>   drivers/base/dd.c   | 21 ++++++++++++++++++++-
>>   3 files changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/base/base.h b/drivers/base/base.h
>> index 95c22c0f9036..93ef1c2f4c1f 100644
>> --- a/drivers/base/base.h
>> +++ b/drivers/base/base.h
>> @@ -93,6 +93,7 @@ struct device_private {
>>   	struct klist_node knode_class;
>>   	struct list_head deferred_probe;
>>   	struct device_driver *async_driver;
>> +	char *deferred_probe_msg;
>>   	struct device *device;
>>   	u8 dead:1;
>>   };
>> @@ -134,6 +135,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 ee9da66bff1b..2a96954d5460 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -3962,6 +3962,8 @@ 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, which can be checked
>> + * later by reading devices_deferred debugfs attribute.
>>    * It replaces code sequence:
>>    * 	if (err != -EPROBE_DEFER)
>>    * 		dev_err(dev, ...);
>> @@ -3977,14 +3979,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 9a1d940342ac..f44d26454b6a 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"
>> @@ -136,6 +137,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);
>>   }
>> @@ -211,6 +214,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);
> What about the device name?  Don't you also want that?


deferred_devs_show prints it already, deferred_probe_msg is appended if 
not null.


Regards

Andrzej


>
> You want the same format that __dev_printk() outputs, please use that
> to be consistant with all other messages that drivers are spitting out.
>
> thanks,
>
> greg k-h
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://protect2.fireeye.com/url?k=28daee95-7508f5cd-28db65da-0cc47a31c8b4-b3e8d1affcda9c08&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel
>

Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper

[Mark Brown]

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

On Wed, Jun 24, 2020 at 01:41:23PM +0200, 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

As I said down the thread that's not a great pattern since it means that
probe deferral errors never get displayed and users have a hard time
figuring out why their driver isn't instantiating.

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

Re: [RESEND PATCH v5 2/5] driver core: add deferring probe reason to devices_deferred property

[Andrzej Hajda]

On 24.06.2020 14:11, Rafael J. Wysocki wrote:
> On Wed, Jun 24, 2020 at 1:41 PM 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>
>> ---
>>   drivers/base/base.h |  3 +++
>>   drivers/base/core.c | 10 ++++++----
>>   drivers/base/dd.c   | 21 ++++++++++++++++++++-
>>   3 files changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/base/base.h b/drivers/base/base.h
>> index 95c22c0f9036..93ef1c2f4c1f 100644
>> --- a/drivers/base/base.h
>> +++ b/drivers/base/base.h
>> @@ -93,6 +93,7 @@ struct device_private {
>>          struct klist_node knode_class;
>>          struct list_head deferred_probe;
>>          struct device_driver *async_driver;
>> +       char *deferred_probe_msg;
> What about calling this deferred_probe_reason?
>
>>          struct device *device;
>>          u8 dead:1;
>>   };
>> @@ -134,6 +135,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);
> I'd call this device_set_deferred_probe_reson() to follow the naming
> convention for the function names in this header file.
>
>>   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 ee9da66bff1b..2a96954d5460 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -3962,6 +3962,8 @@ 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, which can be checked
>> + * later by reading devices_deferred debugfs attribute.
>>    * It replaces code sequence:
>>    *     if (err != -EPROBE_DEFER)
>>    *             dev_err(dev, ...);
>> @@ -3977,14 +3979,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 9a1d940342ac..f44d26454b6a 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"
>> @@ -136,6 +137,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);
>>   }
>> @@ -211,6 +214,21 @@ void device_unblock_probing(void)
>>          driver_deferred_probe_trigger();
>>   }
>>
>> +/*
>> + * __deferred_probe_set_msg() - Set defer probe reason message for device
> I'd change this into a full kerneldoc comment.


OK I will apply all changes in next version. Thx for review.


Regards

Andrzej


>
>> + */
>> +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.
>>    */
>> @@ -221,7 +239,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
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://protect2.fireeye.com/url?k=5adb7884-070fc5c0-5adaf3cb-0cc47a31381a-5588a624ab84213e&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel
>

Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types

[Laurent Pinchart]

On Wed, Jun 24, 2020 at 01:37:52PM +0100, Robin Murphy wrote:
> On 2020-06-24 12:41, Andrzej Hajda wrote:
> > Many resource acquisition functions return error value encapsulated in
> > pointer instead of integer value. To simplify coding we can use macro
> > which will accept both types of error.
> > With this patch user can use:
> > 	probe_err(dev, ptr, ...)
> > instead of:
> > 	probe_err(dev, PTR_ERR(ptr), ...)
> > Without loosing old functionality:
> > 	probe_err(dev, err, ...)
> 
> Personally I'm not convinced that simplification has much value, and I'd 
> say it *does* have a significant downside. This:
> 
> 	if (IS_ERR(x))
> 		do_something_with(PTR_ERR(x));
> 
> is a familiar and expected pattern when reading/reviewing code, and at a 
> glance is almost certainly doing the right thing. If I see this, on the 
> other hand:
> 
> 	if (IS_ERR(x))
> 		do_something_with(x);
> 
> my immediate instinct is to be suspicious, and now I've got to go off 
> and double-check that if do_something_with() really expects a pointer 
> it's also robust against PTR_ERR values. Off-hand I can't think of any 
> APIs that work that way in the areas with which I'm familiar, so it 
> would be a pretty unusual and non-obvious thing.

I second this. Furthermore, the hidden cast to long means that we'll
leak pointer values if one happens to pass a real pointer to this
function.

> Furthermore, an error helper that explicitly claims to accept "pointer 
> type" values seems like it could easily lead to misunderstandings like this:
> 
> 	int init_my_buffer(struct my_device *d)
> 	{
> 		d->buffer = kzalloc(d->buffer_size, GFP_KERNEL);
> 		return probe_err(d->dev, d->buffer, "failed to init buffer\n");
> 	}
> 
> and allowing that to compile without any hint of an error seems a 
> little... unfair.
> 
> Robin.
> 
> > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> > ---
> >   drivers/base/core.c    | 25 ++-----------------------
> >   include/linux/device.h | 25 ++++++++++++++++++++++++-
> >   2 files changed, 26 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 2a96954d5460..df283c62d9c0 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -3953,28 +3953,7 @@ 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.
> > - * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
> > - * later by reading devices_deferred debugfs attribute.
> > - * 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, ...)
> > +int __probe_err(const struct device *dev, int err, const char *fmt, ...)
> >   {
> >   	struct va_format vaf;
> >   	va_list args;
> > @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...)
> >   
> >   	return err;
> >   }
> > -EXPORT_SYMBOL_GPL(probe_err);
> > +EXPORT_SYMBOL_GPL(__probe_err);
> >   
> >   static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
> >   {
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 40a90d9bf799..22d3c3d4f461 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void);
> >   void device_links_supplier_sync_state_resume(void);
> >   
> >   extern __printf(3, 4)
> > -int probe_err(const struct device *dev, int err, const char *fmt, ...);
> > +int __probe_err(const struct device *dev, int err, const char *fmt, ...);
> > +
> > +/**
> > + * probe_err - probe error check and log helper
> > + * @dev: the pointer to the struct device
> > + * @err: error value to test, can be integer or pointer type
> > + * @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.
> > + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
> > + * later by reading devices_deferred debugfs attribute.
> > + * It replaces code sequence:
> > + * 	if (err != -EPROBE_DEFER)
> > + * 		dev_err(dev, ...);
> > + * 	return err;
> > + * with
> > + * 	return probe_err(dev, err, ...);
> > + *
> > + * Returns @err.
> > + *
> > + */
> > +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args)
> >   
> >   /* Create alias, so I can be autoloaded. */
> >   #define MODULE_ALIAS_CHARDEV(major,minor) \
> > 

-- 
Regards,

Laurent Pinchart

Re: [RESEND PATCH v5 5/5] drm/bridge: lvds-codec: simplify error handling code

[Laurent Pinchart]

Hi Andrzej,

On Wed, Jun 24, 2020 at 01:41:27PM +0200, Andrzej Hajda wrote:
> Using probe_err code has following advantages:
> - shorter code,
> - recorded defer probe reason for debugging,
> - uniform error code logging.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/gpu/drm/bridge/lvds-codec.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> index 24fb1befdfa2..c76fa0239e39 100644
> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> @@ -71,13 +71,8 @@ static int lvds_codec_probe(struct platform_device *pdev)
>  	lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
>  	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
>  							     GPIOD_OUT_HIGH);
> -	if (IS_ERR(lvds_codec->powerdown_gpio)) {
> -		int err = PTR_ERR(lvds_codec->powerdown_gpio);
> -
> -		if (err != -EPROBE_DEFER)
> -			dev_err(dev, "powerdown GPIO failure: %d\n", err);
> -		return err;
> -	}
> +	if (IS_ERR(lvds_codec->powerdown_gpio))
> +		return probe_err(dev, lvds_codec->powerdown_gpio, "powerdown GPIO failure\n");

Line wrap please.

It bothers me that the common pattern of writing the error code at the
end of the message isn't possible anymore. Maybe I'll get used to it,
but it removes some flexibility.

>  
>  	/* Locate the panel DT node. */
>  	panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);

-- 
Regards,

Laurent Pinchart

Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper

[Andy Shevchenko]

On Wed, Jun 24, 2020 at 4:27 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Jun 24, 2020 at 01:41:23PM +0200, 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
>
> As I said down the thread that's not a great pattern since it means that
> probe deferral errors never get displayed and users have a hard time
> figuring out why their driver isn't instantiating.

Don't we have a file in the debugfs to list deferred drivers?

In the case of deferred probes the errors out of it makes users more
miserable in order to look through tons of spam and lose really useful
data in the logs.

-- 
With Best Regards,
Andy Shevchenko

Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper

[Mark Brown]

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

On Wed, Jun 24, 2020 at 04:45:28PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 24, 2020 at 4:27 PM Mark Brown <broonie@kernel.org> wrote:

> > As I said down the thread that's not a great pattern since it means that
> > probe deferral errors never get displayed and users have a hard time
> > figuring out why their driver isn't instantiating.

> Don't we have a file in the debugfs to list deferred drivers?

Part of what this patch series aims to solve is that that list is not
very useful since it doesn't provide any information on how things got
deferred which means it's of no use in trying to figure out any
problems.

> In the case of deferred probes the errors out of it makes users more
> miserable in order to look through tons of spam and lose really useful
> data in the logs.

I seem to never manage to end up using any of the systems which generate
excessive deferrals.

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

Re: [RESEND PATCH v5 5/5] drm/bridge: lvds-codec: simplify error handling code

[Andrzej Hajda]

On 24.06.2020 15:33, Laurent Pinchart wrote:
> Hi Andrzej,
>
> On Wed, Jun 24, 2020 at 01:41:27PM +0200, Andrzej Hajda wrote:
>> Using probe_err code has following advantages:
>> - shorter code,
>> - recorded defer probe reason for debugging,
>> - uniform error code logging.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>>   drivers/gpu/drm/bridge/lvds-codec.c | 9 ++-------
>>   1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
>> index 24fb1befdfa2..c76fa0239e39 100644
>> --- a/drivers/gpu/drm/bridge/lvds-codec.c
>> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
>> @@ -71,13 +71,8 @@ static int lvds_codec_probe(struct platform_device *pdev)
>>   	lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
>>   	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
>>   							     GPIOD_OUT_HIGH);
>> -	if (IS_ERR(lvds_codec->powerdown_gpio)) {
>> -		int err = PTR_ERR(lvds_codec->powerdown_gpio);
>> -
>> -		if (err != -EPROBE_DEFER)
>> -			dev_err(dev, "powerdown GPIO failure: %d\n", err);
>> -		return err;
>> -	}
>> +	if (IS_ERR(lvds_codec->powerdown_gpio))
>> +		return probe_err(dev, lvds_codec->powerdown_gpio, "powerdown GPIO failure\n");
> Line wrap please.


I hoped that with latest checkpatch line length limit increase from 80 
to 100 it is acceptable :) but apparently not [1].

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144


>
> It bothers me that the common pattern of writing the error code at the
> end of the message isn't possible anymore. Maybe I'll get used to it,
> but it removes some flexibility.


Yes, but it gives uniformity :) and now with %pe printk format it 
changes anyway.


Regards

Andrzej


>
>>   
>>   	/* Locate the panel DT node. */
>>   	panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);

Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper

[Andrzej Hajda]

On 24.06.2020 15:23, Laurent Pinchart wrote:
> On Wed, Jun 24, 2020 at 02:31:40PM +0200, Greg Kroah-Hartman wrote:
>> On Wed, Jun 24, 2020 at 01:41:23PM +0200, 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, ...);
>>>
>>> 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>
>>> ---
>>>   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 67d39a90b45c..ee9da66bff1b 100644
>>> --- a/drivers/base/core.c
>>> +++ b/drivers/base/core.c
>>> @@ -3953,6 +3953,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);
>> Please be specific in global symbols, how about "driver_probe_error()"?
> Or dev_err_probe() to match the existing dev_* functions ?


OK.


Regards

Andrzej


>
>> And merge the other patch into this one, as Raphael said, otherwise this
>> just looks odd to add something and then fix it up later.

Re: [RESEND PATCH v5 4/5] drm/bridge/sii8620: fix resource acquisition error handling

[Mark Brown]

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

On Wed, Jun 24, 2020 at 03:43:10PM +0200, Andrzej Hajda wrote:
> 
> On 24.06.2020 15:25, Mark Brown wrote:

> > If we silently ignore all deferred probe errors we make it hard for
> > anyone who is experiencing issues with deferred probe to figure out what
> > they're missing.  We should at least be logging problems at debug level
> > so there's something for people to go on without having to hack the
> > kernel source.

> But you can always do:

> cat /sys/kernel/debug/devices_deferred

> And you will find there deferred probe reason (thanks to patch 2/5).

Right, my point is more that we shouldn't be promoting discarding the
diagnostics entirely but rather saying that we want to redirect those
somewhere else.

> Eventually if you want it in dmesg anyway, one can adjust probe_err function
> to log probe error on debug level as well.

That would most likely be very useful as a boot option for problems that
occur before we get a console up.

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

Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types

[Robin Murphy]

On 2020-06-24 13:55, Andy Shevchenko wrote:
> On Wed, Jun 24, 2020 at 3:37 PM Robin Murphy <robin.murphy@arm.com> wrote:
>> On 2020-06-24 12:41, Andrzej Hajda wrote:
>>> Many resource acquisition functions return error value encapsulated in
>>> pointer instead of integer value. To simplify coding we can use macro
>>> which will accept both types of error.
>>> With this patch user can use:
>>>        probe_err(dev, ptr, ...)
>>> instead of:
>>>        probe_err(dev, PTR_ERR(ptr), ...)
>>> Without loosing old functionality:
>>>        probe_err(dev, err, ...)
>>
>> Personally I'm not convinced that simplification has much value, and I'd
>> say it *does* have a significant downside. This:
>>
>>          if (IS_ERR(x))
>>                  do_something_with(PTR_ERR(x));
>>
>> is a familiar and expected pattern when reading/reviewing code, and at a
>> glance is almost certainly doing the right thing. If I see this, on the
>> other hand:
>>
>>          if (IS_ERR(x))
>>                  do_something_with(x);
> 
> I don't consider your arguments strong enough. You are appealing to
> one pattern vs. new coming *pattern* just with a different name and
> actually much less characters to parse. We have a lot of clean ups
> like this, have you protested against them? JFYI: they are now
> *established patterns* and saved a ton of LOCs in some of which even
> were typos.

I'm not against probe_err() itself, I think that stands to be a 
wonderfully helpful thing that will eliminate a hell of a lot of ugly 
mess from drivers. It's only the specific elision of 9 characters worth 
of "PTR_ERR()" in certain cases that I'm questioning. I mean, we've 
already got +20 characters leeway now that checkpatch has acknowledged 
all that blank space on the right-hand side of all my editor windows.

Sure, there's not necessarily anything bad about introducing a new 
pattern in itself, but it's not going to *replace* the existing pattern 
of "IS_ERR() pairs with PTR_ERR()", because IS_ERR() is used for more 
than driver probe error handling. Instead, everybody now has to bear in 
mind that the new pattern is "IS_ERR() pairs with PTR_ERR(), except 
sometimes when it doesn't - last time I looked only probe_err() was 
special, but maybe something's changed, I'll have to go check...", and 
that's just adding cognitive load for the sake of not even saving a 
linewrap any more.

First, the wave of Sparse errors from the build bots hits because it 
turns out casting arbitrary pointers appropriately is hard. As it washes 
past, the reality of authors' and maintainers' personal preferences 
comes to bear... some inevitably prefer to keep spelling out PTR_ERR() 
in probe_err() calls for the sake of clarity, bikeshedding ensues, and 
the checkpatch and Coccinelle armies mobilise to send unwanted patches 
changing things back and forth. Eventually, in all the confusion, a 
synapse misfires in a muddled developer's mind, an ERR_PTR value passed 
to kfree() "because kfree() is robust" slips through, and a bug is born. 
And there's the thing: inconsistency breeds bugs. Sometimes, of course, 
there are really good reasons to be inconsistent. Is 9 characters per 
line for a few hundred lines across the kernel tree really a really good 
reason?

The tersest code is not always the most maintainable. Consider that we 
could also save many more "tons of LoC" by rewriting an entire category 
of small if/else statements with ternaries. Would the overall effect on 
maintainability be positive? I don't think so.

And yeah, anyone who pipes up suggesting that places where an ERR_PTR 
value could be passed to probe_err() could simply refactor IS_ERR() 
checks with more uses of the god-awful PTR_ERR_OR_ZERO() obfuscator gets 
a long stare of disapproval...

Robin.

>> my immediate instinct is to be suspicious, and now I've got to go off
>> and double-check that if do_something_with() really expects a pointer
>> it's also robust against PTR_ERR values. Off-hand I can't think of any
>> APIs that work that way in the areas with which I'm familiar, so it
>> would be a pretty unusual and non-obvious thing.
> 

Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types

[Andrzej Hajda]

On 24.06.2020 14:14, Rafael J. Wysocki wrote:
> On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>> Many resource acquisition functions return error value encapsulated in
>> pointer instead of integer value. To simplify coding we can use macro
>> which will accept both types of error.
>> With this patch user can use:
>>          probe_err(dev, ptr, ...)
>> instead of:
>>          probe_err(dev, PTR_ERR(ptr), ...)
>> Without loosing old functionality:
>>          probe_err(dev, err, ...)
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> The separation of this change from patch [1/5] looks kind of artificial to me.
>
> You are introducing a new function anyway, so why not to make it what
> you want right away?


Two reasons:

1.This patch is my recent idea, I didn't want to mix it with already 
reviewed code.

2. This patch could be treated hacky by some devs due to macro 
definition and type-casting.


If both patches are OK I can merge them of course into one.


Regards

Andrzej


>
>> ---
>>   drivers/base/core.c    | 25 ++-----------------------
>>   include/linux/device.h | 25 ++++++++++++++++++++++++-
>>   2 files changed, 26 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 2a96954d5460..df283c62d9c0 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -3953,28 +3953,7 @@ 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.
>> - * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
>> - * later by reading devices_deferred debugfs attribute.
>> - * 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, ...)
>> +int __probe_err(const struct device *dev, int err, const char *fmt, ...)
>>   {
>>          struct va_format vaf;
>>          va_list args;
>> @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...)
>>
>>          return err;
>>   }
>> -EXPORT_SYMBOL_GPL(probe_err);
>> +EXPORT_SYMBOL_GPL(__probe_err);
>>
>>   static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>>   {
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 40a90d9bf799..22d3c3d4f461 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void);
>>   void device_links_supplier_sync_state_resume(void);
>>
>>   extern __printf(3, 4)
>> -int probe_err(const struct device *dev, int err, const char *fmt, ...);
>> +int __probe_err(const struct device *dev, int err, const char *fmt, ...);
>> +
>> +/**
>> + * probe_err - probe error check and log helper
>> + * @dev: the pointer to the struct device
>> + * @err: error value to test, can be integer or pointer type
>> + * @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.
>> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
>> + * later by reading devices_deferred debugfs attribute.
>> + * It replaces code sequence:
>> + *     if (err != -EPROBE_DEFER)
>> + *             dev_err(dev, ...);
>> + *     return err;
>> + * with
>> + *     return probe_err(dev, err, ...);
>> + *
>> + * Returns @err.
>> + *
>> + */
>> +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args)
>>
>>   /* Create alias, so I can be autoloaded. */
>>   #define MODULE_ALIAS_CHARDEV(major,minor) \
>> --
>> 2.17.1
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://protect2.fireeye.com/url?k=fe383567-a3a29cc4-fe39be28-002590f5b904-1faeb9cf68e31587&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel
>

Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types

[Andrzej Hajda]

On 24.06.2020 14:30, Greg Kroah-Hartman wrote:
> On Wed, Jun 24, 2020 at 01:41:25PM +0200, Andrzej Hajda wrote:
>> Many resource acquisition functions return error value encapsulated in
>> pointer instead of integer value. To simplify coding we can use macro
>> which will accept both types of error.
>> With this patch user can use:
>> 	probe_err(dev, ptr, ...)
>> instead of:
>> 	probe_err(dev, PTR_ERR(ptr), ...)
>> Without loosing old functionality:
>> 	probe_err(dev, err, ...)
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>>   drivers/base/core.c    | 25 ++-----------------------
>>   include/linux/device.h | 25 ++++++++++++++++++++++++-
>>   2 files changed, 26 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 2a96954d5460..df283c62d9c0 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -3953,28 +3953,7 @@ 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.
>> - * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
>> - * later by reading devices_deferred debugfs attribute.
>> - * 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, ...)
>> +int __probe_err(const struct device *dev, int err, const char *fmt, ...)
>>   {
>>   	struct va_format vaf;
>>   	va_list args;
>> @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...)
>>   
>>   	return err;
>>   }
>> -EXPORT_SYMBOL_GPL(probe_err);
>> +EXPORT_SYMBOL_GPL(__probe_err);
>>   
>>   static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>>   {
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 40a90d9bf799..22d3c3d4f461 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void);
>>   void device_links_supplier_sync_state_resume(void);
>>   
>>   extern __printf(3, 4)
>> -int probe_err(const struct device *dev, int err, const char *fmt, ...);
>> +int __probe_err(const struct device *dev, int err, const char *fmt, ...);
>> +
>> +/**
>> + * probe_err - probe error check and log helper
>> + * @dev: the pointer to the struct device
>> + * @err: error value to test, can be integer or pointer type
>> + * @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.
>> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
>> + * later by reading devices_deferred debugfs attribute.
>> + * It replaces code sequence:
>> + * 	if (err != -EPROBE_DEFER)
>> + * 		dev_err(dev, ...);
>> + * 	return err;
>> + * with
>> + * 	return probe_err(dev, err, ...);
>> + *
>> + * Returns @err.
>> + *
>> + */
>> +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args)
> Shouldn't that be "unsigned long" instead of "long"?  That's what we put
> pointers in last I looked...

Unless we know this is error inside pointer, in such case we follow 
practice from PTR_ERR function.


Regards

Andrzej


>
> thanks,
>
> greg k-h
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://protect2.fireeye.com/url?k=75712e41-28bf2f92-7570a50e-000babff317b-a5a76e98e30aecc2&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel
>

Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types

[Rafael J. Wysocki]

On Wed, Jun 24, 2020 at 4:44 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
>
> On 24.06.2020 14:14, Rafael J. Wysocki wrote:
> > On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
> >> Many resource acquisition functions return error value encapsulated in
> >> pointer instead of integer value. To simplify coding we can use macro
> >> which will accept both types of error.
> >> With this patch user can use:
> >>          probe_err(dev, ptr, ...)
> >> instead of:
> >>          probe_err(dev, PTR_ERR(ptr), ...)
> >> Without loosing old functionality:
> >>          probe_err(dev, err, ...)
> >>
> >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> > The separation of this change from patch [1/5] looks kind of artificial to me.
> >
> > You are introducing a new function anyway, so why not to make it what
> > you want right away?
>
>
> Two reasons:
>
> 1.This patch is my recent idea, I didn't want to mix it with already
> reviewed code.
>
> 2. This patch could be treated hacky by some devs due to macro
> definition and type-casting.

Fair enough.

There is some opposition against the $subject one, so I guess it may
be dropped even.

Thanks!

Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper

[Robin Murphy]

On 2020-06-24 15:02, Mark Brown wrote:
> On Wed, Jun 24, 2020 at 04:45:28PM +0300, Andy Shevchenko wrote:
>> On Wed, Jun 24, 2020 at 4:27 PM Mark Brown <broonie@kernel.org> wrote:
> 
>>> As I said down the thread that's not a great pattern since it means that
>>> probe deferral errors never get displayed and users have a hard time
>>> figuring out why their driver isn't instantiating.
> 
>> Don't we have a file in the debugfs to list deferred drivers?
> 
> Part of what this patch series aims to solve is that that list is not
> very useful since it doesn't provide any information on how things got
> deferred which means it's of no use in trying to figure out any
> problems.
> 
>> In the case of deferred probes the errors out of it makes users more
>> miserable in order to look through tons of spam and lose really useful
>> data in the logs.
> 
> I seem to never manage to end up using any of the systems which generate
> excessive deferrals.

Be thankful... And count me in as one of those miserable users; here's one
of mine being bad enough without even printing any specific messages about
deferring ;)

Robin.

-----

[robin@weasel-cheese ~]$ dmesg | grep dwmmc
[    3.046297] dwmmc_rockchip ff0c0000.mmc: IDMAC supports 32-bit address mode.
[    3.054312] dwmmc_rockchip ff0c0000.mmc: Using internal DMA controller.
[    3.061774] dwmmc_rockchip ff0c0000.mmc: Version ID is 270a
[    3.068101] dwmmc_rockchip ff0c0000.mmc: DW MMC controller at irq 30,32 bit host data width,256 deep fifo
[    3.079638] dwmmc_rockchip ff0d0000.mmc: IDMAC supports 32-bit address mode.
[    3.087678] dwmmc_rockchip ff0d0000.mmc: Using internal DMA controller.
[    3.095134] dwmmc_rockchip ff0d0000.mmc: Version ID is 270a
[    3.101480] dwmmc_rockchip ff0d0000.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo
[    3.113071] dwmmc_rockchip ff0f0000.mmc: IDMAC supports 32-bit address mode.
[    3.121110] dwmmc_rockchip ff0f0000.mmc: Using internal DMA controller.
[    3.128565] dwmmc_rockchip ff0f0000.mmc: Version ID is 270a
[    3.134886] dwmmc_rockchip ff0f0000.mmc: DW MMC controller at irq 32,32 bit host data width,256 deep fifo
[    3.948510] dwmmc_rockchip ff0c0000.mmc: IDMAC supports 32-bit address mode.
[    3.956475] dwmmc_rockchip ff0c0000.mmc: Using internal DMA controller.
[    3.963884] dwmmc_rockchip ff0c0000.mmc: Version ID is 270a
[    3.970133] dwmmc_rockchip ff0c0000.mmc: DW MMC controller at irq 30,32 bit host data width,256 deep fifo
[    4.141231] dwmmc_rockchip ff0d0000.mmc: IDMAC supports 32-bit address mode.
[    4.149178] dwmmc_rockchip ff0d0000.mmc: Using internal DMA controller.
[    4.156582] dwmmc_rockchip ff0d0000.mmc: Version ID is 270a
[    4.162823] dwmmc_rockchip ff0d0000.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo
[    4.175606] dwmmc_rockchip ff0f0000.mmc: IDMAC supports 32-bit address mode.
[    4.183540] dwmmc_rockchip ff0f0000.mmc: Using internal DMA controller.
[    4.190946] dwmmc_rockchip ff0f0000.mmc: Version ID is 270a
[    4.197196] dwmmc_rockchip ff0f0000.mmc: DW MMC controller at irq 32,32 bit host data width,256 deep fifo
[    4.250758] dwmmc_rockchip ff0d0000.mmc: IDMAC supports 32-bit address mode.
[    4.258688] dwmmc_rockchip ff0d0000.mmc: Using internal DMA controller.
[    4.266104] dwmmc_rockchip ff0d0000.mmc: Version ID is 270a
[    4.272358] dwmmc_rockchip ff0d0000.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo
[    4.285390] dwmmc_rockchip ff0d0000.mmc: IDMAC supports 32-bit address mode.
[    4.293333] dwmmc_rockchip ff0d0000.mmc: Using internal DMA controller.
[    4.300750] dwmmc_rockchip ff0d0000.mmc: Version ID is 270a
[    4.307005] dwmmc_rockchip ff0d0000.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo
[    4.971373] dwmmc_rockchip ff0f0000.mmc: Successfully tuned phase to 134
[    5.027225] dwmmc_rockchip ff0d0000.mmc: IDMAC supports 32-bit address mode.
[    5.035339] dwmmc_rockchip ff0d0000.mmc: Using internal DMA controller.
[    5.042769] dwmmc_rockchip ff0d0000.mmc: Version ID is 270a
[    5.049050] dwmmc_rockchip ff0d0000.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo
[   24.727583] dwmmc_rockchip ff0d0000.mmc: IDMAC supports 32-bit address mode.
[   24.745541] dwmmc_rockchip ff0d0000.mmc: Using internal DMA controller.
[   24.753003] dwmmc_rockchip ff0d0000.mmc: Version ID is 270a
[   24.763289] dwmmc_rockchip ff0d0000.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo
[   25.589620] dwmmc_rockchip ff0d0000.mmc: IDMAC supports 32-bit address mode.
[   25.603066] dwmmc_rockchip ff0d0000.mmc: Using internal DMA controller.
[   25.615283] dwmmc_rockchip ff0d0000.mmc: Version ID is 270a
[   25.627911] dwmmc_rockchip ff0d0000.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo
[   25.643469] dwmmc_rockchip ff0d0000.mmc: IDMAC supports 32-bit address mode.
[   25.651532] dwmmc_rockchip ff0d0000.mmc: Using internal DMA controller.
[   25.658960] dwmmc_rockchip ff0d0000.mmc: Version ID is 270a
[   25.665246] dwmmc_rockchip ff0d0000.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo
[   25.677154] dwmmc_rockchip ff0d0000.mmc: allocated mmc-pwrseq

Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types

[Mark Brown]

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

On Wed, Jun 24, 2020 at 03:25:33PM +0100, Robin Murphy wrote:

> And yeah, anyone who pipes up suggesting that places where an ERR_PTR value
> could be passed to probe_err() could simply refactor IS_ERR() checks with
> more uses of the god-awful PTR_ERR_OR_ZERO() obfuscator gets a long stare of
> disapproval...

We could also have a probe_err_ptr() or something that took an ERR_PTR()
instead if there really were an issue with explicitly doing this.

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

Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types

[Robin Murphy]

On 2020-06-24 16:04, Mark Brown wrote:
> On Wed, Jun 24, 2020 at 03:25:33PM +0100, Robin Murphy wrote:
> 
>> And yeah, anyone who pipes up suggesting that places where an ERR_PTR value
>> could be passed to probe_err() could simply refactor IS_ERR() checks with
>> more uses of the god-awful PTR_ERR_OR_ZERO() obfuscator gets a long stare of
>> disapproval...
> 
> We could also have a probe_err_ptr() or something that took an ERR_PTR()
> instead if there really were an issue with explicitly doing this.

Yeah, for all my lyrical objection, a static inline <blah>_ptr_err() 
helper to wrap <blah>_err() with sensible type checking might actually 
be an OK compromise if people really feel strongly for having that utility.

(and then we can debate whether it should also convert NULL to -ENOMEM 
and !IS_ERR to 0... :D)

Robin.

Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper

[Mark Brown]

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

On Wed, Jun 24, 2020 at 04:00:34PM +0100, Robin Murphy wrote:

> Be thankful... And count me in as one of those miserable users; here's one
> of mine being bad enough without even printing any specific messages about
> deferring ;)

> [robin@weasel-cheese ~]$ dmesg | grep dwmmc
> [    3.046297] dwmmc_rockchip ff0c0000.mmc: IDMAC supports 32-bit address mode.
> [    3.054312] dwmmc_rockchip ff0c0000.mmc: Using internal DMA controller.
> [    3.061774] dwmmc_rockchip ff0c0000.mmc: Version ID is 270a
> [    3.068101] dwmmc_rockchip ff0c0000.mmc: DW MMC controller at irq 30,32 bit host data width,256 deep fifo

Looking at that driver it's going to have lots of problems whatever
happens - it's printing out a huge amount of stuff before it's finished
acquiring resources.

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

Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types

[Andrzej Hajda]

On 24.06.2020 17:16, Robin Murphy wrote:
> On 2020-06-24 16:04, Mark Brown wrote:
>> On Wed, Jun 24, 2020 at 03:25:33PM +0100, Robin Murphy wrote:
>>
>>> And yeah, anyone who pipes up suggesting that places where an 
>>> ERR_PTR value
>>> could be passed to probe_err() could simply refactor IS_ERR() checks 
>>> with
>>> more uses of the god-awful PTR_ERR_OR_ZERO() obfuscator gets a long 
>>> stare of
>>> disapproval...
>>
>> We could also have a probe_err_ptr() or something that took an ERR_PTR()
>> instead if there really were an issue with explicitly doing this.
>
> Yeah, for all my lyrical objection, a static inline <blah>_ptr_err() 
> helper to wrap <blah>_err() with sensible type checking might actually 
> be an OK compromise if people really feel strongly for having that 
> utility.


I have proposed such thing in my previous iteration[1], except it was 
macro because of variadic arguments.

With current version we save 8 chars and hacky macro, with the old 
version we save only 4 chars and more clear construct - less tempting 
solution for me.

Personally I prefer the current version - it does not seems to me more 
dangerous than all these PTR_ERR, IS_ERR,ERR_PTR helpers, but can 
prevent expression split across  multiple lines due to 80char limit.

Probably the simplest solution is to drop this patch, I will do it then.

[1]: 
https://lwn.net/ml/linux-kernel/20181220102247.4911-4-a.hajda@samsung.com/


Regards

Andrzej


>
> (and then we can debate whether it should also convert NULL to -ENOMEM 
> and !IS_ERR to 0... :D)
>
> Robin.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://protect2.fireeye.com/url?k=074420c0-5ada8e5a-0745ab8f-0cc47a336fae-bba8bb4caf96e14d&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel 
>
>

Re: [RESEND PATCH v5 5/5] drm/bridge: lvds-codec: simplify error handling code

[Laurent Pinchart]

Hi Andrzej,

On Wed, Jun 24, 2020 at 04:03:30PM +0200, Andrzej Hajda wrote:
> On 24.06.2020 15:33, Laurent Pinchart wrote:
> > On Wed, Jun 24, 2020 at 01:41:27PM +0200, Andrzej Hajda wrote:
> >> Using probe_err code has following advantages:
> >> - shorter code,
> >> - recorded defer probe reason for debugging,
> >> - uniform error code logging.
> >>
> >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> >> ---
> >>   drivers/gpu/drm/bridge/lvds-codec.c | 9 ++-------
> >>   1 file changed, 2 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> >> index 24fb1befdfa2..c76fa0239e39 100644
> >> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> >> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> >> @@ -71,13 +71,8 @@ static int lvds_codec_probe(struct platform_device *pdev)
> >>   	lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
> >>   	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> >>   							     GPIOD_OUT_HIGH);
> >> -	if (IS_ERR(lvds_codec->powerdown_gpio)) {
> >> -		int err = PTR_ERR(lvds_codec->powerdown_gpio);
> >> -
> >> -		if (err != -EPROBE_DEFER)
> >> -			dev_err(dev, "powerdown GPIO failure: %d\n", err);
> >> -		return err;
> >> -	}
> >> +	if (IS_ERR(lvds_codec->powerdown_gpio))
> >> +		return probe_err(dev, lvds_codec->powerdown_gpio, "powerdown GPIO failure\n");
> >
> > Line wrap please.
> 
> I hoped that with latest checkpatch line length limit increase from 80 
> to 100 it is acceptable :) but apparently not [1].

I'm all for using longer lines when that improves readability, but in
this case I think it's easy enough to split the line. A longer line
limit doesn't mean we're forced to generate longer lines :-)

On a side note, I've been working on a C++ userspace project where we
had to decide on a coding style. Line length was one parameter, and we
went for a soft limit of 80 columns, and a hard limit of 120 columns.
This works quite well so far. The only pain point is that clang-format
(we use it, wrapped in a python script, to detect coding style
violations) doesn't understand soft and hard limits for line lengths.

> [1]: 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
>
> > It bothers me that the common pattern of writing the error code at the
> > end of the message isn't possible anymore. Maybe I'll get used to it,
> > but it removes some flexibility.
> 
> Yes, but it gives uniformity :) and now with %pe printk format it 
> changes anyway.
> 
> >>   	/* Locate the panel DT node. */
> >>   	panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);

-- 
Regards,

Laurent Pinchart

Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types

[Andy Shevchenko]

On Wed, Jun 24, 2020 at 10:40 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 24.06.2020 17:16, Robin Murphy wrote:

...

> I have proposed such thing in my previous iteration[1], except it was
> macro because of variadic arguments.

You may have a function with variadic arguments. Macros are beasts and
make in some cases more harm than help.

-- 
With Best Regards,
Andy Shevchenko

Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types

[Andrzej Hajda]

On 25.06.2020 10:41, Andy Shevchenko wrote:
> On Wed, Jun 24, 2020 at 10:40 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>> On 24.06.2020 17:16, Robin Murphy wrote:
> ...
>
>> I have proposed such thing in my previous iteration[1], except it was
>> macro because of variadic arguments.
> You may have a function with variadic arguments. Macros are beasts and
> make in some cases more harm than help.


What harm it can do in this particular case?

With macro we have simple straightforward one-liner, with quite good 
type-checking.

Maybe I am wrong, but I suspect creation of variadic function would 
require much more coding.


Regards

Andrzej