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(-)
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))
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 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 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 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.
/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);
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);elsedev_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
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);elsedev_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-5...
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;} @@ -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
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);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;@@ -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-b...
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) \
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
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-1...
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.
- 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!
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
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-a...
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) \
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.
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.
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.
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.
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-b...
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.
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
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) \
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?
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
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";
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.
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-3...
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.
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);
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);
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/?i...
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);
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.
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);
dri-devel@lists.freedesktop.org
-
Andrzej Hajda -
Andy Shevchenko -
Greg Kroah-Hartman -
Laurent Pinchart -
Mark Brown -
Rafael J. Wysocki -
Robin Murphy