| Message ID | 2657553.mvXUDI8C0e@kreacher (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] hisi_lpc: Use acpi_dev_for_each_child() | expand |
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); > } > > /* > > >
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); >> } >> >> /* >> >> >> > .
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!
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); } /*