diff mbox series

[net-next,2/4] net: dsa: automatically bring user ports down when master goes down

Message ID 20210127010028.1619443-3-olteanv@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Automatically manage DSA master interface state | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 36 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Vladimir Oltean Jan. 27, 2021, 1 a.m. UTC 
From: Vladimir Oltean <vladimir.oltean@nxp.com>

This is not fixing any actual bug that I know of, but having a DSA
interface that is up even when its lower (master) interface is down is
one of those things that just do not sound right.

Yes, DSA checks if the master is up before actually bringing the
user interface up, but nobody prevents bringing the master interface
down immediately afterwards... Then the user ports would attempt
dev_queue_xmit on an interface that is down, and wonder what's wrong.

This patch prevents that from happening. NETDEV_GOING_DOWN is the
notification emitted _before_ the master actually goes down, and we are
protected by the rtnl_mutex, so all is well.

$ ip link set eno2 down
[  763.672211] mscc_felix 0000:00:00.5 swp0: Link is Down
[  763.880137] mscc_felix 0000:00:00.5 swp1: Link is Down
[  764.078773] mscc_felix 0000:00:00.5 swp2: Link is Down
[  764.197106] mscc_felix 0000:00:00.5 swp3: Link is Down
[  764.299384] fsl_enetc 0000:00:00.2 eno2: Link is Down

For those of you reading this because you were doing switch testing
such as latency measurements for autonomously forwarded traffic, and you
needed a controlled environment with no extra packets sent by the
network stack, this patch breaks that, because now the user ports go
down too, which may shut down the PHY etc. But please don't do it like
that, just do instead:

tc qdisc add dev eno2 clsact
tc filter add dev eno2 egress flower action drop

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/slave.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

Comments

Andrew Lunn Jan. 28, 2021, 12:46 a.m. UTC | #1 
> +	case NETDEV_GOING_DOWN: {
> +		struct dsa_port *dp, *cpu_dp;
> +		struct dsa_switch_tree *dst;
> +		int err = 0;
> +
> +		if (!netdev_uses_dsa(dev))
> +			return NOTIFY_DONE;
> +
> +		cpu_dp = dev->dsa_ptr;
> +		dst = cpu_dp->ds->dst;
> +
> +		list_for_each_entry(dp, &dst->ports, list) {
> +			if (!dsa_is_user_port(dp->ds, dp->index)) {

!dsa_is_user_port() ??

That ! seems odd. 

> +				struct net_device *slave = dp->slave;
> +
> +				if (!(slave->flags & IFF_UP))
> +					continue;
> +
> +				err = dev_change_flags(slave,
> +						       slave->flags & ~IFF_UP,
> +						       NULL);

  Andrew
Vladimir Oltean Jan. 28, 2021, 12:50 a.m. UTC | #2 
On Thu, Jan 28, 2021 at 01:46:30AM +0100, Andrew Lunn wrote:
> > +	case NETDEV_GOING_DOWN: {
> > +		struct dsa_port *dp, *cpu_dp;
> > +		struct dsa_switch_tree *dst;
> > +		int err = 0;
> > +
> > +		if (!netdev_uses_dsa(dev))
> > +			return NOTIFY_DONE;
> > +
> > +		cpu_dp = dev->dsa_ptr;
> > +		dst = cpu_dp->ds->dst;
> > +
> > +		list_for_each_entry(dp, &dst->ports, list) {
> > +			if (!dsa_is_user_port(dp->ds, dp->index)) {
>
> !dsa_is_user_port() ??
>
> That ! seems odd.

Oops, that's something that I refactored at the last minute after I
prototyped the idea from:
			if (!dsa_is_user_port(dp->ds, dp->index))
				continue;
because it looked uglier that way.
Andrew Lunn Jan. 28, 2021, 12:52 a.m. UTC | #3 
On Thu, Jan 28, 2021 at 02:50:14AM +0200, Vladimir Oltean wrote:
> On Thu, Jan 28, 2021 at 01:46:30AM +0100, Andrew Lunn wrote:
> > > +	case NETDEV_GOING_DOWN: {
> > > +		struct dsa_port *dp, *cpu_dp;
> > > +		struct dsa_switch_tree *dst;
> > > +		int err = 0;
> > > +
> > > +		if (!netdev_uses_dsa(dev))
> > > +			return NOTIFY_DONE;
> > > +
> > > +		cpu_dp = dev->dsa_ptr;
> > > +		dst = cpu_dp->ds->dst;
> > > +
> > > +		list_for_each_entry(dp, &dst->ports, list) {
> > > +			if (!dsa_is_user_port(dp->ds, dp->index)) {
> >
> > !dsa_is_user_port() ??
> >
> > That ! seems odd.
> 
> Oops, that's something that I refactored at the last minute after I
> prototyped the idea from:
> 			if (!dsa_is_user_port(dp->ds, dp->index))
> 				continue;
> because it looked uglier that way.

I was guessing it would be something like that. With that fixed:

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Florian Fainelli Jan. 28, 2021, 1:43 a.m. UTC | #4 
On 1/27/2021 4:52 PM, Andrew Lunn wrote:
> On Thu, Jan 28, 2021 at 02:50:14AM +0200, Vladimir Oltean wrote:
>> On Thu, Jan 28, 2021 at 01:46:30AM +0100, Andrew Lunn wrote:
>>>> +	case NETDEV_GOING_DOWN: {
>>>> +		struct dsa_port *dp, *cpu_dp;
>>>> +		struct dsa_switch_tree *dst;
>>>> +		int err = 0;
>>>> +
>>>> +		if (!netdev_uses_dsa(dev))
>>>> +			return NOTIFY_DONE;
>>>> +
>>>> +		cpu_dp = dev->dsa_ptr;
>>>> +		dst = cpu_dp->ds->dst;
>>>> +
>>>> +		list_for_each_entry(dp, &dst->ports, list) {
>>>> +			if (!dsa_is_user_port(dp->ds, dp->index)) {
>>>
>>> !dsa_is_user_port() ??
>>>
>>> That ! seems odd.
>>
>> Oops, that's something that I refactored at the last minute after I
>> prototyped the idea from:
>> 			if (!dsa_is_user_port(dp->ds, dp->index))
>> 				continue;
>> because it looked uglier that way.
> 
> I was guessing it would be something like that. With that fixed:
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

When you fix it:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
diff mbox series

Patch

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 393294a53834..5e5798b46f34 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2073,6 +2073,36 @@  static int dsa_slave_netdevice_event(struct notifier_block *nb,
 		err = dsa_port_lag_change(dp, info->lower_state_info);
 		return notifier_from_errno(err);
 	}
+	case NETDEV_GOING_DOWN: {
+		struct dsa_port *dp, *cpu_dp;
+		struct dsa_switch_tree *dst;
+		int err = 0;
+
+		if (!netdev_uses_dsa(dev))
+			return NOTIFY_DONE;
+
+		cpu_dp = dev->dsa_ptr;
+		dst = cpu_dp->ds->dst;
+
+		list_for_each_entry(dp, &dst->ports, list) {
+			if (!dsa_is_user_port(dp->ds, dp->index)) {
+				struct net_device *slave = dp->slave;
+
+				if (!(slave->flags & IFF_UP))
+					continue;
+
+				err = dev_change_flags(slave,
+						       slave->flags & ~IFF_UP,
+						       NULL);
+				if (err)
+					break;
+			}
+		}
+
+		return notifier_from_errno(err);
+	}
+	default:
+		break;
 	}
 
 	return NOTIFY_DONE;