Re: [Xen-devel] [PATCH] linux: allow pciback to be built as a module

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

>>> Ryan <hap9@xxxxxxxxxxxxxx> 08.03.06 17:05:42 >>>
>The reason that I did not code this as a module in the first place was
>because the pcistub driver needs to be loaded before other device
>drivers so that it gets first pick at seizing devices from the kernel.
>That's why I use fs_initcall in the pcistub driver instead of
>device_initcall or module_init, it gets called earlier in the kernel
>boot process. As a module, you can't do that. If other PCI device
>drivers (whether modules or not) are loaded before the pciback module,
>they'll have a higher priority (because they're earlier in the linked
>list of drivers) for grabbing devices. This functionality (using
>fs_initcall) is somewhat of a "hack" (because I don't know of a cleaner
>way to ensure that the pcistub driver gets probed for ALL pci devices).

I understood that.

>Recent kernels have the "bind" and "unbind" attributes on drivers in
>sysfs that would help you avoid this problem, but that seems like a lot
>to ask of the user. They'd have to first unbind their device from the
>driver that grabbed it. Then they'd have to load the pciback module (or,
>if it was already loaded, they could use the "bind" attribute"). Either
>way, it turns "hiding" a device from a one-step process (just specify
>all devices on the kernel command-line) to a two-step (unbind each
>device from driver, bind to pcistub driver).

That's why I wanted it to allow being a module; if whoever configures the kernel
wants to avoid this two-step process, he can simply say 'Y' to the config 
question.

>> -               pciback_disable_device(dev);
>> +               pci_disable_device(dev);
>
>If you change the pciback_disable_device to pci_disable_device, you need
>to add a dev->is_enabled = 0 here otherwise the is_enabled flag gets out
>of sync with reality.

I don't think so, that's one of the significant differences between calling
pcibios_disable_device() and pci_disable_device() - the latter takes care of
that adjustment.

>> -               pcibios_set_master(dev);
>> +               pci_set_master(dev);
>
>There's probably not any problem here, but most of pci_set_master is
>already done in the frontend (when the frontend device driver calls
>pci_set_master), I was trying to avoid duplicating the effects of
>pci_set_master here by using pcibios_set_master instead.

The main reason for these changes is that only pci_* are exported, not 
pcibios_*.

>> +       __unsafe(THIS_MODULE);
>> +
>> +       return 0;
>> +}
>> +
>> +static void pciback_cleanup(void)
>> +{
>> +       BUG();
>> +}
>> +
>> +module_init(pciback_init);
>> +module_exit(pciback_cleanup);
>
>I believe that if you don't specify a module_exit routine at all, you
>can't unload the module (see kernel/module.c::sys_delete_module). This
>may be better than letting the user accidentally try unloading the
>module and get a nasty BUG() message (and then you don't need the
>"__unsafe" macro with the scary log message). I don't know the semantics
>of what would happen here: if the module unloading proceeds despite the
>BUG(), you'd be left with some dangling pointers (because there would be
>many places within the PCI code referencing memory within this driver
>since no clean-up occurs).

Correct. If indeed the module is meant to never be unloaded (which I would
hope it isn't), then this should be done the way you say; I did it the other way
because I wanted to retain a reminder that this needs work.

>> -void pciback_disable_device(struct pci_dev *dev)
>> -{
>> -       if (dev->is_enabled) {
>> -               dev->is_enabled = 0;
>> -               pcibios_disable_device(dev);
>> -       }
>> -}
>> -
>
>This should be fine. pciback_disable_device used to do something a bit
>different in a previous iteration of code, but wherever
>pciback_disable_device was called before, you must put a
>"dev->is_enabled = 0" in its place.
>
>The other reason I had for using pcibios_disable_device instead of
>pci_disable_device (which is the method that replaced the calls to
>pciback_disable_device in this patch) is because pci_disable_device is
>already run in the frontend and we're just intercepting the end of it. I
>don't think it's an issue, but there may be some code duplication (like
>reading the PCI_COMMAND register twice to see if the bus master bit is
>set).

Same as above.

>> -static __init int pciback_xenbus_register(void)
>> +#ifndef MODULE
>> +static
>> +#endif
>> +__init int pciback_xenbus_register(void)
>>  {
>>         return xenbus_register_backend(&xenbus_pciback_driver);
>>  }
>>  
>> +#ifndef MODULE
>>  /* Must only initialize our xenbus driver after the pcistub driver */
>>  device_initcall(pciback_xenbus_register);
>> +#endif
>> 
>
>As a coding style preference of mine, if you want to make this code
>module-friendly, I would just make this function non-static and always
>call it from pci_stub.c rather than have the #ifdef MODULE stuff here.
>All those preprocessor statements are intimidating... :)

Yes, probably that'd be cleaner...

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel