[PATCH] hisi_lpc: Use acpi_dev_for_each_child()

[PATCH] hisi_lpc: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Instead of walking the list of children of an ACPI device directly,
use acpi_dev_for_each_child() to carry out an action for all of
the given ACPI device's children.

This will help to eliminate the children list head from struct
acpi_device as it is redundant and it is used in questionable ways
in some places (in particular, locking is needed for walking the
list pointed to it safely, but it is often missing).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/bus/hisi_lpc.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/bus/hisi_lpc.c
===================================================================
--- linux-pm.orig/drivers/bus/hisi_lpc.c
+++ linux-pm/drivers/bus/hisi_lpc.c
@@ -471,6 +471,12 @@ static int hisi_lpc_acpi_remove_subdev(s
 	return 0;
 }
 
+static int hisi_lpc_acpi_clear_enumerated(struct acpi_device *adev, void *not_used)
+{
+	acpi_device_clear_enumerated(adev);
+	return 0;
+}
+
 struct hisi_lpc_acpi_cell {
 	const char *hid;
 	const char *name;
@@ -480,13 +486,11 @@ struct hisi_lpc_acpi_cell {
 
 static void hisi_lpc_acpi_remove(struct device *hostdev)
 {
-	struct acpi_device *adev = ACPI_COMPANION(hostdev);
 	struct acpi_device *child;
 
 	device_for_each_child(hostdev, NULL, hisi_lpc_acpi_remove_subdev);
-
-	list_for_each_entry(child, &adev->children, node)
-		acpi_device_clear_enumerated(child);
+	acpi_dev_for_each_child(ACPI_COMPANION(hostdev),
+				hisi_lpc_acpi_clear_enumerated, NULL);
 }
 
 /*

Re: [PATCH] hisi_lpc: Use acpi_dev_for_each_child()

[John Garry]

On 29/06/2022 13:55, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Instead of walking the list of children of an ACPI device directly,
> use acpi_dev_for_each_child() to carry out an action for all of
> the given ACPI device's children.
> 
> This will help to eliminate the children list head from struct
> acpi_device as it is redundant and it is used in questionable ways
> in some places (in particular, locking is needed for walking the
> list pointed to it safely, but it is often missing).
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Hi Rafael,

> ---
>   drivers/bus/hisi_lpc.c |   12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> Index: linux-pm/drivers/bus/hisi_lpc.c
> ===================================================================
> --- linux-pm.orig/drivers/bus/hisi_lpc.c
> +++ linux-pm/drivers/bus/hisi_lpc.c
> @@ -471,6 +471,12 @@ static int hisi_lpc_acpi_remove_subdev(s
>   	return 0;
>   }
>   
> +static int hisi_lpc_acpi_clear_enumerated(struct acpi_device *adev, void *not_used)
> +{
> +	acpi_device_clear_enumerated(adev);
> +	return 0;
> +}
> +
>   struct hisi_lpc_acpi_cell {
>   	const char *hid;
>   	const char *name;
> @@ -480,13 +486,11 @@ struct hisi_lpc_acpi_cell {
>   
>   static void hisi_lpc_acpi_remove(struct device *hostdev)
>   {
> -	struct acpi_device *adev = ACPI_COMPANION(hostdev);
>   	struct acpi_device *child;
>   
I got this warn:

drivers/bus/hisi_lpc.c: In function ‘hisi_lpc_acpi_remove’:
drivers/bus/hisi_lpc.c:489:22: warning: unused variable ‘child’ 
[-Wunused-variable]
  489 |  struct acpi_device *child;
      |                      ^~~~~
  CC      drivers/bus/brcmstb_gisb.

With that fixed:

Acked-by: John Garry <john.garry@huawei.com>

Can you route this through one of your trees?

>   	device_for_each_child(hostdev, NULL, hisi_lpc_acpi_remove_subdev);
> -
> -	list_for_each_entry(child, &adev->children, node)
> -		acpi_device_clear_enumerated(child);
> +	acpi_dev_for_each_child(ACPI_COMPANION(hostdev),
> +				hisi_lpc_acpi_clear_enumerated, NULL);
>   }
>   
>   /*
> 
> 
> 

BTW, I don't know why I ever added a remove method for this driver 
instead of just setting suppress_bind_attrs....

Thanks,
John

Re: [PATCH] hisi_lpc: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

On Wed, Jun 29, 2022 at 3:34 PM John Garry <john.garry@huawei.com> wrote:
>
> On 29/06/2022 13:55, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Instead of walking the list of children of an ACPI device directly,
> > use acpi_dev_for_each_child() to carry out an action for all of
> > the given ACPI device's children.
> >
> > This will help to eliminate the children list head from struct
> > acpi_device as it is redundant and it is used in questionable ways
> > in some places (in particular, locking is needed for walking the
> > list pointed to it safely, but it is often missing).
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Hi Rafael,
>
> > ---
> >   drivers/bus/hisi_lpc.c |   12 ++++++++----
> >   1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > Index: linux-pm/drivers/bus/hisi_lpc.c
> > ===================================================================
> > --- linux-pm.orig/drivers/bus/hisi_lpc.c
> > +++ linux-pm/drivers/bus/hisi_lpc.c
> > @@ -471,6 +471,12 @@ static int hisi_lpc_acpi_remove_subdev(s
> >       return 0;
> >   }
> >
> > +static int hisi_lpc_acpi_clear_enumerated(struct acpi_device *adev, void *not_used)
> > +{
> > +     acpi_device_clear_enumerated(adev);
> > +     return 0;
> > +}
> > +
> >   struct hisi_lpc_acpi_cell {
> >       const char *hid;
> >       const char *name;
> > @@ -480,13 +486,11 @@ struct hisi_lpc_acpi_cell {
> >
> >   static void hisi_lpc_acpi_remove(struct device *hostdev)
> >   {
> > -     struct acpi_device *adev = ACPI_COMPANION(hostdev);
> >       struct acpi_device *child;
> >
> I got this warn:
>
> drivers/bus/hisi_lpc.c: In function ‘hisi_lpc_acpi_remove’:
> drivers/bus/hisi_lpc.c:489:22: warning: unused variable ‘child’
> [-Wunused-variable]
>   489 |  struct acpi_device *child;
>       |                      ^~~~~
>   CC      drivers/bus/brcmstb_gisb.

I've overlooked that, sorry.  Will send a v2 fixing this shortly.

> With that fixed:
>
> Acked-by: John Garry <john.garry@huawei.com>

Thanks!

> Can you route this through one of your trees?

Yes, I will do that.

[PATCH v2] hisi_lpc: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Instead of walking the list of children of an ACPI device directly,
use acpi_dev_for_each_child() to carry out an action for all of
the given ACPI device's children.

This will help to eliminate the children list head from struct
acpi_device as it is redundant and it is used in questionable ways
in some places (in particular, locking is needed for walking the
list pointed to it safely, but it is often missing).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: John Garry <john.garry@huawei.com>
---

-> v2:
   * Drop unused local variable (John).
   * Add ACK from John.

---
 drivers/bus/hisi_lpc.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/bus/hisi_lpc.c
===================================================================
--- linux-pm.orig/drivers/bus/hisi_lpc.c
+++ linux-pm/drivers/bus/hisi_lpc.c
@@ -471,6 +471,12 @@ static int hisi_lpc_acpi_remove_subdev(s
 	return 0;
 }
 
+static int hisi_lpc_acpi_clear_enumerated(struct acpi_device *adev, void *not_used)
+{
+	acpi_device_clear_enumerated(adev);
+	return 0;
+}
+
 struct hisi_lpc_acpi_cell {
 	const char *hid;
 	const char *name;
@@ -480,13 +486,9 @@ struct hisi_lpc_acpi_cell {
 
 static void hisi_lpc_acpi_remove(struct device *hostdev)
 {
-	struct acpi_device *adev = ACPI_COMPANION(hostdev);
-	struct acpi_device *child;
-
 	device_for_each_child(hostdev, NULL, hisi_lpc_acpi_remove_subdev);
-
-	list_for_each_entry(child, &adev->children, node)
-		acpi_device_clear_enumerated(child);
+	acpi_dev_for_each_child(ACPI_COMPANION(hostdev),
+				hisi_lpc_acpi_clear_enumerated, NULL);
 }
 
 /*

Re: [PATCH v2] hisi_lpc: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

On Wed, Jun 29, 2022 at 3:47 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Instead of walking the list of children of an ACPI device directly,
> use acpi_dev_for_each_child() to carry out an action for all of
> the given ACPI device's children.
>
> This will help to eliminate the children list head from struct
> acpi_device as it is redundant and it is used in questionable ways
> in some places (in particular, locking is needed for walking the
> list pointed to it safely, but it is often missing).

I've overlooked another usage of the children list in hisi_lpc, in
hisi_lpc_acpi_probe(), and eliminating that one is a bit more
complicated.

So please scratch this one and I will send a v3 when 0-day tells me
that it builds.

> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Acked-by: John Garry <john.garry@huawei.com>
> ---
>
> -> v2:
>    * Drop unused local variable (John).
>    * Add ACK from John.
>
> ---
>  drivers/bus/hisi_lpc.c |   14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> Index: linux-pm/drivers/bus/hisi_lpc.c
> ===================================================================
> --- linux-pm.orig/drivers/bus/hisi_lpc.c
> +++ linux-pm/drivers/bus/hisi_lpc.c
> @@ -471,6 +471,12 @@ static int hisi_lpc_acpi_remove_subdev(s
>         return 0;
>  }
>
> +static int hisi_lpc_acpi_clear_enumerated(struct acpi_device *adev, void *not_used)
> +{
> +       acpi_device_clear_enumerated(adev);
> +       return 0;
> +}
> +
>  struct hisi_lpc_acpi_cell {
>         const char *hid;
>         const char *name;
> @@ -480,13 +486,9 @@ struct hisi_lpc_acpi_cell {
>
>  static void hisi_lpc_acpi_remove(struct device *hostdev)
>  {
> -       struct acpi_device *adev = ACPI_COMPANION(hostdev);
> -       struct acpi_device *child;
> -
>         device_for_each_child(hostdev, NULL, hisi_lpc_acpi_remove_subdev);
> -
> -       list_for_each_entry(child, &adev->children, node)
> -               acpi_device_clear_enumerated(child);
> +       acpi_dev_for_each_child(ACPI_COMPANION(hostdev),
> +                               hisi_lpc_acpi_clear_enumerated, NULL);
>  }
>
>  /*
>
>
>

Re: [PATCH v2] hisi_lpc: Use acpi_dev_for_each_child()

[John Garry]

On 30/06/2022 13:48, Rafael J. Wysocki wrote:
> On Wed, Jun 29, 2022 at 3:47 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Instead of walking the list of children of an ACPI device directly,
>> use acpi_dev_for_each_child() to carry out an action for all of
>> the given ACPI device's children.
>>
>> This will help to eliminate the children list head from struct
>> acpi_device as it is redundant and it is used in questionable ways
>> in some places (in particular, locking is needed for walking the
>> list pointed to it safely, but it is often missing).
> 
> I've overlooked another usage of the children list in hisi_lpc, in
> hisi_lpc_acpi_probe(), and eliminating that one is a bit more
> complicated.
> 
> So please scratch this one and I will send a v3 when 0-day tells me
> that it builds.

Hi Rafael,

If it makes things simpler then I can just fix the driver so that we 
can't unload it. Let me know if that suits better.

Cheers

> 
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Acked-by: John Garry <john.garry@huawei.com>
>> ---
>>
>> -> v2:
>>     * Drop unused local variable (John).
>>     * Add ACK from John.
>>
>> ---
>>   drivers/bus/hisi_lpc.c |   14 ++++++++------
>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> Index: linux-pm/drivers/bus/hisi_lpc.c
>> ===================================================================
>> --- linux-pm.orig/drivers/bus/hisi_lpc.c
>> +++ linux-pm/drivers/bus/hisi_lpc.c
>> @@ -471,6 +471,12 @@ static int hisi_lpc_acpi_remove_subdev(s
>>          return 0;
>>   }
>>
>> +static int hisi_lpc_acpi_clear_enumerated(struct acpi_device *adev, void *not_used)
>> +{
>> +       acpi_device_clear_enumerated(adev);
>> +       return 0;
>> +}
>> +
>>   struct hisi_lpc_acpi_cell {
>>          const char *hid;
>>          const char *name;
>> @@ -480,13 +486,9 @@ struct hisi_lpc_acpi_cell {
>>
>>   static void hisi_lpc_acpi_remove(struct device *hostdev)
>>   {
>> -       struct acpi_device *adev = ACPI_COMPANION(hostdev);
>> -       struct acpi_device *child;
>> -
>>          device_for_each_child(hostdev, NULL, hisi_lpc_acpi_remove_subdev);
>> -
>> -       list_for_each_entry(child, &adev->children, node)
>> -               acpi_device_clear_enumerated(child);
>> +       acpi_dev_for_each_child(ACPI_COMPANION(hostdev),
>> +                               hisi_lpc_acpi_clear_enumerated, NULL);
>>   }
>>
>>   /*
>>
>>
>>
> .

Re: [PATCH v2] hisi_lpc: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

On Thu, Jun 30, 2022 at 3:37 PM John Garry <john.garry@huawei.com> wrote:
>
> On 30/06/2022 13:48, Rafael J. Wysocki wrote:
> > On Wed, Jun 29, 2022 at 3:47 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>
> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>
> >> Instead of walking the list of children of an ACPI device directly,
> >> use acpi_dev_for_each_child() to carry out an action for all of
> >> the given ACPI device's children.
> >>
> >> This will help to eliminate the children list head from struct
> >> acpi_device as it is redundant and it is used in questionable ways
> >> in some places (in particular, locking is needed for walking the
> >> list pointed to it safely, but it is often missing).
> >
> > I've overlooked another usage of the children list in hisi_lpc, in
> > hisi_lpc_acpi_probe(), and eliminating that one is a bit more
> > complicated.
> >
> > So please scratch this one and I will send a v3 when 0-day tells me
> > that it builds.
>
> Hi Rafael,
>
> If it makes things simpler then I can just fix the driver so that we
> can't unload it. Let me know if that suits better.

I'd rather do the ACPI change first.

Also it looks like the "remove" is needed to do the cleanup in the
"probe" error path anyway.

Cheers!

[PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] hisi_lpc: Use acpi_dev_for_each_child()

Instead of walking the list of children of an ACPI device directly,
use acpi_dev_for_each_child() to carry out an action for all of
the given ACPI device's children.

This will help to eliminate the children list head from struct
acpi_device as it is redundant and it is used in questionable ways
in some places (in particular, locking is needed for walking the
list pointed to it safely, but it is often missing).

While at it, simplify hisi_lpc_acpi_set_io_res() by making it accept
a struct acpi_device pointer from the caller, instead of going to
struct device and back to get the same result, and clean up confusion
regarding hostdev and its ACPI companion in that function.

Also remove a redundant check from it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 
v2 -> v3:
   * Also cover hisi_lpc_acpi_probe() which has triggered additional
     changes.
   * Drop the ACK as new material was added.

-> v2:
   * Drop unused local variable (John).
   * Add ACK from John.

---
 drivers/bus/hisi_lpc.c |  201 +++++++++++++++++++++++--------------------------
 1 file changed, 98 insertions(+), 103 deletions(-)

Index: linux-pm/drivers/bus/hisi_lpc.c
===================================================================
--- linux-pm.orig/drivers/bus/hisi_lpc.c
+++ linux-pm/drivers/bus/hisi_lpc.c
@@ -379,7 +379,7 @@ static void hisi_lpc_acpi_fixup_child_re
 
 /*
  * hisi_lpc_acpi_set_io_res - set the resources for a child
- * @child: the device node to be updated the I/O resource
+ * @adev: ACPI companion of the device node to be updated the I/O resource
  * @hostdev: the device node associated with host controller
  * @res: double pointer to be set to the address of translated resources
  * @num_res: pointer to variable to hold the number of translated resources
@@ -390,31 +390,24 @@ static void hisi_lpc_acpi_fixup_child_re
  * host-relative address resource.  This function will return the translated
  * logical PIO addresses for each child devices resources.
  */
-static int hisi_lpc_acpi_set_io_res(struct device *child,
+static int hisi_lpc_acpi_set_io_res(struct acpi_device *adev,
 				    struct device *hostdev,
 				    const struct resource **res, int *num_res)
 {
-	struct acpi_device *adev;
-	struct acpi_device *host;
+	struct acpi_device *host = to_acpi_device(adev->dev.parent);
 	struct resource_entry *rentry;
 	LIST_HEAD(resource_list);
 	struct resource *resources;
 	int count;
 	int i;
 
-	if (!child || !hostdev)
-		return -EINVAL;
-
-	host = to_acpi_device(hostdev);
-	adev = to_acpi_device(child);
-
 	if (!adev->status.present) {
-		dev_dbg(child, "device is not present\n");
+		dev_dbg(&adev->dev, "device is not present\n");
 		return -EIO;
 	}
 
 	if (acpi_device_enumerated(adev)) {
-		dev_dbg(child, "has been enumerated\n");
+		dev_dbg(&adev->dev, "has been enumerated\n");
 		return -EIO;
 	}
 
@@ -425,7 +418,7 @@ static int hisi_lpc_acpi_set_io_res(stru
 	 */
 	count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
 	if (count <= 0) {
-		dev_dbg(child, "failed to get resources\n");
+		dev_dbg(&adev->dev, "failed to get resources\n");
 		return count ? count : -EIO;
 	}
 
@@ -454,7 +447,7 @@ static int hisi_lpc_acpi_set_io_res(stru
 			continue;
 		ret = hisi_lpc_acpi_xlat_io_res(adev, host, &resources[i]);
 		if (ret) {
-			dev_err(child, "translate IO range %pR failed (%d)\n",
+			dev_err(&adev->dev, "translate IO range %pR failed (%d)\n",
 				&resources[i], ret);
 			return ret;
 		}
@@ -471,6 +464,12 @@ static int hisi_lpc_acpi_remove_subdev(s
 	return 0;
 }
 
+static int hisi_lpc_acpi_clear_enumerated(struct acpi_device *adev, void *not_used)
+{
+	acpi_device_clear_enumerated(adev);
+	return 0;
+}
+
 struct hisi_lpc_acpi_cell {
 	const char *hid;
 	const char *name;
@@ -480,13 +479,89 @@ struct hisi_lpc_acpi_cell {
 
 static void hisi_lpc_acpi_remove(struct device *hostdev)
 {
-	struct acpi_device *adev = ACPI_COMPANION(hostdev);
-	struct acpi_device *child;
-
 	device_for_each_child(hostdev, NULL, hisi_lpc_acpi_remove_subdev);
+	acpi_dev_for_each_child(ACPI_COMPANION(hostdev),
+				hisi_lpc_acpi_clear_enumerated, NULL);
+}
+
+static int hisi_lpc_acpi_add_child(struct acpi_device *child, void *data)
+{
+	const char *hid = acpi_device_hid(child);
+	struct device *hostdev = data;
+	const struct hisi_lpc_acpi_cell *cell;
+	struct platform_device *pdev;
+	const struct resource *res;
+	bool found = false;
+	int num_res;
+	int ret;
+
+	ret = hisi_lpc_acpi_set_io_res(child, hostdev, &res, &num_res);
+	if (ret) {
+		dev_warn(hostdev, "set resource fail (%d)\n", ret);
+		return ret;
+	}
+
+	cell = (struct hisi_lpc_acpi_cell []){
+		/* ipmi */
+		{
+			.hid = "IPI0001",
+			.name = "hisi-lpc-ipmi",
+		},
+		/* 8250-compatible uart */
+		{
+			.hid = "HISI1031",
+			.name = "serial8250",
+			.pdata = (struct plat_serial8250_port []) {
+				{
+					.iobase = res->start,
+					.uartclk = 1843200,
+					.iotype = UPIO_PORT,
+					.flags = UPF_BOOT_AUTOCONF,
+				},
+				{}
+			},
+			.pdata_size = 2 *
+				sizeof(struct plat_serial8250_port),
+		},
+		{}
+	};
+
+	for (; cell && cell->name; cell++) {
+		if (!strcmp(cell->hid, hid)) {
+			found = true;
+			break;
+		}
+	}
+
+	if (!found) {
+		dev_warn(hostdev,
+			 "could not find cell for child device (%s), discarding\n",
+			 hid);
+		return 0;
+	}
+
+	pdev = platform_device_alloc(cell->name, PLATFORM_DEVID_AUTO);
+	if (!pdev)
+		return -ENOMEM;
+
+	pdev->dev.parent = hostdev;
+	ACPI_COMPANION_SET(&pdev->dev, child);
+
+	ret = platform_device_add_resources(pdev, res, num_res);
+	if (ret)
+		return ret;
+
+	ret = platform_device_add_data(pdev, cell->pdata, cell->pdata_size);
+	if (ret)
+		return ret;
+
+	ret = platform_device_add(pdev);
+	if (ret)
+		return ret;
 
-	list_for_each_entry(child, &adev->children, node)
-		acpi_device_clear_enumerated(child);
+	acpi_device_set_enumerated(child);
+
+	return 0;
 }
 
 /*
@@ -501,94 +576,14 @@ static void hisi_lpc_acpi_remove(struct
  */
 static int hisi_lpc_acpi_probe(struct device *hostdev)
 {
-	struct acpi_device *adev = ACPI_COMPANION(hostdev);
-	struct acpi_device *child;
 	int ret;
 
 	/* Only consider the children of the host */
-	list_for_each_entry(child, &adev->children, node) {
-		const char *hid = acpi_device_hid(child);
-		const struct hisi_lpc_acpi_cell *cell;
-		struct platform_device *pdev;
-		const struct resource *res;
-		bool found = false;
-		int num_res;
-
-		ret = hisi_lpc_acpi_set_io_res(&child->dev, &adev->dev, &res,
-					       &num_res);
-		if (ret) {
-			dev_warn(hostdev, "set resource fail (%d)\n", ret);
-			goto fail;
-		}
-
-		cell = (struct hisi_lpc_acpi_cell []){
-			/* ipmi */
-			{
-				.hid = "IPI0001",
-				.name = "hisi-lpc-ipmi",
-			},
-			/* 8250-compatible uart */
-			{
-				.hid = "HISI1031",
-				.name = "serial8250",
-				.pdata = (struct plat_serial8250_port []) {
-					{
-						.iobase = res->start,
-						.uartclk = 1843200,
-						.iotype = UPIO_PORT,
-						.flags = UPF_BOOT_AUTOCONF,
-					},
-					{}
-				},
-				.pdata_size = 2 *
-					sizeof(struct plat_serial8250_port),
-			},
-			{}
-		};
-
-		for (; cell && cell->name; cell++) {
-			if (!strcmp(cell->hid, hid)) {
-				found = true;
-				break;
-			}
-		}
-
-		if (!found) {
-			dev_warn(hostdev,
-				 "could not find cell for child device (%s), discarding\n",
-				 hid);
-			continue;
-		}
-
-		pdev = platform_device_alloc(cell->name, PLATFORM_DEVID_AUTO);
-		if (!pdev) {
-			ret = -ENOMEM;
-			goto fail;
-		}
-
-		pdev->dev.parent = hostdev;
-		ACPI_COMPANION_SET(&pdev->dev, child);
-
-		ret = platform_device_add_resources(pdev, res, num_res);
-		if (ret)
-			goto fail;
-
-		ret = platform_device_add_data(pdev, cell->pdata,
-					       cell->pdata_size);
-		if (ret)
-			goto fail;
-
-		ret = platform_device_add(pdev);
-		if (ret)
-			goto fail;
-
-		acpi_device_set_enumerated(child);
-	}
-
-	return 0;
+	ret = acpi_dev_for_each_child(ACPI_COMPANION(hostdev),
+				      hisi_lpc_acpi_add_child, hostdev);
+	if (ret)
+		hisi_lpc_acpi_remove(hostdev);
 
-fail:
-	hisi_lpc_acpi_remove(hostdev);
 	return ret;
 }
 

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[Greg Kroah-Hartman]

On Thu, Jun 30, 2022 at 08:13:52PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH] hisi_lpc: Use acpi_dev_for_each_child()
> 
> Instead of walking the list of children of an ACPI device directly,
> use acpi_dev_for_each_child() to carry out an action for all of
> the given ACPI device's children.
> 
> This will help to eliminate the children list head from struct
> acpi_device as it is redundant and it is used in questionable ways
> in some places (in particular, locking is needed for walking the
> list pointed to it safely, but it is often missing).
> 
> While at it, simplify hisi_lpc_acpi_set_io_res() by making it accept
> a struct acpi_device pointer from the caller, instead of going to
> struct device and back to get the same result, and clean up confusion
> regarding hostdev and its ACPI companion in that function.
> 
> Also remove a redundant check from it.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[John Garry]

On 30/06/2022 19:13, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH] hisi_lpc: Use acpi_dev_for_each_child()
> 
> Instead of walking the list of children of an ACPI device directly,
> use acpi_dev_for_each_child() to carry out an action for all of
> the given ACPI device's children.
> 
> This will help to eliminate the children list head from struct
> acpi_device as it is redundant and it is used in questionable ways
> in some places (in particular, locking is needed for walking the
> list pointed to it safely, but it is often missing).
> 
> While at it, simplify hisi_lpc_acpi_set_io_res() by making it accept
> a struct acpi_device pointer from the caller, instead of going to
> struct device and back to get the same result, and clean up confusion
> regarding hostdev and its ACPI companion in that function.
> 
> Also remove a redundant check from it.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

This change itself looks fine and I quickly tested, so:
Reviewed-by: John Garry <john.garry@huawei.com>

However Yang Yingliang spotted a pre-existing bug in the ACPI probe and 
sent a fix today (coincidence?):

https://lore.kernel.org/lkml/20220701094352.2104998-1-yangyingliang@huawei.com/T/#u

And they conflict. This code has been this way for years, so I just 
suggest Yang Yingliang resends the fix on top off Rafael's change.

Thanks,
John

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[Andy Shevchenko]

On Fri, Jul 1, 2022 at 1:04 PM John Garry <john.garry@huawei.com> wrote:
> On 30/06/2022 19:13, Rafael J. Wysocki wrote:

...

> However Yang Yingliang spotted a pre-existing bug in the ACPI probe and
> sent a fix today (coincidence?):
>
> https://lore.kernel.org/lkml/20220701094352.2104998-1-yangyingliang@huawei.com/T/#u
>
> And they conflict. This code has been this way for years, so I just
> suggest Yang Yingliang resends the fix on top off Rafael's change.

Wondering if Yang can actually switch that to use
platform_device_register_full().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[Andy Shevchenko]

On Fri, Jul 1, 2022 at 1:06 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Jul 1, 2022 at 1:04 PM John Garry <john.garry@huawei.com> wrote:
> > On 30/06/2022 19:13, Rafael J. Wysocki wrote:
>
> ...
>
> > However Yang Yingliang spotted a pre-existing bug in the ACPI probe and
> > sent a fix today (coincidence?):
> >
> > https://lore.kernel.org/lkml/20220701094352.2104998-1-yangyingliang@huawei.com/T/#u
> >
> > And they conflict. This code has been this way for years, so I just
> > suggest Yang Yingliang resends the fix on top off Rafael's change.
>
> Wondering if Yang can actually switch that to use
> platform_device_register_full().

And for the record, I think the Fixes even for very rare bug hits
should go first.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[John Garry]

On 01/07/2022 12:07, Andy Shevchenko wrote:
> On Fri, Jul 1, 2022 at 1:06 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Fri, Jul 1, 2022 at 1:04 PM John Garry <john.garry@huawei.com> wrote:
>>> On 30/06/2022 19:13, Rafael J. Wysocki wrote:
>>
>> ...
>>
>>> However Yang Yingliang spotted a pre-existing bug in the ACPI probe and
>>> sent a fix today (coincidence?):
>>>
>>> https://lore.kernel.org/lkml/20220701094352.2104998-1-yangyingliang@huawei.com/T/#u
>>>
>>> And they conflict. This code has been this way for years, so I just
>>> suggest Yang Yingliang resends the fix on top off Rafael's change.
>>
>> Wondering if Yang can actually switch that to use
>> platform_device_register_full().

Maybe that would work and simplify things. Let me check it.

BTW, when we originally upstreamed this driver there was some ACPI 
platform device registration code which you/we thought could be factored 
out later. I can't remember it. I was looking through lore but couldn't 
find it. I don't remember it being so important, though.

> 
> And for the record, I think the Fixes even for very rare bug hits
> should go first.
> 

ok, I have to admit that I was going to feel awkward asking Rafael to 
deal with this fix by having a v4 on top of it.

Thanks,
John

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[Andy Shevchenko]

On Fri, Jul 1, 2022 at 1:54 PM John Garry <john.garry@huawei.com> wrote:
> On 01/07/2022 12:07, Andy Shevchenko wrote:
> > On Fri, Jul 1, 2022 at 1:06 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >> On Fri, Jul 1, 2022 at 1:04 PM John Garry <john.garry@huawei.com> wrote:
> >>> On 30/06/2022 19:13, Rafael J. Wysocki wrote:

...

> >>> However Yang Yingliang spotted a pre-existing bug in the ACPI probe and
> >>> sent a fix today (coincidence?):
> >>>
> >>> https://lore.kernel.org/lkml/20220701094352.2104998-1-yangyingliang@huawei.com/T/#u
> >>>
> >>> And they conflict. This code has been this way for years, so I just
> >>> suggest Yang Yingliang resends the fix on top off Rafael's change.
> >>
> >> Wondering if Yang can actually switch that to use
> >> platform_device_register_full().
>
> Maybe that would work and simplify things. Let me check it.
>
> BTW, when we originally upstreamed this driver there was some ACPI
> platform device registration code which you/we thought could be factored
> out later. I can't remember it. I was looking through lore but couldn't
> find it. I don't remember it being so important, though.

My suggestion is definitely not for the fix itself, but as a follow up.

> > And for the record, I think the Fixes even for very rare bug hits
> > should go first.
>
> ok, I have to admit that I was going to feel awkward asking Rafael to
> deal with this fix by having a v4 on top of it.

I don't think it's a problem as long as we have an immutable branch /
tag with that patch. Another approach could be that Rafael can take it
as a precursor for his series and route via ACPI tree, but let's hear
what he thinks about this himself.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[John Garry]

On 01/07/2022 13:05, Andy Shevchenko wrote:
> On Fri, Jul 1, 2022 at 1:54 PM John Garry <john.garry@huawei.com> wrote:
>> On 01/07/2022 12:07, Andy Shevchenko wrote:
>>> On Fri, Jul 1, 2022 at 1:06 PM Andy Shevchenko
>>> <andy.shevchenko@gmail.com> wrote:
>>>> On Fri, Jul 1, 2022 at 1:04 PM John Garry <john.garry@huawei.com> wrote:
>>>>> On 30/06/2022 19:13, Rafael J. Wysocki wrote:
> 
> ...
> 
>>>>> However Yang Yingliang spotted a pre-existing bug in the ACPI probe and
>>>>> sent a fix today (coincidence?):
>>>>>
>>>>> https://lore.kernel.org/lkml/20220701094352.2104998-1-yangyingliang@huawei.com/T/#u
>>>>>
>>>>> And they conflict. This code has been this way for years, so I just
>>>>> suggest Yang Yingliang resends the fix on top off Rafael's change.
>>>>
>>>> Wondering if Yang can actually switch that to use
>>>> platform_device_register_full().
>>
>> Maybe that would work and simplify things. Let me check it.
>>
>> BTW, when we originally upstreamed this driver there was some ACPI
>> platform device registration code which you/we thought could be factored
>> out later. I can't remember it. I was looking through lore but couldn't
>> find it. I don't remember it being so important, though.
> 
> My suggestion is definitely not for the fix itself, but as a follow up.

FWIW, it works out quite neatly:

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index e0fee1f863e6..70198d5644c7 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -472,9 +472,7 @@ static int hisi_lpc_acpi_clear_enumerated(struct 
acpi_device *adev, void *not_us

  struct hisi_lpc_acpi_cell {
  	const char *hid;
-	const char *name;
-	void *pdata;
-	size_t pdata_size;
+	struct platform_device_info pdevinfo;
  };

  static void hisi_lpc_acpi_remove(struct device *hostdev)
@@ -505,28 +503,36 @@ static int hisi_lpc_acpi_add_child(struct 
acpi_device *child, void *data)
  		/* ipmi */
  		{
  			.hid = "IPI0001",
-			.name = "hisi-lpc-ipmi",
+			.pdevinfo = {
+				.name = "hisi-lpc-ipmi",
+				.num_res = num_res,
+				.res = res,
+			},
  		},
  		/* 8250-compatible uart */
  		{
  			.hid = "HISI1031",
-			.name = "serial8250",
-			.pdata = (struct plat_serial8250_port []) {
-				{
-					.iobase = res->start,
-					.uartclk = 1843200,
-					.iotype = UPIO_PORT,
-					.flags = UPF_BOOT_AUTOCONF,
+			.pdevinfo = {
+				.name = "serial8250",
+				.data = (struct plat_serial8250_port []) {
+					{
+						.iobase = res->start,
+						.uartclk = 1843200,
+						.iotype = UPIO_PORT,
+						.flags = UPF_BOOT_AUTOCONF,
+					},
+					{}
  				},
-				{}
+				.size_data = 2 *
+					sizeof(struct plat_serial8250_port),
+				.num_res = num_res,
+				.res = res,
  			},
-			.pdata_size = 2 *
-				sizeof(struct plat_serial8250_port),
  		},
  		{}
  	};

-	for (; cell && cell->name; cell++) {
+	for (; cell && cell->pdevinfo.name; cell++) {
  		if (!strcmp(cell->hid, hid)) {
  			found = true;
  			break;
@@ -540,25 +546,13 @@ static int hisi_lpc_acpi_add_child(struct 
acpi_device *child, void *data)
  		return 0;
  	}

-	pdev = platform_device_alloc(cell->name, PLATFORM_DEVID_AUTO);
+	pdev = platform_device_register_full(&cell->pdevinfo);
  	if (!pdev)
  		return -ENOMEM;

  	pdev->dev.parent = hostdev;
  	ACPI_COMPANION_SET(&pdev->dev, child);

-	ret = platform_device_add_resources(pdev, res, num_res);
-	if (ret)
-		return ret;
-
-	ret = platform_device_add_data(pdev, cell->pdata, cell->pdata_size);
-	if (ret)
-		return ret;
-
-	ret = platform_device_add(pdev);
-	if (ret)
-		return ret;
-
  	acpi_device_set_enumerated(child);

  	return 0;

> 
>>> And for the record, I think the Fixes even for very rare bug hits
>>> should go first.
>>
>> ok, I have to admit that I was going to feel awkward asking Rafael to
>> deal with this fix by having a v4 on top of it.
> 
> I don't think it's a problem as long as we have an immutable branch /
> tag with that patch. Another approach could be that Rafael can take it
> as a precursor for his series and route via ACPI tree, but let's hear
> what he thinks about this himself.
> 

ok, fine.

Thanks,
John

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

Hi John,

On Fri, Jul 1, 2022 at 2:18 PM John Garry <john.garry@huawei.com> wrote:
>
> On 01/07/2022 13:05, Andy Shevchenko wrote:
> > On Fri, Jul 1, 2022 at 1:54 PM John Garry <john.garry@huawei.com> wrote:
> >> On 01/07/2022 12:07, Andy Shevchenko wrote:
> >>> On Fri, Jul 1, 2022 at 1:06 PM Andy Shevchenko
> >>> <andy.shevchenko@gmail.com> wrote:
> >>>> On Fri, Jul 1, 2022 at 1:04 PM John Garry <john.garry@huawei.com> wrote:
> >>>>> On 30/06/2022 19:13, Rafael J. Wysocki wrote:
> >
> > ...
> >
> >>>>> However Yang Yingliang spotted a pre-existing bug in the ACPI probe and
> >>>>> sent a fix today (coincidence?):
> >>>>>
> >>>>> https://lore.kernel.org/lkml/20220701094352.2104998-1-yangyingliang@huawei.com/T/#u
> >>>>>
> >>>>> And they conflict. This code has been this way for years, so I just
> >>>>> suggest Yang Yingliang resends the fix on top off Rafael's change.
> >>>>
> >>>> Wondering if Yang can actually switch that to use
> >>>> platform_device_register_full().
> >>
> >> Maybe that would work and simplify things. Let me check it.
> >>
> >> BTW, when we originally upstreamed this driver there was some ACPI
> >> platform device registration code which you/we thought could be factored
> >> out later. I can't remember it. I was looking through lore but couldn't
> >> find it. I don't remember it being so important, though.
> >
> > My suggestion is definitely not for the fix itself, but as a follow up.
>

[cut]

> >>> And for the record, I think the Fixes even for very rare bug hits
> >>> should go first.
> >>
> >> ok, I have to admit that I was going to feel awkward asking Rafael to
> >> deal with this fix by having a v4 on top of it.
> >
> > I don't think it's a problem as long as we have an immutable branch /
> > tag with that patch. Another approach could be that Rafael can take it
> > as a precursor for his series and route via ACPI tree, but let's hear
> > what he thinks about this himself.
> >
>
> ok, fine.

I've applied the patch from Yang Yingliang and I thought it would be
OK to add your ACK to it based on the conversation so far (please let
me know if that's not the case).  Next, I've added my patch rebased on
top of it with the tags from you and Greg.

The result is on my bleeding-edge branch:

https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=bleeding-edge

and these are the commits in question

https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=bleeding-edge&id=d674553009afc9b24cab2bbec71628609edbbb27
https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=bleeding-edge&id=54872fea6a5ac967ec2272aea525d1438ac6735a

Please let me know if you see any issues with them.

If not, I'll go ahead and move them to my linux-next branch.

Cheers!

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[John Garry]

On 04/07/2022 20:02, Rafael J. Wysocki wrote:

Hi Rafael,

> I've applied the patch from Yang Yingliang and I thought it would be
> OK to add your ACK to it based on the conversation so far (please let
> me know if that's not the case).  Next, I've added my patch rebased on
> top of it with the tags from you and Greg.
> 
> The result is on my bleeding-edge branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=bleeding-edge
> 
> and these are the commits in question
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=bleeding-edge&id=d674553009afc9b24cab2bbec71628609edbbb27
> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=bleeding-edge&id=54872fea6a5ac967ec2272aea525d1438ac6735a
> 
> Please let me know if you see any issues with them.
> 

I gave these a quick test on my board and they look fine.

Acked-by: John Garry <john.garry@huawei.com>

> If not, I'll go ahead and move them to my linux-next branch.

Thanks,
John

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[Andy Shevchenko]

On Tue, Jul 5, 2022 at 10:37 AM John Garry <john.garry@huawei.com> wrote:
> On 04/07/2022 20:02, Rafael J. Wysocki wrote:

...

> I gave these a quick test on my board and they look fine.
>
> Acked-by: John Garry <john.garry@huawei.com>

John, I believe now you may send a formal clean up to convert to platform_device

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[Andy Shevchenko]

On Tue, Jul 5, 2022 at 11:38 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Jul 5, 2022 at 10:37 AM John Garry <john.garry@huawei.com> wrote:
> > On 04/07/2022 20:02, Rafael J. Wysocki wrote:
>
> ...
>
> > I gave these a quick test on my board and they look fine.
> >
> > Acked-by: John Garry <john.garry@huawei.com>
>
> John, I believe now you may send a formal clean up to convert to platform_device

Hit Enter too early :-)

...to platform_device_register_full().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[John Garry]

On 05/07/2022 10:39, Andy Shevchenko wrote:
> On Tue, Jul 5, 2022 at 11:38 AM Andy Shevchenko
> <andy.shevchenko@gmail.com>  wrote:
>> On Tue, Jul 5, 2022 at 10:37 AM John Garry<john.garry@huawei.com>  wrote:
>>> On 04/07/2022 20:02, Rafael J. Wysocki wrote:
>> ...
>>
>>> I gave these a quick test on my board and they look fine.
>>>
>>> Acked-by: John Garry<john.garry@huawei.com>
>> John, I believe now you may send a formal clean up to convert to platform_device
> Hit Enter too early:-)
> 
> ...to platform_device_register_full().

Sure, I can look at that now. But I just found where we previously 
mentioned the possibility of factoring out some of the ACPI platform 
device creation code:

https://lore.kernel.org/linux-acpi/CAHp75VfOa5pN4MKT-aQmWBwPGWsOaQupyfrN-weWwfR3vMLtuA@mail.gmail.com/

There is actually still a comment in the hisi_lpc driver - I should have 
checked there first :)

So my impression is that the hisi_lpc code is almost the same in 
acpi_create_platform_device(), apart from we need do the resource fixup 
in hisi_lpc_acpi_set_io_res().

So we could factor out by dividing acpi_create_platform_device() into 2x 
parts: resource get and then platform dev create. But that does not seem 
wise as we have 2x parts which don't make sense on their own. Or else 
pass a fixup callback into acpi_create_platform_device(). Any other 
ideas if we want to go this way?

Thanks,
John

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[Andy Shevchenko]

On Tue, Jul 5, 2022 at 12:16 PM John Garry <john.garry@huawei.com> wrote:
> On 05/07/2022 10:39, Andy Shevchenko wrote:
> > On Tue, Jul 5, 2022 at 11:38 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com>  wrote:
> >> On Tue, Jul 5, 2022 at 10:37 AM John Garry<john.garry@huawei.com>  wrote:

...

> >> John, I believe now you may send a formal clean up to convert to platform_device
> > Hit Enter too early:-)
> >
> > ...to platform_device_register_full().
>
> Sure, I can look at that now. But I just found where we previously
> mentioned the possibility of factoring out some of the ACPI platform
> device creation code:
>
> https://lore.kernel.org/linux-acpi/CAHp75VfOa5pN4MKT-aQmWBwPGWsOaQupyfrN-weWwfR3vMLtuA@mail.gmail.com/
>
> There is actually still a comment in the hisi_lpc driver - I should have
> checked there first :)
>
> So my impression is that the hisi_lpc code is almost the same in
> acpi_create_platform_device(), apart from we need do the resource fixup
> in hisi_lpc_acpi_set_io_res().
>
> So we could factor out by dividing acpi_create_platform_device() into 2x
> parts: resource get and then platform dev create. But that does not seem
> wise as we have 2x parts which don't make sense on their own. Or else
> pass a fixup callback into acpi_create_platform_device(). Any other
> ideas if we want to go this way?

I prefer having an ops or so structure where you can pass ->fixup()
and/or ->xlate() function, Btw, it looks like the code in hisi_lpc
needs a lot of cleanups.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

On Tue, Jul 5, 2022 at 12:16 PM John Garry <john.garry@huawei.com> wrote:
>
> On 05/07/2022 10:39, Andy Shevchenko wrote:
> > On Tue, Jul 5, 2022 at 11:38 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com>  wrote:
> >> On Tue, Jul 5, 2022 at 10:37 AM John Garry<john.garry@huawei.com>  wrote:
> >>> On 04/07/2022 20:02, Rafael J. Wysocki wrote:
> >> ...
> >>
> >>> I gave these a quick test on my board and they look fine.
> >>>
> >>> Acked-by: John Garry<john.garry@huawei.com>
> >> John, I believe now you may send a formal clean up to convert to platform_device
> > Hit Enter too early:-)
> >
> > ...to platform_device_register_full().
>
> Sure, I can look at that now. But I just found where we previously
> mentioned the possibility of factoring out some of the ACPI platform
> device creation code:
>
> https://lore.kernel.org/linux-acpi/CAHp75VfOa5pN4MKT-aQmWBwPGWsOaQupyfrN-weWwfR3vMLtuA@mail.gmail.com/
>
> There is actually still a comment in the hisi_lpc driver - I should have
> checked there first :)
>
> So my impression is that the hisi_lpc code is almost the same in
> acpi_create_platform_device(), apart from we need do the resource fixup
> in hisi_lpc_acpi_set_io_res().
>
> So we could factor out by dividing acpi_create_platform_device() into 2x
> parts: resource get and then platform dev create. But that does not seem
> wise as we have 2x parts which don't make sense on their own. Or else
> pass a fixup callback into acpi_create_platform_device(). Any other
> ideas if we want to go this way?

Personally, I would do the cleanup that can be done without
refactoring the library function as the first step, just to reduce the
amount of changes made in one go if nothing else.

Next, I'd look at introducing something like

acpi_create_platform_device_ops(struct acpi_device *adev, const struct
property_entry *properties, const struct *platform_device_create_ops
*ops);

where ops would be a set of callbacks to invoke as a matter of customization.

Then, acpi_create_platform_device() can be defined as a wrapper around
the above.

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[John Garry]

> Next, I'd look at introducing something like
> 
> acpi_create_platform_device_ops(struct acpi_device *adev, const struct
> property_entry *properties, const struct *platform_device_create_ops
> *ops);
> 
> where ops would be a set of callbacks to invoke as a matter of customization.
> 
> Then, acpi_create_platform_device() can be defined as a wrapper around
> the above.
> .

ok, that seems easiest. But alternatively do you see any scope to have 
that platform_device_create_ops * ops in the acpi_device struct (so that 
we don't need to create this new API)?

Thanks,
John

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

On Tue, Jul 5, 2022 at 5:17 PM John Garry <john.garry@huawei.com> wrote:
>
> > Next, I'd look at introducing something like
> >
> > acpi_create_platform_device_ops(struct acpi_device *adev, const struct
> > property_entry *properties, const struct *platform_device_create_ops
> > *ops);
> >
> > where ops would be a set of callbacks to invoke as a matter of customization.
> >
> > Then, acpi_create_platform_device() can be defined as a wrapper around
> > the above.
> > .
>
> ok, that seems easiest. But alternatively do you see any scope to have
> that platform_device_create_ops * ops in the acpi_device struct (so that
> we don't need to create this new API)?

Well, ops and struct acpi_device have different life cycles (the
former is only needed at the init time whereas the latter lives as
long as the platform device based on it), so I'd rather keep them
separate.

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[John Garry]

On 05/07/2022 14:54, Rafael J. Wysocki wrote:
>> So we could factor out by dividing acpi_create_platform_device() into 2x
>> parts: resource get and then platform dev create. But that does not seem
>> wise as we have 2x parts which don't make sense on their own. Or else
>> pass a fixup callback into acpi_create_platform_device(). Any other
>> ideas if we want to go this way?
> Personally, I would do the cleanup that can be done without
> refactoring the library function as the first step, just to reduce the
> amount of changes made in one go if nothing else.
> 
> Next, I'd look at introducing something like
> 
> acpi_create_platform_device_ops(struct acpi_device *adev, const struct
> property_entry *properties, const struct *platform_device_create_ops
> *ops);
> 
> where ops would be a set of callbacks to invoke as a matter of customization.
> 
> Then, acpi_create_platform_device() can be defined as a wrapper around
> the above.
> .
JFYI, I'm trying out this change and it is looking quite disruptive. The 
problems are specifically related to the LPC UART support. Firstly, it 
looks like we require this patch (which was never applied):
https://lore.kernel.org/linux-acpi/1524218846-169934-2-git-send-email-john.garry@huawei.com/#t

Secondly this code in the hisi lpc driver causes an issue:

static int hisi_lpc_acpi_add_child(struct acpi_device *child, void *data)
{
	const char *hid = acpi_device_hid(child);
	struct device *hostdev = data;
	const struct hisi_lpc_acpi_cell *cell;
	struct platform_device *pdev;
	const struct resource *res;
	bool found = false;
	int num_res;
	int ret;

	ret = hisi_lpc_acpi_set_io_res(child, hostdev, &res, &num_res);
	if (ret) {
		dev_warn(hostdev, "set resource fail (%d)\n", ret);
		return ret;
	}

	cell = (struct hisi_lpc_acpi_cell []){
...
		/* 8250-compatible uart */
		{
			.hid = "HISI1031",
			.name = "serial8250",
			.pdata = (struct plat_serial8250_port []) {
				{
***					.iobase = res->start,
					.uartclk = 1843200,
					.iotype = UPIO_PORT,
					.flags = UPF_BOOT_AUTOCONF,
				},
				{}
			},
			.pdata_size = 2 *
				sizeof(struct plat_serial8250_port),
		},
		{}
	};
...

	pdev = platform_device_alloc(cell->name, PLATFORM_DEVID_AUTO);

At ***, above, we need to set the platform data plat_serial8250_port 
iobase at the translated address, but this can only be done after we 
read and translate the resources, which is now all done in the acpi 
platform code - so we have an ordering problem.

Anyway, I'll try to get it working and then send out the patches. We may 
decide it's just not worth it.

Thanks,
John

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

On Tue, Jul 5, 2022 at 10:37 AM John Garry <john.garry@huawei.com> wrote:
>
> On 04/07/2022 20:02, Rafael J. Wysocki wrote:
>
> Hi Rafael,
>
> > I've applied the patch from Yang Yingliang and I thought it would be
> > OK to add your ACK to it based on the conversation so far (please let
> > me know if that's not the case).  Next, I've added my patch rebased on
> > top of it with the tags from you and Greg.
> >
> > The result is on my bleeding-edge branch:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=bleeding-edge
> >
> > and these are the commits in question
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=bleeding-edge&id=d674553009afc9b24cab2bbec71628609edbbb27
> > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=bleeding-edge&id=54872fea6a5ac967ec2272aea525d1438ac6735a
> >
> > Please let me know if you see any issues with them.
> >
>
> I gave these a quick test on my board and they look fine.
>
> Acked-by: John Garry <john.garry@huawei.com>

Thank you!

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

On Fri, Jul 1, 2022 at 2:06 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Fri, Jul 1, 2022 at 1:54 PM John Garry <john.garry@huawei.com> wrote:
> > On 01/07/2022 12:07, Andy Shevchenko wrote:
> > > On Fri, Jul 1, 2022 at 1:06 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > >> On Fri, Jul 1, 2022 at 1:04 PM John Garry <john.garry@huawei.com> wrote:
> > >>> On 30/06/2022 19:13, Rafael J. Wysocki wrote:
>
> ...
>
> > >>> However Yang Yingliang spotted a pre-existing bug in the ACPI probe and
> > >>> sent a fix today (coincidence?):
> > >>>
> > >>> https://lore.kernel.org/lkml/20220701094352.2104998-1-yangyingliang@huawei.com/T/#u
> > >>>
> > >>> And they conflict. This code has been this way for years, so I just
> > >>> suggest Yang Yingliang resends the fix on top off Rafael's change.
> > >>
> > >> Wondering if Yang can actually switch that to use
> > >> platform_device_register_full().
> >
> > Maybe that would work and simplify things. Let me check it.
> >
> > BTW, when we originally upstreamed this driver there was some ACPI
> > platform device registration code which you/we thought could be factored
> > out later. I can't remember it. I was looking through lore but couldn't
> > find it. I don't remember it being so important, though.
>
> My suggestion is definitely not for the fix itself, but as a follow up.
>
> > > And for the record, I think the Fixes even for very rare bug hits
> > > should go first.
> >
> > ok, I have to admit that I was going to feel awkward asking Rafael to
> > deal with this fix by having a v4 on top of it.
>
> I don't think it's a problem as long as we have an immutable branch /
> tag with that patch. Another approach could be that Rafael can take it
> as a precursor for his series and route via ACPI tree, but let's hear
> what he thinks about this himself.

I can take that fix to my tree and rebase my patch on top of it.

Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

On Fri, Jul 1, 2022 at 12:49 PM John Garry <john.garry@huawei.com> wrote:
>
> On 30/06/2022 19:13, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Subject: [PATCH] hisi_lpc: Use acpi_dev_for_each_child()
> >
> > Instead of walking the list of children of an ACPI device directly,
> > use acpi_dev_for_each_child() to carry out an action for all of
> > the given ACPI device's children.
> >
> > This will help to eliminate the children list head from struct
> > acpi_device as it is redundant and it is used in questionable ways
> > in some places (in particular, locking is needed for walking the
> > list pointed to it safely, but it is often missing).
> >
> > While at it, simplify hisi_lpc_acpi_set_io_res() by making it accept
> > a struct acpi_device pointer from the caller, instead of going to
> > struct device and back to get the same result, and clean up confusion
> > regarding hostdev and its ACPI companion in that function.
> >
> > Also remove a redundant check from it.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> This change itself looks fine and I quickly tested, so:
> Reviewed-by: John Garry <john.garry@huawei.com>
>
> However Yang Yingliang spotted a pre-existing bug in the ACPI probe and
> sent a fix today (coincidence?):
>
> https://lore.kernel.org/lkml/20220701094352.2104998-1-yangyingliang@huawei.com/T/#u
>
> And they conflict. This code has been this way for years, so I just
> suggest Yang Yingliang resends the fix on top off Rafael's change.

Well, as I've just said, I can apply both patches just fine.