Re: Autoloading of pseudo-device modules

[Antti Kantee <pooka%cs.hut.fi@localhost>]

On Wed Jan 14 2009 at 14:54:49 +0100, Adam Hamsik wrote:
> Because dm driver is not enabled in kernel yet, and it is installed as  
> new style module.
> I have written this small patch [1] to subr_devsw.c which enable auto- 
> loading of pseudo-device
> drivers. In my patch I'm changing cdev_open/bdev_open routines to try  
> to load module if they
> was not able to look up it.

Good, we need this feature.

> I have also converted some of our well known pseudo-drivers to modules  
> so we can remove them from kernel.
> [2], [3], [4]. I don't have cgd or raidframe* setup but I will test it  
> more.

Both take a few minutes to set up, so I don't think this is a very
convincing excuse for lack of superficial testing.

> There is one outstanding problem with loading device drivers which  
> need argument in kernel
> config file e.g. pseudo-device cgd 4, I do not know how to pass this  
> number to modcmd function
> which calls driver attach routine. With removing these drivers from  
> kernel we can shrink it by
> 200k.

You should fix cgd to not require a static configuration.  Previously
users had to edit the kernel config file if they wanted more.  Now they
have to edit the source.  Making something a module is not a goal for
which we should sacrifice usability.

Some questions based on a quick review (devsw diff only).

Index: kern/subr_devsw.c
===================================================================
- * Device access methods.
+ * If there is no devsw loaded for driver try to autoload it.
  */
+static int
+devsw_do_load(dev_t dev)
+{
+       const char *name;
+       int i;
+
+       name = NULL;
+
+       mutex_enter(&device_lock);
+       for (i = 0 ; i < max_devsw_convs; i++) {
+               if (devsw_conv[i].d_bmajor == major(dev)) {
+                       name = devsw_conv[i].d_name;
+                       break;
+               }
+       }
+       mutex_exit(&device_lock);
+               
+       if (bdevsw[major(dev)] == NULL && name != NULL) {
+               mutex_enter(&module_lock);
+               if (module_autoload(name, MODULE_CLASS_DRIVER)) {
+                       mutex_exit(&module_lock);
+                       return ENXIO;
+               }
+               mutex_exit(&module_lock);
+       }
+       
+       return 0;
+}

If you don't find a name, you still return success?

Where is the major/name loaded into the reverse lookup table in the
first place?  Or can't you load devices which weren't available when
the kernel was config'd?

I don't see a compelling reason to always return ENXIO.  Make the caller
decide if it wants to return ENXIO or the actual error.

@@ -641,9 +673,20 @@ bdev_open(dev_t dev, int flag, int devty
        mutex_enter(&device_lock);
        d = bdevsw_lookup(dev);
        mutex_exit(&device_lock);
-       if (d == NULL)
-               return ENXIO;
+       
+       if (d == NULL) {
+               if ((rv = devsw_do_load(dev)) != 0)
+                       return rv;
 
+               mutex_enter(&device_lock);
+               d = bdevsw_lookup(dev);
+               mutex_exit(&device_lock);
+               /*
+                * This should not happen I loaded module for block device
+                * and therefore I should find cdevsw in next run.
+                */
+               KASSERT(d);

What prevents an unload between the load and the lookup?

And just thinking out loud: would be cool if all the devices attached to
devsw the same way (i.e. dynamically).  Then there would be no separate
hardconfig'd devices and dynamically attached ones, i.e. no "second
class citizens".  But that's not really related to your goal.