| Message ID | 12026357.O9o76ZdvQC@kreacher |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hisi_lpc: Use acpi_dev_for_each_child() | expand |
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
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!
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>
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
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().
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.
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
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.
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
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.
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.
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!
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
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
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().
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
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.
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.
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!
> 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
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.
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
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); } /*