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(-)
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, ...) \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!
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 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 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
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>
© 2016 - 2024 Red Hat, Inc.