| Message ID | 20200624114127.3016-4-a.hajda@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | driver core: add probe error check helper | expand |
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 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 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 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 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 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 >
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 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 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-1faeb9cf68e31587&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel >
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-a5a76e98e30aecc2&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel >
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. > > 2. 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 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-bba8bb4caf96e14d&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel > >
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
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) \
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(-)