2014-07-28 14:34:37

by Luis Chamberlain

[permalink] [raw]
Subject: [PATCH 1/3] driver core: enable drivers to use deferred probe from init

From: "Luis R. Rodriguez" <[email protected]>

Tetsuo bisected and found that commit 786235ee "kthread: make
kthread_create() killable" modified kthread_create() to bail as
soon as SIGKILL is received. This is causing some issues with
some drivers and at times boot. Joseph then found that failures
occur as the systemd-udevd process sends SIGKILL to modprobe if
probe on a driver takes over 30 seconds. When this happens probe
will fail on any driver, its why booting on some system will fail
if the driver happens to be a storage related driver. Some folks
have suggested fixing this by modifying kthread_create() to not
leave upon SIGKILL [3], upon review Oleg rejected this change and
the discussion was punted out to systemd to see if the default
timeout could be increased from 30 seconds to 120. The opinion of
the systemd maintainers is that the driver's behavior should
be fixed [4]. Linus seems to agree [5], however more recently even
networking drivers have been reported to fail on probe since just
writing the firmware to a device and kicking it can take easy over
60 seconds [6]. Benjamim was able to trace the issues recently
reported on cxgb4 down to the same systemd-udevd 30 second timeout [6].

This is an alternative solution which enables drivers that are
known to take long to use deferred probe workqueue. This avoids
the 30 second timeout and lets us annotate drivers with long
init sequences.

As drivers determine a component is not yet available and needs
to defer probe you'll be notified this happen upon init for each
device but now with a message such as:

pci 0000:03:00.0: Driver cxgb4 requests probe deferral on init

You should see one of these per struct device probed.

[0] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705
[1] https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1297248
[2] http://lists.freedesktop.org/archives/systemd-devel/2014-March/018006.html
[3] http://thread.gmane.org/gmane.linux.ubuntu.devel.kernel.general/39123
[4] http://article.gmane.org/gmane.comp.sysutils.systemd.devel/17860
[5] http://article.gmane.org/gmane.linux.kernel/1671333
[6] https://bugzilla.novell.com/show_bug.cgi?id=877622

Cc: Greg Kroah-Hartman <[email protected]>
Cc: Tetsuo Handa <[email protected]>
Cc: Joseph Salisbury <[email protected]>
Cc: Kay Sievers <[email protected]>
Cc: One Thousand Gnomes <[email protected]>
Cc: Tim Gardner <[email protected]>
Cc: Pierre Fersing <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Benjamin Poirier <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Nagalakshmi Nandigama <[email protected]>
Cc: Praveen Krishnamoorthy <[email protected]>
Cc: Sreekanth Reddy <[email protected]>
Cc: Abhijit Mahajan <[email protected]>
Cc: Hariprasad S <[email protected]>
Cc: Santosh Rastapur <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/base/dd.c | 15 ++++++++++++++-
include/linux/device.h | 12 ++++++++++++
2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index e4ffbcf..7a271dc 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -374,6 +374,19 @@ void wait_for_device_probe(void)
}
EXPORT_SYMBOL_GPL(wait_for_device_probe);

+static int __driver_probe_device(struct device_driver *drv, struct device *dev)
+{
+ if (drv->delay_probe && !dev->init_delayed_probe) {
+ dev_info(dev, "Driver %s requests probe deferral on init\n",
+ drv->name);
+ dev->init_delayed_probe = true;
+ driver_deferred_probe_add(dev);
+ return -EPROBE_DEFER;
+ }
+
+ return really_probe(dev, drv);
+}
+
/**
* driver_probe_device - attempt to bind device & driver together
* @drv: driver to bind a device to
@@ -396,7 +409,7 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
drv->bus->name, __func__, dev_name(dev), drv->name);

pm_runtime_barrier(dev);
- ret = really_probe(dev, drv);
+ ret = __driver_probe_device(drv, dev);
pm_request_idle(dev);

return ret;
diff --git a/include/linux/device.h b/include/linux/device.h
index af424ac..11da1b7 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -200,6 +200,12 @@ extern struct klist *bus_get_device_klist(struct bus_type *bus);
* @owner: The module owner.
* @mod_name: Used for built-in modules.
* @suppress_bind_attrs: Disables bind/unbind via sysfs.
+ * @delay_probe: this driver is requesting a deferred probe since
+ * initialization. This can be desirable if its known the device probe
+ * or initialization takes more than 30 seconds.
+ * @delayed_probe_devs: devices which have gone through a delayed probe. This
+ * is used internally by the driver core to keep track of which devices
+ * have gone through a delayed probe.
* @of_match_table: The open firmware table.
* @acpi_match_table: The ACPI match table.
* @probe: Called to query the existence of a specific device,
@@ -234,6 +240,9 @@ struct device_driver {

bool suppress_bind_attrs; /* disables bind/unbind via sysfs */

+ bool delay_probe; /* requests deferred probe */
+ struct list_head delayed_probe_devs;
+
const struct of_device_id *of_match_table;
const struct acpi_device_id *acpi_match_table;

@@ -715,6 +724,8 @@ struct acpi_dev_node {
*
* @offline_disabled: If set, the device is permanently online.
* @offline: Set after successful invocation of bus type's .offline().
+ * @init_delayed_probe: lets the coore keep track if the device has already
+ * gone through a delayed probe upon init.
*
* At the lowest level, every device in a Linux system is represented by an
* instance of struct device. The device structure contains the information
@@ -793,6 +804,7 @@ struct device {

bool offline_disabled:1;
bool offline:1;
+ bool init_delayed_probe:1;
};

static inline struct device *kobj_to_dev(struct kobject *kobj)
--
2.0.1


2014-07-28 14:34:46

by Luis Chamberlain

[permalink] [raw]
Subject: [PATCH 2/3] cxgb4: ask for deferred probe

From: "Luis R. Rodriguez" <[email protected]>

cxgb4 probe can take up to over 1 minute when the firmware is
is written and installed on the device. Use the new delayed probe
preference so that cxgb4 gets probed on the deferred workqueue.
Without this devices that require the update on firmware will fail
on probe.

Cc: Tetsuo Handa <[email protected]>
Cc: Joseph Salisbury <[email protected]>
Cc: One Thousand Gnomes <[email protected]>
Cc: Tim Gardner <[email protected]>
Cc: Pierre Fersing <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Benjamin Poirier <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Nagalakshmi Nandigama <[email protected]>
Cc: Praveen Krishnamoorthy <[email protected]>
Cc: Sreekanth Reddy <[email protected]>
Cc: Abhijit Mahajan <[email protected]>
Cc: Hariprasad S <[email protected]>
Cc: Santosh Rastapur <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 6b11fde..bb5daaf 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -6400,6 +6400,9 @@ static void remove_one(struct pci_dev *pdev)
}

static struct pci_driver cxgb4_driver = {
+ .driver = {
+ .delay_probe = true,
+ },
.name = KBUILD_MODNAME,
.id_table = cxgb4_pci_tbl,
.probe = init_one,
--
2.0.1

2014-07-28 14:34:54

by Luis Chamberlain

[permalink] [raw]
Subject: [PATCH 3/3] mptsas: ask for deferred probe

From: "Luis R. Rodriguez" <[email protected]>

Its reported that mptsas can at times take over 30 seconds
to recognize SCSI storage devices [0], this is done on the
driver's probe path. Use the the deferred probe workqueue
to allow long init sequences to complete.

[0] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705

Cc: Tetsuo Handa <[email protected]>
Cc: Joseph Salisbury <[email protected]>
Cc: One Thousand Gnomes <[email protected]>
Cc: Tim Gardner <[email protected]>
Cc: Pierre Fersing <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Benjamin Poirier <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Nagalakshmi Nandigama <[email protected]>
Cc: Praveen Krishnamoorthy <[email protected]>
Cc: Sreekanth Reddy <[email protected]>
Cc: Abhijit Mahajan <[email protected]>
Cc: Hariprasad S <[email protected]>
Cc: Santosh Rastapur <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/message/fusion/mptsas.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 711fcb5..3859ac3 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -5382,6 +5382,9 @@ MODULE_DEVICE_TABLE(pci, mptsas_pci_table);


static struct pci_driver mptsas_driver = {
+ .driver = {
+ .delay_probe = true,
+ },
.name = "mptsas",
.id_table = mptsas_pci_table,
.probe = mptsas_probe,
--
2.0.1

2014-07-28 14:51:16

by Takashi Iwai

[permalink] [raw]
Subject: Re: [PATCH 1/3] driver core: enable drivers to use deferred probe from init

At Mon, 28 Jul 2014 07:34:25 -0700,
Luis R. Rodriguez wrote:
>
> From: "Luis R. Rodriguez" <[email protected]>
>
> Tetsuo bisected and found that commit 786235ee "kthread: make
> kthread_create() killable" modified kthread_create() to bail as
> soon as SIGKILL is received. This is causing some issues with
> some drivers and at times boot. Joseph then found that failures
> occur as the systemd-udevd process sends SIGKILL to modprobe if
> probe on a driver takes over 30 seconds. When this happens probe
> will fail on any driver, its why booting on some system will fail
> if the driver happens to be a storage related driver. Some folks
> have suggested fixing this by modifying kthread_create() to not
> leave upon SIGKILL [3], upon review Oleg rejected this change and
> the discussion was punted out to systemd to see if the default
> timeout could be increased from 30 seconds to 120. The opinion of
> the systemd maintainers is that the driver's behavior should
> be fixed [4]. Linus seems to agree [5], however more recently even
> networking drivers have been reported to fail on probe since just
> writing the firmware to a device and kicking it can take easy over
> 60 seconds [6]. Benjamim was able to trace the issues recently
> reported on cxgb4 down to the same systemd-udevd 30 second timeout [6].
>
> This is an alternative solution which enables drivers that are
> known to take long to use deferred probe workqueue. This avoids
> the 30 second timeout and lets us annotate drivers with long
> init sequences.
>
> As drivers determine a component is not yet available and needs
> to defer probe you'll be notified this happen upon init for each
> device but now with a message such as:
>
> pci 0000:03:00.0: Driver cxgb4 requests probe deferral on init
>
> You should see one of these per struct device probed.
>
> [0] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705
> [1] https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1297248
> [2] http://lists.freedesktop.org/archives/systemd-devel/2014-March/018006.html
> [3] http://thread.gmane.org/gmane.linux.ubuntu.devel.kernel.general/39123
> [4] http://article.gmane.org/gmane.comp.sysutils.systemd.devel/17860
> [5] http://article.gmane.org/gmane.linux.kernel/1671333
> [6] https://bugzilla.novell.com/show_bug.cgi?id=877622
>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Tetsuo Handa <[email protected]>
> Cc: Joseph Salisbury <[email protected]>
> Cc: Kay Sievers <[email protected]>
> Cc: One Thousand Gnomes <[email protected]>
> Cc: Tim Gardner <[email protected]>
> Cc: Pierre Fersing <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Oleg Nesterov <[email protected]>
> Cc: Benjamin Poirier <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Nagalakshmi Nandigama <[email protected]>
> Cc: Praveen Krishnamoorthy <[email protected]>
> Cc: Sreekanth Reddy <[email protected]>
> Cc: Abhijit Mahajan <[email protected]>
> Cc: Hariprasad S <[email protected]>
> Cc: Santosh Rastapur <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Luis R. Rodriguez <[email protected]>
> ---
> drivers/base/dd.c | 15 ++++++++++++++-
> include/linux/device.h | 12 ++++++++++++
> 2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index e4ffbcf..7a271dc 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -374,6 +374,19 @@ void wait_for_device_probe(void)
> }
> EXPORT_SYMBOL_GPL(wait_for_device_probe);
>
> +static int __driver_probe_device(struct device_driver *drv, struct device *dev)
> +{
> + if (drv->delay_probe && !dev->init_delayed_probe) {
> + dev_info(dev, "Driver %s requests probe deferral on init\n",
> + drv->name);
> + dev->init_delayed_probe = true;
> + driver_deferred_probe_add(dev);
> + return -EPROBE_DEFER;
> + }
> +
> + return really_probe(dev, drv);
> +}
> +
> /**
> * driver_probe_device - attempt to bind device & driver together
> * @drv: driver to bind a device to
> @@ -396,7 +409,7 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
> drv->bus->name, __func__, dev_name(dev), drv->name);
>
> pm_runtime_barrier(dev);
> - ret = really_probe(dev, drv);
> + ret = __driver_probe_device(drv, dev);
> pm_request_idle(dev);
>
> return ret;
> diff --git a/include/linux/device.h b/include/linux/device.h
> index af424ac..11da1b7 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -200,6 +200,12 @@ extern struct klist *bus_get_device_klist(struct bus_type *bus);
> * @owner: The module owner.
> * @mod_name: Used for built-in modules.
> * @suppress_bind_attrs: Disables bind/unbind via sysfs.
> + * @delay_probe: this driver is requesting a deferred probe since
> + * initialization. This can be desirable if its known the device probe
> + * or initialization takes more than 30 seconds.
> + * @delayed_probe_devs: devices which have gone through a delayed probe. This
> + * is used internally by the driver core to keep track of which devices
> + * have gone through a delayed probe.
> * @of_match_table: The open firmware table.
> * @acpi_match_table: The ACPI match table.
> * @probe: Called to query the existence of a specific device,
> @@ -234,6 +240,9 @@ struct device_driver {
>
> bool suppress_bind_attrs; /* disables bind/unbind via sysfs */
>
> + bool delay_probe; /* requests deferred probe */
> + struct list_head delayed_probe_devs;
> +

This field isn't used anywhere actually in the patch...?


Takashi

> const struct of_device_id *of_match_table;
> const struct acpi_device_id *acpi_match_table;
>
> @@ -715,6 +724,8 @@ struct acpi_dev_node {
> *
> * @offline_disabled: If set, the device is permanently online.
> * @offline: Set after successful invocation of bus type's .offline().
> + * @init_delayed_probe: lets the coore keep track if the device has already
> + * gone through a delayed probe upon init.
> *
> * At the lowest level, every device in a Linux system is represented by an
> * instance of struct device. The device structure contains the information
> @@ -793,6 +804,7 @@ struct device {
>
> bool offline_disabled:1;
> bool offline:1;
> + bool init_delayed_probe:1;
> };
>
> static inline struct device *kobj_to_dev(struct kobject *kobj)
> --
> 2.0.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

2014-07-28 15:01:47

by Luis Chamberlain

[permalink] [raw]
Subject: Re: [PATCH 1/3] driver core: enable drivers to use deferred probe from init

On Mon, Jul 28, 2014 at 7:51 AM, Takashi Iwai <[email protected]> wrote:
>
> This field isn't used anywhere actually in the patch...?

Yeah sorry that is not needed, I avoided that by adding the simple
bool on the struct device, forgot to nuke that. Will spin v2 unless I
hear otherwise.

Luis

2014-07-28 15:12:52

by Yuval Mintz

[permalink] [raw]
Subject: RE: [PATCH 1/3] driver core: enable drivers to use deferred probe from init

> +static int __driver_probe_device(struct device_driver *drv, struct
> +device *dev) {
> + if (drv->delay_probe && !dev->init_delayed_probe) {
> + dev_info(dev, "Driver %s requests probe deferral on init\n",
> + drv->name);
> + dev->init_delayed_probe = true;
> + driver_deferred_probe_add(dev);
> + return -EPROBE_DEFER;
> + }
> +
> + return really_probe(dev, drv);
> +}

Perhaps this is a silly question, but what guarantees that the deferred probe
list will actually be triggered, e.g., in case the delayed device is the last device
in the system?

[From drivers/base/dd.c - "A successful driver probe will trigger moving all
devices from the pending to the active list so that the workqueue will
eventually retry them]

2014-07-28 15:38:17

by Luis Chamberlain

[permalink] [raw]
Subject: Re: [PATCH 1/3] driver core: enable drivers to use deferred probe from init

On Mon, Jul 28, 2014 at 03:12:11PM +0000, Yuval Mintz wrote:
> > +static int __driver_probe_device(struct device_driver *drv, struct
> > +device *dev) {
> > + if (drv->delay_probe && !dev->init_delayed_probe) {
> > + dev_info(dev, "Driver %s requests probe deferral on init\n",
> > + drv->name);
> > + dev->init_delayed_probe = true;
> > + driver_deferred_probe_add(dev);
> > + return -EPROBE_DEFER;
> > + }
> > +
> > + return really_probe(dev, drv);
> > +}
>
> Perhaps this is a silly question, but what guarantees that the deferred probe
> list will actually be triggered, e.g., in case the delayed device is the last device
> in the system?

The dev->init_delayed_probe is used to ensure that we'd add the device to the
deferred probe list once making this a per device thing if the driver has the
field delay_probe set to true. This technically also allows this to be a per
device thing so with some more work we could enable drivers to only enable this
for specific devices but at this point this did not seem required.

> [From drivers/base/dd.c - "A successful driver probe will trigger moving all
> devices from the pending to the active list so that the workqueue will
> eventually retry them]

I had not noticed this, thanks for pointing this out, in this case
if __driver_probe_device() is still used to retrigger a probe it will
be added back to the deferred list but since dev->init_delayed_probe
is still false. I checked the original commit that added this feature
d1c3414c but in the code I see that bus_probe_device(dev) is used and
I only see the device itself being removed from the deferred probe list,
nothing else.

Luis

2014-07-28 15:47:13

by Yuval Mintz

[permalink] [raw]
Subject: RE: [PATCH 1/3] driver core: enable drivers to use deferred probe from init

> Subject: Re: [PATCH 1/3] driver core: enable drivers to use deferred probe from
> init
>
> On Mon, Jul 28, 2014 at 03:12:11PM +0000, Yuval Mintz wrote:
> > Perhaps this is a silly question, but what guarantees that the
> > deferred probe list will actually be triggered, e.g., in case the
> > delayed device is the last device in the system?
>
> The dev->init_delayed_probe is used to ensure that we'd add the device to the
> deferred probe list once making this a per device thing if the driver has the field
> delay_probe set to true. This technically also allows this to be a per device thing
> so with some more work we could enable drivers to only enable this for specific
> devices but at this point this did not seem required.

Sorry for not being clear, but I didn't meant 'what guarantees that the device
will be added to the deferred probe', but rather what guarantees that the
deferred workqueue will be scheduled.

To the best of my knowledge the deferring mechanism works only if one device
is dependent upon another, e.g., for Multi-function devices where one device
probe is dependent upon the others - which are soon-to-be probed.

[If I'm incorrect I'd appreciate if someone could correct me]

>
> > [From drivers/base/dd.c - "A successful driver probe will trigger
> > moving all devices from the pending to the active list so that the
> > workqueue will eventually retry them]

2014-07-28 16:52:55

by Luis Chamberlain

[permalink] [raw]
Subject: Re: [PATCH 1/3] driver core: enable drivers to use deferred probe from init

On Mon, Jul 28, 2014 at 03:46:32PM +0000, Yuval Mintz wrote:
> > Subject: Re: [PATCH 1/3] driver core: enable drivers to use deferred probe from
> > init
> >
> > On Mon, Jul 28, 2014 at 03:12:11PM +0000, Yuval Mintz wrote:
> > > Perhaps this is a silly question, but what guarantees that the
> > > deferred probe list will actually be triggered, e.g., in case the
> > > delayed device is the last device in the system?
> >
> > The dev->init_delayed_probe is used to ensure that we'd add the device to the
> > deferred probe list once making this a per device thing if the driver has the field
> > delay_probe set to true. This technically also allows this to be a per device thing
> > so with some more work we could enable drivers to only enable this for specific
> > devices but at this point this did not seem required.
>
> Sorry for not being clear, but I didn't meant 'what guarantees that the device
> will be added to the deferred probe', but rather what guarantees that the
> deferred workqueue will be scheduled.
>
> To the best of my knowledge the deferring mechanism works only if one device
> is dependent upon another, e.g., for Multi-function devices where one device
> probe is dependent upon the others - which are soon-to-be probed.

The workqueue will be kicked when driver_deferred_probe_trigger() gets
poked, we do that in the late_initcall(deferred_probe_initcall), it
also gets flushed there with a flush_workqueue(deferred_wq).

Luis

2014-07-28 16:59:25

by Luis Chamberlain

[permalink] [raw]
Subject: Re: [PATCH 1/3] driver core: enable drivers to use deferred probe from init

On Mon, Jul 28, 2014 at 06:52:48PM +0200, Luis R. Rodriguez wrote:
> On Mon, Jul 28, 2014 at 03:46:32PM +0000, Yuval Mintz wrote:
> > > Subject: Re: [PATCH 1/3] driver core: enable drivers to use deferred probe from
> > > init
> > >
> > > On Mon, Jul 28, 2014 at 03:12:11PM +0000, Yuval Mintz wrote:
> > > > Perhaps this is a silly question, but what guarantees that the
> > > > deferred probe list will actually be triggered, e.g., in case the
> > > > delayed device is the last device in the system?
> > >
> > > The dev->init_delayed_probe is used to ensure that we'd add the device to the
> > > deferred probe list once making this a per device thing if the driver has the field
> > > delay_probe set to true. This technically also allows this to be a per device thing
> > > so with some more work we could enable drivers to only enable this for specific
> > > devices but at this point this did not seem required.
> >
> > Sorry for not being clear, but I didn't meant 'what guarantees that the device
> > will be added to the deferred probe', but rather what guarantees that the
> > deferred workqueue will be scheduled.
> >
> > To the best of my knowledge the deferring mechanism works only if one device
> > is dependent upon another, e.g., for Multi-function devices where one device
> > probe is dependent upon the others - which are soon-to-be probed.
>
> The workqueue will be kicked when driver_deferred_probe_trigger() gets
> poked, we do that in the late_initcall(deferred_probe_initcall), it
> also gets flushed there with a flush_workqueue(deferred_wq).

But come to think of it that will work well for devices already plugged in
so indeed I think that driver_deferred_probe_add() needs a check added
for for if (!driver_deferred_probe_enable) then we have to
driver_deferred_probe_trigger(). The driver_deferred_probe_enable is set
to false upon init, but later on late init it gets set to true so with
that check we'd only generate the trigger after late init call.

I can fold that in the v2.

Luis

2014-07-28 18:31:17

by Yuval Mintz

[permalink] [raw]
Subject: RE: [PATCH 1/3] driver core: enable drivers to use deferred probe from init

> On Mon, Jul 28, 2014 at 06:52:48PM +0200, Luis R. Rodriguez wrote:
> > On Mon, Jul 28, 2014 at 03:46:32PM +0000, Yuval Mintz wrote:
> > > Sorry for not being clear, but I didn't meant 'what guarantees that the device
> > > will be added to the deferred probe', but rather what guarantees that the
> > > deferred workqueue will be scheduled.
> > >
> > > To the best of my knowledge the deferring mechanism works only if one device
> > > is dependent upon another, e.g., for Multi-function devices where one device
> > > probe is dependent upon the others - which are soon-to-be probed.
> >
> > The workqueue will be kicked when driver_deferred_probe_trigger() gets
> > poked, we do that in the late_initcall(deferred_probe_initcall), it
> > also gets flushed there with a flush_workqueue(deferred_wq).

> But come to think of it that will work well for devices already plugged in
> so indeed I think that driver_deferred_probe_add() needs a check added
> for for if (!driver_deferred_probe_enable) then we have to
> driver_deferred_probe_trigger(). The driver_deferred_probe_enable is set
> to false upon init, but later on late init it gets set to true so with
> that check we'd only generate the trigger after late init call.

> I can fold that in the v2.

> Luis

But what about modules being added after the init-calls? If they try try to use this
mechanism, what guarantees they'll eventually get probed?

2014-07-28 18:49:05

by Luis Chamberlain

[permalink] [raw]
Subject: Re: [PATCH 1/3] driver core: enable drivers to use deferred probe from init

On Mon, Jul 28, 2014 at 06:30:29PM +0000, Yuval Mintz wrote:
> > On Mon, Jul 28, 2014 at 06:52:48PM +0200, Luis R. Rodriguez wrote:
> > > On Mon, Jul 28, 2014 at 03:46:32PM +0000, Yuval Mintz wrote:
> > > > Sorry for not being clear, but I didn't meant 'what guarantees that the device
> > > > will be added to the deferred probe', but rather what guarantees that the
> > > > deferred workqueue will be scheduled.
> > > >
> > > > To the best of my knowledge the deferring mechanism works only if one device
> > > > is dependent upon another, e.g., for Multi-function devices where one device
> > > > probe is dependent upon the others - which are soon-to-be probed.
> > >
> > > The workqueue will be kicked when driver_deferred_probe_trigger() gets
> > > poked, we do that in the late_initcall(deferred_probe_initcall), it
> > > also gets flushed there with a flush_workqueue(deferred_wq).
>
> > But come to think of it that will work well for devices already plugged in
> > so indeed I think that driver_deferred_probe_add() needs a check added
> > for for if (!driver_deferred_probe_enable) then we have to
> > driver_deferred_probe_trigger(). The driver_deferred_probe_enable is set
> > to false upon init, but later on late init it gets set to true so with
> > that check we'd only generate the trigger after late init call.
>
> > I can fold that in the v2.
>
> > Luis
>
> But what about modules being added after the init-calls? If they try try to use this
> mechanism, what guarantees they'll eventually get probed?

bus_probe_device --> device_attach() -->
__device_attach() --> driver_probe_device() -->
__driver_probe_device() --> driver_deferred_probe_add()

And with the new hunk I mentioned I'd add then we'd trigger the
workqueue if its after late init. The change is in v2 series.

Luis