Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper
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 ff0c.mmc: IDMAC supports 32-bit address > mode. > [3.054312] dwmmc_rockchip ff0c.mmc: Using internal DMA controller. > [3.061774] dwmmc_rockchip ff0c.mmc: Version ID is 270a > [3.068101] dwmmc_rockchip ff0c.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. signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper
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 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 ff0c.mmc: IDMAC supports 32-bit address mode. [3.054312] dwmmc_rockchip ff0c.mmc: Using internal DMA controller. [3.061774] dwmmc_rockchip ff0c.mmc: Version ID is 270a [3.068101] dwmmc_rockchip ff0c.mmc: DW MMC controller at irq 30,32 bit host data width,256 deep fifo [3.079638] dwmmc_rockchip ff0d.mmc: IDMAC supports 32-bit address mode. [3.087678] dwmmc_rockchip ff0d.mmc: Using internal DMA controller. [3.095134] dwmmc_rockchip ff0d.mmc: Version ID is 270a [3.101480] dwmmc_rockchip ff0d.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [3.113071] dwmmc_rockchip ff0f.mmc: IDMAC supports 32-bit address mode. [3.121110] dwmmc_rockchip ff0f.mmc: Using internal DMA controller. [3.128565] dwmmc_rockchip ff0f.mmc: Version ID is 270a [3.134886] dwmmc_rockchip ff0f.mmc: DW MMC controller at irq 32,32 bit host data width,256 deep fifo [3.948510] dwmmc_rockchip ff0c.mmc: IDMAC supports 32-bit address mode. [3.956475] dwmmc_rockchip ff0c.mmc: Using internal DMA controller. [3.963884] dwmmc_rockchip ff0c.mmc: Version ID is 270a [3.970133] dwmmc_rockchip ff0c.mmc: DW MMC controller at irq 30,32 bit host data width,256 deep fifo [4.141231] dwmmc_rockchip ff0d.mmc: IDMAC supports 32-bit address mode. [4.149178] dwmmc_rockchip ff0d.mmc: Using internal DMA controller. [4.156582] dwmmc_rockchip ff0d.mmc: Version ID is 270a [4.162823] dwmmc_rockchip ff0d.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [4.175606] dwmmc_rockchip ff0f.mmc: IDMAC supports 32-bit address mode. [4.183540] dwmmc_rockchip ff0f.mmc: Using internal DMA controller. [4.190946] dwmmc_rockchip ff0f.mmc: Version ID is 270a [4.197196] dwmmc_rockchip ff0f.mmc: DW MMC controller at irq 32,32 bit host data width,256 deep fifo [4.250758] dwmmc_rockchip ff0d.mmc: IDMAC supports 32-bit address mode. [4.258688] dwmmc_rockchip ff0d.mmc: Using internal DMA controller. [4.266104] dwmmc_rockchip ff0d.mmc: Version ID is 270a [4.272358] dwmmc_rockchip ff0d.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [4.285390] dwmmc_rockchip ff0d.mmc: IDMAC supports 32-bit address mode. [4.29] dwmmc_rockchip ff0d.mmc: Using internal DMA controller. [4.300750] dwmmc_rockchip ff0d.mmc: Version ID is 270a [4.307005] dwmmc_rockchip ff0d.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [4.971373] dwmmc_rockchip ff0f.mmc: Successfully tuned phase to 134 [5.027225] dwmmc_rockchip ff0d.mmc: IDMAC supports 32-bit address mode. [5.035339] dwmmc_rockchip ff0d.mmc: Using internal DMA controller. [5.042769] dwmmc_rockchip ff0d.mmc: Version ID is 270a [5.049050] dwmmc_rockchip ff0d.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [ 24.727583] dwmmc_rockchip ff0d.mmc: IDMAC supports 32-bit address mode. [ 24.745541] dwmmc_rockchip ff0d.mmc: Using internal DMA controller. [ 24.753003] dwmmc_rockchip ff0d.mmc: Version ID is 270a [ 24.763289] dwmmc_rockchip ff0d.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [ 25.589620] dwmmc_rockchip ff0d.mmc: IDMAC supports 32-bit address mode. [ 25.603066] dwmmc_rockchip ff0d.mmc: Using internal DMA controller. [ 25.615283] dwmmc_rockchip ff0d.mmc: Version ID is 270a [ 25.627911] dwmmc_rockchip ff0d.mmc: DW MMC controller at irq 31,32 bit host data width,256 deep fifo [ 25.643469] dwmmc_rockchip ff0d.mmc: IDMAC supports 32-bit address mode. [ 25.651532] dwmmc_rockchip ff0d.mmc: Using internal DMA controller. [
Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper
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
>>> Reviewed-by: Javier Martinez Canillas
>>> Reviewed-by: Mark Brown
>>> Reviewed-by: Andy Shevchenko
>>> ---
>>> 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 =
>>> +
>>> + dev_err(dev, "error %d: %pV", err, );
>>> +
>>> + 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
Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper
On Wed, Jun 24, 2020 at 04:45:28PM +0300, Andy Shevchenko wrote: > On Wed, Jun 24, 2020 at 4:27 PM Mark Brown 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. signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper
On Wed, Jun 24, 2020 at 4:27 PM Mark Brown 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
Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper
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. signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper
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
> > Reviewed-by: Javier Martinez Canillas
> > Reviewed-by: Mark Brown
> > Reviewed-by: Andy Shevchenko
> > ---
> > 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 =
> > +
> > + dev_err(dev, "error %d: %pV", err, );
> > +
> > + 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
Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper
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
> Reviewed-by: Javier Martinez Canillas
> Reviewed-by: Mark Brown
> Reviewed-by: Andy Shevchenko
> ---
> 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 =
> +
> + dev_err(dev, "error %d: %pV", err, );
> +
> + 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
Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper
On Wed, Jun 24, 2020 at 1:41 PM 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
> Reviewed-by: Javier Martinez Canillas
> Reviewed-by: Mark Brown
> Reviewed-by: Andy Shevchenko
Reviewed-by Rafael J. Wysocki
> ---
> 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 =
> +
> + dev_err(dev, "error %d: %pV", err, );
> +
> + 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
[RESEND PATCH v5 1/5] driver core: add probe_err log helper
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
Reviewed-by: Javier Martinez Canillas
Reviewed-by: Mark Brown
Reviewed-by: Andy Shevchenko
---
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 =
+
+ dev_err(dev, "error %d: %pV", err, );
+
+ 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
