* [RESEND PATCH v5 0/5] driver core: add probe error check helper [not found] <CGME20200624114134eucas1p2799e0ae76fcb026a0e4bcccc2026b732@eucas1p2.samsung.com> @ 2020-06-24 11:41 ` Andrzej Hajda [not found] ` <CGME20200624114135eucas1p26e2e4683d60cebdce7acd55177013992@eucas1p2.samsung.com> ` (4 more replies) 0 siblings, 5 replies; 41+ messages in thread From: Andrzej Hajda @ 2020-06-24 11:41 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, Recently I took some time to re-check error handling in drivers probe code, and I have noticed that number of incorrect resource acquisition error handling increased and there are no other propositions which can cure the situation. So I have decided to resend my old proposition of probe_err helper which should simplify resource acquisition error handling, it also extend it with adding defer probe reason to devices_deferred debugfs property, which should improve debugging experience for developers/testers. In v5 I have also attached patch adding macro to replace quite frequent calls: probe_err(dev, PTR_ERR(ptr), ...) with probe_err(dev, ptr, ...) I have also added two patches showing usage and benefits of the helper. My dirty/ad-hoc cocci scripts shows that this helper can be used in at least 2700 places saving about 3500 lines of code. Regards Andrzej Andrzej Hajda (5): driver core: add probe_err log helper driver core: add deferring probe reason to devices_deferred property drivers core: allow probe_err accept integer and pointer types drm/bridge/sii8620: fix resource acquisition error handling drm/bridge: lvds-codec: simplify error handling code drivers/base/base.h | 3 +++ drivers/base/core.c | 20 ++++++++++++++++++++ drivers/base/dd.c | 21 ++++++++++++++++++++- drivers/gpu/drm/bridge/lvds-codec.c | 9 ++------- drivers/gpu/drm/bridge/sil-sii8620.c | 18 ++++++------------ include/linux/device.h | 26 ++++++++++++++++++++++++++ 6 files changed, 77 insertions(+), 20 deletions(-) -- 2.17.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
[parent not found: <CGME20200624114135eucas1p26e2e4683d60cebdce7acd55177013992@eucas1p2.samsung.com>]
* [RESEND PATCH v5 1/5] driver core: add probe_err log helper [not found] ` <CGME20200624114135eucas1p26e2e4683d60cebdce7acd55177013992@eucas1p2.samsung.com> @ 2020-06-24 11:41 ` Andrzej Hajda 2020-06-24 11:52 ` Rafael J. Wysocki ` (2 more replies) 0 siblings, 3 replies; 41+ messages in thread From: Andrzej Hajda @ 2020-06-24 11:41 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 omited or implemented only partially. probe_err helps to replace such code sequences with simple call, so code: if (err != -EPROBE_DEFER) dev_err(dev, ...); return err; becomes: return probe_err(dev, err, ...); Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Reviewed-by: Mark Brown <broonie@kernel.org> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> --- drivers/base/core.c | 39 +++++++++++++++++++++++++++++++++++++++ include/linux/device.h | 3 +++ 2 files changed, 42 insertions(+) diff --git a/drivers/base/core.c b/drivers/base/core.c index 67d39a90b45c..ee9da66bff1b 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -3953,6 +3953,45 @@ define_dev_printk_level(_dev_info, KERN_INFO); #endif +/** + * probe_err - probe error check and log helper + * @dev: the pointer to the struct device + * @err: error value to test + * @fmt: printf-style format string + * @...: arguments as specified in the format string + * + * This helper implements common pattern present in probe functions for error + * checking: print message if the error is not -EPROBE_DEFER and propagate it. + * It replaces code sequence: + * if (err != -EPROBE_DEFER) + * dev_err(dev, ...); + * return err; + * with + * return probe_err(dev, err, ...); + * + * Returns @err. + * + */ +int probe_err(const struct device *dev, int err, const char *fmt, ...) +{ + struct va_format vaf; + va_list args; + + if (err == -EPROBE_DEFER) + return err; + + va_start(args, fmt); + vaf.fmt = fmt; + vaf.va = &args; + + dev_err(dev, "error %d: %pV", err, &vaf); + + va_end(args); + + return err; +} +EXPORT_SYMBOL_GPL(probe_err); + static inline bool fwnode_is_primary(struct fwnode_handle *fwnode) { return fwnode && !IS_ERR(fwnode->secondary); diff --git a/include/linux/device.h b/include/linux/device.h index 15460a5ac024..40a90d9bf799 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -964,6 +964,9 @@ void device_link_remove(void *consumer, struct device *supplier); void device_links_supplier_sync_state_pause(void); void device_links_supplier_sync_state_resume(void); +extern __printf(3, 4) +int probe_err(const struct device *dev, int err, const char *fmt, ...); + /* Create alias, so I can be autoloaded. */ #define MODULE_ALIAS_CHARDEV(major,minor) \ MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor)) -- 2.17.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper 2020-06-24 11:41 ` [RESEND PATCH v5 1/5] driver core: add probe_err log helper Andrzej Hajda @ 2020-06-24 11:52 ` Rafael J. Wysocki 2020-06-24 12:31 ` Greg Kroah-Hartman 2020-06-24 13:27 ` Mark Brown 2 siblings, 0 replies; 41+ messages in thread From: Rafael J. Wysocki @ 2020-06-24 11:52 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, Linux ARM, Marek Szyprowski On Wed, Jun 24, 2020 at 1:41 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 omited or implemented only partially. > probe_err helps to replace such code sequences with simple call, so code: > if (err != -EPROBE_DEFER) > dev_err(dev, ...); > return err; > becomes: > return probe_err(dev, err, ...); > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > Reviewed-by: Mark Brown <broonie@kernel.org> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Reviewed-by Rafael J. Wysocki <rafael@kernel.org> > --- > drivers/base/core.c | 39 +++++++++++++++++++++++++++++++++++++++ > include/linux/device.h | 3 +++ > 2 files changed, 42 insertions(+) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 67d39a90b45c..ee9da66bff1b 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -3953,6 +3953,45 @@ define_dev_printk_level(_dev_info, KERN_INFO); > > #endif > > +/** > + * probe_err - probe error check and log helper > + * @dev: the pointer to the struct device > + * @err: error value to test > + * @fmt: printf-style format string > + * @...: arguments as specified in the format string > + * > + * This helper implements common pattern present in probe functions for error > + * checking: print message if the error is not -EPROBE_DEFER and propagate it. > + * It replaces code sequence: > + * if (err != -EPROBE_DEFER) > + * dev_err(dev, ...); > + * return err; > + * with > + * return probe_err(dev, err, ...); > + * > + * Returns @err. > + * > + */ > +int probe_err(const struct device *dev, int err, const char *fmt, ...) > +{ > + struct va_format vaf; > + va_list args; > + > + if (err == -EPROBE_DEFER) > + return err; > + > + va_start(args, fmt); > + vaf.fmt = fmt; > + vaf.va = &args; > + > + dev_err(dev, "error %d: %pV", err, &vaf); > + > + va_end(args); > + > + return err; > +} > +EXPORT_SYMBOL_GPL(probe_err); > + > static inline bool fwnode_is_primary(struct fwnode_handle *fwnode) > { > return fwnode && !IS_ERR(fwnode->secondary); > diff --git a/include/linux/device.h b/include/linux/device.h > index 15460a5ac024..40a90d9bf799 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -964,6 +964,9 @@ void device_link_remove(void *consumer, struct device *supplier); > void device_links_supplier_sync_state_pause(void); > void device_links_supplier_sync_state_resume(void); > > +extern __printf(3, 4) > +int probe_err(const struct device *dev, int err, const char *fmt, ...); > + > /* Create alias, so I can be autoloaded. */ > #define MODULE_ALIAS_CHARDEV(major,minor) \ > MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor)) > -- > 2.17.1 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper 2020-06-24 11:41 ` [RESEND PATCH v5 1/5] driver core: add probe_err log helper Andrzej Hajda 2020-06-24 11:52 ` Rafael J. Wysocki @ 2020-06-24 12:31 ` Greg Kroah-Hartman 2020-06-24 13:23 ` Laurent Pinchart 2020-06-24 13:27 ` Mark Brown 2 siblings, 1 reply; 41+ messages in thread From: Greg Kroah-Hartman @ 2020-06-24 12:31 UTC (permalink / raw) To: Andrzej Hajda Cc: Jernej Skrabec, Rafael J. Wysocki, Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Jonas Karlman, andy.shevchenko, Mark Brown, Laurent Pinchart, linux-arm-kernel, Marek Szyprowski On Wed, Jun 24, 2020 at 01:41:23PM +0200, Andrzej Hajda wrote: > During probe every time driver gets resource it should usually check for error > printk some message if it is not -EPROBE_DEFER and return the error. This > pattern is simple but requires adding few lines after any resource acquisition > code, as a result it is often omited or implemented only partially. > probe_err helps to replace such code sequences with simple call, so code: > if (err != -EPROBE_DEFER) > dev_err(dev, ...); > return err; > becomes: > return probe_err(dev, err, ...); > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > Reviewed-by: Mark Brown <broonie@kernel.org> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- > drivers/base/core.c | 39 +++++++++++++++++++++++++++++++++++++++ > include/linux/device.h | 3 +++ > 2 files changed, 42 insertions(+) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 67d39a90b45c..ee9da66bff1b 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -3953,6 +3953,45 @@ define_dev_printk_level(_dev_info, KERN_INFO); > > #endif > > +/** > + * probe_err - probe error check and log helper > + * @dev: the pointer to the struct device > + * @err: error value to test > + * @fmt: printf-style format string > + * @...: arguments as specified in the format string > + * > + * This helper implements common pattern present in probe functions for error > + * checking: print message if the error is not -EPROBE_DEFER and propagate it. > + * It replaces code sequence: > + * if (err != -EPROBE_DEFER) > + * dev_err(dev, ...); > + * return err; > + * with > + * return probe_err(dev, err, ...); > + * > + * Returns @err. > + * > + */ > +int probe_err(const struct device *dev, int err, const char *fmt, ...) > +{ > + struct va_format vaf; > + va_list args; > + > + if (err == -EPROBE_DEFER) > + return err; > + > + va_start(args, fmt); > + vaf.fmt = fmt; > + vaf.va = &args; > + > + dev_err(dev, "error %d: %pV", err, &vaf); > + > + va_end(args); > + > + return err; > +} > +EXPORT_SYMBOL_GPL(probe_err); Please be specific in global symbols, how about "driver_probe_error()"? And merge the other patch into this one, as Raphael said, otherwise this just looks odd to add something and then fix it up later. thanks, greg k-h _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper 2020-06-24 12:31 ` Greg Kroah-Hartman @ 2020-06-24 13:23 ` Laurent Pinchart 2020-06-24 14:04 ` Andrzej Hajda 0 siblings, 1 reply; 41+ messages in thread From: Laurent Pinchart @ 2020-06-24 13:23 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, linux-arm-kernel, Marek Szyprowski On Wed, Jun 24, 2020 at 02:31:40PM +0200, Greg Kroah-Hartman wrote: > On Wed, Jun 24, 2020 at 01:41:23PM +0200, Andrzej Hajda wrote: > > During probe every time driver gets resource it should usually check for error > > printk some message if it is not -EPROBE_DEFER and return the error. This > > pattern is simple but requires adding few lines after any resource acquisition > > code, as a result it is often omited or implemented only partially. > > probe_err helps to replace such code sequences with simple call, so code: > > if (err != -EPROBE_DEFER) > > dev_err(dev, ...); > > return err; > > becomes: > > return probe_err(dev, err, ...); > > > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > Reviewed-by: Mark Brown <broonie@kernel.org> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > --- > > drivers/base/core.c | 39 +++++++++++++++++++++++++++++++++++++++ > > include/linux/device.h | 3 +++ > > 2 files changed, 42 insertions(+) > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c > > index 67d39a90b45c..ee9da66bff1b 100644 > > --- a/drivers/base/core.c > > +++ b/drivers/base/core.c > > @@ -3953,6 +3953,45 @@ define_dev_printk_level(_dev_info, KERN_INFO); > > > > #endif > > > > +/** > > + * probe_err - probe error check and log helper > > + * @dev: the pointer to the struct device > > + * @err: error value to test > > + * @fmt: printf-style format string > > + * @...: arguments as specified in the format string > > + * > > + * This helper implements common pattern present in probe functions for error > > + * checking: print message if the error is not -EPROBE_DEFER and propagate it. > > + * It replaces code sequence: > > + * if (err != -EPROBE_DEFER) > > + * dev_err(dev, ...); > > + * return err; > > + * with > > + * return probe_err(dev, err, ...); > > + * > > + * Returns @err. > > + * > > + */ > > +int probe_err(const struct device *dev, int err, const char *fmt, ...) > > +{ > > + struct va_format vaf; > > + va_list args; > > + > > + if (err == -EPROBE_DEFER) > > + return err; > > + > > + va_start(args, fmt); > > + vaf.fmt = fmt; > > + vaf.va = &args; > > + > > + dev_err(dev, "error %d: %pV", err, &vaf); > > + > > + va_end(args); > > + > > + return err; > > +} > > +EXPORT_SYMBOL_GPL(probe_err); > > Please be specific in global symbols, how about "driver_probe_error()"? Or dev_err_probe() to match the existing dev_* functions ? > And merge the other patch into this one, as Raphael said, otherwise this > just looks odd to add something and then fix it up later. -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper 2020-06-24 13:23 ` Laurent Pinchart @ 2020-06-24 14:04 ` Andrzej Hajda 0 siblings, 0 replies; 41+ messages in thread From: Andrzej Hajda @ 2020-06-24 14:04 UTC (permalink / raw) To: Laurent Pinchart, 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, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski On 24.06.2020 15:23, Laurent Pinchart wrote: > On Wed, Jun 24, 2020 at 02:31:40PM +0200, Greg Kroah-Hartman wrote: >> On Wed, Jun 24, 2020 at 01:41:23PM +0200, Andrzej Hajda wrote: >>> During probe every time driver gets resource it should usually check for error >>> printk some message if it is not -EPROBE_DEFER and return the error. This >>> pattern is simple but requires adding few lines after any resource acquisition >>> code, as a result it is often omited or implemented only partially. >>> probe_err helps to replace such code sequences with simple call, so code: >>> if (err != -EPROBE_DEFER) >>> dev_err(dev, ...); >>> return err; >>> becomes: >>> return probe_err(dev, err, ...); >>> >>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> >>> Reviewed-by: Mark Brown <broonie@kernel.org> >>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >>> --- >>> drivers/base/core.c | 39 +++++++++++++++++++++++++++++++++++++++ >>> include/linux/device.h | 3 +++ >>> 2 files changed, 42 insertions(+) >>> >>> diff --git a/drivers/base/core.c b/drivers/base/core.c >>> index 67d39a90b45c..ee9da66bff1b 100644 >>> --- a/drivers/base/core.c >>> +++ b/drivers/base/core.c >>> @@ -3953,6 +3953,45 @@ define_dev_printk_level(_dev_info, KERN_INFO); >>> >>> #endif >>> >>> +/** >>> + * probe_err - probe error check and log helper >>> + * @dev: the pointer to the struct device >>> + * @err: error value to test >>> + * @fmt: printf-style format string >>> + * @...: arguments as specified in the format string >>> + * >>> + * This helper implements common pattern present in probe functions for error >>> + * checking: print message if the error is not -EPROBE_DEFER and propagate it. >>> + * It replaces code sequence: >>> + * if (err != -EPROBE_DEFER) >>> + * dev_err(dev, ...); >>> + * return err; >>> + * with >>> + * return probe_err(dev, err, ...); >>> + * >>> + * Returns @err. >>> + * >>> + */ >>> +int probe_err(const struct device *dev, int err, const char *fmt, ...) >>> +{ >>> + struct va_format vaf; >>> + va_list args; >>> + >>> + if (err == -EPROBE_DEFER) >>> + return err; >>> + >>> + va_start(args, fmt); >>> + vaf.fmt = fmt; >>> + vaf.va = &args; >>> + >>> + dev_err(dev, "error %d: %pV", err, &vaf); >>> + >>> + va_end(args); >>> + >>> + return err; >>> +} >>> +EXPORT_SYMBOL_GPL(probe_err); >> Please be specific in global symbols, how about "driver_probe_error()"? > Or dev_err_probe() to match the existing dev_* functions ? OK. Regards Andrzej > >> And merge the other patch into this one, as Raphael said, otherwise this >> just looks odd to add something and then fix it up later. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper 2020-06-24 11:41 ` [RESEND PATCH v5 1/5] driver core: add probe_err log helper Andrzej Hajda 2020-06-24 11:52 ` Rafael J. Wysocki 2020-06-24 12:31 ` Greg Kroah-Hartman @ 2020-06-24 13:27 ` Mark Brown 2020-06-24 13:45 ` Andy Shevchenko 2 siblings, 1 reply; 41+ messages in thread From: Mark Brown @ 2020-06-24 13:27 UTC (permalink / raw) To: Andrzej Hajda Cc: Jernej Skrabec, Rafael J. Wysocki, Greg Kroah-Hartman, Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Jonas Karlman, andy.shevchenko, Laurent Pinchart, linux-arm-kernel, Marek Szyprowski [-- Attachment #1.1: Type: text/plain, Size: 414 bytes --] On Wed, Jun 24, 2020 at 01:41:23PM +0200, Andrzej Hajda wrote: > During probe every time driver gets resource it should usually check for error > printk some message if it is not -EPROBE_DEFER and return the error. This As I said down the thread that's not a great pattern since it means that probe deferral errors never get displayed and users have a hard time figuring out why their driver isn't instantiating. [-- 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] 41+ messages in thread
* Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper 2020-06-24 13:27 ` Mark Brown @ 2020-06-24 13:45 ` Andy Shevchenko 2020-06-24 14:02 ` Mark Brown 0 siblings, 1 reply; 41+ messages in thread From: Andy Shevchenko @ 2020-06-24 13:45 UTC (permalink / raw) To: Mark Brown Cc: Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Rafael J. Wysocki, Linux Kernel Mailing List, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, Jonas Karlman, Laurent Pinchart, linux-arm Mailing List, Marek Szyprowski On Wed, Jun 24, 2020 at 4:27 PM Mark Brown <broonie@kernel.org> wrote: > > On Wed, Jun 24, 2020 at 01:41:23PM +0200, Andrzej Hajda wrote: > > During probe every time driver gets resource it should usually check for error > > printk some message if it is not -EPROBE_DEFER and return the error. This > > As I said down the thread that's not a great pattern since it means that > probe deferral errors never get displayed and users have a hard time > figuring out why their driver isn't instantiating. Don't we have a file in the debugfs to list deferred drivers? In the case of deferred probes the errors out of it makes users more miserable in order to look through tons of spam and lose really useful data in the logs. -- With Best Regards, Andy Shevchenko _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper 2020-06-24 13:45 ` Andy Shevchenko @ 2020-06-24 14:02 ` Mark Brown 2020-06-24 15:00 ` Robin Murphy 0 siblings, 1 reply; 41+ messages in thread From: Mark Brown @ 2020-06-24 14:02 UTC (permalink / raw) To: Andy Shevchenko Cc: Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Rafael J. Wysocki, Linux Kernel Mailing List, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, Jonas Karlman, Laurent Pinchart, linux-arm Mailing List, Marek Szyprowski [-- Attachment #1.1: Type: text/plain, Size: 885 bytes --] On Wed, Jun 24, 2020 at 04:45:28PM +0300, Andy Shevchenko wrote: > On Wed, Jun 24, 2020 at 4:27 PM Mark Brown <broonie@kernel.org> wrote: > > As I said down the thread that's not a great pattern since it means that > > probe deferral errors never get displayed and users have a hard time > > figuring out why their driver isn't instantiating. > Don't we have a file in the debugfs to list deferred drivers? Part of what this patch series aims to solve is that that list is not very useful since it doesn't provide any information on how things got deferred which means it's of no use in trying to figure out any problems. > In the case of deferred probes the errors out of it makes users more > miserable in order to look through tons of spam and lose really useful > data in the logs. I seem to never manage to end up using any of the systems which generate excessive deferrals. [-- 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] 41+ messages in thread
* Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper 2020-06-24 14:02 ` Mark Brown @ 2020-06-24 15:00 ` Robin Murphy 2020-06-24 15:28 ` Mark Brown 0 siblings, 1 reply; 41+ messages in thread From: Robin Murphy @ 2020-06-24 15:00 UTC (permalink / raw) To: Mark Brown 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, Andrzej Hajda, Andy Shevchenko, Laurent Pinchart, Marek Szyprowski, linux-arm Mailing List, Jonas Karlman On 2020-06-24 15:02, Mark Brown wrote: > On Wed, Jun 24, 2020 at 04:45:28PM +0300, Andy Shevchenko wrote: >> On Wed, Jun 24, 2020 at 4:27 PM Mark Brown <broonie@kernel.org> wrote: > >>> As I said down the thread that's not a great pattern since it means that >>> probe deferral errors never get displayed and users have a hard time >>> figuring out why their driver isn't instantiating. > >> Don't we have a file in the debugfs to list deferred drivers? > > Part of what this patch series aims to solve is that that list is not > very useful since it doesn't provide any information on how things got > deferred which means it's of no use in trying to figure out any > problems. > >> In the case of deferred probes the errors out of it makes users more >> miserable in order to look through tons of spam and lose really useful >> data in the logs. > > I seem to never manage to end up using any of the systems which generate > excessive deferrals. Be thankful... And count me in as one of those miserable users; here's one of mine being bad enough without even printing any specific messages about deferring ;) Robin. ----- [robin@weasel-cheese ~]$ dmesg | grep dwmmc [ 3.046297] dwmmc_rockchip ff0c0000.mmc: IDMAC supports 32-bit address mode. [ 3.054312] dwmmc_rockchip ff0c0000.mmc: Using internal DMA controller. [ 3.061774] dwmmc_rockchip ff0c0000.mmc: Version ID is 270a [ 3.068101] dwmmc_rockchip ff0c0000.mmc: DW MMC controller at irq 30,32 bit host data width,256 deep fifo [ 3.079638] dwmmc_rockchip ff0d0000.mmc: IDMAC supports 32-bit address mode. [ 3.087678] dwmmc_rockchip ff0d0000.mmc: Using internal DMA controller. [ 3.095134] dwmmc_rockchip ff0d0000.mmc: Version ID is 270a [ 3.101480] dwmmc_rockchip ff0d0000.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [ 3.113071] dwmmc_rockchip ff0f0000.mmc: IDMAC supports 32-bit address mode. [ 3.121110] dwmmc_rockchip ff0f0000.mmc: Using internal DMA controller. [ 3.128565] dwmmc_rockchip ff0f0000.mmc: Version ID is 270a [ 3.134886] dwmmc_rockchip ff0f0000.mmc: DW MMC controller at irq 32,32 bit host data width,256 deep fifo [ 3.948510] dwmmc_rockchip ff0c0000.mmc: IDMAC supports 32-bit address mode. [ 3.956475] dwmmc_rockchip ff0c0000.mmc: Using internal DMA controller. [ 3.963884] dwmmc_rockchip ff0c0000.mmc: Version ID is 270a [ 3.970133] dwmmc_rockchip ff0c0000.mmc: DW MMC controller at irq 30,32 bit host data width,256 deep fifo [ 4.141231] dwmmc_rockchip ff0d0000.mmc: IDMAC supports 32-bit address mode. [ 4.149178] dwmmc_rockchip ff0d0000.mmc: Using internal DMA controller. [ 4.156582] dwmmc_rockchip ff0d0000.mmc: Version ID is 270a [ 4.162823] dwmmc_rockchip ff0d0000.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [ 4.175606] dwmmc_rockchip ff0f0000.mmc: IDMAC supports 32-bit address mode. [ 4.183540] dwmmc_rockchip ff0f0000.mmc: Using internal DMA controller. [ 4.190946] dwmmc_rockchip ff0f0000.mmc: Version ID is 270a [ 4.197196] dwmmc_rockchip ff0f0000.mmc: DW MMC controller at irq 32,32 bit host data width,256 deep fifo [ 4.250758] dwmmc_rockchip ff0d0000.mmc: IDMAC supports 32-bit address mode. [ 4.258688] dwmmc_rockchip ff0d0000.mmc: Using internal DMA controller. [ 4.266104] dwmmc_rockchip ff0d0000.mmc: Version ID is 270a [ 4.272358] dwmmc_rockchip ff0d0000.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [ 4.285390] dwmmc_rockchip ff0d0000.mmc: IDMAC supports 32-bit address mode. [ 4.293333] dwmmc_rockchip ff0d0000.mmc: Using internal DMA controller. [ 4.300750] dwmmc_rockchip ff0d0000.mmc: Version ID is 270a [ 4.307005] dwmmc_rockchip ff0d0000.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [ 4.971373] dwmmc_rockchip ff0f0000.mmc: Successfully tuned phase to 134 [ 5.027225] dwmmc_rockchip ff0d0000.mmc: IDMAC supports 32-bit address mode. [ 5.035339] dwmmc_rockchip ff0d0000.mmc: Using internal DMA controller. [ 5.042769] dwmmc_rockchip ff0d0000.mmc: Version ID is 270a [ 5.049050] dwmmc_rockchip ff0d0000.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [ 24.727583] dwmmc_rockchip ff0d0000.mmc: IDMAC supports 32-bit address mode. [ 24.745541] dwmmc_rockchip ff0d0000.mmc: Using internal DMA controller. [ 24.753003] dwmmc_rockchip ff0d0000.mmc: Version ID is 270a [ 24.763289] dwmmc_rockchip ff0d0000.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [ 25.589620] dwmmc_rockchip ff0d0000.mmc: IDMAC supports 32-bit address mode. [ 25.603066] dwmmc_rockchip ff0d0000.mmc: Using internal DMA controller. [ 25.615283] dwmmc_rockchip ff0d0000.mmc: Version ID is 270a [ 25.627911] dwmmc_rockchip ff0d0000.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [ 25.643469] dwmmc_rockchip ff0d0000.mmc: IDMAC supports 32-bit address mode. [ 25.651532] dwmmc_rockchip ff0d0000.mmc: Using internal DMA controller. [ 25.658960] dwmmc_rockchip ff0d0000.mmc: Version ID is 270a [ 25.665246] dwmmc_rockchip ff0d0000.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [ 25.677154] dwmmc_rockchip ff0d0000.mmc: allocated mmc-pwrseq _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper 2020-06-24 15:00 ` Robin Murphy @ 2020-06-24 15:28 ` Mark Brown 0 siblings, 0 replies; 41+ messages in thread From: Mark Brown @ 2020-06-24 15:28 UTC (permalink / raw) To: Robin Murphy 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, Andrzej Hajda, Andy Shevchenko, Laurent Pinchart, Marek Szyprowski, linux-arm Mailing List, Jonas Karlman [-- Attachment #1.1: Type: text/plain, Size: 776 bytes --] On Wed, Jun 24, 2020 at 04:00:34PM +0100, Robin Murphy wrote: > Be thankful... And count me in as one of those miserable users; here's one > of mine being bad enough without even printing any specific messages about > deferring ;) > [robin@weasel-cheese ~]$ dmesg | grep dwmmc > [ 3.046297] dwmmc_rockchip ff0c0000.mmc: IDMAC supports 32-bit address mode. > [ 3.054312] dwmmc_rockchip ff0c0000.mmc: Using internal DMA controller. > [ 3.061774] dwmmc_rockchip ff0c0000.mmc: Version ID is 270a > [ 3.068101] dwmmc_rockchip ff0c0000.mmc: DW MMC controller at irq 30,32 bit host data width,256 deep fifo Looking at that driver it's going to have lots of problems whatever happens - it's printing out a huge amount of stuff before it's finished acquiring resources. [-- 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] 41+ messages in thread
[parent not found: <CGME20200624114136eucas1p1a3a31d95d86754201c7965f26ccd5de0@eucas1p1.samsung.com>]
* [RESEND PATCH v5 2/5] driver core: add deferring probe reason to devices_deferred property [not found] ` <CGME20200624114136eucas1p1a3a31d95d86754201c7965f26ccd5de0@eucas1p1.samsung.com> @ 2020-06-24 11:41 ` Andrzej Hajda 2020-06-24 12:11 ` Rafael J. Wysocki 2020-06-24 12:34 ` Greg Kroah-Hartman 0 siblings, 2 replies; 41+ messages in thread From: Andrzej Hajda @ 2020-06-24 11:41 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 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 | 10 ++++++---- drivers/base/dd.c | 21 ++++++++++++++++++++- 3 files changed, 29 insertions(+), 5 deletions(-) diff --git a/drivers/base/base.h b/drivers/base/base.h index 95c22c0f9036..93ef1c2f4c1f 100644 --- a/drivers/base/base.h +++ b/drivers/base/base.h @@ -93,6 +93,7 @@ struct device_private { struct klist_node knode_class; struct list_head deferred_probe; struct device_driver *async_driver; + char *deferred_probe_msg; struct device *device; u8 dead:1; }; @@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device *dev, extern void driver_detach(struct device_driver *drv); extern int driver_probe_device(struct device_driver *drv, struct device *dev); extern void driver_deferred_probe_del(struct device *dev); +extern void __deferred_probe_set_msg(const struct device *dev, + struct va_format *vaf); static inline int driver_match_device(struct device_driver *drv, struct device *dev) { diff --git a/drivers/base/core.c b/drivers/base/core.c index ee9da66bff1b..2a96954d5460 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -3962,6 +3962,8 @@ define_dev_printk_level(_dev_info, KERN_INFO); * * This helper implements common pattern present in probe functions for error * checking: print message if the error is not -EPROBE_DEFER and propagate it. + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked + * later by reading devices_deferred debugfs attribute. * It replaces code sequence: * if (err != -EPROBE_DEFER) * dev_err(dev, ...); @@ -3977,14 +3979,14 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...) struct va_format vaf; va_list args; - if (err == -EPROBE_DEFER) - return err; - va_start(args, fmt); vaf.fmt = fmt; vaf.va = &args; - dev_err(dev, "error %d: %pV", err, &vaf); + if (err == -EPROBE_DEFER) + __deferred_probe_set_msg(dev, &vaf); + else + dev_err(dev, "error %d: %pV", err, &vaf); va_end(args); diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 9a1d940342ac..f44d26454b6a 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -27,6 +27,7 @@ #include <linux/async.h> #include <linux/pm_runtime.h> #include <linux/pinctrl/devinfo.h> +#include <linux/slab.h> #include "base.h" #include "power/power.h" @@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev) if (!list_empty(&dev->p->deferred_probe)) { dev_dbg(dev, "Removed from deferred list\n"); list_del_init(&dev->p->deferred_probe); + kfree(dev->p->deferred_probe_msg); + dev->p->deferred_probe_msg = NULL; } mutex_unlock(&deferred_probe_mutex); } @@ -211,6 +214,21 @@ void device_unblock_probing(void) driver_deferred_probe_trigger(); } +/* + * __deferred_probe_set_msg() - Set defer probe reason message for device + */ +void __deferred_probe_set_msg(const struct device *dev, struct va_format *vaf) +{ + const char *drv = dev_driver_string(dev); + + mutex_lock(&deferred_probe_mutex); + + kfree(dev->p->deferred_probe_msg); + dev->p->deferred_probe_msg = kasprintf(GFP_KERNEL, "%s: %pV", drv, vaf); + + mutex_unlock(&deferred_probe_mutex); +} + /* * deferred_devs_show() - Show the devices in the deferred probe pending list. */ @@ -221,7 +239,8 @@ static int deferred_devs_show(struct seq_file *s, void *data) mutex_lock(&deferred_probe_mutex); list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe) - seq_printf(s, "%s\n", dev_name(curr->device)); + seq_printf(s, "%s\t%s", dev_name(curr->device), + curr->device->p->deferred_probe_msg ?: "\n"); mutex_unlock(&deferred_probe_mutex); -- 2.17.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 2/5] driver core: add deferring probe reason to devices_deferred property 2020-06-24 11:41 ` [RESEND PATCH v5 2/5] driver core: add deferring probe reason to devices_deferred property Andrzej Hajda @ 2020-06-24 12:11 ` Rafael J. Wysocki 2020-06-24 13:28 ` Andrzej Hajda 2020-06-24 12:34 ` Greg Kroah-Hartman 1 sibling, 1 reply; 41+ messages in thread From: Rafael J. Wysocki @ 2020-06-24 12:11 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, Linux ARM, Marek Szyprowski On Wed, Jun 24, 2020 at 1:41 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> > --- > drivers/base/base.h | 3 +++ > drivers/base/core.c | 10 ++++++---- > drivers/base/dd.c | 21 ++++++++++++++++++++- > 3 files changed, 29 insertions(+), 5 deletions(-) > > diff --git a/drivers/base/base.h b/drivers/base/base.h > index 95c22c0f9036..93ef1c2f4c1f 100644 > --- a/drivers/base/base.h > +++ b/drivers/base/base.h > @@ -93,6 +93,7 @@ struct device_private { > struct klist_node knode_class; > struct list_head deferred_probe; > struct device_driver *async_driver; > + char *deferred_probe_msg; What about calling this deferred_probe_reason? > struct device *device; > u8 dead:1; > }; > @@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device *dev, > extern void driver_detach(struct device_driver *drv); > extern int driver_probe_device(struct device_driver *drv, struct device *dev); > extern void driver_deferred_probe_del(struct device *dev); > +extern void __deferred_probe_set_msg(const struct device *dev, > + struct va_format *vaf); I'd call this device_set_deferred_probe_reson() to follow the naming convention for the function names in this header file. > static inline int driver_match_device(struct device_driver *drv, > struct device *dev) > { > diff --git a/drivers/base/core.c b/drivers/base/core.c > index ee9da66bff1b..2a96954d5460 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -3962,6 +3962,8 @@ define_dev_printk_level(_dev_info, KERN_INFO); > * > * This helper implements common pattern present in probe functions for error > * checking: print message if the error is not -EPROBE_DEFER and propagate it. > + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked > + * later by reading devices_deferred debugfs attribute. > * It replaces code sequence: > * if (err != -EPROBE_DEFER) > * dev_err(dev, ...); > @@ -3977,14 +3979,14 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...) > struct va_format vaf; > va_list args; > > - if (err == -EPROBE_DEFER) > - return err; > - > va_start(args, fmt); > vaf.fmt = fmt; > vaf.va = &args; > > - dev_err(dev, "error %d: %pV", err, &vaf); > + if (err == -EPROBE_DEFER) > + __deferred_probe_set_msg(dev, &vaf); > + else > + dev_err(dev, "error %d: %pV", err, &vaf); > > va_end(args); > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index 9a1d940342ac..f44d26454b6a 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -27,6 +27,7 @@ > #include <linux/async.h> > #include <linux/pm_runtime.h> > #include <linux/pinctrl/devinfo.h> > +#include <linux/slab.h> > > #include "base.h" > #include "power/power.h" > @@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev) > if (!list_empty(&dev->p->deferred_probe)) { > dev_dbg(dev, "Removed from deferred list\n"); > list_del_init(&dev->p->deferred_probe); > + kfree(dev->p->deferred_probe_msg); > + dev->p->deferred_probe_msg = NULL; > } > mutex_unlock(&deferred_probe_mutex); > } > @@ -211,6 +214,21 @@ void device_unblock_probing(void) > driver_deferred_probe_trigger(); > } > > +/* > + * __deferred_probe_set_msg() - Set defer probe reason message for device I'd change this into a full kerneldoc comment. > + */ > +void __deferred_probe_set_msg(const struct device *dev, struct va_format *vaf) > +{ > + const char *drv = dev_driver_string(dev); > + > + mutex_lock(&deferred_probe_mutex); > + > + kfree(dev->p->deferred_probe_msg); > + dev->p->deferred_probe_msg = kasprintf(GFP_KERNEL, "%s: %pV", drv, vaf); > + > + mutex_unlock(&deferred_probe_mutex); > +} > + > /* > * deferred_devs_show() - Show the devices in the deferred probe pending list. > */ > @@ -221,7 +239,8 @@ static int deferred_devs_show(struct seq_file *s, void *data) > mutex_lock(&deferred_probe_mutex); > > list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe) > - seq_printf(s, "%s\n", dev_name(curr->device)); > + seq_printf(s, "%s\t%s", dev_name(curr->device), > + curr->device->p->deferred_probe_msg ?: "\n"); > > mutex_unlock(&deferred_probe_mutex); > > -- > 2.17.1 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 2/5] driver core: add deferring probe reason to devices_deferred property 2020-06-24 12:11 ` Rafael J. Wysocki @ 2020-06-24 13:28 ` Andrzej Hajda 0 siblings, 0 replies; 41+ messages in thread From: Andrzej Hajda @ 2020-06-24 13:28 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Andy Shevchenko, Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Neil Armstrong, Russell King - ARM Linux, open list:DRM DRIVERS, Linux Kernel Mailing List, Jonas Karlman, Mark Brown, Laurent Pinchart, Linux ARM, Marek Szyprowski On 24.06.2020 14:11, Rafael J. Wysocki wrote: > On Wed, Jun 24, 2020 at 1:41 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> >> --- >> drivers/base/base.h | 3 +++ >> drivers/base/core.c | 10 ++++++---- >> drivers/base/dd.c | 21 ++++++++++++++++++++- >> 3 files changed, 29 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/base/base.h b/drivers/base/base.h >> index 95c22c0f9036..93ef1c2f4c1f 100644 >> --- a/drivers/base/base.h >> +++ b/drivers/base/base.h >> @@ -93,6 +93,7 @@ struct device_private { >> struct klist_node knode_class; >> struct list_head deferred_probe; >> struct device_driver *async_driver; >> + char *deferred_probe_msg; > What about calling this deferred_probe_reason? > >> struct device *device; >> u8 dead:1; >> }; >> @@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device *dev, >> extern void driver_detach(struct device_driver *drv); >> extern int driver_probe_device(struct device_driver *drv, struct device *dev); >> extern void driver_deferred_probe_del(struct device *dev); >> +extern void __deferred_probe_set_msg(const struct device *dev, >> + struct va_format *vaf); > I'd call this device_set_deferred_probe_reson() to follow the naming > convention for the function names in this header file. > >> static inline int driver_match_device(struct device_driver *drv, >> struct device *dev) >> { >> diff --git a/drivers/base/core.c b/drivers/base/core.c >> index ee9da66bff1b..2a96954d5460 100644 >> --- a/drivers/base/core.c >> +++ b/drivers/base/core.c >> @@ -3962,6 +3962,8 @@ define_dev_printk_level(_dev_info, KERN_INFO); >> * >> * This helper implements common pattern present in probe functions for error >> * checking: print message if the error is not -EPROBE_DEFER and propagate it. >> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked >> + * later by reading devices_deferred debugfs attribute. >> * It replaces code sequence: >> * if (err != -EPROBE_DEFER) >> * dev_err(dev, ...); >> @@ -3977,14 +3979,14 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...) >> struct va_format vaf; >> va_list args; >> >> - if (err == -EPROBE_DEFER) >> - return err; >> - >> va_start(args, fmt); >> vaf.fmt = fmt; >> vaf.va = &args; >> >> - dev_err(dev, "error %d: %pV", err, &vaf); >> + if (err == -EPROBE_DEFER) >> + __deferred_probe_set_msg(dev, &vaf); >> + else >> + dev_err(dev, "error %d: %pV", err, &vaf); >> >> va_end(args); >> >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c >> index 9a1d940342ac..f44d26454b6a 100644 >> --- a/drivers/base/dd.c >> +++ b/drivers/base/dd.c >> @@ -27,6 +27,7 @@ >> #include <linux/async.h> >> #include <linux/pm_runtime.h> >> #include <linux/pinctrl/devinfo.h> >> +#include <linux/slab.h> >> >> #include "base.h" >> #include "power/power.h" >> @@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev) >> if (!list_empty(&dev->p->deferred_probe)) { >> dev_dbg(dev, "Removed from deferred list\n"); >> list_del_init(&dev->p->deferred_probe); >> + kfree(dev->p->deferred_probe_msg); >> + dev->p->deferred_probe_msg = NULL; >> } >> mutex_unlock(&deferred_probe_mutex); >> } >> @@ -211,6 +214,21 @@ void device_unblock_probing(void) >> driver_deferred_probe_trigger(); >> } >> >> +/* >> + * __deferred_probe_set_msg() - Set defer probe reason message for device > I'd change this into a full kerneldoc comment. OK I will apply all changes in next version. Thx for review. Regards Andrzej > >> + */ >> +void __deferred_probe_set_msg(const struct device *dev, struct va_format *vaf) >> +{ >> + const char *drv = dev_driver_string(dev); >> + >> + mutex_lock(&deferred_probe_mutex); >> + >> + kfree(dev->p->deferred_probe_msg); >> + dev->p->deferred_probe_msg = kasprintf(GFP_KERNEL, "%s: %pV", drv, vaf); >> + >> + mutex_unlock(&deferred_probe_mutex); >> +} >> + >> /* >> * deferred_devs_show() - Show the devices in the deferred probe pending list. >> */ >> @@ -221,7 +239,8 @@ static int deferred_devs_show(struct seq_file *s, void *data) >> mutex_lock(&deferred_probe_mutex); >> >> list_for_each_entry(curr, &deferred_probe_pending_list, deferred_probe) >> - seq_printf(s, "%s\n", dev_name(curr->device)); >> + seq_printf(s, "%s\t%s", dev_name(curr->device), >> + curr->device->p->deferred_probe_msg ?: "\n"); >> >> mutex_unlock(&deferred_probe_mutex); >> >> -- >> 2.17.1 >> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://protect2.fireeye.com/url?k=5adb7884-070fc5c0-5adaf3cb-0cc47a31381a-5588a624ab84213e&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 2/5] driver core: add deferring probe reason to devices_deferred property 2020-06-24 11:41 ` [RESEND PATCH v5 2/5] driver core: add deferring probe reason to devices_deferred property Andrzej Hajda 2020-06-24 12:11 ` Rafael J. Wysocki @ 2020-06-24 12:34 ` Greg Kroah-Hartman 2020-06-24 13:26 ` Andrzej Hajda 1 sibling, 1 reply; 41+ messages in thread From: Greg Kroah-Hartman @ 2020-06-24 12:34 UTC (permalink / raw) To: Andrzej Hajda Cc: Jernej Skrabec, Rafael J. Wysocki, Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Jonas Karlman, andy.shevchenko, Mark Brown, Laurent Pinchart, linux-arm-kernel, Marek Szyprowski On Wed, Jun 24, 2020 at 01:41:24PM +0200, Andrzej Hajda wrote: > /sys/kernel/debug/devices_deferred property contains list of deferred devices. > This list does not contain reason why the driver deferred probe, the patch > improves it. > The natural place to set the reason is probe_err function introduced recently, > ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message > will be attached to deferred device and printed when user read devices_deferred > property. > > Signed-off-by: Andrzej Hajda <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 | 10 ++++++---- > drivers/base/dd.c | 21 ++++++++++++++++++++- > 3 files changed, 29 insertions(+), 5 deletions(-) > > diff --git a/drivers/base/base.h b/drivers/base/base.h > index 95c22c0f9036..93ef1c2f4c1f 100644 > --- a/drivers/base/base.h > +++ b/drivers/base/base.h > @@ -93,6 +93,7 @@ struct device_private { > struct klist_node knode_class; > struct list_head deferred_probe; > struct device_driver *async_driver; > + char *deferred_probe_msg; > struct device *device; > u8 dead:1; > }; > @@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device *dev, > extern void driver_detach(struct device_driver *drv); > extern int driver_probe_device(struct device_driver *drv, struct device *dev); > extern void driver_deferred_probe_del(struct device *dev); > +extern void __deferred_probe_set_msg(const struct device *dev, > + struct va_format *vaf); > static inline int driver_match_device(struct device_driver *drv, > struct device *dev) > { > diff --git a/drivers/base/core.c b/drivers/base/core.c > index ee9da66bff1b..2a96954d5460 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -3962,6 +3962,8 @@ define_dev_printk_level(_dev_info, KERN_INFO); > * > * This helper implements common pattern present in probe functions for error > * checking: print message if the error is not -EPROBE_DEFER and propagate it. > + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked > + * later by reading devices_deferred debugfs attribute. > * It replaces code sequence: > * if (err != -EPROBE_DEFER) > * dev_err(dev, ...); > @@ -3977,14 +3979,14 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...) > struct va_format vaf; > va_list args; > > - if (err == -EPROBE_DEFER) > - return err; > - > va_start(args, fmt); > vaf.fmt = fmt; > vaf.va = &args; > > - dev_err(dev, "error %d: %pV", err, &vaf); > + if (err == -EPROBE_DEFER) > + __deferred_probe_set_msg(dev, &vaf); > + else > + dev_err(dev, "error %d: %pV", err, &vaf); > > va_end(args); > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index 9a1d940342ac..f44d26454b6a 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -27,6 +27,7 @@ > #include <linux/async.h> > #include <linux/pm_runtime.h> > #include <linux/pinctrl/devinfo.h> > +#include <linux/slab.h> > > #include "base.h" > #include "power/power.h" > @@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev) > if (!list_empty(&dev->p->deferred_probe)) { > dev_dbg(dev, "Removed from deferred list\n"); > list_del_init(&dev->p->deferred_probe); > + kfree(dev->p->deferred_probe_msg); > + dev->p->deferred_probe_msg = NULL; > } > mutex_unlock(&deferred_probe_mutex); > } > @@ -211,6 +214,21 @@ void device_unblock_probing(void) > driver_deferred_probe_trigger(); > } > > +/* > + * __deferred_probe_set_msg() - Set defer probe reason message for device > + */ > +void __deferred_probe_set_msg(const struct device *dev, struct va_format *vaf) > +{ > + const char *drv = dev_driver_string(dev); > + > + mutex_lock(&deferred_probe_mutex); > + > + kfree(dev->p->deferred_probe_msg); > + dev->p->deferred_probe_msg = kasprintf(GFP_KERNEL, "%s: %pV", drv, vaf); What about the device name? Don't you also want that? You want the same format that __dev_printk() outputs, please use that to be consistant with all other messages that drivers are spitting out. thanks, greg k-h _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 2/5] driver core: add deferring probe reason to devices_deferred property 2020-06-24 12:34 ` Greg Kroah-Hartman @ 2020-06-24 13:26 ` Andrzej Hajda 0 siblings, 0 replies; 41+ messages in thread From: Andrzej Hajda @ 2020-06-24 13:26 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: andy.shevchenko, Jernej Skrabec, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Russell King - ARM Linux, open list:DRM DRIVERS, linux-kernel, Neil Armstrong, Jonas Karlman, Mark Brown, Laurent Pinchart, linux-arm-kernel, Marek Szyprowski On 24.06.2020 14:34, Greg Kroah-Hartman wrote: > On Wed, Jun 24, 2020 at 01:41:24PM +0200, Andrzej Hajda wrote: >> /sys/kernel/debug/devices_deferred property contains list of deferred devices. >> This list does not contain reason why the driver deferred probe, the patch >> improves it. >> The natural place to set the reason is probe_err function introduced recently, >> ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message >> will be attached to deferred device and printed when user read devices_deferred >> property. >> >> Signed-off-by: Andrzej Hajda <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 | 10 ++++++---- >> drivers/base/dd.c | 21 ++++++++++++++++++++- >> 3 files changed, 29 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/base/base.h b/drivers/base/base.h >> index 95c22c0f9036..93ef1c2f4c1f 100644 >> --- a/drivers/base/base.h >> +++ b/drivers/base/base.h >> @@ -93,6 +93,7 @@ struct device_private { >> struct klist_node knode_class; >> struct list_head deferred_probe; >> struct device_driver *async_driver; >> + char *deferred_probe_msg; >> struct device *device; >> u8 dead:1; >> }; >> @@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device *dev, >> extern void driver_detach(struct device_driver *drv); >> extern int driver_probe_device(struct device_driver *drv, struct device *dev); >> extern void driver_deferred_probe_del(struct device *dev); >> +extern void __deferred_probe_set_msg(const struct device *dev, >> + struct va_format *vaf); >> static inline int driver_match_device(struct device_driver *drv, >> struct device *dev) >> { >> diff --git a/drivers/base/core.c b/drivers/base/core.c >> index ee9da66bff1b..2a96954d5460 100644 >> --- a/drivers/base/core.c >> +++ b/drivers/base/core.c >> @@ -3962,6 +3962,8 @@ define_dev_printk_level(_dev_info, KERN_INFO); >> * >> * This helper implements common pattern present in probe functions for error >> * checking: print message if the error is not -EPROBE_DEFER and propagate it. >> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked >> + * later by reading devices_deferred debugfs attribute. >> * It replaces code sequence: >> * if (err != -EPROBE_DEFER) >> * dev_err(dev, ...); >> @@ -3977,14 +3979,14 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...) >> struct va_format vaf; >> va_list args; >> >> - if (err == -EPROBE_DEFER) >> - return err; >> - >> va_start(args, fmt); >> vaf.fmt = fmt; >> vaf.va = &args; >> >> - dev_err(dev, "error %d: %pV", err, &vaf); >> + if (err == -EPROBE_DEFER) >> + __deferred_probe_set_msg(dev, &vaf); >> + else >> + dev_err(dev, "error %d: %pV", err, &vaf); >> >> va_end(args); >> >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c >> index 9a1d940342ac..f44d26454b6a 100644 >> --- a/drivers/base/dd.c >> +++ b/drivers/base/dd.c >> @@ -27,6 +27,7 @@ >> #include <linux/async.h> >> #include <linux/pm_runtime.h> >> #include <linux/pinctrl/devinfo.h> >> +#include <linux/slab.h> >> >> #include "base.h" >> #include "power/power.h" >> @@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev) >> if (!list_empty(&dev->p->deferred_probe)) { >> dev_dbg(dev, "Removed from deferred list\n"); >> list_del_init(&dev->p->deferred_probe); >> + kfree(dev->p->deferred_probe_msg); >> + dev->p->deferred_probe_msg = NULL; >> } >> mutex_unlock(&deferred_probe_mutex); >> } >> @@ -211,6 +214,21 @@ void device_unblock_probing(void) >> driver_deferred_probe_trigger(); >> } >> >> +/* >> + * __deferred_probe_set_msg() - Set defer probe reason message for device >> + */ >> +void __deferred_probe_set_msg(const struct device *dev, struct va_format *vaf) >> +{ >> + const char *drv = dev_driver_string(dev); >> + >> + mutex_lock(&deferred_probe_mutex); >> + >> + kfree(dev->p->deferred_probe_msg); >> + dev->p->deferred_probe_msg = kasprintf(GFP_KERNEL, "%s: %pV", drv, vaf); > What about the device name? Don't you also want that? deferred_devs_show prints it already, deferred_probe_msg is appended if not null. Regards Andrzej > > You want the same format that __dev_printk() outputs, please use that > to be consistant with all other messages that drivers are spitting out. > > thanks, > > greg k-h > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://protect2.fireeye.com/url?k=28daee95-7508f5cd-28db65da-0cc47a31c8b4-b3e8d1affcda9c08&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
[parent not found: <CGME20200624114136eucas1p1c84f81b1d78e2dbad7ac1b762f0a4b4f@eucas1p1.samsung.com>]
* [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types [not found] ` <CGME20200624114136eucas1p1c84f81b1d78e2dbad7ac1b762f0a4b4f@eucas1p1.samsung.com> @ 2020-06-24 11:41 ` Andrzej Hajda 2020-06-24 12:14 ` Rafael J. Wysocki ` (3 more replies) 0 siblings, 4 replies; 41+ messages in thread From: Andrzej Hajda @ 2020-06-24 11:41 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 Many resource acquisition functions return error value encapsulated in pointer instead of integer value. To simplify coding we can use macro which will accept both types of error. With this patch user can use: probe_err(dev, ptr, ...) instead of: probe_err(dev, PTR_ERR(ptr), ...) Without loosing old functionality: probe_err(dev, err, ...) Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> --- drivers/base/core.c | 25 ++----------------------- include/linux/device.h | 25 ++++++++++++++++++++++++- 2 files changed, 26 insertions(+), 24 deletions(-) diff --git a/drivers/base/core.c b/drivers/base/core.c index 2a96954d5460..df283c62d9c0 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -3953,28 +3953,7 @@ define_dev_printk_level(_dev_info, KERN_INFO); #endif -/** - * probe_err - probe error check and log helper - * @dev: the pointer to the struct device - * @err: error value to test - * @fmt: printf-style format string - * @...: arguments as specified in the format string - * - * This helper implements common pattern present in probe functions for error - * checking: print message if the error is not -EPROBE_DEFER and propagate it. - * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked - * later by reading devices_deferred debugfs attribute. - * It replaces code sequence: - * if (err != -EPROBE_DEFER) - * dev_err(dev, ...); - * return err; - * with - * return probe_err(dev, err, ...); - * - * Returns @err. - * - */ -int probe_err(const struct device *dev, int err, const char *fmt, ...) +int __probe_err(const struct device *dev, int err, const char *fmt, ...) { struct va_format vaf; va_list args; @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...) return err; } -EXPORT_SYMBOL_GPL(probe_err); +EXPORT_SYMBOL_GPL(__probe_err); static inline bool fwnode_is_primary(struct fwnode_handle *fwnode) { diff --git a/include/linux/device.h b/include/linux/device.h index 40a90d9bf799..22d3c3d4f461 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void); void device_links_supplier_sync_state_resume(void); extern __printf(3, 4) -int probe_err(const struct device *dev, int err, const char *fmt, ...); +int __probe_err(const struct device *dev, int err, const char *fmt, ...); + +/** + * probe_err - probe error check and log helper + * @dev: the pointer to the struct device + * @err: error value to test, can be integer or pointer type + * @fmt: printf-style format string + * @...: arguments as specified in the format string + * + * This helper implements common pattern present in probe functions for error + * checking: print message if the error is not -EPROBE_DEFER and propagate it. + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked + * later by reading devices_deferred debugfs attribute. + * It replaces code sequence: + * if (err != -EPROBE_DEFER) + * dev_err(dev, ...); + * return err; + * with + * return probe_err(dev, err, ...); + * + * Returns @err. + * + */ +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args) /* Create alias, so I can be autoloaded. */ #define MODULE_ALIAS_CHARDEV(major,minor) \ -- 2.17.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types 2020-06-24 11:41 ` [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types Andrzej Hajda @ 2020-06-24 12:14 ` Rafael J. Wysocki 2020-06-24 14:44 ` Andrzej Hajda 2020-06-24 12:30 ` Greg Kroah-Hartman ` (2 subsequent siblings) 3 siblings, 1 reply; 41+ messages in thread From: Rafael J. Wysocki @ 2020-06-24 12:14 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, Linux ARM, Marek Szyprowski On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > > Many resource acquisition functions return error value encapsulated in > pointer instead of integer value. To simplify coding we can use macro > which will accept both types of error. > With this patch user can use: > probe_err(dev, ptr, ...) > instead of: > probe_err(dev, PTR_ERR(ptr), ...) > Without loosing old functionality: > probe_err(dev, err, ...) > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> The separation of this change from patch [1/5] looks kind of artificial to me. You are introducing a new function anyway, so why not to make it what you want right away? > --- > drivers/base/core.c | 25 ++----------------------- > include/linux/device.h | 25 ++++++++++++++++++++++++- > 2 files changed, 26 insertions(+), 24 deletions(-) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 2a96954d5460..df283c62d9c0 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -3953,28 +3953,7 @@ define_dev_printk_level(_dev_info, KERN_INFO); > > #endif > > -/** > - * probe_err - probe error check and log helper > - * @dev: the pointer to the struct device > - * @err: error value to test > - * @fmt: printf-style format string > - * @...: arguments as specified in the format string > - * > - * This helper implements common pattern present in probe functions for error > - * checking: print message if the error is not -EPROBE_DEFER and propagate it. > - * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked > - * later by reading devices_deferred debugfs attribute. > - * It replaces code sequence: > - * if (err != -EPROBE_DEFER) > - * dev_err(dev, ...); > - * return err; > - * with > - * return probe_err(dev, err, ...); > - * > - * Returns @err. > - * > - */ > -int probe_err(const struct device *dev, int err, const char *fmt, ...) > +int __probe_err(const struct device *dev, int err, const char *fmt, ...) > { > struct va_format vaf; > va_list args; > @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...) > > return err; > } > -EXPORT_SYMBOL_GPL(probe_err); > +EXPORT_SYMBOL_GPL(__probe_err); > > static inline bool fwnode_is_primary(struct fwnode_handle *fwnode) > { > diff --git a/include/linux/device.h b/include/linux/device.h > index 40a90d9bf799..22d3c3d4f461 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void); > void device_links_supplier_sync_state_resume(void); > > extern __printf(3, 4) > -int probe_err(const struct device *dev, int err, const char *fmt, ...); > +int __probe_err(const struct device *dev, int err, const char *fmt, ...); > + > +/** > + * probe_err - probe error check and log helper > + * @dev: the pointer to the struct device > + * @err: error value to test, can be integer or pointer type > + * @fmt: printf-style format string > + * @...: arguments as specified in the format string > + * > + * This helper implements common pattern present in probe functions for error > + * checking: print message if the error is not -EPROBE_DEFER and propagate it. > + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked > + * later by reading devices_deferred debugfs attribute. > + * It replaces code sequence: > + * if (err != -EPROBE_DEFER) > + * dev_err(dev, ...); > + * return err; > + * with > + * return probe_err(dev, err, ...); > + * > + * Returns @err. > + * > + */ > +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args) > > /* Create alias, so I can be autoloaded. */ > #define MODULE_ALIAS_CHARDEV(major,minor) \ > -- > 2.17.1 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types 2020-06-24 12:14 ` Rafael J. Wysocki @ 2020-06-24 14:44 ` Andrzej Hajda 2020-06-24 14:55 ` Rafael J. Wysocki 0 siblings, 1 reply; 41+ messages in thread From: Andrzej Hajda @ 2020-06-24 14:44 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Andy Shevchenko, Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Neil Armstrong, Russell King - ARM Linux, open list:DRM DRIVERS, Linux Kernel Mailing List, Jonas Karlman, Mark Brown, Laurent Pinchart, Linux ARM, Marek Szyprowski On 24.06.2020 14:14, Rafael J. Wysocki wrote: > On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda <a.hajda@samsung.com> wrote: >> Many resource acquisition functions return error value encapsulated in >> pointer instead of integer value. To simplify coding we can use macro >> which will accept both types of error. >> With this patch user can use: >> probe_err(dev, ptr, ...) >> instead of: >> probe_err(dev, PTR_ERR(ptr), ...) >> Without loosing old functionality: >> probe_err(dev, err, ...) >> >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > The separation of this change from patch [1/5] looks kind of artificial to me. > > You are introducing a new function anyway, so why not to make it what > you want right away? Two reasons: 1.This patch is my recent idea, I didn't want to mix it with already reviewed code. 2. This patch could be treated hacky by some devs due to macro definition and type-casting. If both patches are OK I can merge them of course into one. Regards Andrzej > >> --- >> drivers/base/core.c | 25 ++----------------------- >> include/linux/device.h | 25 ++++++++++++++++++++++++- >> 2 files changed, 26 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/base/core.c b/drivers/base/core.c >> index 2a96954d5460..df283c62d9c0 100644 >> --- a/drivers/base/core.c >> +++ b/drivers/base/core.c >> @@ -3953,28 +3953,7 @@ define_dev_printk_level(_dev_info, KERN_INFO); >> >> #endif >> >> -/** >> - * probe_err - probe error check and log helper >> - * @dev: the pointer to the struct device >> - * @err: error value to test >> - * @fmt: printf-style format string >> - * @...: arguments as specified in the format string >> - * >> - * This helper implements common pattern present in probe functions for error >> - * checking: print message if the error is not -EPROBE_DEFER and propagate it. >> - * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked >> - * later by reading devices_deferred debugfs attribute. >> - * It replaces code sequence: >> - * if (err != -EPROBE_DEFER) >> - * dev_err(dev, ...); >> - * return err; >> - * with >> - * return probe_err(dev, err, ...); >> - * >> - * Returns @err. >> - * >> - */ >> -int probe_err(const struct device *dev, int err, const char *fmt, ...) >> +int __probe_err(const struct device *dev, int err, const char *fmt, ...) >> { >> struct va_format vaf; >> va_list args; >> @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...) >> >> return err; >> } >> -EXPORT_SYMBOL_GPL(probe_err); >> +EXPORT_SYMBOL_GPL(__probe_err); >> >> static inline bool fwnode_is_primary(struct fwnode_handle *fwnode) >> { >> diff --git a/include/linux/device.h b/include/linux/device.h >> index 40a90d9bf799..22d3c3d4f461 100644 >> --- a/include/linux/device.h >> +++ b/include/linux/device.h >> @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void); >> void device_links_supplier_sync_state_resume(void); >> >> extern __printf(3, 4) >> -int probe_err(const struct device *dev, int err, const char *fmt, ...); >> +int __probe_err(const struct device *dev, int err, const char *fmt, ...); >> + >> +/** >> + * probe_err - probe error check and log helper >> + * @dev: the pointer to the struct device >> + * @err: error value to test, can be integer or pointer type >> + * @fmt: printf-style format string >> + * @...: arguments as specified in the format string >> + * >> + * This helper implements common pattern present in probe functions for error >> + * checking: print message if the error is not -EPROBE_DEFER and propagate it. >> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked >> + * later by reading devices_deferred debugfs attribute. >> + * It replaces code sequence: >> + * if (err != -EPROBE_DEFER) >> + * dev_err(dev, ...); >> + * return err; >> + * with >> + * return probe_err(dev, err, ...); >> + * >> + * Returns @err. >> + * >> + */ >> +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args) >> >> /* Create alias, so I can be autoloaded. */ >> #define MODULE_ALIAS_CHARDEV(major,minor) \ >> -- >> 2.17.1 >> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://protect2.fireeye.com/url?k=fe383567-a3a29cc4-fe39be28-002590f5b904-1faeb9cf68e31587&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types 2020-06-24 14:44 ` Andrzej Hajda @ 2020-06-24 14:55 ` Rafael J. Wysocki 0 siblings, 0 replies; 41+ messages in thread From: Rafael J. Wysocki @ 2020-06-24 14:55 UTC (permalink / raw) To: Andrzej Hajda Cc: Andy Shevchenko, Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Rafael J. Wysocki, Linux Kernel Mailing List, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Jonas Karlman, Mark Brown, Laurent Pinchart, Linux ARM, Marek Szyprowski On Wed, Jun 24, 2020 at 4:44 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > > > On 24.06.2020 14:14, Rafael J. Wysocki wrote: > > On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > >> Many resource acquisition functions return error value encapsulated in > >> pointer instead of integer value. To simplify coding we can use macro > >> which will accept both types of error. > >> With this patch user can use: > >> probe_err(dev, ptr, ...) > >> instead of: > >> probe_err(dev, PTR_ERR(ptr), ...) > >> Without loosing old functionality: > >> probe_err(dev, err, ...) > >> > >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > > The separation of this change from patch [1/5] looks kind of artificial to me. > > > > You are introducing a new function anyway, so why not to make it what > > you want right away? > > > Two reasons: > > 1.This patch is my recent idea, I didn't want to mix it with already > reviewed code. > > 2. This patch could be treated hacky by some devs due to macro > definition and type-casting. Fair enough. There is some opposition against the $subject one, so I guess it may be dropped even. Thanks! _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types 2020-06-24 11:41 ` [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types Andrzej Hajda 2020-06-24 12:14 ` Rafael J. Wysocki @ 2020-06-24 12:30 ` Greg Kroah-Hartman 2020-06-24 14:48 ` Andrzej Hajda 2020-06-24 12:37 ` Robin Murphy 2020-06-24 12:53 ` Andy Shevchenko 3 siblings, 1 reply; 41+ messages in thread From: Greg Kroah-Hartman @ 2020-06-24 12:30 UTC (permalink / raw) To: Andrzej Hajda Cc: Jernej Skrabec, Rafael J. Wysocki, Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Jonas Karlman, andy.shevchenko, Mark Brown, Laurent Pinchart, linux-arm-kernel, Marek Szyprowski On Wed, Jun 24, 2020 at 01:41:25PM +0200, Andrzej Hajda wrote: > Many resource acquisition functions return error value encapsulated in > pointer instead of integer value. To simplify coding we can use macro > which will accept both types of error. > With this patch user can use: > probe_err(dev, ptr, ...) > instead of: > probe_err(dev, PTR_ERR(ptr), ...) > Without loosing old functionality: > probe_err(dev, err, ...) > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > --- > drivers/base/core.c | 25 ++----------------------- > include/linux/device.h | 25 ++++++++++++++++++++++++- > 2 files changed, 26 insertions(+), 24 deletions(-) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 2a96954d5460..df283c62d9c0 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -3953,28 +3953,7 @@ define_dev_printk_level(_dev_info, KERN_INFO); > > #endif > > -/** > - * probe_err - probe error check and log helper > - * @dev: the pointer to the struct device > - * @err: error value to test > - * @fmt: printf-style format string > - * @...: arguments as specified in the format string > - * > - * This helper implements common pattern present in probe functions for error > - * checking: print message if the error is not -EPROBE_DEFER and propagate it. > - * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked > - * later by reading devices_deferred debugfs attribute. > - * It replaces code sequence: > - * if (err != -EPROBE_DEFER) > - * dev_err(dev, ...); > - * return err; > - * with > - * return probe_err(dev, err, ...); > - * > - * Returns @err. > - * > - */ > -int probe_err(const struct device *dev, int err, const char *fmt, ...) > +int __probe_err(const struct device *dev, int err, const char *fmt, ...) > { > struct va_format vaf; > va_list args; > @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...) > > return err; > } > -EXPORT_SYMBOL_GPL(probe_err); > +EXPORT_SYMBOL_GPL(__probe_err); > > static inline bool fwnode_is_primary(struct fwnode_handle *fwnode) > { > diff --git a/include/linux/device.h b/include/linux/device.h > index 40a90d9bf799..22d3c3d4f461 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void); > void device_links_supplier_sync_state_resume(void); > > extern __printf(3, 4) > -int probe_err(const struct device *dev, int err, const char *fmt, ...); > +int __probe_err(const struct device *dev, int err, const char *fmt, ...); > + > +/** > + * probe_err - probe error check and log helper > + * @dev: the pointer to the struct device > + * @err: error value to test, can be integer or pointer type > + * @fmt: printf-style format string > + * @...: arguments as specified in the format string > + * > + * This helper implements common pattern present in probe functions for error > + * checking: print message if the error is not -EPROBE_DEFER and propagate it. > + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked > + * later by reading devices_deferred debugfs attribute. > + * It replaces code sequence: > + * if (err != -EPROBE_DEFER) > + * dev_err(dev, ...); > + * return err; > + * with > + * return probe_err(dev, err, ...); > + * > + * Returns @err. > + * > + */ > +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args) Shouldn't that be "unsigned long" instead of "long"? That's what we put pointers in last I looked... thanks, greg k-h _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types 2020-06-24 12:30 ` Greg Kroah-Hartman @ 2020-06-24 14:48 ` Andrzej Hajda 0 siblings, 0 replies; 41+ messages in thread From: Andrzej Hajda @ 2020-06-24 14:48 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: andy.shevchenko, Jernej Skrabec, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Russell King - ARM Linux, open list:DRM DRIVERS, linux-kernel, Neil Armstrong, Jonas Karlman, Mark Brown, Laurent Pinchart, linux-arm-kernel, Marek Szyprowski On 24.06.2020 14:30, Greg Kroah-Hartman wrote: > On Wed, Jun 24, 2020 at 01:41:25PM +0200, Andrzej Hajda wrote: >> Many resource acquisition functions return error value encapsulated in >> pointer instead of integer value. To simplify coding we can use macro >> which will accept both types of error. >> With this patch user can use: >> probe_err(dev, ptr, ...) >> instead of: >> probe_err(dev, PTR_ERR(ptr), ...) >> Without loosing old functionality: >> probe_err(dev, err, ...) >> >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >> --- >> drivers/base/core.c | 25 ++----------------------- >> include/linux/device.h | 25 ++++++++++++++++++++++++- >> 2 files changed, 26 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/base/core.c b/drivers/base/core.c >> index 2a96954d5460..df283c62d9c0 100644 >> --- a/drivers/base/core.c >> +++ b/drivers/base/core.c >> @@ -3953,28 +3953,7 @@ define_dev_printk_level(_dev_info, KERN_INFO); >> >> #endif >> >> -/** >> - * probe_err - probe error check and log helper >> - * @dev: the pointer to the struct device >> - * @err: error value to test >> - * @fmt: printf-style format string >> - * @...: arguments as specified in the format string >> - * >> - * This helper implements common pattern present in probe functions for error >> - * checking: print message if the error is not -EPROBE_DEFER and propagate it. >> - * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked >> - * later by reading devices_deferred debugfs attribute. >> - * It replaces code sequence: >> - * if (err != -EPROBE_DEFER) >> - * dev_err(dev, ...); >> - * return err; >> - * with >> - * return probe_err(dev, err, ...); >> - * >> - * Returns @err. >> - * >> - */ >> -int probe_err(const struct device *dev, int err, const char *fmt, ...) >> +int __probe_err(const struct device *dev, int err, const char *fmt, ...) >> { >> struct va_format vaf; >> va_list args; >> @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...) >> >> return err; >> } >> -EXPORT_SYMBOL_GPL(probe_err); >> +EXPORT_SYMBOL_GPL(__probe_err); >> >> static inline bool fwnode_is_primary(struct fwnode_handle *fwnode) >> { >> diff --git a/include/linux/device.h b/include/linux/device.h >> index 40a90d9bf799..22d3c3d4f461 100644 >> --- a/include/linux/device.h >> +++ b/include/linux/device.h >> @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void); >> void device_links_supplier_sync_state_resume(void); >> >> extern __printf(3, 4) >> -int probe_err(const struct device *dev, int err, const char *fmt, ...); >> +int __probe_err(const struct device *dev, int err, const char *fmt, ...); >> + >> +/** >> + * probe_err - probe error check and log helper >> + * @dev: the pointer to the struct device >> + * @err: error value to test, can be integer or pointer type >> + * @fmt: printf-style format string >> + * @...: arguments as specified in the format string >> + * >> + * This helper implements common pattern present in probe functions for error >> + * checking: print message if the error is not -EPROBE_DEFER and propagate it. >> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked >> + * later by reading devices_deferred debugfs attribute. >> + * It replaces code sequence: >> + * if (err != -EPROBE_DEFER) >> + * dev_err(dev, ...); >> + * return err; >> + * with >> + * return probe_err(dev, err, ...); >> + * >> + * Returns @err. >> + * >> + */ >> +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args) > Shouldn't that be "unsigned long" instead of "long"? That's what we put > pointers in last I looked... Unless we know this is error inside pointer, in such case we follow practice from PTR_ERR function. Regards Andrzej > > thanks, > > greg k-h > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://protect2.fireeye.com/url?k=75712e41-28bf2f92-7570a50e-000babff317b-a5a76e98e30aecc2&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types 2020-06-24 11:41 ` [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types Andrzej Hajda 2020-06-24 12:14 ` Rafael J. Wysocki 2020-06-24 12:30 ` Greg Kroah-Hartman @ 2020-06-24 12:37 ` Robin Murphy 2020-06-24 12:55 ` Andy Shevchenko 2020-06-24 13:29 ` Laurent Pinchart 2020-06-24 12:53 ` Andy Shevchenko 3 siblings, 2 replies; 41+ messages in thread From: Robin Murphy @ 2020-06-24 12:37 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, Rafael J. Wysocki, linux-arm-kernel, Marek Szyprowski On 2020-06-24 12:41, Andrzej Hajda wrote: > Many resource acquisition functions return error value encapsulated in > pointer instead of integer value. To simplify coding we can use macro > which will accept both types of error. > With this patch user can use: > probe_err(dev, ptr, ...) > instead of: > probe_err(dev, PTR_ERR(ptr), ...) > Without loosing old functionality: > probe_err(dev, err, ...) Personally I'm not convinced that simplification has much value, and I'd say it *does* have a significant downside. This: if (IS_ERR(x)) do_something_with(PTR_ERR(x)); is a familiar and expected pattern when reading/reviewing code, and at a glance is almost certainly doing the right thing. If I see this, on the other hand: if (IS_ERR(x)) do_something_with(x); my immediate instinct is to be suspicious, and now I've got to go off and double-check that if do_something_with() really expects a pointer it's also robust against PTR_ERR values. Off-hand I can't think of any APIs that work that way in the areas with which I'm familiar, so it would be a pretty unusual and non-obvious thing. Furthermore, an error helper that explicitly claims to accept "pointer type" values seems like it could easily lead to misunderstandings like this: int init_my_buffer(struct my_device *d) { d->buffer = kzalloc(d->buffer_size, GFP_KERNEL); return probe_err(d->dev, d->buffer, "failed to init buffer\n"); } and allowing that to compile without any hint of an error seems a little... unfair. Robin. > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > --- > drivers/base/core.c | 25 ++----------------------- > include/linux/device.h | 25 ++++++++++++++++++++++++- > 2 files changed, 26 insertions(+), 24 deletions(-) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 2a96954d5460..df283c62d9c0 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -3953,28 +3953,7 @@ define_dev_printk_level(_dev_info, KERN_INFO); > > #endif > > -/** > - * probe_err - probe error check and log helper > - * @dev: the pointer to the struct device > - * @err: error value to test > - * @fmt: printf-style format string > - * @...: arguments as specified in the format string > - * > - * This helper implements common pattern present in probe functions for error > - * checking: print message if the error is not -EPROBE_DEFER and propagate it. > - * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked > - * later by reading devices_deferred debugfs attribute. > - * It replaces code sequence: > - * if (err != -EPROBE_DEFER) > - * dev_err(dev, ...); > - * return err; > - * with > - * return probe_err(dev, err, ...); > - * > - * Returns @err. > - * > - */ > -int probe_err(const struct device *dev, int err, const char *fmt, ...) > +int __probe_err(const struct device *dev, int err, const char *fmt, ...) > { > struct va_format vaf; > va_list args; > @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...) > > return err; > } > -EXPORT_SYMBOL_GPL(probe_err); > +EXPORT_SYMBOL_GPL(__probe_err); > > static inline bool fwnode_is_primary(struct fwnode_handle *fwnode) > { > diff --git a/include/linux/device.h b/include/linux/device.h > index 40a90d9bf799..22d3c3d4f461 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void); > void device_links_supplier_sync_state_resume(void); > > extern __printf(3, 4) > -int probe_err(const struct device *dev, int err, const char *fmt, ...); > +int __probe_err(const struct device *dev, int err, const char *fmt, ...); > + > +/** > + * probe_err - probe error check and log helper > + * @dev: the pointer to the struct device > + * @err: error value to test, can be integer or pointer type > + * @fmt: printf-style format string > + * @...: arguments as specified in the format string > + * > + * This helper implements common pattern present in probe functions for error > + * checking: print message if the error is not -EPROBE_DEFER and propagate it. > + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked > + * later by reading devices_deferred debugfs attribute. > + * It replaces code sequence: > + * if (err != -EPROBE_DEFER) > + * dev_err(dev, ...); > + * return err; > + * with > + * return probe_err(dev, err, ...); > + * > + * Returns @err. > + * > + */ > +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args) > > /* Create alias, so I can be autoloaded. */ > #define MODULE_ALIAS_CHARDEV(major,minor) \ > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types 2020-06-24 12:37 ` Robin Murphy @ 2020-06-24 12:55 ` Andy Shevchenko 2020-06-24 14:25 ` Robin Murphy 2020-06-24 13:29 ` Laurent Pinchart 1 sibling, 1 reply; 41+ messages in thread From: Andy Shevchenko @ 2020-06-24 12:55 UTC (permalink / raw) To: Robin Murphy Cc: Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Rafael J. Wysocki, Linux Kernel Mailing List, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, Jonas Karlman, Mark Brown, Laurent Pinchart, linux-arm Mailing List, Marek Szyprowski On Wed, Jun 24, 2020 at 3:37 PM Robin Murphy <robin.murphy@arm.com> wrote: > On 2020-06-24 12:41, Andrzej Hajda wrote: > > Many resource acquisition functions return error value encapsulated in > > pointer instead of integer value. To simplify coding we can use macro > > which will accept both types of error. > > With this patch user can use: > > probe_err(dev, ptr, ...) > > instead of: > > probe_err(dev, PTR_ERR(ptr), ...) > > Without loosing old functionality: > > probe_err(dev, err, ...) > > Personally I'm not convinced that simplification has much value, and I'd > say it *does* have a significant downside. This: > > if (IS_ERR(x)) > do_something_with(PTR_ERR(x)); > > is a familiar and expected pattern when reading/reviewing code, and at a > glance is almost certainly doing the right thing. If I see this, on the > other hand: > > if (IS_ERR(x)) > do_something_with(x); I don't consider your arguments strong enough. You are appealing to one pattern vs. new coming *pattern* just with a different name and actually much less characters to parse. We have a lot of clean ups like this, have you protested against them? JFYI: they are now *established patterns* and saved a ton of LOCs in some of which even were typos. > my immediate instinct is to be suspicious, and now I've got to go off > and double-check that if do_something_with() really expects a pointer > it's also robust against PTR_ERR values. Off-hand I can't think of any > APIs that work that way in the areas with which I'm familiar, so it > would be a pretty unusual and non-obvious thing. -- With Best Regards, Andy Shevchenko _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types 2020-06-24 12:55 ` Andy Shevchenko @ 2020-06-24 14:25 ` Robin Murphy 2020-06-24 15:04 ` Mark Brown 0 siblings, 1 reply; 41+ messages in thread From: Robin Murphy @ 2020-06-24 14:25 UTC (permalink / raw) To: Andy Shevchenko Cc: Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Rafael J. Wysocki, Linux Kernel Mailing List, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, Jonas Karlman, Mark Brown, Laurent Pinchart, linux-arm Mailing List, Marek Szyprowski On 2020-06-24 13:55, Andy Shevchenko wrote: > On Wed, Jun 24, 2020 at 3:37 PM Robin Murphy <robin.murphy@arm.com> wrote: >> On 2020-06-24 12:41, Andrzej Hajda wrote: >>> Many resource acquisition functions return error value encapsulated in >>> pointer instead of integer value. To simplify coding we can use macro >>> which will accept both types of error. >>> With this patch user can use: >>> probe_err(dev, ptr, ...) >>> instead of: >>> probe_err(dev, PTR_ERR(ptr), ...) >>> Without loosing old functionality: >>> probe_err(dev, err, ...) >> >> Personally I'm not convinced that simplification has much value, and I'd >> say it *does* have a significant downside. This: >> >> if (IS_ERR(x)) >> do_something_with(PTR_ERR(x)); >> >> is a familiar and expected pattern when reading/reviewing code, and at a >> glance is almost certainly doing the right thing. If I see this, on the >> other hand: >> >> if (IS_ERR(x)) >> do_something_with(x); > > I don't consider your arguments strong enough. You are appealing to > one pattern vs. new coming *pattern* just with a different name and > actually much less characters to parse. We have a lot of clean ups > like this, have you protested against them? JFYI: they are now > *established patterns* and saved a ton of LOCs in some of which even > were typos. I'm not against probe_err() itself, I think that stands to be a wonderfully helpful thing that will eliminate a hell of a lot of ugly mess from drivers. It's only the specific elision of 9 characters worth of "PTR_ERR()" in certain cases that I'm questioning. I mean, we've already got +20 characters leeway now that checkpatch has acknowledged all that blank space on the right-hand side of all my editor windows. Sure, there's not necessarily anything bad about introducing a new pattern in itself, but it's not going to *replace* the existing pattern of "IS_ERR() pairs with PTR_ERR()", because IS_ERR() is used for more than driver probe error handling. Instead, everybody now has to bear in mind that the new pattern is "IS_ERR() pairs with PTR_ERR(), except sometimes when it doesn't - last time I looked only probe_err() was special, but maybe something's changed, I'll have to go check...", and that's just adding cognitive load for the sake of not even saving a linewrap any more. First, the wave of Sparse errors from the build bots hits because it turns out casting arbitrary pointers appropriately is hard. As it washes past, the reality of authors' and maintainers' personal preferences comes to bear... some inevitably prefer to keep spelling out PTR_ERR() in probe_err() calls for the sake of clarity, bikeshedding ensues, and the checkpatch and Coccinelle armies mobilise to send unwanted patches changing things back and forth. Eventually, in all the confusion, a synapse misfires in a muddled developer's mind, an ERR_PTR value passed to kfree() "because kfree() is robust" slips through, and a bug is born. And there's the thing: inconsistency breeds bugs. Sometimes, of course, there are really good reasons to be inconsistent. Is 9 characters per line for a few hundred lines across the kernel tree really a really good reason? The tersest code is not always the most maintainable. Consider that we could also save many more "tons of LoC" by rewriting an entire category of small if/else statements with ternaries. Would the overall effect on maintainability be positive? I don't think so. And yeah, anyone who pipes up suggesting that places where an ERR_PTR value could be passed to probe_err() could simply refactor IS_ERR() checks with more uses of the god-awful PTR_ERR_OR_ZERO() obfuscator gets a long stare of disapproval... Robin. >> my immediate instinct is to be suspicious, and now I've got to go off >> and double-check that if do_something_with() really expects a pointer >> it's also robust against PTR_ERR values. Off-hand I can't think of any >> APIs that work that way in the areas with which I'm familiar, so it >> would be a pretty unusual and non-obvious thing. > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types 2020-06-24 14:25 ` Robin Murphy @ 2020-06-24 15:04 ` Mark Brown 2020-06-24 15:16 ` Robin Murphy 0 siblings, 1 reply; 41+ messages in thread From: Mark Brown @ 2020-06-24 15:04 UTC (permalink / raw) To: Robin Murphy Cc: Jernej Skrabec, Rafael J. Wysocki, Greg Kroah-Hartman, Jonas Karlman, Linux Kernel Mailing List, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, Andy Shevchenko, Laurent Pinchart, Marek Szyprowski, linux-arm Mailing List, Bartlomiej Zolnierkiewicz [-- Attachment #1.1: Type: text/plain, Size: 453 bytes --] On Wed, Jun 24, 2020 at 03:25:33PM +0100, Robin Murphy wrote: > And yeah, anyone who pipes up suggesting that places where an ERR_PTR value > could be passed to probe_err() could simply refactor IS_ERR() checks with > more uses of the god-awful PTR_ERR_OR_ZERO() obfuscator gets a long stare of > disapproval... We could also have a probe_err_ptr() or something that took an ERR_PTR() instead if there really were an issue with explicitly doing this. [-- 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] 41+ messages in thread
* Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types 2020-06-24 15:04 ` Mark Brown @ 2020-06-24 15:16 ` Robin Murphy 2020-06-24 19:39 ` Andrzej Hajda 0 siblings, 1 reply; 41+ messages in thread From: Robin Murphy @ 2020-06-24 15:16 UTC (permalink / raw) To: Mark Brown Cc: Jernej Skrabec, Rafael J. Wysocki, Greg Kroah-Hartman, Jonas Karlman, Linux Kernel Mailing List, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, Andy Shevchenko, Laurent Pinchart, Marek Szyprowski, linux-arm Mailing List, Bartlomiej Zolnierkiewicz On 2020-06-24 16:04, Mark Brown wrote: > On Wed, Jun 24, 2020 at 03:25:33PM +0100, Robin Murphy wrote: > >> And yeah, anyone who pipes up suggesting that places where an ERR_PTR value >> could be passed to probe_err() could simply refactor IS_ERR() checks with >> more uses of the god-awful PTR_ERR_OR_ZERO() obfuscator gets a long stare of >> disapproval... > > We could also have a probe_err_ptr() or something that took an ERR_PTR() > instead if there really were an issue with explicitly doing this. Yeah, for all my lyrical objection, a static inline <blah>_ptr_err() helper to wrap <blah>_err() with sensible type checking might actually be an OK compromise if people really feel strongly for having that utility. (and then we can debate whether it should also convert NULL to -ENOMEM and !IS_ERR to 0... :D) Robin. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types 2020-06-24 15:16 ` Robin Murphy @ 2020-06-24 19:39 ` Andrzej Hajda 2020-06-25 8:41 ` Andy Shevchenko 0 siblings, 1 reply; 41+ messages in thread From: Andrzej Hajda @ 2020-06-24 19:39 UTC (permalink / raw) To: Robin Murphy, Mark Brown Cc: Jernej Skrabec, Jonas Karlman, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Rafael J. Wysocki, Linux Kernel Mailing List, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andy Shevchenko, Laurent Pinchart, linux-arm Mailing List, Marek Szyprowski On 24.06.2020 17:16, Robin Murphy wrote: > On 2020-06-24 16:04, Mark Brown wrote: >> On Wed, Jun 24, 2020 at 03:25:33PM +0100, Robin Murphy wrote: >> >>> And yeah, anyone who pipes up suggesting that places where an >>> ERR_PTR value >>> could be passed to probe_err() could simply refactor IS_ERR() checks >>> with >>> more uses of the god-awful PTR_ERR_OR_ZERO() obfuscator gets a long >>> stare of >>> disapproval... >> >> We could also have a probe_err_ptr() or something that took an ERR_PTR() >> instead if there really were an issue with explicitly doing this. > > Yeah, for all my lyrical objection, a static inline <blah>_ptr_err() > helper to wrap <blah>_err() with sensible type checking might actually > be an OK compromise if people really feel strongly for having that > utility. I have proposed such thing in my previous iteration[1], except it was macro because of variadic arguments. With current version we save 8 chars and hacky macro, with the old version we save only 4 chars and more clear construct - less tempting solution for me. Personally I prefer the current version - it does not seems to me more dangerous than all these PTR_ERR, IS_ERR,ERR_PTR helpers, but can prevent expression split across multiple lines due to 80char limit. Probably the simplest solution is to drop this patch, I will do it then. [1]: https://lwn.net/ml/linux-kernel/20181220102247.4911-4-a.hajda@samsung.com/ Regards Andrzej > > (and then we can debate whether it should also convert NULL to -ENOMEM > and !IS_ERR to 0... :D) > > Robin. > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://protect2.fireeye.com/url?k=074420c0-5ada8e5a-0745ab8f-0cc47a336fae-bba8bb4caf96e14d&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types 2020-06-24 19:39 ` Andrzej Hajda @ 2020-06-25 8:41 ` Andy Shevchenko 2020-06-25 10:19 ` Andrzej Hajda 0 siblings, 1 reply; 41+ messages in thread From: Andy Shevchenko @ 2020-06-25 8:41 UTC (permalink / raw) To: Andrzej Hajda Cc: Jernej Skrabec, Jonas Karlman, Greg Kroah-Hartman, Rafael J. Wysocki, Linux Kernel Mailing List, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Bartlomiej Zolnierkiewicz, Mark Brown, Laurent Pinchart, Robin Murphy, linux-arm Mailing List, Marek Szyprowski On Wed, Jun 24, 2020 at 10:40 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > On 24.06.2020 17:16, Robin Murphy wrote: ... > I have proposed such thing in my previous iteration[1], except it was > macro because of variadic arguments. You may have a function with variadic arguments. Macros are beasts and make in some cases more harm than help. -- With Best Regards, Andy Shevchenko _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types 2020-06-25 8:41 ` Andy Shevchenko @ 2020-06-25 10:19 ` Andrzej Hajda 0 siblings, 0 replies; 41+ messages in thread From: Andrzej Hajda @ 2020-06-25 10:19 UTC (permalink / raw) To: Andy Shevchenko Cc: Jernej Skrabec, Rafael J. Wysocki, Greg Kroah-Hartman, Jonas Karlman, Linux Kernel Mailing List, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Bartlomiej Zolnierkiewicz, Mark Brown, Laurent Pinchart, Robin Murphy, linux-arm Mailing List, Marek Szyprowski On 25.06.2020 10:41, Andy Shevchenko wrote: > On Wed, Jun 24, 2020 at 10:40 PM Andrzej Hajda <a.hajda@samsung.com> wrote: >> On 24.06.2020 17:16, Robin Murphy wrote: > ... > >> I have proposed such thing in my previous iteration[1], except it was >> macro because of variadic arguments. > You may have a function with variadic arguments. Macros are beasts and > make in some cases more harm than help. What harm it can do in this particular case? With macro we have simple straightforward one-liner, with quite good type-checking. Maybe I am wrong, but I suspect creation of variadic function would require much more coding. Regards Andrzej _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types 2020-06-24 12:37 ` Robin Murphy 2020-06-24 12:55 ` Andy Shevchenko @ 2020-06-24 13:29 ` Laurent Pinchart 1 sibling, 0 replies; 41+ messages in thread From: Laurent Pinchart @ 2020-06-24 13:29 UTC (permalink / raw) To: Robin Murphy Cc: andy.shevchenko, Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Andrzej Hajda, Jonas Karlman, Mark Brown, linux-arm-kernel, Marek Szyprowski On Wed, Jun 24, 2020 at 01:37:52PM +0100, Robin Murphy wrote: > On 2020-06-24 12:41, Andrzej Hajda wrote: > > Many resource acquisition functions return error value encapsulated in > > pointer instead of integer value. To simplify coding we can use macro > > which will accept both types of error. > > With this patch user can use: > > probe_err(dev, ptr, ...) > > instead of: > > probe_err(dev, PTR_ERR(ptr), ...) > > Without loosing old functionality: > > probe_err(dev, err, ...) > > Personally I'm not convinced that simplification has much value, and I'd > say it *does* have a significant downside. This: > > if (IS_ERR(x)) > do_something_with(PTR_ERR(x)); > > is a familiar and expected pattern when reading/reviewing code, and at a > glance is almost certainly doing the right thing. If I see this, on the > other hand: > > if (IS_ERR(x)) > do_something_with(x); > > my immediate instinct is to be suspicious, and now I've got to go off > and double-check that if do_something_with() really expects a pointer > it's also robust against PTR_ERR values. Off-hand I can't think of any > APIs that work that way in the areas with which I'm familiar, so it > would be a pretty unusual and non-obvious thing. I second this. Furthermore, the hidden cast to long means that we'll leak pointer values if one happens to pass a real pointer to this function. > Furthermore, an error helper that explicitly claims to accept "pointer > type" values seems like it could easily lead to misunderstandings like this: > > int init_my_buffer(struct my_device *d) > { > d->buffer = kzalloc(d->buffer_size, GFP_KERNEL); > return probe_err(d->dev, d->buffer, "failed to init buffer\n"); > } > > and allowing that to compile without any hint of an error seems a > little... unfair. > > Robin. > > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > > --- > > drivers/base/core.c | 25 ++----------------------- > > include/linux/device.h | 25 ++++++++++++++++++++++++- > > 2 files changed, 26 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c > > index 2a96954d5460..df283c62d9c0 100644 > > --- a/drivers/base/core.c > > +++ b/drivers/base/core.c > > @@ -3953,28 +3953,7 @@ define_dev_printk_level(_dev_info, KERN_INFO); > > > > #endif > > > > -/** > > - * probe_err - probe error check and log helper > > - * @dev: the pointer to the struct device > > - * @err: error value to test > > - * @fmt: printf-style format string > > - * @...: arguments as specified in the format string > > - * > > - * This helper implements common pattern present in probe functions for error > > - * checking: print message if the error is not -EPROBE_DEFER and propagate it. > > - * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked > > - * later by reading devices_deferred debugfs attribute. > > - * It replaces code sequence: > > - * if (err != -EPROBE_DEFER) > > - * dev_err(dev, ...); > > - * return err; > > - * with > > - * return probe_err(dev, err, ...); > > - * > > - * Returns @err. > > - * > > - */ > > -int probe_err(const struct device *dev, int err, const char *fmt, ...) > > +int __probe_err(const struct device *dev, int err, const char *fmt, ...) > > { > > struct va_format vaf; > > va_list args; > > @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const char *fmt, ...) > > > > return err; > > } > > -EXPORT_SYMBOL_GPL(probe_err); > > +EXPORT_SYMBOL_GPL(__probe_err); > > > > static inline bool fwnode_is_primary(struct fwnode_handle *fwnode) > > { > > diff --git a/include/linux/device.h b/include/linux/device.h > > index 40a90d9bf799..22d3c3d4f461 100644 > > --- a/include/linux/device.h > > +++ b/include/linux/device.h > > @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void); > > void device_links_supplier_sync_state_resume(void); > > > > extern __printf(3, 4) > > -int probe_err(const struct device *dev, int err, const char *fmt, ...); > > +int __probe_err(const struct device *dev, int err, const char *fmt, ...); > > + > > +/** > > + * probe_err - probe error check and log helper > > + * @dev: the pointer to the struct device > > + * @err: error value to test, can be integer or pointer type > > + * @fmt: printf-style format string > > + * @...: arguments as specified in the format string > > + * > > + * This helper implements common pattern present in probe functions for error > > + * checking: print message if the error is not -EPROBE_DEFER and propagate it. > > + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked > > + * later by reading devices_deferred debugfs attribute. > > + * It replaces code sequence: > > + * if (err != -EPROBE_DEFER) > > + * dev_err(dev, ...); > > + * return err; > > + * with > > + * return probe_err(dev, err, ...); > > + * > > + * Returns @err. > > + * > > + */ > > +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args) > > > > /* Create alias, so I can be autoloaded. */ > > #define MODULE_ALIAS_CHARDEV(major,minor) \ > > -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types 2020-06-24 11:41 ` [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types Andrzej Hajda ` (2 preceding siblings ...) 2020-06-24 12:37 ` Robin Murphy @ 2020-06-24 12:53 ` Andy Shevchenko 2020-06-24 13:12 ` Andrzej Hajda 3 siblings, 1 reply; 41+ messages in thread From: Andy Shevchenko @ 2020-06-24 12:53 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 Wed, Jun 24, 2020 at 2:41 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > > Many resource acquisition functions return error value encapsulated in > pointer instead of integer value. To simplify coding we can use macro > which will accept both types of error. > With this patch user can use: > probe_err(dev, ptr, ...) > instead of: > probe_err(dev, PTR_ERR(ptr), ...) > Without loosing old functionality: > probe_err(dev, err, ...) ... > + * This helper implements common pattern present in probe functions for error > + * checking: print message if the error is not -EPROBE_DEFER and propagate it. > + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked > + * later by reading devices_deferred debugfs attribute. > + * It replaces code sequence: > + * if (err != -EPROBE_DEFER) > + * dev_err(dev, ...); Btw, we have now %pe. Can you consider to use it? > + * return err; > + * with > + * return probe_err(dev, err, ...); > + * > + * Returns @err. > + * > + */ > +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args) Can't we use PTR_ERR() here? -- With Best Regards, Andy Shevchenko _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types 2020-06-24 12:53 ` Andy Shevchenko @ 2020-06-24 13:12 ` Andrzej Hajda 0 siblings, 0 replies; 41+ messages in thread From: Andrzej Hajda @ 2020-06-24 13:12 UTC (permalink / raw) To: Andy Shevchenko Cc: Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Rafael J. Wysocki, Linux Kernel Mailing List, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Jonas Karlman, Mark Brown, Laurent Pinchart, linux-arm Mailing List, Marek Szyprowski On 24.06.2020 14:53, Andy Shevchenko wrote: > On Wed, Jun 24, 2020 at 2:41 PM Andrzej Hajda <a.hajda@samsung.com> wrote: >> Many resource acquisition functions return error value encapsulated in >> pointer instead of integer value. To simplify coding we can use macro >> which will accept both types of error. >> With this patch user can use: >> probe_err(dev, ptr, ...) >> instead of: >> probe_err(dev, PTR_ERR(ptr), ...) >> Without loosing old functionality: >> probe_err(dev, err, ...) > ... > >> + * This helper implements common pattern present in probe functions for error >> + * checking: print message if the error is not -EPROBE_DEFER and propagate it. >> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked >> + * later by reading devices_deferred debugfs attribute. >> + * It replaces code sequence: >> + * if (err != -EPROBE_DEFER) >> + * dev_err(dev, ...); > Btw, we have now %pe. Can you consider to use it? OK, I haven't noticed it finally appeared. > >> + * return err; >> + * with >> + * return probe_err(dev, err, ...); >> + * >> + * Returns @err. >> + * >> + */ >> +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args) > Can't we use PTR_ERR() here? Nope, I want to accept both types: int and pointer. Regards Andrzej > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
[parent not found: <CGME20200624114137eucas1p13599d33a0c4a9abf7708bf8c8e77264b@eucas1p1.samsung.com>]
* [RESEND PATCH v5 4/5] drm/bridge/sii8620: fix resource acquisition error handling [not found] ` <CGME20200624114137eucas1p13599d33a0c4a9abf7708bf8c8e77264b@eucas1p1.samsung.com> @ 2020-06-24 11:41 ` Andrzej Hajda 2020-06-24 13:25 ` Mark Brown 0 siblings, 1 reply; 41+ messages in thread From: Andrzej Hajda @ 2020-06-24 11:41 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 probe_err 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 | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c index 92acd336aa89..2f825b2d0098 100644 --- a/drivers/gpu/drm/bridge/sil-sii8620.c +++ b/drivers/gpu/drm/bridge/sil-sii8620.c @@ -2299,10 +2299,8 @@ static int sii8620_probe(struct i2c_client *client, INIT_LIST_HEAD(&ctx->mt_queue); ctx->clk_xtal = devm_clk_get(dev, "xtal"); - if (IS_ERR(ctx->clk_xtal)) { - dev_err(dev, "failed to get xtal clock from DT\n"); - return PTR_ERR(ctx->clk_xtal); - } + if (IS_ERR(ctx->clk_xtal)) + return probe_err(dev, ctx->clk_xtal, "failed to get xtal clock from DT\n"); if (!client->irq) { dev_err(dev, "no irq provided\n"); @@ -2313,16 +2311,12 @@ static int sii8620_probe(struct i2c_client *client, sii8620_irq_thread, IRQF_TRIGGER_HIGH | IRQF_ONESHOT, "sii8620", ctx); - if (ret < 0) { - dev_err(dev, "failed to install IRQ handler\n"); - return ret; - } + if (ret < 0) + return probe_err(dev, ret, "failed to install IRQ handler\n"); ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); - if (IS_ERR(ctx->gpio_reset)) { - dev_err(dev, "failed to get reset gpio from DT\n"); - return PTR_ERR(ctx->gpio_reset); - } + if (IS_ERR(ctx->gpio_reset)) + return probe_err(dev, ctx->gpio_reset, "failed to get reset gpio from DT\n"); ctx->supplies[0].supply = "cvcc10"; ctx->supplies[1].supply = "iovcc18"; -- 2.17.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 4/5] drm/bridge/sii8620: fix resource acquisition error handling 2020-06-24 11:41 ` [RESEND PATCH v5 4/5] drm/bridge/sii8620: fix resource acquisition error handling Andrzej Hajda @ 2020-06-24 13:25 ` Mark Brown 2020-06-24 13:43 ` Andrzej Hajda 0 siblings, 1 reply; 41+ messages in thread From: Mark Brown @ 2020-06-24 13:25 UTC (permalink / raw) To: Andrzej Hajda Cc: Jernej Skrabec, Rafael J. Wysocki, Greg Kroah-Hartman, Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Jonas Karlman, andy.shevchenko, Laurent Pinchart, linux-arm-kernel, Marek Szyprowski [-- Attachment #1.1: Type: text/plain, Size: 579 bytes --] On Wed, Jun 24, 2020 at 01:41:26PM +0200, Andrzej Hajda wrote: > In case of error during resource acquisition driver should print error > message only in case it is not deferred probe, using probe_err helper > solves the issue. Moreover it records defer probe reason for debugging. If we silently ignore all deferred probe errors we make it hard for anyone who is experiencing issues with deferred probe to figure out what they're missing. We should at least be logging problems at debug level so there's something for people to go on without having to hack the kernel source. [-- 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] 41+ messages in thread
* Re: [RESEND PATCH v5 4/5] drm/bridge/sii8620: fix resource acquisition error handling 2020-06-24 13:25 ` Mark Brown @ 2020-06-24 13:43 ` Andrzej Hajda 2020-06-24 14:11 ` Mark Brown 0 siblings, 1 reply; 41+ messages in thread From: Andrzej Hajda @ 2020-06-24 13:43 UTC (permalink / raw) To: Mark Brown Cc: andy.shevchenko, Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Jonas Karlman, Laurent Pinchart, linux-arm-kernel, Marek Szyprowski [-- Attachment #1.1: Type: text/plain, Size: 1191 bytes --] On 24.06.2020 15:25, Mark Brown wrote: > On Wed, Jun 24, 2020 at 01:41:26PM +0200, Andrzej Hajda wrote: >> In case of error during resource acquisition driver should print error >> message only in case it is not deferred probe, using probe_err helper >> solves the issue. Moreover it records defer probe reason for debugging. > If we silently ignore all deferred probe errors we make it hard for > anyone who is experiencing issues with deferred probe to figure out what > they're missing. We should at least be logging problems at debug level > so there's something for people to go on without having to hack the > kernel source. But you can always do: cat /sys/kernel/debug/devices_deferred And you will find there deferred probe reason (thanks to patch 2/5). Eventually if you want it in dmesg anyway, one can adjust probe_err function to log probe error on debug level as well. Regards Andrzej > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://protect2.fireeye.com/url?k=ef34cf7b-b2ff4845-ef354434-0cc47a31309a-305676031d9eb553&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel [-- Attachment #1.2.1: Type: text/html, Size: 3061 bytes --] [-- Attachment #1.2.2: Type: image/gif, Size: 13168 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] 41+ messages in thread
* Re: [RESEND PATCH v5 4/5] drm/bridge/sii8620: fix resource acquisition error handling 2020-06-24 13:43 ` Andrzej Hajda @ 2020-06-24 14:11 ` Mark Brown 0 siblings, 0 replies; 41+ messages in thread From: Mark Brown @ 2020-06-24 14:11 UTC (permalink / raw) To: Andrzej Hajda Cc: andy.shevchenko, Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Jonas Karlman, Laurent Pinchart, linux-arm-kernel, Marek Szyprowski [-- Attachment #1.1: Type: text/plain, Size: 979 bytes --] On Wed, Jun 24, 2020 at 03:43:10PM +0200, Andrzej Hajda wrote: > > On 24.06.2020 15:25, Mark Brown wrote: > > If we silently ignore all deferred probe errors we make it hard for > > anyone who is experiencing issues with deferred probe to figure out what > > they're missing. We should at least be logging problems at debug level > > so there's something for people to go on without having to hack the > > kernel source. > But you can always do: > cat /sys/kernel/debug/devices_deferred > And you will find there deferred probe reason (thanks to patch 2/5). Right, my point is more that we shouldn't be promoting discarding the diagnostics entirely but rather saying that we want to redirect those somewhere else. > Eventually if you want it in dmesg anyway, one can adjust probe_err function > to log probe error on debug level as well. That would most likely be very useful as a boot option for problems that occur before we get a console up. [-- 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] 41+ messages in thread
[parent not found: <CGME20200624114138eucas1p262505da3ad1067720080d20209ff32de@eucas1p2.samsung.com>]
* [RESEND PATCH v5 5/5] drm/bridge: lvds-codec: simplify error handling code [not found] ` <CGME20200624114138eucas1p262505da3ad1067720080d20209ff32de@eucas1p2.samsung.com> @ 2020-06-24 11:41 ` Andrzej Hajda 2020-06-24 13:33 ` Laurent Pinchart 0 siblings, 1 reply; 41+ messages in thread From: Andrzej Hajda @ 2020-06-24 11:41 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 probe_err 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 | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c index 24fb1befdfa2..c76fa0239e39 100644 --- a/drivers/gpu/drm/bridge/lvds-codec.c +++ b/drivers/gpu/drm/bridge/lvds-codec.c @@ -71,13 +71,8 @@ static int lvds_codec_probe(struct platform_device *pdev) lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev); lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", GPIOD_OUT_HIGH); - if (IS_ERR(lvds_codec->powerdown_gpio)) { - int err = PTR_ERR(lvds_codec->powerdown_gpio); - - if (err != -EPROBE_DEFER) - dev_err(dev, "powerdown GPIO failure: %d\n", err); - return err; - } + if (IS_ERR(lvds_codec->powerdown_gpio)) + return probe_err(dev, lvds_codec->powerdown_gpio, "powerdown GPIO failure\n"); /* Locate the panel DT node. */ panel_node = of_graph_get_remote_node(dev->of_node, 1, 0); -- 2.17.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 5/5] drm/bridge: lvds-codec: simplify error handling code 2020-06-24 11:41 ` [RESEND PATCH v5 5/5] drm/bridge: lvds-codec: simplify error handling code Andrzej Hajda @ 2020-06-24 13:33 ` Laurent Pinchart 2020-06-24 14:03 ` Andrzej Hajda 0 siblings, 1 reply; 41+ messages in thread From: Laurent Pinchart @ 2020-06-24 13:33 UTC (permalink / raw) To: Andrzej Hajda Cc: Jernej Skrabec, Rafael J. Wysocki, Greg Kroah-Hartman, Bartlomiej Zolnierkiewicz, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Jonas Karlman, andy.shevchenko, Mark Brown, linux-arm-kernel, Marek Szyprowski Hi Andrzej, On Wed, Jun 24, 2020 at 01:41:27PM +0200, Andrzej Hajda wrote: > Using probe_err code has following advantages: > - shorter code, > - recorded defer probe reason for debugging, > - uniform error code logging. > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > --- > drivers/gpu/drm/bridge/lvds-codec.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > index 24fb1befdfa2..c76fa0239e39 100644 > --- a/drivers/gpu/drm/bridge/lvds-codec.c > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > @@ -71,13 +71,8 @@ static int lvds_codec_probe(struct platform_device *pdev) > lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev); > lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", > GPIOD_OUT_HIGH); > - if (IS_ERR(lvds_codec->powerdown_gpio)) { > - int err = PTR_ERR(lvds_codec->powerdown_gpio); > - > - if (err != -EPROBE_DEFER) > - dev_err(dev, "powerdown GPIO failure: %d\n", err); > - return err; > - } > + if (IS_ERR(lvds_codec->powerdown_gpio)) > + return probe_err(dev, lvds_codec->powerdown_gpio, "powerdown GPIO failure\n"); Line wrap please. It bothers me that the common pattern of writing the error code at the end of the message isn't possible anymore. Maybe I'll get used to it, but it removes some flexibility. > > /* Locate the panel DT node. */ > panel_node = of_graph_get_remote_node(dev->of_node, 1, 0); -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 5/5] drm/bridge: lvds-codec: simplify error handling code 2020-06-24 13:33 ` Laurent Pinchart @ 2020-06-24 14:03 ` Andrzej Hajda 2020-06-24 21:18 ` Laurent Pinchart 0 siblings, 1 reply; 41+ messages in thread From: Andrzej Hajda @ 2020-06-24 14:03 UTC (permalink / raw) To: Laurent Pinchart Cc: andy.shevchenko, Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Jonas Karlman, Mark Brown, linux-arm-kernel, Marek Szyprowski On 24.06.2020 15:33, Laurent Pinchart wrote: > Hi Andrzej, > > On Wed, Jun 24, 2020 at 01:41:27PM +0200, Andrzej Hajda wrote: >> Using probe_err code has following advantages: >> - shorter code, >> - recorded defer probe reason for debugging, >> - uniform error code logging. >> >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >> --- >> drivers/gpu/drm/bridge/lvds-codec.c | 9 ++------- >> 1 file changed, 2 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c >> index 24fb1befdfa2..c76fa0239e39 100644 >> --- a/drivers/gpu/drm/bridge/lvds-codec.c >> +++ b/drivers/gpu/drm/bridge/lvds-codec.c >> @@ -71,13 +71,8 @@ static int lvds_codec_probe(struct platform_device *pdev) >> lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev); >> lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", >> GPIOD_OUT_HIGH); >> - if (IS_ERR(lvds_codec->powerdown_gpio)) { >> - int err = PTR_ERR(lvds_codec->powerdown_gpio); >> - >> - if (err != -EPROBE_DEFER) >> - dev_err(dev, "powerdown GPIO failure: %d\n", err); >> - return err; >> - } >> + if (IS_ERR(lvds_codec->powerdown_gpio)) >> + return probe_err(dev, lvds_codec->powerdown_gpio, "powerdown GPIO failure\n"); > Line wrap please. I hoped that with latest checkpatch line length limit increase from 80 to 100 it is acceptable :) but apparently not [1]. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144 > > It bothers me that the common pattern of writing the error code at the > end of the message isn't possible anymore. Maybe I'll get used to it, > but it removes some flexibility. Yes, but it gives uniformity :) and now with %pe printk format it changes anyway. Regards Andrzej > >> >> /* Locate the panel DT node. */ >> panel_node = of_graph_get_remote_node(dev->of_node, 1, 0); _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RESEND PATCH v5 5/5] drm/bridge: lvds-codec: simplify error handling code 2020-06-24 14:03 ` Andrzej Hajda @ 2020-06-24 21:18 ` Laurent Pinchart 0 siblings, 0 replies; 41+ messages in thread From: Laurent Pinchart @ 2020-06-24 21:18 UTC (permalink / raw) To: Andrzej Hajda Cc: andy.shevchenko, Jernej Skrabec, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel, open list:DRM DRIVERS, Russell King - ARM Linux, Neil Armstrong, Jonas Karlman, Mark Brown, linux-arm-kernel, Marek Szyprowski Hi Andrzej, On Wed, Jun 24, 2020 at 04:03:30PM +0200, Andrzej Hajda wrote: > On 24.06.2020 15:33, Laurent Pinchart wrote: > > On Wed, Jun 24, 2020 at 01:41:27PM +0200, Andrzej Hajda wrote: > >> Using probe_err code has following advantages: > >> - shorter code, > >> - recorded defer probe reason for debugging, > >> - uniform error code logging. > >> > >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > >> --- > >> drivers/gpu/drm/bridge/lvds-codec.c | 9 ++------- > >> 1 file changed, 2 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > >> index 24fb1befdfa2..c76fa0239e39 100644 > >> --- a/drivers/gpu/drm/bridge/lvds-codec.c > >> +++ b/drivers/gpu/drm/bridge/lvds-codec.c > >> @@ -71,13 +71,8 @@ static int lvds_codec_probe(struct platform_device *pdev) > >> lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev); > >> lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", > >> GPIOD_OUT_HIGH); > >> - if (IS_ERR(lvds_codec->powerdown_gpio)) { > >> - int err = PTR_ERR(lvds_codec->powerdown_gpio); > >> - > >> - if (err != -EPROBE_DEFER) > >> - dev_err(dev, "powerdown GPIO failure: %d\n", err); > >> - return err; > >> - } > >> + if (IS_ERR(lvds_codec->powerdown_gpio)) > >> + return probe_err(dev, lvds_codec->powerdown_gpio, "powerdown GPIO failure\n"); > > > > Line wrap please. > > I hoped that with latest checkpatch line length limit increase from 80 > to 100 it is acceptable :) but apparently not [1]. I'm all for using longer lines when that improves readability, but in this case I think it's easy enough to split the line. A longer line limit doesn't mean we're forced to generate longer lines :-) On a side note, I've been working on a C++ userspace project where we had to decide on a coding style. Line length was one parameter, and we went for a soft limit of 80 columns, and a hard limit of 120 columns. This works quite well so far. The only pain point is that clang-format (we use it, wrapped in a python script, to detect coding style violations) doesn't understand soft and hard limits for line lengths. > [1]: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144 > > > It bothers me that the common pattern of writing the error code at the > > end of the message isn't possible anymore. Maybe I'll get used to it, > > but it removes some flexibility. > > Yes, but it gives uniformity :) and now with %pe printk format it > changes anyway. > > >> /* Locate the panel DT node. */ > >> panel_node = of_graph_get_remote_node(dev->of_node, 1, 0); -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2020-06-25 10:19 UTC | newest] Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20200624114134eucas1p2799e0ae76fcb026a0e4bcccc2026b732@eucas1p2.samsung.com> 2020-06-24 11:41 ` [RESEND PATCH v5 0/5] driver core: add probe error check helper Andrzej Hajda [not found] ` <CGME20200624114135eucas1p26e2e4683d60cebdce7acd55177013992@eucas1p2.samsung.com> 2020-06-24 11:41 ` [RESEND PATCH v5 1/5] driver core: add probe_err log helper Andrzej Hajda 2020-06-24 11:52 ` Rafael J. Wysocki 2020-06-24 12:31 ` Greg Kroah-Hartman 2020-06-24 13:23 ` Laurent Pinchart 2020-06-24 14:04 ` Andrzej Hajda 2020-06-24 13:27 ` Mark Brown 2020-06-24 13:45 ` Andy Shevchenko 2020-06-24 14:02 ` Mark Brown 2020-06-24 15:00 ` Robin Murphy 2020-06-24 15:28 ` Mark Brown [not found] ` <CGME20200624114136eucas1p1a3a31d95d86754201c7965f26ccd5de0@eucas1p1.samsung.com> 2020-06-24 11:41 ` [RESEND PATCH v5 2/5] driver core: add deferring probe reason to devices_deferred property Andrzej Hajda 2020-06-24 12:11 ` Rafael J. Wysocki 2020-06-24 13:28 ` Andrzej Hajda 2020-06-24 12:34 ` Greg Kroah-Hartman 2020-06-24 13:26 ` Andrzej Hajda [not found] ` <CGME20200624114136eucas1p1c84f81b1d78e2dbad7ac1b762f0a4b4f@eucas1p1.samsung.com> 2020-06-24 11:41 ` [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types Andrzej Hajda 2020-06-24 12:14 ` Rafael J. Wysocki 2020-06-24 14:44 ` Andrzej Hajda 2020-06-24 14:55 ` Rafael J. Wysocki 2020-06-24 12:30 ` Greg Kroah-Hartman 2020-06-24 14:48 ` Andrzej Hajda 2020-06-24 12:37 ` Robin Murphy 2020-06-24 12:55 ` Andy Shevchenko 2020-06-24 14:25 ` Robin Murphy 2020-06-24 15:04 ` Mark Brown 2020-06-24 15:16 ` Robin Murphy 2020-06-24 19:39 ` Andrzej Hajda 2020-06-25 8:41 ` Andy Shevchenko 2020-06-25 10:19 ` Andrzej Hajda 2020-06-24 13:29 ` Laurent Pinchart 2020-06-24 12:53 ` Andy Shevchenko 2020-06-24 13:12 ` Andrzej Hajda [not found] ` <CGME20200624114137eucas1p13599d33a0c4a9abf7708bf8c8e77264b@eucas1p1.samsung.com> 2020-06-24 11:41 ` [RESEND PATCH v5 4/5] drm/bridge/sii8620: fix resource acquisition error handling Andrzej Hajda 2020-06-24 13:25 ` Mark Brown 2020-06-24 13:43 ` Andrzej Hajda 2020-06-24 14:11 ` Mark Brown [not found] ` <CGME20200624114138eucas1p262505da3ad1067720080d20209ff32de@eucas1p2.samsung.com> 2020-06-24 11:41 ` [RESEND PATCH v5 5/5] drm/bridge: lvds-codec: simplify error handling code Andrzej Hajda 2020-06-24 13:33 ` Laurent Pinchart 2020-06-24 14:03 ` Andrzej Hajda 2020-06-24 21:18 ` Laurent Pinchart
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).