Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1371716 > unrolled thread

[PATCH net-next 2/3] net: dsa: make the FDB add function return void

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH net-next 2/3] net: dsa: make the FDB add function return void Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2016-04-05 17:30 +0200
    Re: [PATCH net-next 2/3] net: dsa: make the FDB add function return  void Andrew Lunn <andrew@lunn.ch> - 2016-04-06 02:00 +0200
      Re: [PATCH net-next 2/3] net: dsa: make the FDB add function return void Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2016-04-06 05:20 +0200
        Re: [PATCH net-next 2/3] net: dsa: make the FDB add function return  void Andrew Lunn <andrew@lunn.ch> - 2016-04-06 15:00 +0200
          Re: [PATCH net-next 2/3] net: dsa: make the FDB add function return void Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2016-04-06 16:30 +0200

#1371716 — [PATCH net-next 2/3] net: dsa: make the FDB add function return void

The switchdev design implies that a software error should not happen in
the commit phase since it must have been previously reported in the
prepare phase. If an hardware error occurs during the commit phase,
there is nothing switchdev can do about it.

The DSA layer separates port_fdb_prepare and port_fdb_add for simplicity
and convenience. If an hardware error occurs during the commit phase,
there is no need to report it outside the DSA driver itself.

Make the DSA port_fdb_add routine return void for explicitness.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/bcm_sf2.c   |  9 +++++----
 drivers/net/dsa/mv88e6xxx.c | 12 +++++-------
 drivers/net/dsa/mv88e6xxx.h |  6 +++---
 include/net/dsa.h           |  2 +-
 net/dsa/slave.c             | 16 ++++++++--------
 5 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index b847624..feebeaa 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -722,13 +722,14 @@ static int bcm_sf2_sw_fdb_prepare(struct dsa_switch *ds, int port,
 	return 0;
 }
 
-static int bcm_sf2_sw_fdb_add(struct dsa_switch *ds, int port,
-			      const struct switchdev_obj_port_fdb *fdb,
-			      struct switchdev_trans *trans)
+static void bcm_sf2_sw_fdb_add(struct dsa_switch *ds, int port,
+			       const struct switchdev_obj_port_fdb *fdb,
+			       struct switchdev_trans *trans)
 {
 	struct bcm_sf2_priv *priv = ds_to_priv(ds);
 
-	return bcm_sf2_arl_op(priv, 0, port, fdb->addr, fdb->vid, true);
+	if (bcm_sf2_arl_op(priv, 0, port, fdb->addr, fdb->vid, true))
+		pr_err("%s: failed to add address\n", __func__);
 }
 
 static int bcm_sf2_sw_fdb_del(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 5a2e46d..bca9a2c 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -2090,21 +2090,19 @@ int mv88e6xxx_port_fdb_prepare(struct dsa_switch *ds, int port,
 	return 0;
 }
 
-int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
-			   const struct switchdev_obj_port_fdb *fdb,
-			   struct switchdev_trans *trans)
+void mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
+			    const struct switchdev_obj_port_fdb *fdb,
+			    struct switchdev_trans *trans)
 {
 	int state = is_multicast_ether_addr(fdb->addr) ?
 		GLOBAL_ATU_DATA_STATE_MC_STATIC :
 		GLOBAL_ATU_DATA_STATE_UC_STATIC;
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
-	int ret;
 
 	mutex_lock(&ps->smi_mutex);
-	ret = _mv88e6xxx_port_fdb_load(ds, port, fdb->addr, fdb->vid, state);
+	if (_mv88e6xxx_port_fdb_load(ds, port, fdb->addr, fdb->vid, state))
+		netdev_warn(ds->ports[port], "cannot load address\n");
 	mutex_unlock(&ps->smi_mutex);
-
-	return ret;
 }
 
 int mv88e6xxx_port_fdb_del(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 8a62afb..ece008f 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -514,9 +514,9 @@ int mv88e6xxx_port_vlan_dump(struct dsa_switch *ds, int port,
 int mv88e6xxx_port_fdb_prepare(struct dsa_switch *ds, int port,
 			       const struct switchdev_obj_port_fdb *fdb,
 			       struct switchdev_trans *trans);
-int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
-			   const struct switchdev_obj_port_fdb *fdb,
-			   struct switchdev_trans *trans);
+void mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
+			    const struct switchdev_obj_port_fdb *fdb,
+			    struct switchdev_trans *trans);
 int mv88e6xxx_port_fdb_del(struct dsa_switch *ds, int port,
 			   const struct switchdev_obj_port_fdb *fdb);
 int mv88e6xxx_port_fdb_dump(struct dsa_switch *ds, int port,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index eddd0f3..fa42b8c 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -324,7 +324,7 @@ struct dsa_switch_driver {
 	int	(*port_fdb_prepare)(struct dsa_switch *ds, int port,
 				    const struct switchdev_obj_port_fdb *fdb,
 				    struct switchdev_trans *trans);
-	int	(*port_fdb_add)(struct dsa_switch *ds, int port,
+	void	(*port_fdb_add)(struct dsa_switch *ds, int port,
 				const struct switchdev_obj_port_fdb *fdb,
 				struct switchdev_trans *trans);
 	int	(*port_fdb_del)(struct dsa_switch *ds, int port,
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 1c55f96..5a34bab 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -256,17 +256,17 @@ static int dsa_slave_port_fdb_add(struct net_device *dev,
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
 	struct dsa_switch *ds = p->parent;
-	int ret;
 
-	if (!ds->drv->port_fdb_prepare || !ds->drv->port_fdb_add)
-		return -EOPNOTSUPP;
+	if (switchdev_trans_ph_prepare(trans)) {
+		if (!ds->drv->port_fdb_prepare || !ds->drv->port_fdb_add)
+			return -EOPNOTSUPP;
 
-	if (switchdev_trans_ph_prepare(trans))
-		ret = ds->drv->port_fdb_prepare(ds, p->port, fdb, trans);
-	else
-		ret = ds->drv->port_fdb_add(ds, p->port, fdb, trans);
+		return ds->drv->port_fdb_prepare(ds, p->port, fdb, trans);
+	}
 
-	return ret;
+	ds->drv->port_fdb_add(ds, p->port, fdb, trans);
+
+	return 0;
 }
 
 static int dsa_slave_port_fdb_del(struct net_device *dev,
-- 
2.8.0

[toc] | [next] | [standalone]

#1372081 — Re: [PATCH net-next 2/3] net: dsa: make the FDB add function return void

On Tue, Apr 05, 2016 at 11:24:34AM -0400, Vivien Didelot wrote:
> The switchdev design implies that a software error should not happen in
> the commit phase since it must have been previously reported in the
> prepare phase. If an hardware error occurs during the commit phase,
> there is nothing switchdev can do about it.
> 
> The DSA layer separates port_fdb_prepare and port_fdb_add for simplicity
> and convenience. If an hardware error occurs during the commit phase,
> there is no need to report it outside the DSA driver itself.
> 
> Make the DSA port_fdb_add routine return void for explicitness.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  drivers/net/dsa/bcm_sf2.c   |  9 +++++----
>  drivers/net/dsa/mv88e6xxx.c | 12 +++++-------
>  drivers/net/dsa/mv88e6xxx.h |  6 +++---
>  include/net/dsa.h           |  2 +-
>  net/dsa/slave.c             | 16 ++++++++--------
>  5 files changed, 22 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> index b847624..feebeaa 100644
> --- a/drivers/net/dsa/bcm_sf2.c
> +++ b/drivers/net/dsa/bcm_sf2.c
> @@ -722,13 +722,14 @@ static int bcm_sf2_sw_fdb_prepare(struct dsa_switch *ds, int port,
>  	return 0;
>  }
>  
> -static int bcm_sf2_sw_fdb_add(struct dsa_switch *ds, int port,
> -			      const struct switchdev_obj_port_fdb *fdb,
> -			      struct switchdev_trans *trans)
> +static void bcm_sf2_sw_fdb_add(struct dsa_switch *ds, int port,
> +			       const struct switchdev_obj_port_fdb *fdb,
> +			       struct switchdev_trans *trans)
>  {
>  	struct bcm_sf2_priv *priv = ds_to_priv(ds);
>  
> -	return bcm_sf2_arl_op(priv, 0, port, fdb->addr, fdb->vid, true);
> +	if (bcm_sf2_arl_op(priv, 0, port, fdb->addr, fdb->vid, true))
> +		pr_err("%s: failed to add address\n", __func__);
>  }
>  
>  static int bcm_sf2_sw_fdb_del(struct dsa_switch *ds, int port,
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index 5a2e46d..bca9a2c 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -2090,21 +2090,19 @@ int mv88e6xxx_port_fdb_prepare(struct dsa_switch *ds, int port,
>  	return 0;
>  }
>  
> -int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
> -			   const struct switchdev_obj_port_fdb *fdb,
> -			   struct switchdev_trans *trans)
> +void mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
> +			    const struct switchdev_obj_port_fdb *fdb,
> +			    struct switchdev_trans *trans)
>  {
>  	int state = is_multicast_ether_addr(fdb->addr) ?
>  		GLOBAL_ATU_DATA_STATE_MC_STATIC :
>  		GLOBAL_ATU_DATA_STATE_UC_STATIC;
>  	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
> -	int ret;
>  
>  	mutex_lock(&ps->smi_mutex);
> -	ret = _mv88e6xxx_port_fdb_load(ds, port, fdb->addr, fdb->vid, state);
> +	if (_mv88e6xxx_port_fdb_load(ds, port, fdb->addr, fdb->vid, state))
> +		netdev_warn(ds->ports[port], "cannot load address\n");

In the SF2 driver you use pr_err, but here netdev_warn. We probably
should be consistent if we error or warn. I would use netdev_error,
since if this fails we probably have a real hardware problem.

      Andrew

[toc] | [prev] | [next] | [standalone]

#1372163

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>>  	mutex_lock(&ps->smi_mutex);
>> -	ret = _mv88e6xxx_port_fdb_load(ds, port, fdb->addr, fdb->vid, state);
>> +	if (_mv88e6xxx_port_fdb_load(ds, port, fdb->addr, fdb->vid, state))
>> +		netdev_warn(ds->ports[port], "cannot load address\n");
>
> In the SF2 driver you use pr_err, but here netdev_warn. We probably
> should be consistent if we error or warn. I would use netdev_error,
> since if this fails we probably have a real hardware problem.

I used pr_err in the SF2 driver to be consistent with the rest of the
code which only uses pr_err and pr_info.

I was thinking about adding ds_err and ds_port_err to print errors for
ds->master_dev and ds->ports[port], but that might be overkill. What do
you think? Or local to the driver for the moment, like mvsw_err maybe?

I tend to use warn for cases where the user cannot really do something
about the situation, but an hardware problem is indeed critical, so I
agree with you to use error over warn here.

Thanks,
Vivien

[toc] | [prev] | [next] | [standalone]

#1372505 — Re: [PATCH net-next 2/3] net: dsa: make the FDB add function return void

On Tue, Apr 05, 2016 at 11:14:54PM -0400, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> >>  	mutex_lock(&ps->smi_mutex);
> >> -	ret = _mv88e6xxx_port_fdb_load(ds, port, fdb->addr, fdb->vid, state);
> >> +	if (_mv88e6xxx_port_fdb_load(ds, port, fdb->addr, fdb->vid, state))
> >> +		netdev_warn(ds->ports[port], "cannot load address\n");
> >
> > In the SF2 driver you use pr_err, but here netdev_warn. We probably
> > should be consistent if we error or warn. I would use netdev_error,
> > since if this fails we probably have a real hardware problem.
> 
> I used pr_err in the SF2 driver to be consistent with the rest of the
> code which only uses pr_err and pr_info.

O.K, good.
 
> I was thinking about adding ds_err and ds_port_err to print errors for
> ds->master_dev and ds->ports[port], but that might be overkill.

I'm also trying to kill off the use of ds within the mv88e6xxx driver.

> What do you think? Or local to the driver for the moment, like
> mvsw_err maybe?

I would keep it local. Also, for this sort of error, it does not need
to differentiate on port. It is a hardware access error, something is
wrong with the mdio bus/switch. So i would even put the message in the
very low level reg_read/reg_write functions, and no where else.

     Andrew

[toc] | [prev] | [next] | [standalone]

#1372570

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>> >>  	mutex_lock(&ps->smi_mutex);
>> >> -	ret = _mv88e6xxx_port_fdb_load(ds, port, fdb->addr, fdb->vid, state);
>> >> +	if (_mv88e6xxx_port_fdb_load(ds, port, fdb->addr, fdb->vid, state))
>> >> +		netdev_warn(ds->ports[port], "cannot load address\n");
>> >
>> > In the SF2 driver you use pr_err, but here netdev_warn. We probably
>> > should be consistent if we error or warn. I would use netdev_error,
>> > since if this fails we probably have a real hardware problem.
>> 
>> I used pr_err in the SF2 driver to be consistent with the rest of the
>> code which only uses pr_err and pr_info.
>
> O.K, good.
>  
>> I was thinking about adding ds_err and ds_port_err to print errors for
>> ds->master_dev and ds->ports[port], but that might be overkill.
>
> I'm also trying to kill off the use of ds within the mv88e6xxx driver.
>
>> What do you think? Or local to the driver for the moment, like
>> mvsw_err maybe?
>
> I would keep it local. Also, for this sort of error, it does not need
> to differentiate on port. It is a hardware access error, something is
> wrong with the mdio bus/switch. So i would even put the message in the
> very low level reg_read/reg_write functions, and no where else.

OK, so I will keep a netdev_err() in _mv88e6xxx_port_fdb_add since I
don't like to ignore return values, and will send a future separate
patch to add such message in low level functions as you suggested, and
maybe voidify a few high level functions using them.

Thanks,
Vivien

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web