| Message ID | 20210115105834.559-2-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: mv88e6xxx: LAG fixes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 18 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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>
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
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"?
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
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.
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.
> 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
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) {
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(+)