[PATCH v4] amba: Remove deferred device addition

[PATCH v4] amba: Remove deferred device addition

[Saravana Kannan]

The uevents generated for an amba device need PID and CID information
that's available only when the amba device is powered on, clocked and
out of reset. So, if those resources aren't available, the information
can't be read to generate the uevents. To workaround this requirement,
if the resources weren't available, the device addition was deferred and
retried periodically.

However, this deferred addition retry isn't based on resources becoming
available. Instead, it's retried every 5 seconds and causes arbitrary
probe delays for amba devices and their consumers.

Also, maintaining a separate deferred-probe like mechanism is
maintenance headache.

With this commit, instead of deferring the device addition, we simply
defer the generation of uevents for the device and probing of the device
(because drivers needs PID and CID to match) until the PID and CID
information can be read. This allows us to delete all the amba specific
deferring code and also avoid the arbitrary probing delays.

Cc: Rob Herring <robh@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Saravana Kannan <saravanak@google.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Saravana Kannan <saravanak@google.com>
---

v1 -> v2:
- Dropped RFC tag
- Complete rewrite to not use stub devices.

v2 -> v3:
- Flipped the if() condition for hard-coded periphids.
- Added a stub driver to handle the case where all amba drivers are
  modules loaded by uevents.
- Cc Marek after I realized I forgot to add him.

v3 -> v4:
- Finally figured out and fixed the issue reported by Kefeng (bus match
  can't return an error other than -EPROBE_DEFER).
- I tested the patch on "V2P-CA15" on qemu
- Marek tested v3, but that was so long ago and the rebase wasn't clean,
  so I didn't include the tested-by.

Marek/Kefeng,

Mind giving a Tested-by?

-Saravana

 drivers/amba/bus.c | 317 +++++++++++++++++++++------------------------
 1 file changed, 145 insertions(+), 172 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 0e3ed5eb367b..9590c93b2aea 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -130,11 +130,100 @@ static struct attribute *amba_dev_attrs[] = {
 };
 ATTRIBUTE_GROUPS(amba_dev);
 
+static int amba_read_periphid(struct amba_device *dev)
+{
+	struct reset_control *rstc;
+	u32 size, pid, cid;
+	void __iomem *tmp;
+	int i, ret;
+
+	ret = dev_pm_domain_attach(&dev->dev, true);
+	if (ret) {
+		dev_dbg(&dev->dev, "can't get PM domain: %d\n", ret);
+		goto err_out;
+	}
+
+	ret = amba_get_enable_pclk(dev);
+	if (ret) {
+		dev_dbg(&dev->dev, "can't get pclk: %d\n", ret);
+		goto err_pm;
+	}
+
+	/*
+	 * Find reset control(s) of the amba bus and de-assert them.
+	 */
+	rstc = of_reset_control_array_get_optional_shared(dev->dev.of_node);
+	if (IS_ERR(rstc)) {
+		ret = PTR_ERR(rstc);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&dev->dev, "can't get reset: %d\n", ret);
+		goto err_clk;
+	}
+	reset_control_deassert(rstc);
+	reset_control_put(rstc);
+
+	size = resource_size(&dev->res);
+	tmp = ioremap(dev->res.start, size);
+	if (!tmp) {
+		ret = -ENOMEM;
+		goto err_clk;
+	}
+
+	/*
+	 * Read pid and cid based on size of resource
+	 * they are located at end of region
+	 */
+	for (pid = 0, i = 0; i < 4; i++)
+		pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) << (i * 8);
+	for (cid = 0, i = 0; i < 4; i++)
+		cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << (i * 8);
+
+	if (cid == CORESIGHT_CID) {
+		/* set the base to the start of the last 4k block */
+		void __iomem *csbase = tmp + size - 4096;
+
+		dev->uci.devarch = readl(csbase + UCI_REG_DEVARCH_OFFSET);
+		dev->uci.devtype = readl(csbase + UCI_REG_DEVTYPE_OFFSET) & 0xff;
+	}
+
+	if (cid == AMBA_CID || cid == CORESIGHT_CID) {
+		dev->periphid = pid;
+		dev->cid = cid;
+	}
+
+	if (!dev->periphid)
+		ret = -ENODEV;
+
+	iounmap(tmp);
+
+err_clk:
+	amba_put_disable_pclk(dev);
+err_pm:
+	dev_pm_domain_detach(&dev->dev, true);
+err_out:
+	return ret;
+}
+
 static int amba_match(struct device *dev, struct device_driver *drv)
 {
 	struct amba_device *pcdev = to_amba_device(dev);
 	struct amba_driver *pcdrv = to_amba_driver(drv);
 
+	if (!pcdev->periphid) {
+		int ret = amba_read_periphid(pcdev);
+
+		/*
+		 * Returning any error other than -EPROBE_DEFER from bus match
+		 * can cause driver registration failure. So, if there's a
+		 * permanent failure in reading pid and cid, simply say that
+		 * none of the drivers match.
+		 */
+		if (ret)
+			return ret == -EPROBE_DEFER ? ret : 0;
+		dev_set_uevent_suppress(dev, false);
+		kobject_uevent(&dev->kobj, KOBJ_ADD);
+	}
+
 	/* When driver_override is set, only bind to the matching driver */
 	if (pcdev->driver_override)
 		return !strcmp(pcdev->driver_override, drv->name);
@@ -368,6 +457,42 @@ static int __init amba_init(void)
 
 postcore_initcall(amba_init);
 
+static int amba_proxy_probe(struct amba_device *adev,
+			    const struct amba_id *id)
+{
+	WARN(1, "Stub driver should never match any device.\n");
+	return -ENODEV;
+}
+
+static const struct amba_id amba_stub_drv_ids[] = {
+	{ 0, 0 },
+};
+
+static struct amba_driver amba_proxy_drv = {
+	.drv = {
+		.name = "amba-proxy",
+	},
+	.probe = amba_proxy_probe,
+	.id_table = amba_stub_drv_ids,
+};
+
+static int __init amba_stub_drv_init(void)
+{
+	if (!IS_ENABLED(CONFIG_MODULES))
+		return 0;
+
+	/*
+	 * The amba_match() function will get called only if there is at least
+	 * one amba driver registered. If all amba drivers are modules and are
+	 * only loaded based on uevents, then we'll hit a chicken-and-egg
+	 * situation where amba_match() is waiting on drivers and drivers are
+	 * waiting on amba_match(). So, register a stub driver to make sure
+	 * amba_match() is called even if no amba driver has been registered.
+	 */
+	return amba_driver_register(&amba_proxy_drv);
+}
+late_initcall_sync(amba_stub_drv_init);
+
 /**
  *	amba_driver_register - register an AMBA device driver
  *	@drv: amba device driver structure
@@ -410,160 +535,6 @@ static void amba_device_release(struct device *dev)
 	kfree(d);
 }
 
-static int amba_read_periphid(struct amba_device *dev)
-{
-	struct reset_control *rstc;
-	u32 size, pid, cid;
-	void __iomem *tmp;
-	int i, ret;
-
-	ret = dev_pm_domain_attach(&dev->dev, true);
-	if (ret)
-		goto err_out;
-
-	ret = amba_get_enable_pclk(dev);
-	if (ret)
-		goto err_pm;
-
-	/*
-	 * Find reset control(s) of the amba bus and de-assert them.
-	 */
-	rstc = of_reset_control_array_get_optional_shared(dev->dev.of_node);
-	if (IS_ERR(rstc)) {
-		ret = PTR_ERR(rstc);
-		if (ret != -EPROBE_DEFER)
-			dev_err(&dev->dev, "can't get reset: %d\n", ret);
-		goto err_clk;
-	}
-	reset_control_deassert(rstc);
-	reset_control_put(rstc);
-
-	size = resource_size(&dev->res);
-	tmp = ioremap(dev->res.start, size);
-	if (!tmp) {
-		ret = -ENOMEM;
-		goto err_clk;
-	}
-
-	/*
-	 * Read pid and cid based on size of resource
-	 * they are located at end of region
-	 */
-	for (pid = 0, i = 0; i < 4; i++)
-		pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) << (i * 8);
-	for (cid = 0, i = 0; i < 4; i++)
-		cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << (i * 8);
-
-	if (cid == CORESIGHT_CID) {
-		/* set the base to the start of the last 4k block */
-		void __iomem *csbase = tmp + size - 4096;
-
-		dev->uci.devarch = readl(csbase + UCI_REG_DEVARCH_OFFSET);
-		dev->uci.devtype = readl(csbase + UCI_REG_DEVTYPE_OFFSET) & 0xff;
-	}
-
-	if (cid == AMBA_CID || cid == CORESIGHT_CID) {
-		dev->periphid = pid;
-		dev->cid = cid;
-	}
-
-	if (!dev->periphid)
-		ret = -ENODEV;
-
-	iounmap(tmp);
-
-err_clk:
-	amba_put_disable_pclk(dev);
-err_pm:
-	dev_pm_domain_detach(&dev->dev, true);
-err_out:
-	return ret;
-}
-
-static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
-{
-	int ret;
-
-	ret = request_resource(parent, &dev->res);
-	if (ret)
-		goto err_out;
-
-	/* Hard-coded primecell ID instead of plug-n-play */
-	if (dev->periphid != 0)
-		goto skip_probe;
-
-	ret = amba_read_periphid(dev);
-	if (ret) {
-		if (ret != -EPROBE_DEFER) {
-			amba_device_put(dev);
-			goto err_out;
-		}
-		goto err_release;
-	}
-
-skip_probe:
-	ret = device_add(&dev->dev);
-err_release:
-	if (ret)
-		release_resource(&dev->res);
-err_out:
-	return ret;
-}
-
-/*
- * Registration of AMBA device require reading its pid and cid registers.
- * To do this, the device must be turned on (if it is a part of power domain)
- * and have clocks enabled. However in some cases those resources might not be
- * yet available. Returning EPROBE_DEFER is not a solution in such case,
- * because callers don't handle this special error code. Instead such devices
- * are added to the special list and their registration is retried from
- * periodic worker, until all resources are available and registration succeeds.
- */
-struct deferred_device {
-	struct amba_device *dev;
-	struct resource *parent;
-	struct list_head node;
-};
-
-static LIST_HEAD(deferred_devices);
-static DEFINE_MUTEX(deferred_devices_lock);
-
-static void amba_deferred_retry_func(struct work_struct *dummy);
-static DECLARE_DELAYED_WORK(deferred_retry_work, amba_deferred_retry_func);
-
-#define DEFERRED_DEVICE_TIMEOUT (msecs_to_jiffies(5 * 1000))
-
-static int amba_deferred_retry(void)
-{
-	struct deferred_device *ddev, *tmp;
-
-	mutex_lock(&deferred_devices_lock);
-
-	list_for_each_entry_safe(ddev, tmp, &deferred_devices, node) {
-		int ret = amba_device_try_add(ddev->dev, ddev->parent);
-
-		if (ret == -EPROBE_DEFER)
-			continue;
-
-		list_del_init(&ddev->node);
-		kfree(ddev);
-	}
-
-	mutex_unlock(&deferred_devices_lock);
-
-	return 0;
-}
-late_initcall(amba_deferred_retry);
-
-static void amba_deferred_retry_func(struct work_struct *dummy)
-{
-	amba_deferred_retry();
-
-	if (!list_empty(&deferred_devices))
-		schedule_delayed_work(&deferred_retry_work,
-				      DEFERRED_DEVICE_TIMEOUT);
-}
-
 /**
  *	amba_device_add - add a previously allocated AMBA device structure
  *	@dev: AMBA device allocated by amba_device_alloc
@@ -575,28 +546,30 @@ static void amba_deferred_retry_func(struct work_struct *dummy)
  */
 int amba_device_add(struct amba_device *dev, struct resource *parent)
 {
-	int ret = amba_device_try_add(dev, parent);
-
-	if (ret == -EPROBE_DEFER) {
-		struct deferred_device *ddev;
-
-		ddev = kmalloc(sizeof(*ddev), GFP_KERNEL);
-		if (!ddev)
-			return -ENOMEM;
+	int ret;
 
-		ddev->dev = dev;
-		ddev->parent = parent;
-		ret = 0;
+	ret = request_resource(parent, &dev->res);
+	if (ret)
+		return ret;
 
-		mutex_lock(&deferred_devices_lock);
+	/* If primecell ID isn't hard-coded, figure it out */
+	if (!dev->periphid) {
+		/*
+		 * AMBA device uevents require reading its pid and cid
+		 * registers.  To do this, the device must be on, clocked and
+		 * out of reset.  However in some cases those resources might
+		 * not yet be available.  If that's the case, we suppress the
+		 * generation of uevents until we can read the pid and cid
+		 * registers.  See also amba_match().
+		 */
+		if (amba_read_periphid(dev))
+			dev_set_uevent_suppress(&dev->dev, true);
+	}
 
-		if (list_empty(&deferred_devices))
-			schedule_delayed_work(&deferred_retry_work,
-					      DEFERRED_DEVICE_TIMEOUT);
-		list_add_tail(&ddev->node, &deferred_devices);
+	ret = device_add(&dev->dev);
+	if (ret)
+		release_resource(&dev->res);
 
-		mutex_unlock(&deferred_devices_lock);
-	}
 	return ret;
 }
 EXPORT_SYMBOL_GPL(amba_device_add);
-- 
2.37.0.rc0.161.g10f37bed90-goog

Re: [PATCH v4] amba: Remove deferred device addition

[Sudeep Holla]

On Tue, Jul 05, 2022 at 01:39:34AM -0700, Saravana Kannan wrote:
> The uevents generated for an amba device need PID and CID information
> that's available only when the amba device is powered on, clocked and
> out of reset. So, if those resources aren't available, the information
> can't be read to generate the uevents. To workaround this requirement,
> if the resources weren't available, the device addition was deferred and
> retried periodically.
> 
> However, this deferred addition retry isn't based on resources becoming
> available. Instead, it's retried every 5 seconds and causes arbitrary
> probe delays for amba devices and their consumers.
> 
> Also, maintaining a separate deferred-probe like mechanism is
> maintenance headache.
> 
> With this commit, instead of deferring the device addition, we simply
> defer the generation of uevents for the device and probing of the device
> (because drivers needs PID and CID to match) until the PID and CID
> information can be read. This allows us to delete all the amba specific
> deferring code and also avoid the arbitrary probing delays.
> 
> Cc: Rob Herring <robh@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Saravana Kannan <saravanak@google.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
> 
> v1 -> v2:
> - Dropped RFC tag
> - Complete rewrite to not use stub devices.
> 
> v2 -> v3:
> - Flipped the if() condition for hard-coded periphids.
> - Added a stub driver to handle the case where all amba drivers are
>   modules loaded by uevents.
> - Cc Marek after I realized I forgot to add him.
> 
> v3 -> v4:
> - Finally figured out and fixed the issue reported by Kefeng (bus match
>   can't return an error other than -EPROBE_DEFER).
> - I tested the patch on "V2P-CA15" on qemu
> - Marek tested v3, but that was so long ago and the rebase wasn't clean,
>   so I didn't include the tested-by.
> 
> Marek/Kefeng,
> 
> Mind giving a Tested-by?
> 
> -Saravana
> 
>  drivers/amba/bus.c | 317 +++++++++++++++++++++------------------------
>  1 file changed, 145 insertions(+), 172 deletions(-)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 0e3ed5eb367b..9590c93b2aea 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -130,11 +130,100 @@ static struct attribute *amba_dev_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(amba_dev);
>  
> +static int amba_read_periphid(struct amba_device *dev)
> +{
> +	struct reset_control *rstc;
> +	u32 size, pid, cid;
> +	void __iomem *tmp;
> +	int i, ret;
> +
> +	ret = dev_pm_domain_attach(&dev->dev, true);
> +	if (ret) {
> +		dev_dbg(&dev->dev, "can't get PM domain: %d\n", ret);
> +		goto err_out;
> +	}
> +
> +	ret = amba_get_enable_pclk(dev);
> +	if (ret) {
> +		dev_dbg(&dev->dev, "can't get pclk: %d\n", ret);
> +		goto err_pm;
> +	}
> +
> +	/*
> +	 * Find reset control(s) of the amba bus and de-assert them.
> +	 */
> +	rstc = of_reset_control_array_get_optional_shared(dev->dev.of_node);
> +	if (IS_ERR(rstc)) {
> +		ret = PTR_ERR(rstc);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&dev->dev, "can't get reset: %d\n", ret);
> +		goto err_clk;
> +	}
> +	reset_control_deassert(rstc);
> +	reset_control_put(rstc);
> +
> +	size = resource_size(&dev->res);
> +	tmp = ioremap(dev->res.start, size);
> +	if (!tmp) {
> +		ret = -ENOMEM;
> +		goto err_clk;
> +	}
> +

I am seeing constant crash roughly around here. The read below is triggering
synchronous external abort. And adding even a single printk above this
anywhere seem to be making the issue disappear. While any prints after
the below reads just defer the issue and finally hit for some other device
while it always hits for the first delayed amba device.

> +	/*
> +	 * Read pid and cid based on size of resource
> +	 * they are located at end of region
> +	 */
> +	for (pid = 0, i = 0; i < 4; i++)
> +		pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) << (i * 8);
> +	for (cid = 0, i = 0; i < 4; i++)
> +		cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << (i * 8);
> +

I haven't spent time debugging this any further.

--
Regards,
Sudeep

1. No logs, always crash

Internal error: synchronous external abort: 96000210 [#1] PREEMPT SMP
 Modules linked in:
 CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.19.0-rc5-next-20220705-00001-g69f8b939ba4c #244
 Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform, BIOS EDK II Jun 21 2022
 pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
 pc : amba_read_periphid+0x128/0x270
 lr : amba_read_periphid+0x114/0x270
 sp : ffff80000803bc40
 x29: ffff80000803bc40 x28: f675a09e14a55da9 x27: ffff800009c56068
 x26: 0000000000000007 x25: ffff00080035f500 x24: ffff0008016c6468
 x23: ffff80000a5be848 x22: ffff80000a446088 x21: 0000000000000000
 x20: ffff0008003e1c00 x19: 0000000000001000 x18: ffffffffffffffff
 x17: 000000000000001c x16: 000000005885f043 x15: ffff000800032a1c
 x14: ffffffffffffffff x13: 0000000000000000 x12: 0101010101010101
 x11: ffff800008000000 x10: 0000000022010000 x9 : 0140000000000000
 x8 : 0040000000000001 x7 : 0000800019e9b000 x6 : 0000000000000000
 x5 : ffff800048175000 x4 : 0000000000000000 x3 : ffff800008175fe0
 x2 : 0068000000000f13 x1 : 0000000000000000 x0 : ffff800008175000
 Call trace:
  amba_read_periphid+0x128/0x270
  amba_match+0x24/0x90
  __driver_attach+0x2c/0x1c4
  bus_for_each_dev+0x60/0xd0
  driver_attach+0x24/0x30
  bus_add_driver+0xf4/0x230
  driver_register+0xbc/0x110
  amba_stub_drv_init+0x6c/0x9c
  do_one_initcall+0x6c/0x1c0
  kernel_init_freeable+0x34c/0x444
  kernel_init+0x28/0x13c
  ret_from_fork+0x10/0x20
 Code: d1008263 52800004 8b030003 52800006 (b9400062)
 ---[ end trace 0000000000000000 ]---

2. With prints after 2 for loops above

amba_read_periphid 20030000.tpiu
amba_read_periphid 20040000.funnel
amba_read_periphid 20070000.etr
amba_read_periphid 20100000.stm
amba_read_periphid 20120000.replicator
Internal error: synchronous external abort: 96000210 [#1] PREEMPT SMP
Modules linked in:
CPU: 4 PID: 1 Comm: swapper/0 Not tainted 5.19.0-rc5-next-20220705-00001-g69f8b939ba4c-dirty #243
Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform, BIOS EDK II Jun 21 2022
pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : readl+0x4/0x20
lr : amba_read_periphid+0x138/0x250
sp : ffff80000803bc40
x29: ffff80000803bc40 x28: f675a09e14a55da9 x27: ffff800009c56068
x26: 0000000000000007 x25: ffff00080035f500 x24: ffff000800248568
x23: ffff80000a5be848 x22: ffff80000a446088 x21: 0000000000001000
x20: ffff0008003e1c00 x19: 00000000fffffff4 x18: ffffffffffffffff
x17: 000000040044ffff x16: 00400032b5503510 x15: 0000000000000000
x14: ffffffffffffffff x13: 0000000000000000 x12: 0101010101010101
x11: ffff800008000000 x10: 0000000022010000 x9 : 0140000000000000
x8 : 0040000000000001 x7 : 0000800019e9b000 x6 : 0000000000000000
x5 : ffff800048175000 x4 : 0000000000000000 x3 : ffff800008175000
x2 : ffff800008175fe0 x1 : 0000000000000000 x0 : ffff800008175fe0
Call trace:
 readl+0x4/0x20
 amba_match+0x24/0x90
 __driver_attach+0x2c/0x1c4
 bus_for_each_dev+0x60/0xd0
 driver_attach+0x24/0x30
 bus_add_driver+0xf4/0x230
 driver_register+0xbc/0x110
 amba_stub_drv_init+0x6c/0x9c
 do_one_initcall+0x6c/0x1c0
 kernel_init_freeable+0x34c/0x444
 kernel_init+0x28/0x13c
 ret_from_fork+0x10/0x20
Code: d50323bf d65f03c0 00000000 d503233f (b9400000)
---[ end trace 0000000000000000 ]---

Re: [PATCH v4] amba: Remove deferred device addition

[Saravana Kannan]

On Tue, Jul 5, 2022 at 4:05 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Jul 05, 2022 at 01:39:34AM -0700, Saravana Kannan wrote:
> > The uevents generated for an amba device need PID and CID information
> > that's available only when the amba device is powered on, clocked and
> > out of reset. So, if those resources aren't available, the information
> > can't be read to generate the uevents. To workaround this requirement,
> > if the resources weren't available, the device addition was deferred and
> > retried periodically.
> >
> > However, this deferred addition retry isn't based on resources becoming
> > available. Instead, it's retried every 5 seconds and causes arbitrary
> > probe delays for amba devices and their consumers.
> >
> > Also, maintaining a separate deferred-probe like mechanism is
> > maintenance headache.
> >
> > With this commit, instead of deferring the device addition, we simply
> > defer the generation of uevents for the device and probing of the device
> > (because drivers needs PID and CID to match) until the PID and CID
> > information can be read. This allows us to delete all the amba specific
> > deferring code and also avoid the arbitrary probing delays.
> >
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > Cc: John Stultz <john.stultz@linaro.org>
> > Cc: Saravana Kannan <saravanak@google.com>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Sudeep Holla <sudeep.holla@arm.com>
> > Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> > Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
> > Cc: Russell King <linux@armlinux.org.uk>
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >
> > v1 -> v2:
> > - Dropped RFC tag
> > - Complete rewrite to not use stub devices.
> >
> > v2 -> v3:
> > - Flipped the if() condition for hard-coded periphids.
> > - Added a stub driver to handle the case where all amba drivers are
> >   modules loaded by uevents.
> > - Cc Marek after I realized I forgot to add him.
> >
> > v3 -> v4:
> > - Finally figured out and fixed the issue reported by Kefeng (bus match
> >   can't return an error other than -EPROBE_DEFER).
> > - I tested the patch on "V2P-CA15" on qemu
> > - Marek tested v3, but that was so long ago and the rebase wasn't clean,
> >   so I didn't include the tested-by.
> >
> > Marek/Kefeng,
> >
> > Mind giving a Tested-by?
> >
> > -Saravana
> >
> >  drivers/amba/bus.c | 317 +++++++++++++++++++++------------------------
> >  1 file changed, 145 insertions(+), 172 deletions(-)
> >
> > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> > index 0e3ed5eb367b..9590c93b2aea 100644
> > --- a/drivers/amba/bus.c
> > +++ b/drivers/amba/bus.c
> > @@ -130,11 +130,100 @@ static struct attribute *amba_dev_attrs[] = {
> >  };
> >  ATTRIBUTE_GROUPS(amba_dev);
> >
> > +static int amba_read_periphid(struct amba_device *dev)
> > +{
> > +     struct reset_control *rstc;
> > +     u32 size, pid, cid;
> > +     void __iomem *tmp;
> > +     int i, ret;
> > +
> > +     ret = dev_pm_domain_attach(&dev->dev, true);
> > +     if (ret) {
> > +             dev_dbg(&dev->dev, "can't get PM domain: %d\n", ret);
> > +             goto err_out;
> > +     }
> > +
> > +     ret = amba_get_enable_pclk(dev);
> > +     if (ret) {
> > +             dev_dbg(&dev->dev, "can't get pclk: %d\n", ret);
> > +             goto err_pm;
> > +     }
> > +
> > +     /*
> > +      * Find reset control(s) of the amba bus and de-assert them.
> > +      */
> > +     rstc = of_reset_control_array_get_optional_shared(dev->dev.of_node);
> > +     if (IS_ERR(rstc)) {
> > +             ret = PTR_ERR(rstc);
> > +             if (ret != -EPROBE_DEFER)
> > +                     dev_err(&dev->dev, "can't get reset: %d\n", ret);
> > +             goto err_clk;
> > +     }
> > +     reset_control_deassert(rstc);
> > +     reset_control_put(rstc);
> > +
> > +     size = resource_size(&dev->res);
> > +     tmp = ioremap(dev->res.start, size);
> > +     if (!tmp) {
> > +             ret = -ENOMEM;
> > +             goto err_clk;
> > +     }
> > +
>
> I am seeing constant crash roughly around here. The read below is triggering
> synchronous external abort.

I didn't touch anything in amba_read_periphid() except add two
dev_dbg() for the error paths. So I'm really surprised that you are
hitting any problems there.

> And adding even a single printk above this
> anywhere seem to be making the issue disappear.

Makes me wonder if there's a timing bug in one of the provider drivers
like pm domain/clk/reset that aren't adding udelays() for the
power/clk/reset signal to propagate in hardware/take effect. And
somehow my patch is increasing the likelihood of hitting it.

> While any prints after
> the below reads just defer the issue and finally hit for some other device
> while it always hits for the first delayed amba device.

This is so strange. I'm guessing qemu doesn't model this?

>
> > +     /*
> > +      * Read pid and cid based on size of resource
> > +      * they are located at end of region
> > +      */
> > +     for (pid = 0, i = 0; i < 4; i++)
> > +             pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) << (i * 8);
> > +     for (cid = 0, i = 0; i < 4; i++)
> > +             cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << (i * 8);
> > +
>
> I haven't spent time debugging this any further.

I think I'm going to end up needing your help with this one because it
seems to be some hardware related race. Actually, can you try this
patch on 5.18? Basically without my recent changes to linux-next?

Marek/Kefeng,

Can you test it too please? I'm hoping that can shed some additional
light on this.

Or maybe a second set of eyes to review my changes? I did code this
pretty late at night after all :)

Thanks,
Saravana


>
> --
> Regards,
> Sudeep
>
> 1. No logs, always crash
>
> Internal error: synchronous external abort: 96000210 [#1] PREEMPT SMP
>  Modules linked in:
>  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.19.0-rc5-next-20220705-00001-g69f8b939ba4c #244
>  Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform, BIOS EDK II Jun 21 2022
>  pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>  pc : amba_read_periphid+0x128/0x270
>  lr : amba_read_periphid+0x114/0x270
>  sp : ffff80000803bc40
>  x29: ffff80000803bc40 x28: f675a09e14a55da9 x27: ffff800009c56068
>  x26: 0000000000000007 x25: ffff00080035f500 x24: ffff0008016c6468
>  x23: ffff80000a5be848 x22: ffff80000a446088 x21: 0000000000000000
>  x20: ffff0008003e1c00 x19: 0000000000001000 x18: ffffffffffffffff
>  x17: 000000000000001c x16: 000000005885f043 x15: ffff000800032a1c
>  x14: ffffffffffffffff x13: 0000000000000000 x12: 0101010101010101
>  x11: ffff800008000000 x10: 0000000022010000 x9 : 0140000000000000
>  x8 : 0040000000000001 x7 : 0000800019e9b000 x6 : 0000000000000000
>  x5 : ffff800048175000 x4 : 0000000000000000 x3 : ffff800008175fe0
>  x2 : 0068000000000f13 x1 : 0000000000000000 x0 : ffff800008175000
>  Call trace:
>   amba_read_periphid+0x128/0x270
>   amba_match+0x24/0x90
>   __driver_attach+0x2c/0x1c4
>   bus_for_each_dev+0x60/0xd0
>   driver_attach+0x24/0x30
>   bus_add_driver+0xf4/0x230
>   driver_register+0xbc/0x110
>   amba_stub_drv_init+0x6c/0x9c
>   do_one_initcall+0x6c/0x1c0
>   kernel_init_freeable+0x34c/0x444
>   kernel_init+0x28/0x13c
>   ret_from_fork+0x10/0x20
>  Code: d1008263 52800004 8b030003 52800006 (b9400062)
>  ---[ end trace 0000000000000000 ]---
>
> 2. With prints after 2 for loops above
>
> amba_read_periphid 20030000.tpiu
> amba_read_periphid 20040000.funnel
> amba_read_periphid 20070000.etr
> amba_read_periphid 20100000.stm
> amba_read_periphid 20120000.replicator
> Internal error: synchronous external abort: 96000210 [#1] PREEMPT SMP
> Modules linked in:
> CPU: 4 PID: 1 Comm: swapper/0 Not tainted 5.19.0-rc5-next-20220705-00001-g69f8b939ba4c-dirty #243
> Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform, BIOS EDK II Jun 21 2022
> pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : readl+0x4/0x20
> lr : amba_read_periphid+0x138/0x250
> sp : ffff80000803bc40
> x29: ffff80000803bc40 x28: f675a09e14a55da9 x27: ffff800009c56068
> x26: 0000000000000007 x25: ffff00080035f500 x24: ffff000800248568
> x23: ffff80000a5be848 x22: ffff80000a446088 x21: 0000000000001000
> x20: ffff0008003e1c00 x19: 00000000fffffff4 x18: ffffffffffffffff
> x17: 000000040044ffff x16: 00400032b5503510 x15: 0000000000000000
> x14: ffffffffffffffff x13: 0000000000000000 x12: 0101010101010101
> x11: ffff800008000000 x10: 0000000022010000 x9 : 0140000000000000
> x8 : 0040000000000001 x7 : 0000800019e9b000 x6 : 0000000000000000
> x5 : ffff800048175000 x4 : 0000000000000000 x3 : ffff800008175000
> x2 : ffff800008175fe0 x1 : 0000000000000000 x0 : ffff800008175fe0
> Call trace:
>  readl+0x4/0x20
>  amba_match+0x24/0x90
>  __driver_attach+0x2c/0x1c4
>  bus_for_each_dev+0x60/0xd0
>  driver_attach+0x24/0x30
>  bus_add_driver+0xf4/0x230
>  driver_register+0xbc/0x110
>  amba_stub_drv_init+0x6c/0x9c
>  do_one_initcall+0x6c/0x1c0
>  kernel_init_freeable+0x34c/0x444
>  kernel_init+0x28/0x13c
>  ret_from_fork+0x10/0x20
> Code: d50323bf d65f03c0 00000000 d503233f (b9400000)
> ---[ end trace 0000000000000000 ]---
>
>
>

Re: [PATCH v4] amba: Remove deferred device addition

[Marek Szyprowski]

Hi Saravana,

On 05.07.2022 10:39, Saravana Kannan wrote:
> The uevents generated for an amba device need PID and CID information
> that's available only when the amba device is powered on, clocked and
> out of reset. So, if those resources aren't available, the information
> can't be read to generate the uevents. To workaround this requirement,
> if the resources weren't available, the device addition was deferred and
> retried periodically.
>
> However, this deferred addition retry isn't based on resources becoming
> available. Instead, it's retried every 5 seconds and causes arbitrary
> probe delays for amba devices and their consumers.
>
> Also, maintaining a separate deferred-probe like mechanism is
> maintenance headache.
>
> With this commit, instead of deferring the device addition, we simply
> defer the generation of uevents for the device and probing of the device
> (because drivers needs PID and CID to match) until the PID and CID
> information can be read. This allows us to delete all the amba specific
> deferring code and also avoid the arbitrary probing delays.
>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Saravana Kannan <saravanak@google.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>
> v1 -> v2:
> - Dropped RFC tag
> - Complete rewrite to not use stub devices.
>
> v2 -> v3:
> - Flipped the if() condition for hard-coded periphids.
> - Added a stub driver to handle the case where all amba drivers are
>    modules loaded by uevents.
> - Cc Marek after I realized I forgot to add him.
>
> v3 -> v4:
> - Finally figured out and fixed the issue reported by Kefeng (bus match
>    can't return an error other than -EPROBE_DEFER).
> - I tested the patch on "V2P-CA15" on qemu
> - Marek tested v3, but that was so long ago and the rebase wasn't clean,
>    so I didn't include the tested-by.
>
> Marek/Kefeng,
>
> Mind giving a Tested-by?


Yes, it looks that it still works fine.

I've tested it by changing the Exynos power domain driver to initialize 
from late_initcall. This in turn lead me to a bug in generic pm_domains 
code in __genpd_dev_pm_attach(), which returns -2 if the pm domain 
driver is not yet registered. After fixing that, I've successfully 
observed the deferred probe of PL330 driver on Exynos 4210 based boards 
both with this patch and without (with the old timer based code).

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>


> -Saravana
>
>   drivers/amba/bus.c | 317 +++++++++++++++++++++------------------------
>   1 file changed, 145 insertions(+), 172 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 0e3ed5eb367b..9590c93b2aea 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -130,11 +130,100 @@ static struct attribute *amba_dev_attrs[] = {
>   };
>   ATTRIBUTE_GROUPS(amba_dev);
>   
> +static int amba_read_periphid(struct amba_device *dev)
> +{
> +	struct reset_control *rstc;
> +	u32 size, pid, cid;
> +	void __iomem *tmp;
> +	int i, ret;
> +
> +	ret = dev_pm_domain_attach(&dev->dev, true);
> +	if (ret) {
> +		dev_dbg(&dev->dev, "can't get PM domain: %d\n", ret);
> +		goto err_out;
> +	}
> +
> +	ret = amba_get_enable_pclk(dev);
> +	if (ret) {
> +		dev_dbg(&dev->dev, "can't get pclk: %d\n", ret);
> +		goto err_pm;
> +	}
> +
> +	/*
> +	 * Find reset control(s) of the amba bus and de-assert them.
> +	 */
> +	rstc = of_reset_control_array_get_optional_shared(dev->dev.of_node);
> +	if (IS_ERR(rstc)) {
> +		ret = PTR_ERR(rstc);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&dev->dev, "can't get reset: %d\n", ret);
> +		goto err_clk;
> +	}
> +	reset_control_deassert(rstc);
> +	reset_control_put(rstc);
> +
> +	size = resource_size(&dev->res);
> +	tmp = ioremap(dev->res.start, size);
> +	if (!tmp) {
> +		ret = -ENOMEM;
> +		goto err_clk;
> +	}
> +
> +	/*
> +	 * Read pid and cid based on size of resource
> +	 * they are located at end of region
> +	 */
> +	for (pid = 0, i = 0; i < 4; i++)
> +		pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) << (i * 8);
> +	for (cid = 0, i = 0; i < 4; i++)
> +		cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << (i * 8);
> +
> +	if (cid == CORESIGHT_CID) {
> +		/* set the base to the start of the last 4k block */
> +		void __iomem *csbase = tmp + size - 4096;
> +
> +		dev->uci.devarch = readl(csbase + UCI_REG_DEVARCH_OFFSET);
> +		dev->uci.devtype = readl(csbase + UCI_REG_DEVTYPE_OFFSET) & 0xff;
> +	}
> +
> +	if (cid == AMBA_CID || cid == CORESIGHT_CID) {
> +		dev->periphid = pid;
> +		dev->cid = cid;
> +	}
> +
> +	if (!dev->periphid)
> +		ret = -ENODEV;
> +
> +	iounmap(tmp);
> +
> +err_clk:
> +	amba_put_disable_pclk(dev);
> +err_pm:
> +	dev_pm_domain_detach(&dev->dev, true);
> +err_out:
> +	return ret;
> +}
> +
>   static int amba_match(struct device *dev, struct device_driver *drv)
>   {
>   	struct amba_device *pcdev = to_amba_device(dev);
>   	struct amba_driver *pcdrv = to_amba_driver(drv);
>   
> +	if (!pcdev->periphid) {
> +		int ret = amba_read_periphid(pcdev);
> +
> +		/*
> +		 * Returning any error other than -EPROBE_DEFER from bus match
> +		 * can cause driver registration failure. So, if there's a
> +		 * permanent failure in reading pid and cid, simply say that
> +		 * none of the drivers match.
> +		 */
> +		if (ret)
> +			return ret == -EPROBE_DEFER ? ret : 0;
> +		dev_set_uevent_suppress(dev, false);
> +		kobject_uevent(&dev->kobj, KOBJ_ADD);
> +	}
> +
>   	/* When driver_override is set, only bind to the matching driver */
>   	if (pcdev->driver_override)
>   		return !strcmp(pcdev->driver_override, drv->name);
> @@ -368,6 +457,42 @@ static int __init amba_init(void)
>   
>   postcore_initcall(amba_init);
>   
> +static int amba_proxy_probe(struct amba_device *adev,
> +			    const struct amba_id *id)
> +{
> +	WARN(1, "Stub driver should never match any device.\n");
> +	return -ENODEV;
> +}
> +
> +static const struct amba_id amba_stub_drv_ids[] = {
> +	{ 0, 0 },
> +};
> +
> +static struct amba_driver amba_proxy_drv = {
> +	.drv = {
> +		.name = "amba-proxy",
> +	},
> +	.probe = amba_proxy_probe,
> +	.id_table = amba_stub_drv_ids,
> +};
> +
> +static int __init amba_stub_drv_init(void)
> +{
> +	if (!IS_ENABLED(CONFIG_MODULES))
> +		return 0;
> +
> +	/*
> +	 * The amba_match() function will get called only if there is at least
> +	 * one amba driver registered. If all amba drivers are modules and are
> +	 * only loaded based on uevents, then we'll hit a chicken-and-egg
> +	 * situation where amba_match() is waiting on drivers and drivers are
> +	 * waiting on amba_match(). So, register a stub driver to make sure
> +	 * amba_match() is called even if no amba driver has been registered.
> +	 */
> +	return amba_driver_register(&amba_proxy_drv);
> +}
> +late_initcall_sync(amba_stub_drv_init);
> +
>   /**
>    *	amba_driver_register - register an AMBA device driver
>    *	@drv: amba device driver structure
> @@ -410,160 +535,6 @@ static void amba_device_release(struct device *dev)
>   	kfree(d);
>   }
>   
> -static int amba_read_periphid(struct amba_device *dev)
> -{
> -	struct reset_control *rstc;
> -	u32 size, pid, cid;
> -	void __iomem *tmp;
> -	int i, ret;
> -
> -	ret = dev_pm_domain_attach(&dev->dev, true);
> -	if (ret)
> -		goto err_out;
> -
> -	ret = amba_get_enable_pclk(dev);
> -	if (ret)
> -		goto err_pm;
> -
> -	/*
> -	 * Find reset control(s) of the amba bus and de-assert them.
> -	 */
> -	rstc = of_reset_control_array_get_optional_shared(dev->dev.of_node);
> -	if (IS_ERR(rstc)) {
> -		ret = PTR_ERR(rstc);
> -		if (ret != -EPROBE_DEFER)
> -			dev_err(&dev->dev, "can't get reset: %d\n", ret);
> -		goto err_clk;
> -	}
> -	reset_control_deassert(rstc);
> -	reset_control_put(rstc);
> -
> -	size = resource_size(&dev->res);
> -	tmp = ioremap(dev->res.start, size);
> -	if (!tmp) {
> -		ret = -ENOMEM;
> -		goto err_clk;
> -	}
> -
> -	/*
> -	 * Read pid and cid based on size of resource
> -	 * they are located at end of region
> -	 */
> -	for (pid = 0, i = 0; i < 4; i++)
> -		pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) << (i * 8);
> -	for (cid = 0, i = 0; i < 4; i++)
> -		cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << (i * 8);
> -
> -	if (cid == CORESIGHT_CID) {
> -		/* set the base to the start of the last 4k block */
> -		void __iomem *csbase = tmp + size - 4096;
> -
> -		dev->uci.devarch = readl(csbase + UCI_REG_DEVARCH_OFFSET);
> -		dev->uci.devtype = readl(csbase + UCI_REG_DEVTYPE_OFFSET) & 0xff;
> -	}
> -
> -	if (cid == AMBA_CID || cid == CORESIGHT_CID) {
> -		dev->periphid = pid;
> -		dev->cid = cid;
> -	}
> -
> -	if (!dev->periphid)
> -		ret = -ENODEV;
> -
> -	iounmap(tmp);
> -
> -err_clk:
> -	amba_put_disable_pclk(dev);
> -err_pm:
> -	dev_pm_domain_detach(&dev->dev, true);
> -err_out:
> -	return ret;
> -}
> -
> -static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
> -{
> -	int ret;
> -
> -	ret = request_resource(parent, &dev->res);
> -	if (ret)
> -		goto err_out;
> -
> -	/* Hard-coded primecell ID instead of plug-n-play */
> -	if (dev->periphid != 0)
> -		goto skip_probe;
> -
> -	ret = amba_read_periphid(dev);
> -	if (ret) {
> -		if (ret != -EPROBE_DEFER) {
> -			amba_device_put(dev);
> -			goto err_out;
> -		}
> -		goto err_release;
> -	}
> -
> -skip_probe:
> -	ret = device_add(&dev->dev);
> -err_release:
> -	if (ret)
> -		release_resource(&dev->res);
> -err_out:
> -	return ret;
> -}
> -
> -/*
> - * Registration of AMBA device require reading its pid and cid registers.
> - * To do this, the device must be turned on (if it is a part of power domain)
> - * and have clocks enabled. However in some cases those resources might not be
> - * yet available. Returning EPROBE_DEFER is not a solution in such case,
> - * because callers don't handle this special error code. Instead such devices
> - * are added to the special list and their registration is retried from
> - * periodic worker, until all resources are available and registration succeeds.
> - */
> -struct deferred_device {
> -	struct amba_device *dev;
> -	struct resource *parent;
> -	struct list_head node;
> -};
> -
> -static LIST_HEAD(deferred_devices);
> -static DEFINE_MUTEX(deferred_devices_lock);
> -
> -static void amba_deferred_retry_func(struct work_struct *dummy);
> -static DECLARE_DELAYED_WORK(deferred_retry_work, amba_deferred_retry_func);
> -
> -#define DEFERRED_DEVICE_TIMEOUT (msecs_to_jiffies(5 * 1000))
> -
> -static int amba_deferred_retry(void)
> -{
> -	struct deferred_device *ddev, *tmp;
> -
> -	mutex_lock(&deferred_devices_lock);
> -
> -	list_for_each_entry_safe(ddev, tmp, &deferred_devices, node) {
> -		int ret = amba_device_try_add(ddev->dev, ddev->parent);
> -
> -		if (ret == -EPROBE_DEFER)
> -			continue;
> -
> -		list_del_init(&ddev->node);
> -		kfree(ddev);
> -	}
> -
> -	mutex_unlock(&deferred_devices_lock);
> -
> -	return 0;
> -}
> -late_initcall(amba_deferred_retry);
> -
> -static void amba_deferred_retry_func(struct work_struct *dummy)
> -{
> -	amba_deferred_retry();
> -
> -	if (!list_empty(&deferred_devices))
> -		schedule_delayed_work(&deferred_retry_work,
> -				      DEFERRED_DEVICE_TIMEOUT);
> -}
> -
>   /**
>    *	amba_device_add - add a previously allocated AMBA device structure
>    *	@dev: AMBA device allocated by amba_device_alloc
> @@ -575,28 +546,30 @@ static void amba_deferred_retry_func(struct work_struct *dummy)
>    */
>   int amba_device_add(struct amba_device *dev, struct resource *parent)
>   {
> -	int ret = amba_device_try_add(dev, parent);
> -
> -	if (ret == -EPROBE_DEFER) {
> -		struct deferred_device *ddev;
> -
> -		ddev = kmalloc(sizeof(*ddev), GFP_KERNEL);
> -		if (!ddev)
> -			return -ENOMEM;
> +	int ret;
>   
> -		ddev->dev = dev;
> -		ddev->parent = parent;
> -		ret = 0;
> +	ret = request_resource(parent, &dev->res);
> +	if (ret)
> +		return ret;
>   
> -		mutex_lock(&deferred_devices_lock);
> +	/* If primecell ID isn't hard-coded, figure it out */
> +	if (!dev->periphid) {
> +		/*
> +		 * AMBA device uevents require reading its pid and cid
> +		 * registers.  To do this, the device must be on, clocked and
> +		 * out of reset.  However in some cases those resources might
> +		 * not yet be available.  If that's the case, we suppress the
> +		 * generation of uevents until we can read the pid and cid
> +		 * registers.  See also amba_match().
> +		 */
> +		if (amba_read_periphid(dev))
> +			dev_set_uevent_suppress(&dev->dev, true);
> +	}
>   
> -		if (list_empty(&deferred_devices))
> -			schedule_delayed_work(&deferred_retry_work,
> -					      DEFERRED_DEVICE_TIMEOUT);
> -		list_add_tail(&ddev->node, &deferred_devices);
> +	ret = device_add(&dev->dev);
> +	if (ret)
> +		release_resource(&dev->res);
>   
> -		mutex_unlock(&deferred_devices_lock);
> -	}
>   	return ret;
>   }
>   EXPORT_SYMBOL_GPL(amba_device_add);

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v4] amba: Remove deferred device addition

[Marek Szyprowski]

Hi Saravana,

On 12.07.2022 14:25, Marek Szyprowski wrote:
> Hi Saravana,
>
> On 05.07.2022 10:39, Saravana Kannan wrote:
>> The uevents generated for an amba device need PID and CID information
>> that's available only when the amba device is powered on, clocked and
>> out of reset. So, if those resources aren't available, the information
>> can't be read to generate the uevents. To workaround this requirement,
>> if the resources weren't available, the device addition was deferred and
>> retried periodically.
>>
>> However, this deferred addition retry isn't based on resources becoming
>> available. Instead, it's retried every 5 seconds and causes arbitrary
>> probe delays for amba devices and their consumers.
>>
>> Also, maintaining a separate deferred-probe like mechanism is
>> maintenance headache.
>>
>> With this commit, instead of deferring the device addition, we simply
>> defer the generation of uevents for the device and probing of the device
>> (because drivers needs PID and CID to match) until the PID and CID
>> information can be read. This allows us to delete all the amba specific
>> deferring code and also avoid the arbitrary probing delays.
>>
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Cc: John Stultz <john.stultz@linaro.org>
>> Cc: Saravana Kannan <saravanak@google.com>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
>> Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
>> Cc: Russell King <linux@armlinux.org.uk>
>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>> ---
>>
>> v1 -> v2:
>> - Dropped RFC tag
>> - Complete rewrite to not use stub devices.
>>
>> v2 -> v3:
>> - Flipped the if() condition for hard-coded periphids.
>> - Added a stub driver to handle the case where all amba drivers are
>>    modules loaded by uevents.
>> - Cc Marek after I realized I forgot to add him.
>>
>> v3 -> v4:
>> - Finally figured out and fixed the issue reported by Kefeng (bus match
>>    can't return an error other than -EPROBE_DEFER).
>> - I tested the patch on "V2P-CA15" on qemu
>> - Marek tested v3, but that was so long ago and the rebase wasn't clean,
>>    so I didn't include the tested-by.
>>
>> Marek/Kefeng,
>>
>> Mind giving a Tested-by?
>
>
> Yes, it looks that it still works fine.
>
> I've tested it by changing the Exynos power domain driver to 
> initialize from late_initcall. This in turn lead me to a bug in 
> generic pm_domains code in __genpd_dev_pm_attach(), which returns -2 
> if the pm domain driver is not yet registered. After fixing that, I've 
> successfully observed the deferred probe of PL330 driver on Exynos 
> 4210 based boards both with this patch and without (with the old timer 
> based code).


While preparing a fix for the above issue in genpd I found that it has 
been introduced by your commit 5a46079a9645 ("PM: domains: Delete usage 
of driver_deferred_probe_check_state()"). I didn't analyze it enough, 
but it looks that something is missing there if we are trying to probe 
amba device. I assume that returning -EPROBE_DEFER unconditionally there 
is also not a valid solution?


>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>
>
>> -Saravana
>>
>>   drivers/amba/bus.c | 317 +++++++++++++++++++++------------------------
>>   1 file changed, 145 insertions(+), 172 deletions(-)
>>
>> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
>> index 0e3ed5eb367b..9590c93b2aea 100644
>> --- a/drivers/amba/bus.c
>> +++ b/drivers/amba/bus.c
>> @@ -130,11 +130,100 @@ static struct attribute *amba_dev_attrs[] = {
>>   };
>>   ATTRIBUTE_GROUPS(amba_dev);
>>   +static int amba_read_periphid(struct amba_device *dev)
>> +{
>> +    struct reset_control *rstc;
>> +    u32 size, pid, cid;
>> +    void __iomem *tmp;
>> +    int i, ret;
>> +
>> +    ret = dev_pm_domain_attach(&dev->dev, true);
>> +    if (ret) {
>> +        dev_dbg(&dev->dev, "can't get PM domain: %d\n", ret);
>> +        goto err_out;
>> +    }
>> +
>> +    ret = amba_get_enable_pclk(dev);
>> +    if (ret) {
>> +        dev_dbg(&dev->dev, "can't get pclk: %d\n", ret);
>> +        goto err_pm;
>> +    }
>> +
>> +    /*
>> +     * Find reset control(s) of the amba bus and de-assert them.
>> +     */
>> +    rstc = 
>> of_reset_control_array_get_optional_shared(dev->dev.of_node);
>> +    if (IS_ERR(rstc)) {
>> +        ret = PTR_ERR(rstc);
>> +        if (ret != -EPROBE_DEFER)
>> +            dev_err(&dev->dev, "can't get reset: %d\n", ret);
>> +        goto err_clk;
>> +    }
>> +    reset_control_deassert(rstc);
>> +    reset_control_put(rstc);
>> +
>> +    size = resource_size(&dev->res);
>> +    tmp = ioremap(dev->res.start, size);
>> +    if (!tmp) {
>> +        ret = -ENOMEM;
>> +        goto err_clk;
>> +    }
>> +
>> +    /*
>> +     * Read pid and cid based on size of resource
>> +     * they are located at end of region
>> +     */
>> +    for (pid = 0, i = 0; i < 4; i++)
>> +        pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) << (i * 8);
>> +    for (cid = 0, i = 0; i < 4; i++)
>> +        cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << (i * 8);
>> +
>> +    if (cid == CORESIGHT_CID) {
>> +        /* set the base to the start of the last 4k block */
>> +        void __iomem *csbase = tmp + size - 4096;
>> +
>> +        dev->uci.devarch = readl(csbase + UCI_REG_DEVARCH_OFFSET);
>> +        dev->uci.devtype = readl(csbase + UCI_REG_DEVTYPE_OFFSET) & 
>> 0xff;
>> +    }
>> +
>> +    if (cid == AMBA_CID || cid == CORESIGHT_CID) {
>> +        dev->periphid = pid;
>> +        dev->cid = cid;
>> +    }
>> +
>> +    if (!dev->periphid)
>> +        ret = -ENODEV;
>> +
>> +    iounmap(tmp);
>> +
>> +err_clk:
>> +    amba_put_disable_pclk(dev);
>> +err_pm:
>> +    dev_pm_domain_detach(&dev->dev, true);
>> +err_out:
>> +    return ret;
>> +}
>> +
>>   static int amba_match(struct device *dev, struct device_driver *drv)
>>   {
>>       struct amba_device *pcdev = to_amba_device(dev);
>>       struct amba_driver *pcdrv = to_amba_driver(drv);
>>   +    if (!pcdev->periphid) {
>> +        int ret = amba_read_periphid(pcdev);
>> +
>> +        /*
>> +         * Returning any error other than -EPROBE_DEFER from bus match
>> +         * can cause driver registration failure. So, if there's a
>> +         * permanent failure in reading pid and cid, simply say that
>> +         * none of the drivers match.
>> +         */
>> +        if (ret)
>> +            return ret == -EPROBE_DEFER ? ret : 0;
>> +        dev_set_uevent_suppress(dev, false);
>> +        kobject_uevent(&dev->kobj, KOBJ_ADD);
>> +    }
>> +
>>       /* When driver_override is set, only bind to the matching 
>> driver */
>>       if (pcdev->driver_override)
>>           return !strcmp(pcdev->driver_override, drv->name);
>> @@ -368,6 +457,42 @@ static int __init amba_init(void)
>>     postcore_initcall(amba_init);
>>   +static int amba_proxy_probe(struct amba_device *adev,
>> +                const struct amba_id *id)
>> +{
>> +    WARN(1, "Stub driver should never match any device.\n");
>> +    return -ENODEV;
>> +}
>> +
>> +static const struct amba_id amba_stub_drv_ids[] = {
>> +    { 0, 0 },
>> +};
>> +
>> +static struct amba_driver amba_proxy_drv = {
>> +    .drv = {
>> +        .name = "amba-proxy",
>> +    },
>> +    .probe = amba_proxy_probe,
>> +    .id_table = amba_stub_drv_ids,
>> +};
>> +
>> +static int __init amba_stub_drv_init(void)
>> +{
>> +    if (!IS_ENABLED(CONFIG_MODULES))
>> +        return 0;
>> +
>> +    /*
>> +     * The amba_match() function will get called only if there is at 
>> least
>> +     * one amba driver registered. If all amba drivers are modules 
>> and are
>> +     * only loaded based on uevents, then we'll hit a chicken-and-egg
>> +     * situation where amba_match() is waiting on drivers and 
>> drivers are
>> +     * waiting on amba_match(). So, register a stub driver to make sure
>> +     * amba_match() is called even if no amba driver has been 
>> registered.
>> +     */
>> +    return amba_driver_register(&amba_proxy_drv);
>> +}
>> +late_initcall_sync(amba_stub_drv_init);
>> +
>>   /**
>>    *    amba_driver_register - register an AMBA device driver
>>    *    @drv: amba device driver structure
>> @@ -410,160 +535,6 @@ static void amba_device_release(struct device 
>> *dev)
>>       kfree(d);
>>   }
>>   -static int amba_read_periphid(struct amba_device *dev)
>> -{
>> -    struct reset_control *rstc;
>> -    u32 size, pid, cid;
>> -    void __iomem *tmp;
>> -    int i, ret;
>> -
>> -    ret = dev_pm_domain_attach(&dev->dev, true);
>> -    if (ret)
>> -        goto err_out;
>> -
>> -    ret = amba_get_enable_pclk(dev);
>> -    if (ret)
>> -        goto err_pm;
>> -
>> -    /*
>> -     * Find reset control(s) of the amba bus and de-assert them.
>> -     */
>> -    rstc = 
>> of_reset_control_array_get_optional_shared(dev->dev.of_node);
>> -    if (IS_ERR(rstc)) {
>> -        ret = PTR_ERR(rstc);
>> -        if (ret != -EPROBE_DEFER)
>> -            dev_err(&dev->dev, "can't get reset: %d\n", ret);
>> -        goto err_clk;
>> -    }
>> -    reset_control_deassert(rstc);
>> -    reset_control_put(rstc);
>> -
>> -    size = resource_size(&dev->res);
>> -    tmp = ioremap(dev->res.start, size);
>> -    if (!tmp) {
>> -        ret = -ENOMEM;
>> -        goto err_clk;
>> -    }
>> -
>> -    /*
>> -     * Read pid and cid based on size of resource
>> -     * they are located at end of region
>> -     */
>> -    for (pid = 0, i = 0; i < 4; i++)
>> -        pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) << (i * 8);
>> -    for (cid = 0, i = 0; i < 4; i++)
>> -        cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << (i * 8);
>> -
>> -    if (cid == CORESIGHT_CID) {
>> -        /* set the base to the start of the last 4k block */
>> -        void __iomem *csbase = tmp + size - 4096;
>> -
>> -        dev->uci.devarch = readl(csbase + UCI_REG_DEVARCH_OFFSET);
>> -        dev->uci.devtype = readl(csbase + UCI_REG_DEVTYPE_OFFSET) & 
>> 0xff;
>> -    }
>> -
>> -    if (cid == AMBA_CID || cid == CORESIGHT_CID) {
>> -        dev->periphid = pid;
>> -        dev->cid = cid;
>> -    }
>> -
>> -    if (!dev->periphid)
>> -        ret = -ENODEV;
>> -
>> -    iounmap(tmp);
>> -
>> -err_clk:
>> -    amba_put_disable_pclk(dev);
>> -err_pm:
>> -    dev_pm_domain_detach(&dev->dev, true);
>> -err_out:
>> -    return ret;
>> -}
>> -
>> -static int amba_device_try_add(struct amba_device *dev, struct 
>> resource *parent)
>> -{
>> -    int ret;
>> -
>> -    ret = request_resource(parent, &dev->res);
>> -    if (ret)
>> -        goto err_out;
>> -
>> -    /* Hard-coded primecell ID instead of plug-n-play */
>> -    if (dev->periphid != 0)
>> -        goto skip_probe;
>> -
>> -    ret = amba_read_periphid(dev);
>> -    if (ret) {
>> -        if (ret != -EPROBE_DEFER) {
>> -            amba_device_put(dev);
>> -            goto err_out;
>> -        }
>> -        goto err_release;
>> -    }
>> -
>> -skip_probe:
>> -    ret = device_add(&dev->dev);
>> -err_release:
>> -    if (ret)
>> -        release_resource(&dev->res);
>> -err_out:
>> -    return ret;
>> -}
>> -
>> -/*
>> - * Registration of AMBA device require reading its pid and cid 
>> registers.
>> - * To do this, the device must be turned on (if it is a part of 
>> power domain)
>> - * and have clocks enabled. However in some cases those resources 
>> might not be
>> - * yet available. Returning EPROBE_DEFER is not a solution in such 
>> case,
>> - * because callers don't handle this special error code. Instead 
>> such devices
>> - * are added to the special list and their registration is retried from
>> - * periodic worker, until all resources are available and 
>> registration succeeds.
>> - */
>> -struct deferred_device {
>> -    struct amba_device *dev;
>> -    struct resource *parent;
>> -    struct list_head node;
>> -};
>> -
>> -static LIST_HEAD(deferred_devices);
>> -static DEFINE_MUTEX(deferred_devices_lock);
>> -
>> -static void amba_deferred_retry_func(struct work_struct *dummy);
>> -static DECLARE_DELAYED_WORK(deferred_retry_work, 
>> amba_deferred_retry_func);
>> -
>> -#define DEFERRED_DEVICE_TIMEOUT (msecs_to_jiffies(5 * 1000))
>> -
>> -static int amba_deferred_retry(void)
>> -{
>> -    struct deferred_device *ddev, *tmp;
>> -
>> -    mutex_lock(&deferred_devices_lock);
>> -
>> -    list_for_each_entry_safe(ddev, tmp, &deferred_devices, node) {
>> -        int ret = amba_device_try_add(ddev->dev, ddev->parent);
>> -
>> -        if (ret == -EPROBE_DEFER)
>> -            continue;
>> -
>> -        list_del_init(&ddev->node);
>> -        kfree(ddev);
>> -    }
>> -
>> -    mutex_unlock(&deferred_devices_lock);
>> -
>> -    return 0;
>> -}
>> -late_initcall(amba_deferred_retry);
>> -
>> -static void amba_deferred_retry_func(struct work_struct *dummy)
>> -{
>> -    amba_deferred_retry();
>> -
>> -    if (!list_empty(&deferred_devices))
>> -        schedule_delayed_work(&deferred_retry_work,
>> -                      DEFERRED_DEVICE_TIMEOUT);
>> -}
>> -
>>   /**
>>    *    amba_device_add - add a previously allocated AMBA device 
>> structure
>>    *    @dev: AMBA device allocated by amba_device_alloc
>> @@ -575,28 +546,30 @@ static void amba_deferred_retry_func(struct 
>> work_struct *dummy)
>>    */
>>   int amba_device_add(struct amba_device *dev, struct resource *parent)
>>   {
>> -    int ret = amba_device_try_add(dev, parent);
>> -
>> -    if (ret == -EPROBE_DEFER) {
>> -        struct deferred_device *ddev;
>> -
>> -        ddev = kmalloc(sizeof(*ddev), GFP_KERNEL);
>> -        if (!ddev)
>> -            return -ENOMEM;
>> +    int ret;
>>   -        ddev->dev = dev;
>> -        ddev->parent = parent;
>> -        ret = 0;
>> +    ret = request_resource(parent, &dev->res);
>> +    if (ret)
>> +        return ret;
>>   -        mutex_lock(&deferred_devices_lock);
>> +    /* If primecell ID isn't hard-coded, figure it out */
>> +    if (!dev->periphid) {
>> +        /*
>> +         * AMBA device uevents require reading its pid and cid
>> +         * registers.  To do this, the device must be on, clocked and
>> +         * out of reset.  However in some cases those resources might
>> +         * not yet be available.  If that's the case, we suppress the
>> +         * generation of uevents until we can read the pid and cid
>> +         * registers.  See also amba_match().
>> +         */
>> +        if (amba_read_periphid(dev))
>> +            dev_set_uevent_suppress(&dev->dev, true);
>> +    }
>>   -        if (list_empty(&deferred_devices))
>> -            schedule_delayed_work(&deferred_retry_work,
>> -                          DEFERRED_DEVICE_TIMEOUT);
>> -        list_add_tail(&ddev->node, &deferred_devices);
>> +    ret = device_add(&dev->dev);
>> +    if (ret)
>> +        release_resource(&dev->res);
>>   -        mutex_unlock(&deferred_devices_lock);
>> -    }
>>       return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(amba_device_add);
>
> Best regards

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v4] amba: Remove deferred device addition

[Kefeng Wang]

On 2022/7/5 16:39, Saravana Kannan wrote:
> The uevents generated for an amba device need PID and CID information
> that's available only when the amba device is powered on, clocked and
> out of reset. So, if those resources aren't available, the information
> can't be read to generate the uevents. To workaround this requirement,
> if the resources weren't available, the device addition was deferred and
> retried periodically.
>
> However, this deferred addition retry isn't based on resources becoming
> available. Instead, it's retried every 5 seconds and causes arbitrary
> probe delays for amba devices and their consumers.
>
> Also, maintaining a separate deferred-probe like mechanism is
> maintenance headache.
>
> With this commit, instead of deferring the device addition, we simply
> defer the generation of uevents for the device and probing of the device
> (because drivers needs PID and CID to match) until the PID and CID
> information can be read. This allows us to delete all the amba specific
> deferring code and also avoid the arbitrary probing delays.
>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Saravana Kannan <saravanak@google.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>
> v1 -> v2:
> - Dropped RFC tag
> - Complete rewrite to not use stub devices.
>
> v2 -> v3:
> - Flipped the if() condition for hard-coded periphids.
> - Added a stub driver to handle the case where all amba drivers are
>    modules loaded by uevents.
> - Cc Marek after I realized I forgot to add him.
>
> v3 -> v4:
> - Finally figured out and fixed the issue reported by Kefeng (bus match
>    can't return an error other than -EPROBE_DEFER).
> - I tested the patch on "V2P-CA15" on qemu
> - Marek tested v3, but that was so long ago and the rebase wasn't clean,
>    so I didn't include the tested-by.
>
> Marek/Kefeng,
>
> Mind giving a Tested-by?

Hi Saravana, I tested on my qemu, and previous panic[1] disappeared, so

Tested-by: Kefeng Wang <wangkefeng.wang@huawei.com>

[1] 
https://lore.kernel.org/linux-arm-kernel/CAGETcx8RLor0JcboBuMrB96xUot14P1CAcqoen7ZHnYRi7KMEQ@mail.gmail.com/

Re: [PATCH v4] amba: Remove deferred device addition

[Saravana Kannan]

On Tue, Jul 12, 2022 at 5:34 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Saravana,
>
> On 12.07.2022 14:25, Marek Szyprowski wrote:
> > Hi Saravana,
> >
> > On 05.07.2022 10:39, Saravana Kannan wrote:
> >> The uevents generated for an amba device need PID and CID information
> >> that's available only when the amba device is powered on, clocked and
> >> out of reset. So, if those resources aren't available, the information
> >> can't be read to generate the uevents. To workaround this requirement,
> >> if the resources weren't available, the device addition was deferred and
> >> retried periodically.
> >>
> >> However, this deferred addition retry isn't based on resources becoming
> >> available. Instead, it's retried every 5 seconds and causes arbitrary
> >> probe delays for amba devices and their consumers.
> >>
> >> Also, maintaining a separate deferred-probe like mechanism is
> >> maintenance headache.
> >>
> >> With this commit, instead of deferring the device addition, we simply
> >> defer the generation of uevents for the device and probing of the device
> >> (because drivers needs PID and CID to match) until the PID and CID
> >> information can be read. This allows us to delete all the amba specific
> >> deferring code and also avoid the arbitrary probing delays.
> >>
> >> Cc: Rob Herring <robh@kernel.org>
> >> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> >> Cc: John Stultz <john.stultz@linaro.org>
> >> Cc: Saravana Kannan <saravanak@google.com>
> >> Cc: Linus Walleij <linus.walleij@linaro.org>
> >> Cc: Sudeep Holla <sudeep.holla@arm.com>
> >> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> >> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> >> Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
> >> Cc: Russell King <linux@armlinux.org.uk>
> >> Signed-off-by: Saravana Kannan <saravanak@google.com>
> >> ---
> >>
> >> v1 -> v2:
> >> - Dropped RFC tag
> >> - Complete rewrite to not use stub devices.
> >>
> >> v2 -> v3:
> >> - Flipped the if() condition for hard-coded periphids.
> >> - Added a stub driver to handle the case where all amba drivers are
> >>    modules loaded by uevents.
> >> - Cc Marek after I realized I forgot to add him.
> >>
> >> v3 -> v4:
> >> - Finally figured out and fixed the issue reported by Kefeng (bus match
> >>    can't return an error other than -EPROBE_DEFER).
> >> - I tested the patch on "V2P-CA15" on qemu
> >> - Marek tested v3, but that was so long ago and the rebase wasn't clean,
> >>    so I didn't include the tested-by.
> >>
> >> Marek/Kefeng,
> >>
> >> Mind giving a Tested-by?
> >
> >
> > Yes, it looks that it still works fine.
> >
> > I've tested it by changing the Exynos power domain driver to
> > initialize from late_initcall. This in turn lead me to a bug in
> > generic pm_domains code in __genpd_dev_pm_attach(), which returns -2
> > if the pm domain driver is not yet registered. After fixing that, I've
> > successfully observed the deferred probe of PL330 driver on Exynos
> > 4210 based boards both with this patch and without (with the old timer
> > based code).

Thanks for testing it again Marek! I was hoping you'll hit the crash
that Sudeep was hitting and it would give me some more clues.

Sudeep,

This makes me think the issue you are seeing is related to your
hardware drivers. Can you look into those please? I'm leaning towards
merging this amba clean up and adding delays (say 1ms) to your
clock/power domain drivers to avoid the crash you are seeing. And then
you can figure out the actual delays needed and update it.

> While preparing a fix for the above issue in genpd I found that it has
> been introduced by your commit 5a46079a9645 ("PM: domains: Delete usage
> of driver_deferred_probe_check_state()"). I didn't analyze it enough,
> but it looks that something is missing there if we are trying to probe
> amba device. I assume that returning -EPROBE_DEFER unconditionally there
> is also not a valid solution?

Yeah, the unconditionally returning -EPROBE_DEFER wouldn't work
because if the supplier is optional but not present, the consumer
driver would never stop waiting for it. I'm looking into issues
similar to the one you saw in other threads [1]. The problem always
boils down to the supplier device's DT node not having "compatible"
property and therefore fw_devlink creating the device link between the
consumer and the supplier's parent.

Basically if the drivers/DT are implemented "properly", you would
never get to the failure case (-2) if the driver is actually present.
I have some other ideas on how to get these to work better (not sure
if it'll be for 100% of the cases), but until I get those ideas sorted
out, I might do a partial revert of the change you mentioned.

[1] - https://lore.kernel.org/lkml/4799738.LvFx2qVVIh@steina-w/

-Saravana

>
>
> >
> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >
> >
> >> -Saravana
> >>
> >>   drivers/amba/bus.c | 317 +++++++++++++++++++++------------------------
> >>   1 file changed, 145 insertions(+), 172 deletions(-)
> >>
> >> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> >> index 0e3ed5eb367b..9590c93b2aea 100644
> >> --- a/drivers/amba/bus.c
> >> +++ b/drivers/amba/bus.c
> >> @@ -130,11 +130,100 @@ static struct attribute *amba_dev_attrs[] = {
> >>   };
> >>   ATTRIBUTE_GROUPS(amba_dev);
> >>   +static int amba_read_periphid(struct amba_device *dev)
> >> +{
> >> +    struct reset_control *rstc;
> >> +    u32 size, pid, cid;
> >> +    void __iomem *tmp;
> >> +    int i, ret;
> >> +
> >> +    ret = dev_pm_domain_attach(&dev->dev, true);
> >> +    if (ret) {
> >> +        dev_dbg(&dev->dev, "can't get PM domain: %d\n", ret);
> >> +        goto err_out;
> >> +    }
> >> +
> >> +    ret = amba_get_enable_pclk(dev);
> >> +    if (ret) {
> >> +        dev_dbg(&dev->dev, "can't get pclk: %d\n", ret);
> >> +        goto err_pm;
> >> +    }
> >> +
> >> +    /*
> >> +     * Find reset control(s) of the amba bus and de-assert them.
> >> +     */
> >> +    rstc =
> >> of_reset_control_array_get_optional_shared(dev->dev.of_node);
> >> +    if (IS_ERR(rstc)) {
> >> +        ret = PTR_ERR(rstc);
> >> +        if (ret != -EPROBE_DEFER)
> >> +            dev_err(&dev->dev, "can't get reset: %d\n", ret);
> >> +        goto err_clk;
> >> +    }
> >> +    reset_control_deassert(rstc);
> >> +    reset_control_put(rstc);
> >> +
> >> +    size = resource_size(&dev->res);
> >> +    tmp = ioremap(dev->res.start, size);
> >> +    if (!tmp) {
> >> +        ret = -ENOMEM;
> >> +        goto err_clk;
> >> +    }
> >> +
> >> +    /*
> >> +     * Read pid and cid based on size of resource
> >> +     * they are located at end of region
> >> +     */
> >> +    for (pid = 0, i = 0; i < 4; i++)
> >> +        pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) << (i * 8);
> >> +    for (cid = 0, i = 0; i < 4; i++)
> >> +        cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << (i * 8);
> >> +
> >> +    if (cid == CORESIGHT_CID) {
> >> +        /* set the base to the start of the last 4k block */
> >> +        void __iomem *csbase = tmp + size - 4096;
> >> +
> >> +        dev->uci.devarch = readl(csbase + UCI_REG_DEVARCH_OFFSET);
> >> +        dev->uci.devtype = readl(csbase + UCI_REG_DEVTYPE_OFFSET) &
> >> 0xff;
> >> +    }
> >> +
> >> +    if (cid == AMBA_CID || cid == CORESIGHT_CID) {
> >> +        dev->periphid = pid;
> >> +        dev->cid = cid;
> >> +    }
> >> +
> >> +    if (!dev->periphid)
> >> +        ret = -ENODEV;
> >> +
> >> +    iounmap(tmp);
> >> +
> >> +err_clk:
> >> +    amba_put_disable_pclk(dev);
> >> +err_pm:
> >> +    dev_pm_domain_detach(&dev->dev, true);
> >> +err_out:
> >> +    return ret;
> >> +}
> >> +
> >>   static int amba_match(struct device *dev, struct device_driver *drv)
> >>   {
> >>       struct amba_device *pcdev = to_amba_device(dev);
> >>       struct amba_driver *pcdrv = to_amba_driver(drv);
> >>   +    if (!pcdev->periphid) {
> >> +        int ret = amba_read_periphid(pcdev);
> >> +
> >> +        /*
> >> +         * Returning any error other than -EPROBE_DEFER from bus match
> >> +         * can cause driver registration failure. So, if there's a
> >> +         * permanent failure in reading pid and cid, simply say that
> >> +         * none of the drivers match.
> >> +         */
> >> +        if (ret)
> >> +            return ret == -EPROBE_DEFER ? ret : 0;
> >> +        dev_set_uevent_suppress(dev, false);
> >> +        kobject_uevent(&dev->kobj, KOBJ_ADD);
> >> +    }
> >> +
> >>       /* When driver_override is set, only bind to the matching
> >> driver */
> >>       if (pcdev->driver_override)
> >>           return !strcmp(pcdev->driver_override, drv->name);
> >> @@ -368,6 +457,42 @@ static int __init amba_init(void)
> >>     postcore_initcall(amba_init);
> >>   +static int amba_proxy_probe(struct amba_device *adev,
> >> +                const struct amba_id *id)
> >> +{
> >> +    WARN(1, "Stub driver should never match any device.\n");
> >> +    return -ENODEV;
> >> +}
> >> +
> >> +static const struct amba_id amba_stub_drv_ids[] = {
> >> +    { 0, 0 },
> >> +};
> >> +
> >> +static struct amba_driver amba_proxy_drv = {
> >> +    .drv = {
> >> +        .name = "amba-proxy",
> >> +    },
> >> +    .probe = amba_proxy_probe,
> >> +    .id_table = amba_stub_drv_ids,
> >> +};
> >> +
> >> +static int __init amba_stub_drv_init(void)
> >> +{
> >> +    if (!IS_ENABLED(CONFIG_MODULES))
> >> +        return 0;
> >> +
> >> +    /*
> >> +     * The amba_match() function will get called only if there is at
> >> least
> >> +     * one amba driver registered. If all amba drivers are modules
> >> and are
> >> +     * only loaded based on uevents, then we'll hit a chicken-and-egg
> >> +     * situation where amba_match() is waiting on drivers and
> >> drivers are
> >> +     * waiting on amba_match(). So, register a stub driver to make sure
> >> +     * amba_match() is called even if no amba driver has been
> >> registered.
> >> +     */
> >> +    return amba_driver_register(&amba_proxy_drv);
> >> +}
> >> +late_initcall_sync(amba_stub_drv_init);
> >> +
> >>   /**
> >>    *    amba_driver_register - register an AMBA device driver
> >>    *    @drv: amba device driver structure
> >> @@ -410,160 +535,6 @@ static void amba_device_release(struct device
> >> *dev)
> >>       kfree(d);
> >>   }
> >>   -static int amba_read_periphid(struct amba_device *dev)
> >> -{
> >> -    struct reset_control *rstc;
> >> -    u32 size, pid, cid;
> >> -    void __iomem *tmp;
> >> -    int i, ret;
> >> -
> >> -    ret = dev_pm_domain_attach(&dev->dev, true);
> >> -    if (ret)
> >> -        goto err_out;
> >> -
> >> -    ret = amba_get_enable_pclk(dev);
> >> -    if (ret)
> >> -        goto err_pm;
> >> -
> >> -    /*
> >> -     * Find reset control(s) of the amba bus and de-assert them.
> >> -     */
> >> -    rstc =
> >> of_reset_control_array_get_optional_shared(dev->dev.of_node);
> >> -    if (IS_ERR(rstc)) {
> >> -        ret = PTR_ERR(rstc);
> >> -        if (ret != -EPROBE_DEFER)
> >> -            dev_err(&dev->dev, "can't get reset: %d\n", ret);
> >> -        goto err_clk;
> >> -    }
> >> -    reset_control_deassert(rstc);
> >> -    reset_control_put(rstc);
> >> -
> >> -    size = resource_size(&dev->res);
> >> -    tmp = ioremap(dev->res.start, size);
> >> -    if (!tmp) {
> >> -        ret = -ENOMEM;
> >> -        goto err_clk;
> >> -    }
> >> -
> >> -    /*
> >> -     * Read pid and cid based on size of resource
> >> -     * they are located at end of region
> >> -     */
> >> -    for (pid = 0, i = 0; i < 4; i++)
> >> -        pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) << (i * 8);
> >> -    for (cid = 0, i = 0; i < 4; i++)
> >> -        cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << (i * 8);
> >> -
> >> -    if (cid == CORESIGHT_CID) {
> >> -        /* set the base to the start of the last 4k block */
> >> -        void __iomem *csbase = tmp + size - 4096;
> >> -
> >> -        dev->uci.devarch = readl(csbase + UCI_REG_DEVARCH_OFFSET);
> >> -        dev->uci.devtype = readl(csbase + UCI_REG_DEVTYPE_OFFSET) &
> >> 0xff;
> >> -    }
> >> -
> >> -    if (cid == AMBA_CID || cid == CORESIGHT_CID) {
> >> -        dev->periphid = pid;
> >> -        dev->cid = cid;
> >> -    }
> >> -
> >> -    if (!dev->periphid)
> >> -        ret = -ENODEV;
> >> -
> >> -    iounmap(tmp);
> >> -
> >> -err_clk:
> >> -    amba_put_disable_pclk(dev);
> >> -err_pm:
> >> -    dev_pm_domain_detach(&dev->dev, true);
> >> -err_out:
> >> -    return ret;
> >> -}
> >> -
> >> -static int amba_device_try_add(struct amba_device *dev, struct
> >> resource *parent)
> >> -{
> >> -    int ret;
> >> -
> >> -    ret = request_resource(parent, &dev->res);
> >> -    if (ret)
> >> -        goto err_out;
> >> -
> >> -    /* Hard-coded primecell ID instead of plug-n-play */
> >> -    if (dev->periphid != 0)
> >> -        goto skip_probe;
> >> -
> >> -    ret = amba_read_periphid(dev);
> >> -    if (ret) {
> >> -        if (ret != -EPROBE_DEFER) {
> >> -            amba_device_put(dev);
> >> -            goto err_out;
> >> -        }
> >> -        goto err_release;
> >> -    }
> >> -
> >> -skip_probe:
> >> -    ret = device_add(&dev->dev);
> >> -err_release:
> >> -    if (ret)
> >> -        release_resource(&dev->res);
> >> -err_out:
> >> -    return ret;
> >> -}
> >> -
> >> -/*
> >> - * Registration of AMBA device require reading its pid and cid
> >> registers.
> >> - * To do this, the device must be turned on (if it is a part of
> >> power domain)
> >> - * and have clocks enabled. However in some cases those resources
> >> might not be
> >> - * yet available. Returning EPROBE_DEFER is not a solution in such
> >> case,
> >> - * because callers don't handle this special error code. Instead
> >> such devices
> >> - * are added to the special list and their registration is retried from
> >> - * periodic worker, until all resources are available and
> >> registration succeeds.
> >> - */
> >> -struct deferred_device {
> >> -    struct amba_device *dev;
> >> -    struct resource *parent;
> >> -    struct list_head node;
> >> -};
> >> -
> >> -static LIST_HEAD(deferred_devices);
> >> -static DEFINE_MUTEX(deferred_devices_lock);
> >> -
> >> -static void amba_deferred_retry_func(struct work_struct *dummy);
> >> -static DECLARE_DELAYED_WORK(deferred_retry_work,
> >> amba_deferred_retry_func);
> >> -
> >> -#define DEFERRED_DEVICE_TIMEOUT (msecs_to_jiffies(5 * 1000))
> >> -
> >> -static int amba_deferred_retry(void)
> >> -{
> >> -    struct deferred_device *ddev, *tmp;
> >> -
> >> -    mutex_lock(&deferred_devices_lock);
> >> -
> >> -    list_for_each_entry_safe(ddev, tmp, &deferred_devices, node) {
> >> -        int ret = amba_device_try_add(ddev->dev, ddev->parent);
> >> -
> >> -        if (ret == -EPROBE_DEFER)
> >> -            continue;
> >> -
> >> -        list_del_init(&ddev->node);
> >> -        kfree(ddev);
> >> -    }
> >> -
> >> -    mutex_unlock(&deferred_devices_lock);
> >> -
> >> -    return 0;
> >> -}
> >> -late_initcall(amba_deferred_retry);
> >> -
> >> -static void amba_deferred_retry_func(struct work_struct *dummy)
> >> -{
> >> -    amba_deferred_retry();
> >> -
> >> -    if (!list_empty(&deferred_devices))
> >> -        schedule_delayed_work(&deferred_retry_work,
> >> -                      DEFERRED_DEVICE_TIMEOUT);
> >> -}
> >> -
> >>   /**
> >>    *    amba_device_add - add a previously allocated AMBA device
> >> structure
> >>    *    @dev: AMBA device allocated by amba_device_alloc
> >> @@ -575,28 +546,30 @@ static void amba_deferred_retry_func(struct
> >> work_struct *dummy)
> >>    */
> >>   int amba_device_add(struct amba_device *dev, struct resource *parent)
> >>   {
> >> -    int ret = amba_device_try_add(dev, parent);
> >> -
> >> -    if (ret == -EPROBE_DEFER) {
> >> -        struct deferred_device *ddev;
> >> -
> >> -        ddev = kmalloc(sizeof(*ddev), GFP_KERNEL);
> >> -        if (!ddev)
> >> -            return -ENOMEM;
> >> +    int ret;
> >>   -        ddev->dev = dev;
> >> -        ddev->parent = parent;
> >> -        ret = 0;
> >> +    ret = request_resource(parent, &dev->res);
> >> +    if (ret)
> >> +        return ret;
> >>   -        mutex_lock(&deferred_devices_lock);
> >> +    /* If primecell ID isn't hard-coded, figure it out */
> >> +    if (!dev->periphid) {
> >> +        /*
> >> +         * AMBA device uevents require reading its pid and cid
> >> +         * registers.  To do this, the device must be on, clocked and
> >> +         * out of reset.  However in some cases those resources might
> >> +         * not yet be available.  If that's the case, we suppress the
> >> +         * generation of uevents until we can read the pid and cid
> >> +         * registers.  See also amba_match().
> >> +         */
> >> +        if (amba_read_periphid(dev))
> >> +            dev_set_uevent_suppress(&dev->dev, true);
> >> +    }
> >>   -        if (list_empty(&deferred_devices))
> >> -            schedule_delayed_work(&deferred_retry_work,
> >> -                          DEFERRED_DEVICE_TIMEOUT);
> >> -        list_add_tail(&ddev->node, &deferred_devices);
> >> +    ret = device_add(&dev->dev);
> >> +    if (ret)
> >> +        release_resource(&dev->res);
> >>   -        mutex_unlock(&deferred_devices_lock);
> >> -    }
> >>       return ret;
> >>   }
> >>   EXPORT_SYMBOL_GPL(amba_device_add);
> >
> > Best regards
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

Re: [PATCH v4] amba: Remove deferred device addition

[Saravana Kannan]

On Tue, Jul 12, 2022 at 12:38 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Tue, Jul 12, 2022 at 5:34 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
> >
> > Hi Saravana,
> >
> > On 12.07.2022 14:25, Marek Szyprowski wrote:
> > > Hi Saravana,
> > >
> > > On 05.07.2022 10:39, Saravana Kannan wrote:
> > >> The uevents generated for an amba device need PID and CID information
> > >> that's available only when the amba device is powered on, clocked and
> > >> out of reset. So, if those resources aren't available, the information
> > >> can't be read to generate the uevents. To workaround this requirement,
> > >> if the resources weren't available, the device addition was deferred and
> > >> retried periodically.
> > >>
> > >> However, this deferred addition retry isn't based on resources becoming
> > >> available. Instead, it's retried every 5 seconds and causes arbitrary
> > >> probe delays for amba devices and their consumers.
> > >>
> > >> Also, maintaining a separate deferred-probe like mechanism is
> > >> maintenance headache.
> > >>
> > >> With this commit, instead of deferring the device addition, we simply
> > >> defer the generation of uevents for the device and probing of the device
> > >> (because drivers needs PID and CID to match) until the PID and CID
> > >> information can be read. This allows us to delete all the amba specific
> > >> deferring code and also avoid the arbitrary probing delays.
> > >>
> > >> Cc: Rob Herring <robh@kernel.org>
> > >> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > >> Cc: John Stultz <john.stultz@linaro.org>
> > >> Cc: Saravana Kannan <saravanak@google.com>
> > >> Cc: Linus Walleij <linus.walleij@linaro.org>
> > >> Cc: Sudeep Holla <sudeep.holla@arm.com>
> > >> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > >> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > >> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> > >> Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
> > >> Cc: Russell King <linux@armlinux.org.uk>
> > >> Signed-off-by: Saravana Kannan <saravanak@google.com>
> > >> ---
> > >>
> > >> v1 -> v2:
> > >> - Dropped RFC tag
> > >> - Complete rewrite to not use stub devices.
> > >>
> > >> v2 -> v3:
> > >> - Flipped the if() condition for hard-coded periphids.
> > >> - Added a stub driver to handle the case where all amba drivers are
> > >>    modules loaded by uevents.
> > >> - Cc Marek after I realized I forgot to add him.
> > >>
> > >> v3 -> v4:
> > >> - Finally figured out and fixed the issue reported by Kefeng (bus match
> > >>    can't return an error other than -EPROBE_DEFER).
> > >> - I tested the patch on "V2P-CA15" on qemu
> > >> - Marek tested v3, but that was so long ago and the rebase wasn't clean,
> > >>    so I didn't include the tested-by.
> > >>
> > >> Marek/Kefeng,
> > >>
> > >> Mind giving a Tested-by?
> > >
> > >
> > > Yes, it looks that it still works fine.
> > >
> > > I've tested it by changing the Exynos power domain driver to
> > > initialize from late_initcall. This in turn lead me to a bug in
> > > generic pm_domains code in __genpd_dev_pm_attach(), which returns -2
> > > if the pm domain driver is not yet registered. After fixing that, I've
> > > successfully observed the deferred probe of PL330 driver on Exynos
> > > 4210 based boards both with this patch and without (with the old timer
> > > based code).
>
> Thanks for testing it again Marek! I was hoping you'll hit the crash
> that Sudeep was hitting and it would give me some more clues.
>
> Sudeep,
>
> This makes me think the issue you are seeing is related to your
> hardware drivers. Can you look into those please? I'm leaning towards
> merging this amba clean up and adding delays (say 1ms) to your
> clock/power domain drivers to avoid the crash you are seeing. And then
> you can figure out the actual delays needed and update it.

Sudeep,

Is there a DTS (not dtsi) file that corresponds to the board you were
testing this on?

-Saravana

>
> > While preparing a fix for the above issue in genpd I found that it has
> > been introduced by your commit 5a46079a9645 ("PM: domains: Delete usage
> > of driver_deferred_probe_check_state()"). I didn't analyze it enough,
> > but it looks that something is missing there if we are trying to probe
> > amba device. I assume that returning -EPROBE_DEFER unconditionally there
> > is also not a valid solution?
>
> Yeah, the unconditionally returning -EPROBE_DEFER wouldn't work
> because if the supplier is optional but not present, the consumer
> driver would never stop waiting for it. I'm looking into issues
> similar to the one you saw in other threads [1]. The problem always
> boils down to the supplier device's DT node not having "compatible"
> property and therefore fw_devlink creating the device link between the
> consumer and the supplier's parent.
>
> Basically if the drivers/DT are implemented "properly", you would
> never get to the failure case (-2) if the driver is actually present.
> I have some other ideas on how to get these to work better (not sure
> if it'll be for 100% of the cases), but until I get those ideas sorted
> out, I might do a partial revert of the change you mentioned.
>
> [1] - https://lore.kernel.org/lkml/4799738.LvFx2qVVIh@steina-w/
>
> -Saravana
>
> >
> >
> > >
> > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > >
> > >
> > >> -Saravana
> > >>
> > >>   drivers/amba/bus.c | 317 +++++++++++++++++++++------------------------
> > >>   1 file changed, 145 insertions(+), 172 deletions(-)
> > >>
> > >> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> > >> index 0e3ed5eb367b..9590c93b2aea 100644
> > >> --- a/drivers/amba/bus.c
> > >> +++ b/drivers/amba/bus.c
> > >> @@ -130,11 +130,100 @@ static struct attribute *amba_dev_attrs[] = {
> > >>   };
> > >>   ATTRIBUTE_GROUPS(amba_dev);
> > >>   +static int amba_read_periphid(struct amba_device *dev)
> > >> +{
> > >> +    struct reset_control *rstc;
> > >> +    u32 size, pid, cid;
> > >> +    void __iomem *tmp;
> > >> +    int i, ret;
> > >> +
> > >> +    ret = dev_pm_domain_attach(&dev->dev, true);
> > >> +    if (ret) {
> > >> +        dev_dbg(&dev->dev, "can't get PM domain: %d\n", ret);
> > >> +        goto err_out;
> > >> +    }
> > >> +
> > >> +    ret = amba_get_enable_pclk(dev);
> > >> +    if (ret) {
> > >> +        dev_dbg(&dev->dev, "can't get pclk: %d\n", ret);
> > >> +        goto err_pm;
> > >> +    }
> > >> +
> > >> +    /*
> > >> +     * Find reset control(s) of the amba bus and de-assert them.
> > >> +     */
> > >> +    rstc =
> > >> of_reset_control_array_get_optional_shared(dev->dev.of_node);
> > >> +    if (IS_ERR(rstc)) {
> > >> +        ret = PTR_ERR(rstc);
> > >> +        if (ret != -EPROBE_DEFER)
> > >> +            dev_err(&dev->dev, "can't get reset: %d\n", ret);
> > >> +        goto err_clk;
> > >> +    }
> > >> +    reset_control_deassert(rstc);
> > >> +    reset_control_put(rstc);
> > >> +
> > >> +    size = resource_size(&dev->res);
> > >> +    tmp = ioremap(dev->res.start, size);
> > >> +    if (!tmp) {
> > >> +        ret = -ENOMEM;
> > >> +        goto err_clk;
> > >> +    }
> > >> +
> > >> +    /*
> > >> +     * Read pid and cid based on size of resource
> > >> +     * they are located at end of region
> > >> +     */
> > >> +    for (pid = 0, i = 0; i < 4; i++)
> > >> +        pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) << (i * 8);
> > >> +    for (cid = 0, i = 0; i < 4; i++)
> > >> +        cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << (i * 8);
> > >> +
> > >> +    if (cid == CORESIGHT_CID) {
> > >> +        /* set the base to the start of the last 4k block */
> > >> +        void __iomem *csbase = tmp + size - 4096;
> > >> +
> > >> +        dev->uci.devarch = readl(csbase + UCI_REG_DEVARCH_OFFSET);
> > >> +        dev->uci.devtype = readl(csbase + UCI_REG_DEVTYPE_OFFSET) &
> > >> 0xff;
> > >> +    }
> > >> +
> > >> +    if (cid == AMBA_CID || cid == CORESIGHT_CID) {
> > >> +        dev->periphid = pid;
> > >> +        dev->cid = cid;
> > >> +    }
> > >> +
> > >> +    if (!dev->periphid)
> > >> +        ret = -ENODEV;
> > >> +
> > >> +    iounmap(tmp);
> > >> +
> > >> +err_clk:
> > >> +    amba_put_disable_pclk(dev);
> > >> +err_pm:
> > >> +    dev_pm_domain_detach(&dev->dev, true);
> > >> +err_out:
> > >> +    return ret;
> > >> +}
> > >> +
> > >>   static int amba_match(struct device *dev, struct device_driver *drv)
> > >>   {
> > >>       struct amba_device *pcdev = to_amba_device(dev);
> > >>       struct amba_driver *pcdrv = to_amba_driver(drv);
> > >>   +    if (!pcdev->periphid) {
> > >> +        int ret = amba_read_periphid(pcdev);
> > >> +
> > >> +        /*
> > >> +         * Returning any error other than -EPROBE_DEFER from bus match
> > >> +         * can cause driver registration failure. So, if there's a
> > >> +         * permanent failure in reading pid and cid, simply say that
> > >> +         * none of the drivers match.
> > >> +         */
> > >> +        if (ret)
> > >> +            return ret == -EPROBE_DEFER ? ret : 0;
> > >> +        dev_set_uevent_suppress(dev, false);
> > >> +        kobject_uevent(&dev->kobj, KOBJ_ADD);
> > >> +    }
> > >> +
> > >>       /* When driver_override is set, only bind to the matching
> > >> driver */
> > >>       if (pcdev->driver_override)
> > >>           return !strcmp(pcdev->driver_override, drv->name);
> > >> @@ -368,6 +457,42 @@ static int __init amba_init(void)
> > >>     postcore_initcall(amba_init);
> > >>   +static int amba_proxy_probe(struct amba_device *adev,
> > >> +                const struct amba_id *id)
> > >> +{
> > >> +    WARN(1, "Stub driver should never match any device.\n");
> > >> +    return -ENODEV;
> > >> +}
> > >> +
> > >> +static const struct amba_id amba_stub_drv_ids[] = {
> > >> +    { 0, 0 },
> > >> +};
> > >> +
> > >> +static struct amba_driver amba_proxy_drv = {
> > >> +    .drv = {
> > >> +        .name = "amba-proxy",
> > >> +    },
> > >> +    .probe = amba_proxy_probe,
> > >> +    .id_table = amba_stub_drv_ids,
> > >> +};
> > >> +
> > >> +static int __init amba_stub_drv_init(void)
> > >> +{
> > >> +    if (!IS_ENABLED(CONFIG_MODULES))
> > >> +        return 0;
> > >> +
> > >> +    /*
> > >> +     * The amba_match() function will get called only if there is at
> > >> least
> > >> +     * one amba driver registered. If all amba drivers are modules
> > >> and are
> > >> +     * only loaded based on uevents, then we'll hit a chicken-and-egg
> > >> +     * situation where amba_match() is waiting on drivers and
> > >> drivers are
> > >> +     * waiting on amba_match(). So, register a stub driver to make sure
> > >> +     * amba_match() is called even if no amba driver has been
> > >> registered.
> > >> +     */
> > >> +    return amba_driver_register(&amba_proxy_drv);
> > >> +}
> > >> +late_initcall_sync(amba_stub_drv_init);
> > >> +
> > >>   /**
> > >>    *    amba_driver_register - register an AMBA device driver
> > >>    *    @drv: amba device driver structure
> > >> @@ -410,160 +535,6 @@ static void amba_device_release(struct device
> > >> *dev)
> > >>       kfree(d);
> > >>   }
> > >>   -static int amba_read_periphid(struct amba_device *dev)
> > >> -{
> > >> -    struct reset_control *rstc;
> > >> -    u32 size, pid, cid;
> > >> -    void __iomem *tmp;
> > >> -    int i, ret;
> > >> -
> > >> -    ret = dev_pm_domain_attach(&dev->dev, true);
> > >> -    if (ret)
> > >> -        goto err_out;
> > >> -
> > >> -    ret = amba_get_enable_pclk(dev);
> > >> -    if (ret)
> > >> -        goto err_pm;
> > >> -
> > >> -    /*
> > >> -     * Find reset control(s) of the amba bus and de-assert them.
> > >> -     */
> > >> -    rstc =
> > >> of_reset_control_array_get_optional_shared(dev->dev.of_node);
> > >> -    if (IS_ERR(rstc)) {
> > >> -        ret = PTR_ERR(rstc);
> > >> -        if (ret != -EPROBE_DEFER)
> > >> -            dev_err(&dev->dev, "can't get reset: %d\n", ret);
> > >> -        goto err_clk;
> > >> -    }
> > >> -    reset_control_deassert(rstc);
> > >> -    reset_control_put(rstc);
> > >> -
> > >> -    size = resource_size(&dev->res);
> > >> -    tmp = ioremap(dev->res.start, size);
> > >> -    if (!tmp) {
> > >> -        ret = -ENOMEM;
> > >> -        goto err_clk;
> > >> -    }
> > >> -
> > >> -    /*
> > >> -     * Read pid and cid based on size of resource
> > >> -     * they are located at end of region
> > >> -     */
> > >> -    for (pid = 0, i = 0; i < 4; i++)
> > >> -        pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) << (i * 8);
> > >> -    for (cid = 0, i = 0; i < 4; i++)
> > >> -        cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << (i * 8);
> > >> -
> > >> -    if (cid == CORESIGHT_CID) {
> > >> -        /* set the base to the start of the last 4k block */
> > >> -        void __iomem *csbase = tmp + size - 4096;
> > >> -
> > >> -        dev->uci.devarch = readl(csbase + UCI_REG_DEVARCH_OFFSET);
> > >> -        dev->uci.devtype = readl(csbase + UCI_REG_DEVTYPE_OFFSET) &
> > >> 0xff;
> > >> -    }
> > >> -
> > >> -    if (cid == AMBA_CID || cid == CORESIGHT_CID) {
> > >> -        dev->periphid = pid;
> > >> -        dev->cid = cid;
> > >> -    }
> > >> -
> > >> -    if (!dev->periphid)
> > >> -        ret = -ENODEV;
> > >> -
> > >> -    iounmap(tmp);
> > >> -
> > >> -err_clk:
> > >> -    amba_put_disable_pclk(dev);
> > >> -err_pm:
> > >> -    dev_pm_domain_detach(&dev->dev, true);
> > >> -err_out:
> > >> -    return ret;
> > >> -}
> > >> -
> > >> -static int amba_device_try_add(struct amba_device *dev, struct
> > >> resource *parent)
> > >> -{
> > >> -    int ret;
> > >> -
> > >> -    ret = request_resource(parent, &dev->res);
> > >> -    if (ret)
> > >> -        goto err_out;
> > >> -
> > >> -    /* Hard-coded primecell ID instead of plug-n-play */
> > >> -    if (dev->periphid != 0)
> > >> -        goto skip_probe;
> > >> -
> > >> -    ret = amba_read_periphid(dev);
> > >> -    if (ret) {
> > >> -        if (ret != -EPROBE_DEFER) {
> > >> -            amba_device_put(dev);
> > >> -            goto err_out;
> > >> -        }
> > >> -        goto err_release;
> > >> -    }
> > >> -
> > >> -skip_probe:
> > >> -    ret = device_add(&dev->dev);
> > >> -err_release:
> > >> -    if (ret)
> > >> -        release_resource(&dev->res);
> > >> -err_out:
> > >> -    return ret;
> > >> -}
> > >> -
> > >> -/*
> > >> - * Registration of AMBA device require reading its pid and cid
> > >> registers.
> > >> - * To do this, the device must be turned on (if it is a part of
> > >> power domain)
> > >> - * and have clocks enabled. However in some cases those resources
> > >> might not be
> > >> - * yet available. Returning EPROBE_DEFER is not a solution in such
> > >> case,
> > >> - * because callers don't handle this special error code. Instead
> > >> such devices
> > >> - * are added to the special list and their registration is retried from
> > >> - * periodic worker, until all resources are available and
> > >> registration succeeds.
> > >> - */
> > >> -struct deferred_device {
> > >> -    struct amba_device *dev;
> > >> -    struct resource *parent;
> > >> -    struct list_head node;
> > >> -};
> > >> -
> > >> -static LIST_HEAD(deferred_devices);
> > >> -static DEFINE_MUTEX(deferred_devices_lock);
> > >> -
> > >> -static void amba_deferred_retry_func(struct work_struct *dummy);
> > >> -static DECLARE_DELAYED_WORK(deferred_retry_work,
> > >> amba_deferred_retry_func);
> > >> -
> > >> -#define DEFERRED_DEVICE_TIMEOUT (msecs_to_jiffies(5 * 1000))
> > >> -
> > >> -static int amba_deferred_retry(void)
> > >> -{
> > >> -    struct deferred_device *ddev, *tmp;
> > >> -
> > >> -    mutex_lock(&deferred_devices_lock);
> > >> -
> > >> -    list_for_each_entry_safe(ddev, tmp, &deferred_devices, node) {
> > >> -        int ret = amba_device_try_add(ddev->dev, ddev->parent);
> > >> -
> > >> -        if (ret == -EPROBE_DEFER)
> > >> -            continue;
> > >> -
> > >> -        list_del_init(&ddev->node);
> > >> -        kfree(ddev);
> > >> -    }
> > >> -
> > >> -    mutex_unlock(&deferred_devices_lock);
> > >> -
> > >> -    return 0;
> > >> -}
> > >> -late_initcall(amba_deferred_retry);
> > >> -
> > >> -static void amba_deferred_retry_func(struct work_struct *dummy)
> > >> -{
> > >> -    amba_deferred_retry();
> > >> -
> > >> -    if (!list_empty(&deferred_devices))
> > >> -        schedule_delayed_work(&deferred_retry_work,
> > >> -                      DEFERRED_DEVICE_TIMEOUT);
> > >> -}
> > >> -
> > >>   /**
> > >>    *    amba_device_add - add a previously allocated AMBA device
> > >> structure
> > >>    *    @dev: AMBA device allocated by amba_device_alloc
> > >> @@ -575,28 +546,30 @@ static void amba_deferred_retry_func(struct
> > >> work_struct *dummy)
> > >>    */
> > >>   int amba_device_add(struct amba_device *dev, struct resource *parent)
> > >>   {
> > >> -    int ret = amba_device_try_add(dev, parent);
> > >> -
> > >> -    if (ret == -EPROBE_DEFER) {
> > >> -        struct deferred_device *ddev;
> > >> -
> > >> -        ddev = kmalloc(sizeof(*ddev), GFP_KERNEL);
> > >> -        if (!ddev)
> > >> -            return -ENOMEM;
> > >> +    int ret;
> > >>   -        ddev->dev = dev;
> > >> -        ddev->parent = parent;
> > >> -        ret = 0;
> > >> +    ret = request_resource(parent, &dev->res);
> > >> +    if (ret)
> > >> +        return ret;
> > >>   -        mutex_lock(&deferred_devices_lock);
> > >> +    /* If primecell ID isn't hard-coded, figure it out */
> > >> +    if (!dev->periphid) {
> > >> +        /*
> > >> +         * AMBA device uevents require reading its pid and cid
> > >> +         * registers.  To do this, the device must be on, clocked and
> > >> +         * out of reset.  However in some cases those resources might
> > >> +         * not yet be available.  If that's the case, we suppress the
> > >> +         * generation of uevents until we can read the pid and cid
> > >> +         * registers.  See also amba_match().
> > >> +         */
> > >> +        if (amba_read_periphid(dev))
> > >> +            dev_set_uevent_suppress(&dev->dev, true);
> > >> +    }
> > >>   -        if (list_empty(&deferred_devices))
> > >> -            schedule_delayed_work(&deferred_retry_work,
> > >> -                          DEFERRED_DEVICE_TIMEOUT);
> > >> -        list_add_tail(&ddev->node, &deferred_devices);
> > >> +    ret = device_add(&dev->dev);
> > >> +    if (ret)
> > >> +        release_resource(&dev->res);
> > >>   -        mutex_unlock(&deferred_devices_lock);
> > >> -    }
> > >>       return ret;
> > >>   }
> > >>   EXPORT_SYMBOL_GPL(amba_device_add);
> > >
> > > Best regards
> >
> > Best regards
> > --
> > Marek Szyprowski, PhD
> > Samsung R&D Institute Poland
> >

Re: [PATCH v4] amba: Remove deferred device addition

[Marek Szyprowski]

Hi Saravana,

On 12.07.2022 21:38, Saravana Kannan wrote:
> On Tue, Jul 12, 2022 at 5:34 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 12.07.2022 14:25, Marek Szyprowski wrote:
>>> On 05.07.2022 10:39, Saravana Kannan wrote:
>>>> The uevents generated for an amba device need PID and CID information
>>>> that's available only when the amba device is powered on, clocked and
>>>> out of reset. So, if those resources aren't available, the information
>>>> can't be read to generate the uevents. To workaround this requirement,
>>>> if the resources weren't available, the device addition was deferred and
>>>> retried periodically.
>>>>
>>>> However, this deferred addition retry isn't based on resources becoming
>>>> available. Instead, it's retried every 5 seconds and causes arbitrary
>>>> probe delays for amba devices and their consumers.
>>>>
>>>> Also, maintaining a separate deferred-probe like mechanism is
>>>> maintenance headache.
>>>>
>>>> With this commit, instead of deferring the device addition, we simply
>>>> defer the generation of uevents for the device and probing of the device
>>>> (because drivers needs PID and CID to match) until the PID and CID
>>>> information can be read. This allows us to delete all the amba specific
>>>> deferring code and also avoid the arbitrary probing delays.
>>>>
>>>> Cc: Rob Herring <robh@kernel.org>
>>>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>>>> Cc: John Stultz <john.stultz@linaro.org>
>>>> Cc: Saravana Kannan <saravanak@google.com>
>>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>>>> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>> Cc: Russell King <linux@armlinux.org.uk>
>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>> - Dropped RFC tag
>>>> - Complete rewrite to not use stub devices.
>>>>
>>>> v2 -> v3:
>>>> - Flipped the if() condition for hard-coded periphids.
>>>> - Added a stub driver to handle the case where all amba drivers are
>>>>     modules loaded by uevents.
>>>> - Cc Marek after I realized I forgot to add him.
>>>>
>>>> v3 -> v4:
>>>> - Finally figured out and fixed the issue reported by Kefeng (bus match
>>>>     can't return an error other than -EPROBE_DEFER).
>>>> - I tested the patch on "V2P-CA15" on qemu
>>>> - Marek tested v3, but that was so long ago and the rebase wasn't clean,
>>>>     so I didn't include the tested-by.
>>>>
>>>> Marek/Kefeng,
>>>>
>>>> Mind giving a Tested-by?
>>>
>>> Yes, it looks that it still works fine.
>>>
>>> I've tested it by changing the Exynos power domain driver to
>>> initialize from late_initcall. This in turn lead me to a bug in
>>> generic pm_domains code in __genpd_dev_pm_attach(), which returns -2
>>> if the pm domain driver is not yet registered. After fixing that, I've
>>> successfully observed the deferred probe of PL330 driver on Exynos
>>> 4210 based boards both with this patch and without (with the old timer
>>> based code).
> Thanks for testing it again Marek! I was hoping you'll hit the crash
> that Sudeep was hitting and it would give me some more clues.
>
> Sudeep,
>
> This makes me think the issue you are seeing is related to your
> hardware drivers. Can you look into those please? I'm leaning towards
> merging this amba clean up and adding delays (say 1ms) to your
> clock/power domain drivers to avoid the crash you are seeing. And then
> you can figure out the actual delays needed and update it.
>
>> While preparing a fix for the above issue in genpd I found that it has
>> been introduced by your commit 5a46079a9645 ("PM: domains: Delete usage
>> of driver_deferred_probe_check_state()"). I didn't analyze it enough,
>> but it looks that something is missing there if we are trying to probe
>> amba device. I assume that returning -EPROBE_DEFER unconditionally there
>> is also not a valid solution?
> Yeah, the unconditionally returning -EPROBE_DEFER wouldn't work
> because if the supplier is optional but not present, the consumer
> driver would never stop waiting for it. I'm looking into issues
> similar to the one you saw in other threads [1]. The problem always
> boils down to the supplier device's DT node not having "compatible"
> property and therefore fw_devlink creating the device link between the
> consumer and the supplier's parent.
>
> Basically if the drivers/DT are implemented "properly", you would
> never get to the failure case (-2) if the driver is actually present.
Well, I don't get what do you mean by not having the proper 'comaptible' 
property. Both affected devices (amba's pl330 and its power domain) have 
compatible strings: 'arm,pl330' and 'samsung,exynos4210-pd', but the 
devlinks doesn't help. Is it related to the custom device addition code 
in the amba bus?
> I have some other ideas on how to get these to work better (not sure
> if it'll be for 100% of the cases), but until I get those ideas sorted
> out, I might do a partial revert of the change you mentioned.
>
> [1] - https://lore.kernel.org/lkml/4799738.LvFx2qVVIh@steina-w/

 > ...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v4] amba: Remove deferred device addition

[Sudeep Holla]

On Tue, Jul 12, 2022 at 12:38:33PM -0700, Saravana Kannan wrote:
> Sudeep,
> 
> This makes me think the issue you are seeing is related to your
> hardware drivers. Can you look into those please? I'm leaning towards
> merging this amba clean up and adding delays (say 1ms) to your
> clock/power domain drivers to avoid the crash you are seeing. And then
> you can figure out the actual delays needed and update it.

I haven't got a chance to debug the issue on Juno much further. One thing
about the platform is that we can't turn off the debug power domain that
most of the coresight devices share.

One thing I also observed with -next+this patch is that with a little log
it can access the registers while adding first few devices and then crash
which doesn't align with platform behaviour as we can't turn off the domain
though we attached and turn on in amba_read_periphid and then turn off and
detach the power domain. Ideally if first device amba_read_periphid was
successful, it must be the case for all, but I see different behaviour.

I need to check again to confirm if it is issue with platform power domain
driver. It is based on SCMI so there is some role played by the f/w as well.

--
Regards,
Sudeep

Re: [PATCH v4] amba: Remove deferred device addition

[Sudeep Holla]

On Tue, Jul 12, 2022 at 12:53:20PM -0700, Saravana Kannan wrote:
> Sudeep,
> 
> Is there a DTS (not dtsi) file that corresponds to the board you were
> testing this on?
> 

arch/arm64/boot/dts/arm/juno-r2-scmi.dts

-- 
Regards,
Sudeep

Re: [PATCH v4] amba: Remove deferred device addition

[Saravana Kannan]

On Tue, Jul 12, 2022 at 11:53 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Saravana,
>
> On 12.07.2022 21:38, Saravana Kannan wrote:
> > On Tue, Jul 12, 2022 at 5:34 AM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >> On 12.07.2022 14:25, Marek Szyprowski wrote:
> >>> On 05.07.2022 10:39, Saravana Kannan wrote:
> >>>> The uevents generated for an amba device need PID and CID information
> >>>> that's available only when the amba device is powered on, clocked and
> >>>> out of reset. So, if those resources aren't available, the information
> >>>> can't be read to generate the uevents. To workaround this requirement,
> >>>> if the resources weren't available, the device addition was deferred and
> >>>> retried periodically.
> >>>>
> >>>> However, this deferred addition retry isn't based on resources becoming
> >>>> available. Instead, it's retried every 5 seconds and causes arbitrary
> >>>> probe delays for amba devices and their consumers.
> >>>>
> >>>> Also, maintaining a separate deferred-probe like mechanism is
> >>>> maintenance headache.
> >>>>
> >>>> With this commit, instead of deferring the device addition, we simply
> >>>> defer the generation of uevents for the device and probing of the device
> >>>> (because drivers needs PID and CID to match) until the PID and CID
> >>>> information can be read. This allows us to delete all the amba specific
> >>>> deferring code and also avoid the arbitrary probing delays.
> >>>>
> >>>> Cc: Rob Herring <robh@kernel.org>
> >>>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> >>>> Cc: John Stultz <john.stultz@linaro.org>
> >>>> Cc: Saravana Kannan <saravanak@google.com>
> >>>> Cc: Linus Walleij <linus.walleij@linaro.org>
> >>>> Cc: Sudeep Holla <sudeep.holla@arm.com>
> >>>> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> >>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> >>>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> >>>> Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
> >>>> Cc: Russell King <linux@armlinux.org.uk>
> >>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> >>>> ---
> >>>>
> >>>> v1 -> v2:
> >>>> - Dropped RFC tag
> >>>> - Complete rewrite to not use stub devices.
> >>>>
> >>>> v2 -> v3:
> >>>> - Flipped the if() condition for hard-coded periphids.
> >>>> - Added a stub driver to handle the case where all amba drivers are
> >>>>     modules loaded by uevents.
> >>>> - Cc Marek after I realized I forgot to add him.
> >>>>
> >>>> v3 -> v4:
> >>>> - Finally figured out and fixed the issue reported by Kefeng (bus match
> >>>>     can't return an error other than -EPROBE_DEFER).
> >>>> - I tested the patch on "V2P-CA15" on qemu
> >>>> - Marek tested v3, but that was so long ago and the rebase wasn't clean,
> >>>>     so I didn't include the tested-by.
> >>>>
> >>>> Marek/Kefeng,
> >>>>
> >>>> Mind giving a Tested-by?
> >>>
> >>> Yes, it looks that it still works fine.
> >>>
> >>> I've tested it by changing the Exynos power domain driver to
> >>> initialize from late_initcall. This in turn lead me to a bug in
> >>> generic pm_domains code in __genpd_dev_pm_attach(), which returns -2
> >>> if the pm domain driver is not yet registered. After fixing that, I've
> >>> successfully observed the deferred probe of PL330 driver on Exynos
> >>> 4210 based boards both with this patch and without (with the old timer
> >>> based code).
> > Thanks for testing it again Marek! I was hoping you'll hit the crash
> > that Sudeep was hitting and it would give me some more clues.
> >
> > Sudeep,
> >
> > This makes me think the issue you are seeing is related to your
> > hardware drivers. Can you look into those please? I'm leaning towards
> > merging this amba clean up and adding delays (say 1ms) to your
> > clock/power domain drivers to avoid the crash you are seeing. And then
> > you can figure out the actual delays needed and update it.
> >
> >> While preparing a fix for the above issue in genpd I found that it has
> >> been introduced by your commit 5a46079a9645 ("PM: domains: Delete usage
> >> of driver_deferred_probe_check_state()"). I didn't analyze it enough,
> >> but it looks that something is missing there if we are trying to probe
> >> amba device. I assume that returning -EPROBE_DEFER unconditionally there
> >> is also not a valid solution?
> > Yeah, the unconditionally returning -EPROBE_DEFER wouldn't work
> > because if the supplier is optional but not present, the consumer
> > driver would never stop waiting for it. I'm looking into issues
> > similar to the one you saw in other threads [1]. The problem always
> > boils down to the supplier device's DT node not having "compatible"
> > property and therefore fw_devlink creating the device link between the
> > consumer and the supplier's parent.
> >
> > Basically if the drivers/DT are implemented "properly", you would
> > never get to the failure case (-2) if the driver is actually present.
> Well, I don't get what do you mean by not having the proper 'comaptible'
> property. Both affected devices (amba's pl330 and its power domain) have
> compatible strings: 'arm,pl330' and 'samsung,exynos4210-pd', but the
> devlinks doesn't help. Is it related to the custom device addition code
> in the amba bus?

Thanks for pointing this out Marek. This is an interaction between the
two separate series I sent out.

TL;DR is that device links don't block bus->match() attempts. That's
the reason. That's a separate optimization that's in my todo list for
a while.

Longer explanation follows:

5a46079a9645 ("PM: domains: Delete usage of
driver_deferred_probe_check_state()") correctly assumed fw_devlink
will block calls to __genpd_dev_pm_attach() before the power domain
has probed or we have given up waiting on suppliers at the driver core
level. So, __genpd_dev_pm_attach() returning -2 was not a problem
(well, there are other issues, but we'll pretend they don't exist for
now).

Until this amba patch, that was true because really_probe() calls
device_links_check_suppliers() before you'll get anywhere near
__genpd_dev_pm_attach().

But with this amba patch, we try to get power domains before we get to
really_probe() and that doesn't get the device links check. So,
amba_match() has to always return -EPROBE_DEFER on any error until we
optimize out match() calls for devices whose suppliers aren't ready
yet. I'm considering reverting 5a46079a9645 due to other issues, so I
think v4 might be okay as is.

-Saravana

> > I have some other ideas on how to get these to work better (not sure
> > if it'll be for 100% of the cases), but until I get those ideas sorted
> > out, I might do a partial revert of the change you mentioned.
> >
> > [1] - https://lore.kernel.org/lkml/4799738.LvFx2qVVIh@steina-w/
>
>  > ...
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

Re: [PATCH v4] amba: Remove deferred device addition

[Saravana Kannan]

On Wed, Jul 13, 2022 at 7:58 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Jul 12, 2022 at 12:38:33PM -0700, Saravana Kannan wrote:
> > Sudeep,
> >
> > This makes me think the issue you are seeing is related to your
> > hardware drivers. Can you look into those please? I'm leaning towards
> > merging this amba clean up and adding delays (say 1ms) to your
> > clock/power domain drivers to avoid the crash you are seeing. And then
> > you can figure out the actual delays needed and update it.
>
> I haven't got a chance to debug the issue on Juno much further. One thing
> about the platform is that we can't turn off the debug power domain that
> most of the coresight devices share.
>
> One thing I also observed with -next+this patch is that with a little log
> it can access the registers while adding first few devices and then crash
> which doesn't align with platform behaviour as we can't turn off the domain
> though we attached and turn on in amba_read_periphid and then turn off and
> detach the power domain. Ideally if first device amba_read_periphid was
> successful, it must be the case for all, but I see different behaviour.
>
> I need to check again to confirm if it is issue with platform power domain
> driver. It is based on SCMI so there is some role played by the f/w as well.

Yeah, this log timing based behavior is what makes me suspect it's not
a problem with this patch itself.

However, just to rule it out, can you try making this change on top of
v4 and give it a shot? This is related to the issue Marek reported,
but those are more about permanent probe failures. Not a crash.

+++ b/drivers/amba/bus.c
@@ -219,7 +219,7 @@ static int amba_match(struct device *dev, struct
device_driver *drv)
                 * none of the drivers match.
                 */
                if (ret)
-                       return ret == -EPROBE_DEFER ? ret : 0;
+                       return -EPROBE_DEFER;
                dev_set_uevent_suppress(dev, false);
                kobject_uevent(&dev->kobj, KOBJ_ADD);
        }

-Saravana

Re: [PATCH v4] amba: Remove deferred device addition

[Sudeep Holla]

On Mon, Jul 18, 2022 at 06:51:29PM -0700, Saravana Kannan wrote:
> On Tue, Jul 12, 2022 at 11:53 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:

[..]

> Longer explanation follows:
> 
> 5a46079a9645 ("PM: domains: Delete usage of
> driver_deferred_probe_check_state()") correctly assumed fw_devlink
> will block calls to __genpd_dev_pm_attach() before the power domain
> has probed or we have given up waiting on suppliers at the driver core
> level. So, __genpd_dev_pm_attach() returning -2 was not a problem
> (well, there are other issues, but we'll pretend they don't exist for
> now).
> 
> Until this amba patch, that was true because really_probe() calls
> device_links_check_suppliers() before you'll get anywhere near
> __genpd_dev_pm_attach().
>

Last time I started looking at this patch, I was suspecting some issue
around __genpd_dev_pm_attach() but your explanation makes sense to me
now and I am more or less convinced this was what happening on Juno.

> But with this amba patch, we try to get power domains before we get to
> really_probe() and that doesn't get the device links check. So,
> amba_match() has to always return -EPROBE_DEFER on any error until we
> optimize out match() calls for devices whose suppliers aren't ready
> yet. I'm considering reverting 5a46079a9645 due to other issues, so I
> think v4 might be okay as is.
>

OK, do I need to check with 5a46079a9645 reverted then ?

-- 
Regards,
Sudeep

Re: [PATCH v4] amba: Remove deferred device addition

[Sudeep Holla]

On Mon, Jul 18, 2022 at 06:55:23PM -0700, Saravana Kannan wrote:
> On Wed, Jul 13, 2022 at 7:58 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Tue, Jul 12, 2022 at 12:38:33PM -0700, Saravana Kannan wrote:
> > > Sudeep,
> > >
> > > This makes me think the issue you are seeing is related to your
> > > hardware drivers. Can you look into those please? I'm leaning towards
> > > merging this amba clean up and adding delays (say 1ms) to your
> > > clock/power domain drivers to avoid the crash you are seeing. And then
> > > you can figure out the actual delays needed and update it.
> >
> > I haven't got a chance to debug the issue on Juno much further. One thing
> > about the platform is that we can't turn off the debug power domain that
> > most of the coresight devices share.
> >
> > One thing I also observed with -next+this patch is that with a little log
> > it can access the registers while adding first few devices and then crash
> > which doesn't align with platform behaviour as we can't turn off the domain
> > though we attached and turn on in amba_read_periphid and then turn off and
> > detach the power domain. Ideally if first device amba_read_periphid was
> > successful, it must be the case for all, but I see different behaviour.
> >
> > I need to check again to confirm if it is issue with platform power domain
> > driver. It is based on SCMI so there is some role played by the f/w as well.
> 
> Yeah, this log timing based behavior is what makes me suspect it's not
> a problem with this patch itself.
> 
> However, just to rule it out, can you try making this change on top of
> v4 and give it a shot? This is related to the issue Marek reported,
> but those are more about permanent probe failures. Not a crash.
>

This patch(version v4, fails to apply on -next but the conflict is trivial
and in the deleted code so I just retained your copy of all the functions)
plus the below change fixes the issue I reported on Juno.

I won't give you tested-by yet as you have plans to revert some things
and I resolved the conflict here though trivial, I prefer to apply the
patch as is with all associated changes and test once more.

Thanks for digging this and fixing it.

-- 
Regards,
Sudeep

Re: [PATCH v4] amba: Remove deferred device addition

[Saravana Kannan]

On Tue, Jul 19, 2022 at 6:39 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Mon, Jul 18, 2022 at 06:55:23PM -0700, Saravana Kannan wrote:
> > On Wed, Jul 13, 2022 at 7:58 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Tue, Jul 12, 2022 at 12:38:33PM -0700, Saravana Kannan wrote:
> > > > Sudeep,
> > > >
> > > > This makes me think the issue you are seeing is related to your
> > > > hardware drivers. Can you look into those please? I'm leaning towards
> > > > merging this amba clean up and adding delays (say 1ms) to your
> > > > clock/power domain drivers to avoid the crash you are seeing. And then
> > > > you can figure out the actual delays needed and update it.
> > >
> > > I haven't got a chance to debug the issue on Juno much further. One thing
> > > about the platform is that we can't turn off the debug power domain that
> > > most of the coresight devices share.
> > >
> > > One thing I also observed with -next+this patch is that with a little log
> > > it can access the registers while adding first few devices and then crash
> > > which doesn't align with platform behaviour as we can't turn off the domain
> > > though we attached and turn on in amba_read_periphid and then turn off and
> > > detach the power domain. Ideally if first device amba_read_periphid was
> > > successful, it must be the case for all, but I see different behaviour.
> > >
> > > I need to check again to confirm if it is issue with platform power domain
> > > driver. It is based on SCMI so there is some role played by the f/w as well.
> >
> > Yeah, this log timing based behavior is what makes me suspect it's not
> > a problem with this patch itself.
> >
> > However, just to rule it out, can you try making this change on top of
> > v4 and give it a shot? This is related to the issue Marek reported,
> > but those are more about permanent probe failures. Not a crash.
> >
>
> This patch(version v4, fails to apply on -next but the conflict is trivial
> and in the deleted code so I just retained your copy of all the functions)
> plus the below change fixes the issue I reported on Juno.

What the heck? I didn't expect it to fix the issue at all. Without the
fix some amba devices could end up not getting probed. But that
shouldn't have caused crashes. So that still indicates some issue in
your driver you might want to look into.

With that said, I'll just roll this fix into a v5 and send it out.
While the revert would fix it, I don't want this to be blocked on that
or be fragile enough to be broken in the future.

-Saravana

>
> I won't give you tested-by yet as you have plans to revert some things
> and I resolved the conflict here though trivial, I prefer to apply the
> patch as is with all associated changes and test once more.
>
> Thanks for digging this and fixing it.
>
> --
> Regards,
> Sudeep