From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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);
}
/*
On Wed, Jun 29, 2022 at 3:34 PM John Garry <[email protected]> wrote:
>
> On 29/06/2022 13:55, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > 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 <[email protected]>
>
> 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 <[email protected]>
Thanks!
> Can you route this through one of your trees?
Yes, I will do that.
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
Acked-by: John Garry <[email protected]>
---
-> 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);
}
/*
On 29/06/2022 13:55, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
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 <[email protected]>
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
On Wed, Jun 29, 2022 at 3:47 PM Rafael J. Wysocki <[email protected]> wrote:
>
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> Acked-by: John Garry <[email protected]>
> ---
>
> -> 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);
> }
>
> /*
>
>
>
On 30/06/2022 13:48, Rafael J. Wysocki wrote:
> On Wed, Jun 29, 2022 at 3:47 PM Rafael J. Wysocki <[email protected]> wrote:
>>
>> From: Rafael J. Wysocki <[email protected]>
>>
>> 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 <[email protected]>
>> Acked-by: John Garry <[email protected]>
>> ---
>>
>> -> 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);
>> }
>>
>> /*
>>
>>
>>
> .
On Thu, Jun 30, 2022 at 3:37 PM John Garry <[email protected]> wrote:
>
> On 30/06/2022 13:48, Rafael J. Wysocki wrote:
> > On Wed, Jun 29, 2022 at 3:47 PM Rafael J. Wysocki <[email protected]> wrote:
> >>
> >> From: Rafael J. Wysocki <[email protected]>
> >>
> >> 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!
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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;
}
On Thu, Jun 30, 2022 at 08:13:52PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
> 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 <[email protected]>
Reviewed-by: Greg Kroah-Hartman <[email protected]>
On 30/06/2022 19:13, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
> 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 <[email protected]>
This change itself looks fine and I quickly tested, so:
Reviewed-by: John Garry <[email protected]>
However Yang Yingliang spotted a pre-existing bug in the ACPI probe and
sent a fix today (coincidence?):
https://lore.kernel.org/lkml/[email protected]/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
On Fri, Jul 1, 2022 at 1:06 PM Andy Shevchenko
<[email protected]> wrote:
> On Fri, Jul 1, 2022 at 1:04 PM John Garry <[email protected]> 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/[email protected]/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
On Fri, Jul 1, 2022 at 1:04 PM John Garry <[email protected]> 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/[email protected]/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
On Fri, Jul 1, 2022 at 1:54 PM John Garry <[email protected]> wrote:
> On 01/07/2022 12:07, Andy Shevchenko wrote:
> > On Fri, Jul 1, 2022 at 1:06 PM Andy Shevchenko
> > <[email protected]> wrote:
> >> On Fri, Jul 1, 2022 at 1:04 PM John Garry <[email protected]> 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/[email protected]/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
On Fri, Jul 1, 2022 at 2:06 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Fri, Jul 1, 2022 at 1:54 PM John Garry <[email protected]> wrote:
> > On 01/07/2022 12:07, Andy Shevchenko wrote:
> > > On Fri, Jul 1, 2022 at 1:06 PM Andy Shevchenko
> > > <[email protected]> wrote:
> > >> On Fri, Jul 1, 2022 at 1:04 PM John Garry <[email protected]> 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/[email protected]/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.
On Fri, Jul 1, 2022 at 12:49 PM John Garry <[email protected]> wrote:
>
> On 30/06/2022 19:13, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> > 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 <[email protected]>
>
> This change itself looks fine and I quickly tested, so:
> Reviewed-by: John Garry <[email protected]>
>
> However Yang Yingliang spotted a pre-existing bug in the ACPI probe and
> sent a fix today (coincidence?):
>
> https://lore.kernel.org/lkml/[email protected]/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.
Hi John,
On Fri, Jul 1, 2022 at 2:18 PM John Garry <[email protected]> wrote:
>
> On 01/07/2022 13:05, Andy Shevchenko wrote:
> > On Fri, Jul 1, 2022 at 1:54 PM John Garry <[email protected]> wrote:
> >> On 01/07/2022 12:07, Andy Shevchenko wrote:
> >>> On Fri, Jul 1, 2022 at 1:06 PM Andy Shevchenko
> >>> <[email protected]> wrote:
> >>>> On Fri, Jul 1, 2022 at 1:04 PM John Garry <[email protected]> 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/[email protected]/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!
On Tue, Jul 5, 2022 at 11:38 AM Andy Shevchenko
<[email protected]> wrote:
> On Tue, Jul 5, 2022 at 10:37 AM John Garry <[email protected]> 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 <[email protected]>
>
> 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
On Tue, Jul 5, 2022 at 10:37 AM John Garry <[email protected]> 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 <[email protected]>
John, I believe now you may send a formal clean up to convert to platform_device
--
With Best Regards,
Andy Shevchenko
On Tue, Jul 5, 2022 at 12:16 PM John Garry <[email protected]> wrote:
> On 05/07/2022 10:39, Andy Shevchenko wrote:
> > On Tue, Jul 5, 2022 at 11:38 AM Andy Shevchenko
> > <[email protected]> wrote:
> >> On Tue, Jul 5, 2022 at 10:37 AM John Garry<[email protected]> 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
On Tue, Jul 5, 2022 at 12:16 PM John Garry <[email protected]> wrote:
>
> On 05/07/2022 10:39, Andy Shevchenko wrote:
> > On Tue, Jul 5, 2022 at 11:38 AM Andy Shevchenko
> > <[email protected]> wrote:
> >> On Tue, Jul 5, 2022 at 10:37 AM John Garry<[email protected]> 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<[email protected]>
> >> 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.
On Tue, Jul 5, 2022 at 10:37 AM John Garry <[email protected]> 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 <[email protected]>
Thank you!
On Tue, Jul 5, 2022 at 5:17 PM John Garry <[email protected]> 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.