[v2] powerpc/pci: Improve device hotplug initialization

Commit 37f02195b (powerpc/pci: fix PCI-e devices rescan issue on powerpc
platform) fixes a problem with interrupt and DMA initialization on hot
plugged devices. With this commit, interrupt and DMA initialization for
hot plugged devices is handled in the pci device enable function.

This approach has a couple of drawbacks. First, it creates two code paths
for device initialization, one for hot plugged devices and another for devices
known during the initial PCI scan. Second, the initialization code for hot
plugged devices is only called when the device is enabled, ie typically
in the probe function. Also, the platform specific setup code is called each
time pci_enable_device() is called, not only once during device discovery,
meaning it is actually called multiple times, once for devices discovered
during the initial scan and again each time a driver is re-loaded.

The visible result is that interrupt pins are only assigned to hot plugged
devices when the device driver is loaded. Effectively this changes the PCI
probe API, since pci_dev->irq and the device's dma configuration will now
only be valid after pci_enable() was called at least once. A more subtle
change is that platform specific PCI device setup is moved from device
discovery into the driver's probe function, more specifically into the
pci_enable_device() call.

To fix the inconsistencies, add new function pcibios_add_device.
Call pcibios_setup_device from pcibios_setup_bus_devices if device setup
is not complete, and from pcibios_add_device if bus setup is complete.

With this change, device setup code is moved back into device initialization,
and called exactly once for both static and hot plugged devices.

Cc: Yuanquan Chen <Yuanquan.Chen@freescale.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Hiroo Matsumoto <matsumoto.hiroo@jp.fujitsu.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Ensure that PCI bus fixup code has been executed before calling
    device setup code.

 arch/powerpc/kernel/pci-common.c |   17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

On Mon, Jun 10, 2013 at 10:18:08AM -0700, Guenter Roeck wrote:
> Commit 37f02195b (powerpc/pci: fix PCI-e devices rescan issue on powerpc
> platform) fixes a problem with interrupt and DMA initialization on hot
> plugged devices. With this commit, interrupt and DMA initialization for
> hot plugged devices is handled in the pci device enable function.
> 
> This approach has a couple of drawbacks. First, it creates two code paths
> for device initialization, one for hot plugged devices and another for devices
> known during the initial PCI scan. Second, the initialization code for hot
> plugged devices is only called when the device is enabled, ie typically
> in the probe function. Also, the platform specific setup code is called each
> time pci_enable_device() is called, not only once during device discovery,
> meaning it is actually called multiple times, once for devices discovered
> during the initial scan and again each time a driver is re-loaded.
> 
> The visible result is that interrupt pins are only assigned to hot plugged
> devices when the device driver is loaded. Effectively this changes the PCI
> probe API, since pci_dev->irq and the device's dma configuration will now
> only be valid after pci_enable() was called at least once. A more subtle
> change is that platform specific PCI device setup is moved from device
> discovery into the driver's probe function, more specifically into the
> pci_enable_device() call.
> 
> To fix the inconsistencies, add new function pcibios_add_device.
> Call pcibios_setup_device from pcibios_setup_bus_devices if device setup
> is not complete, and from pcibios_add_device if bus setup is complete.
> 
> With this change, device setup code is moved back into device initialization,
> and called exactly once for both static and hot plugged devices.
> 
> Cc: Yuanquan Chen <Yuanquan.Chen@freescale.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Hiroo Matsumoto <matsumoto.hiroo@jp.fujitsu.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Ensure that PCI bus fixup code has been executed before calling
>     device setup code.
> 
Hi Ben,

any comments/feedback on this approach ?

It is much less invasive than before and should address your concerns.

Thanks,
Guenter

>  arch/powerpc/kernel/pci-common.c |   17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index 7f2273c..6909b13 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -992,7 +992,7 @@ void pcibios_setup_bus_self(struct pci_bus *bus)
>  		ppc_md.pci_dma_bus_setup(bus);
>  }
>  
> -void pcibios_setup_device(struct pci_dev *dev)
> +static void pcibios_setup_device(struct pci_dev *dev)
>  {
>  	/* Fixup NUMA node as it may not be setup yet by the generic
>  	 * code and is needed by the DMA init
> @@ -1013,6 +1013,17 @@ void pcibios_setup_device(struct pci_dev *dev)
>  		ppc_md.pci_irq_fixup(dev);
>  }
>  
> +int pcibios_add_device(struct pci_dev *dev)
> +{
> +	/*
> +	 * We can only call pcibios_setup_device() after bus setup is complete,
> +	 * since some of the platform specific DMA setup code depends on it.
> +	 */
> +	if (dev->bus->is_added)
> +		pcibios_setup_device(dev);
> +	return 0;
> +}
> +
>  void pcibios_setup_bus_devices(struct pci_bus *bus)
>  {
>  	struct pci_dev *dev;
> @@ -1467,10 +1478,6 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
>  		if (ppc_md.pcibios_enable_device_hook(dev))
>  			return -EINVAL;
>  
> -	/* avoid pcie irq fix up impact on cardbus */
> -	if (dev->hdr_type != PCI_HEADER_TYPE_CARDBUS)
> -		pcibios_setup_device(dev);
> -
>  	return pci_enable_resources(dev, mask);
>  }
>  
> -- 
> 1.7.9.7
> 
>

On Fri, 2013-06-21 at 09:54 -0700, Guenter Roeck wrote:

> > v2: Ensure that PCI bus fixup code has been executed before calling
> >     device setup code.
> > 
> Hi Ben,
> 
> any comments/feedback on this approach ?
> 
> It is much less invasive than before and should address your concerns.

And also less nice cleanup :-) I'll have a look.

Cheers,
Ben.

> Thanks,
> Guenter
> 
> >  arch/powerpc/kernel/pci-common.c |   17 ++++++++++++-----
> >  1 file changed, 12 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> > index 7f2273c..6909b13 100644
> > --- a/arch/powerpc/kernel/pci-common.c
> > +++ b/arch/powerpc/kernel/pci-common.c
> > @@ -992,7 +992,7 @@ void pcibios_setup_bus_self(struct pci_bus *bus)
> >  		ppc_md.pci_dma_bus_setup(bus);
> >  }
> >  
> > -void pcibios_setup_device(struct pci_dev *dev)
> > +static void pcibios_setup_device(struct pci_dev *dev)
> >  {
> >  	/* Fixup NUMA node as it may not be setup yet by the generic
> >  	 * code and is needed by the DMA init
> > @@ -1013,6 +1013,17 @@ void pcibios_setup_device(struct pci_dev *dev)
> >  		ppc_md.pci_irq_fixup(dev);
> >  }
> >  
> > +int pcibios_add_device(struct pci_dev *dev)
> > +{
> > +	/*
> > +	 * We can only call pcibios_setup_device() after bus setup is complete,
> > +	 * since some of the platform specific DMA setup code depends on it.
> > +	 */
> > +	if (dev->bus->is_added)
> > +		pcibios_setup_device(dev);
> > +	return 0;
> > +}
> > +
> >  void pcibios_setup_bus_devices(struct pci_bus *bus)
> >  {
> >  	struct pci_dev *dev;
> > @@ -1467,10 +1478,6 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
> >  		if (ppc_md.pcibios_enable_device_hook(dev))
> >  			return -EINVAL;
> >  
> > -	/* avoid pcie irq fix up impact on cardbus */
> > -	if (dev->hdr_type != PCI_HEADER_TYPE_CARDBUS)
> > -		pcibios_setup_device(dev);
> > -
> >  	return pci_enable_resources(dev, mask);
> >  }
> >  
> > -- 
> > 1.7.9.7
> > 
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

On Mon, Jun 24, 2013 at 11:49:49AM +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2013-06-21 at 09:54 -0700, Guenter Roeck wrote:
> 
> > > v2: Ensure that PCI bus fixup code has been executed before calling
> > >     device setup code.
> > > 
> > Hi Ben,
> > 
> > any comments/feedback on this approach ?
> > 
> > It is much less invasive than before and should address your concerns.
> 
> And also less nice cleanup :-) I'll have a look.
> 
I know :(.

Reminds me of that old story of being stuck between a rock and a hard place ...

Guenter

> Cheers,
> Ben.
> 
> > Thanks,
> > Guenter
> > 
> > >  arch/powerpc/kernel/pci-common.c |   17 ++++++++++++-----
> > >  1 file changed, 12 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> > > index 7f2273c..6909b13 100644
> > > --- a/arch/powerpc/kernel/pci-common.c
> > > +++ b/arch/powerpc/kernel/pci-common.c
> > > @@ -992,7 +992,7 @@ void pcibios_setup_bus_self(struct pci_bus *bus)
> > >  		ppc_md.pci_dma_bus_setup(bus);
> > >  }
> > >  
> > > -void pcibios_setup_device(struct pci_dev *dev)
> > > +static void pcibios_setup_device(struct pci_dev *dev)
> > >  {
> > >  	/* Fixup NUMA node as it may not be setup yet by the generic
> > >  	 * code and is needed by the DMA init
> > > @@ -1013,6 +1013,17 @@ void pcibios_setup_device(struct pci_dev *dev)
> > >  		ppc_md.pci_irq_fixup(dev);
> > >  }
> > >  
> > > +int pcibios_add_device(struct pci_dev *dev)
> > > +{
> > > +	/*
> > > +	 * We can only call pcibios_setup_device() after bus setup is complete,
> > > +	 * since some of the platform specific DMA setup code depends on it.
> > > +	 */
> > > +	if (dev->bus->is_added)
> > > +		pcibios_setup_device(dev);
> > > +	return 0;
> > > +}
> > > +
> > >  void pcibios_setup_bus_devices(struct pci_bus *bus)
> > >  {
> > >  	struct pci_dev *dev;
> > > @@ -1467,10 +1478,6 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
> > >  		if (ppc_md.pcibios_enable_device_hook(dev))
> > >  			return -EINVAL;
> > >  
> > > -	/* avoid pcie irq fix up impact on cardbus */
> > > -	if (dev->hdr_type != PCI_HEADER_TYPE_CARDBUS)
> > > -		pcibios_setup_device(dev);
> > > -
> > >  	return pci_enable_resources(dev, mask);
> > >  }
> > >  
> > > -- 
> > > 1.7.9.7
> > > 
> > > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> 
>