linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff
@ 2022-05-11 20:18 Bjorn Helgaas
  2022-05-11 20:18 ` [PATCH 1/4] Revert "PCI: brcmstb: Do not turn off WOL regulators on suspend" Bjorn Helgaas
                   ` (4 more replies)
  0 siblings, 5 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2022-05-11 20:18 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: Lorenzo Pieralisi, Cyril Brulebois, Nicolas Saenz Julienne,
	Florian Fainelli, bcm-kernel-feedback-list, linux-pci,
	regressions, Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

Cyril reported that 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup()
into two funcs"), which appeared in v5.17-rc1, broke booting on the
Raspberry Pi Compute Module 4.  Revert 830aa6f29f07 and subsequent patches
for now.

Bjorn Helgaas (4):
  Revert "PCI: brcmstb: Do not turn off WOL regulators on suspend"
  Revert "PCI: brcmstb: Add control of subdevice voltage regulators"
  Revert "PCI: brcmstb: Add mechanism to turn on subdev regulators"
  Revert "PCI: brcmstb: Split brcm_pcie_setup() into two funcs"

 drivers/pci/controller/pcie-brcmstb.c | 257 +++-----------------------
 1 file changed, 30 insertions(+), 227 deletions(-)

-- 
2.25.1


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

* [PATCH 1/4] Revert "PCI: brcmstb: Do not turn off WOL regulators on suspend"
  2022-05-11 20:18 [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff Bjorn Helgaas
@ 2022-05-11 20:18 ` Bjorn Helgaas
  2022-05-12  6:24   ` Thorsten Leemhuis
  2022-05-11 20:18 ` [PATCH 2/4] Revert "PCI: brcmstb: Add control of subdevice voltage regulators" Bjorn Helgaas
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 18+ messages in thread
From: Bjorn Helgaas @ 2022-05-11 20:18 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: Lorenzo Pieralisi, Cyril Brulebois, Nicolas Saenz Julienne,
	Florian Fainelli, bcm-kernel-feedback-list, linux-pci,
	regressions, Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

This reverts commit 11ed8b8624b8085f706864b4addcd304b1e4fc38.

This is part of a revert of the following commits:

  11ed8b8624b8 ("PCI: brcmstb: Do not turn off WOL regulators on suspend")
  93e41f3fca3d ("PCI: brcmstb: Add control of subdevice voltage regulators")
  67211aadcb4b ("PCI: brcmstb: Add mechanism to turn on subdev regulators")
  830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup() into two funcs")

Cyril reported that 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup()
into two funcs"), which appeared in v5.17-rc1, broke booting on the
Raspberry Pi Compute Module 4.  Apparently 830aa6f29f07 panics with an
Asynchronous SError Interrupt, and after further commits here is a black
screen on HDMI and no output on the serial console.

This does not seem to affect the Raspberry Pi 4 B.

Reported-by: Cyril Brulebois <kibi@debian.org>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=215925
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 53 +++++----------------------
 1 file changed, 9 insertions(+), 44 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 375c0c40bbf8..3edd63735948 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -333,7 +333,6 @@ struct brcm_pcie {
 	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 	bool			refusal_mode;
 	struct subdev_regulators *sr;
-	bool			ep_wakeup_capable;
 };
 
 static inline bool is_bmips(const struct brcm_pcie *pcie)
@@ -1351,21 +1350,9 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
 	pcie->bridge_sw_init_set(pcie, 1);
 }
 
-static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
-{
-	bool *ret = data;
-
-	if (device_may_wakeup(&dev->dev)) {
-		*ret = true;
-		dev_info(&dev->dev, "disable cancelled for wake-up device\n");
-	}
-	return (int) *ret;
-}
-
 static int brcm_pcie_suspend(struct device *dev)
 {
 	struct brcm_pcie *pcie = dev_get_drvdata(dev);
-	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
 	int ret;
 
 	brcm_pcie_turn_off(pcie);
@@ -1384,22 +1371,11 @@ static int brcm_pcie_suspend(struct device *dev)
 	}
 
 	if (pcie->sr) {
-		/*
-		 * Now turn off the regulators, but if at least one
-		 * downstream device is enabled as a wake-up source, do not
-		 * turn off regulators.
-		 */
-		pcie->ep_wakeup_capable = false;
-		pci_walk_bus(bridge->bus, pci_dev_may_wakeup,
-			     &pcie->ep_wakeup_capable);
-		if (!pcie->ep_wakeup_capable) {
-			ret = regulator_bulk_disable(pcie->sr->num_supplies,
-						     pcie->sr->supplies);
-			if (ret) {
-				dev_err(dev, "Could not turn off regulators\n");
-				reset_control_reset(pcie->rescal);
-				return ret;
-			}
+		ret = regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies);
+		if (ret) {
+			dev_err(dev, "Could not turn off regulators\n");
+			reset_control_reset(pcie->rescal);
+			return ret;
 		}
 	}
 	clk_disable_unprepare(pcie->clk);
@@ -1420,21 +1396,10 @@ static int brcm_pcie_resume(struct device *dev)
 		return ret;
 
 	if (pcie->sr) {
-		if (pcie->ep_wakeup_capable) {
-			/*
-			 * We are resuming from a suspend.  In the suspend we
-			 * did not disable the power supplies, so there is
-			 * no need to enable them (and falsely increase their
-			 * usage count).
-			 */
-			pcie->ep_wakeup_capable = false;
-		} else {
-			ret = regulator_bulk_enable(pcie->sr->num_supplies,
-						    pcie->sr->supplies);
-			if (ret) {
-				dev_err(dev, "Could not turn on regulators\n");
-				goto err_disable_clk;
-			}
+		ret = regulator_bulk_enable(pcie->sr->num_supplies, pcie->sr->supplies);
+		if (ret) {
+			dev_err(dev, "Could not turn on regulators\n");
+			goto err_disable_clk;
 		}
 	}
 
-- 
2.25.1


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

* [PATCH 2/4] Revert "PCI: brcmstb: Add control of subdevice voltage regulators"
  2022-05-11 20:18 [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff Bjorn Helgaas
  2022-05-11 20:18 ` [PATCH 1/4] Revert "PCI: brcmstb: Do not turn off WOL regulators on suspend" Bjorn Helgaas
@ 2022-05-11 20:18 ` Bjorn Helgaas
  2022-05-11 20:18 ` [PATCH 3/4] Revert "PCI: brcmstb: Add mechanism to turn on subdev regulators" Bjorn Helgaas
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2022-05-11 20:18 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: Lorenzo Pieralisi, Cyril Brulebois, Nicolas Saenz Julienne,
	Florian Fainelli, bcm-kernel-feedback-list, linux-pci,
	regressions, Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

This reverts commit 93e41f3fca3d4a0f927b784012338c37f80a8a80.

This is part of a revert of the following commits:

  11ed8b8624b8 ("PCI: brcmstb: Do not turn off WOL regulators on suspend")
  93e41f3fca3d ("PCI: brcmstb: Add control of subdevice voltage regulators")
  67211aadcb4b ("PCI: brcmstb: Add mechanism to turn on subdev regulators")
  830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup() into two funcs")

Cyril reported that 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup()
into two funcs"), which appeared in v5.17-rc1, broke booting on the
Raspberry Pi Compute Module 4.  Apparently 830aa6f29f07 panics with an
Asynchronous SError Interrupt, and after further commits here is a black
screen on HDMI and no output on the serial console.

This does not seem to affect the Raspberry Pi 4 B.

Reported-by: Cyril Brulebois <kibi@debian.org>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=215925
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 83 ++-------------------------
 1 file changed, 5 insertions(+), 78 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 3edd63735948..fd464d38fecb 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -196,8 +196,6 @@ static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie,
 static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val);
 static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val);
 static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val);
-static int brcm_pcie_linkup(struct brcm_pcie *pcie);
-static int brcm_pcie_add_bus(struct pci_bus *bus);
 
 enum {
 	RGR1_SW_INIT_1,
@@ -331,8 +329,6 @@ struct brcm_pcie {
 	u32			hw_rev;
 	void			(*perst_set)(struct brcm_pcie *pcie, u32 val);
 	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
-	bool			refusal_mode;
-	struct subdev_regulators *sr;
 };
 
 static inline bool is_bmips(const struct brcm_pcie *pcie)
@@ -501,34 +497,6 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
 	return 0;
 }
 
-static int brcm_pcie_add_bus(struct pci_bus *bus)
-{
-	struct device *dev = &bus->dev;
-	struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata;
-	int ret;
-
-	if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent))
-		return 0;
-
-	ret = pci_subdev_regulators_add_bus(bus);
-	if (ret)
-		return ret;
-
-	/* Grab the regulators for suspend/resume */
-	pcie->sr = bus->dev.driver_data;
-
-	/*
-	 * If we have failed linkup there is no point to return an error as
-	 * currently it will cause a WARNING() from pci_alloc_child_bus().
-	 * We return 0 and turn on the "refusal_mode" so that any further
-	 * accesses to the pci_dev just get 0xffffffff
-	 */
-	if (brcm_pcie_linkup(pcie) != 0)
-		pcie->refusal_mode = true;
-
-	return 0;
-}
-
 static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
 {
 	struct device *dev = &bus->dev;
@@ -857,18 +825,6 @@ static void __iomem *brcm_pcie_map_conf(struct pci_bus *bus, unsigned int devfn,
 	/* Accesses to the RC go right to the RC registers if slot==0 */
 	if (pci_is_root_bus(bus))
 		return PCI_SLOT(devfn) ? NULL : base + where;
-	if (pcie->refusal_mode) {
-		/*
-		 * At this point we do not have link.  There will be a CPU
-		 * abort -- a quirk with this controller --if Linux tries
-		 * to read any config-space registers besides those
-		 * targeting the host bridge.  To prevent this we hijack
-		 * the address to point to a safe access that will return
-		 * 0xffffffff.
-		 */
-		writel(0xffffffff, base + PCIE_MISC_RC_BAR2_CONFIG_HI);
-		return base + PCIE_MISC_RC_BAR2_CONFIG_HI + (where & 0x3);
-	}
 
 	/* For devices, write to the config space index register */
 	idx = PCIE_ECAM_OFFSET(bus->number, devfn, 0);
@@ -897,7 +853,7 @@ static struct pci_ops brcm_pcie_ops = {
 	.map_bus = brcm_pcie_map_conf,
 	.read = pci_generic_config_read,
 	.write = pci_generic_config_write,
-	.add_bus = brcm_pcie_add_bus,
+	.add_bus = pci_subdev_regulators_add_bus,
 	.remove_bus = pci_subdev_regulators_remove_bus,
 };
 
@@ -1370,14 +1326,6 @@ static int brcm_pcie_suspend(struct device *dev)
 		return ret;
 	}
 
-	if (pcie->sr) {
-		ret = regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies);
-		if (ret) {
-			dev_err(dev, "Could not turn off regulators\n");
-			reset_control_reset(pcie->rescal);
-			return ret;
-		}
-	}
 	clk_disable_unprepare(pcie->clk);
 
 	return 0;
@@ -1395,17 +1343,9 @@ static int brcm_pcie_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	if (pcie->sr) {
-		ret = regulator_bulk_enable(pcie->sr->num_supplies, pcie->sr->supplies);
-		if (ret) {
-			dev_err(dev, "Could not turn on regulators\n");
-			goto err_disable_clk;
-		}
-	}
-
 	ret = reset_control_reset(pcie->rescal);
 	if (ret)
-		goto err_regulator;
+		goto err_disable_clk;
 
 	ret = brcm_phy_start(pcie);
 	if (ret)
@@ -1437,9 +1377,6 @@ static int brcm_pcie_resume(struct device *dev)
 
 err_reset:
 	reset_control_rearm(pcie->rescal);
-err_regulator:
-	if (pcie->sr)
-		regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies);
 err_disable_clk:
 	clk_disable_unprepare(pcie->clk);
 	return ret;
@@ -1571,17 +1508,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, pcie);
 
-	ret = pci_host_probe(bridge);
-	if (!ret && !brcm_pcie_link_up(pcie))
-		ret = -ENODEV;
-
-	if (ret) {
-		brcm_pcie_remove(pdev);
-		return ret;
-	}
-
-	return 0;
-
+	return pci_host_probe(bridge);
 fail:
 	__brcm_pcie_remove(pcie);
 	return ret;
@@ -1590,8 +1517,8 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 MODULE_DEVICE_TABLE(of, brcm_pcie_match);
 
 static const struct dev_pm_ops brcm_pcie_pm_ops = {
-	.suspend_noirq = brcm_pcie_suspend,
-	.resume_noirq = brcm_pcie_resume,
+	.suspend = brcm_pcie_suspend,
+	.resume = brcm_pcie_resume,
 };
 
 static struct platform_driver brcm_pcie_driver = {
-- 
2.25.1


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

* [PATCH 3/4] Revert "PCI: brcmstb: Add mechanism to turn on subdev regulators"
  2022-05-11 20:18 [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff Bjorn Helgaas
  2022-05-11 20:18 ` [PATCH 1/4] Revert "PCI: brcmstb: Do not turn off WOL regulators on suspend" Bjorn Helgaas
  2022-05-11 20:18 ` [PATCH 2/4] Revert "PCI: brcmstb: Add control of subdevice voltage regulators" Bjorn Helgaas
@ 2022-05-11 20:18 ` Bjorn Helgaas
  2022-05-11 20:18 ` [PATCH 4/4] Revert "PCI: brcmstb: Split brcm_pcie_setup() into two funcs" Bjorn Helgaas
  2022-05-11 20:24 ` [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff Florian Fainelli
  4 siblings, 0 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2022-05-11 20:18 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: Lorenzo Pieralisi, Cyril Brulebois, Nicolas Saenz Julienne,
	Florian Fainelli, bcm-kernel-feedback-list, linux-pci,
	regressions, Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

This reverts commit 67211aadcb4b968d0fdc57bc27240fa71500c2d4.

This is part of a revert of the following commits:

  11ed8b8624b8 ("PCI: brcmstb: Do not turn off WOL regulators on suspend")
  93e41f3fca3d ("PCI: brcmstb: Add control of subdevice voltage regulators")
  67211aadcb4b ("PCI: brcmstb: Add mechanism to turn on subdev regulators")
  830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup() into two funcs")

Cyril reported that 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup()
into two funcs"), which appeared in v5.17-rc1, broke booting on the
Raspberry Pi Compute Module 4.  Apparently 830aa6f29f07 panics with an
Asynchronous SError Interrupt, and after further commits here is a black
screen on HDMI and no output on the serial console.

This does not seem to affect the Raspberry Pi 4 B.

Reported-by: Cyril Brulebois <kibi@debian.org>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=215925
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 76 ---------------------------
 1 file changed, 76 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index fd464d38fecb..0e8346114a8d 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -24,7 +24,6 @@
 #include <linux/pci.h>
 #include <linux/pci-ecam.h>
 #include <linux/printk.h>
-#include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
@@ -284,14 +283,6 @@ static const struct pcie_cfg_data bcm2711_cfg = {
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 };
 
-struct subdev_regulators {
-	unsigned int num_supplies;
-	struct regulator_bulk_data supplies[];
-};
-
-static int pci_subdev_regulators_add_bus(struct pci_bus *bus);
-static void pci_subdev_regulators_remove_bus(struct pci_bus *bus);
-
 struct brcm_msi {
 	struct device		*dev;
 	void __iomem		*base;
@@ -445,71 +436,6 @@ static int brcm_pcie_set_ssc(struct brcm_pcie *pcie)
 	return ssc && pll ? 0 : -EIO;
 }
 
-static void *alloc_subdev_regulators(struct device *dev)
-{
-	static const char * const supplies[] = {
-		"vpcie3v3",
-		"vpcie3v3aux",
-		"vpcie12v",
-	};
-	const size_t size = sizeof(struct subdev_regulators)
-		+ sizeof(struct regulator_bulk_data) * ARRAY_SIZE(supplies);
-	struct subdev_regulators *sr;
-	int i;
-
-	sr = devm_kzalloc(dev, size, GFP_KERNEL);
-	if (sr) {
-		sr->num_supplies = ARRAY_SIZE(supplies);
-		for (i = 0; i < ARRAY_SIZE(supplies); i++)
-			sr->supplies[i].supply = supplies[i];
-	}
-
-	return sr;
-}
-
-static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
-{
-	struct device *dev = &bus->dev;
-	struct subdev_regulators *sr;
-	int ret;
-
-	if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent))
-		return 0;
-
-	if (dev->driver_data)
-		dev_err(dev, "dev.driver_data unexpectedly non-NULL\n");
-
-	sr = alloc_subdev_regulators(dev);
-	if (!sr)
-		return -ENOMEM;
-
-	dev->driver_data = sr;
-	ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
-	if (ret)
-		return ret;
-
-	ret = regulator_bulk_enable(sr->num_supplies, sr->supplies);
-	if (ret) {
-		dev_err(dev, "failed to enable regulators for downstream device\n");
-		return ret;
-	}
-
-	return 0;
-}
-
-static void pci_subdev_regulators_remove_bus(struct pci_bus *bus)
-{
-	struct device *dev = &bus->dev;
-	struct subdev_regulators *sr = dev->driver_data;
-
-	if (!sr || !bus->parent || !pci_is_root_bus(bus->parent))
-		return;
-
-	if (regulator_bulk_disable(sr->num_supplies, sr->supplies))
-		dev_err(dev, "failed to disable regulators for downstream device\n");
-	dev->driver_data = NULL;
-}
-
 /* Limits operation to a specific generation (1, 2, or 3) */
 static void brcm_pcie_set_gen(struct brcm_pcie *pcie, int gen)
 {
@@ -853,8 +779,6 @@ static struct pci_ops brcm_pcie_ops = {
 	.map_bus = brcm_pcie_map_conf,
 	.read = pci_generic_config_read,
 	.write = pci_generic_config_write,
-	.add_bus = pci_subdev_regulators_add_bus,
-	.remove_bus = pci_subdev_regulators_remove_bus,
 };
 
 static struct pci_ops brcm_pcie_ops32 = {
-- 
2.25.1


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

* [PATCH 4/4] Revert "PCI: brcmstb: Split brcm_pcie_setup() into two funcs"
  2022-05-11 20:18 [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff Bjorn Helgaas
                   ` (2 preceding siblings ...)
  2022-05-11 20:18 ` [PATCH 3/4] Revert "PCI: brcmstb: Add mechanism to turn on subdev regulators" Bjorn Helgaas
@ 2022-05-11 20:18 ` Bjorn Helgaas
  2022-05-11 20:24 ` [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff Florian Fainelli
  4 siblings, 0 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2022-05-11 20:18 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: Lorenzo Pieralisi, Cyril Brulebois, Nicolas Saenz Julienne,
	Florian Fainelli, bcm-kernel-feedback-list, linux-pci,
	regressions, Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

This reverts commit 830aa6f29f07a4e2f1a947dfa72b3ccddb46dd21.

This is part of a revert of the following commits:

  11ed8b8624b8 ("PCI: brcmstb: Do not turn off WOL regulators on suspend")
  93e41f3fca3d ("PCI: brcmstb: Add control of subdevice voltage regulators")
  67211aadcb4b ("PCI: brcmstb: Add mechanism to turn on subdev regulators")
  830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup() into two funcs")

Cyril reported that 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup()
into two funcs"), which appeared in v5.17-rc1, broke booting on the
Raspberry Pi Compute Module 4.  Apparently 830aa6f29f07 panics with an
Asynchronous SError Interrupt, and after further commits here is a black
screen on HDMI and no output on the serial console.

This does not seem to affect the Raspberry Pi 4 B.

Reported-by: Cyril Brulebois <kibi@debian.org>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=215925
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 65 +++++++++++----------------
 1 file changed, 26 insertions(+), 39 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 0e8346114a8d..e61058e13818 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -926,9 +926,16 @@ static inline int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
 
 static int brcm_pcie_setup(struct brcm_pcie *pcie)
 {
+	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
 	u64 rc_bar2_offset, rc_bar2_size;
 	void __iomem *base = pcie->base;
-	int ret, memc;
+	struct device *dev = pcie->dev;
+	struct resource_entry *entry;
+	bool ssc_good = false;
+	struct resource *res;
+	int num_out_wins = 0;
+	u16 nlw, cls, lnksta;
+	int i, ret, memc;
 	u32 tmp, burst, aspm_support;
 
 	/* Reset the bridge */
@@ -1018,40 +1025,6 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	if (pcie->gen)
 		brcm_pcie_set_gen(pcie, pcie->gen);
 
-	/* Don't advertise L0s capability if 'aspm-no-l0s' */
-	aspm_support = PCIE_LINK_STATE_L1;
-	if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
-		aspm_support |= PCIE_LINK_STATE_L0S;
-	tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
-	u32p_replace_bits(&tmp, aspm_support,
-		PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
-	writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
-
-	/*
-	 * For config space accesses on the RC, show the right class for
-	 * a PCIe-PCIe bridge (the default setting is to be EP mode).
-	 */
-	tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
-	u32p_replace_bits(&tmp, 0x060400,
-			  PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
-	writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
-
-	return 0;
-}
-
-static int brcm_pcie_linkup(struct brcm_pcie *pcie)
-{
-	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
-	struct device *dev = pcie->dev;
-	void __iomem *base = pcie->base;
-	struct resource_entry *entry;
-	struct resource *res;
-	int num_out_wins = 0;
-	u16 nlw, cls, lnksta;
-	bool ssc_good = false;
-	u32 tmp;
-	int ret, i;
-
 	/* Unassert the fundamental reset */
 	pcie->perst_set(pcie, 0);
 
@@ -1102,6 +1075,24 @@ static int brcm_pcie_linkup(struct brcm_pcie *pcie)
 		num_out_wins++;
 	}
 
+	/* Don't advertise L0s capability if 'aspm-no-l0s' */
+	aspm_support = PCIE_LINK_STATE_L1;
+	if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
+		aspm_support |= PCIE_LINK_STATE_L0S;
+	tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
+	u32p_replace_bits(&tmp, aspm_support,
+		PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
+	writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
+
+	/*
+	 * For config space accesses on the RC, show the right class for
+	 * a PCIe-PCIe bridge (the default setting is to be EP mode).
+	 */
+	tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
+	u32p_replace_bits(&tmp, 0x060400,
+			  PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
+	writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
+
 	if (pcie->ssc) {
 		ret = brcm_pcie_set_ssc(pcie);
 		if (ret == 0)
@@ -1290,10 +1281,6 @@ static int brcm_pcie_resume(struct device *dev)
 	if (ret)
 		goto err_reset;
 
-	ret = brcm_pcie_linkup(pcie);
-	if (ret)
-		goto err_reset;
-
 	if (pcie->msi)
 		brcm_msi_set_regs(pcie->msi);
 
-- 
2.25.1


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

* Re: [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff
  2022-05-11 20:18 [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff Bjorn Helgaas
                   ` (3 preceding siblings ...)
  2022-05-11 20:18 ` [PATCH 4/4] Revert "PCI: brcmstb: Split brcm_pcie_setup() into two funcs" Bjorn Helgaas
@ 2022-05-11 20:24 ` Florian Fainelli
  2022-05-11 20:39   ` Cyril Brulebois
  2022-05-11 20:39   ` Bjorn Helgaas
  4 siblings, 2 replies; 18+ messages in thread
From: Florian Fainelli @ 2022-05-11 20:24 UTC (permalink / raw)
  To: Bjorn Helgaas, Jim Quinlan
  Cc: Lorenzo Pieralisi, Cyril Brulebois, Nicolas Saenz Julienne,
	bcm-kernel-feedback-list, linux-pci, regressions, Bjorn Helgaas

On 5/11/22 13:18, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Cyril reported that 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup()
> into two funcs"), which appeared in v5.17-rc1, broke booting on the
> Raspberry Pi Compute Module 4.  Revert 830aa6f29f07 and subsequent patches
> for now.

How about we get a chance to fix this? Where, when and how was this even 
reported?
-- 
Florian

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

* Re: [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff
  2022-05-11 20:24 ` [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff Florian Fainelli
@ 2022-05-11 20:39   ` Cyril Brulebois
  2022-05-11 20:54     ` Florian Fainelli
  2022-05-11 20:39   ` Bjorn Helgaas
  1 sibling, 1 reply; 18+ messages in thread
From: Cyril Brulebois @ 2022-05-11 20:39 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Bjorn Helgaas, Jim Quinlan, Lorenzo Pieralisi,
	Nicolas Saenz Julienne, bcm-kernel-feedback-list, linux-pci,
	regressions, Bjorn Helgaas

[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]

Florian Fainelli <f.fainelli@gmail.com> (2022-05-11):
> How about we get a chance to fix this? Where, when and how was this even
> reported?

I started downstream:
  https://bugs.debian.org/1010365

did some more debugging and moved upstream (the link is mentioned in
each commit message) roughly 10 days ago:
  https://bugzilla.kernel.org/show_bug.cgi?id=215925

As I wrote in response to Thorsten Leemhuis (regressions@), who looped
in a bunch of people:

   I had spent so much time bisecting that issue that I concentrated on
   trying to summarize it properly in Bugzilla, failing to check
   reporting guidelines and best practices (I even missed the
   Regression: yes flag in my initial submission).

Again, sorry for failing to notify everyone in the first place, I tried
to have the contents of the report squared away.

That being said, as mentioned on regressions@ & linux-pci@, I'm happy to
test any attempts at fixing the issue, instead of a full-on revert. I'll
also try to track mainline more closely, so that such obvious issues
don't go unnoticed for ~1.5 release cycles.


Cheers,
-- 
Cyril Brulebois (kibi@debian.org)            <https://debamax.com/>
D-I release manager -- Release team member -- Freelance Consultant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

* Re: [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff
  2022-05-11 20:24 ` [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff Florian Fainelli
  2022-05-11 20:39   ` Cyril Brulebois
@ 2022-05-11 20:39   ` Bjorn Helgaas
  2022-06-13 17:06     ` Florian Fainelli
  1 sibling, 1 reply; 18+ messages in thread
From: Bjorn Helgaas @ 2022-05-11 20:39 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Jim Quinlan, Lorenzo Pieralisi, Cyril Brulebois,
	Nicolas Saenz Julienne, bcm-kernel-feedback-list, linux-pci,
	regressions, Bjorn Helgaas

On Wed, May 11, 2022 at 01:24:55PM -0700, Florian Fainelli wrote:
> On 5/11/22 13:18, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > Cyril reported that 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup()
> > into two funcs"), which appeared in v5.17-rc1, broke booting on the
> > Raspberry Pi Compute Module 4.  Revert 830aa6f29f07 and subsequent patches
> > for now.
> 
> How about we get a chance to fix this? Where, when and how was this even
> reported?

Sorry, I forgot to cc you, that's my fault:
  https://lore.kernel.org/r/CABhMZUWjZCwK1_qT2ghTSu2dguJBzBTpiTqKohyA72OSGMsaeg@mail.gmail.com

If you come up with a fix, I'll drop the reverts, of course.

Bjorn

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

* Re: [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff
  2022-05-11 20:39   ` Cyril Brulebois
@ 2022-05-11 20:54     ` Florian Fainelli
  0 siblings, 0 replies; 18+ messages in thread
From: Florian Fainelli @ 2022-05-11 20:54 UTC (permalink / raw)
  To: Cyril Brulebois, Florian Fainelli
  Cc: Bjorn Helgaas, Jim Quinlan, Lorenzo Pieralisi,
	Nicolas Saenz Julienne, bcm-kernel-feedback-list, linux-pci,
	regressions, Bjorn Helgaas

On 5/11/22 13:39, Cyril Brulebois wrote:
> Florian Fainelli <f.fainelli@gmail.com> (2022-05-11):
>> How about we get a chance to fix this? Where, when and how was this even
>> reported?
> 
> I started downstream:
>    https://bugs.debian.org/1010365
> 
> did some more debugging and moved upstream (the link is mentioned in
> each commit message) roughly 10 days ago:
>    https://bugzilla.kernel.org/show_bug.cgi?id=215925
> 
> As I wrote in response to Thorsten Leemhuis (regressions@), who looped
> in a bunch of people:
> 
>     I had spent so much time bisecting that issue that I concentrated on
>     trying to summarize it properly in Bugzilla, failing to check
>     reporting guidelines and best practices (I even missed the
>     Regression: yes flag in my initial submission).
> 
> Again, sorry for failing to notify everyone in the first place, I tried
> to have the contents of the report squared away.
> 
> That being said, as mentioned on regressions@ & linux-pci@, I'm happy to
> test any attempts at fixing the issue, instead of a full-on revert. I'll
> also try to track mainline more closely, so that such obvious issues
> don't go unnoticed for ~1.5 release cycles.

No worries, I don't have a CM4 board at the moment but will try to order 
one so we can get to the bottom of this quicker.
-- 
Florian

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

* Re: [PATCH 1/4] Revert "PCI: brcmstb: Do not turn off WOL regulators on suspend"
  2022-05-11 20:18 ` [PATCH 1/4] Revert "PCI: brcmstb: Do not turn off WOL regulators on suspend" Bjorn Helgaas
@ 2022-05-12  6:24   ` Thorsten Leemhuis
  2022-05-12 12:45     ` Bjorn Helgaas
  0 siblings, 1 reply; 18+ messages in thread
From: Thorsten Leemhuis @ 2022-05-12  6:24 UTC (permalink / raw)
  To: Bjorn Helgaas, Jim Quinlan
  Cc: Lorenzo Pieralisi, Cyril Brulebois, Nicolas Saenz Julienne,
	Florian Fainelli, bcm-kernel-feedback-list, linux-pci,
	regressions, Bjorn Helgaas

On 11.05.22 22:18, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> This reverts commit 11ed8b8624b8085f706864b4addcd304b1e4fc38.
> 
> This is part of a revert of the following commits:
> 
>   11ed8b8624b8 ("PCI: brcmstb: Do not turn off WOL regulators on suspend")
>   93e41f3fca3d ("PCI: brcmstb: Add control of subdevice voltage regulators")
>   67211aadcb4b ("PCI: brcmstb: Add mechanism to turn on subdev regulators")
>   830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup() into two funcs")
> 
> Cyril reported that 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup()
> into two funcs"), which appeared in v5.17-rc1, broke booting on the
> Raspberry Pi Compute Module 4.  Apparently 830aa6f29f07 panics with an
> Asynchronous SError Interrupt, and after further commits here is a black
> screen on HDMI and no output on the serial console.
> 
> This does not seem to affect the Raspberry Pi 4 B.
> 
> Reported-by: Cyril Brulebois <kibi@debian.org>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=215925

A "Bugzilla" tag? Why don't you just use a proper "Link:" tag, as
explained by the documentation to use in this case (I clarified the docs
recently with regards to this). Such inventions (some people use
"References:", others "BugLink:" and there were a few others I already
forget about) make my regression tracking efforts hard. :-/

Side note: Linus in the past few days two times reminded people to use
proper Link: tags, but that was in a totally different context (when
there was no link at all or just a Link: pointing to the submission of a
patch)

> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

> [...]

Ciao, Thorsten

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

* Re: [PATCH 1/4] Revert "PCI: brcmstb: Do not turn off WOL regulators on suspend"
  2022-05-12  6:24   ` Thorsten Leemhuis
@ 2022-05-12 12:45     ` Bjorn Helgaas
  0 siblings, 0 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2022-05-12 12:45 UTC (permalink / raw)
  To: Thorsten Leemhuis
  Cc: Jim Quinlan, Lorenzo Pieralisi, Cyril Brulebois,
	Nicolas Saenz Julienne, Florian Fainelli,
	bcm-kernel-feedback-list, linux-pci, regressions, Bjorn Helgaas

On Thu, May 12, 2022 at 08:24:00AM +0200, Thorsten Leemhuis wrote:
> On 11.05.22 22:18, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > This reverts commit 11ed8b8624b8085f706864b4addcd304b1e4fc38.
> > 
> > This is part of a revert of the following commits:
> > 
> >   11ed8b8624b8 ("PCI: brcmstb: Do not turn off WOL regulators on suspend")
> >   93e41f3fca3d ("PCI: brcmstb: Add control of subdevice voltage regulators")
> >   67211aadcb4b ("PCI: brcmstb: Add mechanism to turn on subdev regulators")
> >   830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup() into two funcs")
> > 
> > Cyril reported that 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup()
> > into two funcs"), which appeared in v5.17-rc1, broke booting on the
> > Raspberry Pi Compute Module 4.  Apparently 830aa6f29f07 panics with an
> > Asynchronous SError Interrupt, and after further commits here is a black
> > screen on HDMI and no output on the serial console.
> > 
> > This does not seem to affect the Raspberry Pi 4 B.
> > 
> > Reported-by: Cyril Brulebois <kibi@debian.org>
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=215925
> 
> A "Bugzilla" tag? Why don't you just use a proper "Link:" tag, as
> explained by the documentation to use in this case (I clarified the docs
> recently with regards to this). Such inventions (some people use
> "References:", others "BugLink:" and there were a few others I already
> forget about) make my regression tracking efforts hard. :-/

In this case, I actually looked through the history to try to figure
out the most common way to cite bugzilla but didn't see much
uniformity.

Anyway, I updated these to "Link:".

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

* Re: [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff
  2022-05-11 20:39   ` Bjorn Helgaas
@ 2022-06-13 17:06     ` Florian Fainelli
  2022-06-14  0:00       ` Bjorn Helgaas
  0 siblings, 1 reply; 18+ messages in thread
From: Florian Fainelli @ 2022-06-13 17:06 UTC (permalink / raw)
  To: Bjorn Helgaas, Jim Quinlan
  Cc: Lorenzo Pieralisi, Cyril Brulebois, Nicolas Saenz Julienne,
	bcm-kernel-feedback-list, linux-pci, regressions, Bjorn Helgaas

Bjorn,

On 5/11/22 13:39, Bjorn Helgaas wrote:
> On Wed, May 11, 2022 at 01:24:55PM -0700, Florian Fainelli wrote:
>> On 5/11/22 13:18, Bjorn Helgaas wrote:
>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>
>>> Cyril reported that 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup()
>>> into two funcs"), which appeared in v5.17-rc1, broke booting on the
>>> Raspberry Pi Compute Module 4.  Revert 830aa6f29f07 and subsequent patches
>>> for now.
>>
>> How about we get a chance to fix this? Where, when and how was this even
>> reported?
> 
> Sorry, I forgot to cc you, that's my fault:
>    https://lore.kernel.org/r/CABhMZUWjZCwK1_qT2ghTSu2dguJBzBTpiTqKohyA72OSGMsaeg@mail.gmail.com
> 
> If you come up with a fix, I'll drop the reverts, of course.

OK, so now this patch series has landed in Linus' tree and was committed 
on May 31st and we got no notification that this patch series was applied :/

How did I notice? Because suddenly the stable auto selection started to 
email me about the 4 reverts being included which is kind of the worse 
way to know about a patch having been applied.

What is even better is that meanwhile there was already a candidate fix 
proposed on May 18th, and a v2 on May 28th, so still an alternative to 
the reverts making it to Linus' tree, or so I thought.

This utterly annoys me because:

- the history for pcie-brcmstb.c is now looking super ugly because we 
have 4 commits getting reverted and if we were to add back the original 
feature being added now what? Do we come up with reverts of reverts, or 
the modified (with the fix) original commits applied on top, are not we 
going to sign ourselves for another 13 or so round of patches before we 
all agree on the solution?

- we could have just fixed this with proper communication from the get 
go about the regression in the first place, which remains the failure in 
communicating appropriately with driver authors/maintainers

- v5.17 and v5.18 final were already broken, but who on earth uses v5.17 
or v5.18 and not their stable counter parts, so we had a chance of 
slipping in a fix in a subsequent stable, I mean, it's been broken for 2 
releases on the CM4 and it was not noticed, so what was the urgency?

- the reverts will make it to -stable being bug fixes for regressions, 
however for users like Jim and I, now we will lose a feature that we 
were relying on, thus causing a regression for *many other* platforms 
than just the CM4

I appreciate that as a maintainer you are very sensitive to regressions 
and want to be responsive and responsible but this is not leaving just a 
slightest chance to right a wrong. Can we not do that again please?

Maybe I am being overly sensitive and disgruntled today, but really this 
is the type of thing that makes me want to quit working on the Linux kernel.
-- 
Florian

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

* Re: [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff
  2022-06-13 17:06     ` Florian Fainelli
@ 2022-06-14  0:00       ` Bjorn Helgaas
  2022-06-14 16:16         ` Florian Fainelli
  2022-06-14 18:59         ` Jim Quinlan
  0 siblings, 2 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2022-06-14  0:00 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Jim Quinlan, Lorenzo Pieralisi, Cyril Brulebois,
	Nicolas Saenz Julienne, bcm-kernel-feedback-list, linux-pci,
	regressions, Bjorn Helgaas

On Mon, Jun 13, 2022 at 10:06:12AM -0700, Florian Fainelli wrote:
> On 5/11/22 13:39, Bjorn Helgaas wrote:
> > On Wed, May 11, 2022 at 01:24:55PM -0700, Florian Fainelli wrote:
> > > On 5/11/22 13:18, Bjorn Helgaas wrote:
> > > > From: Bjorn Helgaas <bhelgaas@google.com>
> > > > 
> > > > Cyril reported that 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup()
> > > > into two funcs"), which appeared in v5.17-rc1, broke booting on the
> > > > Raspberry Pi Compute Module 4.  Revert 830aa6f29f07 and subsequent patches
> > > > for now.
> > > 
> > > How about we get a chance to fix this? Where, when and how was this even
> > > reported?
> > 
> > Sorry, I forgot to cc you, that's my fault:
> >    https://lore.kernel.org/r/CABhMZUWjZCwK1_qT2ghTSu2dguJBzBTpiTqKohyA72OSGMsaeg@mail.gmail.com
> > 
> > If you come up with a fix, I'll drop the reverts, of course.

> What is even better is that meanwhile there was already a candidate fix
> proposed on May 18th, and a v2 on May 28th, so still an alternative to the
> reverts making it to Linus' tree, or so I thought.

I hoped for a fix, but neither of those seemed to be clearly better.

> - the history for pcie-brcmstb.c is now looking super ugly because we have 4
> commits getting reverted and if we were to add back the original feature
> being added now what? Do we come up with reverts of reverts, or the modified
> (with the fix) original commits applied on top, are not we going to sign
> ourselves for another 13 or so round of patches before we all agree on the
> solution?

I agree on the ugliness and I try hard to avoid that.  In this case I
waited too long after the regression was discovered, hoping for a fix
that was better than the revert.  And I should have asked for
trade-offs between the revert and the the CM4 regression.

> - we could have just fixed this with proper communication from the get go
> about the regression in the first place, which remains the failure in
> communicating appropriately with driver authors/maintainers

I apologized earlier for omitting you when the regression was
discovered, and I'm still sorry.

> I appreciate that as a maintainer you are very sensitive to regressions and
> want to be responsive and responsible but this is not leaving just a
> slightest chance to right a wrong. Can we not do that again please?

Cyril opened the bugzilla April 30 and I forwarded it to linux-pci and
to Jim (but not you; again, I'm sorry for that omission) on May 2.
From my perspective we had almost a month to push this forward, but we
didn't quite make it.

I posted the reverts May 11, but I did not realize the regression to
you and other users they would cause.  I apologize for that.

Bjorn

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

* Re: [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff
  2022-06-14  0:00       ` Bjorn Helgaas
@ 2022-06-14 16:16         ` Florian Fainelli
  2022-06-14 18:59         ` Jim Quinlan
  1 sibling, 0 replies; 18+ messages in thread
From: Florian Fainelli @ 2022-06-14 16:16 UTC (permalink / raw)
  To: Bjorn Helgaas, Florian Fainelli
  Cc: Jim Quinlan, Lorenzo Pieralisi, Cyril Brulebois,
	Nicolas Saenz Julienne, bcm-kernel-feedback-list, linux-pci,
	regressions, Bjorn Helgaas

On 6/13/22 17:00, Bjorn Helgaas wrote:
> On Mon, Jun 13, 2022 at 10:06:12AM -0700, Florian Fainelli wrote:
>> On 5/11/22 13:39, Bjorn Helgaas wrote:
>>> On Wed, May 11, 2022 at 01:24:55PM -0700, Florian Fainelli wrote:
>>>> On 5/11/22 13:18, Bjorn Helgaas wrote:
>>>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>>>
>>>>> Cyril reported that 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup()
>>>>> into two funcs"), which appeared in v5.17-rc1, broke booting on the
>>>>> Raspberry Pi Compute Module 4.  Revert 830aa6f29f07 and subsequent patches
>>>>> for now.
>>>>
>>>> How about we get a chance to fix this? Where, when and how was this even
>>>> reported?
>>>
>>> Sorry, I forgot to cc you, that's my fault:
>>>     https://lore.kernel.org/r/CABhMZUWjZCwK1_qT2ghTSu2dguJBzBTpiTqKohyA72OSGMsaeg@mail.gmail.com
>>>
>>> If you come up with a fix, I'll drop the reverts, of course.
> 
>> What is even better is that meanwhile there was already a candidate fix
>> proposed on May 18th, and a v2 on May 28th, so still an alternative to the
>> reverts making it to Linus' tree, or so I thought.
> 
> I hoped for a fix, but neither of those seemed to be clearly better.

Humm, OK.

> 
>> - the history for pcie-brcmstb.c is now looking super ugly because we have 4
>> commits getting reverted and if we were to add back the original feature
>> being added now what? Do we come up with reverts of reverts, or the modified
>> (with the fix) original commits applied on top, are not we going to sign
>> ourselves for another 13 or so round of patches before we all agree on the
>> solution?
> 
> I agree on the ugliness and I try hard to avoid that.  In this case I
> waited too long after the regression was discovered, hoping for a fix
> that was better than the revert.  And I should have asked for
> trade-offs between the revert and the the CM4 regression.

Yes, I suppose that is fair, ideally this would have been an one liner 
but it was not quite that simple.

> 
>> - we could have just fixed this with proper communication from the get go
>> about the regression in the first place, which remains the failure in
>> communicating appropriately with driver authors/maintainers
> 
> I apologized earlier for omitting you when the regression was
> discovered, and I'm still sorry.

Apologies accepted :)

> 
>> I appreciate that as a maintainer you are very sensitive to regressions and
>> want to be responsive and responsible but this is not leaving just a
>> slightest chance to right a wrong. Can we not do that again please?
> 
> Cyril opened the bugzilla April 30 and I forwarded it to linux-pci and
> to Jim (but not you; again, I'm sorry for that omission) on May 2.
>  From my perspective we had almost a month to push this forward, but we
> didn't quite make it.

This is fine, I am not technically the driver author but Jim and I work 
together and I can always prioritize his work on upstream versus what we 
do downstream. As the "new" Raspberry Pi maintainer however I do care as 
well about not introducing regressions for Pi users, even if upstream is 
a niche on those platforms.

> 
> I posted the reverts May 11, but I did not realize the regression to
> you and other users they would cause.  I apologize for that.
> 

OK, thanks for your response, this makes me feel better.
-- 
Florian

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

* Re: [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff
  2022-06-14  0:00       ` Bjorn Helgaas
  2022-06-14 16:16         ` Florian Fainelli
@ 2022-06-14 18:59         ` Jim Quinlan
  2022-06-21 23:32           ` Bjorn Helgaas
  1 sibling, 1 reply; 18+ messages in thread
From: Jim Quinlan @ 2022-06-14 18:59 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Florian Fainelli, Lorenzo Pieralisi, Cyril Brulebois,
	Nicolas Saenz Julienne, bcm-kernel-feedback-list, linux-pci,
	regressions, Bjorn Helgaas

On Mon, Jun 13, 2022 at 8:00 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Mon, Jun 13, 2022 at 10:06:12AM -0700, Florian Fainelli wrote:
> > On 5/11/22 13:39, Bjorn Helgaas wrote:
> > > On Wed, May 11, 2022 at 01:24:55PM -0700, Florian Fainelli wrote:
> > > > On 5/11/22 13:18, Bjorn Helgaas wrote:
> > > > > From: Bjorn Helgaas <bhelgaas@google.com>
> > > > >
> > > > > Cyril reported that 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup()
> > > > > into two funcs"), which appeared in v5.17-rc1, broke booting on the
> > > > > Raspberry Pi Compute Module 4.  Revert 830aa6f29f07 and subsequent patches
> > > > > for now.
> > > >
> > > > How about we get a chance to fix this? Where, when and how was this even
> > > > reported?
> > >
> > > Sorry, I forgot to cc you, that's my fault:
> > >    https://lore.kernel.org/r/CABhMZUWjZCwK1_qT2ghTSu2dguJBzBTpiTqKohyA72OSGMsaeg@mail.gmail.com
> > >
> > > If you come up with a fix, I'll drop the reverts, of course.
>
> > What is even better is that meanwhile there was already a candidate fix
> > proposed on May 18th, and a v2 on May 28th, so still an alternative to the
> > reverts making it to Linus' tree, or so I thought.
>
> I hoped for a fix, but neither of those seemed to be clearly better.
>
> > - the history for pcie-brcmstb.c is now looking super ugly because we have 4
> > commits getting reverted and if we were to add back the original feature
> > being added now what? Do we come up with reverts of reverts, or the modified
> > (with the fix) original commits applied on top, are not we going to sign
> > ourselves for another 13 or so round of patches before we all agree on the
> > solution?
>
> I agree on the ugliness and I try hard to avoid that.  In this case I
> waited too long after the regression was discovered, hoping for a fix
> that was better than the revert.  And I should have asked for
> trade-offs between the revert and the the CM4 regression.
>
> > - we could have just fixed this with proper communication from the get go
> > about the regression in the first place, which remains the failure in
> > communicating appropriately with driver authors/maintainers
>
> I apologized earlier for omitting you when the regression was
> discovered, and I'm still sorry.
>
> > I appreciate that as a maintainer you are very sensitive to regressions and
> > want to be responsive and responsible but this is not leaving just a
> > slightest chance to right a wrong. Can we not do that again please?
>
> Cyril opened the bugzilla April 30 and I forwarded it to linux-pci and
> to Jim (but not you; again, I'm sorry for that omission) on May 2.
> From my perspective we had almost a month to push this forward, but we
> didn't quite make it.
Hello Bjorn,

Can you elaborate this? On May 18 I submitted v1, a viable fix.
At no point did you say "you need to get v2 in ASAP because I am planning on
reverting the entire original patchset in N days".  If I had known
this was the situation,
I could have had you a v2 on May 19th, but as it was I let the v1
email review thread
die out before submitting v2.

The original patchset was and is controversial, as it is basically a square peg
that does not fit nicely into a round Linux hole. It took 11 versions
of following reviewers'
suggestions until it was accepted.  And now it has been reverted, I am
wondering if it will ever be let in again or whether I should even try.

Regards,
Jim Quinlan
Broadcom STB


>
> I posted the reverts May 11, but I did not realize the regression to
> you and other users they would cause.  I apologize for that.
>
> Bjorn

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

* Re: [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff
  2022-06-14 18:59         ` Jim Quinlan
@ 2022-06-21 23:32           ` Bjorn Helgaas
  2022-06-27 23:18             ` Bjorn Helgaas
  0 siblings, 1 reply; 18+ messages in thread
From: Bjorn Helgaas @ 2022-06-21 23:32 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: Florian Fainelli, Lorenzo Pieralisi, Cyril Brulebois,
	Nicolas Saenz Julienne, bcm-kernel-feedback-list, linux-pci,
	regressions, Bjorn Helgaas

On Tue, Jun 14, 2022 at 02:59:02PM -0400, Jim Quinlan wrote:
> ...

> The original patchset was and is controversial, as it is basically a
> square peg that does not fit nicely into a round Linux hole. It took
> 11 versions of following reviewers' suggestions until it was
> accepted.  And now it has been reverted, I am wondering if it will
> ever be let in again or whether I should even try.

The original patchset [1] may have been controversial, but that's not
the issue here.  The only thing we need to solve here is to post the
four patches I reverted with a tiny change to one of them to avoid the
regression.  I don't think that should be a problem.

Bjorn

[1] https://lore.kernel.org/r/20220106160332.2143-1-jim2101024@gmail.com

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

* Re: [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff
  2022-06-21 23:32           ` Bjorn Helgaas
@ 2022-06-27 23:18             ` Bjorn Helgaas
  2022-07-01 11:25               ` Jim Quinlan
  0 siblings, 1 reply; 18+ messages in thread
From: Bjorn Helgaas @ 2022-06-27 23:18 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: Florian Fainelli, Lorenzo Pieralisi, Cyril Brulebois,
	Nicolas Saenz Julienne, bcm-kernel-feedback-list, linux-pci,
	regressions, Bjorn Helgaas

On Tue, Jun 21, 2022 at 06:32:59PM -0500, Bjorn Helgaas wrote:
> On Tue, Jun 14, 2022 at 02:59:02PM -0400, Jim Quinlan wrote:
> > ...
> 
> > The original patchset was and is controversial, as it is basically a
> > square peg that does not fit nicely into a round Linux hole. It took
> > 11 versions of following reviewers' suggestions until it was
> > accepted.  And now it has been reverted, I am wondering if it will
> > ever be let in again or whether I should even try.
> 
> The original patchset [1] may have been controversial, but that's not
> the issue here.  The only thing we need to solve here is to post the
> four patches I reverted with a tiny change to one of them to avoid the
> regression.  I don't think that should be a problem.

I'll be on vacation until July 7 or 8.  Just a heads-up so you know
it's not that I'm ignoring anything you may post in the meantime.

Bjorn

> [1] https://lore.kernel.org/r/20220106160332.2143-1-jim2101024@gmail.com

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

* Re: [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff
  2022-06-27 23:18             ` Bjorn Helgaas
@ 2022-07-01 11:25               ` Jim Quinlan
  0 siblings, 0 replies; 18+ messages in thread
From: Jim Quinlan @ 2022-07-01 11:25 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Florian Fainelli, Lorenzo Pieralisi, Cyril Brulebois,
	Nicolas Saenz Julienne, bcm-kernel-feedback-list, linux-pci,
	regressions, Bjorn Helgaas

On Mon, Jun 27, 2022 at 7:18 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, Jun 21, 2022 at 06:32:59PM -0500, Bjorn Helgaas wrote:
> > On Tue, Jun 14, 2022 at 02:59:02PM -0400, Jim Quinlan wrote:
> > > ...
> >
> > > The original patchset was and is controversial, as it is basically a
> > > square peg that does not fit nicely into a round Linux hole. It took
> > > 11 versions of following reviewers' suggestions until it was
> > > accepted.  And now it has been reverted, I am wondering if it will
> > > ever be let in ag I also see that the ain or whether I should even try.
> >
> > The original patchset [1] may have been controversial, but that's not
> > the issue here.  The only thing we need to solve here is to post the
> > four patches I reverted with a tiny change to one of them to avoid the
> > regression.  I don't think that should be a problem.
>
> I'll be on vacation until July 7 or 8.  Just a heads-up so you know
> it's not that I'm ignoring anything you may post in the meantime.

Thanks Bjorn, hopefully I will send it today.  At least for me, it's
not so easy to get the latest Linux running
on an RPi w/o running into unrelated  issues.  :-)

Jim Quinlan
Broadcom STB
>
> Bjorn
>
> > [1] https://lore.kernel.org/r/20220106160332.2143-1-jim2101024@gmail.com

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

end of thread, other threads:[~2022-07-01 11:25 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-11 20:18 [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff Bjorn Helgaas
2022-05-11 20:18 ` [PATCH 1/4] Revert "PCI: brcmstb: Do not turn off WOL regulators on suspend" Bjorn Helgaas
2022-05-12  6:24   ` Thorsten Leemhuis
2022-05-12 12:45     ` Bjorn Helgaas
2022-05-11 20:18 ` [PATCH 2/4] Revert "PCI: brcmstb: Add control of subdevice voltage regulators" Bjorn Helgaas
2022-05-11 20:18 ` [PATCH 3/4] Revert "PCI: brcmstb: Add mechanism to turn on subdev regulators" Bjorn Helgaas
2022-05-11 20:18 ` [PATCH 4/4] Revert "PCI: brcmstb: Split brcm_pcie_setup() into two funcs" Bjorn Helgaas
2022-05-11 20:24 ` [PATCH 0/4] PCI: brcmstb: Revert subdevice regulator stuff Florian Fainelli
2022-05-11 20:39   ` Cyril Brulebois
2022-05-11 20:54     ` Florian Fainelli
2022-05-11 20:39   ` Bjorn Helgaas
2022-06-13 17:06     ` Florian Fainelli
2022-06-14  0:00       ` Bjorn Helgaas
2022-06-14 16:16         ` Florian Fainelli
2022-06-14 18:59         ` Jim Quinlan
2022-06-21 23:32           ` Bjorn Helgaas
2022-06-27 23:18             ` Bjorn Helgaas
2022-07-01 11:25               ` Jim Quinlan

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).