Petr Vandrovec wrote:
>
> On 23 May 00 at 11:51, Jeff Garzik wrote:
> > > pci_register_driver returns 0.
> > > Fixed
> > [...]
> > I do not think this is a bug. In via82cxxx_audio at least, I
> > -purposefully- do not call pci_module_init, because it is not a hotplug
> > driver. I don't want the PCI driver instance hanging around if there is
> > no hardware for it -- even when CONFIG_HOTPLUG is true.
> Why you think that via82cxxx is not hotplug driver?
Because I wrote it, and the guys who make and sell Via chips told me the
Via 686A audio device wasn't found on any hotplug cards of any sort.
:) If anyone has field experience to the contrary, I will immediately
change the driver to pci_module_init and __dev{xxx}.
> > Your patch may also have unintended consequences, since you do not
> > appear to search & replace "__init" with "__devinit" where necessary.
> It is not needed because of pci_unfroze() is invoked before init memory
> is released. Only difference is that with no device of such type in
> computer driver is not unregistered. But because of non-hotplug kernel
> ever calls pci_insert_device, it should not cause any problem.
The unintended consequence are not due to your core change. They are
due to changing the module_init functions to call pci_module_init, (a)
when the driver is not hotplug, and (b) when the driver has not been
reviewed for __devinit correctness.
Your core change looks ok to me, at first glance. (i.e. evaluating the
concept you propose more closely than the code itself)
> > Thus when a driver which DOES hang around (pci_register_driver returns 0
> > under CONFIG_HOTPLUG), and in the unlikely event such a device is
> > plugged in, the kernel crashes because the __devinit code it is
> > searching for has already been dropped (because it is __init instead).
> I believe that I did not introduce new bugs - you can always think that
> in old scheme you had one device driven by driver plugged in when
> kernel booted and you plug another one after that... If you use
> pci_register_driver and you do not have correct __init/__devinit,
> your driver is already buggy. My patch did not change it (unfortunately?).
Overall I am wary of introducing hotplug-enabled code into drivers which
will never support hotplug hardware, because it tends to bloat the
code. That is why __init is preferred over __devinit where possibly --
more memory is made available after kernel boot time in hotplug kernels.
> I'll try to review all PCI drivers, but I cannot promise anything.
Viva la open source! The more eyes the better.
> > Unfortunately since your change only supports PCI drivers, it does not
> > allow us to completely ignore/eliminate a user-specified ethX ordering
> > [for example]. I also think that the call for this patch is less strong
> > as we move to a more modular world.
> Unfortunately, my current computers have one or zero ISA slots and (as
> they are PCs) no sbus or MVE or..., so I have no idea which ordering is
> reasonable for which of these other busses.
Order of busses is an issue, as is the possibility of it being
impossible to determine which device corresponds to which bus slot...
Alas, if only that original IBM PC had been built with PCI not ISA :)
> Even in modular world, it is nice if I can swap my two bttv interfaces
> without opening cover and physically swapping grabbers... Or you can
> swap on-board ide with promise/HPT ide without having ide_probe=reverse.
Agreed. It makes sense to have a single interface which allows custom
ordering of all PCI devices, instead of having one tucked into every
single driver subsystem. The lack of support for ordering other buses
is a bit daunting though. Still, your patch IS likely to be very
useful.
Jeff
-- Jeff Garzik | Liberty is always dangerous, but Building 1024 | it is the safest thing we have. MandrakeSoft, Inc. | -- Harry Emerson Fosdick- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Tue May 23 2000 - 21:00:24 EST