| Message ID | 20181016072244.1216-2-a.hajda@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | driver core: add probe error check helper | expand |
Hello Andrzej, On 10/16/2018 09:22 AM, 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 seqences 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> Best regards,
On Tue, Oct 16, 2018 at 09:22:42AM +0200, Andrzej Hajda wrote: > +int probe_err(const struct device *dev, int err, const char *fmt, ...) > +{ > + struct va_format vaf; > + va_list args; ... > + return err; > +} > + This will need an EXPORT_SYMBOL for modules won't it?
On Tue, Oct 16, 2018 at 10:22 AM 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 seqences with simple call, so code: > if (err != -EPROBE_DEFER) > dev_err(dev, ...); > return err; > becomes: > return probe_err(dev, err, ...); > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > --- > drivers/base/core.c | 37 +++++++++++++++++++++++++++++++++++++ > include/linux/device.h | 2 ++ > 2 files changed, 39 insertions(+) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 04bbcd779e11..23fabefb217a 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -3067,6 +3067,43 @@ 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) { Why not if (err == ...) return err; ... return err; ? Better to read, better to maintain. No? > + va_start(args, fmt); > + > + vaf.fmt = fmt; > + vaf.va = &args; > + > + __dev_printk(KERN_ERR, dev, &vaf); It would be nice to print an error code as well, wouldn't it? > + va_end(args); > + } > + > + return 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 90224e75ade4..06c2c797d132 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -1577,6 +1577,8 @@ do { \ > WARN_ONCE(condition, "%s %s: " format, \ > dev_driver_string(dev), dev_name(dev), ## arg) > > +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.18.0 >
On Tue, Oct 16, 2018 at 11:27:15AM +0100, Mark Brown wrote: > On Tue, Oct 16, 2018 at 09:22:42AM +0200, Andrzej Hajda wrote: > > > +int probe_err(const struct device *dev, int err, const char *fmt, ...) > > +{ > > + struct va_format vaf; > > + va_list args; > > ... > > > + return err; > > +} > > + > > This will need an EXPORT_SYMBOL for modules won't it? EXPORT_SYMBOL_GPL() to be specific, as it is dealing with the driver core. thanks, greg k-h
On 16.10.2018 13:01, Andy Shevchenko wrote: > On Tue, Oct 16, 2018 at 10:22 AM 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 seqences with simple call, so code: >> if (err != -EPROBE_DEFER) >> dev_err(dev, ...); >> return err; >> becomes: >> return probe_err(dev, err, ...); >> >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >> --- >> drivers/base/core.c | 37 +++++++++++++++++++++++++++++++++++++ >> include/linux/device.h | 2 ++ >> 2 files changed, 39 insertions(+) >> >> diff --git a/drivers/base/core.c b/drivers/base/core.c >> index 04bbcd779e11..23fabefb217a 100644 >> --- a/drivers/base/core.c >> +++ b/drivers/base/core.c >> @@ -3067,6 +3067,43 @@ 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) { > Why not > > if (err == ...) > return err; > > ... > return err; > > ? > > Better to read, better to maintain. No? Yes, anyway next patch will re-factor it anyway. > >> + va_start(args, fmt); >> + >> + vaf.fmt = fmt; >> + vaf.va = &args; >> + >> + __dev_printk(KERN_ERR, dev, &vaf); > It would be nice to print an error code as well, wouldn't it? Hmm, on probe fail error is printed anyway (with exception of EPROBE_DEFER, ENODEV and ENXIO): "probe of %s failed with error %d\n" On the other side currently some drivers prints the error code anyway via dev_err or similar, so I guess during conversion to probe_err it should be removed then. If we add error code to probe_err is it OK to report it this way? dev_err(dev, "%V, %d\n", &vaf, err); Regards Andrzej > >> + va_end(args); >> + } >> + >> + return 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 90224e75ade4..06c2c797d132 100644 >> --- a/include/linux/device.h >> +++ b/include/linux/device.h >> @@ -1577,6 +1577,8 @@ do { \ >> WARN_ONCE(condition, "%s %s: " format, \ >> dev_driver_string(dev), dev_name(dev), ## arg) >> >> +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.18.0 >> >
On 16.10.2018 13:29, Andrzej Hajda wrote: > On 16.10.2018 13:01, Andy Shevchenko wrote: >> On Tue, Oct 16, 2018 at 10:22 AM 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 seqences with simple call, so code: >>> if (err != -EPROBE_DEFER) >>> dev_err(dev, ...); >>> return err; >>> becomes: >>> return probe_err(dev, err, ...); >>> >>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >>> --- >>> drivers/base/core.c | 37 +++++++++++++++++++++++++++++++++++++ >>> include/linux/device.h | 2 ++ >>> 2 files changed, 39 insertions(+) >>> >>> diff --git a/drivers/base/core.c b/drivers/base/core.c >>> index 04bbcd779e11..23fabefb217a 100644 >>> --- a/drivers/base/core.c >>> +++ b/drivers/base/core.c >>> @@ -3067,6 +3067,43 @@ 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) { >> Why not >> >> if (err == ...) >> return err; >> >> ... >> return err; >> >> ? >> >> Better to read, better to maintain. No? > Yes, anyway next patch will re-factor it anyway. > >>> + va_start(args, fmt); >>> + >>> + vaf.fmt = fmt; >>> + vaf.va = &args; >>> + >>> + __dev_printk(KERN_ERR, dev, &vaf); >> It would be nice to print an error code as well, wouldn't it? > Hmm, on probe fail error is printed anyway (with exception of > EPROBE_DEFER, ENODEV and ENXIO): > "probe of %s failed with error %d\n" > On the other side currently some drivers prints the error code anyway > via dev_err or similar, so I guess during conversion to probe_err it > should be removed then. > > If we add error code to probe_err is it OK to report it this way? > dev_err(dev, "%V, %d\n", &vaf, err); Ups, I forgot that message passed to probe_err will contain already newline character. So the err must be before message passed to probe_err, for example: dev_err(dev, "err=%d: %V\n", err, &vaf); Is it OK? Or better leave this part of the patch as is? Regards Andrzej > > Regards > Andrzej > >>> + va_end(args); >>> + } >>> + >>> + return 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 90224e75ade4..06c2c797d132 100644 >>> --- a/include/linux/device.h >>> +++ b/include/linux/device.h >>> @@ -1577,6 +1577,8 @@ do { \ >>> WARN_ONCE(condition, "%s %s: " format, \ >>> dev_driver_string(dev), dev_name(dev), ## arg) >>> >>> +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.18.0 >>>
On Tue, Oct 16, 2018 at 3:55 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > On 16.10.2018 13:29, Andrzej Hajda wrote: > > On 16.10.2018 13:01, Andy Shevchenko wrote: > >> On Tue, Oct 16, 2018 at 10:22 AM 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 seqences with simple call, so code: > >>> if (err != -EPROBE_DEFER) > >>> dev_err(dev, ...); > >>> return err; > >>> becomes: > >>> return probe_err(dev, err, ...); > >>> + va_start(args, fmt); > >>> + > >>> + vaf.fmt = fmt; > >>> + vaf.va = &args; > >>> + > >>> + __dev_printk(KERN_ERR, dev, &vaf); > >> It would be nice to print an error code as well, wouldn't it? > > Hmm, on probe fail error is printed anyway (with exception of > > EPROBE_DEFER, ENODEV and ENXIO): > > "probe of %s failed with error %d\n" > > On the other side currently some drivers prints the error code anyway > > via dev_err or similar, so I guess during conversion to probe_err it > > should be removed then. > > > > If we add error code to probe_err is it OK to report it this way? > > dev_err(dev, "%V, %d\n", &vaf, err); > > Ups, I forgot that message passed to probe_err will contain already > newline character. You may consider not to pass it. > So the err must be before message passed to probe_err, for example: > dev_err(dev, "err=%d: %V\n", err, &vaf); > Is it OK? For me would work either (no \n in the message, or err preceding the message).
On Tue, Oct 16, 2018 at 04:55:00PM +0300, Andy Shevchenko wrote: > On Tue, Oct 16, 2018 at 3:55 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > > On 16.10.2018 13:29, Andrzej Hajda wrote: > > > On 16.10.2018 13:01, Andy Shevchenko wrote: > > >> On Tue, Oct 16, 2018 at 10:22 AM 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 seqences with simple call, so code: > > >>> if (err != -EPROBE_DEFER) > > >>> dev_err(dev, ...); > > >>> return err; > > >>> becomes: > > >>> return probe_err(dev, err, ...); > > > >>> + va_start(args, fmt); > > >>> + > > >>> + vaf.fmt = fmt; > > >>> + vaf.va = &args; > > >>> + > > >>> + __dev_printk(KERN_ERR, dev, &vaf); > > > >> It would be nice to print an error code as well, wouldn't it? > > > Hmm, on probe fail error is printed anyway (with exception of > > > EPROBE_DEFER, ENODEV and ENXIO): > > > "probe of %s failed with error %d\n" > > > On the other side currently some drivers prints the error code anyway > > > via dev_err or similar, so I guess during conversion to probe_err it > > > should be removed then. > > > > > > If we add error code to probe_err is it OK to report it this way? > > > dev_err(dev, "%V, %d\n", &vaf, err); > > > > Ups, I forgot that message passed to probe_err will contain already > > newline character. > > You may consider not to pass it. It's normal to pass the '\n', so by doing this, we create the situation where this function becomes the exception to the norm. That's not a good idea - we will see people forget that appending '\n' should not be done for this particular function. While we could add a checkpatch rule, that's hassle (extra rework). In any case, I think the message would be much better formatted if we did: dev_err(dev, "error %d: %V", err, &vaf); which means we end up with (eg): error -5: request_irq failed for irq 9 rather than: request_irq failed for irq 9, -5 which is more confusing.
On Wed, Oct 17, 2018 at 2:29 PM Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > > On Tue, Oct 16, 2018 at 04:55:00PM +0300, Andy Shevchenko wrote: > > On Tue, Oct 16, 2018 at 3:55 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > > > On 16.10.2018 13:29, Andrzej Hajda wrote: > > > > On 16.10.2018 13:01, Andy Shevchenko wrote: > > > >> On Tue, Oct 16, 2018 at 10:22 AM 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 seqences with simple call, so code: > > > >>> if (err != -EPROBE_DEFER) > > > >>> dev_err(dev, ...); > > > >>> return err; > > > >>> becomes: > > > >>> return probe_err(dev, err, ...); > > > > > >>> + va_start(args, fmt); > > > >>> + > > > >>> + vaf.fmt = fmt; > > > >>> + vaf.va = &args; > > > >>> + > > > >>> + __dev_printk(KERN_ERR, dev, &vaf); > > > > > >> It would be nice to print an error code as well, wouldn't it? > > > > Hmm, on probe fail error is printed anyway (with exception of > > > > EPROBE_DEFER, ENODEV and ENXIO): > > > > "probe of %s failed with error %d\n" > > > > On the other side currently some drivers prints the error code anyway > > > > via dev_err or similar, so I guess during conversion to probe_err it > > > > should be removed then. > > > > > > > > If we add error code to probe_err is it OK to report it this way? > > > > dev_err(dev, "%V, %d\n", &vaf, err); > > > > > > Ups, I forgot that message passed to probe_err will contain already > > > newline character. > > > > You may consider not to pass it. > > It's normal to pass the '\n', so by doing this, we create the situation > where this function becomes the exception to the norm. That's not a > good idea - we will see people forget that appending '\n' should not > be done for this particular function. > > While we could add a checkpatch rule, that's hassle (extra rework). In > any case, I think the message would be much better formatted if we did: > > dev_err(dev, "error %d: %V", err, &vaf); > > which means we end up with (eg): > > error -5: request_irq failed for irq 9 > > rather than: > > request_irq failed for irq 9, -5 > > which is more confusing. As I said earlier, I'm fine to either variant and I see your point here, so, I tend to agree to this variant. P.S. Andrzej, in any case my Rb tag, which I just gave, stays.
On Wed, 2018-10-17 at 12:29 +0100, Russell King - ARM Linux wrote: > On Tue, Oct 16, 2018 at 04:55:00PM +0300, Andy Shevchenko wrote: > > On Tue, Oct 16, 2018 at 3:55 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > > > On 16.10.2018 13:29, Andrzej Hajda wrote: > > > > On 16.10.2018 13:01, Andy Shevchenko wrote: > > > > > On Tue, Oct 16, 2018 at 10:22 AM 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 seqences with simple call, so code: > > > > > > if (err != -EPROBE_DEFER) > > > > > > dev_err(dev, ...); > > > > > > return err; > > > > > > becomes: > > > > > > return probe_err(dev, err, ...); > > > > > > + va_start(args, fmt); > > > > > > + > > > > > > + vaf.fmt = fmt; > > > > > > + vaf.va = &args; > > > > > > + > > > > > > + __dev_printk(KERN_ERR, dev, &vaf); > > > > > It would be nice to print an error code as well, wouldn't it? > > > > Hmm, on probe fail error is printed anyway (with exception of > > > > EPROBE_DEFER, ENODEV and ENXIO): > > > > "probe of %s failed with error %d\n" > > > > On the other side currently some drivers prints the error code anyway > > > > via dev_err or similar, so I guess during conversion to probe_err it > > > > should be removed then. > > > > > > > > If we add error code to probe_err is it OK to report it this way? > > > > dev_err(dev, "%V, %d\n", &vaf, err); > > > > > > Ups, I forgot that message passed to probe_err will contain already > > > newline character. > > > > You may consider not to pass it. > > It's normal to pass the '\n', so by doing this, we create the situation > where this function becomes the exception to the norm. That's not a > good idea - we will see people forget that appending '\n' should not > be done for this particular function. > > While we could add a checkpatch rule, that's hassle (extra rework). It would not be a simple checkpatch rule with high confidence because of pr_cont uses that may not be in the patch context. > In I think the message would be much better formatted if we did: > > dev_err(dev, "error %d: %V", err, &vaf); s/%V/%pV/
diff --git a/drivers/base/core.c b/drivers/base/core.c index 04bbcd779e11..23fabefb217a 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -3067,6 +3067,43 @@ 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) { + va_start(args, fmt); + + vaf.fmt = fmt; + vaf.va = &args; + + __dev_printk(KERN_ERR, dev, &vaf); + va_end(args); + } + + return 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 90224e75ade4..06c2c797d132 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -1577,6 +1577,8 @@ do { \ WARN_ONCE(condition, "%s %s: " format, \ dev_driver_string(dev), dev_name(dev), ## arg) +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))
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 seqences with simple call, so code: if (err != -EPROBE_DEFER) dev_err(dev, ...); return err; becomes: return probe_err(dev, err, ...); Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> --- drivers/base/core.c | 37 +++++++++++++++++++++++++++++++++++++ include/linux/device.h | 2 ++ 2 files changed, 39 insertions(+)