[PATCH 0/5] Modularize PCI_DW related drivers.

[PATCH 0/5] Modularize PCI_DW related drivers.

[Paul Gortmaker]

In a recent patch series that aimed to remove code related to module
unload for PCI support that was simply non modular, the discussion
led to people wanting to keep the code and push towards taking the
steps needed to support moving it towards tristate instead[1].

Here, we take step one, which is simply making the Kconfig change
and then dealing with any build fallout or modpost fallout.  What
amounts to essentially a sanity build test.  To be clear, these
have not been runtime validated; that will need to be done by those
with access to real hardware.  However, the changes are not anything
that should disrupt any existing built-in validation, so real world
users should not be impacted by this change.

We start with a smaller family of drivers; those that actively select
PCI_DW, as a nice self contained group to test the waters and see if
everyone is still good with this approach before investing more time
on a wider scale to other pci/host/ code blocks.

As such the drivers here share a dependency on having the same group
of functions exported in order to successfully complete modpost.

In addition, we have to stray outside drivers/pci to add exports
in two places; once for an ARM fault handler, and once for an OF
variable.

The pci-keystone-dw.c instance was handled separately because it
consists of two source files that need their own group of driver
specific exports above and beyond the "shared" ones.

Then we convert the Kconfig for all remaining at once; we could have
done it on a per driver basis for ease of revert if anyone really
objects, but since it would be a one line change, that seemed like
not a real concern.

Build testing was done on the linux-next tree for arm allmodconfig.

[1] https://lkml.kernel.org/r/20160108203102.GH5354 at localhost

-- 

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: devicetree at vger.kernel.org
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Ley Foon Tan <lftan@altera.com>
Cc: Murali Karicheri <m-karicheri2@ti.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Stanimir Varbanov <svarbanov@mm-sol.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-arm-msm at vger.kernel.org
Cc: linux-pci at vger.kernel.org


Paul Gortmaker (5):
  ARM: add EXPORT_SYMBOL of hook_fault_code for PCI host modularization
  drivers/of: add EXPORT_SYMBOL to of_irq_count
  drivers/pci: export dw syms enabling board specific PCI code to be
    tristate
  drivers/pci: make host/pci-keystone-dw.c modular
  drivers/pci: make most of the PCI_DW drivers modular

 arch/arm/mm/fault.c                |  3 ++-
 drivers/of/irq.c                   |  1 +
 drivers/pci/host/Kconfig           | 16 ++++++++--------
 drivers/pci/host/pci-keystone-dw.c | 16 +++++++++++++++-
 drivers/pci/host/pcie-designware.c |  7 +++++++
 5 files changed, 33 insertions(+), 10 deletions(-)

-- 
2.6.1

[PATCH 1/5] ARM: add EXPORT_SYMBOL of hook_fault_code for PCI host modularization

[Paul Gortmaker]

In a discussion of a previous patch set[1], it was suggested that
modularizing some of the PCI host support would be good to keep
multi platform bzImage sizes smaller.

Two of the files that are candidates for conversion to tristate
from bool are:

drivers/pci/host/pci-imx6.c
drivers/pci/host/pci-keystone.c

However, doing the conversion reveals that they are going to fail
at modpost time since hook_fault_code isn't currently exported.

Since we are now going to export it we also need to remove the
__init tag, as the fcn needs to be present at insmod time.

[1] https://lkml.kernel.org/r/20160108203102.GH5354 at localhost

Cc: Russell King <linux@arm.linux.org.uk>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Ley Foon Tan <lftan@altera.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 arch/arm/mm/fault.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index daafcf121ce0..e0696a5ecc9b 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -525,7 +525,7 @@ struct fsr_info {
 #include "fsr-2level.c"
 #endif
 
-void __init
+void
 hook_fault_code(int nr, int (*fn)(unsigned long, unsigned int, struct pt_regs *),
 		int sig, int code, const char *name)
 {
@@ -537,6 +537,7 @@ hook_fault_code(int nr, int (*fn)(unsigned long, unsigned int, struct pt_regs *)
 	fsr_info[nr].code = code;
 	fsr_info[nr].name = name;
 }
+EXPORT_SYMBOL(hook_fault_code);
 
 /*
  * Dispatch a data abort to the relevant handler.
-- 
2.6.1

[PATCH 4/5] drivers/pci: make host/pci-keystone-dw.c modular

[Paul Gortmaker]

Export the symbols that this driver requires in order for it to
be modular.  In addition to the one use case of a dw_pci sym, it
has many instances of its own ks_dw_pci syms that need exporting
in order to modpost w/o error.

Cc: Murali Karicheri <m-karicheri2@ti.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Stanimir Varbanov <svarbanov@mm-sol.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-pci at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/pci/host/Kconfig           |  2 +-
 drivers/pci/host/pci-keystone-dw.c | 16 +++++++++++++++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 75a605426538..b040ad7ba44d 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -69,7 +69,7 @@ config PCIE_SPEAR13XX
 	  Say Y here if you want PCIe support on SPEAr13XX SoCs.
 
 config PCI_KEYSTONE
-	bool "TI Keystone PCIe controller"
+	tristate "TI Keystone PCIe controller"
 	depends on ARCH_KEYSTONE
 	select PCIE_DW
 	select PCIEPORTBUS
diff --git a/drivers/pci/host/pci-keystone-dw.c b/drivers/pci/host/pci-keystone-dw.c
index ed34c9520a02..f1550a8d2e29 100644
--- a/drivers/pci/host/pci-keystone-dw.c
+++ b/drivers/pci/host/pci-keystone-dw.c
@@ -76,6 +76,7 @@ phys_addr_t ks_dw_pcie_get_msi_addr(struct pcie_port *pp)
 
 	return ks_pcie->app.start + MSI_IRQ;
 }
+EXPORT_SYMBOL(ks_dw_pcie_get_msi_addr);
 
 void ks_dw_pcie_handle_msi_irq(struct keystone_pcie *ks_pcie, int offset)
 {
@@ -99,6 +100,7 @@ void ks_dw_pcie_handle_msi_irq(struct keystone_pcie *ks_pcie, int offset)
 		}
 	}
 }
+EXPORT_SYMBOL(ks_dw_pcie_handle_msi_irq);
 
 static void ks_dw_pcie_msi_irq_ack(struct irq_data *d)
 {
@@ -127,6 +129,7 @@ void ks_dw_pcie_msi_set_irq(struct pcie_port *pp, int irq)
 	writel(BIT(bit_pos),
 	       ks_pcie->va_app_base + MSI0_IRQ_ENABLE_SET + (reg_offset << 4));
 }
+EXPORT_SYMBOL(ks_dw_pcie_msi_set_irq);
 
 void ks_dw_pcie_msi_clear_irq(struct pcie_port *pp, int irq)
 {
@@ -137,6 +140,7 @@ void ks_dw_pcie_msi_clear_irq(struct pcie_port *pp, int irq)
 	writel(BIT(bit_pos),
 	       ks_pcie->va_app_base + MSI0_IRQ_ENABLE_CLR + (reg_offset << 4));
 }
+EXPORT_SYMBOL(ks_dw_pcie_msi_clear_irq);
 
 static void ks_dw_pcie_msi_irq_mask(struct irq_data *d)
 {
@@ -220,6 +224,7 @@ int ks_dw_pcie_msi_host_init(struct pcie_port *pp, struct msi_controller *chip)
 
 	return 0;
 }
+EXPORT_SYMBOL(ks_dw_pcie_msi_host_init);
 
 void ks_dw_pcie_enable_legacy_irqs(struct keystone_pcie *ks_pcie)
 {
@@ -228,6 +233,7 @@ void ks_dw_pcie_enable_legacy_irqs(struct keystone_pcie *ks_pcie)
 	for (i = 0; i < MAX_LEGACY_IRQS; i++)
 		writel(0x1, ks_pcie->va_app_base + IRQ_ENABLE_SET + (i << 4));
 }
+EXPORT_SYMBOL(ks_dw_pcie_enable_legacy_irqs);
 
 void ks_dw_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie, int offset)
 {
@@ -247,6 +253,7 @@ void ks_dw_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie, int offset)
 	/* EOI the INTx interrupt */
 	writel(offset, ks_pcie->va_app_base + IRQ_EOI);
 }
+EXPORT_SYMBOL(ks_dw_pcie_handle_legacy_irq);
 
 static void ks_dw_pcie_ack_legacy_irq(struct irq_data *d)
 {
@@ -347,6 +354,7 @@ void ks_dw_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie)
 	writel(OB_XLAT_EN_VAL | readl(ks_pcie->va_app_base + CMD_STATUS),
 	       ks_pcie->va_app_base + CMD_STATUS);
 }
+EXPORT_SYMBOL(ks_dw_pcie_setup_rc_app_regs);
 
 /**
  * ks_pcie_cfg_setup() - Set up configuration space address for a device
@@ -400,6 +408,7 @@ int ks_dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
 
 	return dw_pcie_cfg_read(addr + where, size, val);
 }
+EXPORT_SYMBOL(ks_dw_pcie_rd_other_conf);
 
 int ks_dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
 			     unsigned int devfn, int where, int size, u32 val)
@@ -412,6 +421,7 @@ int ks_dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
 
 	return dw_pcie_cfg_write(addr + where, size, val);
 }
+EXPORT_SYMBOL(ks_dw_pcie_wr_other_conf);
 
 /**
  * ks_dw_pcie_v3_65_scan_bus() - keystone scan_bus post initialization
@@ -437,6 +447,7 @@ void ks_dw_pcie_v3_65_scan_bus(struct pcie_port *pp)
 	  */
 	writel(ks_pcie->app.start, pp->dbi_base + PCI_BASE_ADDRESS_0);
 }
+EXPORT_SYMBOL(ks_dw_pcie_v3_65_scan_bus);
 
 /**
  * ks_dw_pcie_link_up() - Check if link up
@@ -447,6 +458,7 @@ int ks_dw_pcie_link_up(struct pcie_port *pp)
 
 	return (val & LTSSM_STATE_MASK) == LTSSM_STATE_L0;
 }
+EXPORT_SYMBOL(ks_dw_pcie_link_up);
 
 void ks_dw_pcie_initiate_link_train(struct keystone_pcie *ks_pcie)
 {
@@ -461,6 +473,7 @@ void ks_dw_pcie_initiate_link_train(struct keystone_pcie *ks_pcie)
 	val = readl(ks_pcie->va_app_base + CMD_STATUS);
 	writel(LTSSM_EN_VAL | val,  ks_pcie->va_app_base + CMD_STATUS);
 }
+EXPORT_SYMBOL(ks_dw_pcie_initiate_link_train);
 
 /**
  * ks_dw_pcie_host_init() - initialize host for v3_65 dw hardware
@@ -469,7 +482,7 @@ void ks_dw_pcie_initiate_link_train(struct keystone_pcie *ks_pcie)
  * and call dw_pcie_v3_65_host_init() API to initialize the Keystone
  * PCI host controller.
  */
-int __init ks_dw_pcie_host_init(struct keystone_pcie *ks_pcie,
+int ks_dw_pcie_host_init(struct keystone_pcie *ks_pcie,
 				struct device_node *msi_intc_np)
 {
 	struct pcie_port *pp = &ks_pcie->pp;
@@ -510,3 +523,4 @@ int __init ks_dw_pcie_host_init(struct keystone_pcie *ks_pcie,
 
 	return dw_pcie_host_init(pp);
 }
+EXPORT_SYMBOL(ks_dw_pcie_host_init);
-- 
2.6.1

[PATCH 1/5] ARM: add EXPORT_SYMBOL of hook_fault_code for PCI host modularization

[Arnd Bergmann]

On Sunday 07 February 2016 19:00:40 Paul Gortmaker wrote:
> In a discussion of a previous patch set[1], it was suggested that
> modularizing some of the PCI host support would be good to keep
> multi platform bzImage sizes smaller.
> 
> Two of the files that are candidates for conversion to tristate
> from bool are:
> 
> drivers/pci/host/pci-imx6.c
> drivers/pci/host/pci-keystone.c
> 
> However, doing the conversion reveals that they are going to fail
> at modpost time since hook_fault_code isn't currently exported.
> 
> Since we are now going to export it we also need to remove the
> __init tag, as the fcn needs to be present at insmod time.
> 
> [1] https://lkml.kernel.org/r/20160108203102.GH5354 at localhost
> 

If we want to make hook_fault_code() visible, we at least also need
an unhook_fault_code() function that removes the fault handler
when unloading the module, and preferably does so in a safe way.

The usage of hook_fault_code() at least in the imx driver is also
problematic, as it just ignores all "imprecise external abort"
faults, whether they come from PCI or not.

	Arnd

[PATCH 4/5] drivers/pci: make host/pci-keystone-dw.c modular

[Arnd Bergmann]

On Sunday 07 February 2016 19:00:43 Paul Gortmaker wrote:
> Export the symbols that this driver requires in order for it to
> be modular.  In addition to the one use case of a dw_pci sym, it
> has many instances of its own ks_dw_pci syms that need exporting
> in order to modpost w/o error.
> 
> Cc: Murali Karicheri <m-karicheri2@ti.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Stanimir Varbanov <svarbanov@mm-sol.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: linux-pci at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---


Could we just have one module for the driver  instead of two?

Something like below (untested)

	Arnd

diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 7b2f20c6ccc6..0b472c680348 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -8,7 +8,8 @@ obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o
 obj-$(CONFIG_PCI_RCAR_GEN2_PCIE) += pcie-rcar.o
 obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
 obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
-obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
+obj-$(CONFIG_PCI_KEYSTONE) pci-keystone-mod.o
+pci-keystone-mod-objs := pci-keystone-dw.o pci-keystone.o
 obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
 obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
 obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o

[PATCH 4/5] drivers/pci: make host/pci-keystone-dw.c modular

[Paul Gortmaker]

[Re: [PATCH 4/5] drivers/pci: make host/pci-keystone-dw.c modular] On 08/02/2016 (Mon 10:59) Arnd Bergmann wrote:

> On Sunday 07 February 2016 19:00:43 Paul Gortmaker wrote:
> > Export the symbols that this driver requires in order for it to
> > be modular.  In addition to the one use case of a dw_pci sym, it
> > has many instances of its own ks_dw_pci syms that need exporting
> > in order to modpost w/o error.
> > 
> > Cc: Murali Karicheri <m-karicheri2@ti.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Stanimir Varbanov <svarbanov@mm-sol.com>
> > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > Cc: Thierry Reding <thierry.reding@gmail.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: linux-pci at vger.kernel.org
> > Cc: linux-arm-kernel at lists.infradead.org
> > Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> > ---
> 
> 
> Could we just have one module for the driver  instead of two?

That thought crossed my mind, but I was considering merging the C files
and figured it might be more churn than the maintainer wanted.  If
maintainers are good with the below (assuming it works) then yes that
also seems reasonable.

P.
--

> 
> Something like below (untested)
> 
> 	Arnd
> 
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 7b2f20c6ccc6..0b472c680348 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -8,7 +8,8 @@ obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o
>  obj-$(CONFIG_PCI_RCAR_GEN2_PCIE) += pcie-rcar.o
>  obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
>  obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
> -obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
> +obj-$(CONFIG_PCI_KEYSTONE) pci-keystone-mod.o
> +pci-keystone-mod-objs := pci-keystone-dw.o pci-keystone.o
>  obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
>  obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
>  obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
> 

[PATCH 1/5] ARM: add EXPORT_SYMBOL of hook_fault_code for PCI host modularization

[Paul Gortmaker]

[Re: [PATCH 1/5] ARM: add EXPORT_SYMBOL of hook_fault_code for PCI host modularization] On 08/02/2016 (Mon 10:53) Arnd Bergmann wrote:

> On Sunday 07 February 2016 19:00:40 Paul Gortmaker wrote:
> > In a discussion of a previous patch set[1], it was suggested that
> > modularizing some of the PCI host support would be good to keep
> > multi platform bzImage sizes smaller.
> > 
> > Two of the files that are candidates for conversion to tristate
> > from bool are:
> > 
> > drivers/pci/host/pci-imx6.c
> > drivers/pci/host/pci-keystone.c
> > 
> > However, doing the conversion reveals that they are going to fail
> > at modpost time since hook_fault_code isn't currently exported.
> > 
> > Since we are now going to export it we also need to remove the
> > __init tag, as the fcn needs to be present at insmod time.
> > 
> > [1] https://lkml.kernel.org/r/20160108203102.GH5354 at localhost
> > 
> 
> If we want to make hook_fault_code() visible, we at least also need
> an unhook_fault_code() function that removes the fault handler
> when unloading the module, and preferably does so in a safe way.

If it is straight forward by doing an unwind of the existing hook, then
I can look at having a go at implementing it, but again per the 0/N the
original goal here was the "step one" of making it at least buildable,
and then moving on from there, preferably by people with the platforms
and deeper knowledge of the runtime requirements and issues.

> 
> The usage of hook_fault_code() at least in the imx driver is also
> problematic, as it just ignores all "imprecise external abort"
> faults, whether they come from PCI or not.

That is definitely beyond anything I'm suitably armed to fix within the
scope of this series, and will have to be from the imx people or similar.

Paul.
--


> 
> 	Arnd

[PATCH 1/5] ARM: add EXPORT_SYMBOL of hook_fault_code for PCI host modularization

[Russell King - ARM Linux]

On Mon, Feb 08, 2016 at 10:53:10AM +0100, Arnd Bergmann wrote:
> On Sunday 07 February 2016 19:00:40 Paul Gortmaker wrote:
> > In a discussion of a previous patch set[1], it was suggested that
> > modularizing some of the PCI host support would be good to keep
> > multi platform bzImage sizes smaller.
> > 
> > Two of the files that are candidates for conversion to tristate
> > from bool are:
> > 
> > drivers/pci/host/pci-imx6.c
> > drivers/pci/host/pci-keystone.c
> > 
> > However, doing the conversion reveals that they are going to fail
> > at modpost time since hook_fault_code isn't currently exported.
> > 
> > Since we are now going to export it we also need to remove the
> > __init tag, as the fcn needs to be present at insmod time.
> > 
> > [1] https://lkml.kernel.org/r/20160108203102.GH5354 at localhost
> > 
> 
> If we want to make hook_fault_code() visible, we at least also need
> an unhook_fault_code() function that removes the fault handler
> when unloading the module, and preferably does so in a safe way.
> 
> The usage of hook_fault_code() at least in the imx driver is also
> problematic, as it just ignores all "imprecise external abort"
> faults, whether they come from PCI or not.

And that gets a NAK from me.  We can't take locks around this code
(to do so would be deadlocky due to imprecise aborts), so we can't
ever safely remove any function that has been hooked into the fault
processing.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 1/5] ARM: add EXPORT_SYMBOL of hook_fault_code for PCI host modularization

[Russell King - ARM Linux]

On Sun, Feb 07, 2016 at 07:00:40PM -0500, Paul Gortmaker wrote:
> In a discussion of a previous patch set[1], it was suggested that
> modularizing some of the PCI host support would be good to keep
> multi platform bzImage sizes smaller.
> 
> Two of the files that are candidates for conversion to tristate
> from bool are:
> 
> drivers/pci/host/pci-imx6.c
> drivers/pci/host/pci-keystone.c
> 
> However, doing the conversion reveals that they are going to fail
> at modpost time since hook_fault_code isn't currently exported.
> 
> Since we are now going to export it we also need to remove the
> __init tag, as the fcn needs to be present at insmod time.
> 
> [1] https://lkml.kernel.org/r/20160108203102.GH5354 at localhost
> 
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Ley Foon Tan <lftan@altera.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: linux-arm-kernel at lists.infradead.org
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>  arch/arm/mm/fault.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
> index daafcf121ce0..e0696a5ecc9b 100644
> --- a/arch/arm/mm/fault.c
> +++ b/arch/arm/mm/fault.c
> @@ -525,7 +525,7 @@ struct fsr_info {
>  #include "fsr-2level.c"
>  #endif
>  
> -void __init
> +void
>  hook_fault_code(int nr, int (*fn)(unsigned long, unsigned int, struct pt_regs *),
>  		int sig, int code, const char *name)
>  {
> @@ -537,6 +537,7 @@ hook_fault_code(int nr, int (*fn)(unsigned long, unsigned int, struct pt_regs *)
>  	fsr_info[nr].code = code;
>  	fsr_info[nr].name = name;
>  }
> +EXPORT_SYMBOL(hook_fault_code);

I'm really not a fan of this change - the hooks are supposed to remain
mostly static once the system is up and running, and should not change
at runtime (since there is no locking on these - see my previous reply.)
There's no protection (or support) for more than one function - iow, if
we have two modules trying to hook into the same fault code, the last
taker gets it (although the previous owner may get called given the
right timing.)

While it may be limited to two drivers at the moment, I'd like it to
remain that way.

If we really must have this, then please use EXPORT_SYMBOL_GPL() - I
don't want this interface to be used by code which isn't GPL compliant.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 4/5] drivers/pci: make host/pci-keystone-dw.c modular

[Murali Karicheri]

On 02/08/2016 10:53 AM, Paul Gortmaker wrote:
> [Re: [PATCH 4/5] drivers/pci: make host/pci-keystone-dw.c modular] On 08/02/2016 (Mon 10:59) Arnd Bergmann wrote:
> 
>> On Sunday 07 February 2016 19:00:43 Paul Gortmaker wrote:
>>> Export the symbols that this driver requires in order for it to
>>> be modular.  In addition to the one use case of a dw_pci sym, it
>>> has many instances of its own ks_dw_pci syms that need exporting
>>> in order to modpost w/o error.
>>>
>>> Cc: Murali Karicheri <m-karicheri2@ti.com>
>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>>> Cc: Stanimir Varbanov <svarbanov@mm-sol.com>
>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>> Cc: Thierry Reding <thierry.reding@gmail.com>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Cc: linux-pci at vger.kernel.org
>>> Cc: linux-arm-kernel at lists.infradead.org
>>> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
>>> ---
>>
>>
>> Could we just have one module for the driver  instead of two?

I agree. Please note that Keystone PCI is tested only as static. 
I can help test the module if this series get to the next revision,
but it might be a while before I can do the test.

Murali

> 
> That thought crossed my mind, but I was considering merging the C files
> and figured it might be more churn than the maintainer wanted.  If
> maintainers are good with the below (assuming it works) then yes that
> also seems reasonable.
> 
> P.
> --
> 
>>
>> Something like below (untested)
>>
>> 	Arnd
>>
>> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
>> index 7b2f20c6ccc6..0b472c680348 100644
>> --- a/drivers/pci/host/Makefile
>> +++ b/drivers/pci/host/Makefile
>> @@ -8,7 +8,8 @@ obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o
>>  obj-$(CONFIG_PCI_RCAR_GEN2_PCIE) += pcie-rcar.o
>>  obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
>>  obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
>> -obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
>> +obj-$(CONFIG_PCI_KEYSTONE) pci-keystone-mod.o
>> +pci-keystone-mod-objs := pci-keystone-dw.o pci-keystone.o
>>  obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
>>  obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
>>  obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
>>
> 


-- 
Murali Karicheri
Linux Kernel, Keystone

[PATCH 0/5] Modularize PCI_DW related drivers.

[Kishon Vijay Abraham I]

Hi,

On Monday 08 February 2016 05:30 AM, Paul Gortmaker wrote:
> In a recent patch series that aimed to remove code related to module
> unload for PCI support that was simply non modular, the discussion
> led to people wanting to keep the code and push towards taking the
> steps needed to support moving it towards tristate instead[1].
> 
> Here, we take step one, which is simply making the Kconfig change
> and then dealing with any build fallout or modpost fallout.  What
> amounts to essentially a sanity build test.  To be clear, these
> have not been runtime validated; that will need to be done by those
> with access to real hardware.  However, the changes are not anything
> that should disrupt any existing built-in validation, so real world
> users should not be impacted by this change.
> 
> We start with a smaller family of drivers; those that actively select
> PCI_DW, as a nice self contained group to test the waters and see if
> everyone is still good with this approach before investing more time
> on a wider scale to other pci/host/ code blocks.
> 
> As such the drivers here share a dependency on having the same group
> of functions exported in order to successfully complete modpost.
> 
> In addition, we have to stray outside drivers/pci to add exports
> in two places; once for an ARM fault handler, and once for an OF
> variable.
> 
> The pci-keystone-dw.c instance was handled separately because it
> consists of two source files that need their own group of driver
> specific exports above and beyond the "shared" ones.
> 
> Then we convert the Kconfig for all remaining at once; we could have
> done it on a per driver basis for ease of revert if anyone really
> objects, but since it would be a one line change, that seemed like
> not a real concern.
> 
> Build testing was done on the linux-next tree for arm allmodconfig.

I took these patches and gave a test with DRA7xx board. As expected there was
no issues when the driver was built-in. However when I tried to rmmod/modprobe
I got this error [2].

Thanks
Kishon

[2] -> http://pastebin.ubuntu.com/15185894/
> 
> [1] https://lkml.kernel.org/r/20160108203102.GH5354 at localhost
> 

[PATCH 0/5] Modularize PCI_DW related drivers.

[Arnd Bergmann]

On Wednesday 24 February 2016 11:39:26 Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Monday 08 February 2016 05:30 AM, Paul Gortmaker wrote:
> > In a recent patch series that aimed to remove code related to module
> > unload for PCI support that was simply non modular, the discussion
> > led to people wanting to keep the code and push towards taking the
> > steps needed to support moving it towards tristate instead[1].
> > 
> > Here, we take step one, which is simply making the Kconfig change
> > and then dealing with any build fallout or modpost fallout.  What
> > amounts to essentially a sanity build test.  To be clear, these
> > have not been runtime validated; that will need to be done by those
> > with access to real hardware.  However, the changes are not anything
> > that should disrupt any existing built-in validation, so real world
> > users should not be impacted by this change.
> > 
> > We start with a smaller family of drivers; those that actively select
> > PCI_DW, as a nice self contained group to test the waters and see if
> > everyone is still good with this approach before investing more time
> > on a wider scale to other pci/host/ code blocks.
> > 
> > As such the drivers here share a dependency on having the same group
> > of functions exported in order to successfully complete modpost.
> > 
> > In addition, we have to stray outside drivers/pci to add exports
> > in two places; once for an ARM fault handler, and once for an OF
> > variable.
> > 
> > The pci-keystone-dw.c instance was handled separately because it
> > consists of two source files that need their own group of driver
> > specific exports above and beyond the "shared" ones.
> > 
> > Then we convert the Kconfig for all remaining at once; we could have
> > done it on a per driver basis for ease of revert if anyone really
> > objects, but since it would be a one line change, that seemed like
> > not a real concern.
> > 
> > Build testing was done on the linux-next tree for arm allmodconfig.
> 
> I took these patches and gave a test with DRA7xx board. As expected there was
> no issues when the driver was built-in. However when I tried to rmmod/modprobe
> I got this error [2].

Thanks for testing this!

> [2] -> http://pastebin.ubuntu.com/15185894/

It looks like you are hitting the BUG_ON() in ioremap_pte_range()
that checks if a virtual address already has a page table entry,
which in turn is probably a result of dw_pcie_host_init()
calling pci_remap_iospace() again for the same memory area
it has called the last time, and no cleanup done inbetween.

Could you try adding a pci_unmap_iospace() and calling that
in the device remove function? Let me know if you need help
implementing it.

	Arnd

[PATCH 0/5] Modularize PCI_DW related drivers.

[Kishon Vijay Abraham I]

Hi Arnd,

On Wednesday 24 February 2016 02:34 PM, Arnd Bergmann wrote:
> On Wednesday 24 February 2016 11:39:26 Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Monday 08 February 2016 05:30 AM, Paul Gortmaker wrote:
>>> In a recent patch series that aimed to remove code related to module
>>> unload for PCI support that was simply non modular, the discussion
>>> led to people wanting to keep the code and push towards taking the
>>> steps needed to support moving it towards tristate instead[1].
>>>
>>> Here, we take step one, which is simply making the Kconfig change
>>> and then dealing with any build fallout or modpost fallout.  What
>>> amounts to essentially a sanity build test.  To be clear, these
>>> have not been runtime validated; that will need to be done by those
>>> with access to real hardware.  However, the changes are not anything
>>> that should disrupt any existing built-in validation, so real world
>>> users should not be impacted by this change.
>>>
>>> We start with a smaller family of drivers; those that actively select
>>> PCI_DW, as a nice self contained group to test the waters and see if
>>> everyone is still good with this approach before investing more time
>>> on a wider scale to other pci/host/ code blocks.
>>>
>>> As such the drivers here share a dependency on having the same group
>>> of functions exported in order to successfully complete modpost.
>>>
>>> In addition, we have to stray outside drivers/pci to add exports
>>> in two places; once for an ARM fault handler, and once for an OF
>>> variable.
>>>
>>> The pci-keystone-dw.c instance was handled separately because it
>>> consists of two source files that need their own group of driver
>>> specific exports above and beyond the "shared" ones.
>>>
>>> Then we convert the Kconfig for all remaining at once; we could have
>>> done it on a per driver basis for ease of revert if anyone really
>>> objects, but since it would be a one line change, that seemed like
>>> not a real concern.
>>>
>>> Build testing was done on the linux-next tree for arm allmodconfig.
>>
>> I took these patches and gave a test with DRA7xx board. As expected there was
>> no issues when the driver was built-in. However when I tried to rmmod/modprobe
>> I got this error [2].
> 
> Thanks for testing this!
> 
>> [2] -> http://pastebin.ubuntu.com/15185894/
> 
> It looks like you are hitting the BUG_ON() in ioremap_pte_range()
> that checks if a virtual address already has a page table entry,
> which in turn is probably a result of dw_pcie_host_init()
> calling pci_remap_iospace() again for the same memory area
> it has called the last time, and no cleanup done inbetween.
> 
> Could you try adding a pci_unmap_iospace() and calling that
> in the device remove function? Let me know if you need help
> implementing it.

That didn't look straight forward to me :-( I'll try to see this next week. Any
help from you will make it simpler for me.

Thanks
Kishon

[PATCH 0/5] Modularize PCI_DW related drivers.

[Arnd Bergmann]

On Thursday 25 February 2016 13:43:48 Kishon Vijay Abraham I wrote:
> Hi Arnd,
> 
> On Wednesday 24 February 2016 02:34 PM, Arnd Bergmann wrote:
> > On Wednesday 24 February 2016 11:39:26 Kishon Vijay Abraham I wrote:
> >> Hi,
> >>
> >> On Monday 08 February 2016 05:30 AM, Paul Gortmaker wrote:
> >>> In a recent patch series that aimed to remove code related to module
> >>> unload for PCI support that was simply non modular, the discussion
> >>> led to people wanting to keep the code and push towards taking the
> >>> steps needed to support moving it towards tristate instead[1].
> >>>
> >>> Here, we take step one, which is simply making the Kconfig change
> >>> and then dealing with any build fallout or modpost fallout.  What
> >>> amounts to essentially a sanity build test.  To be clear, these
> >>> have not been runtime validated; that will need to be done by those
> >>> with access to real hardware.  However, the changes are not anything
> >>> that should disrupt any existing built-in validation, so real world
> >>> users should not be impacted by this change.
> >>>
> >>> We start with a smaller family of drivers; those that actively select
> >>> PCI_DW, as a nice self contained group to test the waters and see if
> >>> everyone is still good with this approach before investing more time
> >>> on a wider scale to other pci/host/ code blocks.
> >>>
> >>> As such the drivers here share a dependency on having the same group
> >>> of functions exported in order to successfully complete modpost.
> >>>
> >>> In addition, we have to stray outside drivers/pci to add exports
> >>> in two places; once for an ARM fault handler, and once for an OF
> >>> variable.
> >>>
> >>> The pci-keystone-dw.c instance was handled separately because it
> >>> consists of two source files that need their own group of driver
> >>> specific exports above and beyond the "shared" ones.
> >>>
> >>> Then we convert the Kconfig for all remaining at once; we could have
> >>> done it on a per driver basis for ease of revert if anyone really
> >>> objects, but since it would be a one line change, that seemed like
> >>> not a real concern.
> >>>
> >>> Build testing was done on the linux-next tree for arm allmodconfig.
> >>
> >> I took these patches and gave a test with DRA7xx board. As expected there was
> >> no issues when the driver was built-in. However when I tried to rmmod/modprobe
> >> I got this error [2].
> > 
> > Thanks for testing this!
> > 
> >> [2] -> http://pastebin.ubuntu.com/15185894/
> > 
> > It looks like you are hitting the BUG_ON() in ioremap_pte_range()
> > that checks if a virtual address already has a page table entry,
> > which in turn is probably a result of dw_pcie_host_init()
> > calling pci_remap_iospace() again for the same memory area
> > it has called the last time, and no cleanup done inbetween.
> > 
> > Could you try adding a pci_unmap_iospace() and calling that
> > in the device remove function? Let me know if you need help
> > implementing it.
> 
> That didn't look straight forward to me :-( I'll try to see this next week. Any
> help from you will make it simpler for me.

I tried writing the function now, and I think it's actually quite easy:

void pci_unmap_iospace(const struct resource *res)
{
#if defined(PCI_IOBASE) && defined(CONFIG_MMU)
        return iounmap(PCI_IOBASE + res->start);
#endif
}

You just need to pass the same resource in here htat you pass into
pci_remap_iospace().

	Arnd

[PATCH 0/5] Modularize PCI_DW related drivers.

[Kishon Vijay Abraham I]

Hi Arnd,

On Thursday 25 February 2016 02:05 PM, Arnd Bergmann wrote:
> On Thursday 25 February 2016 13:43:48 Kishon Vijay Abraham I wrote:
>> Hi Arnd,
>>
>> On Wednesday 24 February 2016 02:34 PM, Arnd Bergmann wrote:
>>> On Wednesday 24 February 2016 11:39:26 Kishon Vijay Abraham I wrote:
>>>> Hi,
>>>>
>>>> On Monday 08 February 2016 05:30 AM, Paul Gortmaker wrote:
>>>>> In a recent patch series that aimed to remove code related to module
>>>>> unload for PCI support that was simply non modular, the discussion
>>>>> led to people wanting to keep the code and push towards taking the
>>>>> steps needed to support moving it towards tristate instead[1].
>>>>>
>>>>> Here, we take step one, which is simply making the Kconfig change
>>>>> and then dealing with any build fallout or modpost fallout.  What
>>>>> amounts to essentially a sanity build test.  To be clear, these
>>>>> have not been runtime validated; that will need to be done by those
>>>>> with access to real hardware.  However, the changes are not anything
>>>>> that should disrupt any existing built-in validation, so real world
>>>>> users should not be impacted by this change.
>>>>>
>>>>> We start with a smaller family of drivers; those that actively select
>>>>> PCI_DW, as a nice self contained group to test the waters and see if
>>>>> everyone is still good with this approach before investing more time
>>>>> on a wider scale to other pci/host/ code blocks.
>>>>>
>>>>> As such the drivers here share a dependency on having the same group
>>>>> of functions exported in order to successfully complete modpost.
>>>>>
>>>>> In addition, we have to stray outside drivers/pci to add exports
>>>>> in two places; once for an ARM fault handler, and once for an OF
>>>>> variable.
>>>>>
>>>>> The pci-keystone-dw.c instance was handled separately because it
>>>>> consists of two source files that need their own group of driver
>>>>> specific exports above and beyond the "shared" ones.
>>>>>
>>>>> Then we convert the Kconfig for all remaining at once; we could have
>>>>> done it on a per driver basis for ease of revert if anyone really
>>>>> objects, but since it would be a one line change, that seemed like
>>>>> not a real concern.
>>>>>
>>>>> Build testing was done on the linux-next tree for arm allmodconfig.
>>>>
>>>> I took these patches and gave a test with DRA7xx board. As expected there was
>>>> no issues when the driver was built-in. However when I tried to rmmod/modprobe
>>>> I got this error [2].
>>>
>>> Thanks for testing this!
>>>
>>>> [2] -> http://pastebin.ubuntu.com/15185894/
>>>
>>> It looks like you are hitting the BUG_ON() in ioremap_pte_range()
>>> that checks if a virtual address already has a page table entry,
>>> which in turn is probably a result of dw_pcie_host_init()
>>> calling pci_remap_iospace() again for the same memory area
>>> it has called the last time, and no cleanup done inbetween.
>>>
>>> Could you try adding a pci_unmap_iospace() and calling that
>>> in the device remove function? Let me know if you need help
>>> implementing it.
>>
>> That didn't look straight forward to me :-( I'll try to see this next week. Any
>> help from you will make it simpler for me.
> 
> I tried writing the function now, and I think it's actually quite easy:
> 
> void pci_unmap_iospace(const struct resource *res)
> {
> #if defined(PCI_IOBASE) && defined(CONFIG_MMU)
>         return iounmap(PCI_IOBASE + res->start);
> #endif
> }
> 
> You just need to pass the same resource in here htat you pass into
> pci_remap_iospace().

I still seem to get the abort in ioremap_page_range().

Here's the patch I used [3] and here's the kernel log [4].

[3] -> http://pastebin.ubuntu.com/15241614/
[4] -> http://pastebin.ubuntu.com/15241637/

Thanks
Kishon

[PATCH 0/5] Modularize PCI_DW related drivers.

[Arnd Bergmann]

On Monday 29 February 2016 14:59:35 Kishon Vijay Abraham I wrote:
> > }
> > 
> > You just need to pass the same resource in here htat you pass into
> > pci_remap_iospace().
> 
> I still seem to get the abort in ioremap_page_range().
> 
> Here's the patch I used [3] and here's the kernel log [4].
> 
> [3] -> http://pastebin.ubuntu.com/15241614/
> [4] -> http://pastebin.ubuntu.com/15241637/
> 
> 

Sorry, I'm out of ideas here. The patch looks right to me, but the problem
looks unchanged.

	Arnd

[PATCH 0/5] Modularize PCI_DW related drivers.

[Murali Karicheri]

On 03/01/2016 04:35 PM, Arnd Bergmann wrote:
> On Monday 29 February 2016 14:59:35 Kishon Vijay Abraham I wrote:
>>> }
>>>
>>> You just need to pass the same resource in here htat you pass into
>>> pci_remap_iospace().
>>
>> I still seem to get the abort in ioremap_page_range().
>>
>> Here's the patch I used [3] and here's the kernel log [4].
>>
>> [3] -> http://pastebin.ubuntu.com/15241614/
>> [4] -> http://pastebin.ubuntu.com/15241637/
>>
>>
> 
> Sorry, I'm out of ideas here. The patch looks right to me, but the problem
> looks unchanged.
> 
> 	Arnd
> 

Was there any progress since this last response from Arnd? or is it a TBD?

-- 
Murali Karicheri
Linux Kernel, Keystone