Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v7 0/4] driver core: add probe error check helper
       [not found] <CGME20200629112247eucas1p2f5a1c0a4ffe3a2571658646a7b369cfd@eucas1p2.samsung.com>
@ 2020-06-29 11:22 ` Andrzej Hajda
       [not found]   ` <CGME20200629112248eucas1p187e5dac2f4f6120aacbc86e48ad0fff9@eucas1p1.samsung.com>
                     ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Andrzej Hajda @ 2020-06-29 11:22 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jernej Skrabec, Rafael J. Wysocki, Jonas Karlman,
	Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS,
	Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda,
	andy.shevchenko, Mark Brown, Laurent Pinchart, Daniel Vetter,
	linux-arm-kernel, Marek Szyprowski

Hi All,

Thanks for comments.

Changes since v6:
- removed leftovers from old naming scheme in commit descritions,
- added R-Bs.

Changes since v5:
- removed patch adding macro, dev_err_probe(dev, PTR_ERR(ptr), ...) should be used instead,
- added dev_dbg logging in case of -EPROBE_DEFER,
- renamed functions and vars according to comments,
- extended docs,
- cosmetics.

Original message (with small adjustments):

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.

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 (4):
  driver core: add device probe log helper
  driver core: add deferring probe reason to devices_deferred property
  drm/bridge/sii8620: fix resource acquisition error handling
  drm/bridge: lvds-codec: simplify error handling

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

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v7 1/4] driver core: add device probe log helper
       [not found]   ` <CGME20200629112248eucas1p187e5dac2f4f6120aacbc86e48ad0fff9@eucas1p1.samsung.com>
@ 2020-06-29 11:22     ` Andrzej Hajda
  2020-06-29 16:38       ` Andy Shevchenko
  2020-06-30 15:35       ` [PATCH v7 1/4] driver core: add device probe log helper Mark Brown
  0 siblings, 2 replies; 11+ messages in thread
From: Andrzej Hajda @ 2020-06-29 11:22 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jernej Skrabec, Rafael J. Wysocki, Jonas Karlman,
	Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS,
	Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda,
	andy.shevchenko, Mark Brown, Laurent Pinchart, Daniel Vetter,
	linux-arm-kernel, Marek Szyprowski

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 omitted or implemented only
partially.
dev_err_probe helps to replace such code sequences with simple call,
so code:
	if (err != -EPROBE_DEFER)
		dev_err(dev, ...);
	return err;
becomes:
	return dev_err_probe(dev, err, ...);

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>
---
 drivers/base/core.c    | 42 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |  3 +++
 2 files changed, 45 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 67d39a90b45c..3a827c82933f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3953,6 +3953,48 @@ define_dev_printk_level(_dev_info, KERN_INFO);
 
 #endif
 
+/**
+ * dev_err_probe - 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 debug or error message depending if the error value is
+ * -EPROBE_DEFER and propagate error upwards.
+ * It replaces code sequence:
+ * 	if (err != -EPROBE_DEFER)
+ * 		dev_err(dev, ...);
+ * 	else
+ * 		dev_dbg(dev, ...);
+ * 	return err;
+ * with
+ * 	return dev_err_probe(dev, err, ...);
+ *
+ * Returns @err.
+ *
+ */
+int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+
+	va_start(args, fmt);
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	if (err != -EPROBE_DEFER)
+		dev_err(dev, "error %d: %pV", err, &vaf);
+	else
+		dev_dbg(dev, "error %d: %pV", err, &vaf);
+
+	va_end(args);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(dev_err_probe);
+
 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..6b2272ae9af8 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 dev_err_probe(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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v7 2/4] driver core: add deferring probe reason to devices_deferred property
       [not found]   ` <CGME20200629112249eucas1p160b845444f8fbad96bdec41e9d3938da@eucas1p1.samsung.com>
@ 2020-06-29 11:22     ` Andrzej Hajda
  2020-06-29 16:36       ` Andy Shevchenko
  0 siblings, 1 reply; 11+ messages in thread
From: Andrzej Hajda @ 2020-06-29 11:22 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jernej Skrabec, Rafael J. Wysocki, Jonas Karlman,
	Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS,
	Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda,
	andy.shevchenko, Mark Brown, Laurent Pinchart, Daniel Vetter,
	linux-arm-kernel, Marek Szyprowski

/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 dev_err_probe function introduced recently,
ie. if dev_err_probe 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>
Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>
---
 drivers/base/base.h |  3 +++
 drivers/base/core.c |  8 ++++++--
 drivers/base/dd.c   | 23 ++++++++++++++++++++++-
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 95c22c0f9036..6954fccab3d7 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_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 device_set_deferred_probe_reson(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 3a827c82933f..fee047f03681 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3963,6 +3963,8 @@ define_dev_printk_level(_dev_info, KERN_INFO);
  * This helper implements common pattern present in probe functions for error
  * checking: print debug or error message depending if the error value is
  * -EPROBE_DEFER and propagate error upwards.
+ * In case of -EPROBE_DEFER it sets also 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, ...);
@@ -3984,10 +3986,12 @@ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
 	vaf.fmt = fmt;
 	vaf.va = &args;
 
-	if (err != -EPROBE_DEFER)
+	if (err != -EPROBE_DEFER) {
 		dev_err(dev, "error %d: %pV", err, &vaf);
-	else
+	} else {
+		device_set_deferred_probe_reson(dev, &vaf);
 		dev_dbg(dev, "error %d: %pV", err, &vaf);
+	}
 
 	va_end(args);
 
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9a1d940342ac..dd5683b61f74 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_reason);
+		dev->p->deferred_probe_reason = NULL;
 	}
 	mutex_unlock(&deferred_probe_mutex);
 }
@@ -211,6 +214,23 @@ void device_unblock_probing(void)
 	driver_deferred_probe_trigger();
 }
 
+/**
+ * device_set_deferred_probe_reson() - Set defer probe reason message for device
+ * @dev: the pointer to the struct device
+ * @vaf: the pointer to va_format structure with message
+ */
+void device_set_deferred_probe_reson(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_reason);
+	dev->p->deferred_probe_reason = 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 +241,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_reason ?: "\n");
 
 	mutex_unlock(&deferred_probe_mutex);
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v7 3/4] drm/bridge/sii8620: fix resource acquisition error handling
       [not found]   ` <CGME20200629112249eucas1p12fc95f88729c12c78b1a5bfe32afad8f@eucas1p1.samsung.com>
@ 2020-06-29 11:22     ` Andrzej Hajda
  0 siblings, 0 replies; 11+ messages in thread
From: Andrzej Hajda @ 2020-06-29 11:22 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jernej Skrabec, Rafael J. Wysocki, Jonas Karlman,
	Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS,
	Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda,
	andy.shevchenko, Mark Brown, Laurent Pinchart, Daniel Vetter,
	linux-arm-kernel, Marek Szyprowski

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

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/bridge/sil-sii8620.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
index 92acd336aa89..389c1f029774 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -2299,10 +2299,9 @@ 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 dev_err_probe(dev, PTR_ERR(ctx->clk_xtal),
+				     "failed to get xtal clock from DT\n");
 
 	if (!client->irq) {
 		dev_err(dev, "no irq provided\n");
@@ -2313,16 +2312,14 @@ 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 dev_err_probe(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 dev_err_probe(dev, PTR_ERR(ctx->gpio_reset),
+				     "failed to get reset gpio from DT\n");
 
 	ctx->supplies[0].supply = "cvcc10";
 	ctx->supplies[1].supply = "iovcc18";
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v7 4/4] drm/bridge: lvds-codec: simplify error handling
       [not found]   ` <CGME20200629112250eucas1p162ce47eab8946c45357a7e2c03a6da9a@eucas1p1.samsung.com>
@ 2020-06-29 11:22     ` Andrzej Hajda
  0 siblings, 0 replies; 11+ messages in thread
From: Andrzej Hajda @ 2020-06-29 11:22 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jernej Skrabec, Rafael J. Wysocki, Jonas Karlman,
	Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS,
	Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda,
	andy.shevchenko, Mark Brown, Laurent Pinchart, Daniel Vetter,
	linux-arm-kernel, Marek Szyprowski

Using dev_err_probe 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>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/bridge/lvds-codec.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
index 24fb1befdfa2..f19d9f7a5db2 100644
--- a/drivers/gpu/drm/bridge/lvds-codec.c
+++ b/drivers/gpu/drm/bridge/lvds-codec.c
@@ -71,13 +71,9 @@ 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 dev_err_probe(dev, PTR_ERR(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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v7 2/4] driver core: add deferring probe reason to devices_deferred property
  2020-06-29 11:22     ` [PATCH v7 2/4] driver core: add deferring probe reason to devices_deferred property Andrzej Hajda
@ 2020-06-29 16:36       ` Andy Shevchenko
  2020-06-30  8:32         ` Andrzej Hajda
  0 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2020-06-29 16:36 UTC (permalink / raw)
  To: Andrzej Hajda
  Cc: Jernej Skrabec, Rafael J. Wysocki, Greg Kroah-Hartman,
	Bartlomiej Zolnierkiewicz, Linux Kernel Mailing List,
	open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong,
	Jonas Karlman, Mark Brown, Laurent Pinchart, Daniel Vetter,
	linux-arm Mailing List, Marek Szyprowski

On Mon, Jun 29, 2020 at 2:22 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 dev_err_probe function introduced recently,
> ie. if dev_err_probe will be called with -EPROBE_DEFER instead of printk the message
> will be attached to deferred device and printed when user read devices_deferred

to a deferred

reads

> property.

...

> @@ -3984,10 +3986,12 @@ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
>         vaf.fmt = fmt;
>         vaf.va = &args;
>
> -       if (err != -EPROBE_DEFER)

> +       if (err != -EPROBE_DEFER) {

Why not positive conditional? (Okay, I'm fine with either in this case)

>                 dev_err(dev, "error %d: %pV", err, &vaf);
> -       else
> +       } else {
> +               device_set_deferred_probe_reson(dev, &vaf);
>                 dev_dbg(dev, "error %d: %pV", err, &vaf);
> +       }

To reduce churn, you may move {} addition to the first patch.

...

>         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_reason ?: "\n");

Hmm... "\t" will be dangling in the latter case.

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v7 1/4] driver core: add device probe log helper
  2020-06-29 11:22     ` [PATCH v7 1/4] driver core: add device probe log helper Andrzej Hajda
@ 2020-06-29 16:38       ` Andy Shevchenko
       [not found]         ` <CGME20200702134434eucas1p233a3f66f5bdb4b97f4f49d2d43d45297@eucas1p2.samsung.com>
  2020-06-30 15:35       ` [PATCH v7 1/4] driver core: add device probe log helper Mark Brown
  1 sibling, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2020-06-29 16:38 UTC (permalink / raw)
  To: Andrzej Hajda
  Cc: Jernej Skrabec, Rafael J. Wysocki, Greg Kroah-Hartman,
	Bartlomiej Zolnierkiewicz, Linux Kernel Mailing List,
	open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong,
	Jonas Karlman, Mark Brown, Laurent Pinchart, Daniel Vetter,
	linux-arm Mailing List, Marek Szyprowski

On Mon, Jun 29, 2020 at 2:22 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 omitted or implemented only
> partially.
> dev_err_probe helps to replace such code sequences with simple call,
> so code:
>         if (err != -EPROBE_DEFER)
>                 dev_err(dev, ...);
>         return err;
> becomes:
>         return dev_err_probe(dev, err, ...);

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

(I'm fine with current code, though, consider below comments)

> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>
> ---
>  drivers/base/core.c    | 42 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/device.h |  3 +++
>  2 files changed, 45 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 67d39a90b45c..3a827c82933f 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3953,6 +3953,48 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>
>  #endif
>
> +/**
> + * dev_err_probe - 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 debug or error message depending if the error value is
> + * -EPROBE_DEFER and propagate error upwards.
> + * It replaces code sequence:
> + *     if (err != -EPROBE_DEFER)
> + *             dev_err(dev, ...);
> + *     else
> + *             dev_dbg(dev, ...);
> + *     return err;
> + * with
> + *     return dev_err_probe(dev, err, ...);
> + *
> + * Returns @err.
> + *
> + */
> +int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
> +{
> +       struct va_format vaf;
> +       va_list args;
> +
> +       va_start(args, fmt);
> +       vaf.fmt = fmt;
> +       vaf.va = &args;
> +
> +       if (err != -EPROBE_DEFER)

Perhaps positive conditional.

> +               dev_err(dev, "error %d: %pV", err, &vaf);
> +       else
> +               dev_dbg(dev, "error %d: %pV", err, &vaf);

Perhaps {} to be added here.

> +
> +       va_end(args);
> +
> +       return err;
> +}
> +EXPORT_SYMBOL_GPL(dev_err_probe);
> +
>  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..6b2272ae9af8 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 dev_err_probe(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
>


-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v7 2/4] driver core: add deferring probe reason to devices_deferred property
  2020-06-29 16:36       ` Andy Shevchenko
@ 2020-06-30  8:32         ` Andrzej Hajda
  2020-06-30 10:00           ` Andy Shevchenko
  0 siblings, 1 reply; 11+ messages in thread
From: Andrzej Hajda @ 2020-06-30  8:32 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman,
	Rafael J. Wysocki, Linux Kernel Mailing List,
	open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong,
	Jonas Karlman, Mark Brown, Laurent Pinchart,
	linux-arm Mailing List, Marek Szyprowski


On 29.06.2020 18:36, Andy Shevchenko wrote:
> On Mon, Jun 29, 2020 at 2:22 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 dev_err_probe function introduced recently,
>> ie. if dev_err_probe will be called with -EPROBE_DEFER instead of printk the message
>> will be attached to deferred device and printed when user read devices_deferred
> to a deferred
>
> reads
OK, thx.
>
>> property.
> ...
>
>> @@ -3984,10 +3986,12 @@ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
>>          vaf.fmt = fmt;
>>          vaf.va = &args;
>>
>> -       if (err != -EPROBE_DEFER)
>> +       if (err != -EPROBE_DEFER) {
> Why not positive conditional? (Okay, I'm fine with either in this case)


I put more natural branch 1st.


>
>>                  dev_err(dev, "error %d: %pV", err, &vaf);
>> -       else
>> +       } else {
>> +               device_set_deferred_probe_reson(dev, &vaf);
>>                  dev_dbg(dev, "error %d: %pV", err, &vaf);
>> +       }
> To reduce churn, you may move {} addition to the first patch.


But then I need to explain why it is there :)


>
> ...
>
>>          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_reason ?: "\n");
> Hmm... "\t" will be dangling in the latter case


Hmm, I followed your advice [1] :)

[1]: 
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1787370.html


Regards

Andrzej


>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v7 2/4] driver core: add deferring probe reason to devices_deferred property
  2020-06-30  8:32         ` Andrzej Hajda
@ 2020-06-30 10:00           ` Andy Shevchenko
  0 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2020-06-30 10:00 UTC (permalink / raw)
  To: Andrzej Hajda
  Cc: Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman,
	Rafael J. Wysocki, Linux Kernel Mailing List,
	open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong,
	Jonas Karlman, Mark Brown, Laurent Pinchart,
	linux-arm Mailing List, Marek Szyprowski

On Tue, Jun 30, 2020 at 11:32 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 29.06.2020 18:36, Andy Shevchenko wrote:
> > On Mon, Jun 29, 2020 at 2:22 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 dev_err_probe function introduced recently,
> >> ie. if dev_err_probe will be called with -EPROBE_DEFER instead of printk the message
> >> will be attached to deferred device and printed when user read devices_deferred
> > to a deferred
> >
> > reads
> OK, thx.
> >
> >> property.

...

> >>          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_reason ?: "\n");
> > Hmm... "\t" will be dangling in the latter case
>
>
> Hmm, I followed your advice [1] :)
>
> [1]:
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1787370.html

Ah, indeed :-)
Okay, as I  said I'm fine with the code. Since it's debugfs, we are fine.

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v7 1/4] driver core: add device probe log helper
  2020-06-29 11:22     ` [PATCH v7 1/4] driver core: add device probe log helper Andrzej Hajda
  2020-06-29 16:38       ` Andy Shevchenko
@ 2020-06-30 15:35       ` Mark Brown
  1 sibling, 0 replies; 11+ messages in thread
From: Mark Brown @ 2020-06-30 15:35 UTC (permalink / raw)
  To: Andrzej Hajda
  Cc: Jernej Skrabec, Rafael J. Wysocki, Greg Kroah-Hartman,
	Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS,
	Russell King - ARM Linux, Neil Armstrong, Jonas Karlman,
	andy.shevchenko, Laurent Pinchart, Daniel Vetter,
	linux-arm-kernel, Marek Szyprowski

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

On Mon, Jun 29, 2020 at 01:22:39PM +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 omitted or implemented only
> partially.

Reviewed-by: Mark Brown <broonie@kernel.org>

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v8 2/5] driver core: add deferring probe reason to devices_deferred property
       [not found]         ` <CGME20200702134434eucas1p233a3f66f5bdb4b97f4f49d2d43d45297@eucas1p2.samsung.com>
@ 2020-07-02 13:44           ` Andrzej Hajda
  0 siblings, 0 replies; 11+ messages in thread
From: Andrzej Hajda @ 2020-07-02 13:44 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jernej Skrabec, Rafael J. Wysocki, Jonas Karlman,
	Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS,
	Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda,
	andy.shevchenko, Mark Brown, Laurent Pinchart, Daniel Vetter,
	linux-arm-kernel, Marek Szyprowski

/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 dev_err_probe function introduced
recently, ie. if dev_err_probe will be called with -EPROBE_DEFER instead of
printk the message will be attached to a deferred device and printed when user
reads 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>
Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>
---
v8:
- improved commit message
---
 drivers/base/base.h |  3 +++
 drivers/base/core.c |  8 ++++++--
 drivers/base/dd.c   | 23 ++++++++++++++++++++++-
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 95c22c0f9036..6954fccab3d7 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_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 device_set_deferred_probe_reson(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 3a827c82933f..fee047f03681 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3963,6 +3963,8 @@ define_dev_printk_level(_dev_info, KERN_INFO);
  * This helper implements common pattern present in probe functions for error
  * checking: print debug or error message depending if the error value is
  * -EPROBE_DEFER and propagate error upwards.
+ * In case of -EPROBE_DEFER it sets also 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, ...);
@@ -3984,10 +3986,12 @@ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
 	vaf.fmt = fmt;
 	vaf.va = &args;
 
-	if (err != -EPROBE_DEFER)
+	if (err != -EPROBE_DEFER) {
 		dev_err(dev, "error %d: %pV", err, &vaf);
-	else
+	} else {
+		device_set_deferred_probe_reson(dev, &vaf);
 		dev_dbg(dev, "error %d: %pV", err, &vaf);
+	}
 
 	va_end(args);
 
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9a1d940342ac..dd5683b61f74 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_reason);
+		dev->p->deferred_probe_reason = NULL;
 	}
 	mutex_unlock(&deferred_probe_mutex);
 }
@@ -211,6 +214,23 @@ void device_unblock_probing(void)
 	driver_deferred_probe_trigger();
 }
 
+/**
+ * device_set_deferred_probe_reson() - Set defer probe reason message for device
+ * @dev: the pointer to the struct device
+ * @vaf: the pointer to va_format structure with message
+ */
+void device_set_deferred_probe_reson(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_reason);
+	dev->p->deferred_probe_reason = 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 +241,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_reason ?: "\n");
 
 	mutex_unlock(&deferred_probe_mutex);
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20200629112247eucas1p2f5a1c0a4ffe3a2571658646a7b369cfd@eucas1p2.samsung.com>
2020-06-29 11:22 ` [PATCH v7 0/4] driver core: add probe error check helper Andrzej Hajda
     [not found]   ` <CGME20200629112248eucas1p187e5dac2f4f6120aacbc86e48ad0fff9@eucas1p1.samsung.com>
2020-06-29 11:22     ` [PATCH v7 1/4] driver core: add device probe log helper Andrzej Hajda
2020-06-29 16:38       ` Andy Shevchenko
     [not found]         ` <CGME20200702134434eucas1p233a3f66f5bdb4b97f4f49d2d43d45297@eucas1p2.samsung.com>
2020-07-02 13:44           ` [PATCH v8 2/5] driver core: add deferring probe reason to devices_deferred property Andrzej Hajda
2020-06-30 15:35       ` [PATCH v7 1/4] driver core: add device probe log helper Mark Brown
     [not found]   ` <CGME20200629112249eucas1p160b845444f8fbad96bdec41e9d3938da@eucas1p1.samsung.com>
2020-06-29 11:22     ` [PATCH v7 2/4] driver core: add deferring probe reason to devices_deferred property Andrzej Hajda
2020-06-29 16:36       ` Andy Shevchenko
2020-06-30  8:32         ` Andrzej Hajda
2020-06-30 10:00           ` Andy Shevchenko
     [not found]   ` <CGME20200629112249eucas1p12fc95f88729c12c78b1a5bfe32afad8f@eucas1p1.samsung.com>
2020-06-29 11:22     ` [PATCH v7 3/4] drm/bridge/sii8620: fix resource acquisition error handling Andrzej Hajda
     [not found]   ` <CGME20200629112250eucas1p162ce47eab8946c45357a7e2c03a6da9a@eucas1p1.samsung.com>
2020-06-29 11:22     ` [PATCH v7 4/4] drm/bridge: lvds-codec: simplify " Andrzej Hajda

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git