Re: [PATCH net-next v3 11/12] net: dsa: Implement ndo_get_port_parent_id()
Hi Florian,
On Tue, 5 Feb 2019 15:53:25 -0800, Florian Fainelli
wrote:
> DSA implements SWITCHDEV_ATTR_ID_PORT_PARENT_ID and we want to get rid
> of switchdev_ops eventually, ease that migration by implementing a
> ndo_get_port_parent_id() function which returns what
> switchdev_port_attr_get() would do.
>
> Signed-off-by: Florian Fainelli
> ---
> net/dsa/slave.c | 18 --
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 91de3a663226..70395a0ae52e 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -362,18 +362,23 @@ static int dsa_slave_port_obj_del(struct net_device
> *dev,
> return err;
> }
>
> -static int dsa_slave_port_attr_get(struct net_device *dev,
> -struct switchdev_attr *attr)
> +static int dsa_slave_get_port_parent_id(struct net_device *dev,
> + struct netdev_phys_item_id *ppid)
> {
> struct dsa_port *dp = dsa_slave_to_port(dev);
> struct dsa_switch *ds = dp->ds;
> struct dsa_switch_tree *dst = ds->dst;
>
> + ppid->id_len = sizeof(dst->index);
> + memcpy(&ppid->id, &dst->index, ppid->id_len);
> +
> + return 0;
> +}
Finally this will give us a way to distinguish two ports with the same switch
and port IDs on a system with two disjoint switch trees, thanks!
Reviewed-by: Vivien Didelot
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next v3 1/7] net: bridge: Extract boilerplate around switchdev_port_obj_*()
Hi Petr, Petr Machata writes: > A call to switchdev_port_obj_add() or switchdev_port_obj_del() involves > initializing a struct switchdev_obj_port_vlan, a piece of code that > repeats on each call site almost verbatim. While in the current codebase > there is just one duplicated add call, the follow-up patches add more of > both add and del calls. > > Thus to remove the duplication, extract the repetition into named > functions and reuse. > > Signed-off-by: Petr Machata Considering Dan's comment as well: Reviewed-by: Vivien Didelot Thanks, Vivien ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 6/7] net: bridge: Notify about bridge VLANs
Hi Petr,
Petr Machata writes:
> Vivien Didelot writes:
>
>>> + } else {
>>> + err = br_switchdev_port_obj_add(dev, v->vid, flags);
>>> + if (err && err != -EOPNOTSUPP)
>>> + goto out;
>>> }
>>
>> Except that br_switchdev_port_obj_add taking vid and flags arguments
>> seems confusing to me, the change looks good:
>
> I'm not sure what you're aiming at. Both VID and flags are sent with the
> notification, so they need to be passed on to the function somehow. Do
> you have a counterproposal for the API?
I'm only questioning the code organization here, not the functional
aspect which I do agree with. What I'm saying is that you name a new
switchdev helper br_switchdev_port_OBJ_add, which takes VLAN arguments
(vid and flags.) How would you call another eventual helper taking MDB
arguments, br_switchdev_port_OBJ_add again? So something like
br_switchdev_port_VLAN_add would be more intuitive.
At the same time there's an effort to centralize all switchdev helpers
of the bridge layer (i.e. the software -> hardware bridge calls) into
net/bridge/br_switchdev.c, so that file would be more adequate.
You may discard my comments but I think it'd be beneficial to us all to
finally keep a bit of consistency in that bridge layer code.
Thanks,
Vivien
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 0/7] net: bridge: Notify about bridge VLANs
Hi Florian, Florian Fainelli writes: > Andrew, Vivien, if the following hunks get applied are we possibly > breaking mv88e6xxx? This is the use case that is really missing IMHO at > the moment in DSA: we cannot control the VLAN membership and attributes > of the CPU port(s), so either we make it always tagged in every VLAN > (not great), or we introduce the ability to target the CPU port which is > what Petr's patches + mine do. Your change looks good to me. mv88e6xxx programs the DSA and CPU ports' membership as "unmodified" (i.e. "as-is", which is a Marvell feature), so that shouldn't change the current behavior. Thanks, Vivien ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 4/7] dsa: port: Ignore bridge VLAN events
Hi Petr, Petr Machata writes: > Ignore VLAN events where the orig_dev is the bridge device itself. > > Signed-off-by: Petr Machata Reviewed-by: Vivien Didelot Thanks, Vivien ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 1/7] net: bridge: Extract boilerplate around switchdev_port_obj_*()
Hi Petr,
Petr Machata writes:
> -static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
> - u16 vid, u16 flags)
> +static int br_switchdev_port_obj_add(struct net_device *dev, u16 vid, u16
> flags)
> {
> struct switchdev_obj_port_vlan v = {
> .obj.orig_dev = dev,
> @@ -89,12 +88,29 @@ static int __vlan_vid_add(struct net_device *dev, struct
> net_bridge *br,
> .vid_begin = vid,
> .vid_end = vid,
> };
> - int err;
>
> + return switchdev_port_obj_add(dev, &v.obj);
> +}
> +
> +static int br_switchdev_port_obj_del(struct net_device *dev, u16 vid)
> +{
> + struct switchdev_obj_port_vlan v = {
> + .obj.orig_dev = dev,
> + .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
> + .vid_begin = vid,
> + .vid_end = vid,
> + };
> +
> + return switchdev_port_obj_del(dev, &v.obj);
> +}
Shouldn't they be br_switchdev_port_vlan_add (or similar) implemented in
net/bridge/br_switchdev.c instead, since they are VLAN specific?
Other than that, the change looks good!
Thanks,
Vivien
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next 6/7] net: bridge: Notify about bridge VLANs
Hi Petr,
Petr Machata writes:
> A driver might need to react to changes in settings of brentry VLANs.
> Therefore send switchdev port notifications for these as well. Reuse
> SWITCHDEV_OBJ_ID_PORT_VLAN for this purpose. Listeners should use
> netif_is_bridge_master() on orig_dev to determine whether the
> notification is about a bridge port or a bridge.
>
> Signed-off-by: Petr Machata
> + } else {
> + err = br_switchdev_port_obj_add(dev, v->vid, flags);
> + if (err && err != -EOPNOTSUPP)
> + goto out;
> }
Except that br_switchdev_port_obj_add taking vid and flags arguments
seems confusing to me, the change looks good:
Reviewed-by: Vivien Didelot
Thanks,
Vivien
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
