Messages in this thread Patch in this message | | | Subject | Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types | From | Robin Murphy <> | Date | Wed, 24 Jun 2020 13:37:52 +0100 |
| |
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) \ >
| |