[PATCH RFC 0/3] amba: bus: Move reading periphid operation from amba_match() to amba_probe()

[PATCH RFC 0/3] amba: bus: Move reading periphid operation from amba_match() to amba_probe()

[Zijun Hu]

This patch series is to make amba_match(), as bus_type @amba_bustype's
match(), also follow below ideal rule:

bus_type's match() should only return bool type compatible integer 0 or
1 ideally since its main operations are lookup and comparison normally.

Which has been followed by match() of all other bus_types in current
kernel tree.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Zijun Hu (3):
      amba: bus: Warn on adding an AMBA device without valid periphid
      amba: bus: Move empty @amba_proxy_drv's definition to the front
      amba: bus: Move reading periphid operation from amba_match() to amba_probe()

 drivers/amba/bus.c       | 130 ++++++++++++++++++++++++++---------------------
 include/linux/amba/bus.h |   1 -
 2 files changed, 72 insertions(+), 59 deletions(-)
---
base-commit: 888f67e621dda5c2804a696524e28d0ca4cf0a80
change-id: 20240829-fix_amba-0f09aa1fc3b1

Best regards,
-- 
Zijun Hu <quic_zijuhu@quicinc.com>

[PATCH RFC 1/3] amba: bus: Warn on adding an AMBA device without valid periphid

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

In order to handle rare case that an AMBA device is added without valid
periphid, it is costly for @amba_bustype's match() to try to read periphid
from hardware, so remind user to configure periphid via DT|ACPI as far as
possible by warning message.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/amba/bus.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 34bc880ca20b..cc3c57f83798 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -560,6 +560,9 @@ int amba_device_add(struct amba_device *dev, struct resource *parent)
 
 	/* If primecell ID isn't hard-coded, figure it out */
 	if (!dev->periphid) {
+		dev_warn(&dev->dev,
+			 "Periphid of AMBA device '%s' is not configured, please fix it\n",
+			 dev_name(&dev->dev));
 		/*
 		 * AMBA device uevents require reading its pid and cid
 		 * registers.  To do this, the device must be on, clocked and

-- 
2.34.1

[PATCH RFC 2/3] amba: bus: Move empty @amba_proxy_drv's definition to the front

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

Move empty @amba_proxy_drv's definition to the front in preparation for
referring to it without forward declaration.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/amba/bus.c | 72 +++++++++++++++++++++++++++---------------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index cc3c57f83798..033d626aff46 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -205,6 +205,42 @@ static int amba_read_periphid(struct amba_device *dev)
 	return ret;
 }
 
+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, NULL);
+}
+late_initcall_sync(amba_stub_drv_init);
+
 static int amba_match(struct device *dev, const struct device_driver *drv)
 {
 	struct amba_device *pcdev = to_amba_device(dev);
@@ -456,42 +492,6 @@ 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, NULL);
-}
-late_initcall_sync(amba_stub_drv_init);
-
 /**
  *	__amba_driver_register - register an AMBA device driver
  *	@drv: amba device driver structure

-- 
2.34.1

[PATCH RFC 3/3] amba: bus: Move reading periphid operation from amba_match() to amba_probe()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

amba_match(), as bus_type @amba_bustype's match(), reads periphid from
hardware and may return -EPROBE_DEFER consequently, and it is the only
one that breaks below ideal rule in current kernel tree:

bus_type's match() should only return bool type compatible integer 0 or
1 ideally since its main operations are lookup and comparison normally.

fixed by moving reading periphid operation to amba_probe().

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/amba/bus.c       | 55 +++++++++++++++++++++++++++++-------------------
 include/linux/amba/bus.h |  1 -
 2 files changed, 33 insertions(+), 23 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 033d626aff46..8fe2e054b5ce 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -188,7 +188,7 @@ static int amba_read_periphid(struct amba_device *dev)
 	}
 
 	if (cid == AMBA_CID || cid == CORESIGHT_CID) {
-		dev->periphid = pid;
+		WRITE_ONCE(dev->periphid, pid);
 		dev->cid = cid;
 	}
 
@@ -246,24 +246,14 @@ static int amba_match(struct device *dev, const struct device_driver *drv)
 	struct amba_device *pcdev = to_amba_device(dev);
 	const struct amba_driver *pcdrv = to_amba_driver(drv);
 
-	mutex_lock(&pcdev->periphid_lock);
-	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 map it to
-		 * -EPROBE_DEFER.
-		 */
-		if (ret) {
-			mutex_unlock(&pcdev->periphid_lock);
-			return -EPROBE_DEFER;
-		}
-		dev_set_uevent_suppress(dev, false);
-		kobject_uevent(&dev->kobj, KOBJ_ADD);
-	}
-	mutex_unlock(&pcdev->periphid_lock);
+	/*
+	 * For an AMBA device without valid periphid, only read periphid
+	 * in amba_probe() for it when try to bind @amba_proxy_drv.
+	 * For @pcdev->periphid, Reading here has a little race with
+	 * writing in amba_probe().
+	 */
+	if (!READ_ONCE(pcdev->periphid))
+		return pcdrv == &amba_proxy_drv ? 1 : 0;
 
 	/* When driver_override is set, only bind to the matching driver */
 	if (pcdev->driver_override)
@@ -315,10 +305,24 @@ static int amba_probe(struct device *dev)
 {
 	struct amba_device *pcdev = to_amba_device(dev);
 	struct amba_driver *pcdrv = to_amba_driver(dev->driver);
-	const struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
+	const struct amba_id *id;
 	int ret;
 
 	do {
+		if (!pcdev->periphid) {
+			ret = amba_read_periphid(pcdev);
+			if (ret) {
+				dev_err_probe(dev, ret, "failed to read periphid\n");
+			} else {
+				dev_set_uevent_suppress(dev, false);
+				kobject_uevent(&dev->kobj, KOBJ_ADD);
+			}
+
+			ret = -EPROBE_DEFER;
+			break;
+		}
+		id = amba_lookup(pcdrv->id_table, pcdev);
+
 		ret = of_amba_device_decode_irq(pcdev);
 		if (ret)
 			break;
@@ -389,10 +393,15 @@ static void amba_shutdown(struct device *dev)
 
 static int amba_dma_configure(struct device *dev)
 {
+	struct amba_device *pcdev = to_amba_device(dev);
 	struct amba_driver *drv = to_amba_driver(dev->driver);
 	enum dev_dma_attr attr;
 	int ret = 0;
 
+	/* To successfully go to amba_probe() to read periphid */
+	if (!pcdev->periphid)
+		return 0;
+
 	if (dev->of_node) {
 		ret = of_dma_configure(dev, dev->of_node, true);
 	} else if (has_acpi_companion(dev)) {
@@ -411,8 +420,12 @@ static int amba_dma_configure(struct device *dev)
 
 static void amba_dma_cleanup(struct device *dev)
 {
+	struct amba_device *pcdev = to_amba_device(dev);
 	struct amba_driver *drv = to_amba_driver(dev->driver);
 
+	if (!pcdev->periphid)
+		return;
+
 	if (!drv->driver_managed_dma)
 		iommu_device_unuse_default_domain(dev);
 }
@@ -535,7 +548,6 @@ static void amba_device_release(struct device *dev)
 	fwnode_handle_put(dev_fwnode(&d->dev));
 	if (d->res.parent)
 		release_resource(&d->res);
-	mutex_destroy(&d->periphid_lock);
 	kfree(d);
 }
 
@@ -593,7 +605,6 @@ static void amba_device_initialize(struct amba_device *dev, const char *name)
 	dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
 	dev->dev.dma_parms = &dev->dma_parms;
 	dev->res.name = dev_name(&dev->dev);
-	mutex_init(&dev->periphid_lock);
 }
 
 /**
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index 958a55bcc708..4bb3467d9c3d 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -67,7 +67,6 @@ struct amba_device {
 	struct clk		*pclk;
 	struct device_dma_parameters dma_parms;
 	unsigned int		periphid;
-	struct mutex		periphid_lock;
 	unsigned int		cid;
 	struct amba_cs_uci_id	uci;
 	unsigned int		irq[AMBA_NR_IRQS];

-- 
2.34.1

Re: [PATCH RFC 3/3] amba: bus: Move reading periphid operation from amba_match() to amba_probe()

[Zijun Hu]

On 2024/9/9 07:37, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> amba_match(), as bus_type @amba_bustype's match(), reads periphid from
> hardware and may return -EPROBE_DEFER consequently, and it is the only
> one that breaks below ideal rule in current kernel tree:
> 
> bus_type's match() should only return bool type compatible integer 0 or
> 1 ideally since its main operations are lookup and comparison normally.
> 
> fixed by moving reading periphid operation to amba_probe().

or move to amba_dma_configure() which is the first bus_type's function
called after match()?

> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/amba/bus.c       | 55 +++++++++++++++++++++++++++++-------------------
>  include/linux/amba/bus.h |  1 -
>  2 files changed, 33 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 033d626aff46..8fe2e054b5ce 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -188,7 +188,7 @@ static int amba_read_periphid(struct amba_device *dev)
>  	}
>  
>  	if (cid == AMBA_CID || cid == CORESIGHT_CID) {
> -		dev->periphid = pid;
> +		WRITE_ONCE(dev->periphid, pid);
>  		dev->cid = cid;
>  	}
>  
> @@ -246,24 +246,14 @@ static int amba_match(struct device *dev, const struct device_driver *drv)
>  	struct amba_device *pcdev = to_amba_device(dev);
>  	const struct amba_driver *pcdrv = to_amba_driver(drv);
>  
> -	mutex_lock(&pcdev->periphid_lock);
> -	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 map it to
> -		 * -EPROBE_DEFER.
> -		 */
> -		if (ret) {
> -			mutex_unlock(&pcdev->periphid_lock);
> -			return -EPROBE_DEFER;
> -		}
> -		dev_set_uevent_suppress(dev, false);
> -		kobject_uevent(&dev->kobj, KOBJ_ADD);
> -	}
> -	mutex_unlock(&pcdev->periphid_lock);
> +	/*
> +	 * For an AMBA device without valid periphid, only read periphid
> +	 * in amba_probe() for it when try to bind @amba_proxy_drv.
> +	 * For @pcdev->periphid, Reading here has a little race with
> +	 * writing in amba_probe().
> +	 */
> +	if (!READ_ONCE(pcdev->periphid))
> +		return pcdrv == &amba_proxy_drv ? 1 : 0;
>  
>  	/* When driver_override is set, only bind to the matching driver */
>  	if (pcdev->driver_override)
> @@ -315,10 +305,24 @@ static int amba_probe(struct device *dev)
>  {
>  	struct amba_device *pcdev = to_amba_device(dev);
>  	struct amba_driver *pcdrv = to_amba_driver(dev->driver);
> -	const struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
> +	const struct amba_id *id;
>  	int ret;
>  
>  	do {
> +		if (!pcdev->periphid) {
> +			ret = amba_read_periphid(pcdev);
> +			if (ret) {
> +				dev_err_probe(dev, ret, "failed to read periphid\n");
> +			} else {
> +				dev_set_uevent_suppress(dev, false);
> +				kobject_uevent(&dev->kobj, KOBJ_ADD);
> +			}
> +
> +			ret = -EPROBE_DEFER;
> +			break;
> +		}
> +		id = amba_lookup(pcdrv->id_table, pcdev);
> +

or read periphid in this function later since it does some
preparation for hardware ready to read which also is done by
amba_read_periphid() ?

>  		ret = of_amba_device_decode_irq(pcdev);
>  		if (ret)
>  			break;
> @@ -389,10 +393,15 @@ static void amba_shutdown(struct device *dev)
>  
>  static int amba_dma_configure(struct device *dev)
>  {
> +	struct amba_device *pcdev = to_amba_device(dev);
>  	struct amba_driver *drv = to_amba_driver(dev->driver);
>  	enum dev_dma_attr attr;
>  	int ret = 0;
>  
> +	/* To successfully go to amba_probe() to read periphid */
> +	if (!pcdev->periphid)
> +		return 0;
> +
>  	if (dev->of_node) {
>  		ret = of_dma_configure(dev, dev->of_node, true);
>  	} else if (has_acpi_companion(dev)) {
> @@ -411,8 +420,12 @@ static int amba_dma_configure(struct device *dev)
>  
>  static void amba_dma_cleanup(struct device *dev)
>  {
> +	struct amba_device *pcdev = to_amba_device(dev);
>  	struct amba_driver *drv = to_amba_driver(dev->driver);
>  
> +	if (!pcdev->periphid)
> +		return;
> +
>  	if (!drv->driver_managed_dma)
>  		iommu_device_unuse_default_domain(dev);
>  }
> @@ -535,7 +548,6 @@ static void amba_device_release(struct device *dev)
>  	fwnode_handle_put(dev_fwnode(&d->dev));
>  	if (d->res.parent)
>  		release_resource(&d->res);
> -	mutex_destroy(&d->periphid_lock);
>  	kfree(d);
>  }
>  
> @@ -593,7 +605,6 @@ static void amba_device_initialize(struct amba_device *dev, const char *name)
>  	dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
>  	dev->dev.dma_parms = &dev->dma_parms;
>  	dev->res.name = dev_name(&dev->dev);
> -	mutex_init(&dev->periphid_lock);
>  }
>  
>  /**
> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> index 958a55bcc708..4bb3467d9c3d 100644
> --- a/include/linux/amba/bus.h
> +++ b/include/linux/amba/bus.h
> @@ -67,7 +67,6 @@ struct amba_device {
>  	struct clk		*pclk;
>  	struct device_dma_parameters dma_parms;
>  	unsigned int		periphid;
> -	struct mutex		periphid_lock;
>  	unsigned int		cid;
>  	struct amba_cs_uci_id	uci;
>  	unsigned int		irq[AMBA_NR_IRQS];
> 

Re: [PATCH RFC 0/3] amba: bus: Move reading periphid operation from amba_match() to amba_probe()

[Saravana Kannan]

On Sun, Sep 8, 2024 at 4:38 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>
> This patch series is to make amba_match(), as bus_type @amba_bustype's
> match(), also follow below ideal rule:
>
> bus_type's match() should only return bool type compatible integer 0 or
> 1 ideally since its main operations are lookup and comparison normally.
>
> Which has been followed by match() of all other bus_types in current
> kernel tree.

The intent or need for this patch series is completely unclear. The
code you are moving around was also pretty delicate and hard to get
right.

Without a much better description for why we need this, I'd like to
give this a NACK.

Also, patch 3/3 is not at all easy to understand and seems to be doing
way more than what the commit message is trying to do.

-Saravana

>
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
> Zijun Hu (3):
>       amba: bus: Warn on adding an AMBA device without valid periphid
>       amba: bus: Move empty @amba_proxy_drv's definition to the front
>       amba: bus: Move reading periphid operation from amba_match() to amba_probe()
>
>  drivers/amba/bus.c       | 130 ++++++++++++++++++++++++++---------------------
>  include/linux/amba/bus.h |   1 -
>  2 files changed, 72 insertions(+), 59 deletions(-)
> ---
> base-commit: 888f67e621dda5c2804a696524e28d0ca4cf0a80
> change-id: 20240829-fix_amba-0f09aa1fc3b1
>
> Best regards,
> --
> Zijun Hu <quic_zijuhu@quicinc.com>
>

Re: [PATCH RFC 0/3] amba: bus: Move reading periphid operation from amba_match() to amba_probe()

[Zijun Hu]

On 2024/9/9 15:24, Saravana Kannan wrote:
> On Sun, Sep 8, 2024 at 4:38 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>>
>> This patch series is to make amba_match(), as bus_type @amba_bustype's
>> match(), also follow below ideal rule:
>>
>> bus_type's match() should only return bool type compatible integer 0 or
>> 1 ideally since its main operations are lookup and comparison normally.
>>
>> Which has been followed by match() of all other bus_types in current
>> kernel tree.
> 
> The intent or need for this patch series is completely unclear. The
> code you are moving around was also pretty delicate and hard to get
> right.
> 
> Without a much better description for why we need this, I'd like to
> give this a NACK.
> 
> Also, patch 3/3 is not at all easy to understand and seems to be doing
> way more than what the commit message is trying to do.
> 

thanks for your code review.

let me explain the issue here firstly to go on with discussion, will
correct it by next revision.

amba_match(), as bus_type @amba_bustype's match(), operate hardware to
read id, may return -EPROBE_DEFER consequently.

this design is not very good and has several disadvantages shown below:

1) it is not good time to operate hardware in a bus_type's match().
   hardware is not ready to operate normally in a bus_type's match()
   as driver_probe_device() shown, there are still many preparations
   to make hardware to operate after a bus_type's match(), for example,
   resuming device and its ancestors, ensuring all its suppliers have
   drivers bound, activating its power domain, ...

2) it should not operate hardware in a bus_type's match().
   a bus_type's match() will obviously be triggered frequently, and
hardware operation is slow normally, it will reduce efficiency for
device attaching driver if operate hardware in a bus_type's match().

   a bus_type's match() will become not reentry for a device and driver
   if operating hardware is failed but can't recover initial hardware state.

3) for driver_attach(), a bus_type's match() are called without
   device_lock(dev) firstly, it often causes concurrent issue when
operate hardware within a bus_type's match(), look at below AMBA related
fix:
   Commit: 25af7406df59 ("ARM: 9229/1: amba: Fix use-after-free in
amba_read_periphid()")
   which introduce an extra @periphid_lock to fix this issue.

4) it may not be proper for a bus_type's match() to return -EPROBE_DEFER
which will stop driver API bus_rescan_devices() from scanning other
remaining devices, that is not expected as discussed by below thread:

https://lore.kernel.org/all/20240904-bus_match_unlikely-v1-3-122318285261@quicinc.com/

5) amba_match() is the only bus_type's match which breaks below ideal
rule in current kernel tree:
   bus_type's match() should only return bool type compatible integer 0
or 1 ideally since its main operations are lookup and comparison normally.


Our purpose is to solve this issue then enforce the ideal rule mentioned
in 5).

so we send this patch series to start a topic about how to solve this
issue (^^).

> -Saravana
> 

Re: [PATCH RFC 0/3] amba: bus: Move reading periphid operation from amba_match() to amba_probe()

[Saravana Kannan]

On Tue, Sep 10, 2024 at 5:17 AM Zijun Hu <zijun_hu@icloud.com> wrote:
>
> On 2024/9/9 15:24, Saravana Kannan wrote:
> > On Sun, Sep 8, 2024 at 4:38 PM Zijun Hu <zijun_hu@icloud.com> wrote:
> >>
> >> This patch series is to make amba_match(), as bus_type @amba_bustype's
> >> match(), also follow below ideal rule:
> >>
> >> bus_type's match() should only return bool type compatible integer 0 or
> >> 1 ideally since its main operations are lookup and comparison normally.
> >>
> >> Which has been followed by match() of all other bus_types in current
> >> kernel tree.
> >
> > The intent or need for this patch series is completely unclear. The
> > code you are moving around was also pretty delicate and hard to get
> > right.
> >
> > Without a much better description for why we need this, I'd like to
> > give this a NACK.
> >
> > Also, patch 3/3 is not at all easy to understand and seems to be doing
> > way more than what the commit message is trying to do.
> >
>
> thanks for your code review.
>
> let me explain the issue here firstly to go on with discussion, will
> correct it by next revision.
>
> amba_match(), as bus_type @amba_bustype's match(), operate hardware to
> read id, may return -EPROBE_DEFER consequently.
>
> this design is not very good and has several disadvantages shown below:
>
> 1) it is not good time to operate hardware in a bus_type's match().
>    hardware is not ready to operate normally in a bus_type's match()
>    as driver_probe_device() shown, there are still many preparations
>    to make hardware to operate after a bus_type's match(), for example,
>    resuming device and its ancestors, ensuring all its suppliers have
>    drivers bound, activating its power domain, ...
>
> 2) it should not operate hardware in a bus_type's match().
>    a bus_type's match() will obviously be triggered frequently, and
> hardware operation is slow normally, it will reduce efficiency for
> device attaching driver if operate hardware in a bus_type's match().
>
>    a bus_type's match() will become not reentry for a device and driver
>    if operating hardware is failed but can't recover initial hardware state.
>
> 3) for driver_attach(), a bus_type's match() are called without
>    device_lock(dev) firstly, it often causes concurrent issue when
> operate hardware within a bus_type's match(), look at below AMBA related
> fix:
>    Commit: 25af7406df59 ("ARM: 9229/1: amba: Fix use-after-free in
> amba_read_periphid()")
>    which introduce an extra @periphid_lock to fix this issue.
>
> 4) it may not be proper for a bus_type's match() to return -EPROBE_DEFER
> which will stop driver API bus_rescan_devices() from scanning other
> remaining devices, that is not expected as discussed by below thread:
>
> https://lore.kernel.org/all/20240904-bus_match_unlikely-v1-3-122318285261@quicinc.com/
>
> 5) amba_match() is the only bus_type's match which breaks below ideal
> rule in current kernel tree:
>    bus_type's match() should only return bool type compatible integer 0
> or 1 ideally since its main operations are lookup and comparison normally.

All of this used to happen even if the bus match wasn't doing what
it's doing today. You don't seem to have full context on how amba
devices are added and probed. What you see now is a clean
up/simplification of how things used to work.

Please go read this patch history and git log history for these files
to get more context.

Nack for the entire series. It'll never go in.

-Saravana

>
>
> Our purpose is to solve this issue then enforce the ideal rule mentioned
> in 5).
>
> so we send this patch series to start a topic about how to solve this
> issue (^^).
>
> > -Saravana
> >
>
>

Re: [PATCH RFC 0/3] amba: bus: Move reading periphid operation from amba_match() to amba_probe()

[Zijun Hu]

On 2024/9/11 00:27, Saravana Kannan wrote:
> On Tue, Sep 10, 2024 at 5:17 AM Zijun Hu <zijun_hu@icloud.com> wrote:
>>
>> On 2024/9/9 15:24, Saravana Kannan wrote:
>>> On Sun, Sep 8, 2024 at 4:38 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>>>>
>>>> This patch series is to make amba_match(), as bus_type @amba_bustype's
>>>> match(), also follow below ideal rule:
>>>>
>>> Also, patch 3/3 is not at all easy to understand and seems to be doing
>>> way more than what the commit message is trying to do.
>>>
>>
>> thanks for your code review.
>>
>> let me explain the issue here firstly to go on with discussion, will
>> correct it by next revision.
>>
>> amba_match(), as bus_type @amba_bustype's match(), operate hardware to
>> read id, may return -EPROBE_DEFER consequently.
>>
>> this design is not very good and has several disadvantages shown below:
>>
>> 1) it is not good time to operate hardware in a bus_type's match().
>>    hardware is not ready to operate normally in a bus_type's match()
>>    as driver_probe_device() shown, there are still many preparations
>>    to make hardware to operate after a bus_type's match(), for example,
>>    resuming device and its ancestors, ensuring all its suppliers have
>>    drivers bound, activating its power domain, ...
>>
.....

>> 5) amba_match() is the only bus_type's match which breaks below ideal
>> rule in current kernel tree:
>>    bus_type's match() should only return bool type compatible integer 0
>> or 1 ideally since its main operations are lookup and comparison normally.
> 
> All of this used to happen even if the bus match wasn't doing what
> it's doing today. You don't seem to have full context on how amba
> devices are added and probed. What you see now is a clean
> up/simplification of how things used to work.
> 
> Please go read this patch history and git log history for these files
> to get more context.
> 
> Nack for the entire series. It'll never go in.
> 

sorry, not agree with you.

IMO, it is easy to make amba_match() return bool type as shown below:

make amba_match() always match with AMBA device with INvalid periphid
and move reading id operation into amba_dma_configure().

Above solution can have the same logical as current one but it looks ugly.

so i make below optimizations to get this patch series:

1) only make AMBA device with INvalid periphid match with existing empty
   amba_proxy_drv to reduce unnecessary reading id operation.

2) moving reading id operation to amba_probe() looks more graceful.


Look at below 3 consecutive history commits:

git log --pretty='%h (\"%s\")' 656b8035b0ee -3
Commit: 656b8035b0ee ("ARM: 8524/1: driver cohandle -EPROBE_DEFER from
bus_type.match()")
Commit: 17f29d36e473 ("ARM: 8523/1: sa1111: ensure no negative value
gets returned on positive match")
Commit: 82ec2ba2b181 ("ARM: 8522/1: drivers: nvdimm: ensure no negative
value gets returned on positive match")

the first AMBA related commit breaks that a bus_type's match() have bool
type return value.
the remaining two commits at the same time really do not like negative
return value for a bus_type's match().

thanks
> -Saravana
> 
>>
>>
>> Our purpose is to solve this issue then enforce the ideal rule mentioned
>> in 5).
>>
>> so we send this patch series to start a topic about how to solve this
>> issue (^^).
>>
>>> -Saravana
>>>
>>
>>

Re: [PATCH RFC 0/3] amba: bus: Move reading periphid operation from amba_match() to amba_probe()

[Russell King (Oracle)]

On Mon, Sep 09, 2024 at 07:37:31AM +0800, Zijun Hu wrote:
> This patch series is to make amba_match(), as bus_type @amba_bustype's
> match(), also follow below ideal rule:
> 
> bus_type's match() should only return bool type compatible integer 0 or
> 1 ideally since its main operations are lookup and comparison normally.
> 
> Which has been followed by match() of all other bus_types in current
> kernel tree.

How does this work with e.g. udev module loading? If the ID isn't
known until we attempt to probe a device, then if all AMBA drivers
are modular, there'll be no drivers registered to cause an attempt
to match a device to a driver, and thus there will be no
peripheral IDs for udev to use to load modules.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC 0/3] amba: bus: Move reading periphid operation from amba_match() to amba_probe()

[Zijun Hu]

On 2024/9/17 17:52, Russell King (Oracle) wrote:
> On Mon, Sep 09, 2024 at 07:37:31AM +0800, Zijun Hu wrote:
>> This patch series is to make amba_match(), as bus_type @amba_bustype's
>> match(), also follow below ideal rule:
>>
>> bus_type's match() should only return bool type compatible integer 0 or
>> 1 ideally since its main operations are lookup and comparison normally.
>>
>> Which has been followed by match() of all other bus_types in current
>> kernel tree.
> 
> How does this work with e.g. udev module loading? If the ID isn't
> known until we attempt to probe a device, then if all AMBA drivers
> are modular, there'll be no drivers registered to cause an attempt
> to match a device to a driver, and thus there will be no
> peripheral IDs for udev to use to load modules.
> 

drivers/amba/bus.c have registered a empty AMBA driver @amba_proxy_drv
this change makes AMBA device without valid periphid ONLY match with
the empty driver, that will trigger trying to read periphid with
bus_type's @amba_bustype's amba_probe(). it will send AMBA device adding
uevent once periphid is read out, then udev will notice the uevent and
perform further actions.

Re: [PATCH RFC 0/3] amba: bus: Move reading periphid operation from amba_match() to amba_probe()

[Saravana Kannan]

On Wed, Sep 11, 2024 at 5:51 AM Zijun Hu <zijun_hu@icloud.com> wrote:
>
> On 2024/9/11 00:27, Saravana Kannan wrote:
> > On Tue, Sep 10, 2024 at 5:17 AM Zijun Hu <zijun_hu@icloud.com> wrote:
> >>
> >> On 2024/9/9 15:24, Saravana Kannan wrote:
> >>> On Sun, Sep 8, 2024 at 4:38 PM Zijun Hu <zijun_hu@icloud.com> wrote:
> >>>>
> >>>> This patch series is to make amba_match(), as bus_type @amba_bustype's
> >>>> match(), also follow below ideal rule:
> >>>>
> >>> Also, patch 3/3 is not at all easy to understand and seems to be doing
> >>> way more than what the commit message is trying to do.
> >>>
> >>
> >> thanks for your code review.
> >>
> >> let me explain the issue here firstly to go on with discussion, will
> >> correct it by next revision.
> >>
> >> amba_match(), as bus_type @amba_bustype's match(), operate hardware to
> >> read id, may return -EPROBE_DEFER consequently.
> >>
> >> this design is not very good and has several disadvantages shown below:
> >>
> >> 1) it is not good time to operate hardware in a bus_type's match().
> >>    hardware is not ready to operate normally in a bus_type's match()
> >>    as driver_probe_device() shown, there are still many preparations
> >>    to make hardware to operate after a bus_type's match(), for example,
> >>    resuming device and its ancestors, ensuring all its suppliers have
> >>    drivers bound, activating its power domain, ...
> >>
> .....
>
> >> 5) amba_match() is the only bus_type's match which breaks below ideal
> >> rule in current kernel tree:
> >>    bus_type's match() should only return bool type compatible integer 0
> >> or 1 ideally since its main operations are lookup and comparison normally.
> >
> > All of this used to happen even if the bus match wasn't doing what
> > it's doing today. You don't seem to have full context on how amba
> > devices are added and probed. What you see now is a clean
> > up/simplification of how things used to work.
> >
> > Please go read this patch history and git log history for these files
> > to get more context.
> >
> > Nack for the entire series. It'll never go in.
> >
>
> sorry, not agree with you.
>
> IMO, it is easy to make amba_match() return bool type as shown below:
>
> make amba_match() always match with AMBA device with INvalid periphid
> and move reading id operation into amba_dma_configure().
>
> Above solution can have the same logical as current one but it looks ugly.
>
> so i make below optimizations to get this patch series:
>
> 1) only make AMBA device with INvalid periphid match with existing empty
>    amba_proxy_drv to reduce unnecessary reading id operation.

No it doesn't. Once match() returns -EPROBE_DEFER we don't try
matching with other drivers. So it doesn't cause more reads.

> 2) moving reading id operation to amba_probe() looks more graceful.

To do a driver/device match, you need to periphid. It doesn't make
sense to push that into some stub probe function instead of doing it
where it's needed. In the match function.

> Look at below 3 consecutive history commits:
>
> git log --pretty='%h (\"%s\")' 656b8035b0ee -3
> Commit: 656b8035b0ee ("ARM: 8524/1: driver cohandle -EPROBE_DEFER from
> bus_type.match()")
> Commit: 17f29d36e473 ("ARM: 8523/1: sa1111: ensure no negative value
> gets returned on positive match")
> Commit: 82ec2ba2b181 ("ARM: 8522/1: drivers: nvdimm: ensure no negative
> value gets returned on positive match")

Those are commits from 2016! Way before any of the cleanup was done.

> the first AMBA related commit breaks that a bus_type's match() have bool
> type return value.

Have you actually looked at the definition of match and it's doc? It's
return type is int and not bool. And the doc says it should return
-EPROBE_DEFER.

> the remaining two commits at the same time really do not like negative
> return value for a bus_type's match().

This whole series is fixing a non-issue because you have a subjective
opinion that the reading of periphid should happen outside of the
match() function where it's actually needed.

And you even have a comment saying it's adding a race.

Russell,

Definite huge NACK from me. Please don't merge this series. I don't
see it fixing anything and it's moving around code for
pointless/questionable reasons. If it is fixing any real bug, I've yet
to hear it explained properly.

If I don't reply further, it means my NACK stands. If the replies
somehow convince me to remove my NACK, I'll do so.

-Saravana

Re: [PATCH RFC 0/3] amba: bus: Move reading periphid operation from amba_match() to amba_probe()

[Zijun Hu]

On 2024/10/3 09:49, Saravana Kannan wrote:
> On Wed, Sep 11, 2024 at 5:51 AM Zijun Hu <zijun_hu@icloud.com> wrote:
>>
>> On 2024/9/11 00:27, Saravana Kannan wrote:
>>> On Tue, Sep 10, 2024 at 5:17 AM Zijun Hu <zijun_hu@icloud.com> wrote:
>>>>
>>>> On 2024/9/9 15:24, Saravana Kannan wrote:
>>>>> On Sun, Sep 8, 2024 at 4:38 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>>>>>>
>>>>>> This patch series is to make amba_match(), as bus_type @amba_bustype's
>>>>>> match(), also follow below ideal rule:
>>>>>>
>>>>> Also, patch 3/3 is not at all easy to understand and seems to be doing
>>>>> way more than what the commit message is trying to do.
>>>>>
>>>>
>>>> thanks for your code review.
>>>>
>>>> let me explain the issue here firstly to go on with discussion, will
>>>> correct it by next revision.
>>>>
>>>> amba_match(), as bus_type @amba_bustype's match(), operate hardware to
>>>> read id, may return -EPROBE_DEFER consequently.
>>>>
>>>> this design is not very good and has several disadvantages shown below:
>>>>
>>>> 1) it is not good time to operate hardware in a bus_type's match().
>>>>    hardware is not ready to operate normally in a bus_type's match()
>>>>    as driver_probe_device() shown, there are still many preparations
>>>>    to make hardware to operate after a bus_type's match(), for example,
>>>>    resuming device and its ancestors, ensuring all its suppliers have
>>>>    drivers bound, activating its power domain, ...
>>>>
>> .....
>>
>>>> 5) amba_match() is the only bus_type's match which breaks below ideal
>>>> rule in current kernel tree:
>>>>    bus_type's match() should only return bool type compatible integer 0
>>>> or 1 ideally since its main operations are lookup and comparison normally.
>>>
>>> All of this used to happen even if the bus match wasn't doing what
>>> it's doing today. You don't seem to have full context on how amba
>>> devices are added and probed. What you see now is a clean
>>> up/simplification of how things used to work.
>>>
>>> Please go read this patch history and git log history for these files
>>> to get more context.
>>>
>>> Nack for the entire series. It'll never go in.
>>>
>>
>> sorry, not agree with you.
>>
>> IMO, it is easy to make amba_match() return bool type as shown below:
>>
>> make amba_match() always match with AMBA device with INvalid periphid
>> and move reading id operation into amba_dma_configure().
>>
>> Above solution can have the same logical as current one but it looks ugly.
>>
>> so i make below optimizations to get this patch series:
>>
>> 1) only make AMBA device with INvalid periphid match with existing empty
>>    amba_proxy_drv to reduce unnecessary reading id operation.
> 
> No it doesn't. Once match() returns -EPROBE_DEFER we don't try
> matching with other drivers. So it doesn't cause more reads.
> 

sorry to give reply late due to travel.

above points is not applicable for driver attaching as explained below.

devn_n : AMBA device n with periphid n
drvn   : AMBA device devn_n's driver.

AMBA bus
├── dev0_0  // dev0 with Invalid periphid 0
├── @amba_proxy_drv // the empty AMBA driver

now let us register 2 AMBA drivers drv1 and drv2.

-EPROBE_DEFER returned during trying to match dev0_0 with drv1
can NOT stopping reading periphid when trying to match dev0_0 with
drv2 since the error code is ignored for driver attaching.

MY solution is shown below:
1) only make device with Invalid ID 0 match with @amba_proxy_drv
   this will reduce unnecessary reading ID operations.
2) Move reading ID from bus's match() to bus's probe()

>> 2) moving reading id operation to amba_probe() looks more graceful.
> 
> To do a driver/device match, you need to periphid. It doesn't make
> sense to push that into some stub probe function instead of doing it
> where it's needed. In the match function.
> 

1) it is not good location for a bus's match() to operate hardware to
read ID as explained by below link:

https://lore.kernel.org/all/a4cf15fb-bbaa-4ed0-a1d5-c362b7a5c6e2@icloud.com/

2) ideally, amba_proxy_drv's probe() is better than the AMBA bus's
probe() to read ID, i use the later since it is simpler and my limited
AMBA knowledge.

@amba_proxy_drv was introduced to ensure triggering reading ID operation
, perhaps, make it take charge for reading ID and have similar role as
the generic USB driver.

>> Look at below 3 consecutive history commits:
>>
>> git log --pretty='%h (\"%s\")' 656b8035b0ee -3
>> Commit: 656b8035b0ee ("ARM: 8524/1: driver cohandle -EPROBE_DEFER from
>> bus_type.match()")
>> Commit: 17f29d36e473 ("ARM: 8523/1: sa1111: ensure no negative value
>> gets returned on positive match")
>> Commit: 82ec2ba2b181 ("ARM: 8522/1: drivers: nvdimm: ensure no negative
>> value gets returned on positive match")
> 
> Those are commits from 2016! Way before any of the cleanup was done.
> 
>> the first AMBA related commit breaks that a bus_type's match() have bool
>> type return value.
> 
> Have you actually looked at the definition of match and it's doc? It's
> return type is int and not bool. And the doc says it should return
> -EPROBE_DEFER.
>

yes, but ALL other bus's match()s only return 0 and 1, AMBA is the only
one which returns extra -EPROBE_DEFER.

-EPROBE_DEFER is a probe related error code and should not be returned
by a bus's match()
it was below AMBA change which make a bus's match() return -EPROBE_DEFER.

Commit: 656b8035b0ee ("ARM: 8524/1: driver cohandle -EPROBE_DEFER from
bus_type.match()")

>> the remaining two commits at the same time really do not like negative
>> return value for a bus_type's match().
> 
> This whole series is fixing a non-issue because you have a subjective
> opinion that the reading of periphid should happen outside of the
> match() function where it's actually needed.
> 

this patch serials is to improve design and not to fix bugs.

> And you even have a comment saying it's adding a race.

as explained by 3) of below link, we don't need extra periphid_lock
any more if moving reading ID operations into probe(). and relevant
a bit race is acceptable.

https://lore.kernel.org/all/a4cf15fb-bbaa-4ed0-a1d5-c362b7a5c6e2@icloud.com/

> 
> Russell,
> 
> Definite huge NACK from me. Please don't merge this series. I don't
> see it fixing anything and it's moving around code for
> pointless/questionable reasons. If it is fixing any real bug, I've yet
> to hear it explained properly.
> 
> If I don't reply further, it means my NACK stands. If the replies
> somehow convince me to remove my NACK, I'll do so.
> 
> -Saravana