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

Groups > linux.kernel > #1401040 > unrolled thread

[PATCH net] net: dsa: mv88e6xxx: remove bridge work

Back to article view | Back to linux.kernel

Contents

  [PATCH net] net: dsa: mv88e6xxx: remove bridge work Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2016-05-14 02:40 +0200
    Re: [PATCH net] net: dsa: mv88e6xxx: remove bridge work Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2016-05-14 03:30 +0200
      Re: [PATCH net] net: dsa: mv88e6xxx: remove bridge work David Miller <davem@davemloft.net> - 2016-05-16 19:50 +0200
        Re: [PATCH net] net: dsa: mv88e6xxx: remove bridge work Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2016-05-17 17:40 +0200
          Re: [PATCH net] net: dsa: mv88e6xxx: remove bridge work Andrew Lunn <andrew@lunn.ch> - 2016-05-17 18:10 +0200
            Re: [PATCH net] net: dsa: mv88e6xxx: remove bridge work Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2016-05-17 18:30 +0200
          Re: [PATCH net] net: dsa: mv88e6xxx: remove bridge work David Miller <davem@davemloft.net> - 2016-05-17 18:50 +0200

#1401040 — [PATCH net] net: dsa: mv88e6xxx: remove bridge work

Now that the bridge code defers the switchdev port state setting, there
is no need to defer the port STP state change within the mv88e6xxx code.
Thus get rid of the driver's bridge work code.

This also fixes a race condition where the DSA layer assumes that the
bridge code already set the unbridged port's STP state to Disabled
before restoring the Forwarding state.

As a consequence, this also fixes the FDB flush for the unbridged port
which now correctly occurs during the Forwarding to Disabled transition.

Fixes: 0bc05d585d38 ("switchdev: allow caller to explicitly request attr_set as deferred")
Reported-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 37 ++++++++-----------------------------
 drivers/net/dsa/mv88e6xxx.h |  5 -----
 2 files changed, 8 insertions(+), 34 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index a3f0e7e..ba9dfc9 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1373,6 +1373,7 @@ static void mv88e6xxx_port_stp_state_set(struct dsa_switch *ds, int port,
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 	int stp_state;
+	int err;
 
 	if (!mv88e6xxx_has(ps, MV88E6XXX_FLAG_PORTSTATE))
 		return;
@@ -1394,12 +1395,13 @@ static void mv88e6xxx_port_stp_state_set(struct dsa_switch *ds, int port,
 		break;
 	}
 
-	/* mv88e6xxx_port_stp_state_set may be called with softirqs disabled,
-	 * so we can not update the port state directly but need to schedule it.
-	 */
-	ps->ports[port].state = stp_state;
-	set_bit(port, ps->port_state_update_mask);
-	schedule_work(&ps->bridge_work);
+	mutex_lock(&ps->smi_mutex);
+	err = _mv88e6xxx_port_state(ps, port, stp_state);
+	mutex_unlock(&ps->smi_mutex);
+
+	if (err)
+		netdev_err(ds->ports[port], "failed to update state to %s\n",
+			   mv88e6xxx_port_state_names[stp_state]);
 }
 
 static int _mv88e6xxx_port_pvid(struct mv88e6xxx_priv_state *ps, int port,
@@ -2535,27 +2537,6 @@ static void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port)
 	mutex_unlock(&ps->smi_mutex);
 }
 
-static void mv88e6xxx_bridge_work(struct work_struct *work)
-{
-	struct mv88e6xxx_priv_state *ps;
-	struct dsa_switch *ds;
-	int port;
-
-	ps = container_of(work, struct mv88e6xxx_priv_state, bridge_work);
-	ds = ps->ds;
-
-	mutex_lock(&ps->smi_mutex);
-
-	for (port = 0; port < ps->info->num_ports; ++port)
-		if (test_and_clear_bit(port, ps->port_state_update_mask) &&
-		    _mv88e6xxx_port_state(ps, port, ps->ports[port].state))
-			netdev_warn(ds->ports[port],
-				    "failed to update state to %s\n",
-				    mv88e6xxx_port_state_names[ps->ports[port].state]);
-
-	mutex_unlock(&ps->smi_mutex);
-}
-
 static int _mv88e6xxx_phy_page_write(struct mv88e6xxx_priv_state *ps,
 				     int port, int page, int reg, int val)
 {
@@ -3145,8 +3126,6 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 
 	ps->ds = ds;
 
-	INIT_WORK(&ps->bridge_work, mv88e6xxx_bridge_work);
-
 	if (mv88e6xxx_has(ps, MV88E6XXX_FLAG_EEPROM))
 		mutex_init(&ps->eeprom_mutex);
 
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 40e8721..36d0e15 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -543,7 +543,6 @@ struct mv88e6xxx_vtu_stu_entry {
 
 struct mv88e6xxx_priv_port {
 	struct net_device *bridge_dev;
-	u8 state;
 };
 
 struct mv88e6xxx_priv_state {
@@ -593,10 +592,6 @@ struct mv88e6xxx_priv_state {
 
 	struct mv88e6xxx_priv_port	ports[DSA_MAX_PORTS];
 
-	DECLARE_BITMAP(port_state_update_mask, DSA_MAX_PORTS);
-
-	struct work_struct bridge_work;
-
 	/* A switch may have a GPIO line tied to its reset pin. Parse
 	 * this from the device tree, and use it before performing
 	 * switch soft reset.
-- 
2.8.2

[toc] | [next] | [standalone]

#1401043

Hi David,

Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:

> Now that the bridge code defers the switchdev port state setting, there
> is no need to defer the port STP state change within the mv88e6xxx code.
> Thus get rid of the driver's bridge work code.
>
> This also fixes a race condition where the DSA layer assumes that the
> bridge code already set the unbridged port's STP state to Disabled
> before restoring the Forwarding state.
>
> As a consequence, this also fixes the FDB flush for the unbridged port
> which now correctly occurs during the Forwarding to Disabled transition.
>
> Fixes: 0bc05d585d38 ("switchdev: allow caller to explicitly request attr_set as deferred")
> Reported-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

This patch doesn't apply to -net, only applies to net-next...

How should I handle that, do I resend a patch for net-next with the good
subject prefix, and a v2 for -net?

Sorry for the noise,

      Vivien

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

#1401662

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Fri, 13 May 2016 21:28:28 -0400

> Hi David,
> 
> Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:
> 
>> Now that the bridge code defers the switchdev port state setting, there
>> is no need to defer the port STP state change within the mv88e6xxx code.
>> Thus get rid of the driver's bridge work code.
>>
>> This also fixes a race condition where the DSA layer assumes that the
>> bridge code already set the unbridged port's STP state to Disabled
>> before restoring the Forwarding state.
>>
>> As a consequence, this also fixes the FDB flush for the unbridged port
>> which now correctly occurs during the Forwarding to Disabled transition.
>>
>> Fixes: 0bc05d585d38 ("switchdev: allow caller to explicitly request attr_set as deferred")
>> Reported-by: Andrew Lunn <andrew@lunn.ch>
>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> 
> This patch doesn't apply to -net, only applies to net-next...
> 
> How should I handle that, do I resend a patch for net-next with the good
> subject prefix, and a v2 for -net?

I applied this to net-next, thanks.

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

#1402415

Hi David,

David Miller <davem@davemloft.net> writes:

> From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Date: Fri, 13 May 2016 21:28:28 -0400
>
>> Hi David,
>> 
>> Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:
>> 
>>> Now that the bridge code defers the switchdev port state setting, there
>>> is no need to defer the port STP state change within the mv88e6xxx code.
>>> Thus get rid of the driver's bridge work code.
>>>
>>> This also fixes a race condition where the DSA layer assumes that the
>>> bridge code already set the unbridged port's STP state to Disabled
>>> before restoring the Forwarding state.
>>>
>>> As a consequence, this also fixes the FDB flush for the unbridged port
>>> which now correctly occurs during the Forwarding to Disabled transition.
>>>
>>> Fixes: 0bc05d585d38 ("switchdev: allow caller to explicitly request attr_set as deferred")
>>> Reported-by: Andrew Lunn <andrew@lunn.ch>
>>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> 
>> This patch doesn't apply to -net, only applies to net-next...
>> 
>> How should I handle that, do I resend a patch for net-next with the good
>> subject prefix, and a v2 for -net?
>
> I applied this to net-next, thanks.

Do we want to send this fix to -net as well?

Thanks,

        Vivien

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

#1402447

On Tue, May 17, 2016 at 11:39:23AM -0400, Vivien Didelot wrote:
> Hi David,
> 
> David Miller <davem@davemloft.net> writes:
> 
> > From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> > Date: Fri, 13 May 2016 21:28:28 -0400
> >
> >> Hi David,
> >> 
> >> Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:
> >> 
> >>> Now that the bridge code defers the switchdev port state setting, there
> >>> is no need to defer the port STP state change within the mv88e6xxx code.
> >>> Thus get rid of the driver's bridge work code.
> >>>
> >>> This also fixes a race condition where the DSA layer assumes that the
> >>> bridge code already set the unbridged port's STP state to Disabled
> >>> before restoring the Forwarding state.
> >>>
> >>> As a consequence, this also fixes the FDB flush for the unbridged port
> >>> which now correctly occurs during the Forwarding to Disabled transition.
> >>>
> >>> Fixes: 0bc05d585d38 ("switchdev: allow caller to explicitly request attr_set as deferred")
> >>> Reported-by: Andrew Lunn <andrew@lunn.ch>
> >>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> >> 
> >> This patch doesn't apply to -net, only applies to net-next...
> >> 
> >> How should I handle that, do I resend a patch for net-next with the good
> >> subject prefix, and a v2 for -net?
> >
> > I applied this to net-next, thanks.
> 
> Do we want to send this fix to -net as well?

Hi Vivien

I don't see this bug as being highly critical that it needs to be
fixed immediately.

I would suggest we wait until -rc1 is out, and then produce a backport
version. Given the changes we have made to that driver, there is
little chance the existing fix will cherry-pick backwards.

       Andrew

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

#1402454

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>> Do we want to send this fix to -net as well?
>
> I don't see this bug as being highly critical that it needs to be
> fixed immediately.
>
> I would suggest we wait until -rc1 is out, and then produce a backport
> version. Given the changes we have made to that driver, there is
> little chance the existing fix will cherry-pick backwards.

Sounds good to me!

Thanks,

        Vivien

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

#1402465

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Tue, 17 May 2016 11:39:23 -0400

> Do we want to send this fix to -net as well?

net isn't open and is irrelevant right now, everything goes through the net-next
tree since we are in the merge window.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web