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

[PATCH v2 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 hardware
actually supports LAG offloading before trying to configure it.

v1 -> v2:
- Do not allocate LAG ID mappings on unsupported hardware (Vladimir).
- Simplify _has_lag predicate (Vladimir).

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    |  6 +++++-
 drivers/net/dsa/mv88e6xxx/chip.h    |  5 +++++
 drivers/net/dsa/mv88e6xxx/global2.h | 12 ++++++++++++
 3 files changed, 22 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH v2 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>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
Tested-by: Vladimir Oltean <olteanv@gmail.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 v2 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. Refuse the offload as early as
possible on those devices.

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

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index dcb1726b68cc..91286d7b12c7 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;
@@ -5727,7 +5731,7 @@ static int mv88e6xxx_register_switch(struct mv88e6xxx_chip *chip)
 	 * 5-bit port mode, which we do not support. 640k^W16 ought to
 	 * be enough for anyone.
 	 */
-	ds->num_lag_ids = 16;
+	ds->num_lag_ids = mv88e6xxx_has_lag(chip) ? 16 : 0;
 
 	dev_set_drvdata(dev, ds);
 
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 3543055bcb51..788b3f585ef3 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -662,6 +662,11 @@ static inline bool mv88e6xxx_has_pvt(struct mv88e6xxx_chip *chip)
 	return chip->info->pvt;
 }
 
+static inline bool mv88e6xxx_has_lag(struct mv88e6xxx_chip *chip)
+{
+	return !!chip->info->global2_addr;
+}
+
 static inline unsigned int mv88e6xxx_num_databases(struct mv88e6xxx_chip *chip)
 {
 	return chip->info->num_databases;
-- 
2.17.1

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

[Vladimir Oltean]

On Fri, Jan 15, 2021 at 01:52:59PM +0100, Tobias Waldekranz wrote:
> There are chips that do have Global 2 registers, and therefore trunk
> mapping/mask tables are not available. Refuse the offload as early as
> possible on those devices.
> 
> Fixes: 57e661aae6a8 ("net: dsa: mv88e6xxx: Link aggregation support")
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---

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

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

[Jakub Kicinski]

On Fri, 15 Jan 2021 13:52:57 +0100 Tobias Waldekranz wrote:
> 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 hardware
> actually supports LAG offloading before trying to configure it.
> 
> v1 -> v2:
> - Do not allocate LAG ID mappings on unsupported hardware (Vladimir).
> - Simplify _has_lag predicate (Vladimir).

If I'm reading the discussion on v1 right there will be a v3,
LMK if I got it wrong.

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

[Vladimir Oltean]

On Fri, Jan 15, 2021 at 03:02:46PM -0800, Jakub Kicinski wrote:
> On Fri, 15 Jan 2021 13:52:57 +0100 Tobias Waldekranz wrote:
> > 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 hardware
> > actually supports LAG offloading before trying to configure it.
> > 
> > v1 -> v2:
> > - Do not allocate LAG ID mappings on unsupported hardware (Vladimir).
> > - Simplify _has_lag predicate (Vladimir).
> 
> If I'm reading the discussion on v1 right there will be a v3,
> LMK if I got it wrong.

I don't think a v3 was supposed to be coming, what made you think that?

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

[Jakub Kicinski]

On Sat, 16 Jan 2021 01:24:35 +0200 Vladimir Oltean wrote:
> On Fri, Jan 15, 2021 at 03:02:46PM -0800, Jakub Kicinski wrote:
> > On Fri, 15 Jan 2021 13:52:57 +0100 Tobias Waldekranz wrote:  
> > > 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 hardware
> > > actually supports LAG offloading before trying to configure it.
> > > 
> > > v1 -> v2:
> > > - Do not allocate LAG ID mappings on unsupported hardware (Vladimir).
> > > - Simplify _has_lag predicate (Vladimir).  
> > 
> > If I'm reading the discussion on v1 right there will be a v3,
> > LMK if I got it wrong.  
> 
> I don't think a v3 was supposed to be coming, what made you think that?

I thought you concluded that the entire CONFIG_NET_DSA_MV88E6XXX_GLOBAL2
should go, you said:

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

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

[Vladimir Oltean]

On Fri, Jan 15, 2021 at 03:46:22PM -0800, Jakub Kicinski wrote:
> On Sat, 16 Jan 2021 01:24:35 +0200 Vladimir Oltean wrote:
> > On Fri, Jan 15, 2021 at 03:02:46PM -0800, Jakub Kicinski wrote:
> > > On Fri, 15 Jan 2021 13:52:57 +0100 Tobias Waldekranz wrote:
> > > > 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 hardware
> > > > actually supports LAG offloading before trying to configure it.
> > > >
> > > > v1 -> v2:
> > > > - Do not allocate LAG ID mappings on unsupported hardware (Vladimir).
> > > > - Simplify _has_lag predicate (Vladimir).
> > >
> > > If I'm reading the discussion on v1 right there will be a v3,
> > > LMK if I got it wrong.
> >
> > I don't think a v3 was supposed to be coming, what made you think that?
>
> I thought you concluded that the entire CONFIG_NET_DSA_MV88E6XXX_GLOBAL2
> should go, you said:
>
> > So, roughly, you save 10%/13k. That hardly justifies the complexity IMO.

That would be the first time that I hear of fixing a build failure due
to a missing shim by refactoring a driver... Punctual issue, punctual
fix, no?

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

[Tobias Waldekranz]

On Fri, Jan 15, 2021 at 15:46, Jakub Kicinski <kuba@kernel.org> wrote:
> On Sat, 16 Jan 2021 01:24:35 +0200 Vladimir Oltean wrote:
>> On Fri, Jan 15, 2021 at 03:02:46PM -0800, Jakub Kicinski wrote:
>> > On Fri, 15 Jan 2021 13:52:57 +0100 Tobias Waldekranz wrote:  
>> > > 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 hardware
>> > > actually supports LAG offloading before trying to configure it.
>> > > 
>> > > v1 -> v2:
>> > > - Do not allocate LAG ID mappings on unsupported hardware (Vladimir).
>> > > - Simplify _has_lag predicate (Vladimir).  
>> > 
>> > If I'm reading the discussion on v1 right there will be a v3,
>> > LMK if I got it wrong.  
>> 
>> I don't think a v3 was supposed to be coming, what made you think that?
>
> I thought you concluded that the entire CONFIG_NET_DSA_MV88E6XXX_GLOBAL2
> should go, you said:
>
>> So, roughly, you save 10%/13k. That hardly justifies the complexity IMO.

Well, based on what Andrew said, my guess is that that is where we will
end up.

But since at the moment net-next does not build for configs without
Global2-support, I would really appreciate it if this series could be
applied to solve that immediate problem.

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

[Jakub Kicinski]

On Sat, 16 Jan 2021 01:55:47 +0200 Vladimir Oltean wrote:
> On Fri, Jan 15, 2021 at 03:46:22PM -0800, Jakub Kicinski wrote:
> > On Sat, 16 Jan 2021 01:24:35 +0200 Vladimir Oltean wrote:  
> > > On Fri, Jan 15, 2021 at 03:02:46PM -0800, Jakub Kicinski wrote:  
> > > > On Fri, 15 Jan 2021 13:52:57 +0100 Tobias Waldekranz wrote:  
> > > > > 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 hardware
> > > > > actually supports LAG offloading before trying to configure it.
> > > > >
> > > > > v1 -> v2:
> > > > > - Do not allocate LAG ID mappings on unsupported hardware (Vladimir).
> > > > > - Simplify _has_lag predicate (Vladimir).  
> > > >
> > > > If I'm reading the discussion on v1 right there will be a v3,
> > > > LMK if I got it wrong.  
> > >
> > > I don't think a v3 was supposed to be coming, what made you think that?  
> >
> > I thought you concluded that the entire CONFIG_NET_DSA_MV88E6XXX_GLOBAL2
> > should go, you said:
> >  
> > > So, roughly, you save 10%/13k. That hardly justifies the complexity IMO.  
> 
> That would be the first time that I hear of fixing a build failure due
> to a missing shim by refactoring a driver... Punctual issue, punctual
> fix, no?

Sure, without knowing the driver it's hard to tell if it's a matter of
removing those stubs in the header, or more work, hence the question. 

Applied now, thanks!