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