diff mbox series

[v3,4/5] driver core: Add device probe log helper dev_warn_probe()

Message ID 2be0a28538bb2a3d1bcc91e2ca1f2d0dc09146d9.1727601608.git.dsimic@manjaro.org (mailing list archive)
State New
Headers show
Series Add dev_warn_probe() and improve error handling in Rockchip SPI drivers | expand

Commit Message

Dragan Simic Sept. 29, 2024, 9:21 a.m. UTC
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(-)

Comments

Hélène Vulquin Oct. 1, 2024, 9:02 a.m. UTC | #1
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>
Mark Brown Oct. 7, 2024, 2:25 p.m. UTC | #2
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?
Greg Kroah-Hartman Oct. 8, 2024, 1:22 p.m. UTC | #3
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>
Greg Kroah-Hartman Oct. 8, 2024, 1:22 p.m. UTC | #4
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
Dragan Simic Oct. 8, 2024, 4:18 p.m. UTC | #5
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;
Mark Brown Oct. 8, 2024, 5 p.m. UTC | #6
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.
Dragan Simic Oct. 8, 2024, 5:32 p.m. UTC | #7
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/
Mark Brown Oct. 8, 2024, 5:37 p.m. UTC | #8
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.
Dragan Simic Oct. 8, 2024, 5:39 p.m. UTC | #9
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 mbox series

Patch

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, ...) \