mbox series

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

Message ID 20210115105834.559-1-tobias@waldekranz.com
Headers show
Series net: dsa: mv88e6xxx: LAG fixes | expand

Message

Tobias Waldekranz Jan. 15, 2021, 10:58 a.m. UTC 
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(+)

Comments

Vladimir Oltean Jan. 15, 2021, 11:10 a.m. UTC | #1 
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>
Vladimir Oltean Jan. 15, 2021, 2:53 p.m. UTC | #2 
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.
Andrew Lunn Jan. 18, 2021, 5:54 p.m. UTC | #3 
> 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