This patchset is for PCI hotplug.
pcibios_setup_bus_devices which sets DMA and IRQs of PCI device is called
only when boot. DMA setting in probe for PCI driver, like dma_set_mask,
does not work on powerpc platform. So it is need to set DMA and IRQs of
PCI device when hotplug.
1. Moving pcibios_setup_bus_devices code to pcibios_device_change_notifier
which is registered to bus notifier in pcibios_init.
2. Removing caller and callee of pcibios_setup_bus_devices bus notifier
works instead of pcibios_setup_bus_devices.
3. Using this bus notifier for microblaze because microblaze/PCI is similer
with powerpc/PCI.
[PATCH v2 1/2] powerpc/PCI: Add pcibios_device_change_notifier for powerpc
[PATCH v2 2/2] microblaze/PCI: Add pcibios_device_change_notifier for microblaze
Regards.
Hiroo MATSUMOTO
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2012-06-06 05:27:11
On Wed, 2012-05-23 at 11:33 +0900, Hiroo Matsumoto wrote:
This patchset is for PCI hotplug.
pcibios_setup_bus_devices which sets DMA and IRQs of PCI device is called
only when boot. DMA setting in probe for PCI driver, like dma_set_mask,
does not work on powerpc platform. So it is need to set DMA and IRQs of
PCI device when hotplug.
1. Moving pcibios_setup_bus_devices code to pcibios_device_change_notifier
which is registered to bus notifier in pcibios_init.
2. Removing caller and callee of pcibios_setup_bus_devices bus notifier
works instead of pcibios_setup_bus_devices.
3. Using this bus notifier for microblaze because microblaze/PCI is similer
with powerpc/PCI.
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.
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 ?
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 ?
Cheers,
Ben.
On Wed, 2012-05-23 at 11:33 +0900, Hiroo Matsumoto wrote:
quoted
This patchset is for PCI hotplug.
pcibios_setup_bus_devices which sets DMA and IRQs of PCI device is called
only when boot. DMA setting in probe for PCI driver, like dma_set_mask,
does not work on powerpc platform. So it is need to set DMA and IRQs of
PCI device when hotplug.
1. Moving pcibios_setup_bus_devices code to pcibios_device_change_notifier
which is registered to bus notifier in pcibios_init.
2. Removing caller and callee of pcibios_setup_bus_devices bus notifier
works instead of pcibios_setup_bus_devices.
3. Using this bus notifier for microblaze because microblaze/PCI is similer
with powerpc/PCI.
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.
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 ?
As you said, there are times between pcibios_fixup_bus and device_add.
I'm agree with you.
But it is difficult for me to verify these by myself.
Can I ask for your help?
I will wait that you are back from leave.
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 ?
No, I don't have a good reason to add this statement.
I just tried to make this code be same with pcibios_setup_bus_devices.
Thanks for your review and comment.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2012-06-07 21:32:06
On Thu, 2012-06-07 at 21:45 +0900, Hiroo Matsumoto wrote:
quoted
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 ?
No, I don't have a good reason to add this statement.
I just tried to make this code be same with pcibios_setup_bus_devices.
Thanks for your review and comment.
Unless I'm mistaken, this statement was removed from
pcibios_setup_bus_devices() recently so I'd say it's a leftover from
your patch having been originally done on top of an older kernel.
I won't be back for another week or so but I'll see if I can get things
tested in the meantime.
Cheers,
Ben.
On Tue, Jun 5, 2012 at 11:26 PM, Benjamin Herrenschmidt
[off-list ref] wrote:
On Wed, 2012-05-23 at 11:33 +0900, Hiroo Matsumoto wrote:
quoted
This patchset is for PCI hotplug.
pcibios_setup_bus_devices which sets DMA and IRQs of PCI device is calle=
d
quoted
only when boot. DMA setting in probe for PCI driver, like dma_set_mask,
does not work on powerpc platform. So it is need to set DMA and IRQs of
PCI device when hotplug.
1. Moving pcibios_setup_bus_devices code to pcibios_device_change_notifi=
er
quoted
=A0 =A0which is registered to bus notifier in pcibios_init.
2. Removing caller and callee of pcibios_setup_bus_devices bus notifier
=A0 =A0works instead of pcibios_setup_bus_devices.
3. Using this bus notifier for microblaze because microblaze/PCI is simi=
ler
quoted
=A0 =A0with powerpc/PCI.
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 :)
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.
Also you seem to add:
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Setup OF node pointer in the device */
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev->dev.of_node =3D 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.
Bjorn
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2012-06-11 23:24:55
On Mon, 2012-06-11 at 16:51 -0600, Bjorn Helgaas wrote:
quoted
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.
quoted
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.
quoted
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.