| Message ID | 2be0a28538bb2a3d1bcc91e2ca1f2d0dc09146d9.1727601608.git.dsimic@manjaro.org |
|---|---|
| State | Accepted |
| Commit | 36e69b160705b65bf136c2fb6a1194447eeb8478 |
| Headers | show |
| Series | Add dev_warn_probe() and improve error handling in Rockchip SPI drivers | expand |
On Sun Sep 29, 2024 at 11:21 AM CEST, Dragan Simic wrote: > Some drivers can still provide their functionality to a certain extent even > some of their resource acquisitions eventually fail. In such cases, emitting > errors isn't the desired action, but warnings should be emitted instead. > > To solve this, introduce dev_warn_probe() as a new device probe log helper, > which behaves identically as the already existing dev_err_probe(), while it > produces warnings instead of errors. The intended use is with the resources > that are actually optional for a particular driver. > > While there, copyedit the kerneldoc for dev_err_probe() a bit, to simplify > its wording a bit, and reuse it as the kerneldoc for dev_warn_probe(), with > the necessary wording adjustments, of course. > > Signed-off-by: Dragan Simic <dsimic@manjaro.org> Applied and tested on 6.11 for arm64 without noticing any issues. Tested-by: Hélène Vulquin <oss@helene.moe>
On Sun, Sep 29, 2024 at 11:21:16AM +0200, Dragan Simic wrote: > Some drivers can still provide their functionality to a certain extent even > some of their resource acquisitions eventually fail. In such cases, emitting > errors isn't the desired action, but warnings should be emitted instead. > > To solve this, introduce dev_warn_probe() as a new device probe log helper, > which behaves identically as the already existing dev_err_probe(), while it > produces warnings instead of errors. The intended use is with the resources > that are actually optional for a particular driver. > > While there, copyedit the kerneldoc for dev_err_probe() a bit, to simplify > its wording a bit, and reuse it as the kerneldoc for dev_warn_probe(), with > the necessary wording adjustments, of course. Greg, this makes sense to me - are you OK with me applying it?
On Sun, Sep 29, 2024 at 11:21:16AM +0200, Dragan Simic wrote: > Some drivers can still provide their functionality to a certain extent even > some of their resource acquisitions eventually fail. In such cases, emitting > errors isn't the desired action, but warnings should be emitted instead. > > To solve this, introduce dev_warn_probe() as a new device probe log helper, > which behaves identically as the already existing dev_err_probe(), while it > produces warnings instead of errors. The intended use is with the resources > that are actually optional for a particular driver. > > While there, copyedit the kerneldoc for dev_err_probe() a bit, to simplify > its wording a bit, and reuse it as the kerneldoc for dev_warn_probe(), with > the necessary wording adjustments, of course. > > Signed-off-by: Dragan Simic <dsimic@manjaro.org> > --- > drivers/base/core.c | 129 +++++++++++++++++++++++++++++-------- > include/linux/dev_printk.h | 1 + > 2 files changed, 102 insertions(+), 28 deletions(-) Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Mon, Oct 07, 2024 at 03:25:52PM +0100, Mark Brown wrote: > On Sun, Sep 29, 2024 at 11:21:16AM +0200, Dragan Simic wrote: > > Some drivers can still provide their functionality to a certain extent even > > some of their resource acquisitions eventually fail. In such cases, emitting > > errors isn't the desired action, but warnings should be emitted instead. > > > > To solve this, introduce dev_warn_probe() as a new device probe log helper, > > which behaves identically as the already existing dev_err_probe(), while it > > produces warnings instead of errors. The intended use is with the resources > > that are actually optional for a particular driver. > > > > While there, copyedit the kerneldoc for dev_err_probe() a bit, to simplify > > its wording a bit, and reuse it as the kerneldoc for dev_warn_probe(), with > > the necessary wording adjustments, of course. > > Greg, this makes sense to me - are you OK with me applying it? No objection from me, now sent a reviewed-by
Hello Mark, I just spotted a couple of small typos, noted below, and I hope you won't mind to apply the fixes by hand before applying this patch, please? On 2024-09-29 11:21, Dragan Simic wrote: > Some drivers can still provide their functionality to a certain extent > even s/extent even/extent when/ > some of their resource acquisitions eventually fail. In such cases, > emitting > errors isn't the desired action, but warnings should be emitted > instead. > > To solve this, introduce dev_warn_probe() as a new device probe log > helper, > which behaves identically as the already existing dev_err_probe(), > while it > produces warnings instead of errors. The intended use is with the > resources > that are actually optional for a particular driver. > > While there, copyedit the kerneldoc for dev_err_probe() a bit, to > simplify > its wording a bit, and reuse it as the kerneldoc for dev_warn_probe(), > with > the necessary wording adjustments, of course. > > Signed-off-by: Dragan Simic <dsimic@manjaro.org> > --- > drivers/base/core.c | 129 +++++++++++++++++++++++++++++-------- > include/linux/dev_printk.h | 1 + > 2 files changed, 102 insertions(+), 28 deletions(-) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 8c0733d3aad8..f2e41db0c09f 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -4982,71 +4982,144 @@ define_dev_printk_level(_dev_info, KERN_INFO); > > #endif > > +static void __dev_probe_failed(const struct device *dev, int err, bool > fatal, > + const char *fmt, va_list vargsp) > +{ > + struct va_format vaf; > + va_list vargs; > + > + /* > + * On x86_64 and possibly on other architectures, va_list is actually > a > + * size-1 array containing a structure. As a result, function > parameter > + * vargps decays from T[1] to T*, and &vargsp has type T** rather > than s/vargps decays/vargsp decays/ > + * T(*)[1], which is expected by its assignment to vaf.va below. > + * > + * One standard way to solve this mess is by creating a copy in a > local > + * variable of type va_list and then using a pointer to that local > copy > + * instead, which is the approach employed here. > + */ > + va_copy(vargs, vargsp); > + > + vaf.fmt = fmt; > + vaf.va = &vargs;
On Tue, Oct 08, 2024 at 06:18:46PM +0200, Dragan Simic wrote: > I just spotted a couple of small typos, noted below, and I hope you won't > mind to apply the fixes by hand before applying this patch, please? Sorry, your mail arrived after I'd already published the changes - please send an incremental patch for the one in the comments.
Hello Mark, On 2024-10-08 19:00, Mark Brown wrote: > On Tue, Oct 08, 2024 at 06:18:46PM +0200, Dragan Simic wrote: > >> I just spotted a couple of small typos, noted below, and I hope you >> won't >> mind to apply the fixes by hand before applying this patch, please? > > Sorry, your mail arrived after I'd already published the changes - > please send an incremental patch for the one in the comments. No worries. I just sent the incremental patch, [1] please have a look. Thanks for merging this patch series! [1] https://lore.kernel.org/linux-spi/cec37f5568afaef8fca2d35bb01c90556ccbb4f4.1728408464.git.dsimic@manjaro.org/
On Tue, Oct 08, 2024 at 07:32:18PM +0200, Dragan Simic wrote: > No worries. I just sent the incremental patch, [1] please have a look. > Thanks for merging this patch series! Oh, sorry - actually now I look again I got this confused with another patch and it's still in my CI so I can actually fix things up before merging.
On 2024-10-08 19:37, Mark Brown wrote: > On Tue, Oct 08, 2024 at 07:32:18PM +0200, Dragan Simic wrote: > >> No worries. I just sent the incremental patch, [1] please have a >> look. >> Thanks for merging this patch series! > > Oh, sorry - actually now I look again I got this confused with another > patch and it's still in my CI so I can actually fix things up before > merging. Awesome, even better!
diff --git a/drivers/base/core.c b/drivers/base/core.c index 8c0733d3aad8..f2e41db0c09f 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -4982,71 +4982,144 @@ define_dev_printk_level(_dev_info, KERN_INFO); #endif +static void __dev_probe_failed(const struct device *dev, int err, bool fatal, + const char *fmt, va_list vargsp) +{ + struct va_format vaf; + va_list vargs; + + /* + * On x86_64 and possibly on other architectures, va_list is actually a + * size-1 array containing a structure. As a result, function parameter + * vargps decays from T[1] to T*, and &vargsp has type T** rather than + * T(*)[1], which is expected by its assignment to vaf.va below. + * + * One standard way to solve this mess is by creating a copy in a local + * variable of type va_list and then using a pointer to that local copy + * instead, which is the approach employed here. + */ + va_copy(vargs, vargsp); + + vaf.fmt = fmt; + vaf.va = &vargs; + + switch (err) { + case -EPROBE_DEFER: + device_set_deferred_probe_reason(dev, &vaf); + dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), &vaf); + break; + + case -ENOMEM: + /* Don't print anything on -ENOMEM, there's already enough output */ + break; + + default: + /* Log fatal final failures as errors, otherwise produce warnings */ + if (fatal) + dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf); + else + dev_warn(dev, "error %pe: %pV", ERR_PTR(err), &vaf); + break; + } + + va_end(vargs); +} + /** * dev_err_probe - 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 debug or error message depending if the error value is * -EPROBE_DEFER and propagate error upwards. * In case of -EPROBE_DEFER it sets also defer probe reason, which can be * checked later by reading devices_deferred debugfs attribute. - * It replaces code sequence:: + * It replaces the following code sequence:: * * if (err != -EPROBE_DEFER) * dev_err(dev, ...); * else * dev_dbg(dev, ...); * return err; * * with:: * * return dev_err_probe(dev, err, ...); * - * Using this helper in your probe function is totally fine even if @err is - * known to never be -EPROBE_DEFER. + * Using this helper in your probe function is totally fine even if @err + * is known to never be -EPROBE_DEFER. * The benefit compared to a normal dev_err() is the standardized format - * of the error code, it being emitted symbolically (i.e. you get "EAGAIN" - * instead of "-35") and the fact that the error code is returned which allows - * more compact error paths. + * of the error code, which is emitted symbolically (i.e. you get "EAGAIN" + * instead of "-35"), and having the error code returned allows more + * compact error paths. * * Returns @err. */ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...) { - struct va_format vaf; - va_list args; + va_list vargs; - va_start(args, fmt); - vaf.fmt = fmt; - vaf.va = &args; + va_start(vargs, fmt); - switch (err) { - case -EPROBE_DEFER: - device_set_deferred_probe_reason(dev, &vaf); - dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), &vaf); - break; + /* Use dev_err() for logging when err doesn't equal -EPROBE_DEFER */ + __dev_probe_failed(dev, err, true, fmt, vargs); - case -ENOMEM: - /* - * We don't print anything on -ENOMEM, there is already enough - * output. - */ - break; + va_end(vargs); - default: - dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf); - break; - } + return err; +} +EXPORT_SYMBOL_GPL(dev_err_probe); - va_end(args); +/** + * dev_warn_probe - 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 debug or warning message depending if the error value is + * -EPROBE_DEFER and propagate error upwards. + * In case of -EPROBE_DEFER it sets also defer probe reason, which can be + * checked later by reading devices_deferred debugfs attribute. + * It replaces the following code sequence:: + * + * if (err != -EPROBE_DEFER) + * dev_warn(dev, ...); + * else + * dev_dbg(dev, ...); + * return err; + * + * with:: + * + * return dev_warn_probe(dev, err, ...); + * + * Using this helper in your probe function is totally fine even if @err + * is known to never be -EPROBE_DEFER. + * The benefit compared to a normal dev_warn() is the standardized format + * of the error code, which is emitted symbolically (i.e. you get "EAGAIN" + * instead of "-35"), and having the error code returned allows more + * compact error paths. + * + * Returns @err. + */ +int dev_warn_probe(const struct device *dev, int err, const char *fmt, ...) +{ + va_list vargs; + + va_start(vargs, fmt); + + /* Use dev_warn() for logging when err doesn't equal -EPROBE_DEFER */ + __dev_probe_failed(dev, err, false, fmt, vargs); + + va_end(vargs); return err; } -EXPORT_SYMBOL_GPL(dev_err_probe); +EXPORT_SYMBOL_GPL(dev_warn_probe); static inline bool fwnode_is_primary(struct fwnode_handle *fwnode) { diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h index ca32b5bb28eb..eb2094e43050 100644 --- a/include/linux/dev_printk.h +++ b/include/linux/dev_printk.h @@ -276,6 +276,7 @@ do { \ dev_driver_string(dev), dev_name(dev), ## arg) __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...); +__printf(3, 4) int dev_warn_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, ...) \
Some drivers can still provide their functionality to a certain extent even some of their resource acquisitions eventually fail. In such cases, emitting errors isn't the desired action, but warnings should be emitted instead. To solve this, introduce dev_warn_probe() as a new device probe log helper, which behaves identically as the already existing dev_err_probe(), while it produces warnings instead of errors. The intended use is with the resources that are actually optional for a particular driver. While there, copyedit the kerneldoc for dev_err_probe() a bit, to simplify its wording a bit, and reuse it as the kerneldoc for dev_warn_probe(), with the necessary wording adjustments, of course. Signed-off-by: Dragan Simic <dsimic@manjaro.org> --- drivers/base/core.c | 129 +++++++++++++++++++++++++++++-------- include/linux/dev_printk.h | 1 + 2 files changed, 102 insertions(+), 28 deletions(-)