On Mon, 2012-06-11 at 16:51 -0600, Bjorn Helgaas wrote: > > This makes me a bit nervous (that doesn't mean it's not right, but > > we need some careful auditing & testing here, which I won't be > > able to do until I'm back from leave). Mostly due to the change in when > > we do the work. > > > > pcibios_fixup_bus() used to be called early on in the initial scan pass. > > > > Your code causes the code to be called -much- later when registering the > > device with the device model. Are we 100% certain nothing will happen in > > between that might rely on the stuff being setup already ? It might well > > be ok, but I want us to triple check that. > > Here's my theory on this: we're setting up DMA and IRQ stuff. DMA and > IRQ usage is device-specific, so the core can't do anything with them. > Only drivers know how to use them. Drivers can't find the device > until it's registered with the device model. So it seems like it > should be safe to move it later. Subject to thinkos and testing in > the real world, of course :) I am aware of that and that's why I say "might well be ok" :-) But this is old code and you know what can happen in there ... might be a quirk here or a piece of platform code there trying to fixup the IRQ for example ... before we set it up. That sort of thing. I should be allright, but I want to test, which I won't be able to do properly before I'm back at work next week. > > Now, if we are ok to do the setup that late (basically right before the > > driver probe() routine gets called), would it make sense to simplify > > things even further ... and do it from pcibios_enable_device() ? Thus > > avoiding the notifier business completely or is that pushing it too > > far ? > > Kenji-san actually suggested using pcibios_enable_device() early on, > and I'm the one who suggested the notifiers instead. I think I > suggested that because I was copying the amd_iommu_init_notifier() > style. > > But I now think that might have been a mistake. Notifiers are > definitely more complicated, and a pcibios_*() hook seems > straightforward. It could be in pcibios_enable_device(), though we > only need it to be called once, and the enable_device() path may be > called many times, e.g., every time a driver claims it. My new vote > is a pcibios_device_add(), with an empty weak definition in > drivers/pci, and a non-empty definition for microblaze and powerpc. Would it be called before or after the notifiers ? I wonder... if others already use the notifiers maybe we should stick to it. I only suggested pcibios_enable_device() because it's already there. > > Also you seem to add: > > > > + /* Setup OF node pointer in the device */ > > + dev->dev.of_node = pci_device_to_OF_node(dev); > > > > This shouldn't be needed anymore, the device node should be setup by the > > core nowadays. Is this just a remnant of you rebasing an old patch or do > > you have a good reason to add this statement ? > > It sounds like you want to remove this line in any case, so I'll wait > for updated patches. Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
