[PATCH] Add documentation on meaning of -EPROBE_DEFER

[PATCH] Add documentation on meaning of -EPROBE_DEFER

[Grant Likely]

Add a bit of documentation on what it means when a driver .probe() hook
returns the -EPROBE_DEFER error code, including the limitation that
-EPROBE_DEFER should be returned as early as possible, before the driver
starts to register child devices.

Also: minor markup fixes in the same file

Signed-off-by: Grant Likely <grant.likely@arm.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Saravana Kannan <saravanak@google.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 .../driver-api/driver-model/driver.rst        | 32 ++++++++++++++++---
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/Documentation/driver-api/driver-model/driver.rst b/Documentation/driver-api/driver-model/driver.rst
index baa6a85c8287..63057d9bc8a6 100644
--- a/Documentation/driver-api/driver-model/driver.rst
+++ b/Documentation/driver-api/driver-model/driver.rst
@@ -4,7 +4,6 @@ Device Drivers
 
 See the kerneldoc for the struct device_driver.
 
-
 Allocation
 ~~~~~~~~~~
 
@@ -167,9 +166,26 @@ the driver to that device.
 
 A driver's probe() may return a negative errno value to indicate that
 the driver did not bind to this device, in which case it should have
-released all resources it allocated::
+released all resources it allocated.
+
+Optionally, probe() may return -EPROBE_DEFER if the driver depends on
+resources that are not yet available (e.g., supplied by a driver that
+hasn't initialized yet).  The driver core will put the device onto the
+deferred probe list and will try to call it again later. If a driver
+must defer, it should return -EPROBE_DEFER as early as possible to
+reduce the amount of time spent on setup work that will need to be
+unwound and reexecuted at a later time.
+
+.. warning::
+      -EPROBE_DEFER must not be returned if probe() has already created
+      child devices, even if those child devices are removed again
+      in a cleanup path. If -EPROBE_DEFER is returned after a child
+      device has been registered, it may result in an infinite loop of
+      .probe() calls to the same driver.
+
+::
 
-	void (*sync_state)(struct device *dev);
+	void	(*sync_state)	(struct device *dev);
 
 sync_state is called only once for a device. It's called when all the consumer
 devices of the device have successfully probed. The list of consumers of the
@@ -212,6 +228,8 @@ over management of devices from the bootloader, the usage of sync_state() is
 not restricted to that. Use it whenever it makes sense to take an action after
 all the consumers of a device have probed.
 
+::
+
 	int 	(*remove)	(struct device *dev);
 
 remove is called to unbind a driver from a device. This may be
@@ -224,11 +242,15 @@ not. It should free any resources allocated specifically for the
 device; i.e. anything in the device's driver_data field.
 
 If the device is still present, it should quiesce the device and place
-it into a supported low-power state::
+it into a supported low-power state.
+
+::
 
 	int	(*suspend)	(struct device *dev, pm_message_t state);
 
-suspend is called to put the device in a low power state::
+suspend is called to put the device in a low power state.
+
+::
 
 	int	(*resume)	(struct device *dev);
 
-- 
2.20.1

Re: [PATCH] Add documentation on meaning of -EPROBE_DEFER

[Saravana Kannan]

On Fri, Mar 27, 2020 at 10:01 AM Grant Likely <grant.likely@arm.com> wrote:
>
> Add a bit of documentation on what it means when a driver .probe() hook
> returns the -EPROBE_DEFER error code, including the limitation that
> -EPROBE_DEFER should be returned as early as possible, before the driver
> starts to register child devices.
>
> Also: minor markup fixes in the same file
>
> Signed-off-by: Grant Likely <grant.likely@arm.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Saravana Kannan <saravanak@google.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  .../driver-api/driver-model/driver.rst        | 32 ++++++++++++++++---
>  1 file changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/driver-api/driver-model/driver.rst b/Documentation/driver-api/driver-model/driver.rst
> index baa6a85c8287..63057d9bc8a6 100644
> --- a/Documentation/driver-api/driver-model/driver.rst
> +++ b/Documentation/driver-api/driver-model/driver.rst
> @@ -4,7 +4,6 @@ Device Drivers
>
>  See the kerneldoc for the struct device_driver.
>
> -
>  Allocation
>  ~~~~~~~~~~
>
> @@ -167,9 +166,26 @@ the driver to that device.
>
>  A driver's probe() may return a negative errno value to indicate that
>  the driver did not bind to this device, in which case it should have
> -released all resources it allocated::
> +released all resources it allocated.
> +
> +Optionally, probe() may return -EPROBE_DEFER if the driver depends on
> +resources that are not yet available (e.g., supplied by a driver that
> +hasn't initialized yet).  The driver core will put the device onto the
> +deferred probe list and will try to call it again later. If a driver
> +must defer, it should return -EPROBE_DEFER as early as possible to
> +reduce the amount of time spent on setup work that will need to be
> +unwound and reexecuted at a later time.
> +
> +.. warning::
> +      -EPROBE_DEFER must not be returned if probe() has already created
> +      child devices, even if those child devices are removed again
> +      in a cleanup path. If -EPROBE_DEFER is returned after a child
> +      device has been registered, it may result in an infinite loop of
> +      .probe() calls to the same driver.

The infinite loop is a current implementation behavior. Not an
intentional choice. So, maybe we can say the behavior is undefined
instead?

-Saravana

Re: [PATCH] Add documentation on meaning of -EPROBE_DEFER

[Grant Likely]

On 27/03/2020 18:10, Saravana Kannan wrote:
> On Fri, Mar 27, 2020 at 10:01 AM Grant Likely <grant.likely@arm.com> wrote:
>>
>> Add a bit of documentation on what it means when a driver .probe() hook
>> returns the -EPROBE_DEFER error code, including the limitation that
>> -EPROBE_DEFER should be returned as early as possible, before the driver
>> starts to register child devices.
>>
>> Also: minor markup fixes in the same file
>>
>> Signed-off-by: Grant Likely <grant.likely@arm.com>
>> Cc: Jonathan Corbet <corbet@lwn.net>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Saravana Kannan <saravanak@google.com>
>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>   .../driver-api/driver-model/driver.rst        | 32 ++++++++++++++++---
>>   1 file changed, 27 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/driver-api/driver-model/driver.rst b/Documentation/driver-api/driver-model/driver.rst
>> index baa6a85c8287..63057d9bc8a6 100644
>> --- a/Documentation/driver-api/driver-model/driver.rst
>> +++ b/Documentation/driver-api/driver-model/driver.rst
>> @@ -4,7 +4,6 @@ Device Drivers
>>
>>   See the kerneldoc for the struct device_driver.
>>
>> -
>>   Allocation
>>   ~~~~~~~~~~
>>
>> @@ -167,9 +166,26 @@ the driver to that device.
>>
>>   A driver's probe() may return a negative errno value to indicate that
>>   the driver did not bind to this device, in which case it should have
>> -released all resources it allocated::
>> +released all resources it allocated.
>> +
>> +Optionally, probe() may return -EPROBE_DEFER if the driver depends on
>> +resources that are not yet available (e.g., supplied by a driver that
>> +hasn't initialized yet).  The driver core will put the device onto the
>> +deferred probe list and will try to call it again later. If a driver
>> +must defer, it should return -EPROBE_DEFER as early as possible to
>> +reduce the amount of time spent on setup work that will need to be
>> +unwound and reexecuted at a later time.
>> +
>> +.. warning::
>> +      -EPROBE_DEFER must not be returned if probe() has already created
>> +      child devices, even if those child devices are removed again
>> +      in a cleanup path. If -EPROBE_DEFER is returned after a child
>> +      device has been registered, it may result in an infinite loop of
>> +      .probe() calls to the same driver.
> 
> The infinite loop is a current implementation behavior. Not an
> intentional choice. So, maybe we can say the behavior is undefined
> instead?

If you feel strongly about it, but I don't have any problem with 
documenting it as the current implementation behaviour, and then 
changing the text if that ever changes.

g.

Re: [PATCH] Add documentation on meaning of -EPROBE_DEFER

[Saravana Kannan]

On Fri, Mar 27, 2020 at 4:25 PM Grant Likely <grant.likely@arm.com> wrote:
>
>
>
> On 27/03/2020 18:10, Saravana Kannan wrote:
> > On Fri, Mar 27, 2020 at 10:01 AM Grant Likely <grant.likely@arm.com> wrote:
> >>
> >> Add a bit of documentation on what it means when a driver .probe() hook
> >> returns the -EPROBE_DEFER error code, including the limitation that
> >> -EPROBE_DEFER should be returned as early as possible, before the driver
> >> starts to register child devices.
> >>
> >> Also: minor markup fixes in the same file
> >>
> >> Signed-off-by: Grant Likely <grant.likely@arm.com>
> >> Cc: Jonathan Corbet <corbet@lwn.net>
> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> Cc: Saravana Kannan <saravanak@google.com>
> >> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >> ---
> >>   .../driver-api/driver-model/driver.rst        | 32 ++++++++++++++++---
> >>   1 file changed, 27 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/Documentation/driver-api/driver-model/driver.rst b/Documentation/driver-api/driver-model/driver.rst
> >> index baa6a85c8287..63057d9bc8a6 100644
> >> --- a/Documentation/driver-api/driver-model/driver.rst
> >> +++ b/Documentation/driver-api/driver-model/driver.rst
> >> @@ -4,7 +4,6 @@ Device Drivers
> >>
> >>   See the kerneldoc for the struct device_driver.
> >>
> >> -
> >>   Allocation
> >>   ~~~~~~~~~~
> >>
> >> @@ -167,9 +166,26 @@ the driver to that device.
> >>
> >>   A driver's probe() may return a negative errno value to indicate that
> >>   the driver did not bind to this device, in which case it should have
> >> -released all resources it allocated::
> >> +released all resources it allocated.
> >> +
> >> +Optionally, probe() may return -EPROBE_DEFER if the driver depends on
> >> +resources that are not yet available (e.g., supplied by a driver that
> >> +hasn't initialized yet).  The driver core will put the device onto the
> >> +deferred probe list and will try to call it again later. If a driver
> >> +must defer, it should return -EPROBE_DEFER as early as possible to
> >> +reduce the amount of time spent on setup work that will need to be
> >> +unwound and reexecuted at a later time.
> >> +
> >> +.. warning::
> >> +      -EPROBE_DEFER must not be returned if probe() has already created
> >> +      child devices, even if those child devices are removed again
> >> +      in a cleanup path. If -EPROBE_DEFER is returned after a child
> >> +      device has been registered, it may result in an infinite loop of
> >> +      .probe() calls to the same driver.
> >
> > The infinite loop is a current implementation behavior. Not an
> > intentional choice. So, maybe we can say the behavior is undefined
> > instead?
>
> If you feel strongly about it, but I don't have any problem with
> documenting it as the current implementation behaviour, and then
> changing the text if that ever changes.

Assuming Greg is okay with this doc update, I'm kinda leaning towards
"undefined" because if documented as "infinite loop" people might be
hesitant towards removing that behavior. But I'll let Greg make the
final call. Not going to NACK for this point.

-Saravana

Re: [PATCH] Add documentation on meaning of -EPROBE_DEFER

[Andy Shevchenko]

On Sat, Mar 28, 2020 at 1:57 AM Saravana Kannan <saravanak@google.com> wrote:
> On Fri, Mar 27, 2020 at 4:25 PM Grant Likely <grant.likely@arm.com> wrote:
> > On 27/03/2020 18:10, Saravana Kannan wrote:
> > > On Fri, Mar 27, 2020 at 10:01 AM Grant Likely <grant.likely@arm.com> wrote:
> > >>
> > >> Add a bit of documentation on what it means when a driver .probe() hook
> > >> returns the -EPROBE_DEFER error code, including the limitation that
> > >> -EPROBE_DEFER should be returned as early as possible, before the driver
> > >> starts to register child devices.

...

> > >> +Optionally, probe() may return -EPROBE_DEFER if the driver depends on
> > >> +resources that are not yet available (e.g., supplied by a driver that
> > >> +hasn't initialized yet).  The driver core will put the device onto the
> > >> +deferred probe list and will try to call it again later. If a driver
> > >> +must defer, it should return -EPROBE_DEFER as early as possible to
> > >> +reduce the amount of time spent on setup work that will need to be
> > >> +unwound and reexecuted at a later time.
> > >> +
> > >> +.. warning::
> > >> +      -EPROBE_DEFER must not be returned if probe() has already created
> > >> +      child devices, even if those child devices are removed again
> > >> +      in a cleanup path. If -EPROBE_DEFER is returned after a child
> > >> +      device has been registered, it may result in an infinite loop of
> > >> +      .probe() calls to the same driver.
> > >
> > > The infinite loop is a current implementation behavior. Not an
> > > intentional choice. So, maybe we can say the behavior is undefined
> > > instead?

Why? *Good* documentation must describe the actual behaviour, not hide it.

> > If you feel strongly about it, but I don't have any problem with
> > documenting it as the current implementation behaviour, and then
> > changing the text if that ever changes.
>
> Assuming Greg is okay with this doc update, I'm kinda leaning towards
> "undefined"

I think it should not distort the reality.

> because if documented as "infinite loop" people might be
> hesitant towards removing that behavior.

This is funny argument. Won't we do kernel better?

>  But I'll let Greg make the
> final call. Not going to NACK for this point.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] Add documentation on meaning of -EPROBE_DEFER

[Jonathan Corbet]

On Fri, 27 Mar 2020 16:55:34 -0700
Saravana Kannan <saravanak@google.com> wrote:

> > > The infinite loop is a current implementation behavior. Not an
> > > intentional choice. So, maybe we can say the behavior is undefined
> > > instead?  
> >
> > If you feel strongly about it, but I don't have any problem with
> > documenting it as the current implementation behaviour, and then
> > changing the text if that ever changes.  
> 
> Assuming Greg is okay with this doc update, I'm kinda leaning towards
> "undefined" because if documented as "infinite loop" people might be
> hesitant towards removing that behavior. But I'll let Greg make the
> final call. Not going to NACK for this point.

FWIW, kernel developers have to cope with enough trouble from "undefined
behavior" already; I don't think we should really be adding that to our
own docs.  We can certainly document the infinite loop behavior as being
not guaranteed as part of the API if we're worried that somebody might
start to rely on it...:)  

Thanks,

jon

Re: [PATCH] Add documentation on meaning of -EPROBE_DEFER

[Saravana Kannan]

On Sat, Mar 28, 2020 at 10:03 AM Jonathan Corbet <corbet@lwn.net> wrote:
>
> On Fri, 27 Mar 2020 16:55:34 -0700
> Saravana Kannan <saravanak@google.com> wrote:
>
> > > > The infinite loop is a current implementation behavior. Not an
> > > > intentional choice. So, maybe we can say the behavior is undefined
> > > > instead?
> > >
> > > If you feel strongly about it, but I don't have any problem with
> > > documenting it as the current implementation behaviour, and then
> > > changing the text if that ever changes.
> >
> > Assuming Greg is okay with this doc update, I'm kinda leaning towards
> > "undefined" because if documented as "infinite loop" people might be
> > hesitant towards removing that behavior. But I'll let Greg make the
> > final call. Not going to NACK for this point.
>
> FWIW, kernel developers have to cope with enough trouble from "undefined
> behavior" already; I don't think we should really be adding that to our
> own docs.  We can certainly document the infinite loop behavior as being
> not guaranteed as part of the API if we're worried that somebody might
> start to rely on it...:)

Ok, all of you have convinced me of the error of my ways. :)

Thanks,
Saravana

Re: [PATCH] Add documentation on meaning of -EPROBE_DEFER

[Andy Shevchenko]

On Fri, Mar 27, 2020 at 05:01:32PM +0000, Grant Likely wrote:
> Add a bit of documentation on what it means when a driver .probe() hook
> returns the -EPROBE_DEFER error code, including the limitation that
> -EPROBE_DEFER should be returned as early as possible, before the driver
> starts to register child devices.
> 
> Also: minor markup fixes in the same file

Greg, can we at least for time being have this documented, means applied?

FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Grant Likely <grant.likely@arm.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Saravana Kannan <saravanak@google.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  .../driver-api/driver-model/driver.rst        | 32 ++++++++++++++++---
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/driver-api/driver-model/driver.rst b/Documentation/driver-api/driver-model/driver.rst
> index baa6a85c8287..63057d9bc8a6 100644
> --- a/Documentation/driver-api/driver-model/driver.rst
> +++ b/Documentation/driver-api/driver-model/driver.rst
> @@ -4,7 +4,6 @@ Device Drivers
>  
>  See the kerneldoc for the struct device_driver.
>  
> -
>  Allocation
>  ~~~~~~~~~~
>  
> @@ -167,9 +166,26 @@ the driver to that device.
>  
>  A driver's probe() may return a negative errno value to indicate that
>  the driver did not bind to this device, in which case it should have
> -released all resources it allocated::
> +released all resources it allocated.
> +
> +Optionally, probe() may return -EPROBE_DEFER if the driver depends on
> +resources that are not yet available (e.g., supplied by a driver that
> +hasn't initialized yet).  The driver core will put the device onto the
> +deferred probe list and will try to call it again later. If a driver
> +must defer, it should return -EPROBE_DEFER as early as possible to
> +reduce the amount of time spent on setup work that will need to be
> +unwound and reexecuted at a later time.
> +
> +.. warning::
> +      -EPROBE_DEFER must not be returned if probe() has already created
> +      child devices, even if those child devices are removed again
> +      in a cleanup path. If -EPROBE_DEFER is returned after a child
> +      device has been registered, it may result in an infinite loop of
> +      .probe() calls to the same driver.
> +
> +::
>  
> -	void (*sync_state)(struct device *dev);
> +	void	(*sync_state)	(struct device *dev);
>  
>  sync_state is called only once for a device. It's called when all the consumer
>  devices of the device have successfully probed. The list of consumers of the
> @@ -212,6 +228,8 @@ over management of devices from the bootloader, the usage of sync_state() is
>  not restricted to that. Use it whenever it makes sense to take an action after
>  all the consumers of a device have probed.
>  
> +::
> +
>  	int 	(*remove)	(struct device *dev);
>  
>  remove is called to unbind a driver from a device. This may be
> @@ -224,11 +242,15 @@ not. It should free any resources allocated specifically for the
>  device; i.e. anything in the device's driver_data field.
>  
>  If the device is still present, it should quiesce the device and place
> -it into a supported low-power state::
> +it into a supported low-power state.
> +
> +::
>  
>  	int	(*suspend)	(struct device *dev, pm_message_t state);
>  
> -suspend is called to put the device in a low power state::
> +suspend is called to put the device in a low power state.
> +
> +::
>  
>  	int	(*resume)	(struct device *dev);
>  
> -- 
> 2.20.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] Add documentation on meaning of -EPROBE_DEFER

[Greg Kroah-Hartman]

On Tue, Mar 31, 2020 at 05:33:55PM +0300, Andy Shevchenko wrote:
> On Fri, Mar 27, 2020 at 05:01:32PM +0000, Grant Likely wrote:
> > Add a bit of documentation on what it means when a driver .probe() hook
> > returns the -EPROBE_DEFER error code, including the limitation that
> > -EPROBE_DEFER should be returned as early as possible, before the driver
> > starts to register child devices.
> > 
> > Also: minor markup fixes in the same file
> 
> Greg, can we at least for time being have this documented, means applied?

It's the middle of the merge window, you know I can't take anything in
my trees until after -rc1 is out.

I will queue it up then, thanks.

greg k-h

Re: [PATCH] Add documentation on meaning of -EPROBE_DEFER

[Andy Shevchenko]

On Tue, Mar 31, 2020 at 7:46 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Mar 31, 2020 at 05:33:55PM +0300, Andy Shevchenko wrote:
> > On Fri, Mar 27, 2020 at 05:01:32PM +0000, Grant Likely wrote:
> > > Add a bit of documentation on what it means when a driver .probe() hook
> > > returns the -EPROBE_DEFER error code, including the limitation that
> > > -EPROBE_DEFER should be returned as early as possible, before the driver
> > > starts to register child devices.
> > >
> > > Also: minor markup fixes in the same file
> >
> > Greg, can we at least for time being have this documented, means applied?
>
> It's the middle of the merge window, you know I can't take anything in
> my trees until after -rc1 is out.

Right.

> I will queue it up then, thanks.

Thank you!

-- 
With Best Regards,
Andy Shevchenko