[PATCH net-next 1/2] net: dsa: add a dsa_port_is_enabled helper function

[PATCH net-next 1/2] net: dsa: add a dsa_port_is_enabled helper function

[Vladimir Oltean]

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

Sometimes drivers need to do per-port operation outside the port DSA
methods, and in that case they typically iterate through their port list
themselves.

Give them an aid to skip ports that are disabled in the device tree
(which the DSA core already skips).

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/net/dsa.h |  2 ++
 net/dsa/dsa2.c    | 29 +++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index beeb81a532e3..813792e6f0be 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -376,6 +376,8 @@ static inline bool dsa_port_is_vlan_filtering(const struct dsa_port *dp)
 		return dp->vlan_filtering;
 }
 
+bool dsa_port_is_enabled(struct dsa_switch *ds, int port);
+
 typedef int dsa_fdb_dump_cb_t(const unsigned char *addr, u16 vid,
 			      bool is_static, void *data);
 struct dsa_switch_ops {
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index e7c30b472034..752f21273bd6 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -727,6 +727,35 @@ static int dsa_switch_parse_ports_of(struct dsa_switch *ds,
 	return err;
 }
 
+bool dsa_port_is_enabled(struct dsa_switch *ds, int port)
+{
+	struct device_node *dn = ds->dev->of_node;
+	struct device_node *ports, *port_node;
+	bool found = false;
+	int reg, err;
+
+	ports = of_get_child_by_name(dn, "ports");
+	if (!ports) {
+		dev_err(ds->dev, "no ports child node found\n");
+		return false;
+	}
+
+	for_each_available_child_of_node(ports, port_node) {
+		err = of_property_read_u32(port_node, "reg", &reg);
+		if (err)
+			goto out_put_node;
+
+		if (reg == port) {
+			found = true;
+			break;
+		}
+	}
+
+out_put_node:
+	of_node_put(ports);
+	return found;
+}
+
 static int dsa_switch_parse_member_of(struct dsa_switch *ds,
 				      struct device_node *dn)
 {
-- 
2.17.1

[PATCH net-next 2/2] net: dsa: sja1105: Avoid error message for unknown PHY mode on disabled ports

[Vladimir Oltean]

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

When sja1105_init_mii_settings iterates over the port list, it prints
this message for disabled ports, because they don't have a valid
phy-mode:

[    4.778702] sja1105 spi2.0: Unsupported PHY mode unknown!

Use the newly introduced dsa_port_is_enabled helper to avoid this.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index d8123288c572..8152b68389a7 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -162,6 +162,9 @@ static int sja1105_init_mii_settings(struct sja1105_private *priv,
 	mii = table->entries;
 
 	for (i = 0; i < SJA1105_NUM_PORTS; i++) {
+		if (!dsa_port_is_enabled(priv->ds, i))
+			continue;
+
 		switch (ports[i].phy_mode) {
 		case PHY_INTERFACE_MODE_MII:
 			mii->xmii_mode[i] = XMII_MODE_MII;
-- 
2.17.1

Re: [PATCH net-next 1/2] net: dsa: add a dsa_port_is_enabled helper function

[Andrew Lunn]

On Thu, Mar 19, 2020 at 08:56:19PM +0200, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Sometimes drivers need to do per-port operation outside the port DSA
> methods, and in that case they typically iterate through their port list
> themselves.
> 
> Give them an aid to skip ports that are disabled in the device tree
> (which the DSA core already skips).

Hi Vladimir

Why not dsa_is_unused_port()?

    Andrew

Re: [PATCH net-next 1/2] net: dsa: add a dsa_port_is_enabled helper function

[Vivien Didelot]

Hi Vladimir,

On Thu, 19 Mar 2020 20:56:19 +0200, Vladimir Oltean <olteanv@gmail.com> wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Sometimes drivers need to do per-port operation outside the port DSA
> methods, and in that case they typically iterate through their port list
> themselves.
> 
> Give them an aid to skip ports that are disabled in the device tree
> (which the DSA core already skips).
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  include/net/dsa.h |  2 ++
>  net/dsa/dsa2.c    | 29 +++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index beeb81a532e3..813792e6f0be 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -376,6 +376,8 @@ static inline bool dsa_port_is_vlan_filtering(const struct dsa_port *dp)
>  		return dp->vlan_filtering;
>  }
>  
> +bool dsa_port_is_enabled(struct dsa_switch *ds, int port);
> +
>  typedef int dsa_fdb_dump_cb_t(const unsigned char *addr, u16 vid,
>  			      bool is_static, void *data);
>  struct dsa_switch_ops {
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index e7c30b472034..752f21273bd6 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -727,6 +727,35 @@ static int dsa_switch_parse_ports_of(struct dsa_switch *ds,
>  	return err;
>  }
>  
> +bool dsa_port_is_enabled(struct dsa_switch *ds, int port)
> +{
> +	struct device_node *dn = ds->dev->of_node;
> +	struct device_node *ports, *port_node;
> +	bool found = false;
> +	int reg, err;
> +
> +	ports = of_get_child_by_name(dn, "ports");
> +	if (!ports) {
> +		dev_err(ds->dev, "no ports child node found\n");
> +		return false;
> +	}
> +
> +	for_each_available_child_of_node(ports, port_node) {
> +		err = of_property_read_u32(port_node, "reg", &reg);
> +		if (err)
> +			goto out_put_node;
> +
> +		if (reg == port) {
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +out_put_node:
> +	of_node_put(ports);
> +	return found;
> +}

Why do you need to iterate on the device tree? Ideally we could make
abstraction of that (basic platform data is supported too) and use pure DSA
helpers, given that DSA core has already peeled all that for us.

Your helper above doesn't even look at the port's state, only if it is declared
or not. Looking at patch 2, Using !dsa_is_unused_port(ds, port) seems enough.


Thanks,

	Vivien

Re: [PATCH net-next 1/2] net: dsa: add a dsa_port_is_enabled helper function

[Vladimir Oltean]

Hi Andrew, Vivien,

On Thu, 19 Mar 2020 at 21:17, Vivien Didelot <vivien.didelot@gmail.com> wrote:
>
> Hi Vladimir,
>
> On Thu, 19 Mar 2020 20:56:19 +0200, Vladimir Oltean <olteanv@gmail.com> wrote:
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> >
> > Sometimes drivers need to do per-port operation outside the port DSA
> > methods, and in that case they typically iterate through their port list
> > themselves.
> >
> > Give them an aid to skip ports that are disabled in the device tree
> > (which the DSA core already skips).
> >
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> >  include/net/dsa.h |  2 ++
> >  net/dsa/dsa2.c    | 29 +++++++++++++++++++++++++++++
> >  2 files changed, 31 insertions(+)
> >
> > diff --git a/include/net/dsa.h b/include/net/dsa.h
> > index beeb81a532e3..813792e6f0be 100644
> > --- a/include/net/dsa.h
> > +++ b/include/net/dsa.h
> > @@ -376,6 +376,8 @@ static inline bool dsa_port_is_vlan_filtering(const struct dsa_port *dp)
> >               return dp->vlan_filtering;
> >  }
> >
> > +bool dsa_port_is_enabled(struct dsa_switch *ds, int port);
> > +
> >  typedef int dsa_fdb_dump_cb_t(const unsigned char *addr, u16 vid,
> >                             bool is_static, void *data);
> >  struct dsa_switch_ops {
> > diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> > index e7c30b472034..752f21273bd6 100644
> > --- a/net/dsa/dsa2.c
> > +++ b/net/dsa/dsa2.c
> > @@ -727,6 +727,35 @@ static int dsa_switch_parse_ports_of(struct dsa_switch *ds,
> >       return err;
> >  }
> >
> > +bool dsa_port_is_enabled(struct dsa_switch *ds, int port)
> > +{
> > +     struct device_node *dn = ds->dev->of_node;
> > +     struct device_node *ports, *port_node;
> > +     bool found = false;
> > +     int reg, err;
> > +
> > +     ports = of_get_child_by_name(dn, "ports");
> > +     if (!ports) {
> > +             dev_err(ds->dev, "no ports child node found\n");
> > +             return false;
> > +     }
> > +
> > +     for_each_available_child_of_node(ports, port_node) {
> > +             err = of_property_read_u32(port_node, "reg", &reg);
> > +             if (err)
> > +                     goto out_put_node;
> > +
> > +             if (reg == port) {
> > +                     found = true;
> > +                     break;
> > +             }
> > +     }
> > +
> > +out_put_node:
> > +     of_node_put(ports);
> > +     return found;
> > +}
>
> Why do you need to iterate on the device tree? Ideally we could make
> abstraction of that (basic platform data is supported too) and use pure DSA
> helpers, given that DSA core has already peeled all that for us.
>
> Your helper above doesn't even look at the port's state, only if it is declared
> or not. Looking at patch 2, Using !dsa_is_unused_port(ds, port) seems enough.
>

Huh, this one was too smart for me.
I wrote the patch a while ago and did not really look deep enough to
spot this one. Actually I remember I did notice the
DSA_PORT_TYPE_UNUSED keyword, but I hadn't seen the ports being set
anywhere to that value. I realized just now that it's zero so they're
unused by default.
That's exactly why code review is amazing! Thank you, I've sent a v2
with your suggestion, it works a treat.

>
> Thanks,
>
>         Vivien

Thanks,
-Vladimir