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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

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 |