[PATCH v2 0/2] Add pcibios_device_change_notifier
benh at kernel.crashing.org
Tue Jun 12 09:24:41 EST 2012
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()
> 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.
More information about the Linuxppc-dev