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 <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Saravana Kannan <[email protected]>
Cc: Andy Shevchenko <[email protected]>
---
.../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
On Fri, Mar 27, 2020 at 10:01 AM Grant Likely <[email protected]> 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 <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Saravana Kannan <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> ---
> .../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
On 27/03/2020 18:10, Saravana Kannan wrote:
> On Fri, Mar 27, 2020 at 10:01 AM Grant Likely <[email protected]> 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 <[email protected]>
>> Cc: Jonathan Corbet <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: Saravana Kannan <[email protected]>
>> Cc: Andy Shevchenko <[email protected]>
>> ---
>> .../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.
On Fri, Mar 27, 2020 at 4:25 PM Grant Likely <[email protected]> wrote:
>
>
>
> On 27/03/2020 18:10, Saravana Kannan wrote:
> > On Fri, Mar 27, 2020 at 10:01 AM Grant Likely <[email protected]> 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 <[email protected]>
> >> Cc: Jonathan Corbet <[email protected]>
> >> Cc: Greg Kroah-Hartman <[email protected]>
> >> Cc: Saravana Kannan <[email protected]>
> >> Cc: Andy Shevchenko <[email protected]>
> >> ---
> >> .../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
On Sat, Mar 28, 2020 at 1:57 AM Saravana Kannan <[email protected]> wrote:
> On Fri, Mar 27, 2020 at 4:25 PM Grant Likely <[email protected]> wrote:
> > On 27/03/2020 18:10, Saravana Kannan wrote:
> > > On Fri, Mar 27, 2020 at 10:01 AM Grant Likely <[email protected]> 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
On Fri, 27 Mar 2020 16:55:34 -0700
Saravana Kannan <[email protected]> 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
On Sat, Mar 28, 2020 at 10:03 AM Jonathan Corbet <[email protected]> wrote:
>
> On Fri, 27 Mar 2020 16:55:34 -0700
> Saravana Kannan <[email protected]> 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
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 <[email protected]>
> Signed-off-by: Grant Likely <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Saravana Kannan <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> ---
> .../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
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
On Tue, Mar 31, 2020 at 7:46 PM Greg Kroah-Hartman
<[email protected]> 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