On Wed, May 23, 2012 at 11:51 AM, Jesse Larrew
<jlarrew@xxxxxxxxxxxxxxxxxx> wrote:
> On 05/22/2012 09:33 PM, Hiroo Matsumoto wrote:
>
>> +static int pcibios_device_change_notifier(struct notifier_block *nb,
>> + unsigned long action, void *data)
>> +{
>> + struct pci_dev *dev = to_pci_dev(data);
>> +
>
>
>
> In the general case, we don't know what *data contains until we evaluate
> 'action'. This conversion should probably be moved inside the case:
> statement.
We registered it with "bus_register_notifier(&pci_bus_type, ...)" and
every user of the bus_notifier change passes a "struct device *", so I
think we do know that we have a PCI device.
A future bus_notifier user *could* pass something other than a "struct
device *", but the pattern Hiroo used here ("struct pci_dev *dev =
to_pci_dev(data);" before looking at "action") seems pretty common, so
I don't think it's likely.
>> + switch (action) {
>> + case BUS_NOTIFY_ADD_DEVICE:
>> + /* Setup OF node pointer in the device */
>> + dev->dev.of_node = pci_device_to_OF_node(dev);
>> +
>> + /* Fixup NUMA node as it may not be setup yet by the generic
>> + * code and is needed by the DMA init
>> + */
>> + set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
>> +
>> + /* Hook up default DMA ops */
>> + set_dma_ops(&dev->dev, pci_dma_ops);
>> + set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
>> +
>> + /* Additional platform DMA/iommu setup */
>> + if (ppc_md.pci_dma_dev_setup)
>> + ppc_md.pci_dma_dev_setup(dev);
>> +
>> + /* Read default IRQs and fixup if necessary */
>> + pci_read_irq_line(dev);
>> + if (ppc_md.pci_irq_fixup)
>> + ppc_md.pci_irq_fixup(dev);
>> +
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static struct notifier_block device_nb = {
>> + .notifier_call = pcibios_device_change_notifier,
>> +};
>
>
>
> This is just a nit pick, but I think the naming of
> pcibios_device_change_notifier() is a bit misleading. It doesn't
> actually notify anything, but instead it *handles* notifications.
> Perhaps a better name would be pcibios_device_change_handler() or
> pcibios_device_change_callback()?
Good point. "_notify()" seems to be a common name, how about
pci_bus_notify()? It's static to the file, so it doesn't have to be
very unique.
I happened to be merging these patches just now, so I'll make this
change provisionally, pending any more discussion.
Thanks a lot for reviewing this!
Bjorn
--
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
