diff mbox series

[v3,2/2] driver core: Make deferred_probe_timeout global so it can be shared

Message ID 20200218220748.54823-2-john.stultz@linaro.org (mailing list archive)
State Superseded, archived
Headers show
Series [v3,1/2] driver core: Rework logic in __driver_deferred_probe_check_state to allow EPROBE_DEFER to be returned for longer | expand

Commit Message

John Stultz Feb. 18, 2020, 10:07 p.m. UTC
This patch, suggested by Rob, allows deferred_probe_timeout to
be global so other substems can use it.

This also sets the default to 30 instead of -1 (no timeout) and
modifies the regulator code to make use of it instead of its
hard-coded 30 second interval.

In the case that deferred_probe_timeout is manually set to -1,
we preserve the regulator's hard coded 30 second interval (just
to be cautious this doesn't change behavior in that case).

Feedback would be apprecaited!

Cc: Rob Herring <robh@kernel.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Len Brown <len.brown@intel.com>
Cc: Todd Kjos <tkjos@google.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/base/dd.c             |  4 +++-
 drivers/regulator/core.c      | 12 ++++++++----
 include/linux/device/driver.h |  1 +
 3 files changed, 12 insertions(+), 5 deletions(-)

Comments

Greg KH Feb. 19, 2020, 7:57 a.m. UTC | #1
On Tue, Feb 18, 2020 at 10:07:48PM +0000, John Stultz wrote:
> --- a/include/linux/device/driver.h
> +++ b/include/linux/device/driver.h
> @@ -236,6 +236,7 @@ driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev)
>  }
>  #endif
>  
> +extern int deferred_probe_timeout;
>  void driver_deferred_probe_add(struct device *dev);

If this is going to be global now, can you rename it to
"driver_defferred_probe_timeout" to make it more in line with the other
exported symbols here?

thanks,

greg k-h
Randy Dunlap Feb. 19, 2020, 8 a.m. UTC | #2
On 2/18/20 11:57 PM, Greg Kroah-Hartman wrote:
> On Tue, Feb 18, 2020 at 10:07:48PM +0000, John Stultz wrote:
>> --- a/include/linux/device/driver.h
>> +++ b/include/linux/device/driver.h
>> @@ -236,6 +236,7 @@ driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev)
>>  }
>>  #endif
>>  
>> +extern int deferred_probe_timeout;
>>  void driver_deferred_probe_add(struct device *dev);
> 
> If this is going to be global now, can you rename it to
> "driver_defferred_probe_timeout" to make it more in line with the other

or driver_deferred_probe_timeout please.

> exported symbols here?
Greg KH Feb. 19, 2020, 8:15 a.m. UTC | #3
On Wed, Feb 19, 2020 at 12:00:09AM -0800, Randy Dunlap wrote:
> On 2/18/20 11:57 PM, Greg Kroah-Hartman wrote:
> > On Tue, Feb 18, 2020 at 10:07:48PM +0000, John Stultz wrote:
> >> --- a/include/linux/device/driver.h
> >> +++ b/include/linux/device/driver.h
> >> @@ -236,6 +236,7 @@ driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev)
> >>  }
> >>  #endif
> >>  
> >> +extern int deferred_probe_timeout;
> >>  void driver_deferred_probe_add(struct device *dev);
> > 
> > If this is going to be global now, can you rename it to
> > "driver_defferred_probe_timeout" to make it more in line with the other
> 
> or driver_deferred_probe_timeout please.

Yes, that's spelt better :)
Mark Brown Feb. 19, 2020, 11:59 a.m. UTC | #4
On Tue, Feb 18, 2020 at 10:07:48PM +0000, John Stultz wrote:
> This patch, suggested by Rob, allows deferred_probe_timeout to
> be global so other substems can use it.

> This also sets the default to 30 instead of -1 (no timeout) and
> modifies the regulator code to make use of it instead of its
> hard-coded 30 second interval.

This is at least two patches, one adding the new feature and the other
adding a user of that feature.

> @@ -5767,18 +5772,17 @@ static int __init regulator_init_complete(void)
>  		has_full_constraints = true;
>  
>  	/*
> -	 * We punt completion for an arbitrary amount of time since
> +	 * We punt completion for deferred_probe_timeout seconds since
>  	 * systems like distros will load many drivers from userspace
>  	 * so consumers might not always be ready yet, this is
>  	 * particularly an issue with laptops where this might bounce

While I don't see it doing any harm I'm not 100% convinced by this
change - we're not really doing anything directly to do with deferred
probe here, we're shutting off regulators that remain unused late in
boot but even then they'll still be available for use.  It feels a bit
unclear and the way you've adapted the code to always have a timeout
even if the deferred probe timeout gets changed feels a bit off.  If
nothing else this comment needs more of an update than you've done.
John Stultz Feb. 19, 2020, 9:15 p.m. UTC | #5
On Wed, Feb 19, 2020 at 3:59 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Feb 18, 2020 at 10:07:48PM +0000, John Stultz wrote:
> > This patch, suggested by Rob, allows deferred_probe_timeout to
> > be global so other substems can use it.
>
> > This also sets the default to 30 instead of -1 (no timeout) and
> > modifies the regulator code to make use of it instead of its
> > hard-coded 30 second interval.
>
> This is at least two patches, one adding the new feature and the other
> adding a user of that feature.

Yea, agreed. I combined it here just to get input at this point
without flooding folks with a ton of small patches.

> > @@ -5767,18 +5772,17 @@ static int __init regulator_init_complete(void)
> >               has_full_constraints = true;
> >
> >       /*
> > -      * We punt completion for an arbitrary amount of time since
> > +      * We punt completion for deferred_probe_timeout seconds since
> >        * systems like distros will load many drivers from userspace
> >        * so consumers might not always be ready yet, this is
> >        * particularly an issue with laptops where this might bounce
>
> While I don't see it doing any harm I'm not 100% convinced by this
> change - we're not really doing anything directly to do with deferred
> probe here, we're shutting off regulators that remain unused late in
> boot but even then they'll still be available for use.  It feels a bit
> unclear and the way you've adapted the code to always have a timeout
> even if the deferred probe timeout gets changed feels a bit off.  If
> nothing else this comment needs more of an update than you've done.

This was mostly spurred by Rob's suggestion, as it seemed both
subsystems were doing similar timeouts that may need to be customized
(likely around the same amount of time).  I agree it doesn't quite map
perfectly, but I imagine it would be useful to align those in some
way.

I'll likely take a swing first at getting the driver core bits to make
sense, and then I'll come and revisit this shared timeout issue.

thanks
-john
diff mbox series

Patch

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9d916a7b56a6..c8e025a20a9d 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -224,7 +224,9 @@  static int deferred_devs_show(struct seq_file *s, void *data)
 }
 DEFINE_SHOW_ATTRIBUTE(deferred_devs);
 
-static int deferred_probe_timeout = -1;
+int deferred_probe_timeout = 30;
+EXPORT_SYMBOL_GPL(deferred_probe_timeout);
+
 static int __init deferred_probe_timeout_setup(char *str)
 {
 	int timeout;
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d015d99cb59d..889d08e65f19 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5757,6 +5757,11 @@  static DECLARE_DELAYED_WORK(regulator_init_complete_work,
 
 static int __init regulator_init_complete(void)
 {
+	int delay = deferred_probe_timeout;
+
+	/* preserve 30 second interval if deferred_probe_timeout=-1 */
+	if (delay < 0)
+		delay = 30;
 	/*
 	 * Since DT doesn't provide an idiomatic mechanism for
 	 * enabling full constraints and since it's much more natural
@@ -5767,18 +5772,17 @@  static int __init regulator_init_complete(void)
 		has_full_constraints = true;
 
 	/*
-	 * We punt completion for an arbitrary amount of time since
+	 * We punt completion for deferred_probe_timeout seconds since
 	 * systems like distros will load many drivers from userspace
 	 * so consumers might not always be ready yet, this is
 	 * particularly an issue with laptops where this might bounce
 	 * the display off then on.  Ideally we'd get a notification
 	 * from userspace when this happens but we don't so just wait
 	 * a bit and hope we waited long enough.  It'd be better if
-	 * we'd only do this on systems that need it, and a kernel
-	 * command line option might be useful.
+	 * we'd only do this on systems that need it.
 	 */
 	schedule_delayed_work(&regulator_init_complete_work,
-			      msecs_to_jiffies(30000));
+			      delay * HZ);
 
 	return 0;
 }
diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
index 1188260f9a02..b3ff8cb3fbd6 100644
--- a/include/linux/device/driver.h
+++ b/include/linux/device/driver.h
@@ -236,6 +236,7 @@  driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev)
 }
 #endif
 
+extern int deferred_probe_timeout;
 void driver_deferred_probe_add(struct device *dev);
 int driver_deferred_probe_check_state(struct device *dev);
 int driver_deferred_probe_check_state_continue(struct device *dev);