| Message ID | 2851774.e9J7NaK4W3@kreacher (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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; > } > > > >
Hi Rafael, 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). > > 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); Can you move the above comment here too? Otherwise looks good to me, Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> > + return acpi_find_child_by_adr(ACPI_COMPANION(dev->parent), > + tb_to_usb4_port_device(dev)->port->port); > return NULL; > } > > >
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). > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.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; > } > > >
Hi Mika, On Tue, Jun 14, 2022 at 8:07 AM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > Hi Rafael, > > 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). > > > > 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); > > Can you move the above comment here too? Do you mean to move the comment from tb_acpi_find_port() right here or before the if (tb_is_switch(dev)) line above? I think that tb_acpi_switch_find_companion() would be a better place for that comment. At least it would match the code passing 0 to acpi_find_child_device() in there. > Otherwise looks good to me, > > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > + return acpi_find_child_by_adr(ACPI_COMPANION(dev->parent), > > + tb_to_usb4_port_device(dev)->port->port); > > return NULL; > > } Thanks!
On Tue, Jun 14, 2022 at 08:25:53PM +0200, Rafael J. Wysocki wrote: > Hi Mika, > > On Tue, Jun 14, 2022 at 8:07 AM Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > > > Hi Rafael, > > > > 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). > > > > > > 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); > > > > Can you move the above comment here too? > > Do you mean to move the comment from tb_acpi_find_port() right here or > before the if (tb_is_switch(dev)) line above? > > I think that tb_acpi_switch_find_companion() would be a better place > for that comment. At least it would match the code passing 0 to > acpi_find_child_device() in there. Yes, I agree (as long as the comment stays somewhere close ;-))
On Wed, Jun 15, 2022 at 8:27 AM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > On Tue, Jun 14, 2022 at 08:25:53PM +0200, Rafael J. Wysocki wrote: > > Hi Mika, > > > > On Tue, Jun 14, 2022 at 8:07 AM Mika Westerberg > > <mika.westerberg@linux.intel.com> wrote: > > > > > > Hi Rafael, > > > > > > 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). > > > > > > > > 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); > > > > > > Can you move the above comment here too? > > > > Do you mean to move the comment from tb_acpi_find_port() right here or > > before the if (tb_is_switch(dev)) line above? > > > > I think that tb_acpi_switch_find_companion() would be a better place > > for that comment. At least it would match the code passing 0 to > > acpi_find_child_device() in there. > > Yes, I agree (as long as the comment stays somewhere close ;-)) OK, I'll move it to tb_acpi_switch_find_companion() then. Thanks!
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; }