Subject: Re: [PATCH v3 1/2] powerpc/PCI: move DMA & IRQ init to device_add() notification path

From: Hiroo Matsumoto <[email-masked]>
Date: Thu, 07 Jun 2012 21:58:09 +0900

Thanks for your rapid modification about function name and comments.
As Ben said, I want to check that my patch does not cause problems.
I have no complain about my patch being in 3.6 branch (or lately).
I appreciate your time and effort.

Regards.

Hiroo MATSUMOTO

> On Thu, May 24, 2012 at 9:00 PM, Benjamin Herrenschmidt
> <[email-masked]> wrote:
>> On Wed, 2012-05-23 at 16:37 -0600, Bjorn Helgaas wrote:
>>> From: Hiroo Matsumoto <[email-masked]>
>>>
>>> PowerPC initialized DMA and IRQ information in the pci_scan_child_bus()
>>> -> pcibios_fixup_bus() path.  Some hotplug drivers use that path, but
>>> others don't, e.g., pciehp, so sometimes hot-added devices are only
>>> partly initialized.
>>>
>>> This patch moves that initialization from pcibios_fixup_bus() to a new
>>> pci_bus_notify() called in the pci_bus_add_device() -> device_add() path.
>>> That means the initialization happens the same way for all PCI devices,
>>> whether they are present at boot or hot-added later.
>>>
>>> Note that powerpc was the only user of pci_fixup_cardbus(), which was
>>> used to do this same setup for cardbus devices.  That's no longer
>>> needed because this setup will now be done in the same device_add()
>>> notification path as all other PCI devices.
>> Hrm. That will require a good deal of testing... Unfortunately I'm out
>> for a few weeks getting some surgery and then recovering...
>>
>> Our PCI code has ancient roots and I wouldn't be surprised if that
>> change breaks subtle assumptions made here or there, we'd need to test
>> at least on a good range of macs and ibm hotplug stuff.
> 
> OK.  Are you worried about cardbus in particular?
> 
> This is headed for the 3.6, not 3.5, so we should have plenty of time.
>  As soon as everything for the current merge window gets merged and
> -next is ready for the next batch, I'll put this in there.
> 
>>> Typical failure of a hot-added e1000e device prior to this change:
>>>
>>>     # echo 1 > /sys/bus/pci/slots/1/power
>>>     <snip>
>>>     e1000e 0000:03:00.0: enabling device (0000 -> 0002)
>>>     e1000e 0000:03:00.0: No usable DMA configuration, aborting
>>>     e1000e: probe of 0000:03:00.0 failed with error -5
>>>
>>> Successful initialization after this change:
>>>
>>>     # echo 1 > /sys/bus/pci/slots/1/power
>>>     <snip>
>>>     e1000e 0000:03:00.0: enabling device (0000 -> 0002)
>>>     irq: irq 6 on host /soc@ffe00000/msi@41600 mapped to virtual irq 27
>>>     e1000e 0000:03:00.0: eth0: (PCI Express:2.5GT/s:Width x1) 00:15:17:bf:c0:c9
>>>     e1000e 0000:03:00.0: eth0: Intel(R) PRO/1000 Network Connection
>>>     e1000e 0000:03:00.0: eth0: MAC: 1, PHY: 4, PBA No: D50861-003
>>>
>>> [bhelgaas: changelog, notifier name, registration can be __init]
>>> CC: Dominik Brodowski <[email-masked]>
>>> Signed-off-by: Hiroo MATSUMOTO <[email-masked]>
>>> Signed-off-by: Bjorn Helgaas <[email-masked]>
>>> ---
>>>  arch/powerpc/include/asm/pci.h             |    2 -
>>>  arch/powerpc/kernel/pci-common.c           |   87 ++++++++++++++--------------
>>>  arch/powerpc/kernel/pci_32.c               |    2 +
>>>  arch/powerpc/kernel/pci_64.c               |    2 +
>>>  arch/powerpc/kernel/pci_of_scan.c          |    1
>>>  arch/powerpc/platforms/pseries/pci_dlpar.c |    1
>>>  drivers/pci/pci.c                          |    5 --
>>>  drivers/pcmcia/cardbus.c                   |    3 -
>>>  include/linux/pci.h                        |    3 -
>>>  9 files changed, 49 insertions(+), 57 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
>>> index 6653f27..d6a36a4 100644
>>> --- a/arch/powerpc/include/asm/pci.h
>>> +++ b/arch/powerpc/include/asm/pci.h
>>> @@ -183,10 +183,10 @@ extern void pci_resource_to_user(const struct pci_dev *dev, int bar,
>>>                                resource_size_t *start, resource_size_t *end);
>>>
>>>  extern resource_size_t pcibios_io_space_offset(struct pci_controller *hose);
>>> -extern void pcibios_setup_bus_devices(struct pci_bus *bus);
>>>  extern void pcibios_setup_bus_self(struct pci_bus *bus);
>>>  extern void pcibios_setup_phb_io_space(struct pci_controller *hose);
>>>  extern void pcibios_scan_phb(struct pci_controller *hose);
>>> +extern void pcibios_setup_bus_notifier(void);
>>>
>>>  #endif       /* __KERNEL__ */
>>>  #endif /* __ASM_POWERPC_PCI_H */
>>> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
>>> index 7320f36..41b39ba 100644
>>> --- a/arch/powerpc/kernel/pci-common.c
>>> +++ b/arch/powerpc/kernel/pci-common.c
>>> @@ -1009,40 +1009,6 @@ void __devinit pcibios_setup_bus_self(struct pci_bus *bus)
>>>               ppc_md.pci_dma_bus_setup(bus);
>>>  }
>>>
>>> -void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
>>> -{
>>> -     struct pci_dev *dev;
>>> -
>>> -     pr_debug("PCI: Fixup bus devices %d (%s)\n",
>>> -              bus->number, bus->self ? pci_name(bus->self) : "PHB");
>>> -
>>> -     list_for_each_entry(dev, &bus->devices, bus_list) {
>>> -             /* Cardbus can call us to add new devices to a bus, so ignore
>>> -              * those who are already fully discovered
>>> -              */
>>> -             if (dev->is_added)
>>> -                     continue;
>>> -
>>> -             /* 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);
>>> -     }
>>> -}
>>> -
>>>  void pcibios_set_master(struct pci_dev *dev)
>>>  {
>>>       /* No special bus mastering setup handling */
>>> @@ -1059,19 +1025,9 @@ void __devinit pcibios_fixup_bus(struct pci_bus *bus)
>>>
>>>       /* Now fixup the bus bus */
>>>       pcibios_setup_bus_self(bus);
>>> -
>>> -     /* Now fixup devices on that bus */
>>> -     pcibios_setup_bus_devices(bus);
>>>  }
>>>  EXPORT_SYMBOL(pcibios_fixup_bus);
>>>
>>> -void __devinit pci_fixup_cardbus(struct pci_bus *bus)
>>> -{
>>> -     /* Now fixup devices on that bus */
>>> -     pcibios_setup_bus_devices(bus);
>>> -}
>>> -
>>> -
>>>  static int skip_isa_ioresource_align(struct pci_dev *dev)
>>>  {
>>>       if (pci_has_flag(PCI_CAN_SKIP_ISA_ALIGN) &&
>>> @@ -1685,6 +1641,49 @@ void __devinit pcibios_scan_phb(struct pci_controller *hose)
>>>       }
>>>  }
>>>
>>> +static int pci_bus_notify(struct notifier_block *nb, unsigned long action,
>>> +                       void *data)
>>> +{
>>> +     struct pci_dev *dev = to_pci_dev(data);
>>> +
>>> +     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 = pci_bus_notify,
>>> +};
>>> +
>>> +void __init pcibios_setup_bus_notifier(void)
>>> +{
>>> +     bus_register_notifier(&pci_bus_type, &device_nb);
>>> +}
>>> +
>>>  static void fixup_hide_host_resource_fsl(struct pci_dev *dev)
>>>  {
>>>       int i, class = dev->class >> 8;
>>> diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
>>> index 4b06ec5..640cc35 100644
>>> --- a/arch/powerpc/kernel/pci_32.c
>>> +++ b/arch/powerpc/kernel/pci_32.c
>>> @@ -231,6 +231,8 @@ static int __init pcibios_init(void)
>>>
>>>       printk(KERN_INFO "PCI: Probing PCI hardware\n");
>>>
>>> +     pcibios_setup_bus_notifier();
>>> +
>>>       if (pci_has_flag(PCI_REASSIGN_ALL_BUS))
>>>               pci_assign_all_buses = 1;
>>>
>>> diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
>>> index 4ff190f..8b212d3 100644
>>> --- a/arch/powerpc/kernel/pci_64.c
>>> +++ b/arch/powerpc/kernel/pci_64.c
>>> @@ -48,6 +48,8 @@ static int __init pcibios_init(void)
>>>
>>>       printk(KERN_INFO "PCI: Probing PCI hardware\n");
>>>
>>> +     pcibios_setup_bus_notifier();
>>> +
>>>       /* For now, override phys_mem_access_prot. If we need it,g
>>>        * later, we may move that initialization to each ppc_md
>>>        */
>>> diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
>>> index ae5ea5e..eb09eca 100644
>>> --- a/arch/powerpc/kernel/pci_of_scan.c
>>> +++ b/arch/powerpc/kernel/pci_of_scan.c
>>> @@ -333,7 +333,6 @@ static void __devinit __of_scan_bus(struct device_node *node,
>>>        */
>>>       if (!rescan_existing)
>>>               pcibios_setup_bus_self(bus);
>>> -     pcibios_setup_bus_devices(bus);
>>>
>>>       /* Now scan child busses */
>>>       list_for_each_entry(dev, &bus->devices, bus_list) {
>>> diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c
>>> index 3ccebc8..0b1b6b3 100644
>>> --- a/arch/powerpc/platforms/pseries/pci_dlpar.c
>>> +++ b/arch/powerpc/platforms/pseries/pci_dlpar.c
>>> @@ -120,7 +120,6 @@ void pcibios_add_pci_devices(struct pci_bus * bus)
>>>               num = pci_scan_slot(bus, PCI_DEVFN(slotno, 0));
>>>               if (!num)
>>>                       return;
>>> -             pcibios_setup_bus_devices(bus);
>>>               max = bus->busn_res.start;
>>>               for (pass=0; pass < 2; pass++)
>>>                       list_for_each_entry(dev, &bus->devices, bus_list) {
>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>> index 15d442a..43e0a4f 100644
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -3789,11 +3789,6 @@ int __attribute__ ((weak)) pci_ext_cfg_avail(struct pci_dev *dev)
>>>       return 1;
>>>  }
>>>
>>> -void __weak pci_fixup_cardbus(struct pci_bus *bus)
>>> -{
>>> -}
>>> -EXPORT_SYMBOL(pci_fixup_cardbus);
>>> -
>>>  static int __init pci_setup(char *str)
>>>  {
>>>       while (str) {
>>> diff --git a/drivers/pcmcia/cardbus.c b/drivers/pcmcia/cardbus.c
>>> index 24caeaf..a980691 100644
>>> --- a/drivers/pcmcia/cardbus.c
>>> +++ b/drivers/pcmcia/cardbus.c
>>> @@ -71,7 +71,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
>>>       unsigned int max, pass;
>>>
>>>       s->functions = pci_scan_slot(bus, PCI_DEVFN(0, 0));
>>> -     pci_fixup_cardbus(bus);
>>>
>>>       max = bus->busn_res.start;
>>>       for (pass = 0; pass < 2; pass++)
>>> @@ -85,7 +84,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
>>>        */
>>>       pci_bus_size_bridges(bus);
>>>       pci_bus_assign_resources(bus);
>>> -     cardbus_config_irq_and_cls(bus, s->pci_irq);
>>>
>>>       /* socket specific tune function */
>>>       if (s->tune_bridge)
>>> @@ -93,6 +91,7 @@ int __ref cb_alloc(struct pcmcia_socket *s)
>>>
>>>       pci_enable_bridges(bus);
>>>       pci_bus_add_devices(bus);
>>> +     cardbus_config_irq_and_cls(bus, s->pci_irq);
>>>
>>>       return 0;
>>>  }
>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>> index a0e2d7f..3924c02 100644
>>> --- a/include/linux/pci.h
>>> +++ b/include/linux/pci.h
>>> @@ -651,9 +651,6 @@ resource_size_t pcibios_align_resource(void *, const struct resource *,
>>>                               resource_size_t);
>>>  void pcibios_update_irq(struct pci_dev *, int irq);
>>>
>>> -/* Weak but can be overriden by arch */
>>> -void pci_fixup_cardbus(struct pci_bus *);
>>> -
>>>  /* Generic PCI functions used internally */
>>>
>>>  void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
>>
> 
>