| Message ID | 20240423-dev-add_dev_errp_probe-v2-1-12f43c5d8b0d@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dev_printk: add dev_errp_probe() helper | expand |
On Tue, Apr 23, 2024 at 05:20:30PM +0200, Nuno Sa via B4 Relay wrote: > From: Nuno Sa <nuno.sa@analog.com> > > This is similar to dev_err_probe() but for cases where an ERR_PTR() or > ERR_CAST() is to be returned simplifying patterns like: > > dev_err_probe(dev, ret, ...); > return ERR_PTR(ret) > or > dev_err_probe(dev, PTR_ERR(ptr), ...); > return ERR_CAST(ptr) ... > +/* Simple helper for dev_err_probe() when ERR_PTR() is to be returned. */ > +#define dev_err_ptr_probe(dev, ___err, fmt, ...) ({ \ > + ERR_PTR(dev_err_probe(dev, ___err, fmt, ##__VA_ARGS__)); \ > +}) Why ; and hence why ({}) ? I even believe the compiler may warn if you have double ;; in some cases. ... > +#define dev_err_cast_probe(dev, ___err_ptr, fmt, ...) ({ \ > + ERR_PTR(dev_err_probe(dev, PTR_ERR(___err_ptr), fmt, ##__VA_ARGS__)); \ > +}) Ditto.
On Tue, Apr 23, 2024 at 06:31:20PM +0300, Andy Shevchenko wrote: > On Tue, Apr 23, 2024 at 05:20:30PM +0200, Nuno Sa via B4 Relay wrote: > > From: Nuno Sa <nuno.sa@analog.com> ... > > +#define dev_err_cast_probe(dev, ___err_ptr, fmt, ...) ({ \ > > + ERR_PTR(dev_err_probe(dev, PTR_ERR(___err_ptr), fmt, ##__VA_ARGS__)); \ > > +}) After looking into the next patch I think this should be rewritten to use %pe, hence should be an exported function. Or dev_err_probe() should be split to a version that makes the difference between int and const void * (maybe using _Generic()).
On Tue, 2024-04-23 at 18:31 +0300, Andy Shevchenko wrote: > On Tue, Apr 23, 2024 at 05:20:30PM +0200, Nuno Sa via B4 Relay wrote: > > From: Nuno Sa <nuno.sa@analog.com> > > > > This is similar to dev_err_probe() but for cases where an ERR_PTR() or > > ERR_CAST() is to be returned simplifying patterns like: > > > > dev_err_probe(dev, ret, ...); > > return ERR_PTR(ret) > > or > > dev_err_probe(dev, PTR_ERR(ptr), ...); > > return ERR_CAST(ptr) > > ... > > > +/* Simple helper for dev_err_probe() when ERR_PTR() is to be returned. */ > > +#define dev_err_ptr_probe(dev, ___err, fmt, ...) ({ \ > > + ERR_PTR(dev_err_probe(dev, ___err, fmt, ##__VA_ARGS__)); \ > > +}) > > Why ; and hence why ({}) ? > > I even believe the compiler may warn if you have double ;; in some cases. > Oh yes, no need for any of those... - Nuno Sá
On Tue, 2024-04-23 at 18:45 +0300, Andy Shevchenko wrote: > On Tue, Apr 23, 2024 at 06:31:20PM +0300, Andy Shevchenko wrote: > > On Tue, Apr 23, 2024 at 05:20:30PM +0200, Nuno Sa via B4 Relay wrote: > > > From: Nuno Sa <nuno.sa@analog.com> > > ... > > > > +#define dev_err_cast_probe(dev, ___err_ptr, fmt, > > > ...) ({ \ > > > + ERR_PTR(dev_err_probe(dev, PTR_ERR(___err_ptr), fmt, > > > ##__VA_ARGS__)); \ > > > +}) > > After looking into the next patch I think this should be rewritten to use %pe, > hence should be an exported function. Or dev_err_probe() should be split to > a version that makes the difference between int and const void * (maybe using > _Generic()). > I replied a bit in the other patch but I'm of the opinion that's likely just more complicated than it needs to be (IMO). Why is the PTR_ERR(___err_ptr) that bad? If we really want to have a version that takes pointer why not just: #define dev_err_ptr_probe(dev, ___err, fmt, ...) \ dev_err_probe(dev, PTR_ERR(__err), fmt, ##__VA_ARGS__) (yes, while _Generic() could be fun I'm trying to avoid it. In this case, I think having explicit defines is more helpful) - Nuno Sá
On Thu, May 02, 2024 at 01:54:36PM +0200, Nuno Sá wrote: > On Tue, 2024-04-23 at 18:45 +0300, Andy Shevchenko wrote: > > On Tue, Apr 23, 2024 at 06:31:20PM +0300, Andy Shevchenko wrote: > > > On Tue, Apr 23, 2024 at 05:20:30PM +0200, Nuno Sa via B4 Relay wrote: > > > > From: Nuno Sa <nuno.sa@analog.com> ... > > > > +#define dev_err_cast_probe(dev, ___err_ptr, fmt, > > > > ...) ({ \ > > > > + ERR_PTR(dev_err_probe(dev, PTR_ERR(___err_ptr), fmt, > > > > ##__VA_ARGS__)); \ > > > > +}) > > > > After looking into the next patch I think this should be rewritten to use %pe, > > hence should be an exported function. Or dev_err_probe() should be split to > > a version that makes the difference between int and const void * (maybe using > > _Generic()). > > I replied a bit in the other patch but I'm of the opinion that's likely just more > complicated than it needs to be (IMO). Why is the PTR_ERR(___err_ptr) that bad? If we > really want to have a version that takes pointer why not just: > > #define dev_err_ptr_probe(dev, ___err, fmt, ...) \ > dev_err_probe(dev, PTR_ERR(__err), fmt, ##__VA_ARGS__) > > > (yes, while _Generic() could be fun I'm trying to avoid it. In this case, I think > having explicit defines is more helpful) It seems dev_err_probe() already uses %pe, so we are fine.
diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h index ae80a303c216..b9623ec22350 100644 --- a/include/linux/dev_printk.h +++ b/include/linux/dev_printk.h @@ -277,4 +277,13 @@ do { \ __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...); +/* Simple helper for dev_err_probe() when ERR_PTR() is to be returned. */ +#define dev_err_ptr_probe(dev, ___err, fmt, ...) ({ \ + ERR_PTR(dev_err_probe(dev, ___err, fmt, ##__VA_ARGS__)); \ +}) + +/* Simple helper for dev_err_probe() when ERR_CAST() is to be returned. */ +#define dev_err_cast_probe(dev, ___err_ptr, fmt, ...) ({ \ + ERR_PTR(dev_err_probe(dev, PTR_ERR(___err_ptr), fmt, ##__VA_ARGS__)); \ +}) #endif /* _DEVICE_PRINTK_H_ */