* [PATCH v6 0/4] driver core: add probe error check helper [not found] <CGME20200626100108eucas1p2c6d68625f3755a467d7316dd27704f7c@eucas1p2.samsung.com> @ 2020-06-26 10:00 ` Andrzej Hajda [not found] ` <CGME20200626100109eucas1p25331652d017cd17d21c0ae60541d1f73@eucas1p2.samsung.com> ` (3 more replies) 0 siblings, 4 replies; 20+ messages in thread From: Andrzej Hajda @ 2020-06-26 10:00 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Jernej Skrabec, Rafael J. Wysocki, Jonas Karlman, Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, andy.shevchenko, Mark Brown, Laurent Pinchart, Daniel Vetter, linux-arm-kernel, Marek Szyprowski Hi All, Thanks for multiple comments. Changes since v5: - removed patch adding macro, dev_err_probe(dev, PTR_ERR(ptr), ...) should be used instead, - added dev_dbg logging in case of -EPROBE_DEFER, - renamed functions and vars according to comments, - extended docs, - cosmetics. Original message (with small adjustments): Recently I took some time to re-check error handling in drivers probe code, and I have noticed that number of incorrect resource acquisition error handling increased and there are no other propositions which can cure the situation. So I have decided to resend my old proposition of probe_err helper which should simplify resource acquisition error handling, it also extend it with adding defer probe reason to devices_deferred debugfs property, which should improve debugging experience for developers/testers. I have also added two patches showing usage and benefits of the helper. My dirty/ad-hoc cocci scripts shows that this helper can be used in at least 2700 places saving about 3500 lines of code. Regards Andrzej Andrzej Hajda (4): driver core: add device probe log helper driver core: add deferring probe reason to devices_deferred property drm/bridge/sii8620: fix resource acquisition error handling drm/bridge: lvds-codec: simplify error handling drivers/base/base.h | 3 ++ drivers/base/core.c | 46 ++++++++++++++++++++++++++++ drivers/base/dd.c | 23 +++++++++++++- drivers/gpu/drm/bridge/lvds-codec.c | 10 ++---- drivers/gpu/drm/bridge/sil-sii8620.c | 21 ++++++------- include/linux/device.h | 3 ++ 6 files changed, 86 insertions(+), 20 deletions(-) -- 2.17.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 20+ messages in thread
[parent not found: <CGME20200626100109eucas1p25331652d017cd17d21c0ae60541d1f73@eucas1p2.samsung.com>]
* [PATCH v6 1/4] driver core: add device probe log helper [not found] ` <CGME20200626100109eucas1p25331652d017cd17d21c0ae60541d1f73@eucas1p2.samsung.com> @ 2020-06-26 10:01 ` Andrzej Hajda 2020-06-26 15:41 ` Rafael J. Wysocki ` (2 more replies) 0 siblings, 3 replies; 20+ messages in thread From: Andrzej Hajda @ 2020-06-26 10:01 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Jernej Skrabec, Rafael J. Wysocki, Jonas Karlman, Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, andy.shevchenko, Mark Brown, Laurent Pinchart, Daniel Vetter, linux-arm-kernel, Marek Szyprowski During probe every time driver gets resource it should usually check for error printk some message if it is not -EPROBE_DEFER and return the error. This pattern is simple but requires adding few lines after any resource acquisition code, as a result it is often omitted or implemented only partially. dev_err_probe helps to replace such code sequences with simple call, so code: if (err != -EPROBE_DEFER) dev_err(dev, ...); return err; becomes: return probe_err(dev, err, ...); Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> --- drivers/base/core.c | 42 ++++++++++++++++++++++++++++++++++++++++++ include/linux/device.h | 3 +++ 2 files changed, 45 insertions(+) diff --git a/drivers/base/core.c b/drivers/base/core.c index 67d39a90b45c..3a827c82933f 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -3953,6 +3953,48 @@ define_dev_printk_level(_dev_info, KERN_INFO); #endif +/** + * dev_err_probe - probe error check and log helper + * @dev: the pointer to the struct device + * @err: error value to test + * @fmt: printf-style format string + * @...: arguments as specified in the format string + * + * This helper implements common pattern present in probe functions for error + * checking: print debug or error message depending if the error value is + * -EPROBE_DEFER and propagate error upwards. + * It replaces code sequence: + * if (err != -EPROBE_DEFER) + * dev_err(dev, ...); + * else + * dev_dbg(dev, ...); + * return err; + * with + * return dev_err_probe(dev, err, ...); + * + * Returns @err. + * + */ +int dev_err_probe(const struct device *dev, int err, const char *fmt, ...) +{ + struct va_format vaf; + va_list args; + + va_start(args, fmt); + vaf.fmt = fmt; + vaf.va = &args; + + if (err != -EPROBE_DEFER) + dev_err(dev, "error %d: %pV", err, &vaf); + else + dev_dbg(dev, "error %d: %pV", err, &vaf); + + va_end(args); + + return err; +} +EXPORT_SYMBOL_GPL(dev_err_probe); + static inline bool fwnode_is_primary(struct fwnode_handle *fwnode) { return fwnode && !IS_ERR(fwnode->secondary); diff --git a/include/linux/device.h b/include/linux/device.h index 15460a5ac024..6b2272ae9af8 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -964,6 +964,9 @@ void device_link_remove(void *consumer, struct device *supplier); void device_links_supplier_sync_state_pause(void); void device_links_supplier_sync_state_resume(void); +extern __printf(3, 4) +int dev_err_probe(const struct device *dev, int err, const char *fmt, ...); + /* Create alias, so I can be autoloaded. */ #define MODULE_ALIAS_CHARDEV(major,minor) \ MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor)) -- 2.17.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v6 1/4] driver core: add device probe log helper 2020-06-26 10:01 ` [PATCH v6 1/4] driver core: add device probe log helper Andrzej Hajda @ 2020-06-26 15:41 ` Rafael J. Wysocki 2020-06-26 16:22 ` Sam Ravnborg 2020-07-01 9:29 ` Grygorii Strashko 2 siblings, 0 replies; 20+ messages in thread From: Rafael J. Wysocki @ 2020-06-26 15:41 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, Andy Shevchenko, Mark Brown, Laurent Pinchart, Daniel Vetter, Linux ARM, Marek Szyprowski On Fri, Jun 26, 2020 at 12:01 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 probe_err(dev, err, ...); > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> Reviewed-by: Rafael J. Wysocki <rafael@kernel.org> > --- > drivers/base/core.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/device.h | 3 +++ > 2 files changed, 45 insertions(+) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 67d39a90b45c..3a827c82933f 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -3953,6 +3953,48 @@ define_dev_printk_level(_dev_info, KERN_INFO); > > #endif > > +/** > + * dev_err_probe - probe error check and log helper > + * @dev: the pointer to the struct device > + * @err: error value to test > + * @fmt: printf-style format string > + * @...: arguments as specified in the format string > + * > + * This helper implements common pattern present in probe functions for error > + * checking: print debug or error message depending if the error value is > + * -EPROBE_DEFER and propagate error upwards. > + * It replaces code sequence: > + * if (err != -EPROBE_DEFER) > + * dev_err(dev, ...); > + * else > + * dev_dbg(dev, ...); > + * return err; > + * with > + * return dev_err_probe(dev, err, ...); > + * > + * Returns @err. > + * > + */ > +int dev_err_probe(const struct device *dev, int err, const char *fmt, ...) > +{ > + struct va_format vaf; > + va_list args; > + > + va_start(args, fmt); > + vaf.fmt = fmt; > + vaf.va = &args; > + > + if (err != -EPROBE_DEFER) > + dev_err(dev, "error %d: %pV", err, &vaf); > + else > + dev_dbg(dev, "error %d: %pV", err, &vaf); > + > + va_end(args); > + > + return err; > +} > +EXPORT_SYMBOL_GPL(dev_err_probe); > + > static inline bool fwnode_is_primary(struct fwnode_handle *fwnode) > { > return fwnode && !IS_ERR(fwnode->secondary); > diff --git a/include/linux/device.h b/include/linux/device.h > index 15460a5ac024..6b2272ae9af8 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -964,6 +964,9 @@ void device_link_remove(void *consumer, struct device *supplier); > void device_links_supplier_sync_state_pause(void); > void device_links_supplier_sync_state_resume(void); > > +extern __printf(3, 4) > +int dev_err_probe(const struct device *dev, int err, const char *fmt, ...); > + > /* Create alias, so I can be autoloaded. */ > #define MODULE_ALIAS_CHARDEV(major,minor) \ > MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor)) > -- > 2.17.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 1/4] driver core: add device probe log helper 2020-06-26 10:01 ` [PATCH v6 1/4] driver core: add device probe log helper Andrzej Hajda 2020-06-26 15:41 ` Rafael J. Wysocki @ 2020-06-26 16:22 ` Sam Ravnborg 2020-07-01 9:29 ` Grygorii Strashko 2 siblings, 0 replies; 20+ messages in thread From: Sam Ravnborg @ 2020-06-26 16:22 UTC (permalink / raw) To: Andrzej Hajda Cc: Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, 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 Andrzej On Fri, Jun 26, 2020 at 12:01:00PM +0200, Andrzej Hajda wrote: > During probe every time driver gets resource it should usually check for > error printk some message if it is not -EPROBE_DEFER and return the error. > This pattern is simple but requires adding few lines after any resource > acquisition code, as a result it is often omitted or implemented only > partially. > 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 probe_err(dev, err, ...); s/probe_err/dev_err_probe/ Sam > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > --- > 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 1/4] driver core: add device probe log helper 2020-06-26 10:01 ` [PATCH v6 1/4] driver core: add device probe log helper Andrzej Hajda 2020-06-26 15:41 ` Rafael J. Wysocki 2020-06-26 16:22 ` Sam Ravnborg @ 2020-07-01 9:29 ` Grygorii Strashko 2 siblings, 0 replies; 20+ messages in thread From: Grygorii Strashko @ 2020-07-01 9:29 UTC (permalink / raw) To: Andrzej Hajda, 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, Daniel Vetter, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski On 26/06/2020 13:01, Andrzej Hajda wrote: > During probe every time driver gets resource it should usually check for > error printk some message if it is not -EPROBE_DEFER and return the error. > This pattern is simple but requires adding few lines after any resource > acquisition code, as a result it is often omitted or implemented only > partially. > 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 probe_err(dev, err, ...); > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > --- > drivers/base/core.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/device.h | 3 +++ > 2 files changed, 45 insertions(+) > Basic version of the helper looks very good to me, thank you. Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com> > 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)) > -- Best regards, grygorii _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 20+ messages in thread
[parent not found: <CGME20200626100110eucas1p2c5b91f2c98a5c6e5739f5af3207d192e@eucas1p2.samsung.com>]
* [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property [not found] ` <CGME20200626100110eucas1p2c5b91f2c98a5c6e5739f5af3207d192e@eucas1p2.samsung.com> @ 2020-06-26 10:01 ` Andrzej Hajda 2020-06-26 15:43 ` Rafael J. Wysocki 2020-06-26 17:11 ` Grygorii Strashko 0 siblings, 2 replies; 20+ messages in thread From: Andrzej Hajda @ 2020-06-26 10:01 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Jernej Skrabec, Rafael J. Wysocki, Jonas Karlman, Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, andy.shevchenko, Mark Brown, Laurent Pinchart, Daniel Vetter, linux-arm-kernel, Marek Szyprowski /sys/kernel/debug/devices_deferred property contains list of deferred devices. This list does not contain reason why the driver deferred probe, the patch improves it. The natural place to set the reason is probe_err function introduced recently, ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message will be attached to deferred device and printed when user read devices_deferred property. Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> Reviewed-by: Mark Brown <broonie@kernel.org> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> --- drivers/base/base.h | 3 +++ drivers/base/core.c | 8 ++++++-- drivers/base/dd.c | 23 ++++++++++++++++++++++- 3 files changed, 31 insertions(+), 3 deletions(-) diff --git a/drivers/base/base.h b/drivers/base/base.h index 95c22c0f9036..6954fccab3d7 100644 --- a/drivers/base/base.h +++ b/drivers/base/base.h @@ -93,6 +93,7 @@ struct device_private { struct klist_node knode_class; struct list_head deferred_probe; struct device_driver *async_driver; + char *deferred_probe_reason; struct device *device; u8 dead:1; }; @@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device *dev, extern void driver_detach(struct device_driver *drv); extern int driver_probe_device(struct device_driver *drv, struct device *dev); extern void driver_deferred_probe_del(struct device *dev); +extern void device_set_deferred_probe_reson(const struct device *dev, + struct va_format *vaf); static inline int driver_match_device(struct device_driver *drv, struct device *dev) { diff --git a/drivers/base/core.c b/drivers/base/core.c index 3a827c82933f..fee047f03681 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -3963,6 +3963,8 @@ define_dev_printk_level(_dev_info, KERN_INFO); * This helper implements common pattern present in probe functions for error * checking: print debug or error message depending if the error value is * -EPROBE_DEFER and propagate error upwards. + * In case of -EPROBE_DEFER it sets also defer probe reason, which can be + * checked later by reading devices_deferred debugfs attribute. * It replaces code sequence: * if (err != -EPROBE_DEFER) * dev_err(dev, ...); @@ -3984,10 +3986,12 @@ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...) vaf.fmt = fmt; vaf.va = &args; - if (err != -EPROBE_DEFER) + if (err != -EPROBE_DEFER) { dev_err(dev, "error %d: %pV", err, &vaf); - else + } else { + device_set_deferred_probe_reson(dev, &vaf); dev_dbg(dev, "error %d: %pV", err, &vaf); + } va_end(args); diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 9a1d940342ac..dd5683b61f74 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -27,6 +27,7 @@ #include <linux/async.h> #include <linux/pm_runtime.h> #include <linux/pinctrl/devinfo.h> +#include <linux/slab.h> #include "base.h" #include "power/power.h" @@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev) if (!list_empty(&dev->p->deferred_probe)) { dev_dbg(dev, "Removed from deferred list\n"); list_del_init(&dev->p->deferred_probe); + kfree(dev->p->deferred_probe_reason); + dev->p->deferred_probe_reason = NULL; } mutex_unlock(&deferred_probe_mutex); } @@ -211,6 +214,23 @@ void device_unblock_probing(void) driver_deferred_probe_trigger(); } +/** + * device_set_deferred_probe_reson() - Set defer probe reason message for device + * @dev: the pointer to the struct device + * @vaf: the pointer to va_format structure with message + */ +void device_set_deferred_probe_reson(const struct device *dev, struct va_format *vaf) +{ + const char *drv = dev_driver_string(dev); + + mutex_lock(&deferred_probe_mutex); + + kfree(dev->p->deferred_probe_reason); + dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: %pV", drv, vaf); + + mutex_unlock(&deferred_probe_mutex); +} + /* * deferred_devs_show() - Show the devices in the deferred probe pending list. */ @@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s, void *data) mutex_lock(&deferred_probe_mutex); list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe) - seq_printf(s, "%s\n", dev_name(curr->device)); + seq_printf(s, "%s\t%s", dev_name(curr->device), + curr->device->p->deferred_probe_reason ?: "\n"); mutex_unlock(&deferred_probe_mutex); -- 2.17.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property 2020-06-26 10:01 ` [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property Andrzej Hajda @ 2020-06-26 15:43 ` Rafael J. Wysocki 2020-06-26 17:11 ` Grygorii Strashko 1 sibling, 0 replies; 20+ messages in thread From: Rafael J. Wysocki @ 2020-06-26 15:43 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, Andy Shevchenko, Mark Brown, Laurent Pinchart, Daniel Vetter, Linux ARM, Marek Szyprowski On Fri, Jun 26, 2020 at 12:01 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > > /sys/kernel/debug/devices_deferred property contains list of deferred devices. > This list does not contain reason why the driver deferred probe, the patch > improves it. > The natural place to set the reason is probe_err function introduced recently, > ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message > will be attached to deferred device and printed when user read devices_deferred > property. > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > Reviewed-by: Mark Brown <broonie@kernel.org> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Reviewed-by: Rafael J. Wysocki <rafael@kernel.org> > --- > drivers/base/base.h | 3 +++ > drivers/base/core.c | 8 ++++++-- > drivers/base/dd.c | 23 ++++++++++++++++++++++- > 3 files changed, 31 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/base.h b/drivers/base/base.h > index 95c22c0f9036..6954fccab3d7 100644 > --- a/drivers/base/base.h > +++ b/drivers/base/base.h > @@ -93,6 +93,7 @@ struct device_private { > struct klist_node knode_class; > struct list_head deferred_probe; > struct device_driver *async_driver; > + char *deferred_probe_reason; > struct device *device; > u8 dead:1; > }; > @@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device *dev, > extern void driver_detach(struct device_driver *drv); > extern int driver_probe_device(struct device_driver *drv, struct device *dev); > extern void driver_deferred_probe_del(struct device *dev); > +extern void device_set_deferred_probe_reson(const struct device *dev, > + struct va_format *vaf); > static inline int driver_match_device(struct device_driver *drv, > struct device *dev) > { > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 3a827c82933f..fee047f03681 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -3963,6 +3963,8 @@ define_dev_printk_level(_dev_info, KERN_INFO); > * This helper implements common pattern present in probe functions for error > * checking: print debug or error message depending if the error value is > * -EPROBE_DEFER and propagate error upwards. > + * In case of -EPROBE_DEFER it sets also defer probe reason, which can be > + * checked later by reading devices_deferred debugfs attribute. > * It replaces code sequence: > * if (err != -EPROBE_DEFER) > * dev_err(dev, ...); > @@ -3984,10 +3986,12 @@ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...) > vaf.fmt = fmt; > vaf.va = &args; > > - if (err != -EPROBE_DEFER) > + if (err != -EPROBE_DEFER) { > dev_err(dev, "error %d: %pV", err, &vaf); > - else > + } else { > + device_set_deferred_probe_reson(dev, &vaf); > dev_dbg(dev, "error %d: %pV", err, &vaf); > + } > > va_end(args); > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index 9a1d940342ac..dd5683b61f74 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -27,6 +27,7 @@ > #include <linux/async.h> > #include <linux/pm_runtime.h> > #include <linux/pinctrl/devinfo.h> > +#include <linux/slab.h> > > #include "base.h" > #include "power/power.h" > @@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev) > if (!list_empty(&dev->p->deferred_probe)) { > dev_dbg(dev, "Removed from deferred list\n"); > list_del_init(&dev->p->deferred_probe); > + kfree(dev->p->deferred_probe_reason); > + dev->p->deferred_probe_reason = NULL; > } > mutex_unlock(&deferred_probe_mutex); > } > @@ -211,6 +214,23 @@ void device_unblock_probing(void) > driver_deferred_probe_trigger(); > } > > +/** > + * device_set_deferred_probe_reson() - Set defer probe reason message for device > + * @dev: the pointer to the struct device > + * @vaf: the pointer to va_format structure with message > + */ > +void device_set_deferred_probe_reson(const struct device *dev, struct va_format *vaf) > +{ > + const char *drv = dev_driver_string(dev); > + > + mutex_lock(&deferred_probe_mutex); > + > + kfree(dev->p->deferred_probe_reason); > + dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: %pV", drv, vaf); > + > + mutex_unlock(&deferred_probe_mutex); > +} > + > /* > * deferred_devs_show() - Show the devices in the deferred probe pending list. > */ > @@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s, void *data) > mutex_lock(&deferred_probe_mutex); > > list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe) > - seq_printf(s, "%s\n", dev_name(curr->device)); > + seq_printf(s, "%s\t%s", dev_name(curr->device), > + curr->device->p->deferred_probe_reason ?: "\n"); > > mutex_unlock(&deferred_probe_mutex); > > -- > 2.17.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property 2020-06-26 10:01 ` [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property Andrzej Hajda 2020-06-26 15:43 ` Rafael J. Wysocki @ 2020-06-26 17:11 ` Grygorii Strashko 2020-06-29 11:28 ` Andrzej Hajda 1 sibling, 1 reply; 20+ messages in thread From: Grygorii Strashko @ 2020-06-26 17:11 UTC (permalink / raw) To: Andrzej Hajda, 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, Daniel Vetter, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski On 26/06/2020 13:01, Andrzej Hajda wrote: > /sys/kernel/debug/devices_deferred property contains list of deferred devices. > This list does not contain reason why the driver deferred probe, the patch > improves it. > The natural place to set the reason is probe_err function introduced recently, > ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message > will be attached to deferred device and printed when user read devices_deferred > property. > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > Reviewed-by: Mark Brown <broonie@kernel.org> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- > drivers/base/base.h | 3 +++ > drivers/base/core.c | 8 ++++++-- > drivers/base/dd.c | 23 ++++++++++++++++++++++- > 3 files changed, 31 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/base.h b/drivers/base/base.h > index 95c22c0f9036..6954fccab3d7 100644 > --- a/drivers/base/base.h > +++ b/drivers/base/base.h > @@ -93,6 +93,7 @@ struct device_private { > struct klist_node knode_class; > struct list_head deferred_probe; > struct device_driver *async_driver; > + char *deferred_probe_reason; > struct device *device; > u8 dead:1; > }; > @@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device *dev, > extern void driver_detach(struct device_driver *drv); > extern int driver_probe_device(struct device_driver *drv, struct device *dev); > extern void driver_deferred_probe_del(struct device *dev); > +extern void device_set_deferred_probe_reson(const struct device *dev, > + struct va_format *vaf); > static inline int driver_match_device(struct device_driver *drv, > struct device *dev) > { > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 3a827c82933f..fee047f03681 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -3963,6 +3963,8 @@ define_dev_printk_level(_dev_info, KERN_INFO); > * This helper implements common pattern present in probe functions for error > * checking: print debug or error message depending if the error value is > * -EPROBE_DEFER and propagate error upwards. > + * In case of -EPROBE_DEFER it sets also defer probe reason, which can be > + * checked later by reading devices_deferred debugfs attribute. > * It replaces code sequence: > * if (err != -EPROBE_DEFER) > * dev_err(dev, ...); > @@ -3984,10 +3986,12 @@ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...) > vaf.fmt = fmt; > vaf.va = &args; > > - if (err != -EPROBE_DEFER) > + if (err != -EPROBE_DEFER) { > dev_err(dev, "error %d: %pV", err, &vaf); > - else > + } else { > + device_set_deferred_probe_reson(dev, &vaf); > dev_dbg(dev, "error %d: %pV", err, &vaf); > + } > > va_end(args); > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index 9a1d940342ac..dd5683b61f74 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -27,6 +27,7 @@ > #include <linux/async.h> > #include <linux/pm_runtime.h> > #include <linux/pinctrl/devinfo.h> > +#include <linux/slab.h> > > #include "base.h" > #include "power/power.h" > @@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev) > if (!list_empty(&dev->p->deferred_probe)) { > dev_dbg(dev, "Removed from deferred list\n"); > list_del_init(&dev->p->deferred_probe); > + kfree(dev->p->deferred_probe_reason); > + dev->p->deferred_probe_reason = NULL; > } > mutex_unlock(&deferred_probe_mutex); > } > @@ -211,6 +214,23 @@ void device_unblock_probing(void) > driver_deferred_probe_trigger(); > } > > +/** > + * device_set_deferred_probe_reson() - Set defer probe reason message for device > + * @dev: the pointer to the struct device > + * @vaf: the pointer to va_format structure with message > + */ > +void device_set_deferred_probe_reson(const struct device *dev, struct va_format *vaf) > +{ > + const char *drv = dev_driver_string(dev); > + > + mutex_lock(&deferred_probe_mutex); > + > + kfree(dev->p->deferred_probe_reason); > + dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: %pV", drv, vaf); > + > + mutex_unlock(&deferred_probe_mutex); > +} > + > /* > * deferred_devs_show() - Show the devices in the deferred probe pending list. > */ > @@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s, void *data) > mutex_lock(&deferred_probe_mutex); > > list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe) > - seq_printf(s, "%s\n", dev_name(curr->device)); > + seq_printf(s, "%s\t%s", dev_name(curr->device), > + curr->device->p->deferred_probe_reason ?: "\n"); > > mutex_unlock(&deferred_probe_mutex); > > Sry, may be i missing smth, but shouldn't it be optional (CONFIG_DEBUG_FS is probably too generic). -- Best regards, grygorii _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property 2020-06-26 17:11 ` Grygorii Strashko @ 2020-06-29 11:28 ` Andrzej Hajda 2020-06-30 8:59 ` Grygorii Strashko 0 siblings, 1 reply; 20+ messages in thread From: Andrzej Hajda @ 2020-06-29 11:28 UTC (permalink / raw) To: Grygorii Strashko, Greg Kroah-Hartman Cc: Jernej Skrabec, Jonas Karlman, Bartlomiej Zolnierkiewicz, Russell King - ARM Linux, open list:DRM DRIVERS, linux-kernel, Neil Armstrong, andy.shevchenko, Mark Brown, Laurent Pinchart, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski Hi Grygorii, (...) >> /* >> * 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); >> > > Sry, may be i missing smth, but shouldn't it be optional > (CONFIG_DEBUG_FS is probably too generic). > I am not sure what exactly are you referring to, but this patch does not add new property, it just extends functionality of existing one. Regards Andrzej _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property 2020-06-29 11:28 ` Andrzej Hajda @ 2020-06-30 8:59 ` Grygorii Strashko 2020-06-30 15:41 ` Andrzej Hajda 0 siblings, 1 reply; 20+ messages in thread From: Grygorii Strashko @ 2020-06-30 8:59 UTC (permalink / raw) To: Andrzej Hajda, Greg Kroah-Hartman Cc: Jernej Skrabec, Jonas Karlman, Bartlomiej Zolnierkiewicz, Russell King - ARM Linux, open list:DRM DRIVERS, linux-kernel, Neil Armstrong, andy.shevchenko, Mark Brown, Laurent Pinchart, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski Hi On 29/06/2020 14:28, Andrzej Hajda wrote: > Hi Grygorii, > > (...) > >>> /* >>> * 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); >>> >> >> Sry, may be i missing smth, but shouldn't it be optional >> (CONFIG_DEBUG_FS is probably too generic). >> > > I am not sure what exactly are you referring to, but this patch does not > add new property, it just extends functionality of existing one. Sry, needed to be more specific. You've added device_set_deferred_probe_reson(dev, &vaf); which expected to be used on every EPROBE_DEFER in dev_err_probe() in combination with + } else { + device_set_deferred_probe_reson(dev, &vaf); dev_dbg(dev, "error %d: %pV", err, &vaf); ^^ dev_dbg() does not add any runtime overhead during boot unless enabled + } But: +void device_set_deferred_probe_reson(const struct device *dev, struct va_format *vaf) +{ + const char *drv = dev_driver_string(dev); + + mutex_lock(&deferred_probe_mutex); + + kfree(dev->p->deferred_probe_reason); + dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: %pV", drv, vaf); + + mutex_unlock(&deferred_probe_mutex); +} ^^ Adds locking, kfree() and kasprintf() for every deferred probe during boot and can't be disabled. Right? -- Best regards, grygorii _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property 2020-06-30 8:59 ` Grygorii Strashko @ 2020-06-30 15:41 ` Andrzej Hajda 2020-06-30 18:00 ` Dmitry Torokhov 0 siblings, 1 reply; 20+ messages in thread From: Andrzej Hajda @ 2020-06-30 15:41 UTC (permalink / raw) To: Grygorii Strashko, Greg Kroah-Hartman Cc: Jernej Skrabec, Jonas Karlman, Bartlomiej Zolnierkiewicz, Russell King - ARM Linux, open list:DRM DRIVERS, linux-kernel, Neil Armstrong, andy.shevchenko, Mark Brown, Laurent Pinchart, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski On 30.06.2020 10:59, Grygorii Strashko wrote: > Hi > > On 29/06/2020 14:28, Andrzej Hajda wrote: >> Hi Grygorii, >> >> (...) >> >>>> /* >>>> * 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); >>>> >>> >>> Sry, may be i missing smth, but shouldn't it be optional >>> (CONFIG_DEBUG_FS is probably too generic). >>> >> >> I am not sure what exactly are you referring to, but this patch does not >> add new property, it just extends functionality of existing one. > > Sry, needed to be more specific. > > You've added device_set_deferred_probe_reson(dev, &vaf); > which expected to be used on every EPROBE_DEFER in dev_err_probe() in > combination with > > + } else { > + device_set_deferred_probe_reson(dev, &vaf); > dev_dbg(dev, "error %d: %pV", err, &vaf); > > ^^ dev_dbg() does not add any runtime overhead during boot unless enabled > + } > > But: > > +void device_set_deferred_probe_reson(const struct device *dev, struct > va_format *vaf) > +{ > + const char *drv = dev_driver_string(dev); > + > + mutex_lock(&deferred_probe_mutex); > + > + kfree(dev->p->deferred_probe_reason); > + dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: > %pV", drv, vaf); > + > + mutex_unlock(&deferred_probe_mutex); > +} > > ^^ Adds locking, kfree() and kasprintf() for every deferred probe > during boot and can't be disabled. > > Right? Right, but usually the burden should be insignificant in comparison to probe time, so I do not think it is worth optimizing. Regards Andrzej > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property 2020-06-30 15:41 ` Andrzej Hajda @ 2020-06-30 18:00 ` Dmitry Torokhov 2020-07-02 6:57 ` Andrzej Hajda 0 siblings, 1 reply; 20+ messages in thread From: Dmitry Torokhov @ 2020-06-30 18:00 UTC (permalink / raw) To: Andrzej Hajda Cc: Jernej Skrabec, Grygorii Strashko, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Jonas Karlman, Russell King - ARM Linux, open list:DRM DRIVERS, lkml, Neil Armstrong, Andy Shevchenko, Mark Brown, Laurent Pinchart, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski On Tue, Jun 30, 2020 at 8:42 AM Andrzej Hajda <a.hajda@samsung.com> wrote: > > > On 30.06.2020 10:59, Grygorii Strashko wrote: > > Hi > > > > On 29/06/2020 14:28, Andrzej Hajda wrote: > >> Hi Grygorii, > >> > >> (...) > >> > >>>> /* > >>>> * 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); > >>>> > >>> > >>> Sry, may be i missing smth, but shouldn't it be optional > >>> (CONFIG_DEBUG_FS is probably too generic). > >>> > >> > >> I am not sure what exactly are you referring to, but this patch does not > >> add new property, it just extends functionality of existing one. > > > > Sry, needed to be more specific. > > > > You've added device_set_deferred_probe_reson(dev, &vaf); > > which expected to be used on every EPROBE_DEFER in dev_err_probe() in > > combination with > > > > + } else { > > + device_set_deferred_probe_reson(dev, &vaf); > > dev_dbg(dev, "error %d: %pV", err, &vaf); > > > > ^^ dev_dbg() does not add any runtime overhead during boot unless enabled > > + } > > > > But: > > > > +void device_set_deferred_probe_reson(const struct device *dev, struct > > va_format *vaf) > > +{ > > + const char *drv = dev_driver_string(dev); > > + > > + mutex_lock(&deferred_probe_mutex); > > + > > + kfree(dev->p->deferred_probe_reason); > > + dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: > > %pV", drv, vaf); > > + > > + mutex_unlock(&deferred_probe_mutex); > > +} > > > > ^^ Adds locking, kfree() and kasprintf() for every deferred probe > > during boot and can't be disabled. > > > > Right? > > > Right, but usually the burden should be insignificant in comparison to > probe time, so I do not think it is worth optimizing. I do not think this is going to take. You are suggesting that we modify pretty much every driver to supply this deferral reason, and I doubt it will happen. Can we put this burden on providers that raise the deferral? I.e. majority of code are using devm API now, so we most likely know the device for which deferral is being raised. We can have a list of deferral reasons and their devices and when in device code once probe is done we could try reconciling it with the deferred devicelist, and this would mean you only need to implement this in gpiolib, regulator core, clocks core, etc. Thanks. -- Dmitry _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property 2020-06-30 18:00 ` Dmitry Torokhov @ 2020-07-02 6:57 ` Andrzej Hajda 2020-07-07 4:14 ` Dmitry Torokhov 0 siblings, 1 reply; 20+ messages in thread From: Andrzej Hajda @ 2020-07-02 6:57 UTC (permalink / raw) To: Dmitry Torokhov Cc: Jernej Skrabec, Grygorii Strashko, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Jonas Karlman, Russell King - ARM Linux, open list:DRM DRIVERS, lkml, Neil Armstrong, Andy Shevchenko, Mark Brown, Laurent Pinchart, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski On 30.06.2020 20:00, Dmitry Torokhov wrote: > On Tue, Jun 30, 2020 at 8:42 AM Andrzej Hajda <a.hajda@samsung.com> wrote: >> >> On 30.06.2020 10:59, Grygorii Strashko wrote: >>> Hi >>> >>> On 29/06/2020 14:28, Andrzej Hajda wrote: >>>> Hi Grygorii, >>>> >>>> (...) >>>> >>>>>> /* >>>>>> * 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); >>>>>> >>>>> Sry, may be i missing smth, but shouldn't it be optional >>>>> (CONFIG_DEBUG_FS is probably too generic). >>>>> >>>> I am not sure what exactly are you referring to, but this patch does not >>>> add new property, it just extends functionality of existing one. >>> Sry, needed to be more specific. >>> >>> You've added device_set_deferred_probe_reson(dev, &vaf); >>> which expected to be used on every EPROBE_DEFER in dev_err_probe() in >>> combination with >>> >>> + } else { >>> + device_set_deferred_probe_reson(dev, &vaf); >>> dev_dbg(dev, "error %d: %pV", err, &vaf); >>> >>> ^^ dev_dbg() does not add any runtime overhead during boot unless enabled >>> + } >>> >>> But: >>> >>> +void device_set_deferred_probe_reson(const struct device *dev, struct >>> va_format *vaf) >>> +{ >>> + const char *drv = dev_driver_string(dev); >>> + >>> + mutex_lock(&deferred_probe_mutex); >>> + >>> + kfree(dev->p->deferred_probe_reason); >>> + dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: >>> %pV", drv, vaf); >>> + >>> + mutex_unlock(&deferred_probe_mutex); >>> +} >>> >>> ^^ Adds locking, kfree() and kasprintf() for every deferred probe >>> during boot and can't be disabled. >>> >>> Right? >> >> Right, but usually the burden should be insignificant in comparison to >> probe time, so I do not think it is worth optimizing. > I do not think this is going to take. You are suggesting that we > modify pretty much every driver to supply this deferral reason, and I > doubt it will happen. Can we put this burden on providers that raise > the deferral? I wouldn't say they raise the deferral, they just inform resource is not yet available. Only device driver, and only in its probe function can "raise the deferral". > I.e. majority of code are using devm API now, so we most > likely know the device for which deferral is being raised. We can have > a list of deferral reasons and their devices and when in device code > once probe is done we could try reconciling it with the deferred > devicelist, and this would mean you only need to implement this in > gpiolib, regulator core, clocks core, etc. This patchset tries to solve simple issue - replace multiple lines of code present in multiple probe functions (additionally fixing lot of them) with single call and then enhance it little bit, nothing more. What you are proposing is blurry at the moment for me, provider does not know if consumer want to defer, or will continue working without missing resource, moreover some consumers can acquire resources after probe - again no probe deferral. Even if it will be done (it can be, for example by creating probe version of all resource get functions), it will require much more changes but finally it will look like: res = devm_get_resource_from_probe(....) if (IS_ERR(res)) return PTR_ERR(res); vs: res = devm_get_resource(...) if (IS_ERR(res)) return dev_err_probe(dev, PTR_ERR(res), ...); Regards Andrzej > > Thanks. > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property 2020-07-02 6:57 ` Andrzej Hajda @ 2020-07-07 4:14 ` Dmitry Torokhov 2020-07-10 7:42 ` Andrzej Hajda 0 siblings, 1 reply; 20+ messages in thread From: Dmitry Torokhov @ 2020-07-07 4:14 UTC (permalink / raw) To: Andrzej Hajda Cc: Jernej Skrabec, Grygorii Strashko, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Jonas Karlman, Russell King - ARM Linux, open list:DRM DRIVERS, lkml, Neil Armstrong, Andy Shevchenko, Mark Brown, Laurent Pinchart, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski On Thu, Jul 02, 2020 at 08:57:55AM +0200, Andrzej Hajda wrote: > > On 30.06.2020 20:00, Dmitry Torokhov wrote: > > On Tue, Jun 30, 2020 at 8:42 AM Andrzej Hajda <a.hajda@samsung.com> wrote: > >> > >> On 30.06.2020 10:59, Grygorii Strashko wrote: > >>> Hi > >>> > >>> On 29/06/2020 14:28, Andrzej Hajda wrote: > >>>> Hi Grygorii, > >>>> > >>>> (...) > >>>> > >>>>>> /* > >>>>>> * 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); > >>>>>> > >>>>> Sry, may be i missing smth, but shouldn't it be optional > >>>>> (CONFIG_DEBUG_FS is probably too generic). > >>>>> > >>>> I am not sure what exactly are you referring to, but this patch does not > >>>> add new property, it just extends functionality of existing one. > >>> Sry, needed to be more specific. > >>> > >>> You've added device_set_deferred_probe_reson(dev, &vaf); > >>> which expected to be used on every EPROBE_DEFER in dev_err_probe() in > >>> combination with > >>> > >>> + } else { > >>> + device_set_deferred_probe_reson(dev, &vaf); > >>> dev_dbg(dev, "error %d: %pV", err, &vaf); > >>> > >>> ^^ dev_dbg() does not add any runtime overhead during boot unless enabled > >>> + } > >>> > >>> But: > >>> > >>> +void device_set_deferred_probe_reson(const struct device *dev, struct > >>> va_format *vaf) > >>> +{ > >>> + const char *drv = dev_driver_string(dev); > >>> + > >>> + mutex_lock(&deferred_probe_mutex); > >>> + > >>> + kfree(dev->p->deferred_probe_reason); > >>> + dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: > >>> %pV", drv, vaf); > >>> + > >>> + mutex_unlock(&deferred_probe_mutex); > >>> +} > >>> > >>> ^^ Adds locking, kfree() and kasprintf() for every deferred probe > >>> during boot and can't be disabled. > >>> > >>> Right? > >> > >> Right, but usually the burden should be insignificant in comparison to > >> probe time, so I do not think it is worth optimizing. > > I do not think this is going to take. You are suggesting that we > > modify pretty much every driver to supply this deferral reason, and I > > doubt it will happen. Can we put this burden on providers that raise > > the deferral? > > > I wouldn't say they raise the deferral, they just inform resource is not > yet available. Only device driver, and only in its probe function can > "raise the deferral". Well, this is a matter of perspective. If devm_gpiod_get() returns -EBUSY and this is returned to driver core, is it GPIO line signals that line is busy, or is it the driver applies its knowledge. I say that in majority of cases driver does not really get a say in this and simply has to pass whatever error condition that is signalled by providers up the stack. I would consider whenever a driver does not propagate -EPROBE_DEFER to the driver code a bug that needs fixing, because it should not degrade functionality and/or performance just because we have not figured out how to order probing properly and have to rely on deferrals. > > > > I.e. majority of code are using devm API now, so we most > > likely know the device for which deferral is being raised. We can have > > a list of deferral reasons and their devices and when in device code > > once probe is done we could try reconciling it with the deferred > > devicelist, and this would mean you only need to implement this in > > gpiolib, regulator core, clocks core, etc. > > > This patchset tries to solve simple issue - replace multiple lines of > code present in multiple probe functions (additionally fixing lot of > them) with single call and then enhance it little bit, nothing more. > > What you are proposing is blurry at the moment for me, provider does not > know if consumer want to defer, This is my point - the consumer does not get to decide. If deferral is raised, it must be honored. > or will continue working without missing resource, Deferral does not mean resource does not exist and the driver has to get by if it can. It means the resource is not ready, and even if the system can work without it, it will not be working optimally. > moreover some consumers can acquire resources after probe - again no > probe deferral. In this case we should not signal deferral either. > Even if it will be done (it can be, for > example by creating probe version of all resource get functions), it > will require much more changes but finally it will look like: > > res = devm_get_resource_from_probe(....) > > if (IS_ERR(res)) > > return PTR_ERR(res); > > vs: > > res = devm_get_resource(...) > > if (IS_ERR(res)) > > return dev_err_probe(dev, PTR_ERR(res), ...); And we will need to adjust how many hundreds of drivers? Consider that most drivers use devm_clk_get(), devm_gpiod_get() and devm_regulator_get() and their friends. All these APIs already have device for which resource is being allocated, and moreover their use outside of probe() path is highly suspicious (because devm outside of probe() typically result in unwinding in really surprising order). So if you could stash device and deferral reason in a list and then scan this list in driver core when handling the raised deferral you would not need to change anything in individual drivers. Hope this clears what I had in mind. Thanks. -- Dmitry _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property 2020-07-07 4:14 ` Dmitry Torokhov @ 2020-07-10 7:42 ` Andrzej Hajda 2020-07-10 11:07 ` Mark Brown 0 siblings, 1 reply; 20+ messages in thread From: Andrzej Hajda @ 2020-07-10 7:42 UTC (permalink / raw) To: Dmitry Torokhov Cc: Jernej Skrabec, Grygorii Strashko, Jonas Karlman, Greg Kroah-Hartman, Bartlomiej Zolnierkiewicz, Russell King - ARM Linux, open list:DRM DRIVERS, lkml, Neil Armstrong, Andy Shevchenko, Mark Brown, Laurent Pinchart, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski On 07.07.2020 06:14, Dmitry Torokhov wrote: > On Thu, Jul 02, 2020 at 08:57:55AM +0200, Andrzej Hajda wrote: >> On 30.06.2020 20:00, Dmitry Torokhov wrote: >>> On Tue, Jun 30, 2020 at 8:42 AM Andrzej Hajda <a.hajda@samsung.com> wrote: >>>> On 30.06.2020 10:59, Grygorii Strashko wrote: >>>>> Hi >>>>> >>>>> On 29/06/2020 14:28, Andrzej Hajda wrote: >>>>>> Hi Grygorii, >>>>>> >>>>>> (...) >>>>>> >>>>>>>> /* >>>>>>>> * 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); >>>>>>>> >>>>>>> Sry, may be i missing smth, but shouldn't it be optional >>>>>>> (CONFIG_DEBUG_FS is probably too generic). >>>>>>> >>>>>> I am not sure what exactly are you referring to, but this patch does not >>>>>> add new property, it just extends functionality of existing one. >>>>> Sry, needed to be more specific. >>>>> >>>>> You've added device_set_deferred_probe_reson(dev, &vaf); >>>>> which expected to be used on every EPROBE_DEFER in dev_err_probe() in >>>>> combination with >>>>> >>>>> + } else { >>>>> + device_set_deferred_probe_reson(dev, &vaf); >>>>> dev_dbg(dev, "error %d: %pV", err, &vaf); >>>>> >>>>> ^^ dev_dbg() does not add any runtime overhead during boot unless enabled >>>>> + } >>>>> >>>>> But: >>>>> >>>>> +void device_set_deferred_probe_reson(const struct device *dev, struct >>>>> va_format *vaf) >>>>> +{ >>>>> + const char *drv = dev_driver_string(dev); >>>>> + >>>>> + mutex_lock(&deferred_probe_mutex); >>>>> + >>>>> + kfree(dev->p->deferred_probe_reason); >>>>> + dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: >>>>> %pV", drv, vaf); >>>>> + >>>>> + mutex_unlock(&deferred_probe_mutex); >>>>> +} >>>>> >>>>> ^^ Adds locking, kfree() and kasprintf() for every deferred probe >>>>> during boot and can't be disabled. >>>>> >>>>> Right? >>>> Right, but usually the burden should be insignificant in comparison to >>>> probe time, so I do not think it is worth optimizing. >>> I do not think this is going to take. You are suggesting that we >>> modify pretty much every driver to supply this deferral reason, and I >>> doubt it will happen. Can we put this burden on providers that raise >>> the deferral? >> >> I wouldn't say they raise the deferral, they just inform resource is not >> yet available. Only device driver, and only in its probe function can >> "raise the deferral". > Well, this is a matter of perspective. If devm_gpiod_get() returns > -EBUSY and this is returned to driver core, is it GPIO line signals that > line is busy, or is it the driver applies its knowledge. I say that in > majority of cases driver does not really get a say in this and simply > has to pass whatever error condition that is signalled by providers up > the stack. > > I would consider whenever a driver does not propagate -EPROBE_DEFER to > the driver code a bug that needs fixing, because it should not degrade > functionality and/or performance just because we have not figured out > how to order probing properly and have to rely on deferrals. > >> >>> I.e. majority of code are using devm API now, so we most >>> likely know the device for which deferral is being raised. We can have >>> a list of deferral reasons and their devices and when in device code >>> once probe is done we could try reconciling it with the deferred >>> devicelist, and this would mean you only need to implement this in >>> gpiolib, regulator core, clocks core, etc. >> >> This patchset tries to solve simple issue - replace multiple lines of >> code present in multiple probe functions (additionally fixing lot of >> them) with single call and then enhance it little bit, nothing more. >> >> What you are proposing is blurry at the moment for me, provider does not >> know if consumer want to defer, > This is my point - the consumer does not get to decide. If deferral is > raised, it must be honored. > >> or will continue working without missing resource, > Deferral does not mean resource does not exist and the driver has to get > by if it can. It means the resource is not ready, and even if the system > can work without it, it will not be working optimally. > >> moreover some consumers can acquire resources after probe - again no >> probe deferral. > In this case we should not signal deferral either. But the provider does not know if *get is called in probe context or not, so it is not able to differentiate it. So the whole idea is for me suspicious/wrong. Kind of proof: 1. If you insist that provider's EPROBE_ERROR must be always propagated to driver core then. 2. You must enforce that resources can be gathered only from probe. 3. But this is against current practice, even if majority of drivers does it from probe, there are many which doesn't. QED :) So if you really want to go this way it would be good to look at these drivers, check why they do it this way, and try to convert them. But I do not think it is a good way. Regards Andrzej > >> Even if it will be done (it can be, for >> example by creating probe version of all resource get functions), it >> will require much more changes but finally it will look like: >> >> res = devm_get_resource_from_probe(....) >> >> if (IS_ERR(res)) >> >> return PTR_ERR(res); >> >> vs: >> >> res = devm_get_resource(...) >> >> if (IS_ERR(res)) >> >> return dev_err_probe(dev, PTR_ERR(res), ...); > And we will need to adjust how many hundreds of drivers? > > Consider that most drivers use devm_clk_get(), devm_gpiod_get() and > devm_regulator_get() and their friends. All these APIs already have > device for which resource is being allocated, and moreover their use > outside of probe() path is highly suspicious (because devm outside of > probe() typically result in unwinding in really surprising order). So if > you could stash device and deferral reason in a list and then scan this > list in driver core when handling the raised deferral you would not need > to change anything in individual drivers. > Hope this clears what I had in mind. > > Thanks. > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property 2020-07-10 7:42 ` Andrzej Hajda @ 2020-07-10 11:07 ` Mark Brown 0 siblings, 0 replies; 20+ messages in thread From: Mark Brown @ 2020-07-10 11:07 UTC (permalink / raw) To: Andrzej Hajda Cc: Andy Shevchenko, Jernej Skrabec, Grygorii Strashko, Jonas Karlman, Greg Kroah-Hartman, Dmitry Torokhov, Russell King - ARM Linux, open list:DRM DRIVERS, lkml, Neil Armstrong, Bartlomiej Zolnierkiewicz, Laurent Pinchart, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski [-- Attachment #1.1: Type: text/plain, Size: 635 bytes --] On Fri, Jul 10, 2020 at 09:42:49AM +0200, Andrzej Hajda wrote: > But the provider does not know if *get is called in probe context or > not, so it is not able to differentiate it. > So the whole idea is for me suspicious/wrong. Kind of proof: > 1. If you insist that provider's EPROBE_ERROR must be always propagated > to driver core then. > 2. You must enforce that resources can be gathered only from probe. > 3. But this is against current practice, even if majority of drivers > does it from probe, there are many which doesn't. Those drivers are probably buggy anyway at this point given probe deferral. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 20+ messages in thread
[parent not found: <CGME20200626100110eucas1p24327c924dada0c2e86ecf0ab5b5af571@eucas1p2.samsung.com>]
* [PATCH v6 3/4] drm/bridge/sii8620: fix resource acquisition error handling [not found] ` <CGME20200626100110eucas1p24327c924dada0c2e86ecf0ab5b5af571@eucas1p2.samsung.com> @ 2020-06-26 10:01 ` Andrzej Hajda 2020-06-29 8:33 ` Neil Armstrong 0 siblings, 1 reply; 20+ messages in thread From: Andrzej Hajda @ 2020-06-26 10:01 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Jernej Skrabec, Rafael J. Wysocki, Jonas Karlman, Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, andy.shevchenko, Mark Brown, Laurent Pinchart, Daniel Vetter, linux-arm-kernel, Marek Szyprowski In case of error during resource acquisition driver should print error message only in case it is not deferred probe, using dev_err_probe helper solves the issue. Moreover it records defer probe reason for debugging. Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> --- drivers/gpu/drm/bridge/sil-sii8620.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c index 92acd336aa89..389c1f029774 100644 --- a/drivers/gpu/drm/bridge/sil-sii8620.c +++ b/drivers/gpu/drm/bridge/sil-sii8620.c @@ -2299,10 +2299,9 @@ static int sii8620_probe(struct i2c_client *client, INIT_LIST_HEAD(&ctx->mt_queue); ctx->clk_xtal = devm_clk_get(dev, "xtal"); - if (IS_ERR(ctx->clk_xtal)) { - dev_err(dev, "failed to get xtal clock from DT\n"); - return PTR_ERR(ctx->clk_xtal); - } + if (IS_ERR(ctx->clk_xtal)) + return dev_err_probe(dev, PTR_ERR(ctx->clk_xtal), + "failed to get xtal clock from DT\n"); if (!client->irq) { dev_err(dev, "no irq provided\n"); @@ -2313,16 +2312,14 @@ static int sii8620_probe(struct i2c_client *client, sii8620_irq_thread, IRQF_TRIGGER_HIGH | IRQF_ONESHOT, "sii8620", ctx); - if (ret < 0) { - dev_err(dev, "failed to install IRQ handler\n"); - return ret; - } + if (ret < 0) + return dev_err_probe(dev, ret, + "failed to install IRQ handler\n"); ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); - if (IS_ERR(ctx->gpio_reset)) { - dev_err(dev, "failed to get reset gpio from DT\n"); - return PTR_ERR(ctx->gpio_reset); - } + if (IS_ERR(ctx->gpio_reset)) + return dev_err_probe(dev, PTR_ERR(ctx->gpio_reset), + "failed to get reset gpio from DT\n"); ctx->supplies[0].supply = "cvcc10"; ctx->supplies[1].supply = "iovcc18"; -- 2.17.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v6 3/4] drm/bridge/sii8620: fix resource acquisition error handling 2020-06-26 10:01 ` [PATCH v6 3/4] drm/bridge/sii8620: fix resource acquisition error handling Andrzej Hajda @ 2020-06-29 8:33 ` Neil Armstrong 0 siblings, 0 replies; 20+ messages in thread From: Neil Armstrong @ 2020-06-29 8:33 UTC (permalink / raw) To: Andrzej Hajda, Greg Kroah-Hartman Cc: Jernej Skrabec, Rafael J. Wysocki, Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Jonas Karlman, andy.shevchenko, Mark Brown, Laurent Pinchart, Daniel Vetter, linux-arm-kernel, Marek Szyprowski On 26/06/2020 12:01, Andrzej Hajda wrote: > 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> > --- > 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"; > Nice helper, totally missed this patchset before ! Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 20+ messages in thread
[parent not found: <CGME20200626100111eucas1p18e175e6c77af483bd80fb90c171b05db@eucas1p1.samsung.com>]
* [PATCH v6 4/4] drm/bridge: lvds-codec: simplify error handling [not found] ` <CGME20200626100111eucas1p18e175e6c77af483bd80fb90c171b05db@eucas1p1.samsung.com> @ 2020-06-26 10:01 ` Andrzej Hajda 2020-06-29 8:33 ` Neil Armstrong 0 siblings, 1 reply; 20+ messages in thread From: Andrzej Hajda @ 2020-06-26 10:01 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Jernej Skrabec, Rafael J. Wysocki, Jonas Karlman, Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, andy.shevchenko, Mark Brown, Laurent Pinchart, Daniel Vetter, linux-arm-kernel, Marek Szyprowski Using dev_err_probe code has following advantages: - shorter code, - recorded defer probe reason for debugging, - uniform error code logging. Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> --- drivers/gpu/drm/bridge/lvds-codec.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c index 24fb1befdfa2..f19d9f7a5db2 100644 --- a/drivers/gpu/drm/bridge/lvds-codec.c +++ b/drivers/gpu/drm/bridge/lvds-codec.c @@ -71,13 +71,9 @@ static int lvds_codec_probe(struct platform_device *pdev) lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev); lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", GPIOD_OUT_HIGH); - if (IS_ERR(lvds_codec->powerdown_gpio)) { - int err = PTR_ERR(lvds_codec->powerdown_gpio); - - if (err != -EPROBE_DEFER) - dev_err(dev, "powerdown GPIO failure: %d\n", err); - return err; - } + if (IS_ERR(lvds_codec->powerdown_gpio)) + return dev_err_probe(dev, PTR_ERR(lvds_codec->powerdown_gpio), + "powerdown GPIO failure\n"); /* Locate the panel DT node. */ panel_node = of_graph_get_remote_node(dev->of_node, 1, 0); -- 2.17.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v6 4/4] drm/bridge: lvds-codec: simplify error handling 2020-06-26 10:01 ` [PATCH v6 4/4] drm/bridge: lvds-codec: simplify " Andrzej Hajda @ 2020-06-29 8:33 ` Neil Armstrong 0 siblings, 0 replies; 20+ messages in thread From: Neil Armstrong @ 2020-06-29 8:33 UTC (permalink / raw) To: Andrzej Hajda, Greg Kroah-Hartman Cc: Jernej Skrabec, Rafael J. Wysocki, Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Jonas Karlman, andy.shevchenko, Mark Brown, Laurent Pinchart, Daniel Vetter, linux-arm-kernel, Marek Szyprowski On 26/06/2020 12:01, Andrzej Hajda wrote: > 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> > --- > 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); > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-07-10 11:09 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20200626100108eucas1p2c6d68625f3755a467d7316dd27704f7c@eucas1p2.samsung.com> 2020-06-26 10:00 ` [PATCH v6 0/4] driver core: add probe error check helper Andrzej Hajda [not found] ` <CGME20200626100109eucas1p25331652d017cd17d21c0ae60541d1f73@eucas1p2.samsung.com> 2020-06-26 10:01 ` [PATCH v6 1/4] driver core: add device probe log helper Andrzej Hajda 2020-06-26 15:41 ` Rafael J. Wysocki 2020-06-26 16:22 ` Sam Ravnborg 2020-07-01 9:29 ` Grygorii Strashko [not found] ` <CGME20200626100110eucas1p2c5b91f2c98a5c6e5739f5af3207d192e@eucas1p2.samsung.com> 2020-06-26 10:01 ` [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property Andrzej Hajda 2020-06-26 15:43 ` Rafael J. Wysocki 2020-06-26 17:11 ` Grygorii Strashko 2020-06-29 11:28 ` Andrzej Hajda 2020-06-30 8:59 ` Grygorii Strashko 2020-06-30 15:41 ` Andrzej Hajda 2020-06-30 18:00 ` Dmitry Torokhov 2020-07-02 6:57 ` Andrzej Hajda 2020-07-07 4:14 ` Dmitry Torokhov 2020-07-10 7:42 ` Andrzej Hajda 2020-07-10 11:07 ` Mark Brown [not found] ` <CGME20200626100110eucas1p24327c924dada0c2e86ecf0ab5b5af571@eucas1p2.samsung.com> 2020-06-26 10:01 ` [PATCH v6 3/4] drm/bridge/sii8620: fix resource acquisition error handling Andrzej Hajda 2020-06-29 8:33 ` Neil Armstrong [not found] ` <CGME20200626100111eucas1p18e175e6c77af483bd80fb90c171b05db@eucas1p1.samsung.com> 2020-06-26 10:01 ` [PATCH v6 4/4] drm/bridge: lvds-codec: simplify " Andrzej Hajda 2020-06-29 8:33 ` Neil Armstrong
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).