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
/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 <[email protected]>
Reviewed-by: Mark Brown <[email protected]>
Reviewed-by: Javier Martinez Canillas <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Rafael J. Wysocki <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Neil Armstrong <[email protected]>
---
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