| Message ID | 1843211.tdWV9SEqCh@kreacher |
|---|---|
| Headers | show |
| Series | ACPI: Get rid of the list of children in struct acpi_device | expand |
On Thu, Jun 09, 2022 at 03:44:27PM +0200, Rafael J. Wysocki wrote: > Hi All, > > Confusingly enough, the ACPI subsystem stores the information on the given ACPI > device's children in two places: as the list of children in struct acpi_device > and (as a result of device registration) in the list of children in the embedded > struct device. > > These two lists agree with each other most of the time, but not always (like in > error paths in some cases), and the list of children in struct acpi_device is > not generally safe to use without locking. In principle, it should always be > walked under acpi_device_lock, but in practice holding acpi_scan_lock is > sufficient for that too. However, its users may not know whether or not > they operate under acpi_scan_lock and at least in some cases it is not accessed > in a safe way (note that ACPI devices may go away as a result of hot-remove, > unlike OF nodes). Hmm... Does it true for DT overlays? Not an expert in DT overlays, though, adding Rob and Frank. > For this reason, it is better to consolidate the code that needs to walk the > children of an ACPI device which is the purpose of this patch series. > > Overall, it switches over all of the users of the list of children in struct > acpi_device to using helpers based on the driver core's mechanics and finally > drops that list, but some extra cleanups are done on the way. > > Please refer to the patch changelogs for details. I'm going to look the individual patches.
On Thu, Jun 09, 2022 at 03:54:40PM +0200, 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 > in order to find the child matching a given bus address, use > acpi_find_child_by_adr() for this purpose. ... > if (!adev) > return NULL; Already checked in the below call, IIUC. Hence can be removed. > + return acpi_find_child_by_adr(adev, port->port);
On Thu, Jun 9, 2022 at 5:26 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Jun 09, 2022 at 03:54:40PM +0200, 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 > > in order to find the child matching a given bus address, use > > acpi_find_child_by_adr() for this purpose. > > ... > > > if (!adev) > > return NULL; > > Already checked in the below call, IIUC. Hence can be removed. Yes, it can, will update. > > > + return acpi_find_child_by_adr(adev, port->port); > > --
On Thu, Jun 09, 2022 at 04:03:37PM +0200, 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. ... > + return acpi_dev_for_each_child(device, acpi_video_bus_get_one_device, > + video); Perhaps one line?
On Thu, Jun 9, 2022 at 5:23 PM Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote: > > Thanks Rafael. This looks mostly good but I have a doubt on the error > handling, see below. > > > +static int sdw_acpi_check_duplicate(struct acpi_device *adev, void *data) > > +{ > > + struct sdw_acpi_child_walk_data *cwd = data; > > + struct sdw_bus *bus = cwd->bus; > > + struct sdw_slave_id id; > > + > > + if (adev == cwd->adev) > > + return 0; > > + > > + if (!find_slave(bus, adev, &id)) > > + return 0; > > + > > + if (cwd->id.sdw_version != id.sdw_version || cwd->id.mfg_id != id.mfg_id || > > + cwd->id.part_id != id.part_id || cwd->id.class_id != id.class_id) > > + return 0; > > + > > + if (cwd->id.unique_id != id.unique_id) { > > + dev_dbg(bus->dev, > > + "Valid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n", > > + cwd->id.unique_id, id.unique_id, cwd->id.mfg_id, > > + cwd->id.part_id); > > + cwd->ignore_unique_id = false; > > + return 0; > > + } > > + > > + dev_err(bus->dev, > > + "Invalid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n", > > + cwd->id.unique_id, id.unique_id, cwd->id.mfg_id, cwd->id.part_id); > > + return -ENODEV; > > if this error happens, I would guess it's reported .... > > > +} > > + > > +static int sdw_acpi_find_one(struct acpi_device *adev, void *data) > > +{ > > + struct sdw_bus *bus = data; > > + struct sdw_acpi_child_walk_data cwd = { > > + .bus = bus, > > + .adev = adev, > > + .ignore_unique_id = true, > > + }; > > + int ret; > > + > > + if (!find_slave(bus, adev, &cwd.id)) > > + return 0; > > + > > + /* Brute-force O(N^2) search for duplicates. */ > > + ret = acpi_dev_for_each_child(ACPI_COMPANION(bus->dev), > > + sdw_acpi_check_duplicate, &cwd); > > + if (ret) > > + return ret; > > ... here, but I don't see this being propagated further... > > > + > > + if (cwd.ignore_unique_id) > > + cwd.id.unique_id = SDW_IGNORED_UNIQUE_ID; > > + > > + /* Ignore errors and continue. */ > > + sdw_slave_add(bus, &cwd.id, acpi_fwnode_handle(adev)); > > + return 0; > > +} > > + > > /* > > * sdw_acpi_find_slaves() - Find Slave devices in Master ACPI node > > * @bus: SDW bus instance > > @@ -135,8 +200,7 @@ static bool find_slave(struct sdw_bus *b > > */ > > int sdw_acpi_find_slaves(struct sdw_bus *bus) > > { > > - struct acpi_device *adev, *parent; > > - struct acpi_device *adev2, *parent2; > > + struct acpi_device *parent; > > > > parent = ACPI_COMPANION(bus->dev); > > if (!parent) { > > @@ -144,52 +208,7 @@ int sdw_acpi_find_slaves(struct sdw_bus > > return -ENODEV; > > } > > > > - list_for_each_entry(adev, &parent->children, node) { > > - struct sdw_slave_id id; > > - struct sdw_slave_id id2; > > - bool ignore_unique_id = true; > > - > > - if (!find_slave(bus, adev, &id)) > > - continue; > > - > > - /* brute-force O(N^2) search for duplicates */ > > - parent2 = parent; > > - list_for_each_entry(adev2, &parent2->children, node) { > > - > > - if (adev == adev2) > > - continue; > > - > > - if (!find_slave(bus, adev2, &id2)) > > - continue; > > - > > - if (id.sdw_version != id2.sdw_version || > > - id.mfg_id != id2.mfg_id || > > - id.part_id != id2.part_id || > > - id.class_id != id2.class_id) > > - continue; > > - > > - if (id.unique_id != id2.unique_id) { > > - dev_dbg(bus->dev, > > - "Valid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n", > > - id.unique_id, id2.unique_id, id.mfg_id, id.part_id); > > - ignore_unique_id = false; > > - } else { > > - dev_err(bus->dev, > > - "Invalid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n", > > - id.unique_id, id2.unique_id, id.mfg_id, id.part_id); > > - return -ENODEV; > > - } > > - } > > - > > - if (ignore_unique_id) > > - id.unique_id = SDW_IGNORED_UNIQUE_ID; > > - > > - /* > > - * don't error check for sdw_slave_add as we want to continue > > - * adding Slaves > > - */ > > - sdw_slave_add(bus, &id, acpi_fwnode_handle(adev)); > > - } > > + acpi_dev_for_each_child(parent, sdw_acpi_find_one, bus); > > ... here? > > It looks like a change in the error handling flow where > sdw_acpi_find_slaves() is now returning 0 (success) always? > > Shouldn't the return of sdw_acpi_find_one() be trapped, e.g. with > > return acpi_dev_for_each_child(parent, sdw_acpi_find_one, bus); Sure, I'll do that. Thanks!
On Thu, Jun 9, 2022 at 6:21 PM Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote: > > > >> Shouldn't the return of sdw_acpi_find_one() be trapped, e.g. with > >> > >> return acpi_dev_for_each_child(parent, sdw_acpi_find_one, bus); > > > > Sure, I'll do that. Thanks! > > I also added this EXPORT_SYMBOL to work-around link errors, not sure if > this is in your tree already? One of the previous patches in the series is adding the export. > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c > > index 86fa61a21826c..ade6259c19af6 100644 > > --- a/drivers/acpi/bus.c > > +++ b/drivers/acpi/bus.c > > @@ -1113,6 +1113,7 @@ int acpi_dev_for_each_child(struct acpi_device *adev, > > > > return device_for_each_child(&adev->dev, &adwc, > acpi_dev_for_one_check); > > } > > +EXPORT_SYMBOL_GPL(acpi_dev_for_each_child); > >
On 6/9/22 11:12, Andy Shevchenko wrote: > On Thu, Jun 09, 2022 at 03:44:27PM +0200, Rafael J. Wysocki wrote: >> Hi All, >> >> Confusingly enough, the ACPI subsystem stores the information on the given ACPI >> device's children in two places: as the list of children in struct acpi_device >> and (as a result of device registration) in the list of children in the embedded >> struct device. >> >> These two lists agree with each other most of the time, but not always (like in >> error paths in some cases), and the list of children in struct acpi_device is >> not generally safe to use without locking. In principle, it should always be >> walked under acpi_device_lock, but in practice holding acpi_scan_lock is >> sufficient for that too. However, its users may not know whether or not >> they operate under acpi_scan_lock and at least in some cases it is not accessed >> in a safe way (note that ACPI devices may go away as a result of hot-remove, > >> unlike OF nodes). > > Hmm... Does it true for DT overlays? Not an expert in DT overlays, though, > adding Rob and Frank. DT nodes can be removed. The devicetree code uses devtree_lock and of_mutex as needed for protection. -Frank > >> For this reason, it is better to consolidate the code that needs to walk the >> children of an ACPI device which is the purpose of this patch series. >> >> Overall, it switches over all of the users of the list of children in struct >> acpi_device to using helpers based on the driver core's mechanics and finally >> drops that list, but some extra cleanups are done on the way. >> >> Please refer to the patch changelogs for details. > > I'm going to look the individual patches. >
On Thu, Jun 09, 2022 at 03:54:40PM +0200, 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 > in order to find the child matching a given bus address, use > acpi_find_child_by_adr() for this purpose. > > Apart from simplifying the code, 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/thunderbolt/acpi.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > Index: linux-pm/drivers/thunderbolt/acpi.c > =================================================================== > --- linux-pm.orig/drivers/thunderbolt/acpi.c > +++ linux-pm/drivers/thunderbolt/acpi.c > @@ -304,8 +304,6 @@ static bool tb_acpi_bus_match(struct dev > static struct acpi_device *tb_acpi_find_port(struct acpi_device *adev, > const struct tb_port *port) > { > - struct acpi_device *port_adev; > - > if (!adev) > return NULL; > > @@ -313,12 +311,7 @@ static struct acpi_device *tb_acpi_find_ > * Device routers exists under the downstream facing USB4 port > * of the parent router. Their _ADR is always 0. > */ > - list_for_each_entry(port_adev, &adev->children, node) { > - if (acpi_device_adr(port_adev) == port->port) > - return port_adev; > - } > - > - return NULL; > + return acpi_find_child_by_adr(adev, port->port); > } > > static struct acpi_device *tb_acpi_switch_find_companion(struct tb_switch *sw) I don't think you need tb_acpi_find_port() anymore. You can just call acpi_find_child_by_ard(ACPI_COMPANION(...), port->port) directly, no? thanks,
On Fri, Jun 10, 2022 at 8:46 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > On Thu, Jun 09, 2022 at 03:54:40PM +0200, 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 > > in order to find the child matching a given bus address, use > > acpi_find_child_by_adr() for this purpose. > > > > Apart from simplifying the code, 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/thunderbolt/acpi.c | 9 +-------- > > 1 file changed, 1 insertion(+), 8 deletions(-) > > > > Index: linux-pm/drivers/thunderbolt/acpi.c > > =================================================================== > > --- linux-pm.orig/drivers/thunderbolt/acpi.c > > +++ linux-pm/drivers/thunderbolt/acpi.c > > @@ -304,8 +304,6 @@ static bool tb_acpi_bus_match(struct dev > > static struct acpi_device *tb_acpi_find_port(struct acpi_device *adev, > > const struct tb_port *port) > > { > > - struct acpi_device *port_adev; > > - > > if (!adev) > > return NULL; > > > > @@ -313,12 +311,7 @@ static struct acpi_device *tb_acpi_find_ > > * Device routers exists under the downstream facing USB4 port > > * of the parent router. Their _ADR is always 0. > > */ > > - list_for_each_entry(port_adev, &adev->children, node) { > > - if (acpi_device_adr(port_adev) == port->port) > > - return port_adev; > > - } > > - > > - return NULL; > > + return acpi_find_child_by_adr(adev, port->port); > > } > > > > static struct acpi_device *tb_acpi_switch_find_companion(struct tb_switch *sw) > > I don't think you need tb_acpi_find_port() anymore. You can just call > acpi_find_child_by_ard(ACPI_COMPANION(...), port->port) directly, no? Technically I can, but I thought that the comment in tb_acpi_find_port() was worth retaining.
On Fri, Jun 10, 2022 at 8:47 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > On Thu, Jun 09, 2022 at 03:56:42PM +0200, 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 > > in order to find the child matching a given bus address, use > > acpi_find_child_by_adr() for this purpose. > > > > Apart from simplifying the code, 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/usb/core/usb-acpi.c | 9 +-------- > > 1 file changed, 1 insertion(+), 8 deletions(-) > > > > Index: linux-pm/drivers/usb/core/usb-acpi.c > > =================================================================== > > --- linux-pm.orig/drivers/usb/core/usb-acpi.c > > +++ linux-pm/drivers/usb/core/usb-acpi.c > > @@ -127,17 +127,10 @@ out: > > static struct acpi_device *usb_acpi_find_port(struct acpi_device *parent, > > int raw) > > { > > - struct acpi_device *adev; > > - > > if (!parent) > > return NULL; > > > > - list_for_each_entry(adev, &parent->children, node) { > > - if (acpi_device_adr(adev) == raw) > > - return adev; > > - } > > - > > - return acpi_find_child_device(parent, raw, false); > > + return acpi_find_child_by_adr(parent, raw); > > } > > > > static struct acpi_device * > > I think usb_acpi_find_port() can also be dropped now. Yes, it can. I'm dropping it in the new version of the patch to be posted.
On Mon, Jun 13, 2022 at 08:39:37PM +0200, 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 > in order to find the child matching a given bus address, use > acpi_find_child_by_adr() for this purpose. > > Also notice that if acpi_find_child_by_adr() doesn't find a matching > child, acpi_find_child_device() will not find it too, so directly > replace usb_acpi_find_port() in usb_acpi_get_companion_for_port() with > acpi_find_child_by_adr() and drop it entirely. > > Apart from simplifying the code, 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). Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > v1 -> v2: > * Drop usb_acpi_find_port() (Heikki, Andy). > * Change the subject accordingly. > > --- > drivers/usb/core/usb-acpi.c | 18 +----------------- > 1 file changed, 1 insertion(+), 17 deletions(-) > > Index: linux-pm/drivers/usb/core/usb-acpi.c > =================================================================== > --- linux-pm.orig/drivers/usb/core/usb-acpi.c > +++ linux-pm/drivers/usb/core/usb-acpi.c > @@ -124,22 +124,6 @@ out: > */ > #define USB_ACPI_LOCATION_VALID (1 << 31) > > -static struct acpi_device *usb_acpi_find_port(struct acpi_device *parent, > - int raw) > -{ > - struct acpi_device *adev; > - > - if (!parent) > - return NULL; > - > - list_for_each_entry(adev, &parent->children, node) { > - if (acpi_device_adr(adev) == raw) > - return adev; > - } > - > - return acpi_find_child_device(parent, raw, false); > -} > - > static struct acpi_device * > usb_acpi_get_companion_for_port(struct usb_port *port_dev) > { > @@ -170,7 +154,7 @@ usb_acpi_get_companion_for_port(struct u > port1 = port_dev->portnum; > } > > - return usb_acpi_find_port(adev, port1); > + return acpi_find_child_by_adr(adev, port1); > } > > static struct acpi_device * > > >
On Mon, Jun 13, 2022 at 08:11:36PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Use acpi_find_child_by_adr() to find the child matching a given bus > address instead of tb_acpi_find_port() that walks the list of children > of an ACPI device directly for this purpose and drop the latter. > > Apart from simplifying the code, 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). Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > v1 -> v2: > * Drop tb_acpi_find_port() (Heikki, Andy). > * Change the subject accordingly > > --- > drivers/thunderbolt/acpi.c | 27 ++++----------------------- > 1 file changed, 4 insertions(+), 23 deletions(-) > > Index: linux-pm/drivers/thunderbolt/acpi.c > =================================================================== > --- linux-pm.orig/drivers/thunderbolt/acpi.c > +++ linux-pm/drivers/thunderbolt/acpi.c > @@ -301,26 +301,6 @@ static bool tb_acpi_bus_match(struct dev > return tb_is_switch(dev) || tb_is_usb4_port_device(dev); > } > > -static struct acpi_device *tb_acpi_find_port(struct acpi_device *adev, > - const struct tb_port *port) > -{ > - struct acpi_device *port_adev; > - > - if (!adev) > - return NULL; > - > - /* > - * Device routers exists under the downstream facing USB4 port > - * of the parent router. Their _ADR is always 0. > - */ > - list_for_each_entry(port_adev, &adev->children, node) { > - if (acpi_device_adr(port_adev) == port->port) > - return port_adev; > - } > - > - return NULL; > -} > - > static struct acpi_device *tb_acpi_switch_find_companion(struct tb_switch *sw) > { > struct acpi_device *adev = NULL; > @@ -331,7 +311,8 @@ static struct acpi_device *tb_acpi_switc > struct tb_port *port = tb_port_at(tb_route(sw), parent_sw); > struct acpi_device *port_adev; > > - port_adev = tb_acpi_find_port(ACPI_COMPANION(&parent_sw->dev), port); > + port_adev = acpi_find_child_by_adr(ACPI_COMPANION(&parent_sw->dev), > + port->port); > if (port_adev) > adev = acpi_find_child_device(port_adev, 0, false); > } else { > @@ -364,8 +345,8 @@ static struct acpi_device *tb_acpi_find_ > if (tb_is_switch(dev)) > return tb_acpi_switch_find_companion(tb_to_switch(dev)); > else if (tb_is_usb4_port_device(dev)) > - return tb_acpi_find_port(ACPI_COMPANION(dev->parent), > - tb_to_usb4_port_device(dev)->port); > + return acpi_find_child_by_adr(ACPI_COMPANION(dev->parent), > + tb_to_usb4_port_device(dev)->port->port); > return NULL; > } > > > >