| Message ID | 20200624114127.3016-2-a.hajda@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,v5,1/5] driver core: add probe_err log helper | expand |
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 >
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
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.
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.
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.
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.
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.
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
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.
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))