diff mbox series

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

Message ID 20210115105834.559-3-tobias@waldekranz.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: mv88e6xxx: LAG fixes | expand

Checks

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, 28 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

Commit Message

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

Comments

Vladimir Oltean Jan. 15, 2021, 11:15 a.m. UTC | #1 
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
>
Vladimir Oltean Jan. 15, 2021, 11:29 a.m. UTC | #2 
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;
}
Andrew Lunn Jan. 15, 2021, 2:39 p.m. UTC | #3 
> +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
diff mbox series

Patch

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;