Alternative approach to solve the deferred probe

Commit Message

Grygorii Strashko Oct. 22, 2015, 3:12 p.m. UTC

Hi Russell,

On 10/21/2015 09:28 PM, Russell King - ARM Linux wrote:
> On Wed, Oct 21, 2015 at 09:13:48PM +0300, Grygorii Strashko wrote:
>> But I worry a bit (and that my main point) about these few additional
>> rounds of deferred device probing which I have right now and which allows
>> some of drivers to finish, finally, their probes successfully.
>> With proposed change I'll get more messages in boot log, but some of
>> them will belong to drivers which have been probed successfully and so,
>> they will be not really useful.
> 
> Then you haven't properly understood my proposal.
> 
> I want to get rid of all the "X deferred its probing" messages up until
> the point that we set the "please report deferred probes" flag.
> 
> That _should_ mean that all the deferred probing that goes on becomes
> _totally_ silent and becomes hidden (unless you really want to see it,
> in which case we can make a debug option which turns it on) up until
> we're at the point where we want to enter userspace.
> 
> At that point, we then report into the kernel log which devices are
> still deferring and, via appropriately placed dev_warn_deferred(),
> the reasons why the devices are being deferred.
> 
> So, gone will be all the messages earlier in the log about device X
> not having a GPIO/clock/whatever because the device providing the
> GPIO/clock/whatever hasn't been probed.
> 
> If everything is satisfied by the time we run this last round (again,
> I'm not using a three line sentence to describe exactly what I mean,
> I'm sure you know by now... oops, I just did) then the kernel will
> report nothing about any deferrals.  That's _got_ to be an improvement.

Sorry Master, but you really don't need to spend so much time typing the
same things three times  - I understand what are you trying to do :(

I did my comments with assumption that it's not officially prohibited/deprecated
to register drivers (and execute probes) from late_initcall() layer
(just recommended) and there are still bunch of drivers which are doing this.
Now I see that it's not a recommendation any more, and deferred_probe_initcall()
might be a good place to activate driver_deferred_probe_report if goal is to
identify and fix such drivers.

Sorry for your time.

> 
>>
>> As result, I think, the most important thing is to identify (or create)
>> some point during kernel boot when it will be possible to say that all
>> built-in drivers (at least) finish their probes 100% (done or defer).
>>
>> Might be do_initcalls() can be updated (smth like this):
>> static void __init do_initcalls(void)
>> {
>> 	int level;
>>
>> 	for (level = 0; level < ARRAY_SIZE(initcall_levels) - 1; level++)
>> 		do_initcall_level(level);
>>
>> +	wait_for_device_probe();
>> +	/* Now one final round, reporting any devices that remain deferred */
>> +	driver_deferred_probe_report = true;
>> +	driver_deferred_probe_trigger();
>> +	wait_for_device_probe();
>> }
>>
>> Also, in my opinion, it will be useful if this debugging feature will be
>> optional.
> 
> I wonder why you want it optional... so I'm going to guess and cover
> both cases I can think of below to head off another round of reply on
> this point (sorry if this sucks eggs.)
> 
> I don't see it as being optional, because it's going to be cheap to run
> in the case of a system which has very few or no errors - which is what
> you should have for production systems, right?
> 

Also, I've spend some time today testing your proposal - hope you'll find results
useful.

I've applied truncated version of your patch (diff below) on TI's 4.1 kernel and
run few tests (log is below) on dra7-evm/am43xx-gpevm - K4.1 is not far away from LKML,
so I assume this test is valid. Overall boot process consists from two stages:
kernel boot and modules loading. 

My Changes:
 - only really_probe() modified to show deferred device/drivers 

From the log I can see additional messages in log when modules are loading,
because driver_deferred_probe_report is still true - dwc3 probes were deferred,
but then finally succeeded.

So, as you've mentioned, it seems a good thing to deactivate driver_deferred_probe_report and
provide user with ability to turn it on again (and probably re-trigger deferred
device probing).

I've found no issues during Kernel boot (built-in) time, new messages are displayed only
if probe is failed for some drivers:
[    3.219700] ====================================== deferred_probe_initcalll
[    3.226820] platform omapdrm.0: Driver omapdrm requests probe deferral
[    3.233378] platform omapdrm.0: deferring probe: ==== Driver omapdrm requests probe deferral
[    3.242084] dra7evm-tpd12s015 encoder@1: failed to parse CT CP HPD gpio
[    3.248737] platform encoder@1: Driver dra7evm-tpd12s015 requests probe deferral
[    3.256168] platform encoder@1: deferring probe: ==== Driver dra7evm-tpd12s015 requests probe deferral
[    3.265763] connector-hdmi connector@1: failed to find video source
[    3.272067] platform connector@1: Driver connector-hdmi requests probe deferral
[    3.279410] platform connector@1: deferring probe: ==== Driver connector-hdmi requests probe deferral 
^^ above drivers will be deferred forever

Thanks.

Patch

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index e7d2545..d61fa47 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -129,6 +129,27 @@  void driver_deferred_probe_del(struct device *dev)
 	mutex_unlock(&deferred_probe_mutex);
 }
 
+static bool driver_deferred_probe_report;
+
+/**
+ * dev_warn_deferred() - report why a probe has been deferred
+ */
+void dev_warn_deferred(struct device *dev, const char *fmt, ...)
+{
+	if (driver_deferred_probe_report) {
+		struct va_format vaf;
+		va_list ap;
+
+		va_start(ap, fmt);
+		vaf.fmt = fmt;
+		vaf.va = ≈
+
+		dev_err(dev, "deferring probe: %pV", &vaf);
+		va_end(ap);
+	}
+}
+EXPORT_SYMBOL_GPL(dev_warn_deferred);
+
 static bool driver_deferred_probe_enable = false;
 /**
  * driver_deferred_probe_trigger() - Kick off re-probing deferred devices
@@ -188,6 +209,15 @@  static int deferred_probe_initcall(void)
 	driver_deferred_probe_trigger();
 	/* Sort as many dependencies as possible before exiting initcalls */
 	flush_workqueue(deferred_wq);
+
+	pr_err("====================================== deferred_probe_initcalll\n");
+
+	/* Now one final round, reporting any devices that remain deferred */
+	driver_deferred_probe_report = true;
+	driver_deferred_probe_trigger();
+	/* Sort as many dependencies as possible before exiting initcalls */
+	flush_workqueue(deferred_wq);
+
 	return 0;
 }
 late_initcall(deferred_probe_initcall);
@@ -342,7 +372,8 @@  probe_failed:
 	switch (ret) {
 	case -EPROBE_DEFER:
 		/* Driver requested deferred probing */
-		dev_dbg(dev, "Driver %s requests probe deferral\n", drv->name);
+		dev_err(dev, "Driver %s requests probe deferral\n", drv->name);
+		dev_warn_deferred(dev, "==== Driver %s requests probe deferral\n", drv->name);
 		driver_deferred_probe_add(dev);
 		/* Did a trigger occur while probing? Need to re-trigger if yes */
 		if (local_trigger_count != atomic_read(&deferred_trigger_count))