* [PATCH v9 0/4] driver core: add probe error check helper [not found] <CGME20200713144331eucas1p25911c4ffa9315f632d8f6dd833588981@eucas1p2.samsung.com> @ 2020-07-13 14:43 ` Andrzej Hajda [not found] ` <CGME20200713144331eucas1p24107a30783dacc92887b80cf082be9fb@eucas1p2.samsung.com> ` (4 more replies) 0 siblings, 5 replies; 14+ messages in thread From: Andrzej Hajda @ 2020-07-13 14:43 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Jernej Skrabec, Rafael J. Wysocki, Jonas Karlman, Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, andy.shevchenko, Mark Brown, Laurent Pinchart, linux-arm-kernel, Marek Szyprowski Hi All, Thanks for comments. Changes since v8: - fixed typo in function name, - removed cocci script (added by mistake) Changes since v7: - improved commit message - added R-Bs Changes since v6: - removed leftovers from old naming scheme in commit descritions, - added R-Bs. Changes since v5: - removed patch adding macro, dev_err_probe(dev, PTR_ERR(ptr), ...) should be used instead, - added dev_dbg logging in case of -EPROBE_DEFER, - renamed functions and vars according to comments, - extended docs, - cosmetics. Original message (with small adjustments): Recently I took some time to re-check error handling in drivers probe code, and I have noticed that number of incorrect resource acquisition error handling increased and there are no other propositions which can cure the situation. So I have decided to resend my old proposition of probe_err helper which should simplify resource acquisition error handling, it also extend it with adding defer probe reason to devices_deferred debugfs property, which should improve debugging experience for developers/testers. I have also added two patches showing usage and benefits of the helper. My dirty/ad-hoc cocci scripts shows that this helper can be used in at least 2700 places saving about 3500 lines of code. Regards Andrzej Andrzej Hajda (4): driver core: add device probe log helper driver core: add deferring probe reason to devices_deferred property drm/bridge/sii8620: fix resource acquisition error handling drm/bridge: lvds-codec: simplify error handling drivers/base/base.h | 3 ++ drivers/base/core.c | 46 ++++++++++++++++++++++++++++ drivers/base/dd.c | 23 +++++++++++++- drivers/gpu/drm/bridge/lvds-codec.c | 10 ++---- drivers/gpu/drm/bridge/sil-sii8620.c | 21 ++++++------- include/linux/device.h | 3 ++ 6 files changed, 86 insertions(+), 20 deletions(-) -- 2.17.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <CGME20200713144331eucas1p24107a30783dacc92887b80cf082be9fb@eucas1p2.samsung.com>]
* [PATCH v9 1/4] driver core: add device probe log helper [not found] ` <CGME20200713144331eucas1p24107a30783dacc92887b80cf082be9fb@eucas1p2.samsung.com> @ 2020-07-13 14:43 ` Andrzej Hajda 2020-07-13 14:58 ` Andy Shevchenko 0 siblings, 1 reply; 14+ messages in thread From: Andrzej Hajda @ 2020-07-13 14:43 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Jernej Skrabec, Rafael J. Wysocki, Jonas Karlman, Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, andy.shevchenko, Mark Brown, Laurent Pinchart, linux-arm-kernel, Marek Szyprowski During probe every time driver gets resource it should usually check for error printk some message if it is not -EPROBE_DEFER and return the error. This pattern is simple but requires adding few lines after any resource acquisition code, as a result it is often omitted or implemented only partially. dev_err_probe helps to replace such code sequences with simple call, so code: if (err != -EPROBE_DEFER) dev_err(dev, ...); return err; becomes: return dev_err_probe(dev, err, ...); Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> Reviewed-by: Rafael J. Wysocki <rafael@kernel.org> Reviewed-by: Mark Brown <broonie@kernel.org> --- drivers/base/core.c | 42 ++++++++++++++++++++++++++++++++++++++++++ include/linux/device.h | 3 +++ 2 files changed, 45 insertions(+) diff --git a/drivers/base/core.c b/drivers/base/core.c index 67d39a90b45c..3a827c82933f 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -3953,6 +3953,48 @@ define_dev_printk_level(_dev_info, KERN_INFO); #endif +/** + * dev_err_probe - probe error check and log helper + * @dev: the pointer to the struct device + * @err: error value to test + * @fmt: printf-style format string + * @...: arguments as specified in the format string + * + * This helper implements common pattern present in probe functions for error + * checking: print debug or error message depending if the error value is + * -EPROBE_DEFER and propagate error upwards. + * It replaces code sequence: + * if (err != -EPROBE_DEFER) + * dev_err(dev, ...); + * else + * dev_dbg(dev, ...); + * return err; + * with + * return dev_err_probe(dev, err, ...); + * + * Returns @err. + * + */ +int dev_err_probe(const struct device *dev, int err, const char *fmt, ...) +{ + struct va_format vaf; + va_list args; + + va_start(args, fmt); + vaf.fmt = fmt; + vaf.va = &args; + + if (err != -EPROBE_DEFER) + dev_err(dev, "error %d: %pV", err, &vaf); + else + dev_dbg(dev, "error %d: %pV", err, &vaf); + + va_end(args); + + return err; +} +EXPORT_SYMBOL_GPL(dev_err_probe); + static inline bool fwnode_is_primary(struct fwnode_handle *fwnode) { return fwnode && !IS_ERR(fwnode->secondary); diff --git a/include/linux/device.h b/include/linux/device.h index 15460a5ac024..6b2272ae9af8 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -964,6 +964,9 @@ void device_link_remove(void *consumer, struct device *supplier); void device_links_supplier_sync_state_pause(void); void device_links_supplier_sync_state_resume(void); +extern __printf(3, 4) +int dev_err_probe(const struct device *dev, int err, const char *fmt, ...); + /* Create alias, so I can be autoloaded. */ #define MODULE_ALIAS_CHARDEV(major,minor) \ MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor)) -- 2.17.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v9 1/4] driver core: add device probe log helper 2020-07-13 14:43 ` [PATCH v9 1/4] driver core: add device probe log helper Andrzej Hajda @ 2020-07-13 14:58 ` Andy Shevchenko 0 siblings, 0 replies; 14+ messages in thread From: Andy Shevchenko @ 2020-07-13 14:58 UTC (permalink / raw) To: Andrzej Hajda Cc: Jernej Skrabec, Rafael J. Wysocki, Greg Kroah-Hartman, Bartlomiej Zolnierkiewicz, Linux Kernel Mailing List, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Jonas Karlman, Mark Brown, Laurent Pinchart, linux-arm Mailing List, Marek Szyprowski On Mon, Jul 13, 2020 at 5:43 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > > During probe every time driver gets resource it should usually check for > error printk some message if it is not -EPROBE_DEFER and return the error. > This pattern is simple but requires adding few lines after any resource > acquisition code, as a result it is often omitted or implemented only > partially. > dev_err_probe helps to replace such code sequences with simple call, > so code: > if (err != -EPROBE_DEFER) > dev_err(dev, ...); > return err; > becomes: > return dev_err_probe(dev, err, ...); > FWIW, Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > Reviewed-by: Rafael J. Wysocki <rafael@kernel.org> > Reviewed-by: Mark Brown <broonie@kernel.org> > --- > drivers/base/core.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/device.h | 3 +++ > 2 files changed, 45 insertions(+) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 67d39a90b45c..3a827c82933f 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -3953,6 +3953,48 @@ define_dev_printk_level(_dev_info, KERN_INFO); > > #endif > > +/** > + * dev_err_probe - probe error check and log helper > + * @dev: the pointer to the struct device > + * @err: error value to test > + * @fmt: printf-style format string > + * @...: arguments as specified in the format string > + * > + * This helper implements common pattern present in probe functions for error > + * checking: print debug or error message depending if the error value is > + * -EPROBE_DEFER and propagate error upwards. > + * It replaces code sequence: > + * if (err != -EPROBE_DEFER) > + * dev_err(dev, ...); > + * else > + * dev_dbg(dev, ...); > + * return err; > + * with > + * return dev_err_probe(dev, err, ...); > + * > + * Returns @err. > + * > + */ > +int dev_err_probe(const struct device *dev, int err, const char *fmt, ...) > +{ > + struct va_format vaf; > + va_list args; > + > + va_start(args, fmt); > + vaf.fmt = fmt; > + vaf.va = &args; > + if (err != -EPROBE_DEFER) I would rather see positive conditional (slightly better to parse when read), but here I think it's more or less equal. > + dev_err(dev, "error %d: %pV", err, &vaf); > + else > + dev_dbg(dev, "error %d: %pV", err, &vaf); > + va_end(args); > + > + return err; > +} > +EXPORT_SYMBOL_GPL(dev_err_probe); > + > static inline bool fwnode_is_primary(struct fwnode_handle *fwnode) > { > return fwnode && !IS_ERR(fwnode->secondary); > diff --git a/include/linux/device.h b/include/linux/device.h > index 15460a5ac024..6b2272ae9af8 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -964,6 +964,9 @@ void device_link_remove(void *consumer, struct device *supplier); > void device_links_supplier_sync_state_pause(void); > void device_links_supplier_sync_state_resume(void); > > +extern __printf(3, 4) > +int dev_err_probe(const struct device *dev, int err, const char *fmt, ...); > + > /* Create alias, so I can be autoloaded. */ > #define MODULE_ALIAS_CHARDEV(major,minor) \ > MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor)) > -- > 2.17.1 > -- With Best Regards, Andy Shevchenko _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <CGME20200713144332eucas1p2f46275f84ec0cca6e23dcf040234005a@eucas1p2.samsung.com>]
* [PATCH v9 2/4] driver core: add deferring probe reason to devices_deferred property [not found] ` <CGME20200713144332eucas1p2f46275f84ec0cca6e23dcf040234005a@eucas1p2.samsung.com> @ 2020-07-13 14:43 ` Andrzej Hajda 0 siblings, 0 replies; 14+ messages in thread From: Andrzej Hajda @ 2020-07-13 14:43 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Jernej Skrabec, Rafael J. Wysocki, Jonas Karlman, Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, andy.shevchenko, Mark Brown, Laurent Pinchart, linux-arm-kernel, Marek Szyprowski /sys/kernel/debug/devices_deferred property contains list of deferred devices. This list does not contain reason why the driver deferred probe, the patch improves it. The natural place to set the reason is dev_err_probe function introduced recently, ie. if dev_err_probe will be called with -EPROBE_DEFER instead of printk the message will be attached to a deferred device and printed when user reads devices_deferred property. Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> Reviewed-by: Mark Brown <broonie@kernel.org> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Reviewed-by: Rafael J. Wysocki <rafael@kernel.org> --- v9: - fixed typo in function name v8: - improved commit message --- drivers/base/base.h | 3 +++ drivers/base/core.c | 8 ++++++-- drivers/base/dd.c | 23 ++++++++++++++++++++++- 3 files changed, 31 insertions(+), 3 deletions(-) diff --git a/drivers/base/base.h b/drivers/base/base.h index 95c22c0f9036..c3562adf4789 100644 --- a/drivers/base/base.h +++ b/drivers/base/base.h @@ -93,6 +93,7 @@ struct device_private { struct klist_node knode_class; struct list_head deferred_probe; struct device_driver *async_driver; + char *deferred_probe_reason; struct device *device; u8 dead:1; }; @@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device *dev, extern void driver_detach(struct device_driver *drv); extern int driver_probe_device(struct device_driver *drv, struct device *dev); extern void driver_deferred_probe_del(struct device *dev); +extern void device_set_deferred_probe_reason(const struct device *dev, + struct va_format *vaf); static inline int driver_match_device(struct device_driver *drv, struct device *dev) { diff --git a/drivers/base/core.c b/drivers/base/core.c index 3a827c82933f..d04d19458795 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -3963,6 +3963,8 @@ define_dev_printk_level(_dev_info, KERN_INFO); * This helper implements common pattern present in probe functions for error * checking: print debug or error message depending if the error value is * -EPROBE_DEFER and propagate error upwards. + * In case of -EPROBE_DEFER it sets also defer probe reason, which can be + * checked later by reading devices_deferred debugfs attribute. * It replaces code sequence: * if (err != -EPROBE_DEFER) * dev_err(dev, ...); @@ -3984,10 +3986,12 @@ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...) vaf.fmt = fmt; vaf.va = &args; - if (err != -EPROBE_DEFER) + if (err != -EPROBE_DEFER) { dev_err(dev, "error %d: %pV", err, &vaf); - else + } else { + device_set_deferred_probe_reason(dev, &vaf); dev_dbg(dev, "error %d: %pV", err, &vaf); + } va_end(args); diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 9a1d940342ac..7555b31bdfdc 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -27,6 +27,7 @@ #include <linux/async.h> #include <linux/pm_runtime.h> #include <linux/pinctrl/devinfo.h> +#include <linux/slab.h> #include "base.h" #include "power/power.h" @@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev) if (!list_empty(&dev->p->deferred_probe)) { dev_dbg(dev, "Removed from deferred list\n"); list_del_init(&dev->p->deferred_probe); + kfree(dev->p->deferred_probe_reason); + dev->p->deferred_probe_reason = NULL; } mutex_unlock(&deferred_probe_mutex); } @@ -211,6 +214,23 @@ void device_unblock_probing(void) driver_deferred_probe_trigger(); } +/** + * device_set_deferred_probe_reason() - Set defer probe reason message for device + * @dev: the pointer to the struct device + * @vaf: the pointer to va_format structure with message + */ +void device_set_deferred_probe_reason(const struct device *dev, struct va_format *vaf) +{ + const char *drv = dev_driver_string(dev); + + mutex_lock(&deferred_probe_mutex); + + kfree(dev->p->deferred_probe_reason); + dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: %pV", drv, vaf); + + mutex_unlock(&deferred_probe_mutex); +} + /* * deferred_devs_show() - Show the devices in the deferred probe pending list. */ @@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s, void *data) mutex_lock(&deferred_probe_mutex); list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe) - seq_printf(s, "%s\n", dev_name(curr->device)); + seq_printf(s, "%s\t%s", dev_name(curr->device), + curr->device->p->deferred_probe_reason ?: "\n"); mutex_unlock(&deferred_probe_mutex); -- 2.17.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 14+ messages in thread
[parent not found: <CGME20200713144333eucas1p127c600faa8a7416d89f0d84681c73ab9@eucas1p1.samsung.com>]
* [PATCH v9 3/4] drm/bridge/sii8620: fix resource acquisition error handling [not found] ` <CGME20200713144333eucas1p127c600faa8a7416d89f0d84681c73ab9@eucas1p1.samsung.com> @ 2020-07-13 14:43 ` Andrzej Hajda 0 siblings, 0 replies; 14+ messages in thread From: Andrzej Hajda @ 2020-07-13 14:43 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Jernej Skrabec, Rafael J. Wysocki, Jonas Karlman, Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, andy.shevchenko, Mark Brown, Laurent Pinchart, linux-arm-kernel, Marek Szyprowski In case of error during resource acquisition driver should print error message only in case it is not deferred probe, using dev_err_probe helper solves the issue. Moreover it records defer probe reason for debugging. Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/gpu/drm/bridge/sil-sii8620.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c index 92acd336aa89..389c1f029774 100644 --- a/drivers/gpu/drm/bridge/sil-sii8620.c +++ b/drivers/gpu/drm/bridge/sil-sii8620.c @@ -2299,10 +2299,9 @@ static int sii8620_probe(struct i2c_client *client, INIT_LIST_HEAD(&ctx->mt_queue); ctx->clk_xtal = devm_clk_get(dev, "xtal"); - if (IS_ERR(ctx->clk_xtal)) { - dev_err(dev, "failed to get xtal clock from DT\n"); - return PTR_ERR(ctx->clk_xtal); - } + if (IS_ERR(ctx->clk_xtal)) + return dev_err_probe(dev, PTR_ERR(ctx->clk_xtal), + "failed to get xtal clock from DT\n"); if (!client->irq) { dev_err(dev, "no irq provided\n"); @@ -2313,16 +2312,14 @@ static int sii8620_probe(struct i2c_client *client, sii8620_irq_thread, IRQF_TRIGGER_HIGH | IRQF_ONESHOT, "sii8620", ctx); - if (ret < 0) { - dev_err(dev, "failed to install IRQ handler\n"); - return ret; - } + if (ret < 0) + return dev_err_probe(dev, ret, + "failed to install IRQ handler\n"); ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); - if (IS_ERR(ctx->gpio_reset)) { - dev_err(dev, "failed to get reset gpio from DT\n"); - return PTR_ERR(ctx->gpio_reset); - } + if (IS_ERR(ctx->gpio_reset)) + return dev_err_probe(dev, PTR_ERR(ctx->gpio_reset), + "failed to get reset gpio from DT\n"); ctx->supplies[0].supply = "cvcc10"; ctx->supplies[1].supply = "iovcc18"; -- 2.17.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 14+ messages in thread
[parent not found: <CGME20200713144333eucas1p1f393b280cda5eead4e7ca0a005c8a4db@eucas1p1.samsung.com>]
* [PATCH v9 4/4] drm/bridge: lvds-codec: simplify error handling [not found] ` <CGME20200713144333eucas1p1f393b280cda5eead4e7ca0a005c8a4db@eucas1p1.samsung.com> @ 2020-07-13 14:43 ` Andrzej Hajda 0 siblings, 0 replies; 14+ messages in thread From: Andrzej Hajda @ 2020-07-13 14:43 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Jernej Skrabec, Rafael J. Wysocki, Jonas Karlman, Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, andy.shevchenko, Mark Brown, Laurent Pinchart, linux-arm-kernel, Marek Szyprowski Using dev_err_probe code has following advantages: - shorter code, - recorded defer probe reason for debugging, - uniform error code logging. Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/gpu/drm/bridge/lvds-codec.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c index 24fb1befdfa2..f19d9f7a5db2 100644 --- a/drivers/gpu/drm/bridge/lvds-codec.c +++ b/drivers/gpu/drm/bridge/lvds-codec.c @@ -71,13 +71,9 @@ static int lvds_codec_probe(struct platform_device *pdev) lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev); lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", GPIOD_OUT_HIGH); - if (IS_ERR(lvds_codec->powerdown_gpio)) { - int err = PTR_ERR(lvds_codec->powerdown_gpio); - - if (err != -EPROBE_DEFER) - dev_err(dev, "powerdown GPIO failure: %d\n", err); - return err; - } + if (IS_ERR(lvds_codec->powerdown_gpio)) + return dev_err_probe(dev, PTR_ERR(lvds_codec->powerdown_gpio), + "powerdown GPIO failure\n"); /* Locate the panel DT node. */ panel_node = of_graph_get_remote_node(dev->of_node, 1, 0); -- 2.17.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v9 0/4] driver core: add probe error check helper 2020-07-13 14:43 ` [PATCH v9 0/4] driver core: add probe error check helper Andrzej Hajda ` (3 preceding siblings ...) [not found] ` <CGME20200713144333eucas1p1f393b280cda5eead4e7ca0a005c8a4db@eucas1p1.samsung.com> @ 2020-07-28 15:05 ` Andrzej Hajda 2020-07-30 7:08 ` Greg Kroah-Hartman 4 siblings, 1 reply; 14+ messages in thread From: Andrzej Hajda @ 2020-07-28 15:05 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Jernej Skrabec, Bartlomiej Zolnierkiewicz, Jonas Karlman, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, andy.shevchenko, Mark Brown, Laurent Pinchart, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski Hi Greg, Apparently the patchset has no more comments. Could you take the patches to your tree? At least 1st and 2nd. Regards Andrzej On 13.07.2020 16:43, Andrzej Hajda wrote: > Hi All, > > Thanks for comments. > > Changes since v8: > - fixed typo in function name, > - removed cocci script (added by mistake) > > Changes since v7: > - improved commit message > - added R-Bs > > Changes since v6: > - removed leftovers from old naming scheme in commit descritions, > - added R-Bs. > > Changes since v5: > - removed patch adding macro, dev_err_probe(dev, PTR_ERR(ptr), ...) should be used instead, > - added dev_dbg logging in case of -EPROBE_DEFER, > - renamed functions and vars according to comments, > - extended docs, > - cosmetics. > > Original message (with small adjustments): > > Recently I took some time to re-check error handling in drivers probe code, > and I have noticed that number of incorrect resource acquisition error handling > increased and there are no other propositions which can cure the situation. > > So I have decided to resend my old proposition of probe_err helper which should > simplify resource acquisition error handling, it also extend it with adding defer > probe reason to devices_deferred debugfs property, which should improve debugging > experience for developers/testers. > > I have also added two patches showing usage and benefits of the helper. > > My dirty/ad-hoc cocci scripts shows that this helper can be used in at least 2700 places > saving about 3500 lines of code. > > Regards > Andrzej > > > Andrzej Hajda (4): > driver core: add device probe log helper > driver core: add deferring probe reason to devices_deferred property > drm/bridge/sii8620: fix resource acquisition error handling > drm/bridge: lvds-codec: simplify error handling > > drivers/base/base.h | 3 ++ > drivers/base/core.c | 46 ++++++++++++++++++++++++++++ > drivers/base/dd.c | 23 +++++++++++++- > drivers/gpu/drm/bridge/lvds-codec.c | 10 ++---- > drivers/gpu/drm/bridge/sil-sii8620.c | 21 ++++++------- > include/linux/device.h | 3 ++ > 6 files changed, 86 insertions(+), 20 deletions(-) > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v9 0/4] driver core: add probe error check helper 2020-07-28 15:05 ` [PATCH v9 0/4] driver core: add probe error check helper Andrzej Hajda @ 2020-07-30 7:08 ` Greg Kroah-Hartman 2020-07-30 16:18 ` Dmitry Torokhov 0 siblings, 1 reply; 14+ messages in thread From: Greg Kroah-Hartman @ 2020-07-30 7:08 UTC (permalink / raw) To: Andrzej Hajda Cc: Jernej Skrabec, Bartlomiej Zolnierkiewicz, Jonas Karlman, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, andy.shevchenko, Mark Brown, Laurent Pinchart, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski On Tue, Jul 28, 2020 at 05:05:03PM +0200, Andrzej Hajda wrote: > Hi Greg, > > Apparently the patchset has no more comments. > > Could you take the patches to your tree? At least 1st and 2nd. All now queued up, thanks! greg k-h _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v9 0/4] driver core: add probe error check helper 2020-07-30 7:08 ` Greg Kroah-Hartman @ 2020-07-30 16:18 ` Dmitry Torokhov 2020-07-30 16:48 ` Mark Brown 0 siblings, 1 reply; 14+ messages in thread From: Dmitry Torokhov @ 2020-07-30 16:18 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Jernej Skrabec, Jonas Karlman, Bartlomiej Zolnierkiewicz, lkml, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, Andy Shevchenko, Mark Brown, Laurent Pinchart, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski On Thu, Jul 30, 2020 at 12:10 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Tue, Jul 28, 2020 at 05:05:03PM +0200, Andrzej Hajda wrote: > > Hi Greg, > > > > Apparently the patchset has no more comments. > > > > Could you take the patches to your tree? At least 1st and 2nd. > > All now queued up, thanks! I believe it still has not been answered why this can't be pushed into resource providers (clock, regulators, gpio, interrupts, etc), especially for devm APIs where we know exactly what device we are requesting a resource for, so that individual drivers do not need to change anything. We can mark the device as being probed so that probe deferral is only handled when we actually execute probe() (and for the bonus points scream loudly if someone tries to return -EPROBE_DEFER outside of probe path). And now with coccinelle script we can expect a deluge of patches reshuffling drivers... Thanks. -- Dmitry _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v9 0/4] driver core: add probe error check helper 2020-07-30 16:18 ` Dmitry Torokhov @ 2020-07-30 16:48 ` Mark Brown 2020-07-30 17:46 ` Dmitry Torokhov 0 siblings, 1 reply; 14+ messages in thread From: Mark Brown @ 2020-07-30 16:48 UTC (permalink / raw) To: Dmitry Torokhov Cc: Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Jonas Karlman, lkml, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, Andy Shevchenko, Laurent Pinchart, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski [-- Attachment #1.1: Type: text/plain, Size: 831 bytes --] On Thu, Jul 30, 2020 at 09:18:30AM -0700, Dmitry Torokhov wrote: > I believe it still has not been answered why this can't be pushed into > resource providers (clock, regulators, gpio, interrupts, etc), > especially for devm APIs where we know exactly what device we are > requesting a resource for, so that individual drivers do not need to > change anything. The error messages are frequently in the caller rather than the frameworks, it's often helpful for the comprehensibility of the error messages especially in cases where things may be legitimately absent. > We can mark the device as being probed so that probe > deferral is only handled when we actually execute probe() (and for the > bonus points scream loudly if someone tries to return -EPROBE_DEFER > outside of probe path). Is this a big issue? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v9 0/4] driver core: add probe error check helper 2020-07-30 16:48 ` Mark Brown @ 2020-07-30 17:46 ` Dmitry Torokhov 2020-07-30 18:16 ` Mark Brown 0 siblings, 1 reply; 14+ messages in thread From: Dmitry Torokhov @ 2020-07-30 17:46 UTC (permalink / raw) To: Mark Brown Cc: Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Jonas Karlman, lkml, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, Andy Shevchenko, Laurent Pinchart, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski On Thu, Jul 30, 2020 at 9:49 AM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Jul 30, 2020 at 09:18:30AM -0700, Dmitry Torokhov wrote: > > > I believe it still has not been answered why this can't be pushed into > > resource providers (clock, regulators, gpio, interrupts, etc), > > especially for devm APIs where we know exactly what device we are > > requesting a resource for, so that individual drivers do not need to > > change anything. > > The error messages are frequently in the caller rather than the > frameworks, it's often helpful for the comprehensibility of the error > messages especially in cases where things may be legitimately absent. Not for deferral. All you need to know in this case is: "device A is attempting to request resource B which is not ready yet" There is nothing to handle on the caller part except to float the error up. > > > We can mark the device as being probed so that probe > > deferral is only handled when we actually execute probe() (and for the > > bonus points scream loudly if someone tries to return -EPROBE_DEFER > > outside of probe path). > > Is this a big issue? We do not know ;) Probably not. It will just get reported as an ordinary failure and the driver will handle it somehow. Still it would be nice to know if we attempt to raise deferrals in code paths where they do not make sense. Thanks. -- Dmitry _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v9 0/4] driver core: add probe error check helper 2020-07-30 17:46 ` Dmitry Torokhov @ 2020-07-30 18:16 ` Mark Brown 2020-07-30 18:45 ` Dmitry Torokhov 0 siblings, 1 reply; 14+ messages in thread From: Mark Brown @ 2020-07-30 18:16 UTC (permalink / raw) To: Dmitry Torokhov Cc: Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Jonas Karlman, lkml, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, Andy Shevchenko, Laurent Pinchart, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski [-- Attachment #1.1: Type: text/plain, Size: 881 bytes --] On Thu, Jul 30, 2020 at 10:46:31AM -0700, Dmitry Torokhov wrote: > On Thu, Jul 30, 2020 at 9:49 AM Mark Brown <broonie@kernel.org> wrote: > > The error messages are frequently in the caller rather than the > > frameworks, it's often helpful for the comprehensibility of the error > > messages especially in cases where things may be legitimately absent. > Not for deferral. All you need to know in this case is: > "device A is attempting to request resource B which is not ready yet" > There is nothing to handle on the caller part except to float the error up. You can sometimes do a better job of explaining what the resource you were looking for was, and of course you still need diagnostics in the non-deferral case. Whatever happens we'll need a lot of per-driver churn, either removing existing diagnostics that get factored into cores or updating to use this new API. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v9 0/4] driver core: add probe error check helper 2020-07-30 18:16 ` Mark Brown @ 2020-07-30 18:45 ` Dmitry Torokhov 2020-07-30 19:06 ` Mark Brown 0 siblings, 1 reply; 14+ messages in thread From: Dmitry Torokhov @ 2020-07-30 18:45 UTC (permalink / raw) To: Mark Brown Cc: Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Jonas Karlman, lkml, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, Andy Shevchenko, Laurent Pinchart, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski On Thu, Jul 30, 2020 at 11:16 AM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Jul 30, 2020 at 10:46:31AM -0700, Dmitry Torokhov wrote: > > On Thu, Jul 30, 2020 at 9:49 AM Mark Brown <broonie@kernel.org> wrote: > > > > The error messages are frequently in the caller rather than the > > > frameworks, it's often helpful for the comprehensibility of the error > > > messages especially in cases where things may be legitimately absent. > > > Not for deferral. All you need to know in this case is: > > > "device A is attempting to request resource B which is not ready yet" > > > There is nothing to handle on the caller part except to float the error up. > > You can sometimes do a better job of explaining what the resource you > were looking for was, I think it is true for very esoteric cases. I.e. your driver uses 2 interrupt lines, or something like that. For GPIO, regulators, and clocks we normally have a name/connection ID that provides enough of context. We need to remember, the error messages really only make total sense to a person familiar with the driver to begin with, not for a random person looking at the log. > and of course you still need diagnostics in the > non-deferral case. Whatever happens we'll need a lot of per-driver > churn, either removing existing diagnostics that get factored into cores > or updating to use this new API. The point is if you push it into core you'll get the benefit of notifying about the deferral (and can "attach" deferral reason to a device) without changing drivers at all. You can clean them up later if you want, or decide that additional logging in error paths does not hurt. This new API does not do you any good unless you convert drivers, and you need to convert the majority of them to be able to rely on the deferral diagnostic that is being added. Thanks. -- Dmitry _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v9 0/4] driver core: add probe error check helper 2020-07-30 18:45 ` Dmitry Torokhov @ 2020-07-30 19:06 ` Mark Brown 0 siblings, 0 replies; 14+ messages in thread From: Mark Brown @ 2020-07-30 19:06 UTC (permalink / raw) To: Dmitry Torokhov Cc: Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Jonas Karlman, lkml, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, Andy Shevchenko, Laurent Pinchart, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski [-- Attachment #1.1: Type: text/plain, Size: 2136 bytes --] On Thu, Jul 30, 2020 at 11:45:25AM -0700, Dmitry Torokhov wrote: > On Thu, Jul 30, 2020 at 11:16 AM Mark Brown <broonie@kernel.org> wrote: > > You can sometimes do a better job of explaining what the resource you > > were looking for was, > I think it is true for very esoteric cases. I.e. your driver uses 2 > interrupt lines, or something like that. For GPIO, regulators, and > clocks we normally have a name/connection ID that provides enough of *Normally* but not always - some of the older bindings do love their arrays of phandles (or mixes of numbers and phandles!) unfortunately. > context. We need to remember, the error messages really only make > total sense to a person familiar with the driver to begin with, not > for a random person looking at the log. Not really, one of the big targets is people doing system integration who are writing a DT or possibly producing a highly tuned kernel config. They needn't have a strong familiarity with the driver, they're often just picking it up off the shelf. > > and of course you still need diagnostics in the > > non-deferral case. Whatever happens we'll need a lot of per-driver > > churn, either removing existing diagnostics that get factored into cores > > or updating to use this new API. > The point is if you push it into core you'll get the benefit of > notifying about the deferral (and can "attach" deferral reason to a > device) without changing drivers at all. You can clean them up later > if you want, or decide that additional logging in error paths does not > hurt. This new API does not do you any good unless you convert > drivers, and you need to convert the majority of them to be able to > rely on the deferral diagnostic that is being added. The push for this is that there's already people going around modifying drivers whatever happens but at present they're mainly trying to delete diagnostics which isn't wonderful. Besides, even if we push things into the subsystems they'd want to use this interface or something quite like it anyway - it's more a question of if we go quickly add some users to subsystems isn't it? I'm not against that. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-07-31 7:37 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20200713144331eucas1p25911c4ffa9315f632d8f6dd833588981@eucas1p2.samsung.com> 2020-07-13 14:43 ` [PATCH v9 0/4] driver core: add probe error check helper Andrzej Hajda [not found] ` <CGME20200713144331eucas1p24107a30783dacc92887b80cf082be9fb@eucas1p2.samsung.com> 2020-07-13 14:43 ` [PATCH v9 1/4] driver core: add device probe log helper Andrzej Hajda 2020-07-13 14:58 ` Andy Shevchenko [not found] ` <CGME20200713144332eucas1p2f46275f84ec0cca6e23dcf040234005a@eucas1p2.samsung.com> 2020-07-13 14:43 ` [PATCH v9 2/4] driver core: add deferring probe reason to devices_deferred property Andrzej Hajda [not found] ` <CGME20200713144333eucas1p127c600faa8a7416d89f0d84681c73ab9@eucas1p1.samsung.com> 2020-07-13 14:43 ` [PATCH v9 3/4] drm/bridge/sii8620: fix resource acquisition error handling Andrzej Hajda [not found] ` <CGME20200713144333eucas1p1f393b280cda5eead4e7ca0a005c8a4db@eucas1p1.samsung.com> 2020-07-13 14:43 ` [PATCH v9 4/4] drm/bridge: lvds-codec: simplify " Andrzej Hajda 2020-07-28 15:05 ` [PATCH v9 0/4] driver core: add probe error check helper Andrzej Hajda 2020-07-30 7:08 ` Greg Kroah-Hartman 2020-07-30 16:18 ` Dmitry Torokhov 2020-07-30 16:48 ` Mark Brown 2020-07-30 17:46 ` Dmitry Torokhov 2020-07-30 18:16 ` Mark Brown 2020-07-30 18:45 ` Dmitry Torokhov 2020-07-30 19:06 ` Mark Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).