[PATCH v2] base: driver: print dev_err_probe message on permanent probe deferral

[PATCH v2] base: driver: print dev_err_probe message on permanent probe deferral

[Johannes Zink]

This stores the probe deferral reason in the device structure.
If a probe is permanently deferred, the last reason is displayed.

Other dev_err_probe outputs are displayed as before.

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
v1 -> v2:
  - improved commit message
  - improved function prototypes
  - improved debug output, device name is displayed once only per
    message
  - reverted removal of MSG_DEBUG output on deferred probe
  - replaced kfree by free
  - removed unnecessary null checks before free
  - fixed several typos
  - fixed compiler warnings
  - fixed coding style issues
---
drivers/base/driver.c | 47 ++++++++++++++++++++++++++++++++++---------
 include/driver.h      |  6 ++++++
 2 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 303ca061c..02d185e01 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -283,6 +283,7 @@ void free_device_res(struct device_d *dev)
 	dev->name = NULL;
 	free(dev->unique_name);
 	dev->unique_name = NULL;
+	free(dev->deferred_probe_reason);
 }
 EXPORT_SYMBOL(free_device_res);
 
@@ -300,6 +301,12 @@ void free_device(struct device_d *dev)
 }
 EXPORT_SYMBOL(free_device);
 
+
+static char *device_get_deferred_probe_reason(const struct device_d *dev)
+{
+	return dev->deferred_probe_reason;
+}
+
 /*
  * Loop over list of deferred devices as long as at least one
  * device is successfully probed. Devices that again request
@@ -312,6 +319,7 @@ static int device_probe_deferred(void)
 	struct device_d *dev, *tmp;
 	struct driver_d *drv;
 	bool success;
+	char *reason;
 
 	do {
 		success = false;
@@ -333,9 +341,13 @@ static int device_probe_deferred(void)
 		}
 	} while (success);
 
-	list_for_each_entry(dev, &deferred, active)
-		dev_err(dev, "probe permanently deferred\n");
-
+	list_for_each_entry(dev, &deferred, active) {
+		reason = device_get_deferred_probe_reason(dev);
+		if (reason)
+			dev_err(dev, "probe permanently deferred (%s)\n", reason);
+		else
+			dev_err(dev, "probe permanently deferred\n");
+	}
 	return 0;
 }
 late_initcall(device_probe_deferred);
@@ -573,7 +585,16 @@ const void *device_get_match_data(struct device_d *dev)
 	return NULL;
 }
 
-/**
+static void device_set_deferred_probe_reason(struct device_d *dev, const struct va_format *vaf)
+{
+	char *reason;
+
+	free(dev->deferred_probe_reason);
+
+	reason = basprintf("%pV", vaf);
+	dev->deferred_probe_reason = reason;
+}
+/*
  * dev_err_probe - probe error check and log helper
  * @loglevel: log level configured in source file
  * @dev: the pointer to the struct device
@@ -584,8 +605,12 @@ const void *device_get_match_data(struct device_d *dev)
  * 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.
+ *
+ * In case of -EPROBE_DEFER it sets the device's deferred_probe_reason attribute,
+ * but does not report an error. The error is recorded and displayed later, if
+ * (and only if) the probe is permanently  deferred. For all other error codes,
+ * it just outputs the error
+ *
  * It replaces code sequence::
  *
  * 	if (err != -EPROBE_DEFER)
@@ -606,13 +631,17 @@ int dev_err_probe(const struct device_d *dev, int err, const char *fmt, ...)
 {
 	struct va_format vaf;
 	va_list args;
-
+	
 	va_start(args, fmt);
 	vaf.fmt = fmt;
 	vaf.va = &args;
 
-	dev_printf(err == -EPROBE_DEFER ? MSG_DEBUG : MSG_ERR,
-		   dev, "error %pe: %pV", ERR_PTR(err), &vaf);
+	/* deferred probe, just remember the error reason */
+	if (err == -EPROBE_DEFER)
+		device_set_deferred_probe_reason((struct device_d*)dev, &vaf);
+
+	dev_printf(err == -EPROBE_DEFER ? MSG_DEBUG : MSG_ERR, 
+		   dev, "error %pe: %pV\n", ERR_PTR(err), &vaf);
 
 	va_end(args);
 
diff --git a/include/driver.h b/include/driver.h
index b35b5f397..2dfe4719a 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -88,6 +88,12 @@ struct device_d {
 	 * when the driver should actually detect client devices
 	 */
 	int     (*detect) (struct device_d *);
+
+	/*
+	 * if a driver probe is deferred, this stores the last error
+	 */
+
+	char *deferred_probe_reason;
 };
 
 /** @brief Describes a driver present in the system */
-- 
2.30.2

Re: [PATCH v2] base: driver: print dev_err_probe message on permanent probe deferral

[Ahmad Fatoum]

Hello Johannes,

On 07.07.22 17:54, Johannes Zink wrote:
> This stores the probe deferral reason in the device structure.
> If a probe is permanently deferred, the last reason is displayed.
> 
> Other dev_err_probe outputs are displayed as before.
> 
> Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> ---
> v1 -> v2:

Looks good now, some minor feedback below.

> +static char *device_get_deferred_probe_reason(const struct device_d *dev)
> +{
> +	return dev->deferred_probe_reason;
> +}

Compiler would always inline this function, but still, why not use the member
directly? Gets rid of the helper and call sites are even shorter than with the
helper.

> -/**

Comment was kerneldoc before, but now it's not anymore. I guess you didn't
intend that?

> +static void device_set_deferred_probe_reason(struct device_d *dev, const struct va_format *vaf)
> +{
> +	char *reason;

As commented on v1, this is rather unnecessary, just drop it?

> + * In case of -EPROBE_DEFER it sets the device's deferred_probe_reason attribute,
> + * but does not report an error. The error is recorded and displayed later, if
> + * (and only if) the probe is permanently  deferred. For all other error codes,

s/permanently  deferred/permanently deferred/

> + * it just outputs the error

.. code along with the error message (see v1).

> + *
>   * It replaces code sequence::
>   *
>   * 	if (err != -EPROBE_DEFER)
> @@ -606,13 +631,17 @@ int dev_err_probe(const struct device_d *dev, int err, const char *fmt, ...)
>  {
>  	struct va_format vaf;
>  	va_list args;
> -
> +	

Unintended whitespace change?

>  	va_start(args, fmt);
>  	vaf.fmt = fmt;
>  	vaf.va = &args;
>  
> -	dev_printf(err == -EPROBE_DEFER ? MSG_DEBUG : MSG_ERR,
> -		   dev, "error %pe: %pV", ERR_PTR(err), &vaf);
> +	/* deferred probe, just remember the error reason */
> +	if (err == -EPROBE_DEFER)
> +		device_set_deferred_probe_reason((struct device_d*)dev, &vaf);

Better drop const from dev_err_probe's first parameter type. It makes sense
for Linux, which keeps private data separate, but it doesn't for barebox.
This is somewhat ugly too, because the other dev_printf functions (e.g. dev_err)
accept a const struct device_d, but that's most probably less ugly than
having a prototype saying that it's const and cast that away anyway..

> +
> +	dev_printf(err == -EPROBE_DEFER ? MSG_DEBUG : MSG_ERR, 
> +		   dev, "error %pe: %pV\n", ERR_PTR(err), &vaf);

The format string will already be new line terminated and here you
add one more new line that didn't exist before, so you end up with
an empty line after every error message.

> @@ -88,6 +88,12 @@ struct device_d {
>  	 * when the driver should actually detect client devices
>  	 */
>  	int     (*detect) (struct device_d *);
> +
> +	/*
> +	 * if a driver probe is deferred, this stores the last error
> +	 */
> +

At least the previous member didn't have a space after the comment, so you
may want to do likewise here.

> +	char *deferred_probe_reason;
>  };
>  
>  /** @brief Describes a driver present in the system */

Cheers,
Ahmad

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |