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

[PATCH 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.

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  | 63 +++++++++++++++++++------------------
 arch/mips/pci/pci-rt3883.c  | 10 ++----
 4 files changed, 44 insertions(+), 87 deletions(-)

-- 
2.31.1

[PATCH 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 | 50 +++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/arch/mips/pci/pci-rt2880.c b/arch/mips/pci/pci-rt2880.c
index e1f12e398136..19f7860fb28b 100644
--- a/arch/mips/pci/pci-rt2880.c
+++ b/arch/mips/pci/pci-rt2880.c
@@ -66,9 +66,13 @@ static int rt2880_pci_config_read(struct pci_bus *bus, unsigned int devfn,
 	unsigned long flags;
 	u32 address;
 	u32 data;
+	int busn = 0;
 
-	address = rt2880_pci_get_cfgaddr(bus->number, PCI_SLOT(devfn),
-					 PCI_FUNC(devfn), where);
+	if (bus)
+		busn = bus->number;
+
+	address = rt2880_pci_get_cfgaddr(busn, PCI_SLOT(devfn), PCI_FUNC(devfn),
+					 where);
 
 	spin_lock_irqsave(&rt2880_pci_lock, flags);
 	rt2880_pci_reg_write(address, RT2880_PCI_REG_CONFIG_ADDR);
@@ -96,9 +100,13 @@ static int rt2880_pci_config_write(struct pci_bus *bus, unsigned int devfn,
 	unsigned long flags;
 	u32 address;
 	u32 data;
+	int busn = 0;
+
+	if (bus)
+		busn = bus->number;
 
-	address = rt2880_pci_get_cfgaddr(bus->number, PCI_SLOT(devfn),
-					 PCI_FUNC(devfn), where);
+	address = rt2880_pci_get_cfgaddr(busn, PCI_SLOT(devfn), PCI_FUNC(devfn),
+					 where);
 
 	spin_lock_irqsave(&rt2880_pci_lock, flags);
 	rt2880_pci_reg_write(address, RT2880_PCI_REG_CONFIG_ADDR);
@@ -180,7 +188,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 +195,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 +206,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 +246,27 @@ 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) {
+		u32 cmd;
+
+		slot0_init = true;
+
+		rt2880_pci_write_u32(PCI_BASE_ADDRESS_0, 0x08000000);
+		(void) rt2880_pci_read_u32(PCI_BASE_ADDRESS_0);
+
+		rt2880_pci_config_read(NULL, 0, PCI_COMMAND, 2, &cmd);
+		cmd |= PCI_COMMAND_MASTER | PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
+		rt2880_pci_config_write(NULL, 0, PCI_COMMAND, 2, cmd);
+	}
+
 	return 0;
 }
 
-- 
2.31.1

[PATCH 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 19f7860fb28b..b4ee07cbcf2a 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,7 +62,6 @@ 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;
 	int busn = 0;
@@ -74,10 +72,8 @@ static int rt2880_pci_config_read(struct pci_bus *bus, unsigned int devfn,
 	address = rt2880_pci_get_cfgaddr(busn, 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:
@@ -97,7 +93,6 @@ 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;
 	int busn = 0;
@@ -108,7 +103,6 @@ static int rt2880_pci_config_write(struct pci_bus *bus, unsigned int devfn,
 	address = rt2880_pci_get_cfgaddr(busn, 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);
 
@@ -127,7 +121,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;
 }
@@ -159,31 +152,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 2/8] MIPS: pci-rt2880: remove unneeded locks

[Sergey Ryazanov]

Hello Ilya,

On Tue, Apr 13, 2021 at 9:22 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>
> ---
>  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 19f7860fb28b..b4ee07cbcf2a 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,7 +62,6 @@ 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;
>         int busn = 0;
> @@ -74,10 +72,8 @@ static int rt2880_pci_config_read(struct pci_bus *bus, unsigned int devfn,
>         address = rt2880_pci_get_cfgaddr(busn, 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:
> @@ -97,7 +93,6 @@ 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;
>         int busn = 0;
> @@ -108,7 +103,6 @@ static int rt2880_pci_config_write(struct pci_bus *bus, unsigned int devfn,
>         address = rt2880_pci_get_cfgaddr(busn, 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);
>
> @@ -127,7 +121,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;
>  }
> @@ -159,31 +152,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)

RT2880 PCI driver calls rt2880_pci_write_u32()/rt2880_pci_read_u32()
from pcibios_map_irq(), which is called outside the scope of the
pci_lock spinlock that is defined in /drivers/pci/access.c. So it
looks like this could lead to a race between
rt2880_pci_write_u32()/rt2880_pci_read_u32() and
rt2880_pci_config_read()/rt2880_pci_config_write() functions.

The code that uses rt2880_pci_write_u32()/rt2880_pci_read_u32() in the
pcibios_map_irq() duplicates a BAR initialization procedure, which is
already performed by the rt288x_pci_probe().

Maybe we should remove duplicated code in the pcibios_map_irq() to
reduce duplication and to avoid possible race in configuration space
access?

If you fix this possible race, feel free to add in the next patch version my
Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>

-- 
Sergey

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

[Sergey Ryazanov]

On Tue, Apr 13, 2021 at 4:28 PM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
> On Tue, Apr 13, 2021 at 9:22 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>
> > ---
> >  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 19f7860fb28b..b4ee07cbcf2a 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,7 +62,6 @@ 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;
> >         int busn = 0;
> > @@ -74,10 +72,8 @@ static int rt2880_pci_config_read(struct pci_bus *bus, unsigned int devfn,
> >         address = rt2880_pci_get_cfgaddr(busn, 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:
> > @@ -97,7 +93,6 @@ 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;
> >         int busn = 0;
> > @@ -108,7 +103,6 @@ static int rt2880_pci_config_write(struct pci_bus *bus, unsigned int devfn,
> >         address = rt2880_pci_get_cfgaddr(busn, 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);
> >
> > @@ -127,7 +121,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;
> >  }
> > @@ -159,31 +152,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)
>
> RT2880 PCI driver calls rt2880_pci_write_u32()/rt2880_pci_read_u32()
> from pcibios_map_irq(), which is called outside the scope of the
> pci_lock spinlock that is defined in /drivers/pci/access.c. So it
> looks like this could lead to a race between
> rt2880_pci_write_u32()/rt2880_pci_read_u32() and
> rt2880_pci_config_read()/rt2880_pci_config_write() functions.
>
> The code that uses rt2880_pci_write_u32()/rt2880_pci_read_u32() in the
> pcibios_map_irq() duplicates a BAR initialization procedure, which is
> already performed by the rt288x_pci_probe().
>
> Maybe we should remove duplicated code in the pcibios_map_irq() to
> reduce duplication and to avoid possible race in configuration space
> access?
>
> If you fix this possible race, feel free to add in the next patch version my
> Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>

Whoops, I just checked the whole series on the patchwork and realized
that you already removed this duplicated code in the first patch of
the series. In such a case this patch is Ok. Sorry for bothering.

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

-- 
Sergey

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

[Ilya Lipnitskiy]

Hi Sergey,

On Tue, Apr 13, 2021 at 6:40 AM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>
> On Tue, Apr 13, 2021 at 4:28 PM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
> > On Tue, Apr 13, 2021 at 9:22 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>
> > > ---
> > >  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 19f7860fb28b..b4ee07cbcf2a 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,7 +62,6 @@ 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;
> > >         int busn = 0;
> > > @@ -74,10 +72,8 @@ static int rt2880_pci_config_read(struct pci_bus *bus, unsigned int devfn,
> > >         address = rt2880_pci_get_cfgaddr(busn, 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:
> > > @@ -97,7 +93,6 @@ 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;
> > >         int busn = 0;
> > > @@ -108,7 +103,6 @@ static int rt2880_pci_config_write(struct pci_bus *bus, unsigned int devfn,
> > >         address = rt2880_pci_get_cfgaddr(busn, 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);
> > >
> > > @@ -127,7 +121,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;
> > >  }
> > > @@ -159,31 +152,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)
> >
> > RT2880 PCI driver calls rt2880_pci_write_u32()/rt2880_pci_read_u32()
> > from pcibios_map_irq(), which is called outside the scope of the
> > pci_lock spinlock that is defined in /drivers/pci/access.c. So it
> > looks like this could lead to a race between
> > rt2880_pci_write_u32()/rt2880_pci_read_u32() and
> > rt2880_pci_config_read()/rt2880_pci_config_write() functions.
> >
> > The code that uses rt2880_pci_write_u32()/rt2880_pci_read_u32() in the
> > pcibios_map_irq() duplicates a BAR initialization procedure, which is
> > already performed by the rt288x_pci_probe().
> >
> > Maybe we should remove duplicated code in the pcibios_map_irq() to
> > reduce duplication and to avoid possible race in configuration space
> > access?
> >
> > If you fix this possible race, feel free to add in the next patch version my
> > Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
>
> Whoops, I just checked the whole series on the patchwork and realized
> that you already removed this duplicated code in the first patch of
> the series. In such a case this patch is Ok. Sorry for bothering.
Thanks for your prompt review. I think you may still have a point,
since I do call rt2880_pci_{read,write}_u32 and
rt2880_pci_config_{read,write} from pcibios_plat_dev_init, which
happens after PCI bus enumeration. I think I can rework it to make it
cleaner and use pci_{read,write}_config_byte.

I will respin the series soon.

Ilya

[PATCH 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>
Cc: 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

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

[Sergey Ryazanov]

On Tue, Apr 13, 2021 at 9:22 AM Ilya Lipnitskiy
<ilya.lipnitskiy@gmail.com> wrote:
> 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>
> Cc: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> Cc: trivial@kernel.org

Yep, I overlooked these local variables. Thank you.

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

-- 
Sergey

[PATCH 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 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

[PATCH 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 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 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