[PATCH net-next 0/2] net: dsa: mv88e6xxx: LAG fixes

[PATCH net-next 0/2] net: dsa: mv88e6xxx: LAG fixes

[Tobias Waldekranz]

The kernel test robot kindly pointed out that Global 2 support in
mv88e6xxx is optional.

This also made me realize that we should verify that the driver and
hardware actually supports LAG offloading before trying to configure
it.

Tobias Waldekranz (2):
  net: dsa: mv88e6xxx: Provide dummy implementations for trunk setters
  net: dsa: mv88e6xxx: Only allow LAG offload on supported hardware

 drivers/net/dsa/mv88e6xxx/chip.c    |  4 ++++
 drivers/net/dsa/mv88e6xxx/chip.h    |  9 +++++++++
 drivers/net/dsa/mv88e6xxx/global2.h | 12 ++++++++++++
 3 files changed, 25 insertions(+)

-- 
2.17.1

[PATCH net-next 1/2] net: dsa: mv88e6xxx: Provide dummy implementations for trunk setters

[Tobias Waldekranz]

Support for Global 2 registers is build-time optional. In the case
where it was not enabled the build would fail as no "dummy"
implementation of these functions was available.

Fixes: 57e661aae6a8 ("net: dsa: mv88e6xxx: Link aggregation support")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/global2.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/global2.h b/drivers/net/dsa/mv88e6xxx/global2.h
index 60febaf4da76..253a79582a1d 100644
--- a/drivers/net/dsa/mv88e6xxx/global2.h
+++ b/drivers/net/dsa/mv88e6xxx/global2.h
@@ -525,6 +525,18 @@ static inline int mv88e6xxx_g2_trunk_clear(struct mv88e6xxx_chip *chip)
 	return -EOPNOTSUPP;
 }
 
+static inline int mv88e6xxx_g2_trunk_mask_write(struct mv88e6xxx_chip *chip,
+						int num, bool hash, u16 mask)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int mv88e6xxx_g2_trunk_mapping_write(struct mv88e6xxx_chip *chip,
+						   int id, u16 map)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int mv88e6xxx_g2_device_mapping_write(struct mv88e6xxx_chip *chip,
 						    int target, int port)
 {
-- 
2.17.1

[PATCH net-next 2/2] net: dsa: mv88e6xxx: Only allow LAG offload on supported hardware

[Tobias Waldekranz]

There are chips that do have Global 2 registers, and therefore trunk
mapping/mask tables are not available. Additionally Global 2 register
support is build-time optional, so we have to make sure that it is
compiled in.

Fixes: 57e661aae6a8 ("net: dsa: mv88e6xxx: Link aggregation support")
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 4 ++++
 drivers/net/dsa/mv88e6xxx/chip.h | 9 +++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index dcb1726b68cc..c48d166c2a70 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5385,9 +5385,13 @@ static bool mv88e6xxx_lag_can_offload(struct dsa_switch *ds,
 				      struct net_device *lag,
 				      struct netdev_lag_upper_info *info)
 {
+	struct mv88e6xxx_chip *chip = ds->priv;
 	struct dsa_port *dp;
 	int id, members = 0;
 
+	if (!mv88e6xxx_has_lag(chip))
+		return false;
+
 	id = dsa_lag_id(ds->dst, lag);
 	if (id < 0 || id >= ds->num_lag_ids)
 		return false;
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 3543055bcb51..333b4fab5aa2 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -662,6 +662,15 @@ static inline bool mv88e6xxx_has_pvt(struct mv88e6xxx_chip *chip)
 	return chip->info->pvt;
 }
 
+static inline bool mv88e6xxx_has_lag(struct mv88e6xxx_chip *chip)
+{
+#if (defined(CONFIG_NET_DSA_MV88E6XXX_GLOBAL2))
+	return chip->info->global2_addr != 0;
+#else
+	return false;
+#endif
+}
+
 static inline unsigned int mv88e6xxx_num_databases(struct mv88e6xxx_chip *chip)
 {
 	return chip->info->num_databases;
-- 
2.17.1

Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: Provide dummy implementations for trunk setters

[Vladimir Oltean]

On Fri, Jan 15, 2021 at 11:58:33AM +0100, Tobias Waldekranz wrote:
> Support for Global 2 registers is build-time optional. In the case
> where it was not enabled the build would fail as no "dummy"
> implementation of these functions was available.
> 
> Fixes: 57e661aae6a8 ("net: dsa: mv88e6xxx: Link aggregation support")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
Tested-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: Only allow LAG offload on supported hardware

[Vladimir Oltean]

On Fri, Jan 15, 2021 at 11:58:34AM +0100, Tobias Waldekranz wrote:
> There are chips that do have Global 2 registers, and therefore trunk
                       ~~
                       do not
> mapping/mask tables are not available. Additionally Global 2 register
> support is build-time optional, so we have to make sure that it is
> compiled in.
> 
> Fixes: 57e661aae6a8 ("net: dsa: mv88e6xxx: Link aggregation support")
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 4 ++++
>  drivers/net/dsa/mv88e6xxx/chip.h | 9 +++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index dcb1726b68cc..c48d166c2a70 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -5385,9 +5385,13 @@ static bool mv88e6xxx_lag_can_offload(struct dsa_switch *ds,
>  				      struct net_device *lag,
>  				      struct netdev_lag_upper_info *info)
>  {
> +	struct mv88e6xxx_chip *chip = ds->priv;
>  	struct dsa_port *dp;
>  	int id, members = 0;
>  
> +	if (!mv88e6xxx_has_lag(chip))
> +		return false;
> +
>  	id = dsa_lag_id(ds->dst, lag);
>  	if (id < 0 || id >= ds->num_lag_ids)
>  		return false;
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
> index 3543055bcb51..333b4fab5aa2 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.h
> +++ b/drivers/net/dsa/mv88e6xxx/chip.h
> @@ -662,6 +662,15 @@ static inline bool mv88e6xxx_has_pvt(struct mv88e6xxx_chip *chip)
>  	return chip->info->pvt;
>  }
>  
> +static inline bool mv88e6xxx_has_lag(struct mv88e6xxx_chip *chip)
> +{
> +#if (defined(CONFIG_NET_DSA_MV88E6XXX_GLOBAL2))
> +	return chip->info->global2_addr != 0;
> +#else
> +	return false;
> +#endif
> +}
> +

Should we also report ds->num_lag_ids = 0 if !mv88e6xxx_has_lag()?

>  static inline unsigned int mv88e6xxx_num_databases(struct mv88e6xxx_chip *chip)
>  {
>  	return chip->info->num_databases;
> -- 
> 2.17.1
> 

Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: Only allow LAG offload on supported hardware

[Vladimir Oltean]

On Fri, Jan 15, 2021 at 01:15:23PM +0200, Vladimir Oltean wrote:
> On Fri, Jan 15, 2021 at 11:58:34AM +0100, Tobias Waldekranz wrote:
> > There are chips that do have Global 2 registers, and therefore trunk
>                        ~~
>                        do not
> > mapping/mask tables are not available. Additionally Global 2 register
> > support is build-time optional, so we have to make sure that it is
> > compiled in.
> > 
> > Fixes: 57e661aae6a8 ("net: dsa: mv88e6xxx: Link aggregation support")
> > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> > ---
> >  drivers/net/dsa/mv88e6xxx/chip.c | 4 ++++
> >  drivers/net/dsa/mv88e6xxx/chip.h | 9 +++++++++
> >  2 files changed, 13 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> > index dcb1726b68cc..c48d166c2a70 100644
> > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > @@ -5385,9 +5385,13 @@ static bool mv88e6xxx_lag_can_offload(struct dsa_switch *ds,
> >  				      struct net_device *lag,
> >  				      struct netdev_lag_upper_info *info)
> >  {
> > +	struct mv88e6xxx_chip *chip = ds->priv;
> >  	struct dsa_port *dp;
> >  	int id, members = 0;
> >  
> > +	if (!mv88e6xxx_has_lag(chip))
> > +		return false;
> > +
> >  	id = dsa_lag_id(ds->dst, lag);
> >  	if (id < 0 || id >= ds->num_lag_ids)
> >  		return false;
> > diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
> > index 3543055bcb51..333b4fab5aa2 100644
> > --- a/drivers/net/dsa/mv88e6xxx/chip.h
> > +++ b/drivers/net/dsa/mv88e6xxx/chip.h
> > @@ -662,6 +662,15 @@ static inline bool mv88e6xxx_has_pvt(struct mv88e6xxx_chip *chip)
> >  	return chip->info->pvt;
> >  }
> >  
> > +static inline bool mv88e6xxx_has_lag(struct mv88e6xxx_chip *chip)
> > +{
> > +#if (defined(CONFIG_NET_DSA_MV88E6XXX_GLOBAL2))
> > +	return chip->info->global2_addr != 0;
> > +#else
> > +	return false;
> > +#endif
> > +}
> > +
> 
> Should we also report ds->num_lag_ids = 0 if !mv88e6xxx_has_lag()?
> 
> >  static inline unsigned int mv88e6xxx_num_databases(struct mv88e6xxx_chip *chip)
> >  {
> >  	return chip->info->num_databases;
> > -- 
> > 2.17.1
> > 

Actually in mv88e6xxx_detect there is this:

	err = mv88e6xxx_g2_require(chip);
	if (err)
		return err;


#else /* !CONFIG_NET_DSA_MV88E6XXX_GLOBAL2 */

static inline int mv88e6xxx_g2_require(struct mv88e6xxx_chip *chip)
{
	if (chip->info->global2_addr) {
		dev_err(chip->dev, "this chip requires CONFIG_NET_DSA_MV88E6XXX_GLOBAL2 enabled\n");
		return -EOPNOTSUPP;
	}

	return 0;
}

#endif

So CONFIG_NET_DSA_MV88E6XXX_GLOBAL2 is optional only if you use chips
that don't support the global2 area. Otherwise it is mandatory. So I
would update the commit message to not say "Additionally Global 2
register support is build-time optional", because it doesn't matter.

So I would simplify it to:

static inline bool mv88e6xxx_has_lag(struct mv88e6xxx_chip *chip)
{
	return !!chip->info->global2_addr;
}

Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: Provide dummy implementations for trunk setters

[Andrew Lunn]

On Fri, Jan 15, 2021 at 11:58:33AM +0100, Tobias Waldekranz wrote:
> Support for Global 2 registers is build-time optional.

I was never particularly happy about that. Maybe we should revisit
what features we loose when global 2 is dropped, and see if it still
makes sense to have it as optional?

However, until that happens:

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

    Andrew

Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: Provide dummy implementations for trunk setters

[Vladimir Oltean]

On Fri, Jan 15, 2021 at 03:30:30PM +0100, Andrew Lunn wrote:
> On Fri, Jan 15, 2021 at 11:58:33AM +0100, Tobias Waldekranz wrote:
> > Support for Global 2 registers is build-time optional.
> 
> I was never particularly happy about that. Maybe we should revisit
> what features we loose when global 2 is dropped, and see if it still
> makes sense to have it as optional?

Marvell switch newbie here, what do you mean "global 2 is dropped"?

Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: Only allow LAG offload on supported hardware

[Andrew Lunn]

> +static inline bool mv88e6xxx_has_lag(struct mv88e6xxx_chip *chip)
> +{
> +#if (defined(CONFIG_NET_DSA_MV88E6XXX_GLOBAL2))
> +	return chip->info->global2_addr != 0;
> +#else
> +	return false;
> +#endif

Given Vladimirs comments, this is just FYI:

You should not use #if like this. Use

	if (IS_ENABLED(CONFIG_NET_DSA_MV88E6XXX_GLOBAL2))
		return chip->info->global2_addr != 0;
	return false;

The advantage of this is it all gets compiled, so syntax errors in the
mostly unused leg get found quickly. The generated code should still
be optimal, since at build time it can evaluate the if and completely
remove it.

       Andrew

Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: Provide dummy implementations for trunk setters

[Andrew Lunn]

On Fri, Jan 15, 2021 at 04:36:49PM +0200, Vladimir Oltean wrote:
> On Fri, Jan 15, 2021 at 03:30:30PM +0100, Andrew Lunn wrote:
> > On Fri, Jan 15, 2021 at 11:58:33AM +0100, Tobias Waldekranz wrote:
> > > Support for Global 2 registers is build-time optional.
> > 
> > I was never particularly happy about that. Maybe we should revisit
> > what features we loose when global 2 is dropped, and see if it still
> > makes sense to have it as optional?
> 
> Marvell switch newbie here, what do you mean "global 2 is dropped"?

I was not aware detect() actually enforced it when needed. It used to
be, you could leave it out, and you would just get reduced
functionality for devices which had global2, but the code was not
compiled in.

At the beginning of the life of this driver, i guess it was maybe
25%/75% without/with global2, so it might of made sense to reduce the
binary size. But today the driver is much bigger with lots of other
things which those early chips don't have, SERDES for example. And
that ratio has dramatically reduced, there are very few devices
without those registers. This is why i think we can make our lives
easier and make global2 always compiled in.

       Andrew

Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: Provide dummy implementations for trunk setters

[Vladimir Oltean]

On Fri, Jan 15, 2021 at 03:48:36PM +0100, Andrew Lunn wrote:
> On Fri, Jan 15, 2021 at 04:36:49PM +0200, Vladimir Oltean wrote:
> > On Fri, Jan 15, 2021 at 03:30:30PM +0100, Andrew Lunn wrote:
> > > On Fri, Jan 15, 2021 at 11:58:33AM +0100, Tobias Waldekranz wrote:
> > > > Support for Global 2 registers is build-time optional.
> > >
> > > I was never particularly happy about that. Maybe we should revisit
> > > what features we loose when global 2 is dropped, and see if it still
> > > makes sense to have it as optional?
> >
> > Marvell switch newbie here, what do you mean "global 2 is dropped"?
>
> I was not aware detect() actually enforced it when needed. It used to
> be, you could leave it out, and you would just get reduced
> functionality for devices which had global2, but the code was not
> compiled in.
>
> At the beginning of the life of this driver, i guess it was maybe
> 25%/75% without/with global2, so it might of made sense to reduce the
> binary size. But today the driver is much bigger with lots of other
> things which those early chips don't have, SERDES for example. And
> that ratio has dramatically reduced, there are very few devices
> without those registers. This is why i think we can make our lives
> easier and make global2 always compiled in.

That makes sense, I thought you meant something else by "global 2
support is dropped", nevermind.

Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: Provide dummy implementations for trunk setters

[Tobias Waldekranz]

On Fri, Jan 15, 2021 at 15:48, Andrew Lunn <andrew@lunn.ch> wrote:
> On Fri, Jan 15, 2021 at 04:36:49PM +0200, Vladimir Oltean wrote:
>> On Fri, Jan 15, 2021 at 03:30:30PM +0100, Andrew Lunn wrote:
>> > On Fri, Jan 15, 2021 at 11:58:33AM +0100, Tobias Waldekranz wrote:
>> > > Support for Global 2 registers is build-time optional.
>> > 
>> > I was never particularly happy about that. Maybe we should revisit
>> > what features we loose when global 2 is dropped, and see if it still
>> > makes sense to have it as optional?
>> 
>> Marvell switch newbie here, what do you mean "global 2 is dropped"?
>
> I was not aware detect() actually enforced it when needed. It used to
> be, you could leave it out, and you would just get reduced
> functionality for devices which had global2, but the code was not
> compiled in.
>
> At the beginning of the life of this driver, i guess it was maybe
> 25%/75% without/with global2, so it might of made sense to reduce the
> binary size. But today the driver is much bigger with lots of other
> things which those early chips don't have, SERDES for example. And
> that ratio has dramatically reduced, there are very few devices
> without those registers. This is why i think we can make our lives
> easier and make global2 always compiled in.

Hear, hear!

I took a quick look at the (stripped) object sizes (ppc32):

# du -ab
6116    ./global1_vtu.o
5904    ./devlink.o
11500   ./port.o
9640    ./global2.o
3016    ./phy.o
5368    ./global1.o
51784   ./chip.o
9892    ./serdes.o
5140    ./global1_atu.o
1916    ./global2_avb.o
2248    ./global2_scratch.o
948     ./port_hidden.o
1828    ./smi.o
119396  .

So, roughly, you save 10%/13k. That hardly justifies the complexity IMO.

Andrew, do you want to do this? If not, I can look into it.

Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: Provide dummy implementations for trunk setters

[Andrew Lunn]

> Hear, hear!
> 
> I took a quick look at the (stripped) object sizes (ppc32):
> 
> # du -ab
> 6116    ./global1_vtu.o
> 5904    ./devlink.o
> 11500   ./port.o
> 9640    ./global2.o
> 3016    ./phy.o
> 5368    ./global1.o
> 51784   ./chip.o
> 9892    ./serdes.o
> 5140    ./global1_atu.o
> 1916    ./global2_avb.o
> 2248    ./global2_scratch.o
> 948     ./port_hidden.o
> 1828    ./smi.o
> 119396  .

size, part of binutils, is the better tool to use.

$ size *.o
   text	   data	    bss	    dec	    hex	filename
  36055	   2692	      4	  38751	   975f	chip.o
   3821	    116	      4	   3941	    f65	devlink.o
   3229	     96	      0	   3325	    cfd	global1_atu.o
   4388	      0	      0	   4388	   1124	global1.o
   4449	     24	      0	   4473	   1179	global1_vtu.o
   1548	      0	      0	   1548	    60c	global2_avb.o
   6350	      0	      0	   6350	   18ce	global2.o
   1412	      0	      0	   1412	    584	global2_scratch.o
   1757	      0	      0	   1757	    6dd	phy.o
    284	      0	      0	    284	    11c	port_hidden.o
   7516	      0	      0	   7516	   1d5c	port.o
   6570	    188	      0	   6758	   1a66	serdes.o
    764	      0	      0	    764	    2fc	smi.o

> Andrew, do you want to do this? If not, I can look into it.

Yes, i will add it to my TODO list.

     Andrew