On 25/11/14 07:58, Andy Gospodarek wrote:

On Tue, Nov 25, 2014 at 11:28:39AM +0100, Jiri Pirko wrote:

quoted

From: Scott Feldman <redacted>

To notify switch driver of change in STP state of bridge port, add new
.ndo op and provide switchdev wrapper func to call ndo op. Use it in bridge
code then.

Signed-off-by: Scott Feldman <redacted>
Signed-off-by: Jiri Pirko <jiri@resnulli.us>

[snip]

quoted

 #include "br_private.h"
 #include "br_private_stp.h"

@@ -39,6 +40,7 @@ void br_log_state(const struct net_bridge_port *p)
 void br_set_state(struct net_bridge_port *p, unsigned int state)
 {
 	p->state = state;
+	netdev_switch_port_stp_update(p->dev, state);

The only thing that concerns me about this patch is the fact that there
is nothing paying attention to the return code.

This means if *something* in the driver fails to set the STP state we
have no way to feed this information back to the user to let them know
that their hardware isn't exactly functioning as we expect.

This typically translates into an actual HW register write in general
(writing the STP algorithm result per-port), so although they typically
don't fail in most HW transports, I so no harm in returning something
useful just in case.

Once you do this, feel free to add a:

Acked-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks!

I do not expect that this first set would provide full feedback to
br_make_forwarding, br_make_blocking, etc, to allow spanning tree to
properly deal with the failure (that change is fine to add later), but a
short one-liner indicating that the call to the hardware  failed would e
good.  What about something simple like this:

void br_set_state(struct net_bridge_port *p, unsigned int state)
{
	int ret;
	p->state = state;
	ret = netdev_switch_port_stp_update(p->dev, state);
	if (ret && ret != -EOPNOTSUPP)
		br_warn(br, "error setting offload STP state for interface %s\n",
			p->dev->name);
}

quoted

 
 /* called under bridge lock */

diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 66973de..d162b21 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c

@@ -31,3 +31,22 @@ int netdev_switch_parent_id_get(struct net_device *dev,
 	return ops->ndo_switch_parent_id_get(dev, psid);
 }
 EXPORT_SYMBOL(netdev_switch_parent_id_get);
+
+/**
+ *	netdev_switch_port_stp_update - Notify switch device port of STP
+ *					state change
+ *	@dev: port device
+ *	@state: port STP state
+ *
+ *	Notify switch device port of bridge port STP state change.
+ */
+int netdev_switch_port_stp_update(struct net_device *dev, u8 state)
+{
+	const struct net_device_ops *ops = dev->netdev_ops;
+
+	if (!ops->ndo_switch_port_stp_update)
+		return -EOPNOTSUPP;
+	WARN_ON(!ops->ndo_switch_parent_id_get);
+	return ops->ndo_switch_port_stp_update(dev, state);
+}
+EXPORT_SYMBOL(netdev_switch_port_stp_update);

-- 
1.9.3