2020-06-24 11:42:12

by Andrzej Hajda

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

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

2020-06-24 11:42:19

by Andrzej Hajda

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

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 <[email protected]>
---
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

2020-06-24 11:42:26

by Andrzej Hajda

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

/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 <[email protected]>
Reviewed-by: Mark Brown <[email protected]>
Reviewed-by: Javier Martinez Canillas <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
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

2020-06-24 11:43:40

by Andrzej Hajda

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

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 <[email protected]>
---
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

2020-06-24 11:44:12

by Andrzej Hajda

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

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 <[email protected]>
Reviewed-by: Javier Martinez Canillas <[email protected]>
Reviewed-by: Mark Brown <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
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

2020-06-24 11:44:42

by Andrzej Hajda

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

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

Signed-off-by: Andrzej Hajda <[email protected]>
---
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

2020-06-24 11:54:50

by Rafael J. Wysocki

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

On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda <[email protected]> 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 <[email protected]>
> Reviewed-by: Javier Martinez Canillas <[email protected]>
> Reviewed-by: Mark Brown <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>

Reviewed-by Rafael J. Wysocki <[email protected]>

> ---
> 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
>

2020-06-24 12:12:50

by Rafael J. Wysocki

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

On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda <[email protected]> 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 <[email protected]>
> Reviewed-by: Mark Brown <[email protected]>
> Reviewed-by: Javier Martinez Canillas <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> ---
> 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
>

2020-06-24 12:16:47

by Rafael J. Wysocki

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

On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda <[email protected]> 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 <[email protected]>

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
>

2020-06-24 12:32:17

by Greg Kroah-Hartman

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

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 <[email protected]>
> ---
> 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

2020-06-24 12:33:01

by Greg Kroah-Hartman

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

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 <[email protected]>
> Reviewed-by: Javier Martinez Canillas <[email protected]>
> Reviewed-by: Mark Brown <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> ---
> 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

2020-06-24 12:35:47

by Greg Kroah-Hartman

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

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 <[email protected]>
> Reviewed-by: Mark Brown <[email protected]>
> Reviewed-by: Javier Martinez Canillas <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> ---
> 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

2020-06-24 12:38:45

by Robin Murphy

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

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 <[email protected]>
> ---
> 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) \
>

2020-06-24 12:57:37

by Andy Shevchenko

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

On Wed, Jun 24, 2020 at 2:41 PM Andrzej Hajda <[email protected]> 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

2020-06-24 12:57:40

by Andy Shevchenko

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

On Wed, Jun 24, 2020 at 3:37 PM Robin Murphy <[email protected]> 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

2020-06-24 13:14:47

by Andrzej Hajda

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

On 24.06.2020 14:53, Andy Shevchenko wrote:
> On Wed, Jun 24, 2020 at 2:41 PM Andrzej Hajda <[email protected]> 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


>

2020-06-24 13:26:46

by Mark Brown

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

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.

Attachments:

2020-06-24 13:27:37

by Laurent Pinchart

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

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 <[email protected]>
> > Reviewed-by: Javier Martinez Canillas <[email protected]>
> > Reviewed-by: Mark Brown <[email protected]>
> > Reviewed-by: Andy Shevchenko <[email protected]>
> > ---
> > 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

2020-06-24 13:28:46

by Mark Brown

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

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.

Attachments:

2020-06-24 13:29:47

by Andrzej Hajda

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

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 <[email protected]>
>> Reviewed-by: Mark Brown <[email protected]>
>> Reviewed-by: Javier Martinez Canillas <[email protected]>
>> Reviewed-by: Andy Shevchenko <[email protected]>
>> ---
>> 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
> [email protected]
> https://protect2.fireeye.com/url?k=28daee95-7508f5cd-28db65da-0cc47a31c8b4-b3e8d1affcda9c08&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel
>

2020-06-24 13:30:14

by Andrzej Hajda

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

On 24.06.2020 14:11, Rafael J. Wysocki wrote:
> On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda <[email protected]> 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 <[email protected]>
>> Reviewed-by: Mark Brown <[email protected]>
>> Reviewed-by: Javier Martinez Canillas <[email protected]>
>> Reviewed-by: Andy Shevchenko <[email protected]>
>> ---
>> 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
> [email protected]
> https://protect2.fireeye.com/url?k=5adb7884-070fc5c0-5adaf3cb-0cc47a31381a-5588a624ab84213e&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel
>

2020-06-24 13:30:59

by Laurent Pinchart

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

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 <[email protected]>
> > ---
> > 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

2020-06-24 13:36:52

by Laurent Pinchart

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

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 <[email protected]>
> ---
> 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

2020-06-24 13:47:54

by Andy Shevchenko

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

On Wed, Jun 24, 2020 at 4:27 PM Mark Brown <[email protected]> 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

2020-06-24 14:05:05

by Andrzej Hajda

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

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 <[email protected]>
>> ---
>> 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);

2020-06-24 14:06:01

by Andrzej Hajda

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

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 <[email protected]>
>>> Reviewed-by: Javier Martinez Canillas <[email protected]>
>>> Reviewed-by: Mark Brown <[email protected]>
>>> Reviewed-by: Andy Shevchenko <[email protected]>
>>> ---
>>> 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.

2020-06-24 14:06:32

by Mark Brown

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

On Wed, Jun 24, 2020 at 04:45:28PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 24, 2020 at 4:27 PM Mark Brown <[email protected]> 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.

Attachments:

2020-06-24 14:12:48

by Mark Brown

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

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.

Attachments:

2020-06-24 14:28:27

by Robin Murphy

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

On 2020-06-24 13:55, Andy Shevchenko wrote:
> On Wed, Jun 24, 2020 at 3:37 PM Robin Murphy <[email protected]> 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.
>

2020-06-24 14:45:55

by Andrzej Hajda

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

On 24.06.2020 14:14, Rafael J. Wysocki wrote:
> On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda <[email protected]> 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 <[email protected]>
> 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
> [email protected]
> https://protect2.fireeye.com/url?k=fe383567-a3a29cc4-fe39be28-002590f5b904-1faeb9cf68e31587&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel
>

2020-06-24 14:50:40

by Andrzej Hajda

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

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 <[email protected]>
>> ---
>> 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
> [email protected]
> https://protect2.fireeye.com/url?k=75712e41-28bf2f92-7570a50e-000babff317b-a5a76e98e30aecc2&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel
>

2020-06-24 14:57:26

by Rafael J. Wysocki

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

On Wed, Jun 24, 2020 at 4:44 PM Andrzej Hajda <[email protected]> wrote:
>
>
> On 24.06.2020 14:14, Rafael J. Wysocki wrote:
> > On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda <[email protected]> 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 <[email protected]>
> > 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!

2020-06-24 15:02:13

by Robin Murphy

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

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 <[email protected]> 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

2020-06-24 15:05:16

by Mark Brown

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

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.

Attachments:

2020-06-24 15:18:51

by Robin Murphy

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

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.

2020-06-24 15:30:27

by Mark Brown

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

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.

Attachments:

2020-06-24 19:43:17

by Andrzej Hajda

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

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/[email protected]/


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
> [email protected]
> https://protect2.fireeye.com/url?k=074420c0-5ada8e5a-0745ab8f-0cc47a336fae-bba8bb4caf96e14d&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel
>
>

2020-06-24 22:04:14

by Laurent Pinchart

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

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 <[email protected]>
> >> ---
> >> 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

2020-06-25 09:46:06

by Andy Shevchenko

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

On Wed, Jun 24, 2020 at 10:40 PM Andrzej Hajda <[email protected]> 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

2020-06-25 10:23:12

by Andrzej Hajda

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

On 25.06.2020 10:41, Andy Shevchenko wrote:
> On Wed, Jun 24, 2020 at 10:40 PM Andrzej Hajda <[email protected]> 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