From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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;
}
On Mon, Jun 13, 2022 at 08:11:36PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
>
> 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;
> }
>
>
>
>
--
With Best Regards,
Andy Shevchenko
Hi Rafael,
On Mon, Jun 13, 2022 at 08:11:36PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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 <[email protected]>
> + 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 <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Heikki Krogerus <[email protected]>
> ---
>
> 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;
> }
>
>
>
--
heikki
Hi Mika,
On Tue, Jun 14, 2022 at 8:07 AM Mika Westerberg
<[email protected]> wrote:
>
> Hi Rafael,
>
> On Mon, Jun 13, 2022 at 08:11:36PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> >
> > 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 <[email protected]>
>
> > + 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
> <[email protected]> wrote:
> >
> > Hi Rafael,
> >
> > On Mon, Jun 13, 2022 at 08:11:36PM +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > >
> > > 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
<[email protected]> 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
> > <[email protected]> wrote:
> > >
> > > Hi Rafael,
> > >
> > > On Mon, Jun 13, 2022 at 08:11:36PM +0200, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > ---
> > > >
> > > > 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!