[PATCH v2 0/8] MIPS: fixes for PCI legacy drivers (rt2880, rt3883)

[PATCH v2 0/8] MIPS: fixes for PCI legacy drivers (rt2880, rt3883)

[Ilya Lipnitskiy]

One major fix for rt2880-pci in the first patch - fixes breakage that
existed since v4.14.

Other more minor fixes, cleanups, and improvements that either free up
memory, make dmesg messages clearer, or remove redundant dmesg output.

v2:
- Do not use internal pci-rt2880 config read and write functions after
  the device has been registered with the PCI subsystem to avoid races.
  Use safe pci_bus_{read,write}_config_{d}word wrappers instead.

Ilya Lipnitskiy (8):
  MIPS: pci-rt2880: fix slot 0 configuration
  MIPS: pci-rt2880: remove unneeded locks
  MIPS: pci-rt3883: trivial: remove unused variable
  MIPS: pci-rt3883: more accurate DT error messages
  MIPS: pci-legacy: stop using of_pci_range_to_resource
  MIPS: pci-legacy: remove redundant info messages
  MIPS: pci-legacy: remove busn_resource field
  MIPS: pci-legacy: use generic pci_enable_resources

 arch/mips/include/asm/pci.h |  1 -
 arch/mips/pci/pci-legacy.c  | 57 ++++++-------------------------------
 arch/mips/pci/pci-rt2880.c  | 50 ++++++++++++++++----------------
 arch/mips/pci/pci-rt3883.c  | 10 ++-----
 4 files changed, 35 insertions(+), 83 deletions(-)

-- 
2.31.1

[PATCH v2 1/8] MIPS: pci-rt2880: fix slot 0 configuration

[Ilya Lipnitskiy]

pci_fixup_irqs() used to call pcibios_map_irq on every PCI device, which
for RT2880 included bus 0 slot 0. After pci_fixup_irqs() got removed,
only slots/funcs with devices attached would be called. While arguably
the right thing, that left no chance for this driver to ever initialize
slot 0, effectively bricking PCI and USB on RT2880 devices such as the
Belkin F5D8235-4 v1.

Slot 0 configuration needs to happen after PCI bus enumeration, but
before any device at slot 0x11 (func 0 or 1) is talked to. That was
determined empirically by testing on a Belkin F5D8235-4 v1 device. A
minimal BAR 0 config write followed by read, then setting slot 0
PCI_COMMAND to MASTER | IO | MEMORY is all that seems to be required for
proper functionality.

Tested by ensuring that full- and high-speed USB devices get enumerated
on the Belkin F5D8235-4 v1 (with an out of tree DTS file from OpenWrt).

Fixes: 04c81c7293df ("MIPS: PCI: Replace pci_fixup_irqs() call with host bridge IRQ mapping hooks")
Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy@gmail.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Tobias Wolf <dev-NTEO@vplace.de>
Cc: <stable@vger.kernel.org> # v4.14+
---
 arch/mips/pci/pci-rt2880.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/arch/mips/pci/pci-rt2880.c b/arch/mips/pci/pci-rt2880.c
index e1f12e398136..f1538d2be89e 100644
--- a/arch/mips/pci/pci-rt2880.c
+++ b/arch/mips/pci/pci-rt2880.c
@@ -180,7 +180,6 @@ static inline void rt2880_pci_write_u32(unsigned long reg, u32 val)
 
 int pcibios_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 {
-	u16 cmd;
 	int irq = -1;
 
 	if (dev->bus->number != 0)
@@ -188,8 +187,6 @@ int pcibios_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 
 	switch (PCI_SLOT(dev->devfn)) {
 	case 0x00:
-		rt2880_pci_write_u32(PCI_BASE_ADDRESS_0, 0x08000000);
-		(void) rt2880_pci_read_u32(PCI_BASE_ADDRESS_0);
 		break;
 	case 0x11:
 		irq = RT288X_CPU_IRQ_PCI;
@@ -201,16 +198,6 @@ int pcibios_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 		break;
 	}
 
-	pci_write_config_byte((struct pci_dev *) dev,
-		PCI_CACHE_LINE_SIZE, 0x14);
-	pci_write_config_byte((struct pci_dev *) dev, PCI_LATENCY_TIMER, 0xFF);
-	pci_read_config_word((struct pci_dev *) dev, PCI_COMMAND, &cmd);
-	cmd |= PCI_COMMAND_MASTER | PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
-		PCI_COMMAND_INVALIDATE | PCI_COMMAND_FAST_BACK |
-		PCI_COMMAND_SERR | PCI_COMMAND_WAIT | PCI_COMMAND_PARITY;
-	pci_write_config_word((struct pci_dev *) dev, PCI_COMMAND, cmd);
-	pci_write_config_byte((struct pci_dev *) dev, PCI_INTERRUPT_LINE,
-			      dev->irq);
 	return irq;
 }
 
@@ -251,6 +238,30 @@ static int rt288x_pci_probe(struct platform_device *pdev)
 
 int pcibios_plat_dev_init(struct pci_dev *dev)
 {
+	static bool slot0_init;
+
+	/*
+	 * Nobody seems to initialize slot 0, but this platform requires it, so
+	 * do it once when some other slot is being enabled. The PCI subsystem
+	 * should configure other slots properly, so no need to do anything
+	 * special for those.
+	 */
+	if (!slot0_init && dev->bus->number == 0) {
+		u16 cmd;
+		u32 bar0;
+
+		slot0_init = true;
+
+		pci_bus_write_config_dword(dev->bus, 0, PCI_BASE_ADDRESS_0,
+					   0x08000000);
+		pci_bus_read_config_dword(dev->bus, 0, PCI_BASE_ADDRESS_0,
+					  &bar0);
+
+		pci_bus_read_config_word(dev->bus, 0, PCI_COMMAND, &cmd);
+		cmd |= PCI_COMMAND_MASTER | PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
+		pci_bus_write_config_word(dev->bus, 0, PCI_COMMAND, cmd);
+	}
+
 	return 0;
 }
 
-- 
2.31.1

[PATCH v2 2/8] MIPS: pci-rt2880: remove unneeded locks

[Ilya Lipnitskiy]

Mirror pci-rt3883 fix from commit e5067c718b3a ("MIPS: pci-rt3883:
Remove odd locking in PCI config space access code"). pci-rt2880 shares
the driver layout with pci-rt3883 and the same reasons apply.

Caller (generic PCI code) already does proper locking, so no need to add
another one here. Local PCI read/write functions are never called
simultaneously, also they do not require synchronization with the PCI
controller ops, since they are used before the controller registration.

Suggested-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy@gmail.com>
---
 arch/mips/pci/pci-rt2880.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/arch/mips/pci/pci-rt2880.c b/arch/mips/pci/pci-rt2880.c
index f1538d2be89e..e9dd01431f21 100644
--- a/arch/mips/pci/pci-rt2880.c
+++ b/arch/mips/pci/pci-rt2880.c
@@ -41,7 +41,6 @@
 #define RT2880_PCI_REG_ARBCTL		0x80
 
 static void __iomem *rt2880_pci_base;
-static DEFINE_SPINLOCK(rt2880_pci_lock);
 
 static u32 rt2880_pci_reg_read(u32 reg)
 {
@@ -63,17 +62,14 @@ static inline u32 rt2880_pci_get_cfgaddr(unsigned int bus, unsigned int slot,
 static int rt2880_pci_config_read(struct pci_bus *bus, unsigned int devfn,
 				  int where, int size, u32 *val)
 {
-	unsigned long flags;
 	u32 address;
 	u32 data;
 
 	address = rt2880_pci_get_cfgaddr(bus->number, PCI_SLOT(devfn),
 					 PCI_FUNC(devfn), where);
 
-	spin_lock_irqsave(&rt2880_pci_lock, flags);
 	rt2880_pci_reg_write(address, RT2880_PCI_REG_CONFIG_ADDR);
 	data = rt2880_pci_reg_read(RT2880_PCI_REG_CONFIG_DATA);
-	spin_unlock_irqrestore(&rt2880_pci_lock, flags);
 
 	switch (size) {
 	case 1:
@@ -93,14 +89,12 @@ static int rt2880_pci_config_read(struct pci_bus *bus, unsigned int devfn,
 static int rt2880_pci_config_write(struct pci_bus *bus, unsigned int devfn,
 				   int where, int size, u32 val)
 {
-	unsigned long flags;
 	u32 address;
 	u32 data;
 
 	address = rt2880_pci_get_cfgaddr(bus->number, PCI_SLOT(devfn),
 					 PCI_FUNC(devfn), where);
 
-	spin_lock_irqsave(&rt2880_pci_lock, flags);
 	rt2880_pci_reg_write(address, RT2880_PCI_REG_CONFIG_ADDR);
 	data = rt2880_pci_reg_read(RT2880_PCI_REG_CONFIG_DATA);
 
@@ -119,7 +113,6 @@ static int rt2880_pci_config_write(struct pci_bus *bus, unsigned int devfn,
 	}
 
 	rt2880_pci_reg_write(data, RT2880_PCI_REG_CONFIG_DATA);
-	spin_unlock_irqrestore(&rt2880_pci_lock, flags);
 
 	return PCIBIOS_SUCCESSFUL;
 }
@@ -151,31 +144,25 @@ static struct pci_controller rt2880_pci_controller = {
 
 static inline u32 rt2880_pci_read_u32(unsigned long reg)
 {
-	unsigned long flags;
 	u32 address;
 	u32 ret;
 
 	address = rt2880_pci_get_cfgaddr(0, 0, 0, reg);
 
-	spin_lock_irqsave(&rt2880_pci_lock, flags);
 	rt2880_pci_reg_write(address, RT2880_PCI_REG_CONFIG_ADDR);
 	ret = rt2880_pci_reg_read(RT2880_PCI_REG_CONFIG_DATA);
-	spin_unlock_irqrestore(&rt2880_pci_lock, flags);
 
 	return ret;
 }
 
 static inline void rt2880_pci_write_u32(unsigned long reg, u32 val)
 {
-	unsigned long flags;
 	u32 address;
 
 	address = rt2880_pci_get_cfgaddr(0, 0, 0, reg);
 
-	spin_lock_irqsave(&rt2880_pci_lock, flags);
 	rt2880_pci_reg_write(address, RT2880_PCI_REG_CONFIG_ADDR);
 	rt2880_pci_reg_write(val, RT2880_PCI_REG_CONFIG_DATA);
-	spin_unlock_irqrestore(&rt2880_pci_lock, flags);
 }
 
 int pcibios_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
-- 
2.31.1

Re: [PATCH v2 2/8] MIPS: pci-rt2880: remove unneeded locks

[Sergey Ryazanov]

On Wed, Apr 14, 2021 at 6:12 AM Ilya Lipnitskiy
<ilya.lipnitskiy@gmail.com> wrote:
> Mirror pci-rt3883 fix from commit e5067c718b3a ("MIPS: pci-rt3883:
> Remove odd locking in PCI config space access code"). pci-rt2880 shares
> the driver layout with pci-rt3883 and the same reasons apply.
>
> Caller (generic PCI code) already does proper locking, so no need to add
> another one here. Local PCI read/write functions are never called
> simultaneously, also they do not require synchronization with the PCI
> controller ops, since they are used before the controller registration.
>
> Suggested-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy@gmail.com>

Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>

-- 
Sergey

[PATCH v2 3/8] MIPS: pci-rt3883: trivial: remove unused variable

[Ilya Lipnitskiy]

Fixes the following compiler warning:
  warning: unused variable 'flags' [-Wunused-variable]

Fixes: e5067c718b3a ("MIPS: pci-rt3883: Remove odd locking in PCI config space access code")
Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy@gmail.com>
Acked-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
Cc: trivial@kernel.org
---
 arch/mips/pci/pci-rt3883.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/mips/pci/pci-rt3883.c b/arch/mips/pci/pci-rt3883.c
index 0ac6346026d0..e422f78db5bc 100644
--- a/arch/mips/pci/pci-rt3883.c
+++ b/arch/mips/pci/pci-rt3883.c
@@ -100,7 +100,6 @@ static u32 rt3883_pci_read_cfg32(struct rt3883_pci_controller *rpc,
 			       unsigned bus, unsigned slot,
 			       unsigned func, unsigned reg)
 {
-	unsigned long flags;
 	u32 address;
 	u32 ret;
 
@@ -116,7 +115,6 @@ static void rt3883_pci_write_cfg32(struct rt3883_pci_controller *rpc,
 				 unsigned bus, unsigned slot,
 				 unsigned func, unsigned reg, u32 val)
 {
-	unsigned long flags;
 	u32 address;
 
 	address = rt3883_pci_get_cfgaddr(bus, slot, func, reg);
@@ -229,7 +227,6 @@ static int rt3883_pci_config_read(struct pci_bus *bus, unsigned int devfn,
 				  int where, int size, u32 *val)
 {
 	struct rt3883_pci_controller *rpc;
-	unsigned long flags;
 	u32 address;
 	u32 data;
 
@@ -263,7 +260,6 @@ static int rt3883_pci_config_write(struct pci_bus *bus, unsigned int devfn,
 				   int where, int size, u32 val)
 {
 	struct rt3883_pci_controller *rpc;
-	unsigned long flags;
 	u32 address;
 	u32 data;
 
-- 
2.31.1

[PATCH v2 4/8] MIPS: pci-rt3883: more accurate DT error messages

[Ilya Lipnitskiy]

Existing strings do not make sense: one is always NULL and the other
refers to the wrong parent node.

Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy@gmail.com>
---
 arch/mips/pci/pci-rt3883.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/mips/pci/pci-rt3883.c b/arch/mips/pci/pci-rt3883.c
index e422f78db5bc..aebd4964ea34 100644
--- a/arch/mips/pci/pci-rt3883.c
+++ b/arch/mips/pci/pci-rt3883.c
@@ -431,8 +431,7 @@ static int rt3883_pci_probe(struct platform_device *pdev)
 
 	if (!rpc->intc_of_node) {
 		dev_err(dev, "%pOF has no %s child node",
-			rpc->intc_of_node,
-			"interrupt controller");
+			np, "interrupt controller");
 		return -EINVAL;
 	}
 
@@ -446,8 +445,7 @@ static int rt3883_pci_probe(struct platform_device *pdev)
 
 	if (!rpc->pci_controller.of_node) {
 		dev_err(dev, "%pOF has no %s child node",
-			rpc->intc_of_node,
-			"PCI host bridge");
+			np, "PCI host bridge");
 		err = -EINVAL;
 		goto err_put_intc_node;
 	}
-- 
2.31.1

[PATCH v2 5/8] MIPS: pci-legacy: stop using of_pci_range_to_resource

[Ilya Lipnitskiy]

Mirror commit aeba3731b150 ("powerpc/pci: Fix IO space breakage after
of_pci_range_to_resource() change").

Most MIPS platforms do not define PCI_IOBASE, nor implement
pci_address_to_pio(). Moreover, IO_SPACE_LIMIT is 0xffff for most MIPS
platforms. of_pci_range_to_resource passes the _start address_ of the IO
range into pci_address_to_pio, which then checks it against
IO_SPACE_LIMIT and fails, because for MIPS platforms that use
pci-legacy (pci-lantiq, pci-rt3883, pci-mt7620), IO ranges start much
higher than 0xffff.

In fact, pci-mt7621 in staging already works around this problem, see
commit 09dd629eeabb ("staging: mt7621-pci: fix io space and properly set
resource limits")

So just stop using of_pci_range_to_resource, which does not work for
MIPS.

Fixes PCI errors like:
  pci_bus 0000:00: root bus resource [io  0xffffffff]

Fixes: 0b0b0893d49b ("of/pci: Fix the conversion of IO ranges into IO resources")
Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy@gmail.com>
Cc: Liviu Dudau <Liviu.Dudau@arm.com>
---
 arch/mips/pci/pci-legacy.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/mips/pci/pci-legacy.c b/arch/mips/pci/pci-legacy.c
index 39052de915f3..3a909194284a 100644
--- a/arch/mips/pci/pci-legacy.c
+++ b/arch/mips/pci/pci-legacy.c
@@ -166,8 +166,13 @@ void pci_load_of_ranges(struct pci_controller *hose, struct device_node *node)
 			res = hose->mem_resource;
 			break;
 		}
-		if (res != NULL)
-			of_pci_range_to_resource(&range, node, res);
+		if (res != NULL) {
+			res->name = node->full_name;
+			res->flags = range.flags;
+			res->start = range.cpu_addr;
+			res->end = range.cpu_addr + range.size - 1;
+			res->parent = res->child = res->sibling = NULL;
+		}
 	}
 }
 
-- 
2.31.1

Re: [PATCH v2 5/8] MIPS: pci-legacy: stop using of_pci_range_to_resource

[Liviu Dudau]

Hi Ilya,

On Tue, Apr 13, 2021 at 08:12:37PM -0700, Ilya Lipnitskiy wrote:
> Mirror commit aeba3731b150 ("powerpc/pci: Fix IO space breakage after
> of_pci_range_to_resource() change").
> 
> Most MIPS platforms do not define PCI_IOBASE, nor implement
> pci_address_to_pio(). Moreover, IO_SPACE_LIMIT is 0xffff for most MIPS
> platforms. of_pci_range_to_resource passes the _start address_ of the IO
> range into pci_address_to_pio, which then checks it against
> IO_SPACE_LIMIT and fails, because for MIPS platforms that use
> pci-legacy (pci-lantiq, pci-rt3883, pci-mt7620), IO ranges start much
> higher than 0xffff.

I think defining PCI_IOBASE and implementing pci_address_to_pio() would be
easier in the long run as you will then let the core PCI code do most of
the work, but given that this is legacy code I can understand that testing
it would not be simple.

> 
> In fact, pci-mt7621 in staging already works around this problem, see
> commit 09dd629eeabb ("staging: mt7621-pci: fix io space and properly set
> resource limits")
> 
> So just stop using of_pci_range_to_resource, which does not work for
> MIPS.
> 
> Fixes PCI errors like:
>   pci_bus 0000:00: root bus resource [io  0xffffffff]
> 
> Fixes: 0b0b0893d49b ("of/pci: Fix the conversion of IO ranges into IO resources")
> Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy@gmail.com>
> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
> ---
>  arch/mips/pci/pci-legacy.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/mips/pci/pci-legacy.c b/arch/mips/pci/pci-legacy.c
> index 39052de915f3..3a909194284a 100644
> --- a/arch/mips/pci/pci-legacy.c
> +++ b/arch/mips/pci/pci-legacy.c
> @@ -166,8 +166,13 @@ void pci_load_of_ranges(struct pci_controller *hose, struct device_node *node)
>  			res = hose->mem_resource;
>  			break;
>  		}
> -		if (res != NULL)
> -			of_pci_range_to_resource(&range, node, res);
> +		if (res != NULL) {
> +			res->name = node->full_name;
> +			res->flags = range.flags;
> +			res->start = range.cpu_addr;
> +			res->end = range.cpu_addr + range.size - 1;
> +			res->parent = res->child = res->sibling = NULL;
> +		}

Change looks reasonable to me.
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

>  	}
>  }
>  
> -- 
> 2.31.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

[PATCH v2 6/8] MIPS: pci-legacy: remove redundant info messages

[Ilya Lipnitskiy]

Remove the following pci-legacy message:
  PCI host bridge /pci@440000/host-bridge ranges:
   MEM 0x0000000020000000..0x000000002fffffff
    IO 0x0000000000460000..0x000000000046ffff

It is followed shortly by the same data from pci_register_host_bridge:
  PCI host bridge to bus 0000:00
  pci_bus 0000:00: root bus resource [mem 0x20000000-0x2fffffff]
  pci_bus 0000:00: root bus resource [io  0x460000-0x46ffff]

Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy@gmail.com>
---
 arch/mips/pci/pci-legacy.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/arch/mips/pci/pci-legacy.c b/arch/mips/pci/pci-legacy.c
index 3a909194284a..ec3f52ade72d 100644
--- a/arch/mips/pci/pci-legacy.c
+++ b/arch/mips/pci/pci-legacy.c
@@ -140,7 +140,6 @@ void pci_load_of_ranges(struct pci_controller *hose, struct device_node *node)
 	struct of_pci_range range;
 	struct of_pci_range_parser parser;
 
-	pr_info("PCI host bridge %pOF ranges:\n", node);
 	hose->of_node = node;
 
 	if (of_pci_range_parser_init(&parser, node))
@@ -151,18 +150,12 @@ void pci_load_of_ranges(struct pci_controller *hose, struct device_node *node)
 
 		switch (range.flags & IORESOURCE_TYPE_BITS) {
 		case IORESOURCE_IO:
-			pr_info("  IO 0x%016llx..0x%016llx\n",
-				range.cpu_addr,
-				range.cpu_addr + range.size - 1);
 			hose->io_map_base =
 				(unsigned long)ioremap(range.cpu_addr,
 						       range.size);
 			res = hose->io_resource;
 			break;
 		case IORESOURCE_MEM:
-			pr_info(" MEM 0x%016llx..0x%016llx\n",
-				range.cpu_addr,
-				range.cpu_addr + range.size - 1);
 			res = hose->mem_resource;
 			break;
 		}
-- 
2.31.1

[PATCH v2 7/8] MIPS: pci-legacy: remove busn_resource field

[Ilya Lipnitskiy]

No drivers set the busn_resource field in the pci_controller struct.
Commit 7ee214b540d9 ("MIPS: PCI: Remove unused busn_offset") almost
removed it over 3 years ago. Remove it for good to free up memory and
eliminate messages like:
  pci_bus 0000:00: root bus resource [??? 0x00000000 flags 0x0]

Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy@gmail.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 arch/mips/include/asm/pci.h | 1 -
 arch/mips/pci/pci-legacy.c  | 1 -
 2 files changed, 2 deletions(-)

diff --git a/arch/mips/include/asm/pci.h b/arch/mips/include/asm/pci.h
index 6f48649201c5..9ffc8192adae 100644
--- a/arch/mips/include/asm/pci.h
+++ b/arch/mips/include/asm/pci.h
@@ -38,7 +38,6 @@ struct pci_controller {
 	struct resource *io_resource;
 	unsigned long io_offset;
 	unsigned long io_map_base;
-	struct resource *busn_resource;
 
 #ifndef CONFIG_PCI_DOMAINS_GENERIC
 	unsigned int index;
diff --git a/arch/mips/pci/pci-legacy.c b/arch/mips/pci/pci-legacy.c
index ec3f52ade72d..78c22987bef0 100644
--- a/arch/mips/pci/pci-legacy.c
+++ b/arch/mips/pci/pci-legacy.c
@@ -89,7 +89,6 @@ static void pcibios_scanbus(struct pci_controller *hose)
 				hose->mem_resource, hose->mem_offset);
 	pci_add_resource_offset(&resources,
 				hose->io_resource, hose->io_offset);
-	pci_add_resource(&resources, hose->busn_resource);
 	list_splice_init(&resources, &bridge->windows);
 	bridge->dev.parent = NULL;
 	bridge->sysdata = hose;
-- 
2.31.1

[PATCH v2 8/8] MIPS: pci-legacy: use generic pci_enable_resources

[Ilya Lipnitskiy]

Follow the reasoning from commit 842de40d93e0 ("PCI: add generic
pci_enable_resources()"):

  The only functional difference from the MIPS version is that the
  generic one uses "!r->parent" to check for resource collisions
  instead of "!r->start && r->end".

That should have no effect on any pci-legacy driver.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy@gmail.com>
---
 arch/mips/pci/pci-legacy.c | 40 ++------------------------------------
 1 file changed, 2 insertions(+), 38 deletions(-)

diff --git a/arch/mips/pci/pci-legacy.c b/arch/mips/pci/pci-legacy.c
index 78c22987bef0..c24226ea0a6e 100644
--- a/arch/mips/pci/pci-legacy.c
+++ b/arch/mips/pci/pci-legacy.c
@@ -241,47 +241,11 @@ static int __init pcibios_init(void)
 
 subsys_initcall(pcibios_init);
 
-static int pcibios_enable_resources(struct pci_dev *dev, int mask)
-{
-	u16 cmd, old_cmd;
-	int idx;
-	struct resource *r;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	old_cmd = cmd;
-	for (idx=0; idx < PCI_NUM_RESOURCES; idx++) {
-		/* Only set up the requested stuff */
-		if (!(mask & (1<<idx)))
-			continue;
-
-		r = &dev->resource[idx];
-		if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
-			continue;
-		if ((idx == PCI_ROM_RESOURCE) &&
-				(!(r->flags & IORESOURCE_ROM_ENABLE)))
-			continue;
-		if (!r->start && r->end) {
-			pci_err(dev,
-				"can't enable device: resource collisions\n");
-			return -EINVAL;
-		}
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-	if (cmd != old_cmd) {
-		pci_info(dev, "enabling device (%04x -> %04x)\n", old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	return 0;
-}
-
 int pcibios_enable_device(struct pci_dev *dev, int mask)
 {
-	int err;
+	int err = pci_enable_resources(dev, mask);
 
-	if ((err = pcibios_enable_resources(dev, mask)) < 0)
+	if (err < 0)
 		return err;
 
 	return pcibios_plat_dev_init(dev);
-- 
2.31.1

Re: [PATCH v2 8/8] MIPS: pci-legacy: use generic pci_enable_resources

[Guenter Roeck]

On Tue, Apr 13, 2021 at 08:12:40PM -0700, Ilya Lipnitskiy wrote:
> Follow the reasoning from commit 842de40d93e0 ("PCI: add generic
> pci_enable_resources()"):
> 
>   The only functional difference from the MIPS version is that the
>   generic one uses "!r->parent" to check for resource collisions
>   instead of "!r->start && r->end".
> 
> That should have no effect on any pci-legacy driver.
> 

Unfortunately it does. With this patch in place, all my mips qemu emulations
fail to boot from ide/ata drive; the driver is not instantiated. The error
message is:

ata_piix 0000:00:0a.1: can't enable device: BAR 0 [io  0x01f0-0x01f7] not claimed
ata_piix: probe of 0000:00:0a.1 failed with error -22

Reverting this patch fixes the problem, and the message displayed by the driver
is:

ata_piix 0000:00:0a.1: enabling device (0000 -> 0001)

Bisect log is attached.

Guenter

---
# bad: [50b8b1d699ac313c0a07a3c185ffb23aecab8abb] Add linux-next specific files for 20210419
# good: [bf05bf16c76bb44ab5156223e1e58e26dfe30a88] Linux 5.12-rc8
git bisect start 'HEAD' 'v5.12-rc8'
# bad: [c4bb91fc07e59241cde97f913d7a2fbedc248f0d] Merge remote-tracking branch 'crypto/master'
git bisect bad c4bb91fc07e59241cde97f913d7a2fbedc248f0d
# bad: [499f739ad70f2a58aac985dceb25ca7666da88be] Merge remote-tracking branch 'jc_docs/docs-next'
git bisect bad 499f739ad70f2a58aac985dceb25ca7666da88be
# bad: [11b56408a328d1c5c4dfa7667c5dc46956b64aec] Merge remote-tracking branch 'parisc-hd/for-next'
git bisect bad 11b56408a328d1c5c4dfa7667c5dc46956b64aec
# good: [09ccc0ee1227f2cfe50d8dbbe241d115d9b3885f] Merge branch 'arm/defconfig' into for-next
git bisect good 09ccc0ee1227f2cfe50d8dbbe241d115d9b3885f
# good: [a5b76c2f17330e266a5c56dde21430e27b0d0dbb] Merge remote-tracking branch 'arm-soc/for-next'
git bisect good a5b76c2f17330e266a5c56dde21430e27b0d0dbb
# good: [1e4241f6813f1c1a0027d96df075ffd01808b3cf] Merge remote-tracking branch 'ti-k3/ti-k3-next'
git bisect good 1e4241f6813f1c1a0027d96df075ffd01808b3cf
# good: [7496a43be7a362391607d78e49a3f28de80029ce] Merge remote-tracking branch 'h8300/h8300-next'
git bisect good 7496a43be7a362391607d78e49a3f28de80029ce
# good: [66633abd0642f1e89d26e15f36fb13d3a1c535ff] MIPS/bpf: Enable bpf_probe_read{, str}() on MIPS again
git bisect good 66633abd0642f1e89d26e15f36fb13d3a1c535ff
# good: [2c92ef8ff8d327797c1920ae7f938bcc6f3f7421] MIPS: Fix strnlen_user access check
git bisect good 2c92ef8ff8d327797c1920ae7f938bcc6f3f7421
# good: [3a070801c61f4e3987d59b1068368ba71d727208] Merge remote-tracking branch 'microblaze/next'
git bisect good 3a070801c61f4e3987d59b1068368ba71d727208
# good: [317f553bb677e324c9c865ff7f14597bc5ceeb9c] MIPS: pci-legacy: remove redundant info messages
git bisect good 317f553bb677e324c9c865ff7f14597bc5ceeb9c
# bad: [6ce48897ce476bed86fde28752c27596e8753277] MIPS: Loongson64: Add kexec/kdump support
git bisect bad 6ce48897ce476bed86fde28752c27596e8753277
# bad: [99bca615d89510917864fac6b26fd343eff2aba2] MIPS: pci-legacy: use generic pci_enable_resources
git bisect bad 99bca615d89510917864fac6b26fd343eff2aba2
# good: [0af83d2e447af3e5098583cb6320bb1b1fb0976b] MIPS: pci-legacy: remove busn_resource field
git bisect good 0af83d2e447af3e5098583cb6320bb1b1fb0976b
# first bad commit: [99bca615d89510917864fac6b26fd343eff2aba2] MIPS: pci-legacy: use generic pci_enable_resources

[PATCH] MIPS: pci-legacy: revert "use generic pci_enable_resources"

[Ilya Lipnitskiy]

This mostly reverts commit 99bca615d895 ("MIPS: pci-legacy: use generic
pci_enable_resources"). Fixes regressions such as:
  ata_piix 0000:00:0a.1: can't enable device: BAR 0 [io  0x01f0-0x01f7] not
	claimed
  ata_piix: probe of 0000:00:0a.1 failed with error -22

The only changes from the strict revert are to fix checkpatch errors:
  ERROR: spaces required around that '=' (ctx:VxV)
  #33: FILE: arch/mips/pci/pci-legacy.c:252:
  +	for (idx=0; idx < PCI_NUM_RESOURCES; idx++) {
 	        ^

  ERROR: do not use assignment in if condition
  #67: FILE: arch/mips/pci/pci-legacy.c:284:
  +	if ((err = pcibios_enable_resources(dev, mask)) < 0)

Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy@gmail.com>
---
Guenter, sorry about this - let's just revert this for now to minimize
turmoil to the legacy PCI code. Obviously, this needs more testing
before applying. Thanks for your report!

 arch/mips/pci/pci-legacy.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/arch/mips/pci/pci-legacy.c b/arch/mips/pci/pci-legacy.c
index c24226ea0a6e..468722c8a5c6 100644
--- a/arch/mips/pci/pci-legacy.c
+++ b/arch/mips/pci/pci-legacy.c
@@ -241,9 +241,45 @@ static int __init pcibios_init(void)
 
 subsys_initcall(pcibios_init);
 
+static int pcibios_enable_resources(struct pci_dev *dev, int mask)
+{
+	u16 cmd, old_cmd;
+	int idx;
+	struct resource *r;
+
+	pci_read_config_word(dev, PCI_COMMAND, &cmd);
+	old_cmd = cmd;
+	for (idx = 0; idx < PCI_NUM_RESOURCES; idx++) {
+		/* Only set up the requested stuff */
+		if (!(mask & (1<<idx)))
+			continue;
+
+		r = &dev->resource[idx];
+		if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
+			continue;
+		if ((idx == PCI_ROM_RESOURCE) &&
+				(!(r->flags & IORESOURCE_ROM_ENABLE)))
+			continue;
+		if (!r->start && r->end) {
+			pci_err(dev,
+				"can't enable device: resource collisions\n");
+			return -EINVAL;
+		}
+		if (r->flags & IORESOURCE_IO)
+			cmd |= PCI_COMMAND_IO;
+		if (r->flags & IORESOURCE_MEM)
+			cmd |= PCI_COMMAND_MEMORY;
+	}
+	if (cmd != old_cmd) {
+		pci_info(dev, "enabling device (%04x -> %04x)\n", old_cmd, cmd);
+		pci_write_config_word(dev, PCI_COMMAND, cmd);
+	}
+	return 0;
+}
+
 int pcibios_enable_device(struct pci_dev *dev, int mask)
 {
-	int err = pci_enable_resources(dev, mask);
+	int err = pcibios_enable_resources(dev, mask);
 
 	if (err < 0)
 		return err;
-- 
2.31.1

Re: [PATCH] MIPS: pci-legacy: revert "use generic pci_enable_resources"

[Guenter Roeck]

On Mon, Apr 19, 2021 at 11:39:43PM -0700, Ilya Lipnitskiy wrote:
> This mostly reverts commit 99bca615d895 ("MIPS: pci-legacy: use generic
> pci_enable_resources"). Fixes regressions such as:
>   ata_piix 0000:00:0a.1: can't enable device: BAR 0 [io  0x01f0-0x01f7] not
> 	claimed
>   ata_piix: probe of 0000:00:0a.1 failed with error -22
> 
> The only changes from the strict revert are to fix checkpatch errors:
>   ERROR: spaces required around that '=' (ctx:VxV)
>   #33: FILE: arch/mips/pci/pci-legacy.c:252:
>   +	for (idx=0; idx < PCI_NUM_RESOURCES; idx++) {
>  	        ^
> 
>   ERROR: do not use assignment in if condition
>   #67: FILE: arch/mips/pci/pci-legacy.c:284:
>   +	if ((err = pcibios_enable_resources(dev, mask)) < 0)
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy@gmail.com>

Tested-by: Guenter Roeck <linux@roeck-us.net>

Guenter

Re: [PATCH] MIPS: pci-legacy: revert "use generic pci_enable_resources"

[Thomas Bogendoerfer]

On Mon, Apr 19, 2021 at 11:39:43PM -0700, Ilya Lipnitskiy wrote:
> This mostly reverts commit 99bca615d895 ("MIPS: pci-legacy: use generic
> pci_enable_resources"). Fixes regressions such as:
>   ata_piix 0000:00:0a.1: can't enable device: BAR 0 [io  0x01f0-0x01f7] not
> 	claimed
>   ata_piix: probe of 0000:00:0a.1 failed with error -22
> 
> The only changes from the strict revert are to fix checkpatch errors:
>   ERROR: spaces required around that '=' (ctx:VxV)
>   #33: FILE: arch/mips/pci/pci-legacy.c:252:
>   +	for (idx=0; idx < PCI_NUM_RESOURCES; idx++) {
>  	        ^
> 
>   ERROR: do not use assignment in if condition
>   #67: FILE: arch/mips/pci/pci-legacy.c:284:
>   +	if ((err = pcibios_enable_resources(dev, mask)) < 0)
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Ilya Lipnitskiy <ilya.lipnitskiy@gmail.com>
> ---
> Guenter, sorry about this - let's just revert this for now to minimize
> turmoil to the legacy PCI code. Obviously, this needs more testing
> before applying. Thanks for your report!
> 
>  arch/mips/pci/pci-legacy.c | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)

applied to mips-next.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v2 0/8] MIPS: fixes for PCI legacy drivers (rt2880, rt3883)

[Thomas Bogendoerfer]

On Tue, Apr 13, 2021 at 08:12:32PM -0700, Ilya Lipnitskiy wrote:
> One major fix for rt2880-pci in the first patch - fixes breakage that
> existed since v4.14.
> 
> Other more minor fixes, cleanups, and improvements that either free up
> memory, make dmesg messages clearer, or remove redundant dmesg output.
> 
> v2:
> - Do not use internal pci-rt2880 config read and write functions after
>   the device has been registered with the PCI subsystem to avoid races.
>   Use safe pci_bus_{read,write}_config_{d}word wrappers instead.
> 
> Ilya Lipnitskiy (8):
>   MIPS: pci-rt2880: fix slot 0 configuration
>   MIPS: pci-rt2880: remove unneeded locks
>   MIPS: pci-rt3883: trivial: remove unused variable
>   MIPS: pci-rt3883: more accurate DT error messages
>   MIPS: pci-legacy: stop using of_pci_range_to_resource
>   MIPS: pci-legacy: remove redundant info messages
>   MIPS: pci-legacy: remove busn_resource field
>   MIPS: pci-legacy: use generic pci_enable_resources
> 
>  arch/mips/include/asm/pci.h |  1 -
>  arch/mips/pci/pci-legacy.c  | 57 ++++++-------------------------------
>  arch/mips/pci/pci-rt2880.c  | 50 ++++++++++++++++----------------
>  arch/mips/pci/pci-rt3883.c  | 10 ++-----
>  4 files changed, 35 insertions(+), 83 deletions(-)

series applied to mips-next.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]