Re: [PATCH net-next 3/3] net: dsa: refine netdev event notifier

2016-03-14 Thread Ido Schimmel 
Sun, Mar 13, 2016 at 10:21:34PM IST, vivien.dide...@savoirfairelinux.com wrote:
>Rework the netdev event handler, similar to what the Mellanox Spectrum
>driver does, to easily welcome more events later (for example
>NETDEV_PRECHANGEUPPER) and use netdev helpers (such as
>netif_is_bridge_master).
>
>Signed-off-by: Vivien Didelot 

Acked-by: Ido Schimmel

Re: [PATCH net-next 3/3] net: dsa: refine netdev event notifier

2016-03-13 Thread Jiri Pirko 
Sun, Mar 13, 2016 at 09:21:34PM CET, vivien.dide...@savoirfairelinux.com wrote:
>Rework the netdev event handler, similar to what the Mellanox Spectrum
>driver does, to easily welcome more events later (for example
>NETDEV_PRECHANGEUPPER) and use netdev helpers (such as
>netif_is_bridge_master).
>
>Signed-off-by: Vivien Didelot 

Acked-by: Jiri Pirko

[PATCH net-next 3/3] net: dsa: refine netdev event notifier

2016-03-13 Thread Vivien Didelot 
Rework the netdev event handler, similar to what the Mellanox Spectrum
driver does, to easily welcome more events later (for example
NETDEV_PRECHANGEUPPER) and use netdev helpers (such as
netif_is_bridge_master).

Signed-off-by: Vivien Didelot 
---
 net/dsa/slave.c | 54 ++
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 54976c4..a78c2d1e 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -451,7 +451,7 @@ static int dsa_slave_bridge_port_join(struct net_device 
*dev,
if (ds->drv->port_bridge_join)
ret = ds->drv->port_bridge_join(ds, p->port, br);
 
-   return ret;
+   return ret == -EOPNOTSUPP ? 0 : ret;
 }
 
 static void dsa_slave_bridge_port_leave(struct net_device *dev)
@@ -1139,40 +1139,46 @@ static bool dsa_slave_dev_check(struct net_device *dev)
return dev->netdev_ops == _slave_netdev_ops;
 }
 
-static int dsa_slave_master_changed(struct net_device *dev)
+static int dsa_slave_port_upper_event(struct net_device *dev,
+ unsigned long event, void *ptr)
 {
-   struct net_device *master = netdev_master_upper_dev_get(dev);
-   struct dsa_slave_priv *p = netdev_priv(dev);
+   struct netdev_notifier_changeupper_info *info = ptr;
+   struct net_device *upper = info->upper_dev;
int err = 0;
 
-   if (master && master->rtnl_link_ops &&
-   !strcmp(master->rtnl_link_ops->kind, "bridge"))
-   err = dsa_slave_bridge_port_join(dev, master);
-   else if (dsa_port_is_bridged(p))
-   dsa_slave_bridge_port_leave(dev);
+   switch (event) {
+   case NETDEV_CHANGEUPPER:
+   if (netif_is_bridge_master(upper)) {
+   if (info->linking)
+   err = dsa_slave_bridge_port_join(dev, upper);
+   else
+   dsa_slave_bridge_port_leave(dev);
+   }
 
-   return err;
+   break;
+   }
+
+   return notifier_from_errno(err);
 }
 
-int dsa_slave_netdevice_event(struct notifier_block *unused,
- unsigned long event, void *ptr)
+static int dsa_slave_port_event(struct net_device *dev, unsigned long event,
+   void *ptr)
 {
-   struct net_device *dev;
-   int err = 0;
-
switch (event) {
case NETDEV_CHANGEUPPER:
-   dev = netdev_notifier_info_to_dev(ptr);
-   if (!dsa_slave_dev_check(dev))
-   goto out;
+   return dsa_slave_port_upper_event(dev, event, ptr);
+   }
 
-   err = dsa_slave_master_changed(dev);
-   if (err && err != -EOPNOTSUPP)
-   netdev_warn(dev, "failed to reflect master change\n");
+   return NOTIFY_DONE;
+}
 
-   break;
-   }
+int dsa_slave_netdevice_event(struct notifier_block *unused,
+ unsigned long event, void *ptr)
+{
+   struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+
+   if (dsa_slave_dev_check(dev))
+   return dsa_slave_port_event(dev, event, ptr);
 
-out:
return NOTIFY_DONE;
 }
-- 
2.7.2

Re: [RFC PATCH net-next 3/3] net: dsa: refine netdev event notifier

2016-03-13 Thread Vivien Didelot 
Hi Ido,

Ido Schimmel  writes:

>>+ case NETDEV_PRECHANGEUPPER:
>
> Why do you need this here? It seems you are always ignoring it in
> dsa_slave_port_upper_event()? Probably better to introduce it when you
> actually need it.
>
> Other than that, it looks good to me.

I've prepare a real v1 without this switch case locally, I'll send it in
a few moment unless there are other comments.

Thanks Ido!
Vivien

Re: [RFC PATCH net-next 3/3] net: dsa: refine netdev event notifier

2016-03-12 Thread Ido Schimmel 
Hi Vivien,

Sun, Mar 13, 2016 at 08:42:26AM IST, vivien.dide...@savoirfairelinux.com wrote:
>Rework the netdev event handler, similar to what the Mellanox Spectrum
>driver does, to eventually welcome NETDEV_PRECHANGEUPPER actions and use
>netdev helpers, such as netif_is_bridge_master.
>
>Signed-off-by: Vivien Didelot 
>---
> net/dsa/slave.c | 55 +++
> 1 file changed, 31 insertions(+), 24 deletions(-)
>
>diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>index 54976c4..27f7886 100644
>--- a/net/dsa/slave.c
>+++ b/net/dsa/slave.c
>@@ -451,7 +451,7 @@ static int dsa_slave_bridge_port_join(struct net_device 
>*dev,
>   if (ds->drv->port_bridge_join)
>   ret = ds->drv->port_bridge_join(ds, p->port, br);
> 
>-  return ret;
>+  return ret == -EOPNOTSUPP ? 0 : ret;
> }
> 
> static void dsa_slave_bridge_port_leave(struct net_device *dev)
>@@ -1139,40 +1139,47 @@ static bool dsa_slave_dev_check(struct net_device *dev)
>   return dev->netdev_ops == _slave_netdev_ops;
> }
> 
>-static int dsa_slave_master_changed(struct net_device *dev)
>+static int dsa_slave_port_upper_event(struct net_device *dev,
>+unsigned long event, void *ptr)
> {
>-  struct net_device *master = netdev_master_upper_dev_get(dev);
>-  struct dsa_slave_priv *p = netdev_priv(dev);
>+  struct netdev_notifier_changeupper_info *info = ptr;
>+  struct net_device *upper = info->upper_dev;
>   int err = 0;
> 
>-  if (master && master->rtnl_link_ops &&
>-  !strcmp(master->rtnl_link_ops->kind, "bridge"))
>-  err = dsa_slave_bridge_port_join(dev, master);
>-  else if (dsa_port_is_bridged(p))
>-  dsa_slave_bridge_port_leave(dev);
>+  switch (event) {
>+  case NETDEV_CHANGEUPPER:
>+  if (netif_is_bridge_master(upper)) {
>+  if (info->linking)
>+  err = dsa_slave_bridge_port_join(dev, upper);
>+  else
>+  dsa_slave_bridge_port_leave(dev);
>+  }
> 
>-  return err;
>+  break;
>+  }
>+
>+  return notifier_from_errno(err);
> }
> 
>-int dsa_slave_netdevice_event(struct notifier_block *unused,
>-unsigned long event, void *ptr)
>+static int dsa_slave_port_event(struct net_device *dev, unsigned long event,
>+  void *ptr)
> {
>-  struct net_device *dev;
>-  int err = 0;
>-
>   switch (event) {
>+  case NETDEV_PRECHANGEUPPER:

Why do you need this here? It seems you are always ignoring it in
dsa_slave_port_upper_event()? Probably better to introduce it when you
actually need it.

Other than that, it looks good to me.

>   case NETDEV_CHANGEUPPER:
>-  dev = netdev_notifier_info_to_dev(ptr);
>-  if (!dsa_slave_dev_check(dev))
>-  goto out;
>+  return dsa_slave_port_upper_event(dev, event, ptr);
>+  }
> 
>-  err = dsa_slave_master_changed(dev);
>-  if (err && err != -EOPNOTSUPP)
>-  netdev_warn(dev, "failed to reflect master change\n");
>+  return NOTIFY_DONE;
>+}
> 
>-  break;
>-  }
>+int dsa_slave_netdevice_event(struct notifier_block *unused,
>+unsigned long event, void *ptr)
>+{
>+  struct net_device *dev = netdev_notifier_info_to_dev(ptr);
>+
>+  if (dsa_slave_dev_check(dev))
>+  return dsa_slave_port_event(dev, event, ptr);
> 
>-out:
>   return NOTIFY_DONE;
> }
>-- 
>2.7.2
>

[RFC PATCH net-next 3/3] net: dsa: refine netdev event notifier

2016-03-12 Thread Vivien Didelot 
Rework the netdev event handler, similar to what the Mellanox Spectrum
driver does, to eventually welcome NETDEV_PRECHANGEUPPER actions and use
netdev helpers, such as netif_is_bridge_master.

Signed-off-by: Vivien Didelot 
---
 net/dsa/slave.c | 55 +++
 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 54976c4..27f7886 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -451,7 +451,7 @@ static int dsa_slave_bridge_port_join(struct net_device 
*dev,
if (ds->drv->port_bridge_join)
ret = ds->drv->port_bridge_join(ds, p->port, br);
 
-   return ret;
+   return ret == -EOPNOTSUPP ? 0 : ret;
 }
 
 static void dsa_slave_bridge_port_leave(struct net_device *dev)
@@ -1139,40 +1139,47 @@ static bool dsa_slave_dev_check(struct net_device *dev)
return dev->netdev_ops == _slave_netdev_ops;
 }
 
-static int dsa_slave_master_changed(struct net_device *dev)
+static int dsa_slave_port_upper_event(struct net_device *dev,
+ unsigned long event, void *ptr)
 {
-   struct net_device *master = netdev_master_upper_dev_get(dev);
-   struct dsa_slave_priv *p = netdev_priv(dev);
+   struct netdev_notifier_changeupper_info *info = ptr;
+   struct net_device *upper = info->upper_dev;
int err = 0;
 
-   if (master && master->rtnl_link_ops &&
-   !strcmp(master->rtnl_link_ops->kind, "bridge"))
-   err = dsa_slave_bridge_port_join(dev, master);
-   else if (dsa_port_is_bridged(p))
-   dsa_slave_bridge_port_leave(dev);
+   switch (event) {
+   case NETDEV_CHANGEUPPER:
+   if (netif_is_bridge_master(upper)) {
+   if (info->linking)
+   err = dsa_slave_bridge_port_join(dev, upper);
+   else
+   dsa_slave_bridge_port_leave(dev);
+   }
 
-   return err;
+   break;
+   }
+
+   return notifier_from_errno(err);
 }
 
-int dsa_slave_netdevice_event(struct notifier_block *unused,
- unsigned long event, void *ptr)
+static int dsa_slave_port_event(struct net_device *dev, unsigned long event,
+   void *ptr)
 {
-   struct net_device *dev;
-   int err = 0;
-
switch (event) {
+   case NETDEV_PRECHANGEUPPER:
case NETDEV_CHANGEUPPER:
-   dev = netdev_notifier_info_to_dev(ptr);
-   if (!dsa_slave_dev_check(dev))
-   goto out;
+   return dsa_slave_port_upper_event(dev, event, ptr);
+   }
 
-   err = dsa_slave_master_changed(dev);
-   if (err && err != -EOPNOTSUPP)
-   netdev_warn(dev, "failed to reflect master change\n");
+   return NOTIFY_DONE;
+}
 
-   break;
-   }
+int dsa_slave_netdevice_event(struct notifier_block *unused,
+ unsigned long event, void *ptr)
+{
+   struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+
+   if (dsa_slave_dev_check(dev))
+   return dsa_slave_port_event(dev, event, ptr);
 
-out:
return NOTIFY_DONE;
 }
-- 
2.7.2