[PATCH RFC net-next 0/7] Another attempt at moving mv88e6xxx forward

[PATCH RFC net-next 0/7] Another attempt at moving mv88e6xxx forward

[Russell King (Oracle)]

Hi,

This is another attempt to move the mv88e6xxx driver forward so that we
can eventually switch it to use phylink_pcs and become a non-legacy
driver.

The issue is that in order to switch to phylink_pcs, we need DSA and CPU
ports to be known to phylink, otherwise the PCS code will not be called.
In order for such ports to be known to phylink, we need to provide
phylink with a configuration, and mv88e6xxx has a history of not
specifying the configuration in firmware, but the driver internally
handling that. This is fine, but it means we can't use phylink for such
ports - and thus converting them to phylink_pcs can cause regressions.

Therefore, this series provides a way for a software-node configuration
to be provided to DSA by the driver, which will then be used only for
phylink to parse.

Some of this patch set comes from an idea from Vladimir, but
re-implemented in a substantially different way.

 drivers/base/swnode.c            |  14 +++-
 drivers/net/dsa/mv88e6xxx/chip.c | 157 ++++++++++++++++++++++++++++-----------
 drivers/net/phy/phylink.c        |  32 ++++++++
 include/linux/phylink.h          |   1 +
 include/linux/property.h         |   4 +
 include/net/dsa.h                |   3 +
 net/dsa/port.c                   |  33 ++++++--
 7 files changed, 191 insertions(+), 53 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[PATCH RFC net-next 1/7] software node: allow named software node to be created

[Russell King]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Allow a named software node to be created, which is needed for software
nodes for a fixed-link specification for DSA.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/base/swnode.c    | 14 ++++++++++++--
 include/linux/property.h |  4 ++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 1886995a0b3a..e4d07e1655a9 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -911,8 +911,9 @@ void software_node_unregister(const struct software_node *node)
 EXPORT_SYMBOL_GPL(software_node_unregister);
 
 struct fwnode_handle *
-fwnode_create_software_node(const struct property_entry *properties,
-			    const struct fwnode_handle *parent)
+fwnode_create_named_software_node(const struct property_entry *properties,
+				  const struct fwnode_handle *parent,
+				  const char *name)
 {
 	struct fwnode_handle *fwnode;
 	struct software_node *node;
@@ -930,6 +931,7 @@ fwnode_create_software_node(const struct property_entry *properties,
 		return ERR_CAST(node);
 
 	node->parent = p ? p->node : NULL;
+	node->name = name;
 
 	fwnode = swnode_register(node, p, 1);
 	if (IS_ERR(fwnode))
@@ -937,6 +939,14 @@ fwnode_create_software_node(const struct property_entry *properties,
 
 	return fwnode;
 }
+EXPORT_SYMBOL_GPL(fwnode_create_named_software_node);
+
+struct fwnode_handle *
+fwnode_create_software_node(const struct property_entry *properties,
+			    const struct fwnode_handle *parent)
+{
+	return fwnode_create_named_software_node(properties, parent, NULL);
+}
 EXPORT_SYMBOL_GPL(fwnode_create_software_node);
 
 void fwnode_remove_software_node(struct fwnode_handle *fwnode)
diff --git a/include/linux/property.h b/include/linux/property.h
index 0a29db15ff34..f7100e836eb4 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -495,6 +495,10 @@ void software_node_unregister(const struct software_node *node);
 struct fwnode_handle *
 fwnode_create_software_node(const struct property_entry *properties,
 			    const struct fwnode_handle *parent);
+struct fwnode_handle *
+fwnode_create_named_software_node(const struct property_entry *properties,
+				  const struct fwnode_handle *parent,
+				  const char *name);
 void fwnode_remove_software_node(struct fwnode_handle *fwnode);
 
 int device_add_software_node(struct device *dev, const struct software_node *node);
-- 
2.30.2

[PATCH RFC net-next 2/7] net: phylink: provide phylink_find_max_speed()

[Russell King (Oracle)]

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 32 ++++++++++++++++++++++++++++++++
 include/linux/phylink.h   |  1 +
 2 files changed, 33 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index f7da96f0c75b..5cd29cceaf93 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -426,6 +426,38 @@ static unsigned long phylink_cap_from_speed_duplex(int speed,
 	return 0;
 }
 
+/**
+ * phylink_find_max_speed
+ * @caps: bitmask of MAC capabilities
+ * @speed: pointer to int for speed
+ * @duplex: pointer to int for duplex
+ */
+int phylink_find_max_speed(unsigned long caps, int *speed, int *duplex)
+{
+	static const int duplex_order[] = {
+		DUPLEX_FULL,
+		DUPLEX_HALF,
+	};
+	int i, j;
+
+	*speed = SPEED_UNKNOWN;
+	*duplex = DUPLEX_UNKNOWN;
+
+	for (j = 0; j < ARRAY_SIZE(duplex_order); j++) {
+		for (i = 0; i < ARRAY_SIZE(phylink_caps_params); i++) {
+			if (phylink_caps_params[i].duplex == duplex_order[j] &&
+			    caps & phylink_caps_params[i].mask) {
+				*speed = phylink_caps_params[i].speed;
+				*duplex = phylink_caps_params[i].duplex;
+				return 0;
+			}
+		}
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(phylink_find_max_speed);
+
 /**
  * phylink_get_capabilities() - get capabilities for a given MAC
  * @interface: phy interface mode defined by &typedef phy_interface_t
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 9ff56b050584..f8e20ff1e70e 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -557,6 +557,7 @@ void pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
 		 phy_interface_t interface, int speed, int duplex);
 #endif
 
+int phylink_find_max_speed(unsigned long caps, int *speed, int *duplex);
 void phylink_caps_to_linkmodes(unsigned long *linkmodes, unsigned long caps);
 unsigned long phylink_get_capabilities(phy_interface_t interface,
 				       unsigned long mac_capabilities,
-- 
2.30.2

[PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

[Russell King (Oracle)]

In preparation for supporting the use of software nodes to setup
phylink, switch DSA to use fwnode_get_phy_mode() to retrieve the
phy interface mode, rather than using of_get_phy_mode() which is
DT specific.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 net/dsa/port.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index 67ad1adec2a2..07f9cb374a5d 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -1678,13 +1678,9 @@ static const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
 int dsa_port_phylink_create(struct dsa_port *dp)
 {
 	struct dsa_switch *ds = dp->ds;
-	phy_interface_t mode;
+	struct fwnode_handle *fwnode;
 	struct phylink *pl;
-	int err;
-
-	err = of_get_phy_mode(dp->dn, &mode);
-	if (err)
-		mode = PHY_INTERFACE_MODE_NA;
+	int mode;
 
 	/* Presence of phylink_mac_link_state or phylink_mac_an_restart is
 	 * an indicator of a legacy phylink driver.
@@ -1696,8 +1692,14 @@ int dsa_port_phylink_create(struct dsa_port *dp)
 	if (ds->ops->phylink_get_caps)
 		ds->ops->phylink_get_caps(ds, dp->index, &dp->pl_config);
 
-	pl = phylink_create(&dp->pl_config, of_fwnode_handle(dp->dn),
-			    mode, &dsa_port_phylink_mac_ops);
+	fwnode = of_fwnode_handle(dp->dn);
+
+	mode = fwnode_get_phy_mode(fwnode);
+	if (mode < 0)
+		mode = PHY_INTERFACE_MODE_NA;
+
+	pl = phylink_create(&dp->pl_config, fwnode, mode,
+			    &dsa_port_phylink_mac_ops);
 	if (IS_ERR(pl)) {
 		pr_err("error creating PHYLINK: %ld\n", PTR_ERR(pl));
 		return PTR_ERR(pl);
-- 
2.30.2

[PATCH RFC net-next 4/7] net: dsa: add ability for switch driver to provide a swnode

[Russell King (Oracle)]

Add a method to dsa_switch_ops to allow a switch driver to override
the fwnode used to setup phylink for a port. This will be used for
the Marvell 88e6xxx driver to provide its "maximum interface" and
"maximum speed" mode to keep compatibility with existing behaviour
for CPU and DSA ports via a swnode-backed fwnode.

We need to release the swnode after phylink_create() has completed
no matter what the outcome was.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 include/net/dsa.h |  3 +++
 net/dsa/port.c    | 12 ++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index a15f17a38eca..764f3e74448b 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -854,6 +854,9 @@ struct dsa_switch_ops {
 	 */
 	int	(*port_setup)(struct dsa_switch *ds, int port);
 	void	(*port_teardown)(struct dsa_switch *ds, int port);
+	struct fwnode_handle *(*port_get_fwnode)(struct dsa_switch *ds,
+						 int port,
+						 struct fwnode_handle *h);
 
 	u32	(*get_phy_flags)(struct dsa_switch *ds, int port);
 
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 07f9cb374a5d..c30e3a7d2145 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -1693,6 +1693,15 @@ int dsa_port_phylink_create(struct dsa_port *dp)
 		ds->ops->phylink_get_caps(ds, dp->index, &dp->pl_config);
 
 	fwnode = of_fwnode_handle(dp->dn);
+	if (ds->ops->port_get_fwnode) {
+		fwnode = ds->ops->port_get_fwnode(ds, dp->index, fwnode);
+		if (IS_ERR(fwnode)) {
+			dev_err(ds->dev,
+				"Failed to get fwnode for port %d: %pe\n",
+				dp->index, fwnode);
+			return PTR_ERR(fwnode);
+		}
+	}
 
 	mode = fwnode_get_phy_mode(fwnode);
 	if (mode < 0)
@@ -1700,6 +1709,9 @@ int dsa_port_phylink_create(struct dsa_port *dp)
 
 	pl = phylink_create(&dp->pl_config, fwnode, mode,
 			    &dsa_port_phylink_mac_ops);
+
+	fwnode_remove_software_node(fwnode);
+
 	if (IS_ERR(pl)) {
 		pr_err("error creating PHYLINK: %ld\n", PTR_ERR(pl));
 		return PTR_ERR(pl);
-- 
2.30.2

[PATCH RFC net-next 5/7] net: dsa: avoid DT validation for drivers which provide default config

[Russell King (Oracle)]

When a DSA driver (e.g. mv88e6xxx) provides a default configuration,
avoid validating the DT description as missing elements will be
provided by the DSA driver.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 net/dsa/port.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index c30e3a7d2145..23d9970c02d3 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -1951,6 +1951,9 @@ static void dsa_shared_port_validate_of(struct dsa_port *dp,
 	*missing_phy_mode = false;
 	*missing_link_description = false;
 
+	if (dp->ds->ops->port_get_fwnode)
+		return;
+
 	if (of_get_phy_mode(dn, &mode)) {
 		*missing_phy_mode = true;
 		dev_err(ds->dev,
-- 
2.30.2

[PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Russell King (Oracle)]

Provide a software node for default settings when no phy, sfp or
fixed link is specified.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 109 +++++++++++++++++++++++++++++++
 1 file changed, 109 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index b73d1d6747b7..dde0c306fb3a 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -26,6 +26,7 @@
 #include <linux/of_device.h>
 #include <linux/of_irq.h>
 #include <linux/of_mdio.h>
+#include <linux/of_net.h>
 #include <linux/platform_data/mv88e6xxx.h>
 #include <linux/netdevice.h>
 #include <linux/gpio/consumer.h>
@@ -841,6 +842,113 @@ static void mv88e6xxx_get_caps(struct dsa_switch *ds, int port,
 	}
 }
 
+static struct fwnode_handle *mv88e6xxx_create_fixed_swnode(struct fwnode_handle *parent,
+							   int speed,
+							   int duplex)
+{
+	struct property_entry fixed_link_props[3] = { };
+
+	fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed);
+	if (duplex == DUPLEX_FULL)
+		fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex");
+
+	return fwnode_create_named_software_node(fixed_link_props, parent,
+						 "fixed-link");
+}
+
+static struct fwnode_handle *mv88e6xxx_create_port_swnode(phy_interface_t mode,
+							  int speed,
+							  int duplex)
+{
+	struct property_entry port_props[2] = {};
+	struct fwnode_handle *fixed_link_fwnode;
+	struct fwnode_handle *new_port_fwnode;
+
+	port_props[0] = PROPERTY_ENTRY_STRING("phy-mode", phy_modes(mode));
+	new_port_fwnode = fwnode_create_software_node(port_props, NULL);
+	if (IS_ERR(new_port_fwnode))
+		return new_port_fwnode;
+
+	fixed_link_fwnode = mv88e6xxx_create_fixed_swnode(new_port_fwnode,
+							  speed, duplex);
+	if (IS_ERR(fixed_link_fwnode)) {
+		fwnode_remove_software_node(new_port_fwnode);
+		return fixed_link_fwnode;
+	}
+
+	return new_port_fwnode;
+}
+
+static unsigned long mv88e6xxx_get_max_caps(struct phylink_config *config,
+					    phy_interface_t *mode)
+{
+	unsigned long max_caps, caps, mac_caps;
+	phy_interface_t interface;
+
+	mac_caps = config->mac_capabilities;
+	if (*mode != PHY_INTERFACE_MODE_NA)
+		return phylink_get_capabilities(*mode, mac_caps,
+						RATE_MATCH_NONE);
+
+	max_caps = 0;
+	for_each_set_bit(interface, config->supported_interfaces,
+			 PHY_INTERFACE_MODE_MAX) {
+		caps = phylink_get_capabilities(interface, mac_caps,
+						RATE_MATCH_NONE);
+		if (caps > max_caps) {
+			max_caps = caps;
+			*mode = interface;
+		}
+	}
+
+	return max_caps;
+}
+
+static struct fwnode_handle *mv88e6xxx_port_get_fwnode(struct dsa_switch *ds,
+						       int port,
+						       struct fwnode_handle *h)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	struct device_node *np, *phy_node;
+	int speed, duplex, err;
+	phy_interface_t mode;
+	struct dsa_port *dp;
+	unsigned long caps;
+
+	dp = dsa_to_port(ds, port);
+	if (dsa_port_is_user(dp))
+		return h;
+
+	/* No DT? Eh? */
+	np = to_of_node(h);
+	if (!np)
+		return h;
+
+	/* If a PHY, fixed-link specification, or in-band mode, then we
+	 * don't need to do any fixup. Simply return the provided fwnode.
+	 */
+	phy_node = of_parse_phandle(np, "phy-handle", 0);
+	of_node_put(phy_node);
+	if (phy_node || of_phy_is_fixed_link(np))
+		return h;
+
+	/* Get the DT specified phy-mode. If missing, try the chip's max speed
+	 * mode method if implemented, otherwise try the supported_interfaces
+	 * mask for the interface mode that gives the fastest speeds.
+	 */
+	err = of_get_phy_mode(np, &mode);
+	if (err && chip->info->ops->port_max_speed_mode)
+		mode = chip->info->ops->port_max_speed_mode(port);
+
+	caps = mv88e6xxx_get_max_caps(&dp->pl_config, &mode);
+
+	err = phylink_find_max_speed(caps, &speed, &duplex);
+	if (err)
+		return ERR_PTR(err);
+
+	return mv88e6xxx_create_port_swnode(mode, speed, duplex);
+}
+
 static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port,
 				 unsigned int mode,
 				 const struct phylink_link_state *state)
@@ -6994,6 +7102,7 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.teardown		= mv88e6xxx_teardown,
 	.port_setup		= mv88e6xxx_port_setup,
 	.port_teardown		= mv88e6xxx_port_teardown,
+	.port_get_fwnode	= mv88e6xxx_port_get_fwnode,
 	.phylink_get_caps	= mv88e6xxx_get_caps,
 	.phylink_mac_link_state	= mv88e6xxx_serdes_pcs_get_state,
 	.phylink_mac_config	= mv88e6xxx_mac_config,
-- 
2.30.2

[PATCH RFC net-next 7/7] net: dsa: mv88e6xxx: remove handling for DSA and CPU ports

[Russell King (Oracle)]

As we now always use a fixed-link for DSA and CPU ports, we no longer
need the hack in the Marvell code to make this work. Remove it.

This is especially important with the conversion of DSA drivers to
phylink_pcs, as the PCS code only gets called if we are using
phylink for the port.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 48 ++++----------------------------
 1 file changed, 5 insertions(+), 43 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index dde0c306fb3a..35dcfe86bdc4 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3395,56 +3395,17 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 {
 	struct device_node *phy_handle = NULL;
 	struct dsa_switch *ds = chip->ds;
-	phy_interface_t mode;
 	struct dsa_port *dp;
-	int tx_amp, speed;
+	int tx_amp;
 	int err;
 	u16 reg;
 
 	chip->ports[port].chip = chip;
 	chip->ports[port].port = port;
 
-	dp = dsa_to_port(ds, port);
-
-	/* MAC Forcing register: don't force link, speed, duplex or flow control
-	 * state to any particular values on physical ports, but force the CPU
-	 * port and all DSA ports to their maximum bandwidth and full duplex.
-	 */
-	if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)) {
-		struct phylink_config pl_config = {};
-		unsigned long caps;
-
-		chip->info->ops->phylink_get_caps(chip, port, &pl_config);
-
-		caps = pl_config.mac_capabilities;
-
-		if (chip->info->ops->port_max_speed_mode)
-			mode = chip->info->ops->port_max_speed_mode(port);
-		else
-			mode = PHY_INTERFACE_MODE_NA;
-
-		if (caps & MAC_10000FD)
-			speed = SPEED_10000;
-		else if (caps & MAC_5000FD)
-			speed = SPEED_5000;
-		else if (caps & MAC_2500FD)
-			speed = SPEED_2500;
-		else if (caps & MAC_1000)
-			speed = SPEED_1000;
-		else if (caps & MAC_100)
-			speed = SPEED_100;
-		else
-			speed = SPEED_10;
-
-		err = mv88e6xxx_port_setup_mac(chip, port, LINK_FORCED_UP,
-					       speed, DUPLEX_FULL,
-					       PAUSE_OFF, mode);
-	} else {
-		err = mv88e6xxx_port_setup_mac(chip, port, LINK_UNFORCED,
-					       SPEED_UNFORCED, DUPLEX_UNFORCED,
-					       PAUSE_ON,
-					       PHY_INTERFACE_MODE_NA);
-	}
+	err = mv88e6xxx_port_setup_mac(chip, port, LINK_UNFORCED,
+				       SPEED_UNFORCED, DUPLEX_UNFORCED,
+				       PAUSE_ON, PHY_INTERFACE_MODE_NA);
 	if (err)
 		return err;
 
@@ -3616,6 +3577,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 	}
 
 	if (chip->info->ops->serdes_set_tx_amplitude) {
+		dp = dsa_to_port(ds, port);
 		if (dp)
 			phy_handle = of_parse_phandle(dp->dn, "phy-handle", 0);
 
-- 
2.30.2

Re: [PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

[Andrew Lunn]

On Wed, Mar 22, 2023 at 12:00:06PM +0000, Russell King (Oracle) wrote:
> In preparation for supporting the use of software nodes to setup
> phylink, switch DSA to use fwnode_get_phy_mode() to retrieve the
> phy interface mode, rather than using of_get_phy_mode() which is
> DT specific.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH RFC net-next 2/7] net: phylink: provide phylink_find_max_speed()

[Andrew Lunn]

On Wed, Mar 22, 2023 at 12:00:01PM +0000, Russell King (Oracle) wrote:
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH RFC net-next 5/7] net: dsa: avoid DT validation for drivers which provide default config

[Andrew Lunn]

On Wed, Mar 22, 2023 at 12:00:16PM +0000, Russell King (Oracle) wrote:
> When a DSA driver (e.g. mv88e6xxx) provides a default configuration,
> avoid validating the DT description as missing elements will be
> provided by the DSA driver.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  net/dsa/port.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index c30e3a7d2145..23d9970c02d3 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -1951,6 +1951,9 @@ static void dsa_shared_port_validate_of(struct dsa_port *dp,
>  	*missing_phy_mode = false;
>  	*missing_link_description = false;
>  
> +	if (dp->ds->ops->port_get_fwnode)
> +		return;

I wounder if you should actually call it for the given port, and
ensure it does not return -EOPNOTSUPP, or -EINVAL, etc, because it is
not going to override that port? Then the DT values should be
validated?

	Andrew

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Andrew Lunn]

> +static struct fwnode_handle *mv88e6xxx_port_get_fwnode(struct dsa_switch *ds,
> +						       int port,
> +						       struct fwnode_handle *h)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	struct device_node *np, *phy_node;
> +	int speed, duplex, err;
> +	phy_interface_t mode;
> +	struct dsa_port *dp;
> +	unsigned long caps;
> +
> +	dp = dsa_to_port(ds, port);
> +	if (dsa_port_is_user(dp))
> +		return h;
> +
> +	/* No DT? Eh? */
> +	np = to_of_node(h);
> +	if (!np)
> +		return h;

I've not looked at the big picture yet, but you can have a simple
switch setup without DT. I have a couple of amd64 boards which use
platform data. The user ports all have internal PHYs, and the CPU port
defaults to 1G, it might even be strapped that way.

	 Andrew

Re: [PATCH RFC net-next 5/7] net: dsa: avoid DT validation for drivers which provide default config

[Russell King (Oracle)]

On Wed, Mar 22, 2023 at 07:51:22PM +0100, Andrew Lunn wrote:
> On Wed, Mar 22, 2023 at 12:00:16PM +0000, Russell King (Oracle) wrote:
> > When a DSA driver (e.g. mv88e6xxx) provides a default configuration,
> > avoid validating the DT description as missing elements will be
> > provided by the DSA driver.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  net/dsa/port.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/net/dsa/port.c b/net/dsa/port.c
> > index c30e3a7d2145..23d9970c02d3 100644
> > --- a/net/dsa/port.c
> > +++ b/net/dsa/port.c
> > @@ -1951,6 +1951,9 @@ static void dsa_shared_port_validate_of(struct dsa_port *dp,
> >  	*missing_phy_mode = false;
> >  	*missing_link_description = false;
> >  
> > +	if (dp->ds->ops->port_get_fwnode)
> > +		return;
> 
> I wounder if you should actually call it for the given port, and
> ensure it does not return -EOPNOTSUPP, or -EINVAL, etc, because it is
> not going to override that port? Then the DT values should be
> validated?

Won't that mean that we need to implement the method for all DSA
drivers?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Russell King (Oracle)]

On Wed, Mar 22, 2023 at 07:57:19PM +0100, Andrew Lunn wrote:
> > +static struct fwnode_handle *mv88e6xxx_port_get_fwnode(struct dsa_switch *ds,
> > +						       int port,
> > +						       struct fwnode_handle *h)
> > +{
> > +	struct mv88e6xxx_chip *chip = ds->priv;
> > +	struct device_node *np, *phy_node;
> > +	int speed, duplex, err;
> > +	phy_interface_t mode;
> > +	struct dsa_port *dp;
> > +	unsigned long caps;
> > +
> > +	dp = dsa_to_port(ds, port);
> > +	if (dsa_port_is_user(dp))
> > +		return h;
> > +
> > +	/* No DT? Eh? */
> > +	np = to_of_node(h);
> > +	if (!np)
> > +		return h;
> 
> I've not looked at the big picture yet, but you can have a simple
> switch setup without DT. I have a couple of amd64 boards which use
> platform data. The user ports all have internal PHYs, and the CPU port
> defaults to 1G, it might even be strapped that way.

Are you suggesting that we should generate some swnode description of
the max interface mode and speed if we are missing a DT node?

I'm not seeing any port specific data in the mv88e6xxx platform data.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 5/7] net: dsa: avoid DT validation for drivers which provide default config

[Andrew Lunn]

On Wed, Mar 22, 2023 at 08:09:12PM +0000, Russell King (Oracle) wrote:
> On Wed, Mar 22, 2023 at 07:51:22PM +0100, Andrew Lunn wrote:
> > On Wed, Mar 22, 2023 at 12:00:16PM +0000, Russell King (Oracle) wrote:
> > > When a DSA driver (e.g. mv88e6xxx) provides a default configuration,
> > > avoid validating the DT description as missing elements will be
> > > provided by the DSA driver.
> > > 
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > ---
> > >  net/dsa/port.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/net/dsa/port.c b/net/dsa/port.c
> > > index c30e3a7d2145..23d9970c02d3 100644
> > > --- a/net/dsa/port.c
> > > +++ b/net/dsa/port.c
> > > @@ -1951,6 +1951,9 @@ static void dsa_shared_port_validate_of(struct dsa_port *dp,
> > >  	*missing_phy_mode = false;
> > >  	*missing_link_description = false;
> > >  
> > > +	if (dp->ds->ops->port_get_fwnode)
> > > +		return;
> > 
> > I wounder if you should actually call it for the given port, and
> > ensure it does not return -EOPNOTSUPP, or -EINVAL, etc, because it is
> > not going to override that port? Then the DT values should be
> > validated?
> 
> Won't that mean that we need to implement the method for all DSA
> drivers?

I mean call it if it exists. We should be only providing overrides for
CPU and DSA ports, i think. So i expect it returns an error for user
ports? And then we want to continue with the validation with what
actually is in DT for user ports.

	 Andrew

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Andrew Lunn]

On Wed, Mar 22, 2023 at 08:13:51PM +0000, Russell King (Oracle) wrote:
> On Wed, Mar 22, 2023 at 07:57:19PM +0100, Andrew Lunn wrote:
> > > +static struct fwnode_handle *mv88e6xxx_port_get_fwnode(struct dsa_switch *ds,
> > > +						       int port,
> > > +						       struct fwnode_handle *h)
> > > +{
> > > +	struct mv88e6xxx_chip *chip = ds->priv;
> > > +	struct device_node *np, *phy_node;
> > > +	int speed, duplex, err;
> > > +	phy_interface_t mode;
> > > +	struct dsa_port *dp;
> > > +	unsigned long caps;
> > > +
> > > +	dp = dsa_to_port(ds, port);
> > > +	if (dsa_port_is_user(dp))
> > > +		return h;
> > > +
> > > +	/* No DT? Eh? */
> > > +	np = to_of_node(h);
> > > +	if (!np)
> > > +		return h;
> > 
> > I've not looked at the big picture yet, but you can have a simple
> > switch setup without DT. I have a couple of amd64 boards which use
> > platform data. The user ports all have internal PHYs, and the CPU port
> > defaults to 1G, it might even be strapped that way.
> 
> Are you suggesting that we should generate some swnode description of
> the max interface mode and speed if we are missing a DT node?
> 
> I'm not seeing any port specific data in the mv88e6xxx platform data.

No, i'm just pointing out that not have DT is not an error, and can
happen. I just wanted to make sure you are not assuming there is
always DT.

	Andrew

Re: [PATCH RFC net-next 5/7] net: dsa: avoid DT validation for drivers which provide default config

[Russell King (Oracle)]

On Wed, Mar 22, 2023 at 09:14:31PM +0100, Andrew Lunn wrote:
> On Wed, Mar 22, 2023 at 08:09:12PM +0000, Russell King (Oracle) wrote:
> > On Wed, Mar 22, 2023 at 07:51:22PM +0100, Andrew Lunn wrote:
> > > On Wed, Mar 22, 2023 at 12:00:16PM +0000, Russell King (Oracle) wrote:
> > > > When a DSA driver (e.g. mv88e6xxx) provides a default configuration,
> > > > avoid validating the DT description as missing elements will be
> > > > provided by the DSA driver.
> > > > 
> > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > > ---
> > > >  net/dsa/port.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/net/dsa/port.c b/net/dsa/port.c
> > > > index c30e3a7d2145..23d9970c02d3 100644
> > > > --- a/net/dsa/port.c
> > > > +++ b/net/dsa/port.c
> > > > @@ -1951,6 +1951,9 @@ static void dsa_shared_port_validate_of(struct dsa_port *dp,
> > > >  	*missing_phy_mode = false;
> > > >  	*missing_link_description = false;
> > > >  
> > > > +	if (dp->ds->ops->port_get_fwnode)
> > > > +		return;
> > > 
> > > I wounder if you should actually call it for the given port, and
> > > ensure it does not return -EOPNOTSUPP, or -EINVAL, etc, because it is
> > > not going to override that port? Then the DT values should be
> > > validated?
> > 
> > Won't that mean that we need to implement the method for all DSA
> > drivers?
> 
> I mean call it if it exists. We should be only providing overrides for
> CPU and DSA ports, i think. So i expect it returns an error for user
> ports? And then we want to continue with the validation with what
> actually is in DT for user ports.

I suppose we could do - but before adding that complexity, I think we
should have a real use case for it.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Russell King (Oracle)]

On Wed, Mar 22, 2023 at 09:17:02PM +0100, Andrew Lunn wrote:
> On Wed, Mar 22, 2023 at 08:13:51PM +0000, Russell King (Oracle) wrote:
> > On Wed, Mar 22, 2023 at 07:57:19PM +0100, Andrew Lunn wrote:
> > > > +static struct fwnode_handle *mv88e6xxx_port_get_fwnode(struct dsa_switch *ds,
> > > > +						       int port,
> > > > +						       struct fwnode_handle *h)
> > > > +{
> > > > +	struct mv88e6xxx_chip *chip = ds->priv;
> > > > +	struct device_node *np, *phy_node;
> > > > +	int speed, duplex, err;
> > > > +	phy_interface_t mode;
> > > > +	struct dsa_port *dp;
> > > > +	unsigned long caps;
> > > > +
> > > > +	dp = dsa_to_port(ds, port);
> > > > +	if (dsa_port_is_user(dp))
> > > > +		return h;
> > > > +
> > > > +	/* No DT? Eh? */
> > > > +	np = to_of_node(h);
> > > > +	if (!np)
> > > > +		return h;
> > > 
> > > I've not looked at the big picture yet, but you can have a simple
> > > switch setup without DT. I have a couple of amd64 boards which use
> > > platform data. The user ports all have internal PHYs, and the CPU port
> > > defaults to 1G, it might even be strapped that way.
> > 
> > Are you suggesting that we should generate some swnode description of
> > the max interface mode and speed if we are missing a DT node?
> > 
> > I'm not seeing any port specific data in the mv88e6xxx platform data.
> 
> No, i'm just pointing out that not have DT is not an error, and can
> happen. I just wanted to make sure you are not assuming there is
> always DT.

What I'm trying to find out is what you think the behaviour should be
in this case. Are you suggesting we should fall back to what we do now
which is let the driver do it internally without phylink.

The problem is that if we don't go down the phylink route for everything
then we /can't/ convert mv88e6xxx to phylink_pcs, because the "serdes"
stuff will be gone, and the absence of phylink will mean those won't be
called e.g. to power up the serdes.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Andrew Lunn]

> What I'm trying to find out is what you think the behaviour should be
> in this case. Are you suggesting we should fall back to what we do now
> which is let the driver do it internally without phylink.
> 
> The problem is that if we don't go down the phylink route for everything
> then we /can't/ convert mv88e6xxx to phylink_pcs, because the "serdes"
> stuff will be gone, and the absence of phylink will mean those won't be
> called e.g. to power up the serdes.

I'm pretty sure non-DT systems have never used SERDES. They are using
a back to back PHY, or maybe RGMII. So long as this keeps working, we
can convert to phylink.

And i have such a amd64 system, using back to back PHYs so i can test
it does not regress.

    Andrew

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Russell King (Oracle)]

On Wed, Mar 22, 2023 at 10:40:26PM +0100, Andrew Lunn wrote:
> > What I'm trying to find out is what you think the behaviour should be
> > in this case. Are you suggesting we should fall back to what we do now
> > which is let the driver do it internally without phylink.
> > 
> > The problem is that if we don't go down the phylink route for everything
> > then we /can't/ convert mv88e6xxx to phylink_pcs, because the "serdes"
> > stuff will be gone, and the absence of phylink will mean those won't be
> > called e.g. to power up the serdes.
> 
> I'm pretty sure non-DT systems have never used SERDES. They are using
> a back to back PHY, or maybe RGMII. So long as this keeps working, we
> can convert to phylink.
> 
> And i have such a amd64 system, using back to back PHYs so i can test
> it does not regress.

Reading the code, I don't think we have any issue with the DSA and CPU
ports, as these check whether dp->dn is not NULL before calling
dsa_shared_port_link_register_of() and the validator. This means these
paths will only be used for setups that have DT.

For the user ports, we can end up calling dsa_port_phylink_create()
with a NULL dp->dn, and must not fail.

So, given that this is only supposed to be used for mv88e6xxx because
of it's legacy, maybe the check in dsa_port_phylink_create() should
be:

        fwnode = of_fwnode_handle(dp->dn);
        if (fwnode && ds->ops->port_get_fwnode) {

In other words, we only allow the replacement of the firmware
description if one already existed.

Alternatively, we could use:

	if (!dsa_port_is_user(dp) && ds->ops->port_get_fwnode) {

since mv88e6xxx today only does this "max speed" thing for CPU and
DSA ports, and thus we only need to replace the firmware description
for these ports - and we can document that port_get_fwnode is only
for CPU and DSA ports.

Hmm?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 1/7] software node: allow named software node to be created

[Andy Shevchenko]

On Wed, Mar 22, 2023 at 11:59:55AM +0000, Russell King wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Allow a named software node to be created, which is needed for software
> nodes for a fixed-link specification for DSA.

...

> +fwnode_create_named_software_node(const struct property_entry *properties,
> +				  const struct fwnode_handle *parent,
> +				  const char *name)
>  {
>  	struct fwnode_handle *fwnode;
>  	struct software_node *node;
> @@ -930,6 +931,7 @@ fwnode_create_software_node(const struct property_entry *properties,
>  		return ERR_CAST(node);
>  
>  	node->parent = p ? p->node : NULL;
> +	node->name = name;

The same question stays as before: how can we be sure that the name is unique
and we won't have a collision?

>  	fwnode = swnode_register(node, p, 1);
>  	if (IS_ERR(fwnode))
> @@ -937,6 +939,14 @@ fwnode_create_software_node(const struct property_entry *properties,
>  
>  	return fwnode;
>  }
> +EXPORT_SYMBOL_GPL(fwnode_create_named_software_node);


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

[Andy Shevchenko]

On Wed, Mar 22, 2023 at 12:00:06PM +0000, Russell King (Oracle) wrote:
> In preparation for supporting the use of software nodes to setup
> phylink, switch DSA to use fwnode_get_phy_mode() to retrieve the
> phy interface mode, rather than using of_get_phy_mode() which is
> DT specific.

...

> +	struct fwnode_handle *fwnode;

> +	fwnode = of_fwnode_handle(dp->dn);

	const struct fwnode_handle *fwnode = of_fwnode_handle(dp->dn);

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC net-next 1/7] software node: allow named software node to be created

[Russell King (Oracle)]

On Thu, Mar 23, 2023 at 03:59:07PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 22, 2023 at 11:59:55AM +0000, Russell King wrote:
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > 
> > Allow a named software node to be created, which is needed for software
> > nodes for a fixed-link specification for DSA.
> 
> ...
> 
> > +fwnode_create_named_software_node(const struct property_entry *properties,
> > +				  const struct fwnode_handle *parent,
> > +				  const char *name)
> >  {
> >  	struct fwnode_handle *fwnode;
> >  	struct software_node *node;
> > @@ -930,6 +931,7 @@ fwnode_create_software_node(const struct property_entry *properties,
> >  		return ERR_CAST(node);
> >  
> >  	node->parent = p ? p->node : NULL;
> > +	node->name = name;
> 
> The same question stays as before: how can we be sure that the name is unique
> and we won't have a collision?

This got discussed at length last time around, starting here:

https://lore.kernel.org/all/YtHGwz4v7VWKhIXG@smile.fi.intel.com/

My conclusion is that your concern is invalid, because we're creating
this tree:

	node%d
	+- phy-mode property
	`- fixed-link node
	   +- speed property
	   `- full-duplex (optional) property

Given that node%d will be allocated against the swnode_root_ids IDA,
then how can there possibly be a naming collision.

You would be correct if the "fixed-link" node were to be created at
root level, or if we were intentionally creating two swnodes under
the same parent with the same name, but we aren't.

Plus, the code _already_ allows for e.g. multiple "node1" names - for
example, one in root and one as a child node, since the code uses
separate IDAs to allocate those.

Hence, I do not recognise the conern you are raising, and I believe
your concern is not valid.

Your concern would be valid if it was a general concern about
fwnode_create_named_software_node() being used to create the same
named node under the same parent, but that IMHO is a programming
bug, no different from trying to create two devices under the same
parent with the same name.

So, unless you can be more expansive about _precisely_ what your
concern is, then I don't think there exists any problem with this.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

[Russell King (Oracle)]

On Thu, Mar 23, 2023 at 04:03:05PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 22, 2023 at 12:00:06PM +0000, Russell King (Oracle) wrote:
> > In preparation for supporting the use of software nodes to setup
> > phylink, switch DSA to use fwnode_get_phy_mode() to retrieve the
> > phy interface mode, rather than using of_get_phy_mode() which is
> > DT specific.
> 
> ...
> 
> > +	struct fwnode_handle *fwnode;
> 
> > +	fwnode = of_fwnode_handle(dp->dn);
> 
> 	const struct fwnode_handle *fwnode = of_fwnode_handle(dp->dn);
> 
> ?

Why const?

Why do you want it on one line? The code as written conforms to
netdev coding standards which as you well know are different from
the rest of the kernel.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

[Andy Shevchenko]

On Thu, Mar 23, 2023 at 02:31:04PM +0000, Russell King (Oracle) wrote:
> On Thu, Mar 23, 2023 at 04:03:05PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 22, 2023 at 12:00:06PM +0000, Russell King (Oracle) wrote:

...

> > > +	struct fwnode_handle *fwnode;
> > 
> > > +	fwnode = of_fwnode_handle(dp->dn);
> > 
> > 	const struct fwnode_handle *fwnode = of_fwnode_handle(dp->dn);
> > 
> > ?
> 
> Why const?

Do you modify its content on the fly?

For fwnode as a basic object type we want to reduce the scope of the possible
modifications. If you don't modify and APIs you call do not require non-const
object, use const for fwnode.

...

> Why do you want it on one line? The code as written conforms to
> netdev coding standards which as you well know are different from
> the rest of the kernel.

This is up to you. I don't care.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC net-next 1/7] software node: allow named software node to be created

[Andy Shevchenko]

On Thu, Mar 23, 2023 at 02:29:24PM +0000, Russell King (Oracle) wrote:
> On Thu, Mar 23, 2023 at 03:59:07PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 22, 2023 at 11:59:55AM +0000, Russell King wrote:
> > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > 
> > > Allow a named software node to be created, which is needed for software
> > > nodes for a fixed-link specification for DSA.

...

> > > +fwnode_create_named_software_node(const struct property_entry *properties,
> > > +				  const struct fwnode_handle *parent,
> > > +				  const char *name)
> > >  {
> > >  	struct fwnode_handle *fwnode;
> > >  	struct software_node *node;
> > > @@ -930,6 +931,7 @@ fwnode_create_software_node(const struct property_entry *properties,
> > >  		return ERR_CAST(node);
> > >  
> > >  	node->parent = p ? p->node : NULL;
> > > +	node->name = name;
> > 
> > The same question stays as before: how can we be sure that the name is unique
> > and we won't have a collision?
> 
> This got discussed at length last time around, starting here:
> 
> https://lore.kernel.org/all/YtHGwz4v7VWKhIXG@smile.fi.intel.com/
> 
> My conclusion is that your concern is invalid, because we're creating
> this tree:
> 
> 	node%d
> 	+- phy-mode property
> 	`- fixed-link node
> 	   +- speed property
> 	   `- full-duplex (optional) property
> 
> Given that node%d will be allocated against the swnode_root_ids IDA,
> then how can there possibly be a naming collision.
> 
> You would be correct if the "fixed-link" node were to be created at
> root level, or if we were intentionally creating two swnodes under
> the same parent with the same name, but we aren't.
> 
> Plus, the code _already_ allows for e.g. multiple "node1" names - for
> example, one in root and one as a child node, since the code uses
> separate IDAs to allocate those.
> 
> Hence, I do not recognise the conern you are raising, and I believe
> your concern is not valid.
> 
> Your concern would be valid if it was a general concern about
> fwnode_create_named_software_node() being used to create the same
> named node under the same parent, but that IMHO is a programming
> bug, no different from trying to create two devices under the same
> parent with the same name.
> 
> So, unless you can be more expansive about _precisely_ what your
> concern is, then I don't think there exists any problem with this.

OK.

I leave it to others to review. I have nothing to add.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

[Russell King (Oracle)]

On Thu, Mar 23, 2023 at 04:38:29PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 23, 2023 at 02:31:04PM +0000, Russell King (Oracle) wrote:
> > On Thu, Mar 23, 2023 at 04:03:05PM +0200, Andy Shevchenko wrote:
> > > On Wed, Mar 22, 2023 at 12:00:06PM +0000, Russell King (Oracle) wrote:
> 
> ...
> 
> > > > +	struct fwnode_handle *fwnode;
> > > 
> > > > +	fwnode = of_fwnode_handle(dp->dn);
> > > 
> > > 	const struct fwnode_handle *fwnode = of_fwnode_handle(dp->dn);
> > > 
> > > ?
> > 
> > Why const?
> 
> Do you modify its content on the fly?

Do you want to litter code with casts to get rid of the const?

> For fwnode as a basic object type we want to reduce the scope of the possible
> modifications. If you don't modify and APIs you call do not require non-const
> object, use const for fwnode.

Let's start here. We pass this fwnode to fwnode_get_phy_mode():

include/linux/property.h:int fwnode_get_phy_mode(struct fwnode_handle *fwnode);

Does fwnode_get_phy_mode() alter the contents of the fwnode? Probably
not, but it doesn't take a const pointer. Therefore, to declare my
fwnode as const, I'd need to cast the const-ness away before calling
this.

Then there's phylink_create(). Same problem.

So NAK to this const - until such time that we have a concerted effort
to making functions we call which do not modify the "fwnode" argument
constify that argument. Otherwise it's just rediculously crazy to
declare a variable const only to then litter the code with casts to get
rid of it at every call site.

Please do a bit of research before making suggestions. Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

[Andy Shevchenko]

On Thu, Mar 23, 2023 at 02:49:01PM +0000, Russell King (Oracle) wrote:
> On Thu, Mar 23, 2023 at 04:38:29PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 23, 2023 at 02:31:04PM +0000, Russell King (Oracle) wrote:
> > > On Thu, Mar 23, 2023 at 04:03:05PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Mar 22, 2023 at 12:00:06PM +0000, Russell King (Oracle) wrote:

...

> > > > > +	struct fwnode_handle *fwnode;
> > > > 
> > > > > +	fwnode = of_fwnode_handle(dp->dn);
> > > > 
> > > > 	const struct fwnode_handle *fwnode = of_fwnode_handle(dp->dn);
> > > > 
> > > > ?
> > > 
> > > Why const?
> > 
> > Do you modify its content on the fly?
> 
> Do you want to litter code with casts to get rid of the const?
> 
> > For fwnode as a basic object type we want to reduce the scope of the possible
> > modifications. If you don't modify and APIs you call do not require non-const
> > object, use const for fwnode.
> 
> Let's start here. We pass this fwnode to fwnode_get_phy_mode():
> 
> include/linux/property.h:int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
> 
> Does fwnode_get_phy_mode() alter the contents of the fwnode? Probably
> not, but it doesn't take a const pointer. Therefore, to declare my
> fwnode as const, I'd need to cast the const-ness away before calling
> this.

So, fix the fwnode_get_phy_mode(). Is it a problem?

> Then there's phylink_create(). Same problem.

So, fix that. Is it a problem?

> So NAK to this const - until such time that we have a concerted effort
> to making functions we call which do not modify the "fwnode" argument
> constify that argument. Otherwise it's just rediculously crazy to
> declare a variable const only to then litter the code with casts to get
> rid of it at every call site.
> 
> Please do a bit of research before making suggestions. Thanks.

So, MAK to your patch. You can fix that, and you know that.

P.S. Please, move that phy thingy away from property.h, it doesn't belong
there.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

[Russell King (Oracle)]

On Thu, Mar 23, 2023 at 05:00:08PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 23, 2023 at 02:49:01PM +0000, Russell King (Oracle) wrote:
> > On Thu, Mar 23, 2023 at 04:38:29PM +0200, Andy Shevchenko wrote:
> > > On Thu, Mar 23, 2023 at 02:31:04PM +0000, Russell King (Oracle) wrote:
> > > > On Thu, Mar 23, 2023 at 04:03:05PM +0200, Andy Shevchenko wrote:
> > > > > On Wed, Mar 22, 2023 at 12:00:06PM +0000, Russell King (Oracle) wrote:
> 
> ...
> 
> > > > > > +	struct fwnode_handle *fwnode;
> > > > > 
> > > > > > +	fwnode = of_fwnode_handle(dp->dn);
> > > > > 
> > > > > 	const struct fwnode_handle *fwnode = of_fwnode_handle(dp->dn);
> > > > > 
> > > > > ?
> > > > 
> > > > Why const?
> > > 
> > > Do you modify its content on the fly?
> > 
> > Do you want to litter code with casts to get rid of the const?
> > 
> > > For fwnode as a basic object type we want to reduce the scope of the possible
> > > modifications. If you don't modify and APIs you call do not require non-const
> > > object, use const for fwnode.
> > 
> > Let's start here. We pass this fwnode to fwnode_get_phy_mode():
> > 
> > include/linux/property.h:int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
> > 
> > Does fwnode_get_phy_mode() alter the contents of the fwnode? Probably
> > not, but it doesn't take a const pointer. Therefore, to declare my
> > fwnode as const, I'd need to cast the const-ness away before calling
> > this.
> 
> So, fix the fwnode_get_phy_mode(). Is it a problem?

No, I refuse. That's for a different patch set.

> > Then there's phylink_create(). Same problem.
> 
> So, fix that. Is it a problem?

No for the same reason.

> > So NAK to this const - until such time that we have a concerted effort
> > to making functions we call which do not modify the "fwnode" argument
> > constify that argument. Otherwise it's just rediculously crazy to
> > declare a variable const only to then litter the code with casts to get
> > rid of it at every call site.
> > 
> > Please do a bit of research before making suggestions. Thanks.
> 
> So, MAK to your patch. You can fix that, and you know that.

Sorry, I don't accept your NAK. While you have a valid point about
these things being const, that is not the fault of this patch series,
and is something that should be addressed separately.

The lack of const-ness that has been there for quite some time is no
reason to NAK a patch that has nothing to do with this.

> P.S. Please, move that phy thingy away from property.h, it doesn't belong
> there.

Again, that's a subject for a separate patch.

I will re-post this in due course and ignore your NAK (due to your
lack of research, and confrontational nature.)

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

[Andy Shevchenko]

On Thu, Mar 23, 2023 at 03:23:12PM +0000, Russell King (Oracle) wrote:
> On Thu, Mar 23, 2023 at 05:00:08PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 23, 2023 at 02:49:01PM +0000, Russell King (Oracle) wrote:
> > > On Thu, Mar 23, 2023 at 04:38:29PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Mar 23, 2023 at 02:31:04PM +0000, Russell King (Oracle) wrote:
> > > > > On Thu, Mar 23, 2023 at 04:03:05PM +0200, Andy Shevchenko wrote:
> > > > > > On Wed, Mar 22, 2023 at 12:00:06PM +0000, Russell King (Oracle) wrote:

...

> > > > > > > +	struct fwnode_handle *fwnode;
> > > > > > 
> > > > > > > +	fwnode = of_fwnode_handle(dp->dn);
> > > > > > 
> > > > > > 	const struct fwnode_handle *fwnode = of_fwnode_handle(dp->dn);
> > > > > > 
> > > > > > ?
> > > > > 
> > > > > Why const?
> > > > 
> > > > Do you modify its content on the fly?
> > > 
> > > Do you want to litter code with casts to get rid of the const?
> > > 
> > > > For fwnode as a basic object type we want to reduce the scope of the possible
> > > > modifications. If you don't modify and APIs you call do not require non-const
> > > > object, use const for fwnode.
> > > 
> > > Let's start here. We pass this fwnode to fwnode_get_phy_mode():
> > > 
> > > include/linux/property.h:int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
> > > 
> > > Does fwnode_get_phy_mode() alter the contents of the fwnode? Probably
> > > not, but it doesn't take a const pointer. Therefore, to declare my
> > > fwnode as const, I'd need to cast the const-ness away before calling
> > > this.
> > 
> > So, fix the fwnode_get_phy_mode(). Is it a problem?
> 
> No, I refuse. That's for a different patch set.

I don't disagree, but it can be done as a precursor to your RFC.

> > > Then there's phylink_create(). Same problem.
> > 
> > So, fix that. Is it a problem?
> 
> No for the same reason.
> 
> > > So NAK to this const - until such time that we have a concerted effort
> > > to making functions we call which do not modify the "fwnode" argument
> > > constify that argument. Otherwise it's just rediculously crazy to
> > > declare a variable const only to then litter the code with casts to get
> > > rid of it at every call site.
> > > 
> > > Please do a bit of research before making suggestions. Thanks.
> > 
> > So, MAK to your patch. You can fix that, and you know that.
> 
> Sorry, I don't accept your NAK. While you have a valid point about
> these things being const, that is not the fault of this patch series,
> and is something that should be addressed separately.

Yes, and since it's not a big deal it can be done as a precursor work.

> The lack of const-ness that has been there for quite some time is no
> reason to NAK a patch that has nothing to do with this.

Instead of saying politely that you didn't agree of the necessity of the asked
changes, you shoowed your confrontational manner with a strong NAK. Let's not
escalate it further, it won't play well with a nervous system.

> > P.S. Please, move that phy thingy away from property.h, it doesn't belong
> > there.
> 
> Again, that's a subject for a separate patch.
> 
> I will re-post this in due course and ignore your NAK (due to your
> lack of research, and confrontational nature.)

Don't make a drama out of it. Many maintainers are asking for a small cleanups
before applying a feature.

Nevertheless, since I'm neither a net nor a DSA maintainer, I have only thing
to push is to move the PHY APIs out from the property.h. The rest is up to you.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

[Russell King (Oracle)]

On Thu, Mar 23, 2023 at 03:23:12PM +0000, Russell King (Oracle) wrote:
> On Thu, Mar 23, 2023 at 05:00:08PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 23, 2023 at 02:49:01PM +0000, Russell King (Oracle) wrote:
> > > On Thu, Mar 23, 2023 at 04:38:29PM +0200, Andy Shevchenko wrote:
> > > > Do you modify its content on the fly?
> > > 
> > > Do you want to litter code with casts to get rid of the const?
> > > 
> > > > For fwnode as a basic object type we want to reduce the scope of the possible
> > > > modifications. If you don't modify and APIs you call do not require non-const
> > > > object, use const for fwnode.
> > > 
> > > Let's start here. We pass this fwnode to fwnode_get_phy_mode():
> > > 
> > > include/linux/property.h:int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
> > > 
> > > Does fwnode_get_phy_mode() alter the contents of the fwnode? Probably
> > > not, but it doesn't take a const pointer. Therefore, to declare my
> > > fwnode as const, I'd need to cast the const-ness away before calling
> > > this.
> > 
> > So, fix the fwnode_get_phy_mode(). Is it a problem?
> 
> No, I refuse. That's for a different patch set.
> 
> > > Then there's phylink_create(). Same problem.
> > 
> > So, fix that. Is it a problem?
> 
> No for the same reason.
> 
> > > So NAK to this const - until such time that we have a concerted effort
> > > to making functions we call which do not modify the "fwnode" argument
> > > constify that argument. Otherwise it's just rediculously crazy to
> > > declare a variable const only to then litter the code with casts to get
> > > rid of it at every call site.
> > > 
> > > Please do a bit of research before making suggestions. Thanks.
> > 
> > So, MAK to your patch. You can fix that, and you know that.
> 
> Sorry, I don't accept your NAK. While you have a valid point about
> these things being const, that is not the fault of this patch series,
> and is something that should be addressed separately.
> 
> The lack of const-ness that has been there for quite some time is no
> reason to NAK a patch that has nothing to do with this.

To illustrate how rediculous this is:

$ git grep 'struct fwnode_handle \*.*='

gives 134 instances. Of those, only five are const, which means 129
aren't. So I question - why are you singling mine out for what appears
to be special treatment.


Let's look at other parts of the fwnode API.

void __iomem *fwnode_iomap(struct fwnode_handle *fwnode, int index);

Does that modify the fwnode it was passed? It calls:

        void __iomem *(*iomap)(struct fwnode_handle *fwnode, int index);

in struct fwnode_operations, so that would need to be made const as well.
The only implementation of that which I can find is of_fwnode_iomap()
which uses to_of_node() on that, which casts away the const-ness. So
this would be a candidate to making const.


bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle *child);

I'd be surprised if that modifies either of those fwnodes. It seems
to use fwnode_for_each_parent_node() from the child, which passes
"child" to fwnode_get_parent(), which itself is const. Therefore, it
seems there's no reason not to make "child" const. "ancestor" can
also be made const since it's only being used for pointer-compares.


unsigned int fwnode_graph_get_endpoint_count(struct fwnode_handle *fwnode,
                                             unsigned long flags);

Similar story with this, although it uses
fwnode_graph_for_each_endpoint(), which seems to mean that "fwnode"
can also be const.


My point is that there are several things in the fwnode API that
should be made const but that aren't, but which should likely be
fixed before requiring const-ness of those fwnode_handle
declarations in people's code.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

[Russell King (Oracle)]

On Thu, Mar 23, 2023 at 05:33:17PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 23, 2023 at 03:23:12PM +0000, Russell King (Oracle) wrote:
> > On Thu, Mar 23, 2023 at 05:00:08PM +0200, Andy Shevchenko wrote:
> > > On Thu, Mar 23, 2023 at 02:49:01PM +0000, Russell King (Oracle) wrote:
> > > > On Thu, Mar 23, 2023 at 04:38:29PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Mar 23, 2023 at 02:31:04PM +0000, Russell King (Oracle) wrote:
> > > > > > On Thu, Mar 23, 2023 at 04:03:05PM +0200, Andy Shevchenko wrote:
> > > > > > > On Wed, Mar 22, 2023 at 12:00:06PM +0000, Russell King (Oracle) wrote:
> 
> ...
> 
> > > > > > > > +	struct fwnode_handle *fwnode;
> > > > > > > 
> > > > > > > > +	fwnode = of_fwnode_handle(dp->dn);
> > > > > > > 
> > > > > > > 	const struct fwnode_handle *fwnode = of_fwnode_handle(dp->dn);
> > > > > > > 
> > > > > > > ?
> > > > > > 
> > > > > > Why const?
> > > > > 
> > > > > Do you modify its content on the fly?
> > > > 
> > > > Do you want to litter code with casts to get rid of the const?
> > > > 
> > > > > For fwnode as a basic object type we want to reduce the scope of the possible
> > > > > modifications. If you don't modify and APIs you call do not require non-const
> > > > > object, use const for fwnode.
> > > > 
> > > > Let's start here. We pass this fwnode to fwnode_get_phy_mode():
> > > > 
> > > > include/linux/property.h:int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
> > > > 
> > > > Does fwnode_get_phy_mode() alter the contents of the fwnode? Probably
> > > > not, but it doesn't take a const pointer. Therefore, to declare my
> > > > fwnode as const, I'd need to cast the const-ness away before calling
> > > > this.
> > > 
> > > So, fix the fwnode_get_phy_mode(). Is it a problem?
> > 
> > No, I refuse. That's for a different patch set.
> 
> I don't disagree, but it can be done as a precursor to your RFC.

And you want that merged through net-next?

> > > > Then there's phylink_create(). Same problem.
> > > 
> > > So, fix that. Is it a problem?
> > 
> > No for the same reason.
> > 
> > > > So NAK to this const - until such time that we have a concerted effort
> > > > to making functions we call which do not modify the "fwnode" argument
> > > > constify that argument. Otherwise it's just rediculously crazy to
> > > > declare a variable const only to then litter the code with casts to get
> > > > rid of it at every call site.
> > > > 
> > > > Please do a bit of research before making suggestions. Thanks.
> > > 
> > > So, MAK to your patch. You can fix that, and you know that.
> > 
> > Sorry, I don't accept your NAK. While you have a valid point about
> > these things being const, that is not the fault of this patch series,
> > and is something that should be addressed separately.
> 
> Yes, and since it's not a big deal it can be done as a precursor work.
> 
> > The lack of const-ness that has been there for quite some time is no
> > reason to NAK a patch that has nothing to do with this.
> 
> Instead of saying politely that you didn't agree of the necessity of the asked
> changes, you shoowed your confrontational manner with a strong NAK. Let's not
> escalate it further, it won't play well with a nervous system.
> 
> > > P.S. Please, move that phy thingy away from property.h, it doesn't belong
> > > there.
> > 
> > Again, that's a subject for a separate patch.
> > 
> > I will re-post this in due course and ignore your NAK (due to your
> > lack of research, and confrontational nature.)
> 
> Don't make a drama out of it. Many maintainers are asking for a small cleanups
> before applying a feature.
> 
> Nevertheless, since I'm neither a net nor a DSA maintainer, I have only thing
> to push is to move the PHY APIs out from the property.h. The rest is up to you.

Really? In your previous message, you were NAKing the patch based on the
lack of "const"ness. So you've changed your tune to something that was
a request in a post-script (PS).

If you had done due diligence, you would have realised that its
implementation is in property.c, so presumably if you had known that
either (a) you wouldn't be making the request or (b) you would be
asking for that to be moved as well.

Now, where do you expect it to be moved to? There is nowhere convenient
in net/ nor drivers/net/ for it today. It's corresponding DT equivalent
is in net/core/of_net.c, but that is only built if CONFIG_OF is enabled
which is unsuitable for the fwnode version. I guess we could have a
net/core/fwnode.c just for this single function... then where do we put
the prototype? include/linux/fwnode_net.h (which would be a new header
just for this), and updating the five drivers for this change.

In any case, due to the way netdev works, this should *not* be part
of this patch series, because if there's a reason to revert this
series, we wouldn't want the move of fwnode_get_phy_mode() to also
get reverted - that would potentially cause chaos.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

[Andy Shevchenko]

On Thu, Mar 23, 2023 at 04:18:15PM +0000, Russell King (Oracle) wrote:
> On Thu, Mar 23, 2023 at 03:23:12PM +0000, Russell King (Oracle) wrote:
> > On Thu, Mar 23, 2023 at 05:00:08PM +0200, Andy Shevchenko wrote:
> > > On Thu, Mar 23, 2023 at 02:49:01PM +0000, Russell King (Oracle) wrote:
> > > > On Thu, Mar 23, 2023 at 04:38:29PM +0200, Andy Shevchenko wrote:
> > > > > Do you modify its content on the fly?
> > > > 
> > > > Do you want to litter code with casts to get rid of the const?
> > > > 
> > > > > For fwnode as a basic object type we want to reduce the scope of the possible
> > > > > modifications. If you don't modify and APIs you call do not require non-const
> > > > > object, use const for fwnode.
> > > > 
> > > > Let's start here. We pass this fwnode to fwnode_get_phy_mode():
> > > > 
> > > > include/linux/property.h:int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
> > > > 
> > > > Does fwnode_get_phy_mode() alter the contents of the fwnode? Probably
> > > > not, but it doesn't take a const pointer. Therefore, to declare my
> > > > fwnode as const, I'd need to cast the const-ness away before calling
> > > > this.
> > > 
> > > So, fix the fwnode_get_phy_mode(). Is it a problem?
> > 
> > No, I refuse. That's for a different patch set.
> > 
> > > > Then there's phylink_create(). Same problem.
> > > 
> > > So, fix that. Is it a problem?
> > 
> > No for the same reason.
> > 
> > > > So NAK to this const - until such time that we have a concerted effort
> > > > to making functions we call which do not modify the "fwnode" argument
> > > > constify that argument. Otherwise it's just rediculously crazy to
> > > > declare a variable const only to then litter the code with casts to get
> > > > rid of it at every call site.
> > > > 
> > > > Please do a bit of research before making suggestions. Thanks.
> > > 
> > > So, MAK to your patch. You can fix that, and you know that.
> > 
> > Sorry, I don't accept your NAK. While you have a valid point about
> > these things being const, that is not the fault of this patch series,
> > and is something that should be addressed separately.
> > 
> > The lack of const-ness that has been there for quite some time is no
> > reason to NAK a patch that has nothing to do with this.
> 
> To illustrate how rediculous this is:

It's not. But does it make difference?

> $ git grep 'struct fwnode_handle \*.*='
> 
> gives 134 instances. Of those, only five are const, which means 129
> aren't. So I question - why are you singling mine out for what appears
> to be special treatment.
> 
> 
> Let's look at other parts of the fwnode API.
> 
> void __iomem *fwnode_iomap(struct fwnode_handle *fwnode, int index);
> 
> Does that modify the fwnode it was passed? It calls:
> 
>         void __iomem *(*iomap)(struct fwnode_handle *fwnode, int index);
> 
> in struct fwnode_operations, so that would need to be made const as well.
> The only implementation of that which I can find is of_fwnode_iomap()
> which uses to_of_node() on that, which casts away the const-ness. So
> this would be a candidate to making const.

Correct.

> bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle *child);
> 
> I'd be surprised if that modifies either of those fwnodes.

It does. Now your time to be surprised.

> It seems
> to use fwnode_for_each_parent_node() from the child, which passes
> "child" to fwnode_get_parent(), which itself is const. Therefore, it
> seems there's no reason not to make "child" const. "ancestor" can
> also be made const since it's only being used for pointer-compares.

All getters return _different_ fwnode which is not const due to modification
of the _returned_ fwnode.

Do a bit of investigation, please. Thanks.

> unsigned int fwnode_graph_get_endpoint_count(struct fwnode_handle *fwnode,
>                                              unsigned long flags);
> 
> Similar story with this, although it uses
> fwnode_graph_for_each_endpoint(), which seems to mean that "fwnode"
> can also be const.

Correct.

> My point is that there are several things in the fwnode API that
> should be made const but that aren't, but which should likely be
> fixed before requiring const-ness of those fwnode_handle
> declarations in people's code.

OK.

I started doing something about this as you may easily check with `git log`.
Now, instead of playing a good citizen of the community you are trying to
diminish the others' asks.

I think the further continuation of this discussion doesn't make much sense.
But thank you for your opinion.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

[Andy Shevchenko]

On Thu, Mar 23, 2023 at 06:34:32PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 23, 2023 at 04:18:15PM +0000, Russell King (Oracle) wrote:
> > On Thu, Mar 23, 2023 at 03:23:12PM +0000, Russell King (Oracle) wrote:
> > > On Thu, Mar 23, 2023 at 05:00:08PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Mar 23, 2023 at 02:49:01PM +0000, Russell King (Oracle) wrote:
> > > > > On Thu, Mar 23, 2023 at 04:38:29PM +0200, Andy Shevchenko wrote:
> > > > > > Do you modify its content on the fly?
> > > > > 
> > > > > Do you want to litter code with casts to get rid of the const?
> > > > > 
> > > > > > For fwnode as a basic object type we want to reduce the scope of the possible
> > > > > > modifications. If you don't modify and APIs you call do not require non-const
> > > > > > object, use const for fwnode.
> > > > > 
> > > > > Let's start here. We pass this fwnode to fwnode_get_phy_mode():
> > > > > 
> > > > > include/linux/property.h:int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
> > > > > 
> > > > > Does fwnode_get_phy_mode() alter the contents of the fwnode? Probably
> > > > > not, but it doesn't take a const pointer. Therefore, to declare my
> > > > > fwnode as const, I'd need to cast the const-ness away before calling
> > > > > this.
> > > > 
> > > > So, fix the fwnode_get_phy_mode(). Is it a problem?
> > > 
> > > No, I refuse. That's for a different patch set.
> > > 
> > > > > Then there's phylink_create(). Same problem.
> > > > 
> > > > So, fix that. Is it a problem?
> > > 
> > > No for the same reason.
> > > 
> > > > > So NAK to this const - until such time that we have a concerted effort
> > > > > to making functions we call which do not modify the "fwnode" argument
> > > > > constify that argument. Otherwise it's just rediculously crazy to
> > > > > declare a variable const only to then litter the code with casts to get
> > > > > rid of it at every call site.
> > > > > 
> > > > > Please do a bit of research before making suggestions. Thanks.
> > > > 
> > > > So, MAK to your patch. You can fix that, and you know that.
> > > 
> > > Sorry, I don't accept your NAK. While you have a valid point about
> > > these things being const, that is not the fault of this patch series,
> > > and is something that should be addressed separately.
> > > 
> > > The lack of const-ness that has been there for quite some time is no
> > > reason to NAK a patch that has nothing to do with this.
> > 
> > To illustrate how rediculous this is:
> 
> It's not. But does it make difference?
> 
> > $ git grep 'struct fwnode_handle \*.*='
> > 
> > gives 134 instances. Of those, only five are const, which means 129
> > aren't. So I question - why are you singling mine out for what appears
> > to be special treatment.
> > 
> > 
> > Let's look at other parts of the fwnode API.
> > 
> > void __iomem *fwnode_iomap(struct fwnode_handle *fwnode, int index);
> > 
> > Does that modify the fwnode it was passed? It calls:
> > 
> >         void __iomem *(*iomap)(struct fwnode_handle *fwnode, int index);
> > 
> > in struct fwnode_operations, so that would need to be made const as well.
> > The only implementation of that which I can find is of_fwnode_iomap()
> > which uses to_of_node() on that, which casts away the const-ness. So
> > this would be a candidate to making const.
> 
> Correct.
> 
> > bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle *child);
> > 
> > I'd be surprised if that modifies either of those fwnodes.

> It does. Now your time to be surprised.

Oops, I put it into a wrong place. The above does not touch them, but...

> > It seems
> > to use fwnode_for_each_parent_node() from the child, which passes
> > "child" to fwnode_get_parent(),

...this one touches.

> >	which itself is const. Therefore, it
> > seems there's no reason not to make "child" const. "ancestor" can
> > also be made const since it's only being used for pointer-compares.
> 
> All getters return _different_ fwnode which is not const due to modification
> of the _returned_ fwnode.
> 
> Do a bit of investigation, please. Thanks.
> 
> > unsigned int fwnode_graph_get_endpoint_count(struct fwnode_handle *fwnode,
> >                                              unsigned long flags);
> > 
> > Similar story with this, although it uses
> > fwnode_graph_for_each_endpoint(), which seems to mean that "fwnode"
> > can also be const.
> 
> Correct.
> 
> > My point is that there are several things in the fwnode API that
> > should be made const but that aren't, but which should likely be
> > fixed before requiring const-ness of those fwnode_handle
> > declarations in people's code.
> 
> OK.
> 
> I started doing something about this as you may easily check with `git log`.
> Now, instead of playing a good citizen of the community you are trying to
> diminish the others' asks.
> 
> I think the further continuation of this discussion doesn't make much sense.
> But thank you for your opinion.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

[Russell King (Oracle)]

On Thu, Mar 23, 2023 at 06:34:32PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 23, 2023 at 04:18:15PM +0000, Russell King (Oracle) wrote:
> > On Thu, Mar 23, 2023 at 03:23:12PM +0000, Russell King (Oracle) wrote:
> > > On Thu, Mar 23, 2023 at 05:00:08PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Mar 23, 2023 at 02:49:01PM +0000, Russell King (Oracle) wrote:
> > > > > On Thu, Mar 23, 2023 at 04:38:29PM +0200, Andy Shevchenko wrote:
> > > > > > Do you modify its content on the fly?
> > > > > 
> > > > > Do you want to litter code with casts to get rid of the const?
> > > > > 
> > > > > > For fwnode as a basic object type we want to reduce the scope of the possible
> > > > > > modifications. If you don't modify and APIs you call do not require non-const
> > > > > > object, use const for fwnode.
> > > > > 
> > > > > Let's start here. We pass this fwnode to fwnode_get_phy_mode():
> > > > > 
> > > > > include/linux/property.h:int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
> > > > > 
> > > > > Does fwnode_get_phy_mode() alter the contents of the fwnode? Probably
> > > > > not, but it doesn't take a const pointer. Therefore, to declare my
> > > > > fwnode as const, I'd need to cast the const-ness away before calling
> > > > > this.
> > > > 
> > > > So, fix the fwnode_get_phy_mode(). Is it a problem?
> > > 
> > > No, I refuse. That's for a different patch set.
> > > 
> > > > > Then there's phylink_create(). Same problem.
> > > > 
> > > > So, fix that. Is it a problem?
> > > 
> > > No for the same reason.
> > > 
> > > > > So NAK to this const - until such time that we have a concerted effort
> > > > > to making functions we call which do not modify the "fwnode" argument
> > > > > constify that argument. Otherwise it's just rediculously crazy to
> > > > > declare a variable const only to then litter the code with casts to get
> > > > > rid of it at every call site.
> > > > > 
> > > > > Please do a bit of research before making suggestions. Thanks.
> > > > 
> > > > So, MAK to your patch. You can fix that, and you know that.
> > > 
> > > Sorry, I don't accept your NAK. While you have a valid point about
> > > these things being const, that is not the fault of this patch series,
> > > and is something that should be addressed separately.
> > > 
> > > The lack of const-ness that has been there for quite some time is no
> > > reason to NAK a patch that has nothing to do with this.
> > 
> > To illustrate how rediculous this is:
> 
> It's not. But does it make difference?
> 
> > $ git grep 'struct fwnode_handle \*.*='
> > 
> > gives 134 instances. Of those, only five are const, which means 129
> > aren't. So I question - why are you singling mine out for what appears
> > to be special treatment.

You failed to answer this. I can only assume you don't have an answer
for why you are singling out my use compared to all the 129 other
uses in the kernel that follow this pattern. Given the number of them,
that is no basis to NAK this patch.

> > Let's look at other parts of the fwnode API.
> > 
> > void __iomem *fwnode_iomap(struct fwnode_handle *fwnode, int index);
> > 
> > Does that modify the fwnode it was passed? It calls:
> > 
> >         void __iomem *(*iomap)(struct fwnode_handle *fwnode, int index);
> > 
> > in struct fwnode_operations, so that would need to be made const as well.
> > The only implementation of that which I can find is of_fwnode_iomap()
> > which uses to_of_node() on that, which casts away the const-ness. So
> > this would be a candidate to making const.
> 
> Correct.
> 
> > bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle *child);
> > 
> > I'd be surprised if that modifies either of those fwnodes.
> 
> It does. Now your time to be surprised.

I believe you are mistaken.

> > It seems
> > to use fwnode_for_each_parent_node() from the child, which passes
> > "child" to fwnode_get_parent(), which itself is const. Therefore, it
> > seems there's no reason not to make "child" const. "ancestor" can
> > also be made const since it's only being used for pointer-compares.
> 
> All getters return _different_ fwnode which is not const due to modification
> of the _returned_ fwnode.
> 
> Do a bit of investigation, please. Thanks.

I did exactly that, and I included the research in my email. How is
fwnode_is_ancestor_of() a "getter" when it returns a _boolean_?

Did you read my fully researched explanation? "child" is *not* modified,
it is merely passed to another function that accepts a *const* pointer.

Here's teaching you to suck eggs, because that's clearly what it's
going to take to make you see sense:

bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle *child)
{
        struct fwnode_handle *parent;

        if (IS_ERR_OR_NULL(ancestor))
                return false;

        if (child == ancestor)
                return true;

        fwnode_for_each_parent_node(child, parent) {
                if (parent == ancestor) {
                        fwnode_handle_put(parent);
                        return true;
                }
        }
        return false;
}

"child" is only used by the if() "child == ancestor" and
fwnode_for_each_parent_node(). fwnode_for_each_parent_node() is:

#define fwnode_for_each_parent_node(fwnode, parent)             \
        for (parent = fwnode_get_parent(fwnode); parent;        \
             parent = fwnode_get_next_parent(parent))

so child is passed in as fwnode there, and gets passed to
fwnode_get_parent(). fwnode_get_parent() is declared as:

struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);

So "child" ends up passed into this function, which takes a const
pointer. At no point is "child" assigned to in fwnode_is_ancestor_of().
It is only used in situations where it can be a const pointer.

Now, if we look at "ancestor" then there are three locations it is used:

        if (IS_ERR_OR_NULL(ancestor))

        if (child == ancestor)

                if (parent == ancestor) {

All of these can cope with ancestor being const.

I will grant you that "parent" can't be const, but then I *never* said
it would be as I was only taking about the functions arguments.

So, I think you need to revise your comment on this, because clearly it
was incorrect.

> > unsigned int fwnode_graph_get_endpoint_count(struct fwnode_handle *fwnode,
> >                                              unsigned long flags);
> > 
> > Similar story with this, although it uses
> > fwnode_graph_for_each_endpoint(), which seems to mean that "fwnode"
> > can also be const.
> 
> Correct.
> 
> > My point is that there are several things in the fwnode API that
> > should be made const but that aren't, but which should likely be
> > fixed before requiring const-ness of those fwnode_handle
> > declarations in people's code.
> 
> OK.
> 
> I started doing something about this as you may easily check with `git log`.
> Now, instead of playing a good citizen of the community you are trying to
> diminish the others' asks.

No, I'm finding your requests to be rather inconsistent, lacking
in useful value, and lacking an appreciation of how other parts of
the kernel's development process works.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

[Andy Shevchenko]

On Thu, Mar 23, 2023 at 05:06:49PM +0000, Russell King (Oracle) wrote:

OK, you are right as always, I'm wrong. Case closed. Thank you.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

[Russell King (Oracle)]

On Thu, Mar 23, 2023 at 05:00:08PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 23, 2023 at 02:49:01PM +0000, Russell King (Oracle) wrote:
> > Let's start here. We pass this fwnode to fwnode_get_phy_mode():
> > 
> > include/linux/property.h:int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
> > 
> > Does fwnode_get_phy_mode() alter the contents of the fwnode? Probably
> > not, but it doesn't take a const pointer. Therefore, to declare my
> > fwnode as const, I'd need to cast the const-ness away before calling
> > this.
> 
> So, fix the fwnode_get_phy_mode(). Is it a problem?
> 
> > Then there's phylink_create(). Same problem.
> 
> So, fix that. Is it a problem?

To do both of these creates a five patch series, because there are so
many things that need to be constified:

fwnode_get_phy_mode() is the trivial one.

sfp_bus_find_fwnode(), and the sfp-bus internal fwnode uses.

fwnode_get_phy_node().

phylink_create(), phylink_parse_fixedlink(), phylink_parse_mode(),
phylink_fwnode_phy_connect().

Hopefully nothing breaks as a result of changing all those - but that
can hardly be "tacked" on to the start of my series as a trivial
change - and clearly such a change should _not_ be part of this
series.

Those five patches do not include moving fwnode_get_phy_mode(), whose
location remains undecided.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

[Andy Shevchenko]

On Thu, Mar 23, 2023 at 05:53:46PM +0000, Russell King (Oracle) wrote:
> On Thu, Mar 23, 2023 at 05:00:08PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 23, 2023 at 02:49:01PM +0000, Russell King (Oracle) wrote:
> > > Let's start here. We pass this fwnode to fwnode_get_phy_mode():
> > > 
> > > include/linux/property.h:int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
> > > 
> > > Does fwnode_get_phy_mode() alter the contents of the fwnode? Probably
> > > not, but it doesn't take a const pointer. Therefore, to declare my
> > > fwnode as const, I'd need to cast the const-ness away before calling
> > > this.
> > 
> > So, fix the fwnode_get_phy_mode(). Is it a problem?
> > 
> > > Then there's phylink_create(). Same problem.
> > 
> > So, fix that. Is it a problem?
> 
> To do both of these creates a five patch series, because there are so
> many things that need to be constified:
> 
> fwnode_get_phy_mode() is the trivial one.
> 
> sfp_bus_find_fwnode(), and the sfp-bus internal fwnode uses.
> 
> fwnode_get_phy_node().
> 
> phylink_create(), phylink_parse_fixedlink(), phylink_parse_mode(),
> phylink_fwnode_phy_connect().
> 
> Hopefully nothing breaks as a result of changing all those - but that
> can hardly be "tacked" on to the start of my series as a trivial
> change - and clearly such a change should _not_ be part of this
> series.

Thank you for doing that!

> Those five patches do not include moving fwnode_get_phy_mode(), whose
> location remains undecided.

No problem, we like iterative work.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Andrew Lunn]

> So, given that this is only supposed to be used for mv88e6xxx because
> of it's legacy, maybe the check in dsa_port_phylink_create() should
> be:
> 
>         fwnode = of_fwnode_handle(dp->dn);
>         if (fwnode && ds->ops->port_get_fwnode) {
> 
> In other words, we only allow the replacement of the firmware
> description if one already existed.

That sounds reasonable.

> Alternatively, we could use:
> 
> 	if (!dsa_port_is_user(dp) && ds->ops->port_get_fwnode) {
> 
> since mv88e6xxx today only does this "max speed" thing for CPU and
> DSA ports, and thus we only need to replace the firmware description
> for these ports - and we can document that port_get_fwnode is only
> for CPU and DSA ports.

Also reasonable.

The first seems better for the Non-DT, where as the second makes it
clear it is supposed to be for CPU and DSA ports only.

Is it over the top to combine them?

   Andrew

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Russell King (Oracle)]

On Thu, Mar 23, 2023 at 07:17:27PM +0100, Andrew Lunn wrote:
> > So, given that this is only supposed to be used for mv88e6xxx because
> > of it's legacy, maybe the check in dsa_port_phylink_create() should
> > be:
> > 
> >         fwnode = of_fwnode_handle(dp->dn);
> >         if (fwnode && ds->ops->port_get_fwnode) {
> > 
> > In other words, we only allow the replacement of the firmware
> > description if one already existed.
> 
> That sounds reasonable.
> 
> > Alternatively, we could use:
> > 
> > 	if (!dsa_port_is_user(dp) && ds->ops->port_get_fwnode) {
> > 
> > since mv88e6xxx today only does this "max speed" thing for CPU and
> > DSA ports, and thus we only need to replace the firmware description
> > for these ports - and we can document that port_get_fwnode is only
> > for CPU and DSA ports.
> 
> Also reasonable.
> 
> The first seems better for the Non-DT, where as the second makes it
> clear it is supposed to be for CPU and DSA ports only.
> 
> Is it over the top to combine them?

To be clear, you're suggesting:

	if (!dsa_port_is_user(dp) && fwnode && ds->ops->port_get_fwnode) {

?

If so, yes - you know better than I how these bits are supposed to work.
Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Andrew Lunn]

> To be clear, you're suggesting:
> 
> 	if (!dsa_port_is_user(dp) && fwnode && ds->ops->port_get_fwnode) {
> 
> ?
> 
> If so, yes - you know better than I how these bits are supposed to work.
> Thanks.

Yes, that is what i was thinking.

Thanks
	Andrew

Re: [PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

[Russell King (Oracle)]

On Thu, Mar 23, 2023 at 08:04:45PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 23, 2023 at 05:53:46PM +0000, Russell King (Oracle) wrote:
> > On Thu, Mar 23, 2023 at 05:00:08PM +0200, Andy Shevchenko wrote:
> > > On Thu, Mar 23, 2023 at 02:49:01PM +0000, Russell King (Oracle) wrote:
> > > > Let's start here. We pass this fwnode to fwnode_get_phy_mode():
> > > > 
> > > > include/linux/property.h:int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
> > > > 
> > > > Does fwnode_get_phy_mode() alter the contents of the fwnode? Probably
> > > > not, but it doesn't take a const pointer. Therefore, to declare my
> > > > fwnode as const, I'd need to cast the const-ness away before calling
> > > > this.
> > > 
> > > So, fix the fwnode_get_phy_mode(). Is it a problem?
> > > 
> > > > Then there's phylink_create(). Same problem.
> > > 
> > > So, fix that. Is it a problem?
> > 
> > To do both of these creates a five patch series, because there are so
> > many things that need to be constified:
> > 
> > fwnode_get_phy_mode() is the trivial one.
> > 
> > sfp_bus_find_fwnode(), and the sfp-bus internal fwnode uses.
> > 
> > fwnode_get_phy_node().
> > 
> > phylink_create(), phylink_parse_fixedlink(), phylink_parse_mode(),
> > phylink_fwnode_phy_connect().
> > 
> > Hopefully nothing breaks as a result of changing all those - but that
> > can hardly be "tacked" on to the start of my series as a trivial
> > change - and clearly such a change should _not_ be part of this
> > series.
> 
> Thank you for doing that!
> 
> > Those five patches do not include moving fwnode_get_phy_mode(), whose
> > location remains undecided.
> 
> No problem, we like iterative work.

Oh, and what a waste of time that was.

You request that the fwnode should be declared const, but now I
realise that you never looked at patch 4, where we call
fwnode_remove_software_node() on this fwnode (so that the swnodes
returned by ->port_get_fwnode are released).

Since fwnode_remove_software_node() modifies the swnode containing
the fwnode, and therefore does not take a const fwnode pointer, we
also can't make _this_ fwnode pointer const - even with all those
changes.

So, I feel like I've been on a wild goose chase and what a needless
effort it has been to concoct patches that I don't even need for
this.

So again, rmk was right! Sigh.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Heikki Krogerus]

Hi Russell,

On Wed, Mar 22, 2023 at 12:00:21PM +0000, Russell King (Oracle) wrote:
> +static struct fwnode_handle *mv88e6xxx_create_fixed_swnode(struct fwnode_handle *parent,
> +							   int speed,
> +							   int duplex)
> +{
> +	struct property_entry fixed_link_props[3] = { };
> +
> +	fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed);
> +	if (duplex == DUPLEX_FULL)
> +		fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex");
> +
> +	return fwnode_create_named_software_node(fixed_link_props, parent,
> +						 "fixed-link");
> +}
> +
> +static struct fwnode_handle *mv88e6xxx_create_port_swnode(phy_interface_t mode,
> +							  int speed,
> +							  int duplex)
> +{
> +	struct property_entry port_props[2] = {};
> +	struct fwnode_handle *fixed_link_fwnode;
> +	struct fwnode_handle *new_port_fwnode;
> +
> +	port_props[0] = PROPERTY_ENTRY_STRING("phy-mode", phy_modes(mode));
> +	new_port_fwnode = fwnode_create_software_node(port_props, NULL);
> +	if (IS_ERR(new_port_fwnode))
> +		return new_port_fwnode;
> +
> +	fixed_link_fwnode = mv88e6xxx_create_fixed_swnode(new_port_fwnode,
> +							  speed, duplex);
> +	if (IS_ERR(fixed_link_fwnode)) {
> +		fwnode_remove_software_node(new_port_fwnode);
> +		return fixed_link_fwnode;
> +	}
> +
> +	return new_port_fwnode;
> +}

That new fwnode_create_named_software_node() function looks like a
conflict waiting to happen - if a driver adds a node to the root level
(does not have to be root level), all the tests will pass because
there is only a single device, but when a user later tries the driver
with two devices, it fails, because the node already exist. But you
don't need that function at all.

Here's an example how you can add the nodes with the already existing
APIs. To keep this example simple, I'm expecting that you have members
for the software nodes to the struct mv88e6xxx_chip:

struct mv88e6xxx_chip {
        ...
        /* swnodes */
        struct software_node port_swnode;
        struct software_node fixed_link_swnode;
};

Of course, you don't have to add those members if you don't want to.
Then you just need to allocate the nodes separately, but that should
not be a problem. In any case, something like this:

static struct fwnode_handle *mv88e6xxx_create_port_swnode(struct mv88e6xxx_chip *chip,
                                                          phy_interface_t mode,
							  int speed,
							  int duplex)
{
	struct property_entry fixed_link_props[3] = { };
	struct property_entry port_props[2] = { };
	int ret;

        /*
         * First register the port node.
         */
	port_props[0] = PROPERTY_ENTRY_STRING("phy-mode", phy_modes(mode));

	chip->port_swnode.properties = property_entries_dup(port_props);
        if (IS_ERR(chip->port_swnode.properties))
                return ERR_CAST(chip->port_swnode.properties);

	ret = software_node_register(&chip->port_swnode);
	if (ret) {
                kfree(chip->port_swnode.properties);
		return ERR_PTR(ret);
        }

        /*
         * Then the second node, child of the port node.
         */
	fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed);
	if (duplex == DUPLEX_FULL)
		fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex");

        chip->fixed_link_swnode.name = "fixed-link";
        chip->fixed_link_swnode.parent = &chip->port_swnode;
	chip->fixed_link_swnode.properties = property_entries_dup(fixed_link_props);
        if (IS_ERR(chip->port_swnode.properties)) {
                software_node_unregister(&chip->port_swnode);
                kfree(chip->port_swnode.properties);
                return ERR_CAST(chip->fixed_link_swnode.properties);
        }

	ret = software_node_register(&chip->fixed_link_swnode);
        if (ret) {
                software_node_unregister(&chip->port_swnode);
                kfree(chip->port_swnode.properties);
                kfree(chip->fixed_link_swnode.properties);
		return ERR_PTR(ret);
        }

        /*
         * Finally, return the port fwnode.
         */
        return software_node_fwnode(&chip->port_swnode);
}

thanks,

-- 
heikki

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Russell King (Oracle)]

On Fri, Mar 24, 2023 at 04:49:32PM +0200, Heikki Krogerus wrote:
> Hi Russell,
> 
> On Wed, Mar 22, 2023 at 12:00:21PM +0000, Russell King (Oracle) wrote:
> > +static struct fwnode_handle *mv88e6xxx_create_fixed_swnode(struct fwnode_handle *parent,
> > +							   int speed,
> > +							   int duplex)
> > +{
> > +	struct property_entry fixed_link_props[3] = { };
> > +
> > +	fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed);
> > +	if (duplex == DUPLEX_FULL)
> > +		fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex");
> > +
> > +	return fwnode_create_named_software_node(fixed_link_props, parent,
> > +						 "fixed-link");
> > +}
> > +
> > +static struct fwnode_handle *mv88e6xxx_create_port_swnode(phy_interface_t mode,
> > +							  int speed,
> > +							  int duplex)
> > +{
> > +	struct property_entry port_props[2] = {};
> > +	struct fwnode_handle *fixed_link_fwnode;
> > +	struct fwnode_handle *new_port_fwnode;
> > +
> > +	port_props[0] = PROPERTY_ENTRY_STRING("phy-mode", phy_modes(mode));
> > +	new_port_fwnode = fwnode_create_software_node(port_props, NULL);
> > +	if (IS_ERR(new_port_fwnode))
> > +		return new_port_fwnode;
> > +
> > +	fixed_link_fwnode = mv88e6xxx_create_fixed_swnode(new_port_fwnode,
> > +							  speed, duplex);
> > +	if (IS_ERR(fixed_link_fwnode)) {
> > +		fwnode_remove_software_node(new_port_fwnode);
> > +		return fixed_link_fwnode;
> > +	}
> > +
> > +	return new_port_fwnode;
> > +}
> 
> That new fwnode_create_named_software_node() function looks like a
> conflict waiting to happen - if a driver adds a node to the root level
> (does not have to be root level), all the tests will pass because
> there is only a single device, but when a user later tries the driver
> with two devices, it fails, because the node already exist. But you
> don't need that function at all.

I think you're totally failing to explain how this can fail.

Let me reiterate what thestructure of the swnodes here is:

	root
	`- node%d (%d allocated by root IDA)
	   +- phy-mode property
	   `- fixed-link
	      +- speed property
	      `- optional full-duplex property

If we have two different devices creating these nodes, then at the
root level, they will end up having different root names. The
"fixed-link" is a child of this node.

swnode already allows multiple identical names at the sub-node
level - each node ends up with its own IDA to allocate the generic
"node%d" names from. So as soon as we have multiple nodes, they
end up as this:

	root
	+- node0
	|  `- node 0
	+- node1
	|  `- node 0
	+- node2
	|  `- node 0
	etc

So, if we end up with two devices creating these at the same time,
we end up with:

	root
	+- nodeA (A allocated by root IDA)
	|  +- phy-mode property
	|  `- fixed-link
	|     +- speed property
	|     `- optional full-duplex property
	`- nodeB (B allocated by root IDA, different from above)
	   +- phy-mode property
	   `- fixed-link
	      +- speed property
	      `- optional full-duplex property

Since the kobject is parented to the parent's kobject, what we
end up with in sysfs is:

	.../nodeA/fixed-link/speed
	.../nodeB/fixed-link/speed

Thus, the "fixed-link" ndoes can _not_ conflict.

Please explain in detail where you think the conflict is, because
so far no one has been able to counter my assertions that this is
_safe_ with a proper full technical description of the problem.
All I get is hand-wavey "this conflicts".

Honestly, I'm getting sick of poor quality reviews... the next
poor review that claims there's a conflict here without properly
explain it will be told where to go.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Heikki Krogerus]

On Fri, Mar 24, 2023 at 05:04:25PM +0000, Russell King (Oracle) wrote:
> On Fri, Mar 24, 2023 at 04:49:32PM +0200, Heikki Krogerus wrote:
> > Hi Russell,
> > 
> > On Wed, Mar 22, 2023 at 12:00:21PM +0000, Russell King (Oracle) wrote:
> > > +static struct fwnode_handle *mv88e6xxx_create_fixed_swnode(struct fwnode_handle *parent,
> > > +							   int speed,
> > > +							   int duplex)
> > > +{
> > > +	struct property_entry fixed_link_props[3] = { };
> > > +
> > > +	fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed);
> > > +	if (duplex == DUPLEX_FULL)
> > > +		fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex");
> > > +
> > > +	return fwnode_create_named_software_node(fixed_link_props, parent,
> > > +						 "fixed-link");
> > > +}
> > > +
> > > +static struct fwnode_handle *mv88e6xxx_create_port_swnode(phy_interface_t mode,
> > > +							  int speed,
> > > +							  int duplex)
> > > +{
> > > +	struct property_entry port_props[2] = {};
> > > +	struct fwnode_handle *fixed_link_fwnode;
> > > +	struct fwnode_handle *new_port_fwnode;
> > > +
> > > +	port_props[0] = PROPERTY_ENTRY_STRING("phy-mode", phy_modes(mode));
> > > +	new_port_fwnode = fwnode_create_software_node(port_props, NULL);
> > > +	if (IS_ERR(new_port_fwnode))
> > > +		return new_port_fwnode;
> > > +
> > > +	fixed_link_fwnode = mv88e6xxx_create_fixed_swnode(new_port_fwnode,
> > > +							  speed, duplex);
> > > +	if (IS_ERR(fixed_link_fwnode)) {
> > > +		fwnode_remove_software_node(new_port_fwnode);
> > > +		return fixed_link_fwnode;
> > > +	}
> > > +
> > > +	return new_port_fwnode;
> > > +}
> > 
> > That new fwnode_create_named_software_node() function looks like a
> > conflict waiting to happen - if a driver adds a node to the root level
> > (does not have to be root level), all the tests will pass because
> > there is only a single device, but when a user later tries the driver
> > with two devices, it fails, because the node already exist. But you
> > don't need that function at all.
> 
> I think you're totally failing to explain how this can fail.
> 
> Let me reiterate what thestructure of the swnodes here is:
> 
> 	root
> 	`- node%d (%d allocated by root IDA)
> 	   +- phy-mode property
> 	   `- fixed-link
> 	      +- speed property
> 	      `- optional full-duplex property
> 
> If we have two different devices creating these nodes, then at the
> root level, they will end up having different root names. The
> "fixed-link" is a child of this node.

Ah, sorry, the problem is not with this patch, or your use case. The
problem is with the PATCH 1/7 of this series where you introduce that
new function fwnode_create_named_software_node() which will not be
tied to your use case only. In this patch you just use that function.
I should have been more clear on that.

I really just wanted to show how you can create those nodes by using
the API designed for the statically described software nodes. So you
don't need that new function. Please check that proposal from my
original reply.

If the potential conflict that the new function creates is still not
clear, then - firstly, you have to remember that that API is not only
for your drivers, it's generic API! - the problem comes from the fact
that there simply is nothing preventing it from being used to place
the new nodes always at the same level. So for example using NULL as
the parent:

        fwnode = fwnode_create_named_software_node(props,
                                                   NULL, /* NOTE */
                                                   "same_name");

thanks,

-- 
heikki

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Russell King (Oracle)]

On Mon, Mar 27, 2023 at 01:28:06PM +0300, Heikki Krogerus wrote:
> On Fri, Mar 24, 2023 at 05:04:25PM +0000, Russell King (Oracle) wrote:
> > On Fri, Mar 24, 2023 at 04:49:32PM +0200, Heikki Krogerus wrote:
> > > Hi Russell,
> > > 
> > > On Wed, Mar 22, 2023 at 12:00:21PM +0000, Russell King (Oracle) wrote:
> > > > +static struct fwnode_handle *mv88e6xxx_create_fixed_swnode(struct fwnode_handle *parent,
> > > > +							   int speed,
> > > > +							   int duplex)
> > > > +{
> > > > +	struct property_entry fixed_link_props[3] = { };
> > > > +
> > > > +	fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed);
> > > > +	if (duplex == DUPLEX_FULL)
> > > > +		fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex");
> > > > +
> > > > +	return fwnode_create_named_software_node(fixed_link_props, parent,
> > > > +						 "fixed-link");
> > > > +}
> > > > +
> > > > +static struct fwnode_handle *mv88e6xxx_create_port_swnode(phy_interface_t mode,
> > > > +							  int speed,
> > > > +							  int duplex)
> > > > +{
> > > > +	struct property_entry port_props[2] = {};
> > > > +	struct fwnode_handle *fixed_link_fwnode;
> > > > +	struct fwnode_handle *new_port_fwnode;
> > > > +
> > > > +	port_props[0] = PROPERTY_ENTRY_STRING("phy-mode", phy_modes(mode));
> > > > +	new_port_fwnode = fwnode_create_software_node(port_props, NULL);
> > > > +	if (IS_ERR(new_port_fwnode))
> > > > +		return new_port_fwnode;
> > > > +
> > > > +	fixed_link_fwnode = mv88e6xxx_create_fixed_swnode(new_port_fwnode,
> > > > +							  speed, duplex);
> > > > +	if (IS_ERR(fixed_link_fwnode)) {
> > > > +		fwnode_remove_software_node(new_port_fwnode);
> > > > +		return fixed_link_fwnode;
> > > > +	}
> > > > +
> > > > +	return new_port_fwnode;
> > > > +}
> > > 
> > > That new fwnode_create_named_software_node() function looks like a
> > > conflict waiting to happen - if a driver adds a node to the root level
> > > (does not have to be root level), all the tests will pass because
> > > there is only a single device, but when a user later tries the driver
> > > with two devices, it fails, because the node already exist. But you
> > > don't need that function at all.
> > 
> > I think you're totally failing to explain how this can fail.
> > 
> > Let me reiterate what thestructure of the swnodes here is:
> > 
> > 	root
> > 	`- node%d (%d allocated by root IDA)
> > 	   +- phy-mode property
> > 	   `- fixed-link
> > 	      +- speed property
> > 	      `- optional full-duplex property
> > 
> > If we have two different devices creating these nodes, then at the
> > root level, they will end up having different root names. The
> > "fixed-link" is a child of this node.
> 
> Ah, sorry, the problem is not with this patch, or your use case. The
> problem is with the PATCH 1/7 of this series where you introduce that
> new function fwnode_create_named_software_node() which will not be
> tied to your use case only. In this patch you just use that function.
> I should have been more clear on that.

How is this any different from creating two struct device's with the
same parent and the same name? Or kobject_add() with the same parent
and name?

> I really just wanted to show how you can create those nodes by using
> the API designed for the statically described software nodes. So you
> don't need that new function. Please check that proposal from my
> original reply.

I don't see why I should. This is clearly a case that if one creates
two named nodes with the same name and same parent, it should fail and
it's definitely a "well don't do that then" in just the same way that
one doesn't do it with kobject_add() or any of the other numerous
interfaces that take names in a space that need to be unique.

I really don't think there is any issue here to be solved. In fact,
I think solving it will add additional useless complexity that just
isn't required - which adds extra code that can be wrong and fail.

Let's keep this simple. This approach is simple. If one does something
stupid (like creating two named nodes with the same name and same
parent) then it will verbosely fail. That is a good thing.

Internal kernel APIs are not supposed to protect people from being
stupid.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Heikki Krogerus]

Hi Russell,

On Mon, Mar 27, 2023 at 11:55:00AM +0100, Russell King (Oracle) wrote:
> On Mon, Mar 27, 2023 at 01:28:06PM +0300, Heikki Krogerus wrote:
> > On Fri, Mar 24, 2023 at 05:04:25PM +0000, Russell King (Oracle) wrote:
> > > On Fri, Mar 24, 2023 at 04:49:32PM +0200, Heikki Krogerus wrote:
> > > > Hi Russell,
> > > > 
> > > > On Wed, Mar 22, 2023 at 12:00:21PM +0000, Russell King (Oracle) wrote:
> > > > > +static struct fwnode_handle *mv88e6xxx_create_fixed_swnode(struct fwnode_handle *parent,
> > > > > +							   int speed,
> > > > > +							   int duplex)
> > > > > +{
> > > > > +	struct property_entry fixed_link_props[3] = { };
> > > > > +
> > > > > +	fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed);
> > > > > +	if (duplex == DUPLEX_FULL)
> > > > > +		fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex");
> > > > > +
> > > > > +	return fwnode_create_named_software_node(fixed_link_props, parent,
> > > > > +						 "fixed-link");
> > > > > +}
> > > > > +
> > > > > +static struct fwnode_handle *mv88e6xxx_create_port_swnode(phy_interface_t mode,
> > > > > +							  int speed,
> > > > > +							  int duplex)
> > > > > +{
> > > > > +	struct property_entry port_props[2] = {};
> > > > > +	struct fwnode_handle *fixed_link_fwnode;
> > > > > +	struct fwnode_handle *new_port_fwnode;
> > > > > +
> > > > > +	port_props[0] = PROPERTY_ENTRY_STRING("phy-mode", phy_modes(mode));
> > > > > +	new_port_fwnode = fwnode_create_software_node(port_props, NULL);
> > > > > +	if (IS_ERR(new_port_fwnode))
> > > > > +		return new_port_fwnode;
> > > > > +
> > > > > +	fixed_link_fwnode = mv88e6xxx_create_fixed_swnode(new_port_fwnode,
> > > > > +							  speed, duplex);
> > > > > +	if (IS_ERR(fixed_link_fwnode)) {
> > > > > +		fwnode_remove_software_node(new_port_fwnode);
> > > > > +		return fixed_link_fwnode;
> > > > > +	}
> > > > > +
> > > > > +	return new_port_fwnode;
> > > > > +}
> > > > 
> > > > That new fwnode_create_named_software_node() function looks like a
> > > > conflict waiting to happen - if a driver adds a node to the root level
> > > > (does not have to be root level), all the tests will pass because
> > > > there is only a single device, but when a user later tries the driver
> > > > with two devices, it fails, because the node already exist. But you
> > > > don't need that function at all.
> > > 
> > > I think you're totally failing to explain how this can fail.
> > > 
> > > Let me reiterate what thestructure of the swnodes here is:
> > > 
> > > 	root
> > > 	`- node%d (%d allocated by root IDA)
> > > 	   +- phy-mode property
> > > 	   `- fixed-link
> > > 	      +- speed property
> > > 	      `- optional full-duplex property
> > > 
> > > If we have two different devices creating these nodes, then at the
> > > root level, they will end up having different root names. The
> > > "fixed-link" is a child of this node.
> > 
> > Ah, sorry, the problem is not with this patch, or your use case. The
> > problem is with the PATCH 1/7 of this series where you introduce that
> > new function fwnode_create_named_software_node() which will not be
> > tied to your use case only. In this patch you just use that function.
> > I should have been more clear on that.
> 
> How is this any different from creating two struct device's with the
> same parent and the same name? Or kobject_add() with the same parent
> and name?

But that can not mean we have to take the same risk everywhere. I do
understand that we don't protect developers from doing silly decisions
in kernel, but that does not mean that we should simply accept
interfaces into the kernel that expose these risk if we don't need
them.

> > I really just wanted to show how you can create those nodes by using
> > the API designed for the statically described software nodes. So you
> > don't need that new function. Please check that proposal from my
> > original reply.
> 
> I don't see why I should. This is clearly a case that if one creates
> two named nodes with the same name and same parent, it should fail and
> it's definitely a "well don't do that then" in just the same way that
> one doesn't do it with kobject_add() or any of the other numerous
> interfaces that take names in a space that need to be unique.
> 
> I really don't think there is any issue here to be solved. In fact,
> I think solving it will add additional useless complexity that just
> isn't required - which adds extra code that can be wrong and fail.
> 
> Let's keep this simple. This approach is simple. If one does something
> stupid (like creating two named nodes with the same name and same
> parent) then it will verbosely fail. That is a good thing.

Well, I think the most simplest approach would be to have both the
nodes and the properties as part of that struct mv88e6xxx_chip:

struct mv88e6xxx_chip {
        ...
       struct property_entry port_props[2];
       struct property_entry fixed_link_props[3];

       struct software_node port_swnode;
       struct software_node fixed_link_swnode;
};

That allows you to register both nodes in one go:

static struct fwnode_handle *mv88e6xxx_create_port_swnode(struct mv88e6xxx_chip *chip,
                                                          phy_interface_t mode,
                                                          int speed,
                                                          int duplex)
{
        struct software_node *nodes[3] = {
                &chip->port_swnode,
                &chip->fixed_link_swnode,
        };
        int ret;

        chip->port_props[0] = PROPERTY_ENTRY_STRING("phy-mode", phy_modes(mode));
        chip->port_swnode.properties = chip->port_props;

        chip->fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed);
        if (duplex == DUPLEX_FULL)
                chip->fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex");

        chip->fixed_link_swnode.name = "fixed-link";
        chip->fixed_link_swnode.parent = &chip->port_swnode;
        chip->fixed_link_swnode.properties = chip->fixed_link_props;

        ret = software_node_register_node_group(nodes);
        if (ret)
                return ERR_PTR(ret);

        return software_node_fwnode(&chip->port_swnode);
}

> Internal kernel APIs are not supposed to protect people from being
> stupid.

This is an interesting topic. I used to agree with this
idea/philosophy without much thought, but then after (years of :-)
listening maintainers complaining about how new developers always
repeat the same mistakes over an over again, I've started thinking
about it. To use a bit rough analog, if we give a gun to a monkey, how
can we be surprised if if ends up shooting first its mates and then
its own brains out...

Perhaps this is a more generic subject, a topic for some conference
maybe, but I in any case don't think this is a black and white matter.
A little bit of protection is not only a harmful thing and always only
in the way of flexibility.

At the very least, I don't think we should not use this philosophy as
an argument for doing things in ways that may expose even minute risks
in the cases like this were an alternative approach already exists.

Br,

-- 
heikki

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Russell King (Oracle)]

On Mon, Mar 27, 2023 at 05:13:25PM +0300, Heikki Krogerus wrote:
> Hi Russell,
> 
> On Mon, Mar 27, 2023 at 11:55:00AM +0100, Russell King (Oracle) wrote:
> > On Mon, Mar 27, 2023 at 01:28:06PM +0300, Heikki Krogerus wrote:
> > > On Fri, Mar 24, 2023 at 05:04:25PM +0000, Russell King (Oracle) wrote:
> > > > On Fri, Mar 24, 2023 at 04:49:32PM +0200, Heikki Krogerus wrote:
> > > > > Hi Russell,
> > > > > 
> > > > > On Wed, Mar 22, 2023 at 12:00:21PM +0000, Russell King (Oracle) wrote:
> > > > > > +static struct fwnode_handle *mv88e6xxx_create_fixed_swnode(struct fwnode_handle *parent,
> > > > > > +							   int speed,
> > > > > > +							   int duplex)
> > > > > > +{
> > > > > > +	struct property_entry fixed_link_props[3] = { };
> > > > > > +
> > > > > > +	fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed);
> > > > > > +	if (duplex == DUPLEX_FULL)
> > > > > > +		fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex");
> > > > > > +
> > > > > > +	return fwnode_create_named_software_node(fixed_link_props, parent,
> > > > > > +						 "fixed-link");
> > > > > > +}
> > > > > > +
> > > > > > +static struct fwnode_handle *mv88e6xxx_create_port_swnode(phy_interface_t mode,
> > > > > > +							  int speed,
> > > > > > +							  int duplex)
> > > > > > +{
> > > > > > +	struct property_entry port_props[2] = {};
> > > > > > +	struct fwnode_handle *fixed_link_fwnode;
> > > > > > +	struct fwnode_handle *new_port_fwnode;
> > > > > > +
> > > > > > +	port_props[0] = PROPERTY_ENTRY_STRING("phy-mode", phy_modes(mode));
> > > > > > +	new_port_fwnode = fwnode_create_software_node(port_props, NULL);
> > > > > > +	if (IS_ERR(new_port_fwnode))
> > > > > > +		return new_port_fwnode;
> > > > > > +
> > > > > > +	fixed_link_fwnode = mv88e6xxx_create_fixed_swnode(new_port_fwnode,
> > > > > > +							  speed, duplex);
> > > > > > +	if (IS_ERR(fixed_link_fwnode)) {
> > > > > > +		fwnode_remove_software_node(new_port_fwnode);
> > > > > > +		return fixed_link_fwnode;
> > > > > > +	}
> > > > > > +
> > > > > > +	return new_port_fwnode;
> > > > > > +}
> > > > > 
> > > > > That new fwnode_create_named_software_node() function looks like a
> > > > > conflict waiting to happen - if a driver adds a node to the root level
> > > > > (does not have to be root level), all the tests will pass because
> > > > > there is only a single device, but when a user later tries the driver
> > > > > with two devices, it fails, because the node already exist. But you
> > > > > don't need that function at all.
> > > > 
> > > > I think you're totally failing to explain how this can fail.
> > > > 
> > > > Let me reiterate what thestructure of the swnodes here is:
> > > > 
> > > > 	root
> > > > 	`- node%d (%d allocated by root IDA)
> > > > 	   +- phy-mode property
> > > > 	   `- fixed-link
> > > > 	      +- speed property
> > > > 	      `- optional full-duplex property
> > > > 
> > > > If we have two different devices creating these nodes, then at the
> > > > root level, they will end up having different root names. The
> > > > "fixed-link" is a child of this node.
> > > 
> > > Ah, sorry, the problem is not with this patch, or your use case. The
> > > problem is with the PATCH 1/7 of this series where you introduce that
> > > new function fwnode_create_named_software_node() which will not be
> > > tied to your use case only. In this patch you just use that function.
> > > I should have been more clear on that.
> > 
> > How is this any different from creating two struct device's with the
> > same parent and the same name? Or kobject_add() with the same parent
> > and name?
> 
> But that can not mean we have to take the same risk everywhere. I do
> understand that we don't protect developers from doing silly decisions
> in kernel, but that does not mean that we should simply accept
> interfaces into the kernel that expose these risk if we don't need
> them.
> 
> > > I really just wanted to show how you can create those nodes by using
> > > the API designed for the statically described software nodes. So you
> > > don't need that new function. Please check that proposal from my
> > > original reply.
> > 
> > I don't see why I should. This is clearly a case that if one creates
> > two named nodes with the same name and same parent, it should fail and
> > it's definitely a "well don't do that then" in just the same way that
> > one doesn't do it with kobject_add() or any of the other numerous
> > interfaces that take names in a space that need to be unique.
> > 
> > I really don't think there is any issue here to be solved. In fact,
> > I think solving it will add additional useless complexity that just
> > isn't required - which adds extra code that can be wrong and fail.
> > 
> > Let's keep this simple. This approach is simple. If one does something
> > stupid (like creating two named nodes with the same name and same
> > parent) then it will verbosely fail. That is a good thing.
> 
> Well, I think the most simplest approach would be to have both the
> nodes and the properties as part of that struct mv88e6xxx_chip:
> 
> struct mv88e6xxx_chip {
>         ...
>        struct property_entry port_props[2];
>        struct property_entry fixed_link_props[3];
> 
>        struct software_node port_swnode;
>        struct software_node fixed_link_swnode;
> };
> 
> That allows you to register both nodes in one go:
> 
> static struct fwnode_handle *mv88e6xxx_create_port_swnode(struct mv88e6xxx_chip *chip,
>                                                           phy_interface_t mode,
>                                                           int speed,
>                                                           int duplex)
> {
>         struct software_node *nodes[3] = {
>                 &chip->port_swnode,
>                 &chip->fixed_link_swnode,
>         };
>         int ret;
> 
>         chip->port_props[0] = PROPERTY_ENTRY_STRING("phy-mode", phy_modes(mode));
>         chip->port_swnode.properties = chip->port_props;
> 
>         chip->fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed);
>         if (duplex == DUPLEX_FULL)
>                 chip->fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex");
> 
>         chip->fixed_link_swnode.name = "fixed-link";
>         chip->fixed_link_swnode.parent = &chip->port_swnode;
>         chip->fixed_link_swnode.properties = chip->fixed_link_props;
> 
>         ret = software_node_register_node_group(nodes);
>         if (ret)
>                 return ERR_PTR(ret);
> 
>         return software_node_fwnode(&chip->port_swnode);
> }

You're suggesting code that passes a fwnode pointer back up layers
that has been allocated in the driver's private structure, assuming
that those upper layers are going to release this before re-calling
this function for a different port. They do today, but in the future?

There are always plenty of guns...

If we don't want to give the monkey the gun, we need a more complex
solution than that... and it becomes a question about how far you
want to take gun control.

Then there's the question about why we should have this data allocated
permanently in the system when it is only used for a very short period
during driver initialisation. That seems to be a complete waste of
resources.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Russell King (Oracle)]

On Mon, Mar 27, 2023 at 03:32:46PM +0100, Russell King (Oracle) wrote:
> On Mon, Mar 27, 2023 at 05:13:25PM +0300, Heikki Krogerus wrote:
> > Hi Russell,
> > 
> > On Mon, Mar 27, 2023 at 11:55:00AM +0100, Russell King (Oracle) wrote:
> > > On Mon, Mar 27, 2023 at 01:28:06PM +0300, Heikki Krogerus wrote:
> > > > On Fri, Mar 24, 2023 at 05:04:25PM +0000, Russell King (Oracle) wrote:
> > > > > On Fri, Mar 24, 2023 at 04:49:32PM +0200, Heikki Krogerus wrote:
> > > > > > Hi Russell,
> > > > > > 
> > > > > > On Wed, Mar 22, 2023 at 12:00:21PM +0000, Russell King (Oracle) wrote:
> > > > > > > +static struct fwnode_handle *mv88e6xxx_create_fixed_swnode(struct fwnode_handle *parent,
> > > > > > > +							   int speed,
> > > > > > > +							   int duplex)
> > > > > > > +{
> > > > > > > +	struct property_entry fixed_link_props[3] = { };
> > > > > > > +
> > > > > > > +	fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed);
> > > > > > > +	if (duplex == DUPLEX_FULL)
> > > > > > > +		fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex");
> > > > > > > +
> > > > > > > +	return fwnode_create_named_software_node(fixed_link_props, parent,
> > > > > > > +						 "fixed-link");
> > > > > > > +}
> > > > > > > +
> > > > > > > +static struct fwnode_handle *mv88e6xxx_create_port_swnode(phy_interface_t mode,
> > > > > > > +							  int speed,
> > > > > > > +							  int duplex)
> > > > > > > +{
> > > > > > > +	struct property_entry port_props[2] = {};
> > > > > > > +	struct fwnode_handle *fixed_link_fwnode;
> > > > > > > +	struct fwnode_handle *new_port_fwnode;
> > > > > > > +
> > > > > > > +	port_props[0] = PROPERTY_ENTRY_STRING("phy-mode", phy_modes(mode));
> > > > > > > +	new_port_fwnode = fwnode_create_software_node(port_props, NULL);
> > > > > > > +	if (IS_ERR(new_port_fwnode))
> > > > > > > +		return new_port_fwnode;
> > > > > > > +
> > > > > > > +	fixed_link_fwnode = mv88e6xxx_create_fixed_swnode(new_port_fwnode,
> > > > > > > +							  speed, duplex);
> > > > > > > +	if (IS_ERR(fixed_link_fwnode)) {
> > > > > > > +		fwnode_remove_software_node(new_port_fwnode);
> > > > > > > +		return fixed_link_fwnode;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return new_port_fwnode;
> > > > > > > +}
> > > > > > 
> > > > > > That new fwnode_create_named_software_node() function looks like a
> > > > > > conflict waiting to happen - if a driver adds a node to the root level
> > > > > > (does not have to be root level), all the tests will pass because
> > > > > > there is only a single device, but when a user later tries the driver
> > > > > > with two devices, it fails, because the node already exist. But you
> > > > > > don't need that function at all.
> > > > > 
> > > > > I think you're totally failing to explain how this can fail.
> > > > > 
> > > > > Let me reiterate what thestructure of the swnodes here is:
> > > > > 
> > > > > 	root
> > > > > 	`- node%d (%d allocated by root IDA)
> > > > > 	   +- phy-mode property
> > > > > 	   `- fixed-link
> > > > > 	      +- speed property
> > > > > 	      `- optional full-duplex property
> > > > > 
> > > > > If we have two different devices creating these nodes, then at the
> > > > > root level, they will end up having different root names. The
> > > > > "fixed-link" is a child of this node.
> > > > 
> > > > Ah, sorry, the problem is not with this patch, or your use case. The
> > > > problem is with the PATCH 1/7 of this series where you introduce that
> > > > new function fwnode_create_named_software_node() which will not be
> > > > tied to your use case only. In this patch you just use that function.
> > > > I should have been more clear on that.
> > > 
> > > How is this any different from creating two struct device's with the
> > > same parent and the same name? Or kobject_add() with the same parent
> > > and name?
> > 
> > But that can not mean we have to take the same risk everywhere. I do
> > understand that we don't protect developers from doing silly decisions
> > in kernel, but that does not mean that we should simply accept
> > interfaces into the kernel that expose these risk if we don't need
> > them.
> > 
> > > > I really just wanted to show how you can create those nodes by using
> > > > the API designed for the statically described software nodes. So you
> > > > don't need that new function. Please check that proposal from my
> > > > original reply.
> > > 
> > > I don't see why I should. This is clearly a case that if one creates
> > > two named nodes with the same name and same parent, it should fail and
> > > it's definitely a "well don't do that then" in just the same way that
> > > one doesn't do it with kobject_add() or any of the other numerous
> > > interfaces that take names in a space that need to be unique.
> > > 
> > > I really don't think there is any issue here to be solved. In fact,
> > > I think solving it will add additional useless complexity that just
> > > isn't required - which adds extra code that can be wrong and fail.
> > > 
> > > Let's keep this simple. This approach is simple. If one does something
> > > stupid (like creating two named nodes with the same name and same
> > > parent) then it will verbosely fail. That is a good thing.
> > 
> > Well, I think the most simplest approach would be to have both the
> > nodes and the properties as part of that struct mv88e6xxx_chip:
> > 
> > struct mv88e6xxx_chip {
> >         ...
> >        struct property_entry port_props[2];
> >        struct property_entry fixed_link_props[3];
> > 
> >        struct software_node port_swnode;
> >        struct software_node fixed_link_swnode;
> > };
> > 
> > That allows you to register both nodes in one go:
> > 
> > static struct fwnode_handle *mv88e6xxx_create_port_swnode(struct mv88e6xxx_chip *chip,
> >                                                           phy_interface_t mode,
> >                                                           int speed,
> >                                                           int duplex)
> > {
> >         struct software_node *nodes[3] = {
> >                 &chip->port_swnode,
> >                 &chip->fixed_link_swnode,
> >         };
> >         int ret;
> > 
> >         chip->port_props[0] = PROPERTY_ENTRY_STRING("phy-mode", phy_modes(mode));
> >         chip->port_swnode.properties = chip->port_props;
> > 
> >         chip->fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed);
> >         if (duplex == DUPLEX_FULL)
> >                 chip->fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex");
> > 
> >         chip->fixed_link_swnode.name = "fixed-link";
> >         chip->fixed_link_swnode.parent = &chip->port_swnode;
> >         chip->fixed_link_swnode.properties = chip->fixed_link_props;
> > 
> >         ret = software_node_register_node_group(nodes);
> >         if (ret)
> >                 return ERR_PTR(ret);
> > 
> >         return software_node_fwnode(&chip->port_swnode);
> > }
> 
> You're suggesting code that passes a fwnode pointer back up layers
> that has been allocated in the driver's private structure, assuming
> that those upper layers are going to release this before re-calling
> this function for a different port. They do today, but in the future?
> 
> There are always plenty of guns...
> 
> If we don't want to give the monkey the gun, we need a more complex
> solution than that... and it becomes a question about how far you
> want to take gun control.
> 
> Then there's the question about why we should have this data allocated
> permanently in the system when it is only used for a very short period
> during driver initialisation. That seems to be a complete waste of
> resources.

Also, given that the data structures get re-used, your above example
code is actually buggy - so you seem to have taken the gun and shot
yourself! Why is it buggy?

If on the first call to it, duplex is DUPLEX_FULL, then we set
fixed_link_props[1] to point at the full-duplex property. On the next
call, if we pass DUPLEX_HALF, then fixed_link_props[1] is left#
untouched and will still point at the full-duplex property.

This is a great illustration why trying to remove one gun from
circulation results in other more subtle guns being manufactured.

I'm in favour of simple obviously correct code, which is what my
proposal is. If someone does something stupid with it such as
creating two swnodes with the same name, that isn't a problem -
kobject (and sysfs) will detect the error, print a warning and
return an error - resulting in a graceful cleanup. It won't crash
the kernel.

I think you're making a mountain out of a mole hill over the "someone
might use fwnode_create_named_software_node() to create two nodes with
the same name under the same parent" issue. At least to me, it's
really not an issue that should have been raised, and I am firmly
of the opinion that this is a total non-issue.

I'm also of the opinion that trying to "fix" this non-issue creates
more problems than it solves.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Heikki Krogerus]

On Mon, Mar 27, 2023 at 04:45:19PM +0100, Russell King (Oracle) wrote:
> On Mon, Mar 27, 2023 at 03:32:46PM +0100, Russell King (Oracle) wrote:
> > On Mon, Mar 27, 2023 at 05:13:25PM +0300, Heikki Krogerus wrote:
> > > Hi Russell,
> > > 
> > > On Mon, Mar 27, 2023 at 11:55:00AM +0100, Russell King (Oracle) wrote:
> > > > On Mon, Mar 27, 2023 at 01:28:06PM +0300, Heikki Krogerus wrote:
> > > > > On Fri, Mar 24, 2023 at 05:04:25PM +0000, Russell King (Oracle) wrote:
> > > > > > On Fri, Mar 24, 2023 at 04:49:32PM +0200, Heikki Krogerus wrote:
> > > > > > > Hi Russell,
> > > > > > > 
> > > > > > > On Wed, Mar 22, 2023 at 12:00:21PM +0000, Russell King (Oracle) wrote:
> > > > > > > > +static struct fwnode_handle *mv88e6xxx_create_fixed_swnode(struct fwnode_handle *parent,
> > > > > > > > +							   int speed,
> > > > > > > > +							   int duplex)
> > > > > > > > +{
> > > > > > > > +	struct property_entry fixed_link_props[3] = { };
> > > > > > > > +
> > > > > > > > +	fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed);
> > > > > > > > +	if (duplex == DUPLEX_FULL)
> > > > > > > > +		fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex");
> > > > > > > > +
> > > > > > > > +	return fwnode_create_named_software_node(fixed_link_props, parent,
> > > > > > > > +						 "fixed-link");
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static struct fwnode_handle *mv88e6xxx_create_port_swnode(phy_interface_t mode,
> > > > > > > > +							  int speed,
> > > > > > > > +							  int duplex)
> > > > > > > > +{
> > > > > > > > +	struct property_entry port_props[2] = {};
> > > > > > > > +	struct fwnode_handle *fixed_link_fwnode;
> > > > > > > > +	struct fwnode_handle *new_port_fwnode;
> > > > > > > > +
> > > > > > > > +	port_props[0] = PROPERTY_ENTRY_STRING("phy-mode", phy_modes(mode));
> > > > > > > > +	new_port_fwnode = fwnode_create_software_node(port_props, NULL);
> > > > > > > > +	if (IS_ERR(new_port_fwnode))
> > > > > > > > +		return new_port_fwnode;
> > > > > > > > +
> > > > > > > > +	fixed_link_fwnode = mv88e6xxx_create_fixed_swnode(new_port_fwnode,
> > > > > > > > +							  speed, duplex);
> > > > > > > > +	if (IS_ERR(fixed_link_fwnode)) {
> > > > > > > > +		fwnode_remove_software_node(new_port_fwnode);
> > > > > > > > +		return fixed_link_fwnode;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	return new_port_fwnode;
> > > > > > > > +}
> > > > > > > 
> > > > > > > That new fwnode_create_named_software_node() function looks like a
> > > > > > > conflict waiting to happen - if a driver adds a node to the root level
> > > > > > > (does not have to be root level), all the tests will pass because
> > > > > > > there is only a single device, but when a user later tries the driver
> > > > > > > with two devices, it fails, because the node already exist. But you
> > > > > > > don't need that function at all.
> > > > > > 
> > > > > > I think you're totally failing to explain how this can fail.
> > > > > > 
> > > > > > Let me reiterate what thestructure of the swnodes here is:
> > > > > > 
> > > > > > 	root
> > > > > > 	`- node%d (%d allocated by root IDA)
> > > > > > 	   +- phy-mode property
> > > > > > 	   `- fixed-link
> > > > > > 	      +- speed property
> > > > > > 	      `- optional full-duplex property
> > > > > > 
> > > > > > If we have two different devices creating these nodes, then at the
> > > > > > root level, they will end up having different root names. The
> > > > > > "fixed-link" is a child of this node.
> > > > > 
> > > > > Ah, sorry, the problem is not with this patch, or your use case. The
> > > > > problem is with the PATCH 1/7 of this series where you introduce that
> > > > > new function fwnode_create_named_software_node() which will not be
> > > > > tied to your use case only. In this patch you just use that function.
> > > > > I should have been more clear on that.
> > > > 
> > > > How is this any different from creating two struct device's with the
> > > > same parent and the same name? Or kobject_add() with the same parent
> > > > and name?
> > > 
> > > But that can not mean we have to take the same risk everywhere. I do
> > > understand that we don't protect developers from doing silly decisions
> > > in kernel, but that does not mean that we should simply accept
> > > interfaces into the kernel that expose these risk if we don't need
> > > them.
> > > 
> > > > > I really just wanted to show how you can create those nodes by using
> > > > > the API designed for the statically described software nodes. So you
> > > > > don't need that new function. Please check that proposal from my
> > > > > original reply.
> > > > 
> > > > I don't see why I should. This is clearly a case that if one creates
> > > > two named nodes with the same name and same parent, it should fail and
> > > > it's definitely a "well don't do that then" in just the same way that
> > > > one doesn't do it with kobject_add() or any of the other numerous
> > > > interfaces that take names in a space that need to be unique.
> > > > 
> > > > I really don't think there is any issue here to be solved. In fact,
> > > > I think solving it will add additional useless complexity that just
> > > > isn't required - which adds extra code that can be wrong and fail.
> > > > 
> > > > Let's keep this simple. This approach is simple. If one does something
> > > > stupid (like creating two named nodes with the same name and same
> > > > parent) then it will verbosely fail. That is a good thing.
> > > 
> > > Well, I think the most simplest approach would be to have both the
> > > nodes and the properties as part of that struct mv88e6xxx_chip:
> > > 
> > > struct mv88e6xxx_chip {
> > >         ...
> > >        struct property_entry port_props[2];
> > >        struct property_entry fixed_link_props[3];
> > > 
> > >        struct software_node port_swnode;
> > >        struct software_node fixed_link_swnode;
> > > };
> > > 
> > > That allows you to register both nodes in one go:
> > > 
> > > static struct fwnode_handle *mv88e6xxx_create_port_swnode(struct mv88e6xxx_chip *chip,
> > >                                                           phy_interface_t mode,
> > >                                                           int speed,
> > >                                                           int duplex)
> > > {
> > >         struct software_node *nodes[3] = {
> > >                 &chip->port_swnode,
> > >                 &chip->fixed_link_swnode,
> > >         };
> > >         int ret;
> > > 
> > >         chip->port_props[0] = PROPERTY_ENTRY_STRING("phy-mode", phy_modes(mode));
> > >         chip->port_swnode.properties = chip->port_props;
> > > 
> > >         chip->fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed);
> > >         if (duplex == DUPLEX_FULL)
> > >                 chip->fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex");
> > > 
> > >         chip->fixed_link_swnode.name = "fixed-link";
> > >         chip->fixed_link_swnode.parent = &chip->port_swnode;
> > >         chip->fixed_link_swnode.properties = chip->fixed_link_props;
> > > 
> > >         ret = software_node_register_node_group(nodes);
> > >         if (ret)
> > >                 return ERR_PTR(ret);
> > > 
> > >         return software_node_fwnode(&chip->port_swnode);
> > > }
> > 
> > You're suggesting code that passes a fwnode pointer back up layers
> > that has been allocated in the driver's private structure, assuming
> > that those upper layers are going to release this before re-calling
> > this function for a different port. They do today, but in the future?
> > 
> > There are always plenty of guns...
> > 
> > If we don't want to give the monkey the gun, we need a more complex
> > solution than that... and it becomes a question about how far you
> > want to take gun control.
> > 
> > Then there's the question about why we should have this data allocated
> > permanently in the system when it is only used for a very short period
> > during driver initialisation. That seems to be a complete waste of
> > resources.
> 
> Also, given that the data structures get re-used, your above example
> code is actually buggy - so you seem to have taken the gun and shot
> yourself! Why is it buggy?

> If on the first call to it, duplex is DUPLEX_FULL, then we set
> fixed_link_props[1] to point at the full-duplex property. On the next
> call, if we pass DUPLEX_HALF, then fixed_link_props[1] is left#
> untouched and will still point at the full-duplex property.

I have not shared a patch with you, I've only shared code snippet with
you - an example like you said. The only purpose of it is to give you
a rough idea how you can use the API. Please note that I did also
explain you that if you need to allocate those structures then you
just go ahead allocate them (see my original reply). I'm not giving
you the final solution, only the correct approach in a form of a
sketch.

The way you adapt that into this driver is not up to me, it's
something you need to do.

> This is a great illustration why trying to remove one gun from
> circulation results in other more subtle guns being manufactured.
> 
> I'm in favour of simple obviously correct code, which is what my
> proposal is. If someone does something stupid with it such as
> creating two swnodes with the same name, that isn't a problem -
> kobject (and sysfs) will detect the error, print a warning and
> return an error - resulting in a graceful cleanup. It won't crash
> the kernel.

The problem is that the function you are proposing will be exploited
silently - people will use NULL as the parent without anybody
noticing. Everything will work for a while, because everybody will
first only have a single device for that driver. But as time goes by
and new hardware appears, suddenly there are multiple devices for
those drivers, and the conflict start to appear.

At that point the changes that added the function call will have
trickled down to the stable trees, so the distros are affected. Now we
are no longer talking about a simple cleanup that fixes the issue. In
the unlikely, but possible case, this will turn into ABI problem if
there is something in user space that for whatever reason now expects
the node to be always accessible at the same level and with the same
name.

As you pointed out, this kind of risks we have to live with kbojects,
struct device stuff and many others, but the thing is, with the
software node and device property APIs right now we don't. So the fact
that a risk exists in one place just isn't justification to accept the
same risk absolutely everywhere.

> I think you're making a mountain out of a mole hill over the "someone
> might use fwnode_create_named_software_node() to create two nodes with
> the same name under the same parent" issue. At least to me, it's
> really not an issue that should have been raised, and I am firmly
> of the opinion that this is a total non-issue.
> 
> I'm also of the opinion that trying to "fix" this non-issue creates
> more problems than it solves.

Russell, if you have some good arguments for accepting your proposal,
I assure you I will agree with you, but so far all you have given are
attacks on a sketch details and statements like that "I think you're
making a mountain out of a mole". Those just are not good enough.

I've explained, repeatedly, the risk involved with your proposal. Your
counter arguments to that has been that the same risk exists
elsewhere, which does not matter like I explained above, and basically
that we can live with the risk, which really should not be acceptable
answer in general, but in this case since we have an alternative
approach it is definitely not an argument.

Your claim that using the existing API adds complexity is also
complete bogus, because the changes you will need to do to this patch
(this driver) are going to very small - the code will not look that
different. Just try the existing API.

So please note that I'm not trying to "fix" anything here - I don't
have to. Right now it seems all I'm doing is preventing useless
function from being added to the kernel.

thanks,

-- 
heikki

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Russell King (Oracle)]

On Tue, Mar 28, 2023 at 03:09:56PM +0300, Heikki Krogerus wrote:
> The problem is that the function you are proposing will be exploited
> silently - people will use NULL as the parent without anybody
> noticing. Everything will work for a while, because everybody will
> first only have a single device for that driver. But as time goes by
> and new hardware appears, suddenly there are multiple devices for
> those drivers, and the conflict start to appear.

So, an easy solution would be to reject a call to
fwnode_create_named_software_node() when parent is NULL, thereby
preventing named nodes at the root level.

> At that point the changes that added the function call will have
> trickled down to the stable trees, so the distros are affected. Now we
> are no longer talking about a simple cleanup that fixes the issue. In
> the unlikely, but possible case, this will turn into ABI problem if

There is no such thing as stable APIs for internal kernel interfaces.

Documentation/process/stable-api-nonsense.rst

> As you pointed out, this kind of risks we have to live with kbojects,
> struct device stuff and many others, but the thing is, with the
> software node and device property APIs right now we don't. So the fact
> that a risk exists in one place just isn't justification to accept the
> same risk absolutely everywhere.

Meanwhile, firmware descriptions explicitly permit looking up nodes by
their names, but here we are, with the software node maintainers
basically stating that they don't wish to support creating software
nodes with explicit names.

> Russell, if you have some good arguments for accepting your proposal,
> I assure you I will agree with you, but so far all you have given are
> attacks on a sketch details and statements like that "I think you're
> making a mountain out of a mole". Those just are not good enough.

Basically, I think you are outright wrong for all the reasons I have
given in all my emails on this subject.

Yes, I accept there is a *slight* risk of abuse, but I see it as no
different from the risk from incorrect usage of any other kernel
internal interface. Therefore I just do not accept your argument
that we should not have this function, and I do not accept your
reasoning.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Heikki Krogerus]

On Tue, Mar 28, 2023 at 02:23:41PM +0100, Russell King (Oracle) wrote:
> On Tue, Mar 28, 2023 at 03:09:56PM +0300, Heikki Krogerus wrote:
> > The problem is that the function you are proposing will be exploited
> > silently - people will use NULL as the parent without anybody
> > noticing. Everything will work for a while, because everybody will
> > first only have a single device for that driver. But as time goes by
> > and new hardware appears, suddenly there are multiple devices for
> > those drivers, and the conflict start to appear.
> 
> So, an easy solution would be to reject a call to
> fwnode_create_named_software_node() when parent is NULL, thereby
> preventing named nodes at the root level.
> 
> > At that point the changes that added the function call will have
> > trickled down to the stable trees, so the distros are affected. Now we
> > are no longer talking about a simple cleanup that fixes the issue. In
> > the unlikely, but possible case, this will turn into ABI problem if
> 
> There is no such thing as stable APIs for internal kernel interfaces.
> 
> Documentation/process/stable-api-nonsense.rst
> 
> > As you pointed out, this kind of risks we have to live with kbojects,
> > struct device stuff and many others, but the thing is, with the
> > software node and device property APIs right now we don't. So the fact
> > that a risk exists in one place just isn't justification to accept the
> > same risk absolutely everywhere.
> 
> Meanwhile, firmware descriptions explicitly permit looking up nodes by
> their names, but here we are, with the software node maintainers
> basically stating that they don't wish to support creating software
> nodes with explicit names.

If you want to name the nodes then you just go ahead and name them,
nobody is preventing you and you can already do that, but if you do
so, then you will take full responsibility of the entire software node
- that is what you are naming here - instead of just the fwnode that
it contains. The users of the node can deal with the fwnode alone, but
you as the creator of the software node have to take proper ownership
of it.

> > Russell, if you have some good arguments for accepting your proposal,
> > I assure you I will agree with you, but so far all you have given are
> > attacks on a sketch details and statements like that "I think you're
> > making a mountain out of a mole". Those just are not good enough.
> 
> Basically, I think you are outright wrong for all the reasons I have
> given in all my emails on this subject.
> 
> Yes, I accept there is a *slight* risk of abuse, but I see it as no
> different from the risk from incorrect usage of any other kernel
> internal interface. Therefore I just do not accept your argument
> that we should not have this function, and I do not accept your
> reasoning.

I would not be so against the function if there wasn't any other way
to handle your case, but there is.

You really can not claim that the existing API is in any way inferior,
or even more complex, compared to your function before you actually
try it. You simply can not make judgement based on a sketch that is
basically just showing you the functions and structures that you need.

If there are issues with the API, then we need to of course fix those
issues, but please keep in mind that still does not mean we have any
need for the function you are proposing.

Please also note that helpers are welcome if you feel we need them. If
you want to add for example an allocation routine that duplicates also
the properties in one go, then that alone would reduce the complexity
needed in the drivers that create the nodes. I think in most cases,
possibly also in yours, that alone would allow most stuff to be
handled from stack memory.

fwnode_create_software_node() is there just to support the legacy
device properties. You really should not be using even that. If you
need to deal with software nodes then you deal with them with struct
software_node.

thanks,

-- 
heikki

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Russell King (Oracle)]

On Wed, Mar 29, 2023 at 05:07:26PM +0300, Heikki Krogerus wrote:
> On Tue, Mar 28, 2023 at 02:23:41PM +0100, Russell King (Oracle) wrote:
> > On Tue, Mar 28, 2023 at 03:09:56PM +0300, Heikki Krogerus wrote:
> > > The problem is that the function you are proposing will be exploited
> > > silently - people will use NULL as the parent without anybody
> > > noticing. Everything will work for a while, because everybody will
> > > first only have a single device for that driver. But as time goes by
> > > and new hardware appears, suddenly there are multiple devices for
> > > those drivers, and the conflict start to appear.
> > 
> > So, an easy solution would be to reject a call to
> > fwnode_create_named_software_node() when parent is NULL, thereby
> > preventing named nodes at the root level.
> > 
> > > At that point the changes that added the function call will have
> > > trickled down to the stable trees, so the distros are affected. Now we
> > > are no longer talking about a simple cleanup that fixes the issue. In
> > > the unlikely, but possible case, this will turn into ABI problem if
> > 
> > There is no such thing as stable APIs for internal kernel interfaces.
> > 
> > Documentation/process/stable-api-nonsense.rst
> > 
> > > As you pointed out, this kind of risks we have to live with kbojects,
> > > struct device stuff and many others, but the thing is, with the
> > > software node and device property APIs right now we don't. So the fact
> > > that a risk exists in one place just isn't justification to accept the
> > > same risk absolutely everywhere.
> > 
> > Meanwhile, firmware descriptions explicitly permit looking up nodes by
> > their names, but here we are, with the software node maintainers
> > basically stating that they don't wish to support creating software
> > nodes with explicit names.
> 
> If you want to name the nodes then you just go ahead and name them,
> nobody is preventing you and you can already do that, but if you do
> so, then you will take full responsibility of the entire software node
> - that is what you are naming here - instead of just the fwnode that
> it contains. The users of the node can deal with the fwnode alone, but
> you as the creator of the software node have to take proper ownership
> of it.
> 
> > > Russell, if you have some good arguments for accepting your proposal,
> > > I assure you I will agree with you, but so far all you have given are
> > > attacks on a sketch details and statements like that "I think you're
> > > making a mountain out of a mole". Those just are not good enough.
> > 
> > Basically, I think you are outright wrong for all the reasons I have
> > given in all my emails on this subject.
> > 
> > Yes, I accept there is a *slight* risk of abuse, but I see it as no
> > different from the risk from incorrect usage of any other kernel
> > internal interface. Therefore I just do not accept your argument
> > that we should not have this function, and I do not accept your
> > reasoning.
> 
> I would not be so against the function if there wasn't any other way
> to handle your case, but there is.
> 
> You really can not claim that the existing API is in any way inferior,
> or even more complex, compared to your function before you actually
> try it. You simply can not make judgement based on a sketch that is
> basically just showing you the functions and structures that you need.
> 
> If there are issues with the API, then we need to of course fix those
> issues, but please keep in mind that still does not mean we have any
> need for the function you are proposing.
> 
> Please also note that helpers are welcome if you feel we need them. If
> you want to add for example an allocation routine that duplicates also
> the properties in one go, then that alone would reduce the complexity
> needed in the drivers that create the nodes. I think in most cases,
> possibly also in yours, that alone would allow most stuff to be
> handled from stack memory.
> 
> fwnode_create_software_node() is there just to support the legacy
> device properties. You really should not be using even that. If you
> need to deal with software nodes then you deal with them with struct
> software_node.

You forgot to explain how to free them once they're done, because
struct swnode will contain a pointer to the struct software_node
which can be a dangling stale reference - and there's no way for
code outside swnode.c to know when that reference has gone.

That is another reason why I prefer my existing solution. That
problem is taken care of already by the existing code - and as
it's taken care of there, and properly, there's less possibilities
for users of swnode to get it wrong.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Heikki Krogerus]

On Wed, Mar 29, 2023 at 03:33:48PM +0100, Russell King (Oracle) wrote:
> On Wed, Mar 29, 2023 at 05:07:26PM +0300, Heikki Krogerus wrote:
> > On Tue, Mar 28, 2023 at 02:23:41PM +0100, Russell King (Oracle) wrote:
> > > On Tue, Mar 28, 2023 at 03:09:56PM +0300, Heikki Krogerus wrote:
> > > > The problem is that the function you are proposing will be exploited
> > > > silently - people will use NULL as the parent without anybody
> > > > noticing. Everything will work for a while, because everybody will
> > > > first only have a single device for that driver. But as time goes by
> > > > and new hardware appears, suddenly there are multiple devices for
> > > > those drivers, and the conflict start to appear.
> > > 
> > > So, an easy solution would be to reject a call to
> > > fwnode_create_named_software_node() when parent is NULL, thereby
> > > preventing named nodes at the root level.
> > > 
> > > > At that point the changes that added the function call will have
> > > > trickled down to the stable trees, so the distros are affected. Now we
> > > > are no longer talking about a simple cleanup that fixes the issue. In
> > > > the unlikely, but possible case, this will turn into ABI problem if
> > > 
> > > There is no such thing as stable APIs for internal kernel interfaces.
> > > 
> > > Documentation/process/stable-api-nonsense.rst
> > > 
> > > > As you pointed out, this kind of risks we have to live with kbojects,
> > > > struct device stuff and many others, but the thing is, with the
> > > > software node and device property APIs right now we don't. So the fact
> > > > that a risk exists in one place just isn't justification to accept the
> > > > same risk absolutely everywhere.
> > > 
> > > Meanwhile, firmware descriptions explicitly permit looking up nodes by
> > > their names, but here we are, with the software node maintainers
> > > basically stating that they don't wish to support creating software
> > > nodes with explicit names.
> > 
> > If you want to name the nodes then you just go ahead and name them,
> > nobody is preventing you and you can already do that, but if you do
> > so, then you will take full responsibility of the entire software node
> > - that is what you are naming here - instead of just the fwnode that
> > it contains. The users of the node can deal with the fwnode alone, but
> > you as the creator of the software node have to take proper ownership
> > of it.
> > 
> > > > Russell, if you have some good arguments for accepting your proposal,
> > > > I assure you I will agree with you, but so far all you have given are
> > > > attacks on a sketch details and statements like that "I think you're
> > > > making a mountain out of a mole". Those just are not good enough.
> > > 
> > > Basically, I think you are outright wrong for all the reasons I have
> > > given in all my emails on this subject.
> > > 
> > > Yes, I accept there is a *slight* risk of abuse, but I see it as no
> > > different from the risk from incorrect usage of any other kernel
> > > internal interface. Therefore I just do not accept your argument
> > > that we should not have this function, and I do not accept your
> > > reasoning.
> > 
> > I would not be so against the function if there wasn't any other way
> > to handle your case, but there is.
> > 
> > You really can not claim that the existing API is in any way inferior,
> > or even more complex, compared to your function before you actually
> > try it. You simply can not make judgement based on a sketch that is
> > basically just showing you the functions and structures that you need.
> > 
> > If there are issues with the API, then we need to of course fix those
> > issues, but please keep in mind that still does not mean we have any
> > need for the function you are proposing.
> > 
> > Please also note that helpers are welcome if you feel we need them. If
> > you want to add for example an allocation routine that duplicates also
> > the properties in one go, then that alone would reduce the complexity
> > needed in the drivers that create the nodes. I think in most cases,
> > possibly also in yours, that alone would allow most stuff to be
> > handled from stack memory.
> > 
> > fwnode_create_software_node() is there just to support the legacy
> > device properties. You really should not be using even that. If you
> > need to deal with software nodes then you deal with them with struct
> > software_node.
> 
> You forgot to explain how to free them once they're done, because
> struct swnode will contain a pointer to the struct software_node
> which can be a dangling stale reference - and there's no way for
> code outside swnode.c to know when that reference has gone.
> 
> That is another reason why I prefer my existing solution. That
> problem is taken care of already by the existing code - and as
> it's taken care of there, and properly, there's less possibilities
> for users of swnode to get it wrong.

We need an improved release mechanism, yes.

My idea with the new dynamic allocation routine was that it could be
introduced together with a release callback that we add to the struct
software_node.

The idea of adding the release callback to the structure was actually
considered already some time ago - I think it was discussed at least
shortly also on the public ACPI mailing list. The idea back then
included a default release function that simply frees the struct
software_node instance. That default release function we could then
assign to the release callback in that new software node
allocation/creation routine. That way the drivers should be able to
continue to rely on the underlying code to take care of freeing the
node instance.

Back then there was nobody who really needed that functionality, so
nobody even tried to implement it. Now we of course clearly do need
something like it.

I think the release callback together with the default release
function should work. Let me know what you guys think.

thanks,

-- 
heikki

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Russell King (Oracle)]

On Thu, Mar 30, 2023 at 04:54:51PM +0300, Heikki Krogerus wrote:
> On Wed, Mar 29, 2023 at 03:33:48PM +0100, Russell King (Oracle) wrote:
> > On Wed, Mar 29, 2023 at 05:07:26PM +0300, Heikki Krogerus wrote:
> > > On Tue, Mar 28, 2023 at 02:23:41PM +0100, Russell King (Oracle) wrote:
> > > > On Tue, Mar 28, 2023 at 03:09:56PM +0300, Heikki Krogerus wrote:
> > > > > The problem is that the function you are proposing will be exploited
> > > > > silently - people will use NULL as the parent without anybody
> > > > > noticing. Everything will work for a while, because everybody will
> > > > > first only have a single device for that driver. But as time goes by
> > > > > and new hardware appears, suddenly there are multiple devices for
> > > > > those drivers, and the conflict start to appear.
> > > > 
> > > > So, an easy solution would be to reject a call to
> > > > fwnode_create_named_software_node() when parent is NULL, thereby
> > > > preventing named nodes at the root level.
> > > > 
> > > > > At that point the changes that added the function call will have
> > > > > trickled down to the stable trees, so the distros are affected. Now we
> > > > > are no longer talking about a simple cleanup that fixes the issue. In
> > > > > the unlikely, but possible case, this will turn into ABI problem if
> > > > 
> > > > There is no such thing as stable APIs for internal kernel interfaces.
> > > > 
> > > > Documentation/process/stable-api-nonsense.rst
> > > > 
> > > > > As you pointed out, this kind of risks we have to live with kbojects,
> > > > > struct device stuff and many others, but the thing is, with the
> > > > > software node and device property APIs right now we don't. So the fact
> > > > > that a risk exists in one place just isn't justification to accept the
> > > > > same risk absolutely everywhere.
> > > > 
> > > > Meanwhile, firmware descriptions explicitly permit looking up nodes by
> > > > their names, but here we are, with the software node maintainers
> > > > basically stating that they don't wish to support creating software
> > > > nodes with explicit names.
> > > 
> > > If you want to name the nodes then you just go ahead and name them,
> > > nobody is preventing you and you can already do that, but if you do
> > > so, then you will take full responsibility of the entire software node
> > > - that is what you are naming here - instead of just the fwnode that
> > > it contains. The users of the node can deal with the fwnode alone, but
> > > you as the creator of the software node have to take proper ownership
> > > of it.
> > > 
> > > > > Russell, if you have some good arguments for accepting your proposal,
> > > > > I assure you I will agree with you, but so far all you have given are
> > > > > attacks on a sketch details and statements like that "I think you're
> > > > > making a mountain out of a mole". Those just are not good enough.
> > > > 
> > > > Basically, I think you are outright wrong for all the reasons I have
> > > > given in all my emails on this subject.
> > > > 
> > > > Yes, I accept there is a *slight* risk of abuse, but I see it as no
> > > > different from the risk from incorrect usage of any other kernel
> > > > internal interface. Therefore I just do not accept your argument
> > > > that we should not have this function, and I do not accept your
> > > > reasoning.
> > > 
> > > I would not be so against the function if there wasn't any other way
> > > to handle your case, but there is.
> > > 
> > > You really can not claim that the existing API is in any way inferior,
> > > or even more complex, compared to your function before you actually
> > > try it. You simply can not make judgement based on a sketch that is
> > > basically just showing you the functions and structures that you need.
> > > 
> > > If there are issues with the API, then we need to of course fix those
> > > issues, but please keep in mind that still does not mean we have any
> > > need for the function you are proposing.
> > > 
> > > Please also note that helpers are welcome if you feel we need them. If
> > > you want to add for example an allocation routine that duplicates also
> > > the properties in one go, then that alone would reduce the complexity
> > > needed in the drivers that create the nodes. I think in most cases,
> > > possibly also in yours, that alone would allow most stuff to be
> > > handled from stack memory.
> > > 
> > > fwnode_create_software_node() is there just to support the legacy
> > > device properties. You really should not be using even that. If you
> > > need to deal with software nodes then you deal with them with struct
> > > software_node.
> > 
> > You forgot to explain how to free them once they're done, because
> > struct swnode will contain a pointer to the struct software_node
> > which can be a dangling stale reference - and there's no way for
> > code outside swnode.c to know when that reference has gone.
> > 
> > That is another reason why I prefer my existing solution. That
> > problem is taken care of already by the existing code - and as
> > it's taken care of there, and properly, there's less possibilities
> > for users of swnode to get it wrong.
> 
> We need an improved release mechanism, yes.
> 
> My idea with the new dynamic allocation routine was that it could be
> introduced together with a release callback that we add to the struct
> software_node.
> 
> The idea of adding the release callback to the structure was actually
> considered already some time ago - I think it was discussed at least
> shortly also on the public ACPI mailing list. The idea back then
> included a default release function that simply frees the struct
> software_node instance. That default release function we could then
> assign to the release callback in that new software node
> allocation/creation routine. That way the drivers should be able to
> continue to rely on the underlying code to take care of freeing the
> node instance.
> 
> Back then there was nobody who really needed that functionality, so
> nobody even tried to implement it. Now we of course clearly do need
> something like it.
> 
> I think the release callback together with the default release
> function should work. Let me know what you guys think.

Thinking about this more, no. This is utterly vile, and I will not
create code that is vile.

Greg, please can you take a look at this, and give your opinion on
how named software nodes (which are required to describe things in
specific ways by firmware descriptions) should be handled? Is my
proposal reasonable in your eyes? Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software node for default settings

[Greg Kroah-Hartman]

On Mon, Apr 03, 2023 at 02:02:48PM +0100, Russell King (Oracle) wrote:
> On Thu, Mar 30, 2023 at 04:54:51PM +0300, Heikki Krogerus wrote:
> > On Wed, Mar 29, 2023 at 03:33:48PM +0100, Russell King (Oracle) wrote:
> > > On Wed, Mar 29, 2023 at 05:07:26PM +0300, Heikki Krogerus wrote:
> > > > On Tue, Mar 28, 2023 at 02:23:41PM +0100, Russell King (Oracle) wrote:
> > > > > On Tue, Mar 28, 2023 at 03:09:56PM +0300, Heikki Krogerus wrote:
> > > > > > The problem is that the function you are proposing will be exploited
> > > > > > silently - people will use NULL as the parent without anybody
> > > > > > noticing. Everything will work for a while, because everybody will
> > > > > > first only have a single device for that driver. But as time goes by
> > > > > > and new hardware appears, suddenly there are multiple devices for
> > > > > > those drivers, and the conflict start to appear.
> > > > > 
> > > > > So, an easy solution would be to reject a call to
> > > > > fwnode_create_named_software_node() when parent is NULL, thereby
> > > > > preventing named nodes at the root level.
> > > > > 
> > > > > > At that point the changes that added the function call will have
> > > > > > trickled down to the stable trees, so the distros are affected. Now we
> > > > > > are no longer talking about a simple cleanup that fixes the issue. In
> > > > > > the unlikely, but possible case, this will turn into ABI problem if
> > > > > 
> > > > > There is no such thing as stable APIs for internal kernel interfaces.
> > > > > 
> > > > > Documentation/process/stable-api-nonsense.rst
> > > > > 
> > > > > > As you pointed out, this kind of risks we have to live with kbojects,
> > > > > > struct device stuff and many others, but the thing is, with the
> > > > > > software node and device property APIs right now we don't. So the fact
> > > > > > that a risk exists in one place just isn't justification to accept the
> > > > > > same risk absolutely everywhere.
> > > > > 
> > > > > Meanwhile, firmware descriptions explicitly permit looking up nodes by
> > > > > their names, but here we are, with the software node maintainers
> > > > > basically stating that they don't wish to support creating software
> > > > > nodes with explicit names.
> > > > 
> > > > If you want to name the nodes then you just go ahead and name them,
> > > > nobody is preventing you and you can already do that, but if you do
> > > > so, then you will take full responsibility of the entire software node
> > > > - that is what you are naming here - instead of just the fwnode that
> > > > it contains. The users of the node can deal with the fwnode alone, but
> > > > you as the creator of the software node have to take proper ownership
> > > > of it.
> > > > 
> > > > > > Russell, if you have some good arguments for accepting your proposal,
> > > > > > I assure you I will agree with you, but so far all you have given are
> > > > > > attacks on a sketch details and statements like that "I think you're
> > > > > > making a mountain out of a mole". Those just are not good enough.
> > > > > 
> > > > > Basically, I think you are outright wrong for all the reasons I have
> > > > > given in all my emails on this subject.
> > > > > 
> > > > > Yes, I accept there is a *slight* risk of abuse, but I see it as no
> > > > > different from the risk from incorrect usage of any other kernel
> > > > > internal interface. Therefore I just do not accept your argument
> > > > > that we should not have this function, and I do not accept your
> > > > > reasoning.
> > > > 
> > > > I would not be so against the function if there wasn't any other way
> > > > to handle your case, but there is.
> > > > 
> > > > You really can not claim that the existing API is in any way inferior,
> > > > or even more complex, compared to your function before you actually
> > > > try it. You simply can not make judgement based on a sketch that is
> > > > basically just showing you the functions and structures that you need.
> > > > 
> > > > If there are issues with the API, then we need to of course fix those
> > > > issues, but please keep in mind that still does not mean we have any
> > > > need for the function you are proposing.
> > > > 
> > > > Please also note that helpers are welcome if you feel we need them. If
> > > > you want to add for example an allocation routine that duplicates also
> > > > the properties in one go, then that alone would reduce the complexity
> > > > needed in the drivers that create the nodes. I think in most cases,
> > > > possibly also in yours, that alone would allow most stuff to be
> > > > handled from stack memory.
> > > > 
> > > > fwnode_create_software_node() is there just to support the legacy
> > > > device properties. You really should not be using even that. If you
> > > > need to deal with software nodes then you deal with them with struct
> > > > software_node.
> > > 
> > > You forgot to explain how to free them once they're done, because
> > > struct swnode will contain a pointer to the struct software_node
> > > which can be a dangling stale reference - and there's no way for
> > > code outside swnode.c to know when that reference has gone.
> > > 
> > > That is another reason why I prefer my existing solution. That
> > > problem is taken care of already by the existing code - and as
> > > it's taken care of there, and properly, there's less possibilities
> > > for users of swnode to get it wrong.
> > 
> > We need an improved release mechanism, yes.
> > 
> > My idea with the new dynamic allocation routine was that it could be
> > introduced together with a release callback that we add to the struct
> > software_node.
> > 
> > The idea of adding the release callback to the structure was actually
> > considered already some time ago - I think it was discussed at least
> > shortly also on the public ACPI mailing list. The idea back then
> > included a default release function that simply frees the struct
> > software_node instance. That default release function we could then
> > assign to the release callback in that new software node
> > allocation/creation routine. That way the drivers should be able to
> > continue to rely on the underlying code to take care of freeing the
> > node instance.
> > 
> > Back then there was nobody who really needed that functionality, so
> > nobody even tried to implement it. Now we of course clearly do need
> > something like it.
> > 
> > I think the release callback together with the default release
> > function should work. Let me know what you guys think.
> 
> Thinking about this more, no. This is utterly vile, and I will not
> create code that is vile.
> 
> Greg, please can you take a look at this, and give your opinion on
> how named software nodes (which are required to describe things in
> specific ways by firmware descriptions) should be handled? Is my
> proposal reasonable in your eyes? Thanks.

I'm lost, sorry, this thread is crazy long and I do not have the
bandwidth to try to dig through it.  I think you two need to work it out
together please.

thanks,

greg k-h

> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!