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) \