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

Groups > linux.kernel > #1472101 > unrolled thread

[PATCH net-next 3/3] net: dsa: mv88e6xxx: add MDB support

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 3/3] net: dsa: mv88e6xxx: add MDB support Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2016-08-29 22:40 +0200
    Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: add MDB support Andrew Lunn <andrew@lunn.ch> - 2016-08-31 16:00 +0200
      Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: add MDB support Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2016-08-31 16:50 +0200
        Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: add MDB support Andrew Lunn <andrew@lunn.ch> - 2016-08-31 17:00 +0200
        Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: add MDB support Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-08-31 19:40 +0200

#1472101 — [PATCH net-next 3/3] net: dsa: mv88e6xxx: add MDB support

Add support for the MDB operations. This consists of
loading/purging/dumping multicast addresses for a given port in the ATU.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 65 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 93abfff..812cb47 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2240,6 +2240,15 @@ static int mv88e6xxx_port_db_dump_one(struct mv88e6xxx_chip *chip,
 				fdb->ndm_state = NUD_NOARP;
 			else
 				fdb->ndm_state = NUD_REACHABLE;
+		} else {
+			struct switchdev_obj_port_mdb *mdb;
+
+			if (!is_multicast_ether_addr(addr.mac))
+				continue;
+
+			mdb = SWITCHDEV_OBJ_PORT_MDB(obj);
+			mdb->vid = vid;
+			ether_addr_copy(mdb->addr, addr.mac);
 		}
 
 		err = cb(obj);
@@ -3988,6 +3997,58 @@ free:
 	return NULL;
 }
 
+static int mv88e6xxx_port_mdb_prepare(struct dsa_switch *ds, int port,
+				      const struct switchdev_obj_port_mdb *mdb,
+				      struct switchdev_trans *trans)
+{
+	/* We don't need any dynamic resource from the kernel (yet),
+	 * so skip the prepare phase.
+	 */
+
+	return 0;
+}
+
+static void mv88e6xxx_port_mdb_add(struct dsa_switch *ds, int port,
+				   const struct switchdev_obj_port_mdb *mdb,
+				   struct switchdev_trans *trans)
+{
+	struct mv88e6xxx_chip *chip = ds_to_priv(ds);
+
+	mutex_lock(&chip->reg_lock);
+	if (mv88e6xxx_port_db_load_purge(chip, port, mdb->addr, mdb->vid,
+					 GLOBAL_ATU_DATA_STATE_MC_STATIC))
+		netdev_err(ds->ports[port].netdev, "failed to load multicast MAC address\n");
+	mutex_unlock(&chip->reg_lock);
+}
+
+static int mv88e6xxx_port_mdb_del(struct dsa_switch *ds, int port,
+				  const struct switchdev_obj_port_mdb *mdb)
+{
+	struct mv88e6xxx_chip *chip = ds_to_priv(ds);
+	int err;
+
+	mutex_lock(&chip->reg_lock);
+	err = mv88e6xxx_port_db_load_purge(chip, port, mdb->addr, mdb->vid,
+					   GLOBAL_ATU_DATA_STATE_UNUSED);
+	mutex_unlock(&chip->reg_lock);
+
+	return err;
+}
+
+static int mv88e6xxx_port_mdb_dump(struct dsa_switch *ds, int port,
+				   struct switchdev_obj_port_mdb *mdb,
+				   int (*cb)(struct switchdev_obj *obj))
+{
+	struct mv88e6xxx_chip *chip = ds_to_priv(ds);
+	int err;
+
+	mutex_lock(&chip->reg_lock);
+	err = mv88e6xxx_port_db_dump(chip, port, &mdb->obj, cb);
+	mutex_unlock(&chip->reg_lock);
+
+	return err;
+}
+
 static struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.probe			= mv88e6xxx_drv_probe,
 	.get_tag_protocol	= mv88e6xxx_get_tag_protocol,
@@ -4023,6 +4084,10 @@ static struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.port_fdb_add           = mv88e6xxx_port_fdb_add,
 	.port_fdb_del           = mv88e6xxx_port_fdb_del,
 	.port_fdb_dump          = mv88e6xxx_port_fdb_dump,
+	.port_mdb_prepare       = mv88e6xxx_port_mdb_prepare,
+	.port_mdb_add           = mv88e6xxx_port_mdb_add,
+	.port_mdb_del           = mv88e6xxx_port_mdb_del,
+	.port_mdb_dump          = mv88e6xxx_port_mdb_dump,
 };
 
 static int mv88e6xxx_register_switch(struct mv88e6xxx_chip *chip,
-- 
2.9.3

[toc] | [next] | [standalone]

#1473504

Hi Vivien

> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 93abfff..812cb47 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -2240,6 +2240,15 @@ static int mv88e6xxx_port_db_dump_one(struct mv88e6xxx_chip *chip,
>  				fdb->ndm_state = NUD_NOARP;
>  			else
>  				fdb->ndm_state = NUD_REACHABLE;
> +		} else {

Rather than else, i think it would be safer to do

		if (obj->id == SWITCHDEV_OBJ_ID_PORT_MDB) {
> +			struct switchdev_obj_port_mdb *mdb;
> +
> +			if (!is_multicast_ether_addr(addr.mac))
> +				continue;
> +
> +			mdb = SWITCHDEV_OBJ_PORT_MDB(obj);
> +			mdb->vid = vid;
> +			ether_addr_copy(mdb->addr, addr.mac);
>  		}

It should not happen, but the day it does, we get very confused...

> +static int mv88e6xxx_port_mdb_dump(struct dsa_switch *ds, int port,
> +				   struct switchdev_obj_port_mdb *mdb,
> +				   int (*cb)(struct switchdev_obj *obj))
> +{
> +	struct mv88e6xxx_chip *chip = ds_to_priv(ds);
> +	int err;
> +
> +	mutex_lock(&chip->reg_lock);
> +	err = mv88e6xxx_port_db_dump(chip, port, &mdb->obj, cb);
> +	mutex_unlock(&chip->reg_lock);
> +
> +	return err;
> +}

Isn't this identical to mv88e6xxx_port_fdb_dump()? Maybe we should
just have one function, and register it twice?

     Andrew

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

#1473562

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> Hi Vivien
>
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>> index 93abfff..812cb47 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -2240,6 +2240,15 @@ static int mv88e6xxx_port_db_dump_one(struct mv88e6xxx_chip *chip,
>>  				fdb->ndm_state = NUD_NOARP;
>>  			else
>>  				fdb->ndm_state = NUD_REACHABLE;
>> +		} else {
>
> Rather than else, i think it would be safer to do
>
> 		if (obj->id == SWITCHDEV_OBJ_ID_PORT_MDB) {
>> +			struct switchdev_obj_port_mdb *mdb;
>> +
>> +			if (!is_multicast_ether_addr(addr.mac))
>> +				continue;
>> +
>> +			mdb = SWITCHDEV_OBJ_PORT_MDB(obj);
>> +			mdb->vid = vid;
>> +			ether_addr_copy(mdb->addr, addr.mac);
>>  		}
>
> It should not happen, but the day it does, we get very confused...

Do you mean the something like this?

    if (obj->id == SWITCHDEV_OBJ_ID_PORT_FDB) {
        ...
    } else if (obj->id == SWITCHDEV_OBJ_ID_PORT_MDB) {
        ...
    } else {
        return -EOPNOTSUPP;
    }

I'm OK with that if you think it is better.

>> +static int mv88e6xxx_port_mdb_dump(struct dsa_switch *ds, int port,
>> +				   struct switchdev_obj_port_mdb *mdb,
>> +				   int (*cb)(struct switchdev_obj *obj))
>> +{
>> +	struct mv88e6xxx_chip *chip = ds_to_priv(ds);
>> +	int err;
>> +
>> +	mutex_lock(&chip->reg_lock);
>> +	err = mv88e6xxx_port_db_dump(chip, port, &mdb->obj, cb);
>> +	mutex_unlock(&chip->reg_lock);
>> +
>> +	return err;
>> +}
>
> Isn't this identical to mv88e6xxx_port_fdb_dump()? Maybe we should
> just have one function, and register it twice?

No the signatures are differentes. See _fdb vs. _mdb. They are basically
the same thing (at least, from the switch driver's point of view).

We can abstract the FDB (unicast and multicast) related operations
directly in the DSA layer, but I think it is better to map directly the
switchdev obj operations for the moment...

What do you guys think?

Thanks,

        Vivien

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

#1473586

> Do you mean the something like this?
> 
>     if (obj->id == SWITCHDEV_OBJ_ID_PORT_FDB) {
>         ...
>     } else if (obj->id == SWITCHDEV_OBJ_ID_PORT_MDB) {
>         ...
>     } else {
>         return -EOPNOTSUPP;
>     }

Hi Vivien

The -ENONOTSUPP is even better. I was not going as far as that.
 
> No the signatures are differentes. See _fdb vs. _mdb.

Ah, missed that. Then two functions are O.K. I don't think we should
consolidate it in the DSA layer.

	    Andrew

	    

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

#1473746

Hello.

On 08/31/2016 05:46 PM, Vivien Didelot wrote:

>>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>>> index 93abfff..812cb47 100644
>>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>>> @@ -2240,6 +2240,15 @@ static int mv88e6xxx_port_db_dump_one(struct mv88e6xxx_chip *chip,
>>>  				fdb->ndm_state = NUD_NOARP;
>>>  			else
>>>  				fdb->ndm_state = NUD_REACHABLE;
>>> +		} else {
>>
>> Rather than else, i think it would be safer to do
>>
>> 		if (obj->id == SWITCHDEV_OBJ_ID_PORT_MDB) {
>>> +			struct switchdev_obj_port_mdb *mdb;
>>> +
>>> +			if (!is_multicast_ether_addr(addr.mac))
>>> +				continue;
>>> +
>>> +			mdb = SWITCHDEV_OBJ_PORT_MDB(obj);
>>> +			mdb->vid = vid;
>>> +			ether_addr_copy(mdb->addr, addr.mac);
>>>  		}
>>
>> It should not happen, but the day it does, we get very confused...
>
> Do you mean the something like this?
>
>     if (obj->id == SWITCHDEV_OBJ_ID_PORT_FDB) {
>         ...
>     } else if (obj->id == SWITCHDEV_OBJ_ID_PORT_MDB) {
>         ...
>     } else {
>         return -EOPNOTSUPP;
>     }
>
> I'm OK with that if you think it is better.

    Just code it as a *switch*, please. :-)

[...]

MBR, Sergei

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web