linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* pci: Add generic pcibios_{fixup_bus,align_resource}
@ 2017-06-24  1:50 Palmer Dabbelt
  2017-06-24  1:50 ` [PATCH 1/3] pci: Add a generic, weakly-linked pcibios_fixup_bus Palmer Dabbelt
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Palmer Dabbelt @ 2017-06-24  1:50 UTC (permalink / raw)
  To: linux-pci, linux-kernel, bhelgaas, hch, Arnd Bergmann
  Cc: linux-snps-arc, linux-cris-kernel, linux-ia64, linux-s390,
	linux-sh, sparclinux

While upstreaming the RISC-V port, it was pointed out that multiple
architectures have copied the mostly empty versions of at least one of these
functions.  This defines weakly bound versions of the common functions and
removes the now obselete functions from other ports.

This has been split out from the RISC-V submission so we can decouple all these
generic changes from our port review process.  There's some discussion on an
earlier version of the patch here

  https://lkml.org/lkml/2017/6/6/998

but I'm afraid a lot of this is really out of my wheelhouse (and I'm pretty
slammed trying to get the RISC-V port in better shape), so I'm afraid I'm not
going to be able to perform the full cleanup suggested.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/3] pci: Add a generic, weakly-linked pcibios_fixup_bus
  2017-06-24  1:50 pci: Add generic pcibios_{fixup_bus,align_resource} Palmer Dabbelt
@ 2017-06-24  1:50 ` Palmer Dabbelt
  2017-06-24  9:34   ` Geert Uytterhoeven
  2017-06-24  1:50 ` [PATCH 2/3] pci: Add a generic, weakly-linked pcibios_align_resource Palmer Dabbelt
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Palmer Dabbelt @ 2017-06-24  1:50 UTC (permalink / raw)
  To: linux-pci, linux-kernel, bhelgaas, hch, Arnd Bergmann
  Cc: linux-snps-arc, linux-cris-kernel, linux-ia64, linux-s390,
	linux-sh, sparclinux, Palmer Dabbelt

Multiple architectures define this as an empty function, and I'm adding
another one as part of the RISC-V port.  This adds a __weak version of
pci_fixup_bios and deletes the now obselete ones in a handful of ports.

The only functional change should be that microblaze used to export
pcibios_fixup_bus.  None of the other architectures export this, so I
just dropped it.

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
---
 arch/arc/kernel/pcibios.c             |  4 ----
 arch/arm64/kernel/pci.c               |  8 --------
 arch/cris/arch-v32/drivers/pci/bios.c |  4 ----
 arch/microblaze/pci/pci-common.c      |  6 ------
 arch/s390/pci/pci.c                   |  4 ----
 arch/sh/drivers/pci/pci.c             |  8 --------
 arch/sparc/kernel/pci.c               |  4 ----
 arch/tile/kernel/pci.c                |  8 --------
 arch/tile/kernel/pci_gx.c             |  5 -----
 drivers/pci/probe.c                   | 10 ++++++++++
 10 files changed, 10 insertions(+), 51 deletions(-)

diff --git a/arch/arc/kernel/pcibios.c b/arch/arc/kernel/pcibios.c
index 72e1d73d0bd6..1c8df8fd5fed 100644
--- a/arch/arc/kernel/pcibios.c
+++ b/arch/arc/kernel/pcibios.c
@@ -16,7 +16,3 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 {
 	return res->start;
 }
-
-void pcibios_fixup_bus(struct pci_bus *bus)
-{
-}
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index c7e3e6387a49..4c7f451aca05 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -23,14 +23,6 @@
 #include <linux/slab.h>
 
 /*
- * Called after each bus is probed, but before its children are examined
- */
-void pcibios_fixup_bus(struct pci_bus *bus)
-{
-	/* nothing to do, expected to be removed in the future */
-}
-
-/*
  * We don't have to worry about legacy ISA devices, so nothing to do here
  */
 resource_size_t pcibios_align_resource(void *data, const struct resource *res,
diff --git a/arch/cris/arch-v32/drivers/pci/bios.c b/arch/cris/arch-v32/drivers/pci/bios.c
index 394c2a73d5e2..5cc622c0225e 100644
--- a/arch/cris/arch-v32/drivers/pci/bios.c
+++ b/arch/cris/arch-v32/drivers/pci/bios.c
@@ -2,10 +2,6 @@
 #include <linux/kernel.h>
 #include <hwregs/intr_vect.h>
 
-void pcibios_fixup_bus(struct pci_bus *b)
-{
-}
-
 void pcibios_set_master(struct pci_dev *dev)
 {
 	u8 lat;
diff --git a/arch/microblaze/pci/pci-common.c b/arch/microblaze/pci/pci-common.c
index 404fb38d06b7..940f266e5d5c 100644
--- a/arch/microblaze/pci/pci-common.c
+++ b/arch/microblaze/pci/pci-common.c
@@ -810,12 +810,6 @@ void pcibios_setup_bus_devices(struct pci_bus *bus)
 	}
 }
 
-void pcibios_fixup_bus(struct pci_bus *bus)
-{
-	/* nothing to do */
-}
-EXPORT_SYMBOL(pcibios_fixup_bus);
-
 /*
  * We need to avoid collisions with `mirrored' VGA ports
  * and other strange ISA hardware, so we always want the
diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index 8051df109db3..98a54dd63483 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -234,10 +234,6 @@ static int zpci_cfg_store(struct zpci_dev *zdev, int offset, u32 val, u8 len)
 	return rc;
 }
 
-void pcibios_fixup_bus(struct pci_bus *bus)
-{
-}
-
 resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 				       resource_size_t size,
 				       resource_size_t align)
diff --git a/arch/sh/drivers/pci/pci.c b/arch/sh/drivers/pci/pci.c
index c99ee286b69f..749642e1272e 100644
--- a/arch/sh/drivers/pci/pci.c
+++ b/arch/sh/drivers/pci/pci.c
@@ -155,14 +155,6 @@ static int __init pcibios_init(void)
 subsys_initcall(pcibios_init);
 
 /*
- *  Called after each bus is probed, but before its children
- *  are examined.
- */
-void pcibios_fixup_bus(struct pci_bus *bus)
-{
-}
-
-/*
  * We need to avoid collisions with `mirrored' VGA ports
  * and other strange ISA hardware, so we always want the
  * addresses to be allocated in the 0x000-0x0ff region
diff --git a/arch/sparc/kernel/pci.c b/arch/sparc/kernel/pci.c
index 7eceaa10836f..78d3dc25e126 100644
--- a/arch/sparc/kernel/pci.c
+++ b/arch/sparc/kernel/pci.c
@@ -690,10 +690,6 @@ struct pci_bus *pci_scan_one_pbm(struct pci_pbm_info *pbm,
 	return bus;
 }
 
-void pcibios_fixup_bus(struct pci_bus *pbus)
-{
-}
-
 resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 				resource_size_t size, resource_size_t align)
 {
diff --git a/arch/tile/kernel/pci.c b/arch/tile/kernel/pci.c
index bc6656b5708b..3113d4d5c329 100644
--- a/arch/tile/kernel/pci.c
+++ b/arch/tile/kernel/pci.c
@@ -369,14 +369,6 @@ int __init pcibios_init(void)
 }
 subsys_initcall(pcibios_init);
 
-/*
- * No bus fixups needed.
- */
-void pcibios_fixup_bus(struct pci_bus *bus)
-{
-	/* Nothing needs to be done. */
-}
-
 void pcibios_set_master(struct pci_dev *dev)
 {
 	/* No special bus mastering setup handling. */
diff --git a/arch/tile/kernel/pci_gx.c b/arch/tile/kernel/pci_gx.c
index b554a68eea1b..b89172b592cc 100644
--- a/arch/tile/kernel/pci_gx.c
+++ b/arch/tile/kernel/pci_gx.c
@@ -1038,11 +1038,6 @@ int __init pcibios_init(void)
 }
 subsys_initcall(pcibios_init);
 
-/* No bus fixups needed. */
-void pcibios_fixup_bus(struct pci_bus *bus)
-{
-}
-
 /* Process any "pci=" kernel boot arguments. */
 char *__init pcibios_setup(char *str)
 {
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 19c8950c6c38..1ff44dde7164 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2227,6 +2227,16 @@ void pcie_bus_configure_settings(struct pci_bus *bus)
 }
 EXPORT_SYMBOL_GPL(pcie_bus_configure_settings);
 
+/*
+ * Called after each bus is probed, but before its children are examined.  This
+ * is marked as __weak because multiple architectures define it
+ */
+__attribute__ ((weak))
+void pcibios_fixup_bus(struct pci_bus *bus)
+{
+       /* nothing to do, expected to be removed in the future */
+}
+
 unsigned int pci_scan_child_bus(struct pci_bus *bus)
 {
 	unsigned int devfn, pass, max = bus->busn_res.start;
-- 
2.13.0

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/3] pci: Add a generic, weakly-linked pcibios_align_resource
  2017-06-24  1:50 pci: Add generic pcibios_{fixup_bus,align_resource} Palmer Dabbelt
  2017-06-24  1:50 ` [PATCH 1/3] pci: Add a generic, weakly-linked pcibios_fixup_bus Palmer Dabbelt
@ 2017-06-24  1:50 ` Palmer Dabbelt
  2017-06-24  9:41   ` Geert Uytterhoeven
  2017-06-24  1:50 ` [PATCH 3/3] arc: kernel/pcibios.c is empty, delete it Palmer Dabbelt
  2017-08-02 20:00 ` pci: Add generic pcibios_{fixup_bus,align_resource} Bjorn Helgaas
  3 siblings, 1 reply; 9+ messages in thread
From: Palmer Dabbelt @ 2017-06-24  1:50 UTC (permalink / raw)
  To: linux-pci, linux-kernel, bhelgaas, hch, Arnd Bergmann
  Cc: linux-snps-arc, linux-cris-kernel, linux-ia64, linux-s390,
	linux-sh, sparclinux, Palmer Dabbelt

Multiple architectures define this as trivial function, and I'm adding
another one as part of the RISC-V port.  This adds a __weak version of
pcibios_align_resource and deletes the now obselete ones in a handful of
ports.

The only functional change should be that a handful of ports used to
export pcibios_fixup_bus.  Only some architectures export this, so I
just dropped it.

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
---
 arch/arc/kernel/pcibios.c        |  9 ---------
 arch/arm64/kernel/pci.c          |  9 ---------
 arch/ia64/pci/pci.c              |  7 -------
 arch/microblaze/pci/pci-common.c |  7 -------
 arch/sparc/kernel/leon_pci.c     |  6 ------
 arch/sparc/kernel/pci.c          |  6 ------
 arch/sparc/kernel/pcic.c         |  6 ------
 arch/tile/kernel/pci.c           | 10 ----------
 arch/tile/kernel/pci_gx.c        |  9 ---------
 drivers/pci/setup-res.c          | 12 ++++++++++++
 10 files changed, 12 insertions(+), 69 deletions(-)

diff --git a/arch/arc/kernel/pcibios.c b/arch/arc/kernel/pcibios.c
index 1c8df8fd5fed..05aba5a7b5d2 100644
--- a/arch/arc/kernel/pcibios.c
+++ b/arch/arc/kernel/pcibios.c
@@ -7,12 +7,3 @@
  */
 
 #include <linux/pci.h>
-
-/*
- * We don't have to worry about legacy ISA devices, so nothing to do here
- */
-resource_size_t pcibios_align_resource(void *data, const struct resource *res,
-				resource_size_t size, resource_size_t align)
-{
-	return res->start;
-}
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index 4c7f451aca05..9753ca23cfa1 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -23,15 +23,6 @@
 #include <linux/slab.h>
 
 /*
- * We don't have to worry about legacy ISA devices, so nothing to do here
- */
-resource_size_t pcibios_align_resource(void *data, const struct resource *res,
-				resource_size_t size, resource_size_t align)
-{
-	return res->start;
-}
-
-/*
  * Try to assign the IRQ number when probing a new device
  */
 int pcibios_alloc_irq(struct pci_dev *dev)
diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c
index 4068bde623dc..f5ec736100ee 100644
--- a/arch/ia64/pci/pci.c
+++ b/arch/ia64/pci/pci.c
@@ -411,13 +411,6 @@ pcibios_disable_device (struct pci_dev *dev)
 		acpi_pci_irq_disable(dev);
 }
 
-resource_size_t
-pcibios_align_resource (void *data, const struct resource *res,
-		        resource_size_t size, resource_size_t align)
-{
-	return res->start;
-}
-
 /**
  * ia64_pci_get_legacy_mem - generic legacy mem routine
  * @bus: bus to get legacy memory base address for
diff --git a/arch/microblaze/pci/pci-common.c b/arch/microblaze/pci/pci-common.c
index 940f266e5d5c..5835c09c6e26 100644
--- a/arch/microblaze/pci/pci-common.c
+++ b/arch/microblaze/pci/pci-common.c
@@ -823,13 +823,6 @@ void pcibios_setup_bus_devices(struct pci_bus *bus)
  * but we want to try to avoid allocating at 0x2900-0x2bff
  * which might have be mirrored at 0x0100-0x03ff..
  */
-resource_size_t pcibios_align_resource(void *data, const struct resource *res,
-				resource_size_t size, resource_size_t align)
-{
-	return res->start;
-}
-EXPORT_SYMBOL(pcibios_align_resource);
-
 int pcibios_add_device(struct pci_dev *dev)
 {
 	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
diff --git a/arch/sparc/kernel/leon_pci.c b/arch/sparc/kernel/leon_pci.c
index 4371f72ff025..0eafdf3d036d 100644
--- a/arch/sparc/kernel/leon_pci.c
+++ b/arch/sparc/kernel/leon_pci.c
@@ -94,9 +94,3 @@ void pcibios_fixup_bus(struct pci_bus *pbus)
 		}
 	}
 }
-
-resource_size_t pcibios_align_resource(void *data, const struct resource *res,
-				resource_size_t size, resource_size_t align)
-{
-	return res->start;
-}
diff --git a/arch/sparc/kernel/pci.c b/arch/sparc/kernel/pci.c
index 78d3dc25e126..3f8670c92951 100644
--- a/arch/sparc/kernel/pci.c
+++ b/arch/sparc/kernel/pci.c
@@ -690,12 +690,6 @@ struct pci_bus *pci_scan_one_pbm(struct pci_pbm_info *pbm,
 	return bus;
 }
 
-resource_size_t pcibios_align_resource(void *data, const struct resource *res,
-				resource_size_t size, resource_size_t align)
-{
-	return res->start;
-}
-
 int pcibios_enable_device(struct pci_dev *dev, int mask)
 {
 	u16 cmd, oldcmd;
diff --git a/arch/sparc/kernel/pcic.c b/arch/sparc/kernel/pcic.c
index a38787b84322..e038e343f2c1 100644
--- a/arch/sparc/kernel/pcic.c
+++ b/arch/sparc/kernel/pcic.c
@@ -746,12 +746,6 @@ static void watchdog_reset() {
 }
 #endif
 
-resource_size_t pcibios_align_resource(void *data, const struct resource *res,
-				resource_size_t size, resource_size_t align)
-{
-	return res->start;
-}
-
 int pcibios_enable_device(struct pci_dev *pdev, int mask)
 {
 	return 0;
diff --git a/arch/tile/kernel/pci.c b/arch/tile/kernel/pci.c
index 3113d4d5c329..8999a20ed9d1 100644
--- a/arch/tile/kernel/pci.c
+++ b/arch/tile/kernel/pci.c
@@ -67,16 +67,6 @@ static struct pci_ops tile_cfg_ops;
 
 
 /*
- * We don't need to worry about the alignment of resources.
- */
-resource_size_t pcibios_align_resource(void *data, const struct resource *res,
-			    resource_size_t size, resource_size_t align)
-{
-	return res->start;
-}
-EXPORT_SYMBOL(pcibios_align_resource);
-
-/*
  * Open a FD to the hypervisor PCI device.
  *
  * controller_id is the controller number, config type is 0 or 1 for
diff --git a/arch/tile/kernel/pci_gx.c b/arch/tile/kernel/pci_gx.c
index b89172b592cc..0a7642184a9a 100644
--- a/arch/tile/kernel/pci_gx.c
+++ b/arch/tile/kernel/pci_gx.c
@@ -108,15 +108,6 @@ static struct pci_ops tile_cfg_ops;
 /* Mask of CPUs that should receive PCIe interrupts. */
 static struct cpumask intr_cpus_map;
 
-/* We don't need to worry about the alignment of resources. */
-resource_size_t pcibios_align_resource(void *data, const struct resource *res,
-				       resource_size_t size,
-				       resource_size_t align)
-{
-	return res->start;
-}
-EXPORT_SYMBOL(pcibios_align_resource);
-
 /*
  * Pick a CPU to receive and handle the PCIe interrupts, based on the IRQ #.
  * For now, we simply send interrupts to non-dataplane CPUs.
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 85774b7a316a..597ed1f8b15c 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -234,6 +234,18 @@ static int pci_revert_fw_address(struct resource *res, struct pci_dev *dev,
 	return 0;
 }
 
+/*
+ * We don't have to worry about legacy ISA devices, so nothing to do here.
+ * This is marked as __weak because multiple architectures define it, it should
+ * eventually go away.
+ */
+__attribute__ ((weak))
+resource_size_t pcibios_align_resource(void *data, const struct resource *res,
+                                      resource_size_t size, resource_size_t align)
+{
+       return res->start;
+}
+
 static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 		int resno, resource_size_t size, resource_size_t align)
 {
-- 
2.13.0

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/3] arc: kernel/pcibios.c is empty, delete it
  2017-06-24  1:50 pci: Add generic pcibios_{fixup_bus,align_resource} Palmer Dabbelt
  2017-06-24  1:50 ` [PATCH 1/3] pci: Add a generic, weakly-linked pcibios_fixup_bus Palmer Dabbelt
  2017-06-24  1:50 ` [PATCH 2/3] pci: Add a generic, weakly-linked pcibios_align_resource Palmer Dabbelt
@ 2017-06-24  1:50 ` Palmer Dabbelt
  2017-08-02 20:00 ` pci: Add generic pcibios_{fixup_bus,align_resource} Bjorn Helgaas
  3 siblings, 0 replies; 9+ messages in thread
From: Palmer Dabbelt @ 2017-06-24  1:50 UTC (permalink / raw)
  To: linux-pci, linux-kernel, bhelgaas, hch, Arnd Bergmann
  Cc: linux-snps-arc, linux-cris-kernel, linux-ia64, linux-s390,
	linux-sh, sparclinux, Palmer Dabbelt

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
---
 arch/arc/kernel/Makefile  | 1 -
 arch/arc/kernel/pcibios.c | 9 ---------
 2 files changed, 10 deletions(-)
 delete mode 100644 arch/arc/kernel/pcibios.c

diff --git a/arch/arc/kernel/Makefile b/arch/arc/kernel/Makefile
index 8942c5c3b4c5..2dc5f4296d44 100644
--- a/arch/arc/kernel/Makefile
+++ b/arch/arc/kernel/Makefile
@@ -12,7 +12,6 @@ obj-y	:= arcksyms.o setup.o irq.o reset.o ptrace.o process.o devtree.o
 obj-y	+= signal.o traps.o sys.o troubleshoot.o stacktrace.o disasm.o
 obj-$(CONFIG_ISA_ARCOMPACT)		+= entry-compact.o intc-compact.o
 obj-$(CONFIG_ISA_ARCV2)			+= entry-arcv2.o intc-arcv2.o
-obj-$(CONFIG_PCI)  			+= pcibios.o
 
 obj-$(CONFIG_MODULES)			+= arcksyms.o module.o
 obj-$(CONFIG_SMP) 			+= smp.o
diff --git a/arch/arc/kernel/pcibios.c b/arch/arc/kernel/pcibios.c
deleted file mode 100644
index 05aba5a7b5d2..000000000000
--- a/arch/arc/kernel/pcibios.c
+++ /dev/null
@@ -1,9 +0,0 @@
-/*
- * Copyright (C) 2014-2015 Synopsys, Inc. (www.synopsys.com)
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-
-#include <linux/pci.h>
-- 
2.13.0

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] pci: Add a generic, weakly-linked pcibios_fixup_bus
  2017-06-24  1:50 ` [PATCH 1/3] pci: Add a generic, weakly-linked pcibios_fixup_bus Palmer Dabbelt
@ 2017-06-24  9:34   ` Geert Uytterhoeven
  2017-06-24 21:32     ` Palmer Dabbelt
  0 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2017-06-24  9:34 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: linux-pci, linux-kernel, Bjorn Helgaas, Christoph Hellwig,
	Arnd Bergmann, arcml, Cris, linux-ia64, linux-s390,
	Linux-sh list, sparclinux

Hi Palmer,

On Sat, Jun 24, 2017 at 3:50 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> Multiple architectures define this as an empty function, and I'm adding
> another one as part of the RISC-V port.  This adds a __weak version of
> pci_fixup_bios and deletes the now obselete ones in a handful of ports.
>
> The only functional change should be that microblaze used to export
> pcibios_fixup_bus.  None of the other architectures export this, so I
> just dropped it.
>
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>

Given this is an empty function, wouldn't it make more sense to have
a static inline in asm-generic, protected by #ifndef pcibios_fixup_bus?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] pci: Add a generic, weakly-linked pcibios_align_resource
  2017-06-24  1:50 ` [PATCH 2/3] pci: Add a generic, weakly-linked pcibios_align_resource Palmer Dabbelt
@ 2017-06-24  9:41   ` Geert Uytterhoeven
  2017-06-24 21:32     ` Palmer Dabbelt
  0 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2017-06-24  9:41 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: linux-pci, linux-kernel, Bjorn Helgaas, Christoph Hellwig,
	Arnd Bergmann, arcml, Cris, linux-ia64, linux-s390,
	Linux-sh list, sparclinux

Hi Palmer,

On Sat, Jun 24, 2017 at 3:50 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> Multiple architectures define this as trivial function, and I'm adding
> another one as part of the RISC-V port.  This adds a __weak version of
> pcibios_align_resource and deletes the now obselete ones in a handful of
> ports.
>
> The only functional change should be that a handful of ports used to
> export pcibios_fixup_bus.  Only some architectures export this, so I
> just dropped it.
>
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>

This function is only ever used as a pointer passed to
pci_bus_alloc_resource()?

What about having

    #ifndef pcibios_fixup_bus
    #define pcibios_fixup_bus NULL
    #endif

in asm-generic/pci.h, letting the architecture with a non-trivial
implementation predefine the preprocessor symbol, and teaching
pci_bus_alloc_resource() to handle NULL?

[...]

Oh, the latter eventually calls into allocate_resource(), which already falls
back to simple_align_resource() if the alignment function is NULL, which
does the same thing.
So NULL should already work.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] pci: Add a generic, weakly-linked pcibios_fixup_bus
  2017-06-24  9:34   ` Geert Uytterhoeven
@ 2017-06-24 21:32     ` Palmer Dabbelt
  0 siblings, 0 replies; 9+ messages in thread
From: Palmer Dabbelt @ 2017-06-24 21:32 UTC (permalink / raw)
  To: geert
  Cc: linux-pci, linux-kernel, bhelgaas, hch, Arnd Bergmann,
	linux-snps-arc, linux-cris-kernel, linux-ia64, linux-s390,
	linux-sh, sparclinux

On Sat, 24 Jun 2017 02:34:06 PDT (-0700), geert@linux-m68k.org wrote:
> Hi Palmer,
>
> On Sat, Jun 24, 2017 at 3:50 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> Multiple architectures define this as an empty function, and I'm adding
>> another one as part of the RISC-V port.  This adds a __weak version of
>> pci_fixup_bios and deletes the now obselete ones in a handful of ports.
>>
>> The only functional change should be that microblaze used to export
>> pcibios_fixup_bus.  None of the other architectures export this, so I
>> just dropped it.
>>
>> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
>
> Given this is an empty function, wouldn't it make more sense to have
> a static inline in asm-generic, protected by #ifndef pcibios_fixup_bus?

I think the PCI people were considering changing this from a per-arch function
to a per-controller function, so I think the inline won't help any there.  I
think since they hope to eventually clean up all the __weak functions it fits a
bit better this way, but I'm really fine with anything here.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] pci: Add a generic, weakly-linked pcibios_align_resource
  2017-06-24  9:41   ` Geert Uytterhoeven
@ 2017-06-24 21:32     ` Palmer Dabbelt
  0 siblings, 0 replies; 9+ messages in thread
From: Palmer Dabbelt @ 2017-06-24 21:32 UTC (permalink / raw)
  To: geert
  Cc: linux-pci, linux-kernel, bhelgaas, hch, Arnd Bergmann,
	linux-snps-arc, linux-cris-kernel, linux-ia64, linux-s390,
	linux-sh, sparclinux

On Sat, 24 Jun 2017 02:41:42 PDT (-0700), geert@linux-m68k.org wrote:
> Hi Palmer,
>
> On Sat, Jun 24, 2017 at 3:50 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> Multiple architectures define this as trivial function, and I'm adding
>> another one as part of the RISC-V port.  This adds a __weak version of
>> pcibios_align_resource and deletes the now obselete ones in a handful of
>> ports.
>>
>> The only functional change should be that a handful of ports used to
>> export pcibios_fixup_bus.  Only some architectures export this, so I
>> just dropped it.
>>
>> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
>
> This function is only ever used as a pointer passed to
> pci_bus_alloc_resource()?
>
> What about having
>
>     #ifndef pcibios_fixup_bus
>     #define pcibios_fixup_bus NULL
>     #endif
>
> in asm-generic/pci.h, letting the architecture with a non-trivial
> implementation predefine the preprocessor symbol, and teaching
> pci_bus_alloc_resource() to handle NULL?
>
> [...]
>
> Oh, the latter eventually calls into allocate_resource(), which already falls
> back to simple_align_resource() if the alignment function is NULL, which
> does the same thing.
> So NULL should already work.

I'm OK with that, but like your comments on the last one I think it might fit
better as a __weak function.  I think there were also some questions as to
whether these should actually be empty functions or not.  I'm really happy with
either way.

Since this is all out of my wheelhouse, can one of the PCI maintainers chime in
as to what I should do here?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: pci: Add generic pcibios_{fixup_bus,align_resource}
  2017-06-24  1:50 pci: Add generic pcibios_{fixup_bus,align_resource} Palmer Dabbelt
                   ` (2 preceding siblings ...)
  2017-06-24  1:50 ` [PATCH 3/3] arc: kernel/pcibios.c is empty, delete it Palmer Dabbelt
@ 2017-08-02 20:00 ` Bjorn Helgaas
  3 siblings, 0 replies; 9+ messages in thread
From: Bjorn Helgaas @ 2017-08-02 20:00 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: linux-pci, linux-kernel, bhelgaas, hch, Arnd Bergmann,
	linux-snps-arc, linux-cris-kernel, linux-ia64, linux-s390,
	linux-sh, sparclinux

On Fri, Jun 23, 2017 at 06:50:41PM -0700, Palmer Dabbelt wrote:
> While upstreaming the RISC-V port, it was pointed out that multiple
> architectures have copied the mostly empty versions of at least one of these
> functions.  This defines weakly bound versions of the common functions and
> removes the now obselete functions from other ports.
> 
> This has been split out from the RISC-V submission so we can decouple all these
> generic changes from our port review process.  There's some discussion on an
> earlier version of the patch here
> 
>   https://lkml.org/lkml/2017/6/6/998
> 
> but I'm afraid a lot of this is really out of my wheelhouse (and I'm pretty
> slammed trying to get the RISC-V port in better shape), so I'm afraid I'm not
> going to be able to perform the full cleanup suggested.

Applied to pci/resource for v4.14, thanks!

There's room for future improvement, e.g., using per-host callbacks,
etc., but I think this is a clear improvement as-is.

There's some folklore about the evils of weak symbols, but I don't
know the details.  It would be useful to have them codified somewhere
in Documentation/ or near the __weak definition.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-08-02 20:00 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-24  1:50 pci: Add generic pcibios_{fixup_bus,align_resource} Palmer Dabbelt
2017-06-24  1:50 ` [PATCH 1/3] pci: Add a generic, weakly-linked pcibios_fixup_bus Palmer Dabbelt
2017-06-24  9:34   ` Geert Uytterhoeven
2017-06-24 21:32     ` Palmer Dabbelt
2017-06-24  1:50 ` [PATCH 2/3] pci: Add a generic, weakly-linked pcibios_align_resource Palmer Dabbelt
2017-06-24  9:41   ` Geert Uytterhoeven
2017-06-24 21:32     ` Palmer Dabbelt
2017-06-24  1:50 ` [PATCH 3/3] arc: kernel/pcibios.c is empty, delete it Palmer Dabbelt
2017-08-02 20:00 ` pci: Add generic pcibios_{fixup_bus,align_resource} Bjorn Helgaas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).