| Message ID | 20220620150225.1307946-11-mw@semihalf.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ACPI support for DSA | expand |
> static int dsa_port_parse_dsa(struct dsa_port *dp) > { > + /* Cascade switch connection is not supported in ACPI world. */ > + if (is_acpi_node(dp->fwnode)) { > + dev_warn(dp->ds->dev, > + "DSA type is not supported with ACPI, disable port #%d\n", > + dp->index); > + dp->type = DSA_PORT_TYPE_UNUSED; > + return 0; > + } > + Did you try this? I'm not sure it will work correctly. When a switch registers with the DSA core, the core will poke around in DT and fill in various bits of information, including the DSA links. Once that has completed, the core will look at all the switches registered so far and try to determine if it has a complete set, i.e, it has both ends of all DSA links. If it does have a complete set, it then calls the setup methods on each switch, and gets them configured. If it finds it does not have a complete setup, it does nothing, waiting for the next switch to register. So if somebody passed an ACPI description with multiple switches, it is likely to call the setup methods as soon as the first switch is registered. And it might call those same setup methods a second time, when the second switch registers, on both switches. And when the third switch registers, it will probably call the setup methods yet again on all the switches.... You will have a much safer system if you return -EINVAL if you find a DSA link in ACPI. That should abort the switch probe. Andrew
pon., 20 cze 2022 o 20:32 Andrew Lunn <andrew@lunn.ch> napisał(a): > > > static int dsa_port_parse_dsa(struct dsa_port *dp) > > { > > + /* Cascade switch connection is not supported in ACPI world. */ > > + if (is_acpi_node(dp->fwnode)) { > > + dev_warn(dp->ds->dev, > > + "DSA type is not supported with ACPI, disable port #%d\n", > > + dp->index); > > + dp->type = DSA_PORT_TYPE_UNUSED; > > + return 0; > > + } > > + > > Did you try this? I'm not sure it will work correctly. When a switch > registers with the DSA core, the core will poke around in DT and fill > in various bits of information, including the DSA links. Once that has > completed, the core will look at all the switches registered so far > and try to determine if it has a complete set, i.e, it has both ends > of all DSA links. If it does have a complete set, it then calls the > setup methods on each switch, and gets them configured. If it finds it > does not have a complete setup, it does nothing, waiting for the next > switch to register. > > So if somebody passed an ACPI description with multiple switches, it > is likely to call the setup methods as soon as the first switch is > registered. And it might call those same setup methods a second time, > when the second switch registers, on both switches. And when the third > switch registers, it will probably call the setup methods yet again on > all the switches.... > > You will have a much safer system if you return -EINVAL if you find a > DSA link in ACPI. That should abort the switch probe. > I only set a single port to "dsa" label to check if this condition is entered. I see 2 devices in the arm64 tree (fsl-lx2160a-bluebox3.dts and armada-3720-turris-mox.dts) that support cascade switches via "link" property. I don't have access to real life setup (and those seem to not support ACPI anyway...). In case this temporarily would remain as unsupported feature, I agree -EINVAL is a safer solution. Thanks, Marcin
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index 5e11d66f9057..53837dad1cca 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -6,6 +6,8 @@ * Copyright (c) 2016 Andrew Lunn <andrew@lunn.ch> */ +#include <linux/acpi.h> +#include <linux/acpi_mdio.h> #include <linux/device.h> #include <linux/etherdevice.h> #include <linux/err.h> @@ -854,6 +856,7 @@ static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds) static int dsa_switch_setup(struct dsa_switch *ds) { struct dsa_devlink_priv *dl_priv; + char mdio_node_name[] = "mdio"; struct fwnode_handle *fwnode; struct dsa_port *dp; int err; @@ -910,9 +913,16 @@ static int dsa_switch_setup(struct dsa_switch *ds) dsa_slave_mii_bus_init(ds); - fwnode = fwnode_get_named_child_node(ds->dev->fwnode, "mdio"); + /* Update subnode name if operating in the ACPI world. */ + if (is_acpi_node(fwnode)) + strncpy(mdio_node_name, "MDIO", ACPI_NAMESEG_SIZE); - err = of_mdiobus_register(ds->slave_mii_bus, to_of_node(fwnode)); + fwnode = fwnode_get_named_child_node(ds->dev->fwnode, mdio_node_name); + + if (is_acpi_node(fwnode)) + err = acpi_mdiobus_register(ds->slave_mii_bus, fwnode); + else + err = of_mdiobus_register(ds->slave_mii_bus, to_of_node(fwnode)); fwnode_handle_put(fwnode); if (err < 0) goto free_slave_mii_bus; @@ -1374,6 +1384,15 @@ static int dsa_port_parse_user(struct dsa_port *dp, const char *name) static int dsa_port_parse_dsa(struct dsa_port *dp) { + /* Cascade switch connection is not supported in ACPI world. */ + if (is_acpi_node(dp->fwnode)) { + dev_warn(dp->ds->dev, + "DSA type is not supported with ACPI, disable port #%d\n", + dp->index); + dp->type = DSA_PORT_TYPE_UNUSED; + return 0; + } + dp->type = DSA_PORT_TYPE_DSA; return 0; @@ -1524,11 +1543,16 @@ static int dsa_switch_parse_ports_of(struct dsa_switch *ds, struct fwnode_handle *fwnode) { struct fwnode_handle *ports, *port; + char ports_node_name[] = "ports"; struct dsa_port *dp; int err = 0; u32 reg; - ports = fwnode_get_named_child_node(fwnode, "ports"); + /* Update subnode name if operating in the ACPI world. */ + if (is_acpi_node(fwnode)) + strncpy(ports_node_name, "PRTS", ACPI_NAMESEG_SIZE); + + ports = fwnode_get_named_child_node(fwnode, ports_node_name); if (!ports) { /* The second possibility is "ethernet-ports" */ ports = fwnode_get_named_child_node(fwnode, "ethernet-ports"); @@ -1539,7 +1563,10 @@ static int dsa_switch_parse_ports_of(struct dsa_switch *ds, } fwnode_for_each_available_child_node(ports, port) { - err = fwnode_property_read_u32(port, "reg", ®); + if (is_acpi_node(port)) + err = acpi_get_local_address(ACPI_HANDLE_FWNODE(port), ®); + else + err = fwnode_property_read_u32(port, "reg", ®); if (err) { fwnode_handle_put(port); goto out_put_node;
Although the recent switch to device_/fwnode_ API covers the most of modifications required for supporting ACPI, minor additional changes should be applied: * Use different subnode names with ACPI for 'ports' and 'mdio': * Naming of the nodes in ACPI must conform the namespace constraints, with regards to the characters' type and ACPI_NAMESEG_SIZE [1]. Because of that, the 'ports' subnode name used in device tree case, is not appropriate in ACPI world. This patch updates the subnode name depending on the hardware description type in runtime. * Obtain ports indexes from _ADR fields * Same as in the MDIO PHY case, use this standard field instead of parsing 'reg' property from _DSD object. * For now cascade topology remains unsupported in ACPI world, so disable ports connected to other switch devices. [1] https://uefi.org/specs/ACPI/6.4/05_ACPI_Software_Programming_Model/ACPI_Software_Programming_Model.html#acpi-namespace Signed-off-by: Marcin Wojtas <mw@semihalf.com> --- net/dsa/dsa2.c | 35 +++++++++++++++++--- 1 file changed, 31 insertions(+), 4 deletions(-)