Re: [PATCH v2 0/9] drm: Switch from dev_err to dev_err_probe for missing DSI host error path

[Nícolas F. R. A. Prado]

On Fri, Mar 01, 2024 at 08:34:31AM +0200, Laurent Pinchart wrote:
> Hi Nícolas,
> 
> On Thu, Feb 29, 2024 at 07:12:06PM -0500, Nícolas F. R. A. Prado wrote:
> > This series changes every occurence of the following pattern: 
> > 
> > 	dsi_host = of_find_mipi_dsi_host_by_node(dsi);
> > 	if (!dsi_host) {
> > 		dev_err(dev, "failed to find dsi host\n");
> > 		return -EPROBE_DEFER;
> > 	}
> > 
> > into
> > 
> > 	dsi_host = of_find_mipi_dsi_host_by_node(dsi);
> > 	if (!dsi_host)
> > 		return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi host\n");
> > 
> > This registers the defer probe reason (so it can later be printed by the
> > driver core or checked on demand through the devices_deferred file in
> > debugfs) and prevents errors to be spammed in the kernel log every time
> > the driver retries to probe, unnecessarily alerting userspace about
> > something that is a normal part of the boot process.
> 
> The idea is good, but I have a small issue with patches 1/9 to 7/9. They
> all patch a function that is called by the probe function. Calling
> dev_err_probe() in such functions is error-prone. I had to manually
> check when reviewing the patches that those functions were indeed called
> at probe time, and not through other code paths, and I also had to check
> that no callers were using dev_err_probe() in the error handling path,
> as that would have overridden the error message.
> 
> Would there be a way to move the dev_err_probe() to the top-level ? I
> understand it's not always possible or convenient, but if it was doable
> in at least some of the drivers, I think it would be better. I'll let
> you be the judge.

Hey Laurent, thanks for the review.

I get where you're coming from, as I checked those things myself while writing
the patch. That said, I don't think moving dev_err_probe() to the top-level is a
good move for a few reasons:
* Keeping the log message as close to the source of the error makes it more
  specific, and consequently, more useful.
* The original code already returned -EPROBE_DEFER, implying the function is
  expected to be called only from the probe function.

With those points in mind, the only way I see to guarantee
dev_err_probe(...,-EPROBE_DEFER...) would only be called by probe, and that the
reason wouldn't be overriden, would be to move the entire code path of that
function that calls into dev_err_probe() up into the probe function. But if we
adopt this pattern consistently across the drivers in the tree, I think it would
drastically worsen readability and cancel out the benefits.

IMO the way forward with the API we have, is to make use of warnings and static
checkers to catch cases where dev_err_probe() is overriding a defer probe
reason, and where it's called outside of the probe function scope.

So I'm inclined to leave the patches as they are, but am happy to discuss this
further or other ideas.

Thanks,
Nícolas