[PATCH v2 1/2] ARM: PCI: bios32: replace panic with WARN messages on failures

[PATCH v2 1/2] ARM: PCI: bios32: replace panic with WARN messages on failures

[Lorenzo Pieralisi]

In the ARM PCI bios32 layer, failures to dynamically allocate pci_sys_data
for a PCI bus, or a PCI bus scan failure have to be considered serious
warnings but they should not trigger a system panic so that at least the
system is given a chance to be debugged.

This patch replaces the panic statements with WARN() messages to
improve error reporting in the ARM PCI bios32 layer.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kernel/bios32.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index fcbbbb1..a5c782c 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -459,8 +459,8 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
 
 	for (nr = busnr = 0; nr < hw->nr_controllers; nr++) {
 		sys = kzalloc(sizeof(struct pci_sys_data), GFP_KERNEL);
-		if (!sys)
-			panic("PCI: unable to allocate sys data!");
+		if (WARN(!sys, "PCI: unable to allocate sys data!"))
+			break;
 
 #ifdef CONFIG_PCI_MSI
 		sys->msi_ctrl = hw->msi_ctrl;
@@ -489,8 +489,10 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
 				sys->bus = pci_scan_root_bus(parent, sys->busnr,
 						hw->ops, sys, &sys->resources);
 
-			if (!sys->bus)
-				panic("PCI: unable to scan bus!");
+			if (WARN(!sys->bus, "PCI: unable to scan bus!")) {
+				kfree(sys);
+				break;
+			}
 
 			busnr = sys->bus->busn_res.end + 1;
 
-- 
2.2.1

[PATCH v2 2/2] ARM: pci: kill pcibios_msi_controller

[Lorenzo Pieralisi]

On ARM PCI systems relying on the pcibios API to initialize PCI host
controllers, the pcibios_msi_controller weak callback is used to look-up
the msi_controller pointer, through pci_sys_data msi_ctrl pointer.

pci_sys_data is an ARM specific structure, which prevents using the
same mechanism (so same PCI host controller drivers) on ARM64 systems.

Since the struct pci_bus already contains an msi_controller pointer and
the kernel already uses it to look-up the msi controller,
this patch converts ARM host controller and related pcibios/host bridges
initialization routines so that the msi_controller pointer look-up can be
carried out by PCI core code through the struct pci_bus msi pointer,
removing the need for the arch specific pcibios_msi_controller callback
and the related pci_sys_data msi_ctrl pointer.

ARM is the only arch relying on the pcibios_msi_controller() weak
function, hence this patch removes the default weak implementation
from PCI core code since it becomes of no use.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Cc: Pratyush Anand <pratyush.anand@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Jingoo Han <jingoohan1@gmail.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Simon Horman <horms@verge.net.au>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
---
v1->v2

- Added patch to replace panic statements with WARN
- Removed unused pcibios_msi_controller() and pci_msi_controller() from
  core code
- Dropped RFT status

v1: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/356028.html

 arch/arm/include/asm/mach/pci.h    |  3 ---
 arch/arm/kernel/bios32.c           | 29 +++++++++++++----------------
 drivers/pci/host/pcie-designware.c |  9 +++++++--
 drivers/pci/host/pcie-xilinx.c     | 12 ++++++++++--
 drivers/pci/msi.c                  | 17 +----------------
 5 files changed, 31 insertions(+), 39 deletions(-)

diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
index 28b9bb3..32abc0c 100644
--- a/arch/arm/include/asm/mach/pci.h
+++ b/arch/arm/include/asm/mach/pci.h
@@ -42,9 +42,6 @@ struct hw_pci {
  * Per-controller structure
  */
 struct pci_sys_data {
-#ifdef CONFIG_PCI_MSI
-	struct msi_controller *msi_ctrl;
-#endif
 	struct list_head node;
 	int		busnr;		/* primary bus number			*/
 	u64		mem_offset;	/* bus->cpu memory mapping offset	*/
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index a5c782c..3a258e5 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -18,15 +18,6 @@
 
 static int debug_pci;
 
-#ifdef CONFIG_PCI_MSI
-struct msi_controller *pcibios_msi_controller(struct pci_dev *dev)
-{
-	struct pci_sys_data *sysdata = dev->bus->sysdata;
-
-	return sysdata->msi_ctrl;
-}
-#endif
-
 /*
  * We can't use pci_get_device() here since we are
  * called from interrupt context.
@@ -462,9 +453,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
 		if (WARN(!sys, "PCI: unable to allocate sys data!"))
 			break;
 
-#ifdef CONFIG_PCI_MSI
-		sys->msi_ctrl = hw->msi_ctrl;
-#endif
 		sys->busnr   = busnr;
 		sys->swizzle = hw->swizzle;
 		sys->map_irq = hw->map_irq;
@@ -483,11 +471,20 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
 				break;
 			}
 
-			if (hw->scan)
+			if (hw->scan) {
 				sys->bus = hw->scan(nr, sys);
-			else
-				sys->bus = pci_scan_root_bus(parent, sys->busnr,
-						hw->ops, sys, &sys->resources);
+			} else {
+				sys->bus = pci_create_root_bus(parent,
+							       sys->busnr,
+							       hw->ops, sys,
+							       &sys->resources);
+				if (sys->bus) {
+#ifdef CONFIG_PCI_MSI
+					sys->bus->msi = hw->msi_ctrl;
+#endif
+					pci_scan_child_bus(sys->bus);
+				}
+			}
 
 			if (WARN(!sys->bus, "PCI: unable to scan bus!")) {
 				kfree(sys);
diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 69486be..e584dfa 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -526,7 +526,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
 
 #ifdef CONFIG_PCI_MSI
 	dw_pcie_msi_chip.dev = pp->dev;
-	dw_pci.msi_ctrl = &dw_pcie_msi_chip;
 #endif
 
 	dw_pci.nr_controllers = 1;
@@ -708,11 +707,17 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
 	struct pcie_port *pp = sys_to_pcie(sys);
 
 	pp->root_bus_nr = sys->busnr;
-	bus = pci_scan_root_bus(pp->dev, sys->busnr,
+	bus = pci_create_root_bus(pp->dev, sys->busnr,
 				  &dw_pcie_ops, sys, &sys->resources);
 	if (!bus)
 		return NULL;
 
+#ifdef CONFIG_PCI_MSI
+	bus->msi = &dw_pcie_msi_chip;
+#endif
+
+	pci_scan_child_bus(bus);
+
 	if (bus && pp->ops->scan_bus)
 		pp->ops->scan_bus(pp);
 
diff --git a/drivers/pci/host/pcie-xilinx.c b/drivers/pci/host/pcie-xilinx.c
index f1a06a0..b21eb7d 100644
--- a/drivers/pci/host/pcie-xilinx.c
+++ b/drivers/pci/host/pcie-xilinx.c
@@ -647,9 +647,18 @@ static struct pci_bus *xilinx_pcie_scan_bus(int nr, struct pci_sys_data *sys)
 	struct pci_bus *bus;
 
 	port->root_busno = sys->busnr;
-	bus = pci_scan_root_bus(port->dev, sys->busnr, &xilinx_pcie_ops,
+	bus = pci_create_root_bus(port->dev, sys->busnr, &xilinx_pcie_ops,
 				sys, &sys->resources);
 
+	if (!bus)
+		return NULL;
+
+#ifdef CONFIG_PCI_MSI
+	bus->msi = &xilinx_pcie_msi_chip;
+#endif
+
+	pci_scan_child_bus(bus);
+
 	return bus;
 }
 
@@ -847,7 +856,6 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
 
 #ifdef CONFIG_PCI_MSI
 	xilinx_pcie_msi_chip.dev = port->dev;
-	hw.msi_ctrl = &xilinx_pcie_msi_chip;
 #endif
 	pci_common_init_dev(dev, &hw);
 
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index f66be86..0d20142 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -77,24 +77,9 @@ static void pci_msi_teardown_msi_irqs(struct pci_dev *dev)
 
 /* Arch hooks */
 
-struct msi_controller * __weak pcibios_msi_controller(struct pci_dev *dev)
-{
-	return NULL;
-}
-
-static struct msi_controller *pci_msi_controller(struct pci_dev *dev)
-{
-	struct msi_controller *msi_ctrl = dev->bus->msi;
-
-	if (msi_ctrl)
-		return msi_ctrl;
-
-	return pcibios_msi_controller(dev);
-}
-
 int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
 {
-	struct msi_controller *chip = pci_msi_controller(dev);
+	struct msi_controller *chip = dev->bus->msi;
 	int err;
 
 	if (!chip || !chip->setup_irq)
-- 
2.2.1

Re: [PATCH v2 2/2] ARM: pci: kill pcibios_msi_controller

[Lorenzo Pieralisi]

On Sun, Jul 26, 2015 at 04:25:29PM +0100, Jayachandran C. wrote:
> On Fri, Jul 24, 2015 at 05:13:03PM +0100, Lorenzo Pieralisi wrote:
> > On ARM PCI systems relying on the pcibios API to initialize PCI host
> > controllers, the pcibios_msi_controller weak callback is used to look-up
> > the msi_controller pointer, through pci_sys_data msi_ctrl pointer.
> > 
> > pci_sys_data is an ARM specific structure, which prevents using the
> > same mechanism (so same PCI host controller drivers) on ARM64 systems.
> > 
> > Since the struct pci_bus already contains an msi_controller pointer and
> > the kernel already uses it to look-up the msi controller,
> > this patch converts ARM host controller and related pcibios/host bridges
> > initialization routines so that the msi_controller pointer look-up can be
> > carried out by PCI core code through the struct pci_bus msi pointer,
> > removing the need for the arch specific pcibios_msi_controller callback
> > and the related pci_sys_data msi_ctrl pointer.
> > 
> > ARM is the only arch relying on the pcibios_msi_controller() weak
> > function, hence this patch removes the default weak implementation
> > from PCI core code since it becomes of no use.
> > 
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> > Cc: Pratyush Anand <pratyush.anand@gmail.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Jingoo Han <jingoohan1@gmail.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Simon Horman <horms@verge.net.au>
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > Cc: Thierry Reding <thierry.reding@gmail.com>
> > Cc: Michal Simek <michal.simek@xilinx.com>
> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> > v1->v2
> > 
> > - Added patch to replace panic statements with WARN
> > - Removed unused pcibios_msi_controller() and pci_msi_controller() from
> >   core code
> > - Dropped RFT status
> > 
> > v1: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/356028.html
> > 
> >  arch/arm/include/asm/mach/pci.h    |  3 ---
> >  arch/arm/kernel/bios32.c           | 29 +++++++++++++----------------
> >  drivers/pci/host/pcie-designware.c |  9 +++++++--
> >  drivers/pci/host/pcie-xilinx.c     | 12 ++++++++++--
> >  drivers/pci/msi.c                  | 17 +----------------
> >  5 files changed, 31 insertions(+), 39 deletions(-)
> > 
> [...]
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index f66be86..0d20142 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -77,24 +77,9 @@ static void pci_msi_teardown_msi_irqs(struct pci_dev *dev)
> >  
> >  /* Arch hooks */
> >  
> > -struct msi_controller * __weak pcibios_msi_controller(struct pci_dev *dev)
> > -{
> > -	return NULL;
> > -}
> > -
> > -static struct msi_controller *pci_msi_controller(struct pci_dev *dev)
> > -{
> > -	struct msi_controller *msi_ctrl = dev->bus->msi;
> > -
> > -	if (msi_ctrl)
> > -		return msi_ctrl;
> > -
> > -	return pcibios_msi_controller(dev);
> > -}
> > -
> >  int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
> >  {
> > -	struct msi_controller *chip = pci_msi_controller(dev);
> > +	struct msi_controller *chip = dev->bus->msi;
> >  	int err;
> >  
> >  	if (!chip || !chip->setup_irq)
> 
> Don't you have to go to the top level bus and get the ->msi pointer? Something
> like:
> 
> 	for (bus = dev->bus; bus != NULL; bus = bus->parent)
> 		if (bus->msi)
> 			return bus->msi;
> 
> I have not been following this closely, so I may have missed some patches.

The msi pointer is initialized from parent to child in
pci_alloc_child_bus(), so PCI core does that for us,
that's my understanding.

It works the same as sysdata pointer, which is why
pcibios_msi_controller works in the current kernel.

On a side note, are you able to prepare a new version
of your set to enable the PCI generic host on ARM64
(and remove pci_sys_data and related ifdef CONFIG_ARM
from it) ?

We are not far from removing the pci_sys_data dependency,
if you can't prepare a new version of your series let me
know I can do it on your behalf.

Thanks !
Lorenzo

Re: [PATCH v2 2/2] ARM: pci: kill pcibios_msi_controller

[Lorenzo Pieralisi]

On Sun, Jul 26, 2015 at 07:43:46PM +0100, Jayachandran C. wrote:
> On Sun, Jul 26, 2015 at 07:16:46PM +0100, Lorenzo Pieralisi wrote:
> > On Sun, Jul 26, 2015 at 04:25:29PM +0100, Jayachandran C. wrote:
> > > On Fri, Jul 24, 2015 at 05:13:03PM +0100, Lorenzo Pieralisi wrote:
> > > > On ARM PCI systems relying on the pcibios API to initialize PCI host
> > > > controllers, the pcibios_msi_controller weak callback is used to look-up
> > > > the msi_controller pointer, through pci_sys_data msi_ctrl pointer.
> > > > 
> > > > pci_sys_data is an ARM specific structure, which prevents using the
> > > > same mechanism (so same PCI host controller drivers) on ARM64 systems.
> > > > 
> > > > Since the struct pci_bus already contains an msi_controller pointer and
> > > > the kernel already uses it to look-up the msi controller,
> > > > this patch converts ARM host controller and related pcibios/host bridges
> > > > initialization routines so that the msi_controller pointer look-up can be
> > > > carried out by PCI core code through the struct pci_bus msi pointer,
> > > > removing the need for the arch specific pcibios_msi_controller callback
> > > > and the related pci_sys_data msi_ctrl pointer.
> > > > 
> > > > ARM is the only arch relying on the pcibios_msi_controller() weak
> > > > function, hence this patch removes the default weak implementation
> > > > from PCI core code since it becomes of no use.
> > > > 
> > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> > > > Cc: Pratyush Anand <pratyush.anand@gmail.com>
> > > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > > Cc: Jingoo Han <jingoohan1@gmail.com>
> > > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > > Cc: Simon Horman <horms@verge.net.au>
> > > > Cc: Russell King <linux@arm.linux.org.uk>
> > > > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > > > Cc: Thierry Reding <thierry.reding@gmail.com>
> > > > Cc: Michal Simek <michal.simek@xilinx.com>
> > > > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > > > ---
> > > > v1->v2
> > > > 
> > > > - Added patch to replace panic statements with WARN
> > > > - Removed unused pcibios_msi_controller() and pci_msi_controller() from
> > > >   core code
> > > > - Dropped RFT status
> > > > 
> > > > v1: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/356028.html
> > > > 
> > > >  arch/arm/include/asm/mach/pci.h    |  3 ---
> > > >  arch/arm/kernel/bios32.c           | 29 +++++++++++++----------------
> > > >  drivers/pci/host/pcie-designware.c |  9 +++++++--
> > > >  drivers/pci/host/pcie-xilinx.c     | 12 ++++++++++--
> > > >  drivers/pci/msi.c                  | 17 +----------------
> > > >  5 files changed, 31 insertions(+), 39 deletions(-)
> > > > 
> > > [...]
> > > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > > > index f66be86..0d20142 100644
> > > > --- a/drivers/pci/msi.c
> > > > +++ b/drivers/pci/msi.c
> > > > @@ -77,24 +77,9 @@ static void pci_msi_teardown_msi_irqs(struct pci_dev *dev)
> > > >  
> > > >  /* Arch hooks */
> > > >  
> > > > -struct msi_controller * __weak pcibios_msi_controller(struct pci_dev *dev)
> > > > -{
> > > > -	return NULL;
> > > > -}
> > > > -
> > > > -static struct msi_controller *pci_msi_controller(struct pci_dev *dev)
> > > > -{
> > > > -	struct msi_controller *msi_ctrl = dev->bus->msi;
> > > > -
> > > > -	if (msi_ctrl)
> > > > -		return msi_ctrl;
> > > > -
> > > > -	return pcibios_msi_controller(dev);
> > > > -}
> > > > -
> > > >  int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
> > > >  {
> > > > -	struct msi_controller *chip = pci_msi_controller(dev);
> > > > +	struct msi_controller *chip = dev->bus->msi;
> > > >  	int err;
> > > >  
> > > >  	if (!chip || !chip->setup_irq)
> > > 
> > > Don't you have to go to the top level bus and get the ->msi pointer? Something
> > > like:
> > > 
> > > 	for (bus = dev->bus; bus != NULL; bus = bus->parent)
> > > 		if (bus->msi)
> > > 			return bus->msi;
> > > 
> > > I have not been following this closely, so I may have missed some patches.
> > 
> > The msi pointer is initialized from parent to child in
> > pci_alloc_child_bus(), so PCI core does that for us,
> > that's my understanding.
> > 
> > It works the same as sysdata pointer, which is why
> > pcibios_msi_controller works in the current kernel.
> 
> Thanks.
> 
> > On a side note, are you able to prepare a new version
> > of your set to enable the PCI generic host on ARM64
> > (and remove pci_sys_data and related ifdef CONFIG_ARM
> > from it) ?
> > 
> > We are not far from removing the pci_sys_data dependency,
> > if you can't prepare a new version of your series let me
> > know I can do it on your behalf.
> 
> I will post an updated patch with a minor fix. I will also
> add a patch for parsing msi-parent and setting ->msi on
> the root bus.

There is no need for any msi parsing in the PCI generic host,
Marc implemented mechanism for us in core code:

https://lkml.org/lkml/2015/7/7/724

Thanks,
Lorenzo

Re: [PATCH v2 2/2] ARM: pci: kill pcibios_msi_controller

[Marc Zyngier]

On 26/07/15 19:43, Jayachandran C. wrote:
> On Sun, Jul 26, 2015 at 07:16:46PM +0100, Lorenzo Pieralisi wrote:
>> On Sun, Jul 26, 2015 at 04:25:29PM +0100, Jayachandran C. wrote:
>>> On Fri, Jul 24, 2015 at 05:13:03PM +0100, Lorenzo Pieralisi wrote:
>>>> On ARM PCI systems relying on the pcibios API to initialize PCI host
>>>> controllers, the pcibios_msi_controller weak callback is used to look-up
>>>> the msi_controller pointer, through pci_sys_data msi_ctrl pointer.
>>>>
>>>> pci_sys_data is an ARM specific structure, which prevents using the
>>>> same mechanism (so same PCI host controller drivers) on ARM64 systems.
>>>>
>>>> Since the struct pci_bus already contains an msi_controller pointer and
>>>> the kernel already uses it to look-up the msi controller,
>>>> this patch converts ARM host controller and related pcibios/host bridges
>>>> initialization routines so that the msi_controller pointer look-up can be
>>>> carried out by PCI core code through the struct pci_bus msi pointer,
>>>> removing the need for the arch specific pcibios_msi_controller callback
>>>> and the related pci_sys_data msi_ctrl pointer.
>>>>
>>>> ARM is the only arch relying on the pcibios_msi_controller() weak
>>>> function, hence this patch removes the default weak implementation
>>>> from PCI core code since it becomes of no use.
>>>>
>>>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>>> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> Cc: Pratyush Anand <pratyush.anand@gmail.com>
>>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>>> Cc: Jingoo Han <jingoohan1@gmail.com>
>>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>>>> Cc: Simon Horman <horms@verge.net.au>
>>>> Cc: Russell King <linux@arm.linux.org.uk>
>>>> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>>>> Cc: Thierry Reding <thierry.reding@gmail.com>
>>>> Cc: Michal Simek <michal.simek@xilinx.com>
>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>> ---
>>>> v1->v2
>>>>
>>>> - Added patch to replace panic statements with WARN
>>>> - Removed unused pcibios_msi_controller() and pci_msi_controller() from
>>>>   core code
>>>> - Dropped RFT status
>>>>
>>>> v1: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/356028.html
>>>>
>>>>  arch/arm/include/asm/mach/pci.h    |  3 ---
>>>>  arch/arm/kernel/bios32.c           | 29 +++++++++++++----------------
>>>>  drivers/pci/host/pcie-designware.c |  9 +++++++--
>>>>  drivers/pci/host/pcie-xilinx.c     | 12 ++++++++++--
>>>>  drivers/pci/msi.c                  | 17 +----------------
>>>>  5 files changed, 31 insertions(+), 39 deletions(-)
>>>>
>>> [...]
>>>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>>>> index f66be86..0d20142 100644
>>>> --- a/drivers/pci/msi.c
>>>> +++ b/drivers/pci/msi.c
>>>> @@ -77,24 +77,9 @@ static void pci_msi_teardown_msi_irqs(struct pci_dev *dev)
>>>>  
>>>>  /* Arch hooks */
>>>>  
>>>> -struct msi_controller * __weak pcibios_msi_controller(struct pci_dev *dev)
>>>> -{
>>>> -	return NULL;
>>>> -}
>>>> -
>>>> -static struct msi_controller *pci_msi_controller(struct pci_dev *dev)
>>>> -{
>>>> -	struct msi_controller *msi_ctrl = dev->bus->msi;
>>>> -
>>>> -	if (msi_ctrl)
>>>> -		return msi_ctrl;
>>>> -
>>>> -	return pcibios_msi_controller(dev);
>>>> -}
>>>> -
>>>>  int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
>>>>  {
>>>> -	struct msi_controller *chip = pci_msi_controller(dev);
>>>> +	struct msi_controller *chip = dev->bus->msi;
>>>>  	int err;
>>>>  
>>>>  	if (!chip || !chip->setup_irq)
>>>
>>> Don't you have to go to the top level bus and get the ->msi pointer? Something
>>> like:
>>>
>>> 	for (bus = dev->bus; bus != NULL; bus = bus->parent)
>>> 		if (bus->msi)
>>> 			return bus->msi;
>>>
>>> I have not been following this closely, so I may have missed some patches.
>>
>> The msi pointer is initialized from parent to child in
>> pci_alloc_child_bus(), so PCI core does that for us,
>> that's my understanding.
>>
>> It works the same as sysdata pointer, which is why
>> pcibios_msi_controller works in the current kernel.
> 
> Thanks.
> 
>> On a side note, are you able to prepare a new version
>> of your set to enable the PCI generic host on ARM64
>> (and remove pci_sys_data and related ifdef CONFIG_ARM
>> from it) ?
>>
>> We are not far from removing the pci_sys_data dependency,
>> if you can't prepare a new version of your series let me
>> know I can do it on your behalf.
> 
> I will post an updated patch with a minor fix. I will also
> add a patch for parsing msi-parent and setting ->msi on
> the root bus.

That shouldn't be necessary if you base your series on this one:

http://lwn.net/Articles/652151/

Patch 5 in this series does it for you as part of the core PCI code.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 2/2] ARM: pci: kill pcibios_msi_controller

[Russell King - ARM Linux]

On Fri, Jul 24, 2015 at 05:13:03PM +0100, Lorenzo Pieralisi wrote:
> On ARM PCI systems relying on the pcibios API to initialize PCI host
> controllers, the pcibios_msi_controller weak callback is used to look-up
> the msi_controller pointer, through pci_sys_data msi_ctrl pointer.
> 
> pci_sys_data is an ARM specific structure, which prevents using the
> same mechanism (so same PCI host controller drivers) on ARM64 systems.
> 
> Since the struct pci_bus already contains an msi_controller pointer and
> the kernel already uses it to look-up the msi controller,
> this patch converts ARM host controller and related pcibios/host bridges
> initialization routines so that the msi_controller pointer look-up can be
> carried out by PCI core code through the struct pci_bus msi pointer,
> removing the need for the arch specific pcibios_msi_controller callback
> and the related pci_sys_data msi_ctrl pointer.
> 
> ARM is the only arch relying on the pcibios_msi_controller() weak
> function, hence this patch removes the default weak implementation
> from PCI core code since it becomes of no use.

You don't mention the change from using pci_scan_root_bus() to using
pci_create_root_bus() here.

> -				sys->bus = pci_scan_root_bus(parent, sys->busnr,
> -						hw->ops, sys, &sys->resources);
> +			} else {
> +				sys->bus = pci_create_root_bus(parent,
> +							       sys->busnr,
> +							       hw->ops, sys,
> +							       &sys->resources);

By making this change, there is no nothing which will call
pci_bus_insert_busn_res().

What about the 18 users of the ->scan method, at least IOP13xx appears
to be MSI-enabled, though it's not clear whether it works with MSI.

This doesn't seem to be a good approach.  Maybe having a version of
pci_scan_root_bus() which takes the MSI data as an argument would be
better than selectively copying pci_scan_root_bus() into the ARM code?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 2/2] ARM: pci: kill pcibios_msi_controller

[Lorenzo Pieralisi]

On Sun, Jul 26, 2015 at 07:58:22PM +0100, Russell King - ARM Linux wrote:
> On Fri, Jul 24, 2015 at 05:13:03PM +0100, Lorenzo Pieralisi wrote:
> > On ARM PCI systems relying on the pcibios API to initialize PCI host
> > controllers, the pcibios_msi_controller weak callback is used to look-up
> > the msi_controller pointer, through pci_sys_data msi_ctrl pointer.
> > 
> > pci_sys_data is an ARM specific structure, which prevents using the
> > same mechanism (so same PCI host controller drivers) on ARM64 systems.
> > 
> > Since the struct pci_bus already contains an msi_controller pointer and
> > the kernel already uses it to look-up the msi controller,
> > this patch converts ARM host controller and related pcibios/host bridges
> > initialization routines so that the msi_controller pointer look-up can be
> > carried out by PCI core code through the struct pci_bus msi pointer,
> > removing the need for the arch specific pcibios_msi_controller callback
> > and the related pci_sys_data msi_ctrl pointer.
> > 
> > ARM is the only arch relying on the pcibios_msi_controller() weak
> > function, hence this patch removes the default weak implementation
> > from PCI core code since it becomes of no use.
> 
> You don't mention the change from using pci_scan_root_bus() to using
> pci_create_root_bus() here.
> 
> > -				sys->bus = pci_scan_root_bus(parent, sys->busnr,
> > -						hw->ops, sys, &sys->resources);
> > +			} else {
> > +				sys->bus = pci_create_root_bus(parent,
> > +							       sys->busnr,
> > +							       hw->ops, sys,
> > +							       &sys->resources);
> 
> By making this change, there is no nothing which will call
> pci_bus_insert_busn_res().

Yes, you are correct I noticed that too (I guess all ARM bridges
relying on pci_scan_root_bus do NOT initialize the BUS resource
so they rely on pci_scan_root_bus, and related BUS resource insertion
to function properly).

> What about the 18 users of the ->scan method, at least IOP13xx appears
> to be MSI-enabled, though it's not clear whether it works with MSI.

I do not think IOP13xx is affected by this change so we should be fine on
that side, I should have chased all msi_ctrl users in this series,
but the point raised above needs tackling regardless.

> This doesn't seem to be a good approach.  Maybe having a version of
> pci_scan_root_bus() which takes the MSI data as an argument would be
> better than selectively copying pci_scan_root_bus() into the ARM code?

I understand your point, let's try to find a fast way forward, we are
stuck otherwise:

1) I do not touch bios32 at all. I convert all host bridges that require
   an msi pointer to use the scan pointer in struct hw_pci and basically
   implement the change in bios32 in their scan method (I did that
   already for the in tree host bridges that rely on the scan method
   and require an msi pointer in pci_bus to function)
2) I add pci_bus_insert_busn_res() in the bios32 code, therefore as you
   said literally copying core code into bios32 (+ msi pointer
   initialization), horrible, but I promise it will disappear as soon
   as we are done converting all ARM PCI host to the new IRQ MSI domain
   infrastructure, which do not require an msi pointer in the struct
   pci_bus structure to be populated at all.
3) I patch pci_scan_root_bus() to pass the MSI pointer. I think that
   pcibios_msi_controller was added to avoid doing this, that
   function has already 5 parameters and it is a treewide change,
   I suspect Bjorn won't be happy about this at all.

Those are the reasonable ideas I have in mind, I think (1) is the
fastest (basically I _hope_ it should mean that I only have to patch
drivers/pci/host/pcie-rcar.c) and I avoid patching ARM bios32.

Comments very welcome before I proceed and I would appreciate some
feedback from PCI host bridges platform maintainers too to find a way
forward.

Thanks,
Lorenzo

Re: [PATCH v2 2/2] ARM: pci: kill pcibios_msi_controller

[Russell King - ARM Linux]

On Mon, Jul 27, 2015 at 10:40:46AM +0100, Lorenzo Pieralisi wrote:
> On Sun, Jul 26, 2015 at 07:58:22PM +0100, Russell King - ARM Linux wrote:
> > This doesn't seem to be a good approach.  Maybe having a version of
> > pci_scan_root_bus() which takes the MSI data as an argument would be
> > better than selectively copying pci_scan_root_bus() into the ARM code?
> 
> I understand your point, let's try to find a fast way forward, we are
> stuck otherwise:
> 
> 3) I patch pci_scan_root_bus() to pass the MSI pointer. I think that
>    pcibios_msi_controller was added to avoid doing this, that
>    function has already 5 parameters and it is a treewide change,
>    I suspect Bjorn won't be happy about this at all.

Or you could use the suggestion I made in the paragraph you quoted above,
which is a variation on your (3) without the down-sides of needing to
change lots of callsites.  Something like this (bios32.c is incomplete):

 arch/arm/kernel/bios32.c |  8 +++-----
 drivers/pci/probe.c      | 15 +++++++++++++--
 include/linux/pci.h      |  4 ++++
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index fcbbbb1b9e95..e4f7c0eb6c14 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -462,9 +462,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
 		if (!sys)
 			panic("PCI: unable to allocate sys data!");
 
-#ifdef CONFIG_PCI_MSI
-		sys->msi_ctrl = hw->msi_ctrl;
-#endif
 		sys->busnr   = busnr;
 		sys->swizzle = hw->swizzle;
 		sys->map_irq = hw->map_irq;
@@ -486,8 +483,9 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
 			if (hw->scan)
 				sys->bus = hw->scan(nr, sys);
 			else
-				sys->bus = pci_scan_root_bus(parent, sys->busnr,
-						hw->ops, sys, &sys->resources);
+				sys->bus = pci_scan_root_bus_msi(parent,
+						sys->busnr, hw->ops, sys,
+						&sys->resources, hw->msi_ctrl);
 
 			if (!sys->bus)
 				panic("PCI: unable to scan bus!");
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cefd636681b6..4915c6d9c22d 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2096,8 +2096,9 @@ void pci_bus_release_busn_res(struct pci_bus *b)
 			res, ret ? "can not be" : "is");
 }
 
-struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
-		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+struct pci_bus *pci_scan_root_bus_msi(struct device *parent, int bus,
+		struct pci_ops *ops, void *sysdata,
+		struct list_head *resources, struct msi_controller *msi)
 {
 	struct resource_entry *window;
 	bool found = false;
@@ -2114,6 +2115,8 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
 	if (!b)
 		return NULL;
 
+	b->msi = msi;
+
 	if (!found) {
 		dev_info(&b->dev,
 		 "No busn resource found for root bus, will use [bus %02x-ff]\n",
@@ -2128,6 +2131,14 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
 
 	return b;
 }
+EXPORT_SYMBOL(pci_scan_root_bus_msi);
+
+struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
+		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+{
+	return pci_scan_root_bus_msi(parent, bus, ops, sysdata, resources,
+				     NULL);
+}
 EXPORT_SYMBOL(pci_scan_root_bus);
 
 struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops,
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8a0321a8fb59..4d4f9d29fcc9 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -787,6 +787,10 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
 int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
 void pci_bus_release_busn_res(struct pci_bus *b);
+struct pci_bus *pci_scan_root_bus_msi(struct device *parent, int bus,
+				      struct pci_ops *ops, void *sysdata,
+				      struct list_head *resources,
+				      struct msi_controller *msi);
 struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
 					     struct pci_ops *ops, void *sysdata,
 					     struct list_head *resources);

And it's probably a good idea to kill off the ifdef around this:

struct hw_pci {
#ifdef CONFIG_PCI_MSI
        struct msi_controller *msi_ctrl;
#endif

so that we can avoid ifdefs in random places in arch/arm code (as is the
decision in the rest of PCI for this.)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 2/2] ARM: pci: kill pcibios_msi_controller

[Lorenzo Pieralisi]

On Mon, Jul 27, 2015 at 11:54:20AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 27, 2015 at 10:40:46AM +0100, Lorenzo Pieralisi wrote:
> > On Sun, Jul 26, 2015 at 07:58:22PM +0100, Russell King - ARM Linux wrote:
> > > This doesn't seem to be a good approach.  Maybe having a version of
> > > pci_scan_root_bus() which takes the MSI data as an argument would be
> > > better than selectively copying pci_scan_root_bus() into the ARM code?
> > 
> > I understand your point, let's try to find a fast way forward, we are
> > stuck otherwise:
> > 
> > 3) I patch pci_scan_root_bus() to pass the MSI pointer. I think that
> >    pcibios_msi_controller was added to avoid doing this, that
> >    function has already 5 parameters and it is a treewide change,
> >    I suspect Bjorn won't be happy about this at all.
> 
> Or you could use the suggestion I made in the paragraph you quoted above,
> which is a variation on your (3) without the down-sides of needing to
> change lots of callsites.  Something like this (bios32.c is incomplete):

It is fine by me, I do not know if Bjorn is willing to accept the
PCI core change required below.

Bjorn, what do you think ? I will fold the diff below in the original
patch if we are all happy with the change.

Thanks !
Lorenzo

>  arch/arm/kernel/bios32.c |  8 +++-----
>  drivers/pci/probe.c      | 15 +++++++++++++--
>  include/linux/pci.h      |  4 ++++
>  3 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index fcbbbb1b9e95..e4f7c0eb6c14 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -462,9 +462,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
>  		if (!sys)
>  			panic("PCI: unable to allocate sys data!");
>  
> -#ifdef CONFIG_PCI_MSI
> -		sys->msi_ctrl = hw->msi_ctrl;
> -#endif
>  		sys->busnr   = busnr;
>  		sys->swizzle = hw->swizzle;
>  		sys->map_irq = hw->map_irq;
> @@ -486,8 +483,9 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
>  			if (hw->scan)
>  				sys->bus = hw->scan(nr, sys);
>  			else
> -				sys->bus = pci_scan_root_bus(parent, sys->busnr,
> -						hw->ops, sys, &sys->resources);
> +				sys->bus = pci_scan_root_bus_msi(parent,
> +						sys->busnr, hw->ops, sys,
> +						&sys->resources, hw->msi_ctrl);
>  
>  			if (!sys->bus)
>  				panic("PCI: unable to scan bus!");
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index cefd636681b6..4915c6d9c22d 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2096,8 +2096,9 @@ void pci_bus_release_busn_res(struct pci_bus *b)
>  			res, ret ? "can not be" : "is");
>  }
>  
> -struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
> -		struct pci_ops *ops, void *sysdata, struct list_head *resources)
> +struct pci_bus *pci_scan_root_bus_msi(struct device *parent, int bus,
> +		struct pci_ops *ops, void *sysdata,
> +		struct list_head *resources, struct msi_controller *msi)
>  {
>  	struct resource_entry *window;
>  	bool found = false;
> @@ -2114,6 +2115,8 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
>  	if (!b)
>  		return NULL;
>  
> +	b->msi = msi;
> +
>  	if (!found) {
>  		dev_info(&b->dev,
>  		 "No busn resource found for root bus, will use [bus %02x-ff]\n",
> @@ -2128,6 +2131,14 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
>  
>  	return b;
>  }
> +EXPORT_SYMBOL(pci_scan_root_bus_msi);
> +
> +struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
> +		struct pci_ops *ops, void *sysdata, struct list_head *resources)
> +{
> +	return pci_scan_root_bus_msi(parent, bus, ops, sysdata, resources,
> +				     NULL);
> +}
>  EXPORT_SYMBOL(pci_scan_root_bus);
>  
>  struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops,
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8a0321a8fb59..4d4f9d29fcc9 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -787,6 +787,10 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
>  int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
>  void pci_bus_release_busn_res(struct pci_bus *b);
> +struct pci_bus *pci_scan_root_bus_msi(struct device *parent, int bus,
> +				      struct pci_ops *ops, void *sysdata,
> +				      struct list_head *resources,
> +				      struct msi_controller *msi);
>  struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
>  					     struct pci_ops *ops, void *sysdata,
>  					     struct list_head *resources);
> 
> And it's probably a good idea to kill off the ifdef around this:
> 
> struct hw_pci {
> #ifdef CONFIG_PCI_MSI
>         struct msi_controller *msi_ctrl;
> #endif
> 
> so that we can avoid ifdefs in random places in arch/arm code (as is the
> decision in the rest of PCI for this.)
> 
> -- 
> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> according to speedtest.net.
> 

Re: [PATCH v2 1/2] ARM: PCI: bios32: replace panic with WARN messages on failures

[Marc Zyngier]

On 24/07/15 17:13, Lorenzo Pieralisi wrote:
> In the ARM PCI bios32 layer, failures to dynamically allocate pci_sys_data
> for a PCI bus, or a PCI bus scan failure have to be considered serious
> warnings but they should not trigger a system panic so that at least the
> system is given a chance to be debugged.
> 
> This patch replaces the panic statements with WARN() messages to
> improve error reporting in the ARM PCI bios32 layer.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Marc Zyngier <marc.zyngier@arm.com>

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...