[PATCH v2 1/4] driver core: move deferred probe add / remove helpers down a bit

[PATCH v2 1/4] driver core: move deferred probe add / remove helpers down a bit

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

We need to do this if we want to use the trigger at a later point.
This change has introduces no functional changes.

Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Joseph Salisbury <joseph.salisbury@canonical.com>
Cc: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
Cc: Tim Gardner <tim.gardner@canonical.com>
Cc: Pierre Fersing <pierre-fersing@pierref.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Benjamin Poirier <bpoirier@suse.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Nagalakshmi Nandigama <nagalakshmi.nandigama@avagotech.com>
Cc: Praveen Krishnamoorthy <praveen.krishnamoorthy@avagotech.com>
Cc: Sreekanth Reddy <sreekanth.reddy@avagotech.com>
Cc: Abhijit Mahajan <abhijit.mahajan@avagotech.com>
Cc: Hariprasad S <hariprasad@chelsio.com>
Cc: Santosh Rastapur <santosh@chelsio.com>
Cc: MPT-FusionLinux.pdl@avagotech.com
Cc: linux-scsi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/base/dd.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index e4ffbcf..9947c2e 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -109,26 +109,6 @@ static void deferred_probe_work_func(struct work_struct *work)
 }
 static DECLARE_WORK(deferred_probe_work, deferred_probe_work_func);
 
-static void driver_deferred_probe_add(struct device *dev)
-{
-	mutex_lock(&deferred_probe_mutex);
-	if (list_empty(&dev->p->deferred_probe)) {
-		dev_dbg(dev, "Added to deferred list\n");
-		list_add_tail(&dev->p->deferred_probe, &deferred_probe_pending_list);
-	}
-	mutex_unlock(&deferred_probe_mutex);
-}
-
-void driver_deferred_probe_del(struct device *dev)
-{
-	mutex_lock(&deferred_probe_mutex);
-	if (!list_empty(&dev->p->deferred_probe)) {
-		dev_dbg(dev, "Removed from deferred list\n");
-		list_del_init(&dev->p->deferred_probe);
-	}
-	mutex_unlock(&deferred_probe_mutex);
-}
-
 static bool driver_deferred_probe_enable = false;
 /**
  * driver_deferred_probe_trigger() - Kick off re-probing deferred devices
@@ -171,6 +151,26 @@ static void driver_deferred_probe_trigger(void)
 	queue_work(deferred_wq, &deferred_probe_work);
 }
 
+static void driver_deferred_probe_add(struct device *dev)
+{
+	mutex_lock(&deferred_probe_mutex);
+	if (list_empty(&dev->p->deferred_probe)) {
+		dev_dbg(dev, "Added to deferred list\n");
+		list_add_tail(&dev->p->deferred_probe, &deferred_probe_pending_list);
+	}
+	mutex_unlock(&deferred_probe_mutex);
+}
+
+void driver_deferred_probe_del(struct device *dev)
+{
+	mutex_lock(&deferred_probe_mutex);
+	if (!list_empty(&dev->p->deferred_probe)) {
+		dev_dbg(dev, "Removed from deferred list\n");
+		list_del_init(&dev->p->deferred_probe);
+	}
+	mutex_unlock(&deferred_probe_mutex);
+}
+
 /**
  * deferred_probe_initcall() - Enable probing of deferred devices
  *
-- 
2.0.1

[PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

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 <gregkh@linuxfoundation.org>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Joseph Salisbury <joseph.salisbury@canonical.com>
Cc: Kay Sievers <kay@vrfy.org>
Cc: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
Cc: Tim Gardner <tim.gardner@canonical.com>
Cc: Pierre Fersing <pierre-fersing@pierref.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Benjamin Poirier <bpoirier@suse.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Nagalakshmi Nandigama <nagalakshmi.nandigama@avagotech.com>
Cc: Praveen Krishnamoorthy <praveen.krishnamoorthy@avagotech.com>
Cc: Sreekanth Reddy <sreekanth.reddy@avagotech.com>
Cc: Abhijit Mahajan <abhijit.mahajan@avagotech.com>
Cc: Hariprasad S <hariprasad@chelsio.com>
Cc: Santosh Rastapur <santosh@chelsio.com>
Cc: MPT-FusionLinux.pdl@avagotech.com
Cc: linux-scsi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/base/dd.c      | 18 +++++++++++++++++-
 include/linux/device.h |  7 +++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9947c2e..d59e4b5 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -159,6 +159,9 @@ static void driver_deferred_probe_add(struct device *dev)
 		list_add_tail(&dev->p->deferred_probe, &deferred_probe_pending_list);
 	}
 	mutex_unlock(&deferred_probe_mutex);
+
+	if (driver_deferred_probe_enable)
+		driver_deferred_probe_trigger();
 }
 
 void driver_deferred_probe_del(struct device *dev)
@@ -374,6 +377,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 +412,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..c317dfa 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -200,6 +200,9 @@ 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.
  * @of_match_table: The open firmware table.
  * @acpi_match_table: The ACPI match table.
  * @probe:	Called to query the existence of a specific device,
@@ -233,6 +236,7 @@ struct device_driver {
 	const char		*mod_name;	/* used for built-in modules */
 
 	bool suppress_bind_attrs;	/* disables bind/unbind via sysfs */
+	bool delay_probe;		/* requests deferred probe */
 
 	const struct of_device_id	*of_match_table;
 	const struct acpi_device_id	*acpi_match_table;
@@ -715,6 +719,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 +799,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

[PATCH v2 3/4] cxgb4: ask for deferred probe

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

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 <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Joseph Salisbury <joseph.salisbury@canonical.com>
Cc: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
Cc: Tim Gardner <tim.gardner@canonical.com>
Cc: Pierre Fersing <pierre-fersing@pierref.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Benjamin Poirier <bpoirier@suse.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Nagalakshmi Nandigama <nagalakshmi.nandigama@avagotech.com>
Cc: Praveen Krishnamoorthy <praveen.krishnamoorthy@avagotech.com>
Cc: Sreekanth Reddy <sreekanth.reddy@avagotech.com>
Cc: Abhijit Mahajan <abhijit.mahajan@avagotech.com>
Cc: Hariprasad S <hariprasad@chelsio.com>
Cc: Santosh Rastapur <santosh@chelsio.com>
Cc: MPT-FusionLinux.pdl@avagotech.com
Cc: linux-scsi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 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

[PATCH v2 4/4] mptsas: ask for deferred probe

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

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 <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Joseph Salisbury <joseph.salisbury@canonical.com>
Cc: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
Cc: Tim Gardner <tim.gardner@canonical.com>
Cc: Pierre Fersing <pierre-fersing@pierref.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Benjamin Poirier <bpoirier@suse.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Nagalakshmi Nandigama <nagalakshmi.nandigama@avagotech.com>
Cc: Praveen Krishnamoorthy <praveen.krishnamoorthy@avagotech.com>
Cc: Sreekanth Reddy <sreekanth.reddy@avagotech.com>
Cc: Abhijit Mahajan <abhijit.mahajan@avagotech.com>
Cc: Hariprasad S <hariprasad@chelsio.com>
Cc: Santosh Rastapur <santosh@chelsio.com>
Cc: MPT-FusionLinux.pdl@avagotech.com
Cc: linux-scsi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 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

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[Greg KH]

On Mon, Jul 28, 2014 at 11:28:28AM -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> 
> 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.

Because no driver should ever take that long for their probe function to
return.  Why not fix those drivers?

> 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].

Then use the async firmware interface, why is any driver taking longer
than less than a second in their init function?

> 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.

I'm all for abusing kernel interfaces, but please, no, don't try to use
the deferred init code to cover up for broken drivers.  Just fix them
properly, we have the interfaces to handle it properly (i.e. async
firmware loading), please use it.

And no PCI driver should ever need "deferred init" as the resources for
such a device is already present in the system.  Now if userspace is up
and running yet is a different issue, one that deferred init is not
there for at all, sorry.

So, what drivers are having problems in their init sequence, and why
aren't they using async firmware loading?

thanks,

greg k-h

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[Luis R. Rodriguez]

On Mon, Jul 28, 2014 at 11:55 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> So, what drivers are having problems in their init sequence, and why
> aren't they using async firmware loading?

Fixing drivers is one thing, fixing drivers *now* because *now*
drivers are failing due to a regression is another thing and that's
what we have now so lets just be clear on that. The 30 second rule is
a major driver requirement change and it should not be taken slightly,
all of a sudden this is a new requirement but you won't know that
unless you're reading these threads or hit an issue. That's an issue
in itself.

The cxgb4: driver is an example where although I did propose patches
to use asynch firmware loading the entire registration of the
netdevice would need to be changed as well in order to get this right.
In short we have to scramble now to first identify drivers that have
long init sequences and then fix. There's an assumption that we can
easily fix drivers, this can take time. This series, although does
take advantage of a kernel interface for other uses, lets us identify
these drivers on the kernel ring buffer, so we can go and address
fixing them with time.

 Luis

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[Luis R. Rodriguez]

On Mon, Jul 28, 2014 at 12:04 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> On Mon, Jul 28, 2014 at 11:55 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> So, what drivers are having problems in their init sequence, and why
>> aren't they using async firmware loading?
>
> Fixing drivers is one thing, fixing drivers *now* because *now*
> drivers are failing due to a regression is another thing and that's
> what we have now so lets just be clear on that. The 30 second rule is
> a major driver requirement change and it should not be taken slightly,
> all of a sudden this is a new requirement but you won't know that
> unless you're reading these threads or hit an issue. That's an issue
> in itself.
>
> The cxgb4: driver is an example where although I did propose patches
> to use asynch firmware loading the entire registration of the
> netdevice would need to be changed as well in order to get this right.
> In short we have to scramble now to first identify drivers that have
> long init sequences and then fix. There's an assumption that we can
> easily fix drivers, this can take time. This series, although does
> take advantage of a kernel interface for other uses, lets us identify
> these drivers on the kernel ring buffer, so we can go and address
> fixing them with time.

Another thing that came up during asynch firmware review / integration
on cxgb4 was that it would not be the only thing that would need to be
fixed, the driver also has a ton of ports and at least as per my
review the proper fix would be to use platform regiister stuff. It is
a major rewrite of the driver but an example of a driver that needs
quite a bit of work to meet this new 30 second driver requirement.

 Luis

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[Greg KH]

On Mon, Jul 28, 2014 at 12:48:32PM -0700, Luis R. Rodriguez wrote:
> On Mon, Jul 28, 2014 at 12:04 PM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
> > On Mon, Jul 28, 2014 at 11:55 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> So, what drivers are having problems in their init sequence, and why
> >> aren't they using async firmware loading?
> >
> > Fixing drivers is one thing, fixing drivers *now* because *now*
> > drivers are failing due to a regression is another thing and that's
> > what we have now so lets just be clear on that. The 30 second rule is
> > a major driver requirement change and it should not be taken slightly,
> > all of a sudden this is a new requirement but you won't know that
> > unless you're reading these threads or hit an issue. That's an issue
> > in itself.

That "regression" is something that userspace has decided to do, not
anything the kernel changed, so perhaps you should just patch your
modprobe and be done with it until you can fix up those drivers?

And putting a horrid hack in the driver core, just because of some
really bad drivers, is not ok, that's an interface _I_ will have to
support for the next few decades.

And it's going to take you a while to get something like this ever
merged in anyway, odds are you can fix up the driver faster...

> > The cxgb4: driver is an example where although I did propose patches
> > to use asynch firmware loading the entire registration of the
> > netdevice would need to be changed as well in order to get this right.
> > In short we have to scramble now to first identify drivers that have
> > long init sequences and then fix. There's an assumption that we can
> > easily fix drivers, this can take time. This series, although does
> > take advantage of a kernel interface for other uses, lets us identify
> > these drivers on the kernel ring buffer, so we can go and address
> > fixing them with time.
> 
> Another thing that came up during asynch firmware review / integration
> on cxgb4 was that it would not be the only thing that would need to be
> fixed, the driver also has a ton of ports and at least as per my
> review the proper fix would be to use platform regiister stuff. It is
> a major rewrite of the driver but an example of a driver that needs
> quite a bit of work to meet this new 30 second driver requirement.

It shouldn't be using any platform driver stuff, it's a pci device, not
a platform device.

Why not just put the initial "register the device" in a single-shot
workqueue or thread or something like that so that modprobe returns
instantly back with a success and all is fine?

Again, trying to hack the "deferred init" logic for PCI drivers isn't
ok, I'm not going to take that into the driver core if at all possible,
sorry.

greg k-h

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[Luis R. Rodriguez]

On Mon, Jul 28, 2014 at 4:46 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Jul 28, 2014 at 12:48:32PM -0700, Luis R. Rodriguez wrote:
>> On Mon, Jul 28, 2014 at 12:04 PM, Luis R. Rodriguez
>> <mcgrof@do-not-panic.com> wrote:
>> > On Mon, Jul 28, 2014 at 11:55 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> >> So, what drivers are having problems in their init sequence, and why
>> >> aren't they using async firmware loading?
>> >
>> > Fixing drivers is one thing, fixing drivers *now* because *now*
>> > drivers are failing due to a regression is another thing and that's
>> > what we have now so lets just be clear on that. The 30 second rule is
>> > a major driver requirement change and it should not be taken slightly,
>> > all of a sudden this is a new requirement but you won't know that
>> > unless you're reading these threads or hit an issue. That's an issue
>> > in itself.
>
> That "regression" is something that userspace has decided to do, not
> anything the kernel changed,

Actually commit 786235ee seems to have been the one that caused this
issue, systemd would just send the SIGKILL and that change forced a
bail on probe then hence Canonical's work around to modify
kthread_create() to not leave upon SIGKILL:

http://thread.gmane.org/gmane.linux.ubuntu.devel.kernel.general/39123

> so perhaps you should just patch your
> modprobe and be done with it until you can fix up those drivers?

To ignore SIGKILL ?

> And putting a horrid hack in the driver core, just because of some
> really bad drivers, is not ok, that's an interface _I_ will have to
> support for the next few decades.

I understand, hence review.

> And it's going to take you a while to get something like this ever
> merged in anyway, odds are you can fix up the driver faster...

That requires quite a bit of changes and commitment and again, there
are quite a bit of drivers that we can run into in the community,
we've just spotted 2 so far here for now.

>> > The cxgb4: driver is an example where although I did propose patches
>> > to use asynch firmware loading the entire registration of the
>> > netdevice would need to be changed as well in order to get this right.
>> > In short we have to scramble now to first identify drivers that have
>> > long init sequences and then fix. There's an assumption that we can
>> > easily fix drivers, this can take time. This series, although does
>> > take advantage of a kernel interface for other uses, lets us identify
>> > these drivers on the kernel ring buffer, so we can go and address
>> > fixing them with time.
>>
>> Another thing that came up during asynch firmware review / integration
>> on cxgb4 was that it would not be the only thing that would need to be
>> fixed, the driver also has a ton of ports and at least as per my
>> review the proper fix would be to use platform regiister stuff. It is
>> a major rewrite of the driver but an example of a driver that needs
>> quite a bit of work to meet this new 30 second driver requirement.
>
> It shouldn't be using any platform driver stuff, it's a pci device, not
> a platform device.

The general PCI stuff is already used, the reason for suggesting the
platform_device_register_simple() stuff is it has tons of ports and
each port will register in turn a new struct netdevice, essentially
one device can end up having tons of different network devices, the
platform stuff would be to allow handling each netdevice candidate
separately as part of the internal driver architecture, right now its
some scary loop thing that in my eyes can be very error prone.
drivers/net/ethernet/8390/ne.c. This discussion:

https://lkml.org/lkml/2014/6/25/815

> Why not just put the initial "register the device" in a single-shot
> workqueue or thread or something like that so that modprobe returns
> instantly back with a success and all is fine?

That surely is possible but why not a general solution for such kludges?

> Again, trying to hack the "deferred init" logic for PCI drivers isn't
> ok, I'm not going to take that into the driver core if at all possible,
> sorry.

No need to apologize I'm looking for the best solution here after all.
One userspace kludge is surely better than a one per driver while
drivers are fixed for this new driver requirement. Its just kind of
odd the circle of events for a kernel issue from kernel --> systemd
--> modprobe as a work around.

 Luis

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[Greg KH]

On Mon, Jul 28, 2014 at 05:26:34PM -0700, Luis R. Rodriguez wrote:
> On Mon, Jul 28, 2014 at 4:46 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Mon, Jul 28, 2014 at 12:48:32PM -0700, Luis R. Rodriguez wrote:
> >> On Mon, Jul 28, 2014 at 12:04 PM, Luis R. Rodriguez
> >> <mcgrof@do-not-panic.com> wrote:
> >> > On Mon, Jul 28, 2014 at 11:55 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> >> So, what drivers are having problems in their init sequence, and why
> >> >> aren't they using async firmware loading?
> >> >
> >> > Fixing drivers is one thing, fixing drivers *now* because *now*
> >> > drivers are failing due to a regression is another thing and that's
> >> > what we have now so lets just be clear on that. The 30 second rule is
> >> > a major driver requirement change and it should not be taken slightly,
> >> > all of a sudden this is a new requirement but you won't know that
> >> > unless you're reading these threads or hit an issue. That's an issue
> >> > in itself.
> >
> > That "regression" is something that userspace has decided to do, not
> > anything the kernel changed,
> 
> Actually commit 786235ee seems to have been the one that caused this
> issue, systemd would just send the SIGKILL and that change forced a
> bail on probe then hence Canonical's work around to modify
> kthread_create() to not leave upon SIGKILL:
> 
> http://thread.gmane.org/gmane.linux.ubuntu.devel.kernel.general/39123
> 
> > so perhaps you should just patch your
> > modprobe and be done with it until you can fix up those drivers?
> 
> To ignore SIGKILL ?

Sorry, I thought this was a userspace change that caused this.

As it's a kernel change, well, maybe that patch should be reverted...

> > And putting a horrid hack in the driver core, just because of some
> > really bad drivers, is not ok, that's an interface _I_ will have to
> > support for the next few decades.
> 
> I understand, hence review.
> 
> > And it's going to take you a while to get something like this ever
> > merged in anyway, odds are you can fix up the driver faster...
> 
> That requires quite a bit of changes and commitment and again, there
> are quite a bit of drivers that we can run into in the community,
> we've just spotted 2 so far here for now.
> 
> >> > The cxgb4: driver is an example where although I did propose patches
> >> > to use asynch firmware loading the entire registration of the
> >> > netdevice would need to be changed as well in order to get this right.
> >> > In short we have to scramble now to first identify drivers that have
> >> > long init sequences and then fix. There's an assumption that we can
> >> > easily fix drivers, this can take time. This series, although does
> >> > take advantage of a kernel interface for other uses, lets us identify
> >> > these drivers on the kernel ring buffer, so we can go and address
> >> > fixing them with time.
> >>
> >> Another thing that came up during asynch firmware review / integration
> >> on cxgb4 was that it would not be the only thing that would need to be
> >> fixed, the driver also has a ton of ports and at least as per my
> >> review the proper fix would be to use platform regiister stuff. It is
> >> a major rewrite of the driver but an example of a driver that needs
> >> quite a bit of work to meet this new 30 second driver requirement.
> >
> > It shouldn't be using any platform driver stuff, it's a pci device, not
> > a platform device.
> 
> The general PCI stuff is already used, the reason for suggesting the
> platform_device_register_simple() stuff is it has tons of ports and
> each port will register in turn a new struct netdevice, essentially
> one device can end up having tons of different network devices, the
> platform stuff would be to allow handling each netdevice candidate
> separately as part of the internal driver architecture, right now its
> some scary loop thing that in my eyes can be very error prone.
> drivers/net/ethernet/8390/ne.c. This discussion:

No, don't use platform devices as a "catch-all" for when you don't want
to write your own bus code.  This looks like a device-specific bus,
great, write the code to do that, it can be done in about 200 lines,
with comments and whitespace.

Only use platform devices for, wait for it, platform devices.  And even
then, reconsider and use something else if at all possible.

> > Why not just put the initial "register the device" in a single-shot
> > workqueue or thread or something like that so that modprobe returns
> > instantly back with a success and all is fine?
> 
> That surely is possible but why not a general solution for such kludges?

Because the driver should be fixed.  How hard would it be to do what I
just suggested here, 20 lines added at most?

thanks,

greg k-h

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[Luis R. Rodriguez]

On Mon, Jul 28, 2014 at 5:35 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Jul 28, 2014 at 05:26:34PM -0700, Luis R. Rodriguez wrote:
>> On Mon, Jul 28, 2014 at 4:46 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Mon, Jul 28, 2014 at 12:48:32PM -0700, Luis R. Rodriguez wrote:
>> >> On Mon, Jul 28, 2014 at 12:04 PM, Luis R. Rodriguez
>> >> <mcgrof@do-not-panic.com> wrote:
>> >> > On Mon, Jul 28, 2014 at 11:55 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> >> >> So, what drivers are having problems in their init sequence, and why
>> >> >> aren't they using async firmware loading?
>> >> >
>> >> > Fixing drivers is one thing, fixing drivers *now* because *now*
>> >> > drivers are failing due to a regression is another thing and that's
>> >> > what we have now so lets just be clear on that. The 30 second rule is
>> >> > a major driver requirement change and it should not be taken slightly,
>> >> > all of a sudden this is a new requirement but you won't know that
>> >> > unless you're reading these threads or hit an issue. That's an issue
>> >> > in itself.
>> >
>> > That "regression" is something that userspace has decided to do, not
>> > anything the kernel changed,
>>
>> Actually commit 786235ee seems to have been the one that caused this
>> issue, systemd would just send the SIGKILL and that change forced a
>> bail on probe then hence Canonical's work around to modify
>> kthread_create() to not leave upon SIGKILL:
>>
>> http://thread.gmane.org/gmane.linux.ubuntu.devel.kernel.general/39123
>>
>> > so perhaps you should just patch your
>> > modprobe and be done with it until you can fix up those drivers?
>>
>> To ignore SIGKILL ?
>
> Sorry, I thought this was a userspace change that caused this.
>
> As it's a kernel change, well, maybe that patch should be reverted...

That's certainly viable. Oleg?

If its reverted we won't know which drivers are hitting over the new
30 second limit requirement imposed by userspace, which the culprit
commit forces failure on probe now. This series at least would allow
us to annotate which drivers are brake the 30 second init limit, and
enable a work around alternative if we wish to not revert the commit.
This  series essentially should be considered an alternative solution
to what was proposed initially by Joseph Salisbury, it may not be
perfect but its a proposal. I welcome others.

>> > And putting a horrid hack in the driver core, just because of some
>> > really bad drivers, is not ok, that's an interface _I_ will have to
>> > support for the next few decades.
>>
>> I understand, hence review.
>>
>> > And it's going to take you a while to get something like this ever
>> > merged in anyway, odds are you can fix up the driver faster...
>>
>> That requires quite a bit of changes and commitment and again, there
>> are quite a bit of drivers that we can run into in the community,
>> we've just spotted 2 so far here for now.
>>
>> >> > The cxgb4: driver is an example where although I did propose patches
>> >> > to use asynch firmware loading the entire registration of the
>> >> > netdevice would need to be changed as well in order to get this right.
>> >> > In short we have to scramble now to first identify drivers that have
>> >> > long init sequences and then fix. There's an assumption that we can
>> >> > easily fix drivers, this can take time. This series, although does
>> >> > take advantage of a kernel interface for other uses, lets us identify
>> >> > these drivers on the kernel ring buffer, so we can go and address
>> >> > fixing them with time.
>> >>
>> >> Another thing that came up during asynch firmware review / integration
>> >> on cxgb4 was that it would not be the only thing that would need to be
>> >> fixed, the driver also has a ton of ports and at least as per my
>> >> review the proper fix would be to use platform regiister stuff. It is
>> >> a major rewrite of the driver but an example of a driver that needs
>> >> quite a bit of work to meet this new 30 second driver requirement.
>> >
>> > It shouldn't be using any platform driver stuff, it's a pci device, not
>> > a platform device.
>>
>> The general PCI stuff is already used, the reason for suggesting the
>> platform_device_register_simple() stuff is it has tons of ports and
>> each port will register in turn a new struct netdevice, essentially
>> one device can end up having tons of different network devices, the
>> platform stuff would be to allow handling each netdevice candidate
>> separately as part of the internal driver architecture, right now its
>> some scary loop thing that in my eyes can be very error prone.
>> drivers/net/ethernet/8390/ne.c. This discussion:
>
> No, don't use platform devices as a "catch-all" for when you don't want
> to write your own bus code.  This looks like a device-specific bus,
> great, write the code to do that, it can be done in about 200 lines,
> with comments and whitespace.
>
> Only use platform devices for, wait for it, platform devices.  And even
> then, reconsider and use something else if at all possible.

OK thanks I had asked for advice for this a while back on that old
thread as I wasn't sure if that was the best.

>> > Why not just put the initial "register the device" in a single-shot
>> > workqueue or thread or something like that so that modprobe returns
>> > instantly back with a success and all is fine?
>>
>> That surely is possible but why not a general solution for such kludges?
>
> Because the driver should be fixed.  How hard would it be to do what I
> just suggested here, 20 lines added at most?

I appreciate the feedback, but don't look at me, I'd happy to go nutty
on ripping things apart from hairy drivers, however Chelsie folks
expressed concerns over major rework on the driver... so even if we
did have something I expect things to take a while to bake / gain
confidence from others.

This also just addresses this *one* Ethernet driver, there was that
SCSI driver that created the original report -- Canonical merged
Joseph's fix as a work around, there is no general solution for this
yet, and again with that work around you won't find which drivers run
into this issue. There may be others found later so this is why I
resorted to the deferred workqueue as a solution for now and to enable
us to annotate which drivers need fixing as I expect getting the fix
done / everyone happy can take time.

 Luis

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[Hannes Reinecke]

On 07/29/2014 03:13 AM, Luis R. Rodriguez wrote:
> On Mon, Jul 28, 2014 at 5:35 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> On Mon, Jul 28, 2014 at 05:26:34PM -0700, Luis R. Rodriguez wrote:
>>> On Mon, Jul 28, 2014 at 4:46 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>>> On Mon, Jul 28, 2014 at 12:48:32PM -0700, Luis R. Rodriguez wrote:
>>>>> On Mon, Jul 28, 2014 at 12:04 PM, Luis R. Rodriguez
>>>>> <mcgrof@do-not-panic.com> wrote:
>>>>>> On Mon, Jul 28, 2014 at 11:55 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>>>>>> So, what drivers are having problems in their init sequence, and why
>>>>>>> aren't they using async firmware loading?
>>>>>>
>>>>>> Fixing drivers is one thing, fixing drivers *now* because *now*
>>>>>> drivers are failing due to a regression is another thing and that's
>>>>>> what we have now so lets just be clear on that. The 30 second rule is
>>>>>> a major driver requirement change and it should not be taken slightly,
>>>>>> all of a sudden this is a new requirement but you won't know that
>>>>>> unless you're reading these threads or hit an issue. That's an issue
>>>>>> in itself.
>>>>
>>>> That "regression" is something that userspace has decided to do, not
>>>> anything the kernel changed,
>>>
>>> Actually commit 786235ee seems to have been the one that caused this
>>> issue, systemd would just send the SIGKILL and that change forced a
>>> bail on probe then hence Canonical's work around to modify
>>> kthread_create() to not leave upon SIGKILL:
>>>
>>> http://thread.gmane.org/gmane.linux.ubuntu.devel.kernel.general/39123
>>>
>>>> so perhaps you should just patch your
>>>> modprobe and be done with it until you can fix up those drivers?
>>>
>>> To ignore SIGKILL ?
>>
>> Sorry, I thought this was a userspace change that caused this.
>>
>> As it's a kernel change, well, maybe that patch should be reverted...
>
> That's certainly viable. Oleg?
>
> If its reverted we won't know which drivers are hitting over the new
> 30 second limit requirement imposed by userspace, which the culprit
> commit forces failure on probe now. This series at least would allow
> us to annotate which drivers are brake the 30 second init limit, and
> enable a work around alternative if we wish to not revert the commit.
> This  series essentially should be considered an alternative solution
> to what was proposed initially by Joseph Salisbury, it may not be
> perfect but its a proposal. I welcome others.
>
>>>> And putting a horrid hack in the driver core, just because of some
>>>> really bad drivers, is not ok, that's an interface _I_ will have to
>>>> support for the next few decades.
>>>
>>> I understand, hence review.
>>>
>>>> And it's going to take you a while to get something like this ever
>>>> merged in anyway, odds are you can fix up the driver faster...
>>>
>>> That requires quite a bit of changes and commitment and again, there
>>> are quite a bit of drivers that we can run into in the community,
>>> we've just spotted 2 so far here for now.
>>>
>>>>>> The cxgb4: driver is an example where although I did propose patches
>>>>>> to use asynch firmware loading the entire registration of the
>>>>>> netdevice would need to be changed as well in order to get this right.
>>>>>> In short we have to scramble now to first identify drivers that have
>>>>>> long init sequences and then fix. There's an assumption that we can
>>>>>> easily fix drivers, this can take time. This series, although does
>>>>>> take advantage of a kernel interface for other uses, lets us identify
>>>>>> these drivers on the kernel ring buffer, so we can go and address
>>>>>> fixing them with time.
>>>>>
>>>>> Another thing that came up during asynch firmware review / integration
>>>>> on cxgb4 was that it would not be the only thing that would need to be
>>>>> fixed, the driver also has a ton of ports and at least as per my
>>>>> review the proper fix would be to use platform regiister stuff. It is
>>>>> a major rewrite of the driver but an example of a driver that needs
>>>>> quite a bit of work to meet this new 30 second driver requirement.
>>>>
>>>> It shouldn't be using any platform driver stuff, it's a pci device, not
>>>> a platform device.
>>>
>>> The general PCI stuff is already used, the reason for suggesting the
>>> platform_device_register_simple() stuff is it has tons of ports and
>>> each port will register in turn a new struct netdevice, essentially
>>> one device can end up having tons of different network devices, the
>>> platform stuff would be to allow handling each netdevice candidate
>>> separately as part of the internal driver architecture, right now its
>>> some scary loop thing that in my eyes can be very error prone.
>>> drivers/net/ethernet/8390/ne.c. This discussion:
>>
>> No, don't use platform devices as a "catch-all" for when you don't want
>> to write your own bus code.  This looks like a device-specific bus,
>> great, write the code to do that, it can be done in about 200 lines,
>> with comments and whitespace.
>>
>> Only use platform devices for, wait for it, platform devices.  And even
>> then, reconsider and use something else if at all possible.
>
> OK thanks I had asked for advice for this a while back on that old
> thread as I wasn't sure if that was the best.
>
>>>> Why not just put the initial "register the device" in a single-shot
>>>> workqueue or thread or something like that so that modprobe returns
>>>> instantly back with a success and all is fine?
>>>
>>> That surely is possible but why not a general solution for such kludges?
>>
>> Because the driver should be fixed.  How hard would it be to do what I
>> just suggested here, 20 lines added at most?
>
> I appreciate the feedback, but don't look at me, I'd happy to go nutty
> on ripping things apart from hairy drivers, however Chelsie folks
> expressed concerns over major rework on the driver... so even if we
> did have something I expect things to take a while to bake / gain
> confidence from others.
>
> This also just addresses this *one* Ethernet driver, there was that
> SCSI driver that created the original report -- Canonical merged
> Joseph's fix as a work around, there is no general solution for this
> yet, and again with that work around you won't find which drivers run
> into this issue. There may be others found later so this is why I
> resorted to the deferred workqueue as a solution for now and to enable
> us to annotate which drivers need fixing as I expect getting the fix
> done / everyone happy can take time.
>
Well ... from my POV there are two issues here:

The one is that systemd essentially nailed its internal worker 
timeout to 30 seconds. And there is no way of modifying that short 
of recompiling. Which should be fixed, so that at least one can
modify this timeout.

The other one is that systemd killing a worker by sending SIGKILL, 
which will kill modprobe terminally.
Which definitely needs to be fixed.

But if we have both issues resolved (eg by allowing udevd to use a 
longer timeout and revert the latter patch) we can identify the 
offending drivers _and_ get the system to boot by simply adding a 
kernel commandline parameter.
Which is _far_ preferrable from a maintenance perspective.
Users tend to become annoyed if their system doesn't boot ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probefrom init

[Tetsuo Handa]

Luis R. Rodriguez wrote:
> On Mon, Jul 28, 2014 at 5:35 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Mon, Jul 28, 2014 at 05:26:34PM -0700, Luis R. Rodriguez wrote:
> >> To ignore SIGKILL ?
> >
> > Sorry, I thought this was a userspace change that caused this.
> >
> > As it's a kernel change, well, maybe that patch should be reverted...
> 
> That's certainly viable. Oleg?

I don't want to revert that patch.

I'm trying to reduce use of blocking operations that wait in unkillable state,
for the OOM killer is optimistic enough to wait forever even if the OOM-killed
process cannot be terminated due to having dependency on other threads that are
waiting the OOM-killed process to terminate and release memory. Linux kernel is
too optimistic about memory reclaim; memory allocation/reclaim deadlock is not
detectable.

> 
> If its reverted we won't know which drivers are hitting over the new
> 30 second limit requirement imposed by userspace, which the culprit
> commit forces failure on probe now. This series at least would allow
> us to annotate which drivers are brake the 30 second init limit, and
> enable a work around alternative if we wish to not revert the commit.
> This  series essentially should be considered an alternative solution
> to what was proposed initially by Joseph Salisbury, it may not be
> perfect but its a proposal. I welcome others.
(...snipped...)
> This also just addresses this *one* Ethernet driver, there was that
> SCSI driver that created the original report -- Canonical merged
> Joseph's fix as a work around, there is no general solution for this
> yet, and again with that work around you won't find which drivers run
> into this issue. There may be others found later so this is why I
> resorted to the deferred workqueue as a solution for now and to enable
> us to annotate which drivers need fixing as I expect getting the fix
> done / everyone happy can take time.

If you want to know which drivers are hitting over the new 30 second
limit requirement but don't want to break the boot, I think we can do
"what Ubuntu chose as a work around" + "a warning message" like below.

diff --git a/kernel/kthread.c b/kernel/kthread.c
index c2390f4..43da2b1 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -292,6 +292,26 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
 	 * new kernel thread.
 	 */
 	if (unlikely(wait_for_completion_killable(&done))) {
+		int i = 0;
+
+		/*
+		 * I got SIGKILL, but wait for 10 more seconds for completion
+		 * unless chosen by the OOM killer. This delay is there as a
+		 * workaround for boot failure caused by SIGKILL upon device
+		 * driver initialization timeout.
+		 */
+		if (!test_tsk_thread_flag(current, TIF_MEMDIE)) {
+			pr_warn("I already got SIGKILL but ignoring it up to "
+				"10 seconds, in case the caller can't survive "
+				"fail-immediately-upon-SIGKILL event. "
+				"Please make sure that the caller can survive "
+				"this event, for this delay will be removed "
+				"in the future.\n");
+			dump_stack();
+		}
+		while (i++ < 10 && !test_tsk_thread_flag(current, TIF_MEMDIE))
+			if (wait_for_completion_timeout(&done, HZ))
+				goto ready;
 		/*
 		 * If I was SIGKILLed before kthreadd (or new kernel thread)
 		 * calls complete(), leave the cleanup of this structure to
@@ -305,6 +325,7 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
 		 */
 		wait_for_completion(&done);
 	}
+ready:
 	task = create->result;
 	if (!IS_ERR(task)) {
 		static const struct sched_param param = { .sched_priority = 0 };

Hannes Reinecke wrote:
> Well ... from my POV there are two issues here:
> 
> The one is that systemd essentially nailed its internal worker 
> timeout to 30 seconds. And there is no way of modifying that short 
> of recompiling. Which should be fixed, so that at least one can
> modify this timeout.
> 
> The other one is that systemd killing a worker by sending SIGKILL, 
> which will kill modprobe terminally.
> Which definitely needs to be fixed.
> 
> But if we have both issues resolved (eg by allowing udevd to use a 
> longer timeout and revert the latter patch) we can identify the 
> offending drivers _and_ get the system to boot by simply adding a 
> kernel commandline parameter.
> Which is _far_ preferrable from a maintenance perspective.
> Users tend to become annoyed if their system doesn't boot ...

The proposal which allows longer timeout was expired.
https://bugs.launchpad.net/bugs/1297248

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probefrom init

[Benjamin Poirier]

On 2014/07/29 21:07, Tetsuo Handa wrote:
> Luis R. Rodriguez wrote:
> > On Mon, Jul 28, 2014 at 5:35 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > On Mon, Jul 28, 2014 at 05:26:34PM -0700, Luis R. Rodriguez wrote:
> > >> To ignore SIGKILL ?
> > >
> > > Sorry, I thought this was a userspace change that caused this.
> > >
> > > As it's a kernel change, well, maybe that patch should be reverted...
> > 
> > That's certainly viable. Oleg?
> 
> I don't want to revert that patch.

I agree that 786235ee should not be reverted to fix the problem of
modules that receive sigkill from udev while they are initializing. In
fact, while it may fix the case that was reported with mptsas, it would
not fix cxgb4 because there are other code paths that check for pending
signals and that abort (ex. pci_vpd_pci22_wait()).

Reverting 786235ee effectively works around the problem by making
modprobe unkillable. The proper solution would be to make sure that udev
does not send sigkill to modprobe in the first place, either by making
the timeout longer or by making the module probe faster.

If you have other reasons for reverting 786235ee, then it's a different
story.

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[Greg KH]

On Mon, Jul 28, 2014 at 06:13:43PM -0700, Luis R. Rodriguez wrote:
> >> > Why not just put the initial "register the device" in a single-shot
> >> > workqueue or thread or something like that so that modprobe returns
> >> > instantly back with a success and all is fine?
> >>
> >> That surely is possible but why not a general solution for such kludges?
> >
> > Because the driver should be fixed.  How hard would it be to do what I
> > just suggested here, 20 lines added at most?
> 
> I appreciate the feedback, but don't look at me, I'd happy to go nutty
> on ripping things apart from hairy drivers, however Chelsie folks
> expressed concerns over major rework on the driver... so even if we
> did have something I expect things to take a while to bake / gain
> confidence from others.

"rework"?  Heh, here's a patch that adds 10 lines to the mptsas driver
that should also work for any other driver as well.  Why not just do
this instead?

> This also just addresses this *one* Ethernet driver, there was that
> SCSI driver that created the original report -- Canonical merged
> Joseph's fix as a work around,

What fix was that?

> there is no general solution for this yet, and again with that work
> around you won't find which drivers run into this issue.

Great, fix them as they are found, that's fine.  Again, don't add stuff
to the driver core to paper over crappy drivers, I'm not going to take
that type of change.  I _only_ took the defer binding code as there was
no other way for an individual driver to deal with things if it's
resources were not present yet due to binding order in the system.

So, anyone care to test the patch below on a system that has this
problem to let me know if it would work or not?  Odds are, this should
be a workqueue, to make it cleaner, but a kthread is just so easy to
use...

thanks,

greg k-h

diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 711fcb5cec87..4fd4f36a2d9e 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -51,6 +51,7 @@
 #include <linux/jiffies.h>
 #include <linux/workqueue.h>
 #include <linux/delay.h>	/* for mdelay */
+#include <linux/kthread.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
@@ -5393,8 +5394,7 @@ static struct pci_driver mptsas_driver = {
 #endif
 };
 
-static int __init
-mptsas_init(void)
+static int mptsas_real_init(void *data)
 {
 	int error;
 
@@ -5429,9 +5429,19 @@ mptsas_init(void)
 	return error;
 }
 
+static struct task_struct *init_thread;
+
+static int __init
+mptsas_init(void)
+{
+	init_thread = kthread_run(mptsas_real_init, NULL, "mptsas_init");
+	return 0;
+}
+
 static void __exit
 mptsas_exit(void)
 {
+	kthread_stop(init_thread);
 	pci_unregister_driver(&mptsas_driver);
 	sas_release_transport(mptsas_transport_template);
 

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[Luis R. Rodriguez]

On Tue, Jul 29, 2014 at 04:14:22PM -0700, Greg KH wrote:
> On Mon, Jul 28, 2014 at 06:13:43PM -0700, Luis R. Rodriguez wrote:
> > >> > Why not just put the initial "register the device" in a single-shot
> > >> > workqueue or thread or something like that so that modprobe returns
> > >> > instantly back with a success and all is fine?
> > >>
> > >> That surely is possible but why not a general solution for such kludges?
> > >
> > > Because the driver should be fixed.  How hard would it be to do what I
> > > just suggested here, 20 lines added at most?
> > 
> > I appreciate the feedback, but don't look at me, I'd happy to go nutty
> > on ripping things apart from hairy drivers, however Chelsie folks
> > expressed concerns over major rework on the driver... so even if we
> > did have something I expect things to take a while to bake / gain
> > confidence from others.
> 
> "rework"?  Heh, here's a patch that adds 10 lines to the mptsas driver
> that should also work for any other driver as well.  Why not just do
> this instead?

That's not a rework, that's a kludge, doing something similar for other
drivers would be replicating kludges, the deferred probe use was trying
to generalize a kludge with 3 lines of code. But I'm no not yet convinced
its the best solution now.

> > This also just addresses this *one* Ethernet driver, there was that
> > SCSI driver that created the original report -- Canonical merged
> > Joseph's fix as a work around,
> 
> What fix was that?

https://launchpadlibrarian.net/169714201/kthread-Do-not-leave-kthread_create-immediately.patch

It was reviewed and Oleg preferred the timeout instead be reviewed
on systemd devel mailing list. That didn't go anywhere but today Hannes
posted a patch and that got merged. Its still not solving all issues
though as it lets us override the timeout value, systems / drivers
can still crash without a long timeout.

> > there is no general solution for this yet, and again with that work
> > around you won't find which drivers run into this issue.
> 
> Great, fix them as they are found, that's fine. 

Are we really OK in letting distributions have to deal with crashes
because of this new driver 30 second timeout ? I think warning about
it would be better and more friendlier, no? What gains do we have to
kill the damn thing?

> Again, don't add stuff
> to the driver core to paper over crappy drivers, I'm not going to take
> that type of change.  I _only_ took the defer binding code as there was
> no other way for an individual driver to deal with things if it's
> resources were not present yet due to binding order in the system.

Ok but its a bit unfair to force killing drivers over a new driver 30 second
timeout requirement for a change that was made implicitly through a series
of collateral changes.

> So, anyone care to test the patch below on a system that has this
> problem to let me know if it would work or not?  Odds are, this should
> be a workqueue, to make it cleaner, but a kthread is just so easy to
> use...
> 
> thanks,
> 
> greg k-h
> 
> diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
> index 711fcb5cec87..4fd4f36a2d9e 100644
> --- a/drivers/message/fusion/mptsas.c
> +++ b/drivers/message/fusion/mptsas.c
> @@ -51,6 +51,7 @@
>  #include <linux/jiffies.h>
>  #include <linux/workqueue.h>
>  #include <linux/delay.h>	/* for mdelay */
> +#include <linux/kthread.h>
>  
>  #include <scsi/scsi.h>
>  #include <scsi/scsi_cmnd.h>
> @@ -5393,8 +5394,7 @@ static struct pci_driver mptsas_driver = {
>  #endif
>  };
>  
> -static int __init
> -mptsas_init(void)
> +static int mptsas_real_init(void *data)
>  {
>  	int error;
>  
> @@ -5429,9 +5429,19 @@ mptsas_init(void)
>  	return error;
>  }
>  
> +static struct task_struct *init_thread;
> +
> +static int __init
> +mptsas_init(void)
> +{
> +	init_thread = kthread_run(mptsas_real_init, NULL, "mptsas_init");
> +	return 0;
> +}
> +
>  static void __exit
>  mptsas_exit(void)
>  {
> +	kthread_stop(init_thread);
>  	pci_unregister_driver(&mptsas_driver);
>  	sas_release_transport(mptsas_transport_template);

So we're OK to see these kludges sprinkled over the kernel instead of
genrealizing somethiing for them in the meantime?

  Luis

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[Greg KH]

On Wed, Jul 30, 2014 at 02:05:41AM +0200, Luis R. Rodriguez wrote:
> On Tue, Jul 29, 2014 at 04:14:22PM -0700, Greg KH wrote:
> > On Mon, Jul 28, 2014 at 06:13:43PM -0700, Luis R. Rodriguez wrote:
> > > >> > Why not just put the initial "register the device" in a single-shot
> > > >> > workqueue or thread or something like that so that modprobe returns
> > > >> > instantly back with a success and all is fine?
> > > >>
> > > >> That surely is possible but why not a general solution for such kludges?
> > > >
> > > > Because the driver should be fixed.  How hard would it be to do what I
> > > > just suggested here, 20 lines added at most?
> > > 
> > > I appreciate the feedback, but don't look at me, I'd happy to go nutty
> > > on ripping things apart from hairy drivers, however Chelsie folks
> > > expressed concerns over major rework on the driver... so even if we
> > > did have something I expect things to take a while to bake / gain
> > > confidence from others.
> > 
> > "rework"?  Heh, here's a patch that adds 10 lines to the mptsas driver
> > that should also work for any other driver as well.  Why not just do
> > this instead?
> 
> That's not a rework, that's a kludge, doing something similar for other
> drivers would be replicating kludges, the deferred probe use was trying
> to generalize a kludge with 3 lines of code. But I'm no not yet convinced
> its the best solution now.

I'm not saying it's pretty, but it confied to just the broken module,
and also, it's obvious what needs to be fixed if someone cares.

It sounds like no one cares about these moduls :)

Adding it to the driver core ensures that the drivers will _never_ be
changed, which isn't ok with me, sorry.


> > > This also just addresses this *one* Ethernet driver, there was that
> > > SCSI driver that created the original report -- Canonical merged
> > > Joseph's fix as a work around,
> > 
> > What fix was that?
> 
> https://launchpadlibrarian.net/169714201/kthread-Do-not-leave-kthread_create-immediately.patch
> 
> It was reviewed and Oleg preferred the timeout instead be reviewed
> on systemd devel mailing list. That didn't go anywhere but today Hannes
> posted a patch and that got merged. Its still not solving all issues
> though as it lets us override the timeout value, systems / drivers
> can still crash without a long timeout.

Great, work it out with them, again, stay away from the driver core for
this...

> > > there is no general solution for this yet, and again with that work
> > > around you won't find which drivers run into this issue.
> > 
> > Great, fix them as they are found, that's fine. 
> 
> Are we really OK in letting distributions have to deal with crashes
> because of this new driver 30 second timeout ?

If you don't want to, then revert the kernel change that caused it for
your distro kernels.

> I think warning about it would be better and more friendlier, no? What
> gains do we have to kill the damn thing?

Take it up with the owners of that code...

> > Again, don't add stuff
> > to the driver core to paper over crappy drivers, I'm not going to take
> > that type of change.  I _only_ took the defer binding code as there was
> > no other way for an individual driver to deal with things if it's
> > resources were not present yet due to binding order in the system.
> 
> Ok but its a bit unfair to force killing drivers over a new driver 30 second
> timeout requirement for a change that was made implicitly through a series
> of collateral changes.

I'm not disagreeing with that, but hey, life isn't fair, and things
needs to get fixed all the time...

I say fix it _properly_ by fixing the drivers.

best of luck,

greg k-h

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probefrom init

[Luis R. Rodriguez]

On Tue, Jul 29, 2014 at 03:25:29PM -0700, Benjamin Poirier wrote:
> On 2014/07/29 21:07, Tetsuo Handa wrote:
> > Luis R. Rodriguez wrote:
> > > On Mon, Jul 28, 2014 at 5:35 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > On Mon, Jul 28, 2014 at 05:26:34PM -0700, Luis R. Rodriguez wrote:
> > > >> To ignore SIGKILL ?
> > > >
> > > > Sorry, I thought this was a userspace change that caused this.
> > > >
> > > > As it's a kernel change, well, maybe that patch should be reverted...
> > > 
> > > That's certainly viable. Oleg?
> > 
> > I don't want to revert that patch.
> 
> I agree that 786235ee should not be reverted to fix the problem of
> modules that receive sigkill from udev while they are initializing. In
> fact, while it may fix the case that was reported with mptsas, it would
> not fix cxgb4 because there are other code paths that check for pending
> signals and that abort (ex. pci_vpd_pci22_wait()).
> 
> Reverting 786235ee effectively works around the problem by making
> modprobe unkillable. The proper solution would be to make sure that udev
> does not send sigkill to modprobe in the first place, either by making
> the timeout longer or by making the module probe faster.

Hannes sent a patch for systemd that enables a kernel command line override
for the timeout, this however still means some drivers can fail and distros
would have to use the longest known timeout for the supported kernel.

http://lists.freedesktop.org/archives/systemd-devel/2014-July/021601.html

Tetsuo is it possible / desirable to allow tasks to not kill unless the
reason is OOM ? Its unclear if this was discussed before, sorry if it was,
have just been a bit busy today to review the archive / discussions on this.

To *fatally* kill a module if it does not reach a time limit is rather harsh
without properly thinking about the entire picture of possible issues and 
reasons for the timeout and also consequences of the kill, essentially
what has happened is we are breaking ome boots on at least storage drivers
that take long, and now networking on one driver at least. I think we all
agree these drivers need fixing, there is no one arguing over that.
but to allow a timeout to fatally kill the damn system seems rather
stupid too if what we want to do is to get drivers fixed. It is both *hard
to debug* (see the bug reports) and simply just irritating to users.

The original commit on systemd that introduced the timeout is commit
e64fae55 but the purpose of that commit was to send to hell drivers
that are not using asynch firmware loading, but this is not the only
reason why some drivers would hit the timeout limit. As Benjamin notes
the cxgb4 driver issue is a bit more complex than that, as I've noted
I've sent some initial patches to help with asynch firmware but proper
integration is a bit more complex and even if we remove firmware out
of the picture (this was tried) the driver *still* takes more than
30 seconds to load and fails as Benjamin indicated. As Greg notes a bus
driver stub can be written -- but this will take a bit of time folks,
even if its a day or two, or a week or to just test things. In the
meantime we simply have broken systems / networking as collateral to
a CVE patch that in turn allowed systemd to also kill drivers on a
30 second timeout under the assumption it was all asynch firmware
loading. Collateral should not be the way to introduce new driver
requirements, specially if its breaking boots. All I'm saying is we
should just try to warn here, and not be fatal.

Hannes' patch will allow us to override the timeout through the comand
line but we're essentially still killing drivers that don't meet the new
implicit rules. This doesn't seem optimal and hence the discussion.

  Luis

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probefrom init

[Tetsuo Handa]

Luis R. Rodriguez wrote:
> Tetsuo is it possible / desirable to allow tasks to not kill unless the
> reason is OOM ? Its unclear if this was discussed before, sorry if it was,
> have just been a bit busy today to review the archive / discussions on this.

Are we aware that the 10 seconds timeout after SIGKILL is not the duration
between the beginning of module loading and the end of kthread_create() but
the duration to wait for kthreadd to create a new kernel thread?

If the kthreadd is unable to create a new kernel thread within 10 seconds,
something very bad is happening. For example, memory allocation deadlock
sequence shown below might be happening.

 (1) process1 holds a mutex using mutex_lock().
 (2) process1 calls kthread_create() and enters into killable wait state
     at wait_for_completion_killable().
 (3) kthreadd calls kernel_thread() and enters into oom-killable busy loop
     due to out of memory at alloc_pages_nodemask().
 (4) process2 is chosen by the OOM killer, but process2 is unable to
     terminate because process2 is waiting in unkillable state at
     mutex_lock() which was held by process1 at (1).
 (5) kthreadd continues busy loop because process2 does not release memory
     and the OOM killer does not kill more processes.
 (6) process1 continues waiting in oom-killable state because process1 is
     not chosen by the OOM killer.

See? The system will remain unresponding unless somebody releases memory
that is enough for kthreadd to complete. We cannot teach process1 that
process1 needs to give up waiting for kthreadd and call mutex_unlock()
in order to allow process2 to terminate. Also, we cannot teach the OOM
killer that process1 needs to be oom-killed after process2 is oom-killed.

Making the 10 seconds timeout after SIGKILL longer is safe.
Changing it to no-timeout-unless-oom-killed is unsafe.

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probefrom init

[Luis R. Rodriguez]

On Wed, Jul 30, 2014 at 11:22:14AM +0900, Tetsuo Handa wrote:
> Luis R. Rodriguez wrote:
> > Tetsuo is it possible / desirable to allow tasks to not kill unless the
> > reason is OOM ? Its unclear if this was discussed before, sorry if it was,
> > have just been a bit busy today to review the archive / discussions on this.
> 
> Are we aware that the 10 seconds timeout after SIGKILL is not the duration
> between the beginning of module loading and the end of kthread_create() but
> the duration to wait for kthreadd to create a new kernel thread?
> 
> If the kthreadd is unable to create a new kernel thread within 10 seconds,
> something very bad is happening. For example, memory allocation deadlock
> sequence shown below might be happening.
> 
>  (1) process1 holds a mutex using mutex_lock().
>  (2) process1 calls kthread_create() and enters into killable wait state
>      at wait_for_completion_killable().
>  (3) kthreadd calls kernel_thread() and enters into oom-killable busy loop
>      due to out of memory at alloc_pages_nodemask().
>  (4) process2 is chosen by the OOM killer, but process2 is unable to
>      terminate because process2 is waiting in unkillable state at
>      mutex_lock() which was held by process1 at (1).
>  (5) kthreadd continues busy loop because process2 does not release memory
>      and the OOM killer does not kill more processes.
>  (6) process1 continues waiting in oom-killable state because process1 is
>      not chosen by the OOM killer.
> 
> See? The system will remain unresponding unless somebody releases memory
> that is enough for kthreadd to complete.

I see but we're talking about large systems with gobs of memory so I'm pretty
sure memory should not be the issue here.

> We cannot teach process1 that
> process1 needs to give up waiting for kthreadd and call mutex_unlock()
> in order to allow process2 to terminate. Also, we cannot teach the OOM
> killer that process1 needs to be oom-killed after process2 is oom-killed.
> 
> Making the 10 seconds timeout after SIGKILL longer is safe.
> Changing it to no-timeout-unless-oom-killed is unsafe.

To be clear we have *not* merged the 10 second workaround:

https://launchpadlibrarian.net/169714201/kthread-Do-not-leave-kthread_create-immediately.patch

and come to think of it the work aroaund is aligned with what I was thinking
*without *waiting for 10 seconds, but my question was whether or not it was
reasonable to have the process request to go through this excemption.
So we would not do this all the time, but only for processes that would
request this, in this case modprobe.

  Luis

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[David Miller]

From: "Luis R. Rodriguez" <mcgrof@do-not-panic.com>
Date: Mon, 28 Jul 2014 11:28:28 -0700

> 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.

It seems we're still discussing this.

I think I understand all of the underlying issues, and what I'll say
is that perhaps we should use what Greg KH requested but via a helper
that is easy to grep for.

I don't care if it's something like "module_long_probe_init()" and
"module_long_probe_exit()", but it just needs to be some properly
named interface which does the whole kthread or whatever bit.

Thanks.

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[Luis R. Rodriguez]

On Wed, Jul 30, 2014 at 03:11:07PM -0700, David Miller wrote:
> From: "Luis R. Rodriguez" <mcgrof@do-not-panic.com>
> Date: Mon, 28 Jul 2014 11:28:28 -0700
> 
> > 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.
> 
> It seems we're still discussing this.
> 
> I think I understand all of the underlying issues, and what I'll say
> is that perhaps we should use what Greg KH requested but via a helper
> that is easy to grep for.
> 
> I don't care if it's something like "module_long_probe_init()" and
> "module_long_probe_exit()", but it just needs to be some properly
> named interface which does the whole kthread or whatever bit.

I've tested the alternative kthread_run() proposal but unfortunately it
does not help resolve the issue, the timeout is still hit and a SIGKILL
still kills the driver probe. Please let me know how you'd all like us
to proceed, these defer probe patches do help cure the issue though.

I should also note that these work around patches can only be done once we
already know a driver fails to go over the timeout, root causing and
associating driver issues to the timeout has been very difficult with a few
drivers already, for this reason I've submitted a change for systemd to issue a
warning instead of killing kmod usage on udev after a timeout, that would make
this regression non-fatal, and let us more easily then hunt drivers that need
fixing much easily [0] [1]. As noted we'd still want to have drivers easily
annotated which require fixing, this orignal series would allow us to do that
by hunting for delay_probe. If there alternative and preferred strategies
please let me know.

[0] http://lists.freedesktop.org/archives/systemd-devel/2014-August/021812.html
[1] http://lists.freedesktop.org/archives/systemd-devel/2014-August/021821.html

  Luis

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[Greg KH]

On Sat, Aug 09, 2014 at 06:41:19PM +0200, Luis R. Rodriguez wrote:
> On Wed, Jul 30, 2014 at 03:11:07PM -0700, David Miller wrote:
> > From: "Luis R. Rodriguez" <mcgrof@do-not-panic.com>
> > Date: Mon, 28 Jul 2014 11:28:28 -0700
> > 
> > > 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.
> > 
> > It seems we're still discussing this.
> > 
> > I think I understand all of the underlying issues, and what I'll say
> > is that perhaps we should use what Greg KH requested but via a helper
> > that is easy to grep for.
> > 
> > I don't care if it's something like "module_long_probe_init()" and
> > "module_long_probe_exit()", but it just needs to be some properly
> > named interface which does the whole kthread or whatever bit.
> 
> I've tested the alternative kthread_run() proposal but unfortunately it
> does not help resolve the issue, the timeout is still hit and a SIGKILL
> still kills the driver probe. Please let me know how you'd all like us
> to proceed, these defer probe patches do help cure the issue though.

Why doesn't it work?  Doesn't modprobe come right back and the init
sequence still takes a while to run?  What exactly fails?

thanks,

greg k-h

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probefrom init

[Tetsuo Handa]

Greg KH wrote:
> Why doesn't it work?  Doesn't modprobe come right back and the init
> sequence still takes a while to run?  What exactly fails?

I guess ...

> @@ -5429,9 +5429,19 @@ mptsas_init(void)
>  	return error;
>  }
>  
> +static struct task_struct *init_thread;
> +
> +static int __init
> +mptsas_init(void)
> +{
> +	init_thread = kthread_run(mptsas_real_init, NULL, "mptsas_init");
> +	return 0;
> +}
> +
>  static void __exit
>  mptsas_exit(void)
>  {
> +	kthread_stop(init_thread);
>  	pci_unregister_driver(&mptsas_driver);
>  	sas_release_transport(mptsas_transport_template);
>  
> 

kthread_run() can fail. sas_attach_transport() and/or pci_register_driver()
in mptsas_real_init() can fail. Caller process may fail to continue if
sas_attach_transport() and pci_register_driver() in mptsas_real_init() has
not completed yet.

kthread_stop() must not be called when kthread_run() failed.
pci_unregister_driver() and/or sas_release_transport() must not be called
when mptsas_real_init() did not return 0 (or has not returned 0 yet).

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[Luis R. Rodriguez]

On Sun, Aug 10, 2014 at 08:43:31PM +0800, Greg KH wrote:
> On Sat, Aug 09, 2014 at 06:41:19PM +0200, Luis R. Rodriguez wrote:
> > On Wed, Jul 30, 2014 at 03:11:07PM -0700, David Miller wrote:
> > > From: "Luis R. Rodriguez" <mcgrof@do-not-panic.com>
> > > Date: Mon, 28 Jul 2014 11:28:28 -0700
> > > 
> > > > 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.
> > > 
> > > It seems we're still discussing this.
> > > 
> > > I think I understand all of the underlying issues, and what I'll say
> > > is that perhaps we should use what Greg KH requested but via a helper
> > > that is easy to grep for.
> > > 
> > > I don't care if it's something like "module_long_probe_init()" and
> > > "module_long_probe_exit()", but it just needs to be some properly
> > > named interface which does the whole kthread or whatever bit.
> > 
> > I've tested the alternative kthread_run() proposal but unfortunately it
> > does not help resolve the issue, the timeout is still hit and a SIGKILL
> > still kills the driver probe. Please let me know how you'd all like us
> > to proceed, these defer probe patches do help cure the issue though.
> 
> Why doesn't it work?  Doesn't modprobe come right back and the init
> sequence still takes a while to run?  What exactly fails?

systemd uses kmod kmod_module_probe_insert_module(), what I see is that using
kthread_run() as a work around still causes probe to fail on a driver that
otherwise would work fine if all you do is sprinkle ssleep(33) (seconds) on the
init sequence. To see the issue you can test this on any of your network
drivers that get loaded automatically, I did my testing with iwlwifi by
purposely breaking it and then using the work around. It seems the probe
somehow still gets killed as before, I haven't debugged this further.

For example by breaking and fixing iwlwifi this yields:

ergon:~ # journalctl -b -0 -u systemd-udevd 

-- Logs begin at Mon 2014-08-04 21:55:28 EDT, end at Sun 2014-08-10 10:50:14 EDT. --
Aug 10 10:48:49 ergon systemd-udevd[257]: specified group 'input' unknown
Aug 10 10:48:55 ergon mtp-probe[493]: checking bus 3, device 2: "/sys/devices/pci0000:00/0000:00:14.0/usb3/3-7"
Aug 10 10:48:55 ergon mtp-probe[492]: checking bus 3, device 4: "/sys/devices/pci0000:00/0000:00:14.0/usb3/3-12"
Aug 10 10:49:24 ergon systemd-udevd[465]: seq 1755 '/devices/pci0000:00/0000:00:1c.1/0000:03:00.0' killed

ergon:~ # dmesg | grep iwlwifi
[   10.315538] iwlwifi Going to sleep for 33 seconds
[   43.323958] iwlwifi Done sleeping
[   43.324400] iwlwifi 0000:03:00.0: irq 50 for MSI/MSI-X
[   43.324534] iwlwifi 0000:03:00.0: Error allocating IRQ 50
[   43.326951] iwlwifi: probe of 0000:03:00.0 failed with error -4

The patch used:

diff --git a/drivers/net/wireless/iwlwifi/mvm/ops.c b/drivers/net/wireless/iwlwifi/mvm/ops.c
index 610dbcb..65e0ab2 100644
--- a/drivers/net/wireless/iwlwifi/mvm/ops.c
+++ b/drivers/net/wireless/iwlwifi/mvm/ops.c
@@ -63,6 +63,7 @@
 #include <linux/module.h>
 #include <linux/vmalloc.h>
 #include <net/mac80211.h>
+#include <linux/kthread.h>
 
 #include "iwl-notif-wait.h"
 #include "iwl-trans.h"
@@ -111,7 +112,7 @@ MODULE_PARM_DESC(power_scheme,
 /*
  * module init and exit functions
  */
-static int __init iwl_mvm_init(void)
+static int iwl_mvm_init_real(void *arg)
 {
 	int ret;
 
@@ -130,12 +131,21 @@ static int __init iwl_mvm_init(void)
 
 	return ret;
 }
+
+static struct task_struct *init_thread;
+
+static int __init iwl_mvm_init(void)
+{
+	init_thread = kthread_run(iwl_mvm_init_real, NULL, "iwl_mvm_init");
+	return 0;
+}
 module_init(iwl_mvm_init);
 
 static void __exit iwl_mvm_exit(void)
 {
 	iwl_opmode_deregister("iwlmvm");
 	iwl_mvm_rate_control_unregister();
+	kthread_stop(init_thread);
 }
 module_exit(iwl_mvm_exit);
 
diff --git a/drivers/net/wireless/iwlwifi/pcie/drv.c b/drivers/net/wireless/iwlwifi/pcie/drv.c
index 98950e4..c1f2823 100644
--- a/drivers/net/wireless/iwlwifi/pcie/drv.c
+++ b/drivers/net/wireless/iwlwifi/pcie/drv.c
@@ -489,6 +489,10 @@ static int iwl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	struct iwl_trans_pcie *trans_pcie;
 	int ret;
 
+	printk("iwlwifi Going to sleep for 33 seconds\n");
+	ssleep(33);
+	printk("iwlwifi Done sleeping\n");
+
 	iwl_trans = iwl_trans_pcie_alloc(pdev, ent, cfg);
 	if (IS_ERR(iwl_trans))
 		return PTR_ERR(iwl_trans);

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[Takashi Iwai]

At Sun, 10 Aug 2014 16:58:02 +0200,
Luis R. Rodriguez wrote:
> 
> On Sun, Aug 10, 2014 at 08:43:31PM +0800, Greg KH wrote:
> > On Sat, Aug 09, 2014 at 06:41:19PM +0200, Luis R. Rodriguez wrote:
> > > On Wed, Jul 30, 2014 at 03:11:07PM -0700, David Miller wrote:
> > > > From: "Luis R. Rodriguez" <mcgrof@do-not-panic.com>
> > > > Date: Mon, 28 Jul 2014 11:28:28 -0700
> > > > 
> > > > > 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.
> > > > 
> > > > It seems we're still discussing this.
> > > > 
> > > > I think I understand all of the underlying issues, and what I'll say
> > > > is that perhaps we should use what Greg KH requested but via a helper
> > > > that is easy to grep for.
> > > > 
> > > > I don't care if it's something like "module_long_probe_init()" and
> > > > "module_long_probe_exit()", but it just needs to be some properly
> > > > named interface which does the whole kthread or whatever bit.
> > > 
> > > I've tested the alternative kthread_run() proposal but unfortunately it
> > > does not help resolve the issue, the timeout is still hit and a SIGKILL
> > > still kills the driver probe. Please let me know how you'd all like us
> > > to proceed, these defer probe patches do help cure the issue though.
> > 
> > Why doesn't it work?  Doesn't modprobe come right back and the init
> > sequence still takes a while to run?  What exactly fails?
> 
> systemd uses kmod kmod_module_probe_insert_module(), what I see is that using
> kthread_run() as a work around still causes probe to fail on a driver that
> otherwise would work fine if all you do is sprinkle ssleep(33) (seconds) on the
> init sequence. To see the issue you can test this on any of your network
> drivers that get loaded automatically, I did my testing with iwlwifi by
> purposely breaking it and then using the work around. It seems the probe
> somehow still gets killed as before, I haven't debugged this further.
> 
> For example by breaking and fixing iwlwifi this yields:
> 
> ergon:~ # journalctl -b -0 -u systemd-udevd 
> 
> -- Logs begin at Mon 2014-08-04 21:55:28 EDT, end at Sun 2014-08-10 10:50:14 EDT. --
> Aug 10 10:48:49 ergon systemd-udevd[257]: specified group 'input' unknown
> Aug 10 10:48:55 ergon mtp-probe[493]: checking bus 3, device 2: "/sys/devices/pci0000:00/0000:00:14.0/usb3/3-7"
> Aug 10 10:48:55 ergon mtp-probe[492]: checking bus 3, device 4: "/sys/devices/pci0000:00/0000:00:14.0/usb3/3-12"
> Aug 10 10:49:24 ergon systemd-udevd[465]: seq 1755 '/devices/pci0000:00/0000:00:1c.1/0000:03:00.0' killed
> 
> ergon:~ # dmesg | grep iwlwifi
> [   10.315538] iwlwifi Going to sleep for 33 seconds
> [   43.323958] iwlwifi Done sleeping
> [   43.324400] iwlwifi 0000:03:00.0: irq 50 for MSI/MSI-X
> [   43.324534] iwlwifi 0000:03:00.0: Error allocating IRQ 50
> [   43.326951] iwlwifi: probe of 0000:03:00.0 failed with error -4
> 
> The patch used:
> 
> diff --git a/drivers/net/wireless/iwlwifi/mvm/ops.c b/drivers/net/wireless/iwlwifi/mvm/ops.c
> index 610dbcb..65e0ab2 100644
> --- a/drivers/net/wireless/iwlwifi/mvm/ops.c
> +++ b/drivers/net/wireless/iwlwifi/mvm/ops.c
> @@ -63,6 +63,7 @@
>  #include <linux/module.h>
>  #include <linux/vmalloc.h>
>  #include <net/mac80211.h>
> +#include <linux/kthread.h>
>  
>  #include "iwl-notif-wait.h"
>  #include "iwl-trans.h"
> @@ -111,7 +112,7 @@ MODULE_PARM_DESC(power_scheme,
>  /*
>   * module init and exit functions
>   */
> -static int __init iwl_mvm_init(void)
> +static int iwl_mvm_init_real(void *arg)
>  {
>  	int ret;
>  
> @@ -130,12 +131,21 @@ static int __init iwl_mvm_init(void)
>  
>  	return ret;
>  }
> +
> +static struct task_struct *init_thread;
> +
> +static int __init iwl_mvm_init(void)
> +{
> +	init_thread = kthread_run(iwl_mvm_init_real, NULL, "iwl_mvm_init");
> +	return 0;
> +}
>  module_init(iwl_mvm_init);
>  
>  static void __exit iwl_mvm_exit(void)
>  {
>  	iwl_opmode_deregister("iwlmvm");
>  	iwl_mvm_rate_control_unregister();
> +	kthread_stop(init_thread);
>  }
>  module_exit(iwl_mvm_exit);
>  
> diff --git a/drivers/net/wireless/iwlwifi/pcie/drv.c b/drivers/net/wireless/iwlwifi/pcie/drv.c
> index 98950e4..c1f2823 100644
> --- a/drivers/net/wireless/iwlwifi/pcie/drv.c
> +++ b/drivers/net/wireless/iwlwifi/pcie/drv.c
> @@ -489,6 +489,10 @@ static int iwl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	struct iwl_trans_pcie *trans_pcie;
>  	int ret;
>  
> +	printk("iwlwifi Going to sleep for 33 seconds\n");
> +	ssleep(33);
> +	printk("iwlwifi Done sleeping\n");
> +
>  	iwl_trans = iwl_trans_pcie_alloc(pdev, ent, cfg);
>  	if (IS_ERR(iwl_trans))
>  		return PTR_ERR(iwl_trans);
> 

I guess that the problem is that you moved the init stuff into kthread
for iwlmvm module, but the stall happens in a different module,
iwlwifi.  So iwlwifi is killed as expected.  You had to put the
kthread hack into pcie/drv.c instead.

(Actually, it's interesting how the SIGKILL error is reported back;
 it failed at request_threaded_irq().  If this were the normal irq
 handler, it might have worked :)


Takashi

Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[Luis R. Rodriguez]

On Mon, Aug 11, 2014 at 11:27 AM, Takashi Iwai <tiwai@suse.de> wrote:
> Luis R. Rodriguez wrote:
>>
>> On Sun, Aug 10, 2014 at 08:43:31PM +0800, Greg KH wrote:
>> > On Sat, Aug 09, 2014 at 06:41:19PM +0200, Luis R. Rodriguez wrote:
>> > > On Wed, Jul 30, 2014 at 03:11:07PM -0700, David Miller wrote:
>> > > > From: "Luis R. Rodriguez" <mcgrof@do-not-panic.com>
>> > > > Date: Mon, 28 Jul 2014 11:28:28 -0700
>> > > >
>> > > > > 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.
>> > > >
>> > > > It seems we're still discussing this.
>> > > >
>> > > > I think I understand all of the underlying issues, and what I'll say
>> > > > is that perhaps we should use what Greg KH requested but via a helper
>> > > > that is easy to grep for.
>> > > >
>> > > > I don't care if it's something like "module_long_probe_init()" and
>> > > > "module_long_probe_exit()", but it just needs to be some properly
>> > > > named interface which does the whole kthread or whatever bit.
>> > >
>> > > I've tested the alternative kthread_run() proposal but unfortunately it
>> > > does not help resolve the issue, the timeout is still hit and a SIGKILL
>> > > still kills the driver probe. Please let me know how you'd all like us
>> > > to proceed, these defer probe patches do help cure the issue though.
>> >
>> > Why doesn't it work?  Doesn't modprobe come right back and the init
>> > sequence still takes a while to run?  What exactly fails?
>>
>> systemd uses kmod kmod_module_probe_insert_module(), what I see is that using
>> kthread_run() as a work around still causes probe to fail on a driver that
>> otherwise would work fine if all you do is sprinkle ssleep(33) (seconds) on the
>> init sequence. To see the issue you can test this on any of your network
>> drivers that get loaded automatically, I did my testing with iwlwifi by
>> purposely breaking it and then using the work around. It seems the probe
>> somehow still gets killed as before, I haven't debugged this further.
>>
>> For example by breaking and fixing iwlwifi this yields:
>>
>> ergon:~ # journalctl -b -0 -u systemd-udevd
>>
>> -- Logs begin at Mon 2014-08-04 21:55:28 EDT, end at Sun 2014-08-10 10:50:14 EDT. --
>> Aug 10 10:48:49 ergon systemd-udevd[257]: specified group 'input' unknown
>> Aug 10 10:48:55 ergon mtp-probe[493]: checking bus 3, device 2: "/sys/devices/pci0000:00/0000:00:14.0/usb3/3-7"
>> Aug 10 10:48:55 ergon mtp-probe[492]: checking bus 3, device 4: "/sys/devices/pci0000:00/0000:00:14.0/usb3/3-12"
>> Aug 10 10:49:24 ergon systemd-udevd[465]: seq 1755 '/devices/pci0000:00/0000:00:1c.1/0000:03:00.0' killed
>>
>> ergon:~ # dmesg | grep iwlwifi
>> [   10.315538] iwlwifi Going to sleep for 33 seconds
>> [   43.323958] iwlwifi Done sleeping
>> [   43.324400] iwlwifi 0000:03:00.0: irq 50 for MSI/MSI-X
>> [   43.324534] iwlwifi 0000:03:00.0: Error allocating IRQ 50
>> [   43.326951] iwlwifi: probe of 0000:03:00.0 failed with error -4
>>
>> The patch used:
>>
>> diff --git a/drivers/net/wireless/iwlwifi/mvm/ops.c b/drivers/net/wireless/iwlwifi/mvm/ops.c
>> index 610dbcb..65e0ab2 100644
>> --- a/drivers/net/wireless/iwlwifi/mvm/ops.c
>> +++ b/drivers/net/wireless/iwlwifi/mvm/ops.c
>> @@ -63,6 +63,7 @@
>>  #include <linux/module.h>
>>  #include <linux/vmalloc.h>
>>  #include <net/mac80211.h>
>> +#include <linux/kthread.h>
>>
>>  #include "iwl-notif-wait.h"
>>  #include "iwl-trans.h"
>> @@ -111,7 +112,7 @@ MODULE_PARM_DESC(power_scheme,
>>  /*
>>   * module init and exit functions
>>   */
>> -static int __init iwl_mvm_init(void)
>> +static int iwl_mvm_init_real(void *arg)
>>  {
>>       int ret;
>>
>> @@ -130,12 +131,21 @@ static int __init iwl_mvm_init(void)
>>
>>       return ret;
>>  }
>> +
>> +static struct task_struct *init_thread;
>> +
>> +static int __init iwl_mvm_init(void)
>> +{
>> +     init_thread = kthread_run(iwl_mvm_init_real, NULL, "iwl_mvm_init");
>> +     return 0;
>> +}
>>  module_init(iwl_mvm_init);
>>
>>  static void __exit iwl_mvm_exit(void)
>>  {
>>       iwl_opmode_deregister("iwlmvm");
>>       iwl_mvm_rate_control_unregister();
>> +     kthread_stop(init_thread);
>>  }
>>  module_exit(iwl_mvm_exit);
>>
>> diff --git a/drivers/net/wireless/iwlwifi/pcie/drv.c b/drivers/net/wireless/iwlwifi/pcie/drv.c
>> index 98950e4..c1f2823 100644
>> --- a/drivers/net/wireless/iwlwifi/pcie/drv.c
>> +++ b/drivers/net/wireless/iwlwifi/pcie/drv.c
>> @@ -489,6 +489,10 @@ static int iwl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>       struct iwl_trans_pcie *trans_pcie;
>>       int ret;
>>
>> +     printk("iwlwifi Going to sleep for 33 seconds\n");
>> +     ssleep(33);
>> +     printk("iwlwifi Done sleeping\n");
>> +
>>       iwl_trans = iwl_trans_pcie_alloc(pdev, ent, cfg);
>>       if (IS_ERR(iwl_trans))
>>               return PTR_ERR(iwl_trans);
>>
>
> I guess that the problem is that you moved the init stuff into kthread
> for iwlmvm module, but the stall happens in a different module,
> iwlwifi.  So iwlwifi is killed as expected.  You had to put the
> kthread hack into pcie/drv.c instead.

Indeed, will work on a general solution next, thanks!

> (Actually, it's interesting how the SIGKILL error is reported back;
>  it failed at request_threaded_irq().  If this were the normal irq
>  handler, it might have worked :)

:)

  Luis