[PATCH] ioc4: automatically load sgiioc4 subordinate module

[PATCH] ioc4: automatically load sgiioc4 subordinate module

[Brent Casavant]

The main ioc4 driver which manages ownership of the SGI IOC4 PCI device
does not automatically load any of the ioc4 submodules (the sgiioc4 IDE
and ioc4_serial serial drivers) during PCI device probing.  This causes
problems when the root device is a DVD-ROM attached to the IOC4 IDE
interface (e.g. during system installation) as the sgiioc4 module
will not be loaded and thus the DVD-ROM device will not be available.

Modify ioc4 to always load the sgiioc4 IDE module if the board carrying
the IOC4 hardware actually implements the IDE interface (not all boards
bring this functionality off the IOC4 chip).

The use of a work procedure is necessary as request_module() cannot be
called from the device probe path as it eventually calls out to userspace.

Signed-off-by: Michael Reed <mdr@sgi.com>
Signed-off-by: Brent Casavant <bcasavan@sgi.com>
---
 ioc4.c |   26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/misc/ioc4.c b/drivers/misc/ioc4.c
index 6f76573..36320bd 100644
--- a/drivers/misc/ioc4.c
+++ b/drivers/misc/ioc4.c
@@ -269,6 +269,16 @@ ioc4_variant(struct ioc4_driver_data *idd)
 	return IOC4_VARIANT_PCI_RT;
 }
 
+static void
+ioc4_load_modules(struct work_struct *work)
+{
+	/* arg just has to be freed */
+
+	request_module("sgiioc4");
+
+	kfree(work);
+}
+
 /* Adds a new instance of an IOC4 card */
 static int
 ioc4_probe(struct pci_dev *pdev, const struct pci_device_id *pci_id)
@@ -378,6 +388,22 @@ ioc4_probe(struct pci_dev *pdev, const struct pci_device_id *pci_id)
 	}
 	mutex_unlock(&ioc4_mutex);
 
+	if (idd->idd_variant != IOC4_VARIANT_PCI_RT) {
+		struct work_struct *work;
+		work = kzalloc(sizeof(struct work_struct), GFP_KERNEL);
+		if (!work) {
+			printk(KERN_WARNING
+			       "%s: IOC4 unable to allocate memory for "
+			       "load of sub-modules.\n",
+			       __FUNCTION__);
+		}
+		else {
+			printk(KERN_INFO "IOC4 loading ioc4 submodule\n");
+			INIT_WORK(work, ioc4_load_modules);
+			schedule_work(work);
+		}
+	}
+
 	return 0;
 
 out_misc_region:

-- 
Brent Casavant                          All music is folk music.  I ain't
bcasavan@sgi.com                        never heard a horse sing a song.
Silicon Graphics, Inc.                    -- Louis Armstrong

Re: [PATCH] ioc4: automatically load sgiioc4 subordinate module

[Andrew Morton]

On Fri, 5 Dec 2008 16:04:37 -0600 (CST)
Brent Casavant <bcasavan@sgi.com> wrote:

> The main ioc4 driver which manages ownership of the SGI IOC4 PCI device
> does not automatically load any of the ioc4 submodules (the sgiioc4 IDE
> and ioc4_serial serial drivers) during PCI device probing.  This causes
> problems when the root device is a DVD-ROM attached to the IOC4 IDE
> interface (e.g. during system installation) as the sgiioc4 module
> will not be loaded and thus the DVD-ROM device will not be available.
> 
> Modify ioc4 to always load the sgiioc4 IDE module if the board carrying
> the IOC4 hardware actually implements the IDE interface (not all boards
> bring this functionality off the IOC4 chip).
> 
> The use of a work procedure is necessary as request_module() cannot be
> called from the device probe path as it eventually calls out to userspace.
> 

This is a fairly common problem, isn't it?  Other drivers handle it via
appropriate udev magic and initrd/initramfs setups.

Is there something special about ioc4 which requires that it have this
magical special handling?

> ---
>  ioc4.c |   26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/misc/ioc4.c b/drivers/misc/ioc4.c
> index 6f76573..36320bd 100644
> --- a/drivers/misc/ioc4.c
> +++ b/drivers/misc/ioc4.c
> @@ -269,6 +269,16 @@ ioc4_variant(struct ioc4_driver_data *idd)
>  	return IOC4_VARIANT_PCI_RT;
>  }
>  
> +static void
> +ioc4_load_modules(struct work_struct *work)
> +{
> +	/* arg just has to be freed */
> +
> +	request_module("sgiioc4");
> +
> +	kfree(work);
> +}
> +
>  /* Adds a new instance of an IOC4 card */
>  static int
>  ioc4_probe(struct pci_dev *pdev, const struct pci_device_id *pci_id)
> @@ -378,6 +388,22 @@ ioc4_probe(struct pci_dev *pdev, const struct pci_device_id *pci_id)
>  	}
>  	mutex_unlock(&ioc4_mutex);
>  
> +	if (idd->idd_variant != IOC4_VARIANT_PCI_RT) {
> +		struct work_struct *work;
> +		work = kzalloc(sizeof(struct work_struct), GFP_KERNEL);
> +		if (!work) {
> +			printk(KERN_WARNING
> +			       "%s: IOC4 unable to allocate memory for "
> +			       "load of sub-modules.\n",
> +			       __FUNCTION__);
> +		}
> +		else {
> +			printk(KERN_INFO "IOC4 loading ioc4 submodule\n");
> +			INIT_WORK(work, ioc4_load_modules);
> +			schedule_work(work);
> +		}
> +	}
> +
>  	return 0;
>  

ioc4 can be compiled as a module.  So if someone does an `rmmod ioc4' after
the schedule_work() and before or during the execution of the work
function, dead machine.

This race is fixable by adding a flush_scheduled_work() into
ioc4_exit(), I expect.

Re: [PATCH] ioc4: automatically load sgiioc4 subordinate module

[Brent Casavant]

Mike, Raymond, or Kay, could you take this question?

I mostly just passed along Mike's patch, so I'm not 100% familiar with
why those other mechanisms Andrew mentions aren't being employed.

Brent

On Mon, 8 Dec 2008, Andrew Morton wrote:

> On Fri, 5 Dec 2008 16:04:37 -0600 (CST)
> Brent Casavant <bcasavan@sgi.com> wrote:
> 
> > The main ioc4 driver which manages ownership of the SGI IOC4 PCI device
> > does not automatically load any of the ioc4 submodules (the sgiioc4 IDE
> > and ioc4_serial serial drivers) during PCI device probing.  This causes
> > problems when the root device is a DVD-ROM attached to the IOC4 IDE
> > interface (e.g. during system installation) as the sgiioc4 module
> > will not be loaded and thus the DVD-ROM device will not be available.
> > 
> > Modify ioc4 to always load the sgiioc4 IDE module if the board carrying
> > the IOC4 hardware actually implements the IDE interface (not all boards
> > bring this functionality off the IOC4 chip).
> > 
> > The use of a work procedure is necessary as request_module() cannot be
> > called from the device probe path as it eventually calls out to userspace.
> > 
> 
> This is a fairly common problem, isn't it?  Other drivers handle it via
> appropriate udev magic and initrd/initramfs setups.
> 
> Is there something special about ioc4 which requires that it have this
> magical special handling?
> 
> > ---
> >  ioc4.c |   26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> > 
> > diff --git a/drivers/misc/ioc4.c b/drivers/misc/ioc4.c
> > index 6f76573..36320bd 100644
> > --- a/drivers/misc/ioc4.c
> > +++ b/drivers/misc/ioc4.c
> > @@ -269,6 +269,16 @@ ioc4_variant(struct ioc4_driver_data *idd)
> >  	return IOC4_VARIANT_PCI_RT;
> >  }
> >  
> > +static void
> > +ioc4_load_modules(struct work_struct *work)
> > +{
> > +	/* arg just has to be freed */
> > +
> > +	request_module("sgiioc4");
> > +
> > +	kfree(work);
> > +}
> > +
> >  /* Adds a new instance of an IOC4 card */
> >  static int
> >  ioc4_probe(struct pci_dev *pdev, const struct pci_device_id *pci_id)
> > @@ -378,6 +388,22 @@ ioc4_probe(struct pci_dev *pdev, const struct pci_device_id *pci_id)
> >  	}
> >  	mutex_unlock(&ioc4_mutex);
> >  
> > +	if (idd->idd_variant != IOC4_VARIANT_PCI_RT) {
> > +		struct work_struct *work;
> > +		work = kzalloc(sizeof(struct work_struct), GFP_KERNEL);
> > +		if (!work) {
> > +			printk(KERN_WARNING
> > +			       "%s: IOC4 unable to allocate memory for "
> > +			       "load of sub-modules.\n",
> > +			       __FUNCTION__);
> > +		}
> > +		else {
> > +			printk(KERN_INFO "IOC4 loading ioc4 submodule\n");
> > +			INIT_WORK(work, ioc4_load_modules);
> > +			schedule_work(work);
> > +		}
> > +	}
> > +
> >  	return 0;
> >  
> 
> ioc4 can be compiled as a module.  So if someone does an `rmmod ioc4' after
> the schedule_work() and before or during the execution of the work
> function, dead machine.
> 
> This race is fixable by adding a flush_scheduled_work() into
> ioc4_exit(), I expect.
> 
> 

-- 
Brent Casavant                          All music is folk music.  I ain't
bcasavan@sgi.com                        never heard a horse sing a song.
Silicon Graphics, Inc.                    -- Louis Armstrong

Re: [PATCH] ioc4: automatically load sgiioc4 subordinate module

[Kay Sievers]

On Mon, 2008-12-08 at 14:55 -0800, Andrew Morton wrote:
> On Fri, 5 Dec 2008 16:04:37 -0600 (CST)
> Brent Casavant <bcasavan@sgi.com> wrote:
> 
> > The main ioc4 driver which manages ownership of the SGI IOC4 PCI device
> > does not automatically load any of the ioc4 submodules (the sgiioc4 IDE
> > and ioc4_serial serial drivers) during PCI device probing.  This causes
> > problems when the root device is a DVD-ROM attached to the IOC4 IDE
> > interface (e.g. during system installation) as the sgiioc4 module
> > will not be loaded and thus the DVD-ROM device will not be available.
> > 
> > Modify ioc4 to always load the sgiioc4 IDE module if the board carrying
> > the IOC4 hardware actually implements the IDE interface (not all boards
> > bring this functionality off the IOC4 chip).
> > 
> > The use of a work procedure is necessary as request_module() cannot be
> > called from the device probe path as it eventually calls out to userspace.
> > 
> 
> This is a fairly common problem, isn't it?

It is, but it's getting better with recent drivers. It is only a
problem, when there is "magic hidden mid-layer device binding" in the
kernel, which is not exposed to the driver core.

In an ideal world, the driver would create a custom bus "ioc4" (struct
bus_type), with the devices hanging off this bus. These devices would
then trigger the loading of the sub-driver modules, and the sub-drivers
would bind to the device created on the "ioc4" bus, just by driver-core
logic.

That would be the proper logic to make it work without any special
magic, just by the generic uevent/modalias/modprobe mechanism.

> Other drivers handle it via
> appropriate udev magic and initrd/initramfs setups.

We recently got rid of almost all custom hacks in udev rules.

Initramfs still has a bunch, but it's also getting smaller. We are down
from hundreds of exceptions/tables/rules, to a few exceptions today.

We have a real problem of maintaining/distributing such exceptions in/to
the installed systems. The exceptions and the magic have been growing,
and every system out there got almost all of these rules installed. Over
the time people completely lost track which of the rules are still
needed, and which are not. The split between the people doing kernel and
userspace work really hits us here.

You are right, that it looks a bit weird if you look at it just from
inside the kernel, but as long as we don't have a sane way to instruct
userspace/udev/initramfs/the installer from the kernel, like dropping
magic instructions along with the driver to the installed system,
changes like this are very welcome, not to bury userspace people in
exceptions.

One other problem is kernel updates on released systems, which we should
make as easy as possible. In some cases the installed magic does no
longer work for some reason, some innocent looking change mostly. So we
very much prefer, if possible, the driver to be self-contained regarding
the special magic it needs.

If it is not possible/too much work to change a driver to integrate
properly with the driver core, which as a result usually does not need
any magic, we welcome every such change that moves the needed magic into
the kernel itself.

Thanks,
Kay

Re: [PATCH] ioc4: automatically load sgiioc4 subordinate module

[Andrew Morton]

On Tue, 09 Dec 2008 00:36:03 +0100
Kay Sievers <kay.sievers@vrfy.org> wrote:

> > This is a fairly common problem, isn't it?
> 
> [ ... ]

Useful, thanks.


Brent, please take a look at the schedule_work() race for v2 and we'll
do it that way.

Re: [PATCH] ioc4: automatically load sgiioc4 subordinate module

[Brent Casavant]

Modify ioc4 to always load the sgiioc4 IDE module if the board carrying
the IOC4 hardware actually implements the IDE interface (not all boards
bring this functionality off the IOC4 chip).  A drive hosted on the
IDE interface may contain the root filesystem, and sgiioc4 doesn't
load automatically as ioc4 owns the PCI device ID, not sgiioc4.

Signed-off-by: Michael Reed <mdr@sgi.com>
Signed-off-by: Brent Casavant <bcasavan@sgi.com>
---
Version 2: Add flush_scheduled_work() in ioc4_exit(), and add
explanatory comments.

 ioc4.c |   38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/misc/ioc4.c b/drivers/misc/ioc4.c
index 6f76573..652629c 100644
--- a/drivers/misc/ioc4.c
+++ b/drivers/misc/ioc4.c
@@ -269,6 +269,16 @@ ioc4_variant(struct ioc4_driver_data *idd)
 	return IOC4_VARIANT_PCI_RT;
 }
 
+static void
+ioc4_load_modules(struct work_struct *work)
+{
+	/* arg just has to be freed */
+
+	request_module("sgiioc4");
+
+	kfree(work);
+}
+
 /* Adds a new instance of an IOC4 card */
 static int
 ioc4_probe(struct pci_dev *pdev, const struct pci_device_id *pci_id)
@@ -378,6 +388,32 @@ ioc4_probe(struct pci_dev *pdev, const struct pci_device_id *pci_id)
 	}
 	mutex_unlock(&ioc4_mutex);
 
+	/* Request sgiioc4 IDE driver on boards that bring that functionality
+	 * off of IOC4.  The root filesystem may be hosted on a drive connected
+	 * to IOC4, so we need to make sure the sgiioc4 driver is loaded as it
+	 * won't be picked up by modprobes due to the ioc4 module owning the
+	 * PCI device.
+	 */
+	if (idd->idd_variant != IOC4_VARIANT_PCI_RT) {
+		struct work_struct *work;
+		work = kzalloc(sizeof(struct work_struct), GFP_KERNEL);
+		if (!work) {
+			printk(KERN_WARNING
+			       "%s: IOC4 unable to allocate memory for "
+			       "load of sub-modules.\n",
+			       __FUNCTION__);
+		}
+		else {
+			/* Request the module from a work procedure as the
+			 * modprobe goes out to a userland helper and that
+			 * will hang if done directly from ioc4_probe().
+			 */
+			printk(KERN_INFO "IOC4 loading sgiioc4 submodule\n");
+			INIT_WORK(work, ioc4_load_modules);
+			schedule_work(work);
+		}
+	}
+
 	return 0;
 
 out_misc_region:
@@ -462,6 +498,8 @@ ioc4_init(void)
 static void __devexit
 ioc4_exit(void)
 {
+	/* Ensure ioc4_load_modules() has completed before exiting */
+	flush_scheduled_work();
 	pci_unregister_driver(&ioc4_driver);
 }
 
-- 
Brent Casavant                          All music is folk music.  I ain't
bcasavan@sgi.com                        never heard a horse sing a song.
Silicon Graphics, Inc.                    -- Louis Armstrong

Re: [PATCH] ioc4: automatically load sgiioc4 subordinate module

[Brent Casavant]

On Mon, 8 Dec 2008, Andrew Morton wrote:

> On Fri, 5 Dec 2008 16:04:37 -0600 (CST)
> Brent Casavant <bcasavan@sgi.com> wrote:

> > ---
> >  ioc4.c |   26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> > 
> > diff --git a/drivers/misc/ioc4.c b/drivers/misc/ioc4.c
> > index 6f76573..36320bd 100644
> > --- a/drivers/misc/ioc4.c
> > +++ b/drivers/misc/ioc4.c
> > @@ -269,6 +269,16 @@ ioc4_variant(struct ioc4_driver_data *idd)
> >  	return IOC4_VARIANT_PCI_RT;
> >  }
> >  
> > +static void
> > +ioc4_load_modules(struct work_struct *work)
> > +{
> > +	/* arg just has to be freed */
> > +
> > +	request_module("sgiioc4");
> > +
> > +	kfree(work);
> > +}
> > +
> >  /* Adds a new instance of an IOC4 card */
> >  static int
> >  ioc4_probe(struct pci_dev *pdev, const struct pci_device_id *pci_id)
> > @@ -378,6 +388,22 @@ ioc4_probe(struct pci_dev *pdev, const struct pci_device_id *pci_id)
> >  	}
> >  	mutex_unlock(&ioc4_mutex);
> >  
> > +	if (idd->idd_variant != IOC4_VARIANT_PCI_RT) {
> > +		struct work_struct *work;
> > +		work = kzalloc(sizeof(struct work_struct), GFP_KERNEL);
> > +		if (!work) {
> > +			printk(KERN_WARNING
> > +			       "%s: IOC4 unable to allocate memory for "
> > +			       "load of sub-modules.\n",
> > +			       __FUNCTION__);
> > +		}
> > +		else {
> > +			printk(KERN_INFO "IOC4 loading ioc4 submodule\n");
> > +			INIT_WORK(work, ioc4_load_modules);
> > +			schedule_work(work);
> > +		}
> > +	}
> > +
> >  	return 0;
> >  
> 
> ioc4 can be compiled as a module.  So if someone does an `rmmod ioc4' after
> the schedule_work() and before or during the execution of the work
> function, dead machine.
> 
> This race is fixable by adding a flush_scheduled_work() into
> ioc4_exit(), I expect.

In this case I believe a better spot for the flush_schedule_work() is
immediately after calling schedule_work(), for two reasons.

First, and something I should have realized earlier, with the work
scheduled but not guaranteed to have completed, we're not sure
exactly when the IDE device will show up, and thus have no guarantee
when the desired root device will appear.  Thus it would be good to
make sure sgiioc4 has completed loading before ioc4_probe() returns.

Second, if ioc4_exit() can be invoked at an unfortunate time as
mentioned, that implies the following sequence is possible:

	
	Thread A			Thread B
					ioc4_probe();
					schedule_work();
	ioc4_exit();
	flush_scheduled_work();
					request_module("sgiioc4");
					... sgiioc4 loads ...
					ioc4_register_submodule();
	pci_unregister_driver();

This leaves us with sgiioc4 loaded and containing references to the
now removed IOC4 driver.  I must assume badness ensues. :(

This is normally prevented by the module reference counting mechanism,
but in this case the reference count on ioc4 wouldn't bump until sgiioc4
loads, which is after we're in the ioc4_exit() path.  I suppose some
reference count tricks could work around this (i.e. bump ioc4's refcount
before schedule_work(), and de-bump it when sgiioc4 is finished loading),
but that has a hackish flavor.

Anyway, I think both of these argue for putting the flush_scheduled_work()
immediatley after the schedule_work() call.  I'll test this idea after
a test system is configured overnight and should have a patch available
tomorrow.

Brent

-- 
Brent Casavant                          All music is folk music.  I ain't
bcasavan@sgi.com                        never heard a horse sing a song.
Silicon Graphics, Inc.                    -- Louis Armstrong

Re: [PATCH] ioc4: automatically load sgiioc4 subordinate module

[Andrew Morton]

On Mon, 8 Dec 2008 20:01:31 -0600 (CST) Brent Casavant <bcasavan@sgi.com> wrote:

> > This race is fixable by adding a flush_scheduled_work() into
> > ioc4_exit(), I expect.
> 
> In this case I believe a better spot for the flush_schedule_work() is
> immediately after calling schedule_work()

That is equivalent to calling the work function directly.