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

[Ryan <hap9@xxxxxxxxxxxxxx>]

On Wed, 2006-03-08 at 17:34 +0100, Jan Beulich wrote:
> >>> 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.
> 

That's fine. I just wanted to make sure that the trade-offs between the
approaches were understood. I might recommend, though, that you add a
blurb to the help section in Kconfig so that people doing a
custom-compile will understand the trade-offs between built-in and
module that we've discussed here.

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

Yes, you are correct. I missed that. :)

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

I think the only place where there could potentially be an issue is
changing pcibios_enable_device to pci_enable_device. pci_enable_device
calls pci_fixup_device which *may* have some side effects that a
non-linux driver domain may not expect or want. Maybe you could call
pci_enable_device_bars (which is exported) just like it's called in
pci_enable_device to skip the pci_fixup_device method.

Ryan


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