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

[David Miller]

From: Vivien Didelot 
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.

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

[David Miller]

From: Vivien Didelot 
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.

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

[Vivien Didelot]

Hi Andrew,

Andrew Lunn  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

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

[Vivien Didelot]

Hi Andrew,

Andrew Lunn  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

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

[Andrew Lunn]

On Tue, May 17, 2016 at 11:39:23AM -0400, Vivien Didelot wrote:
> Hi David,
> 
> David Miller  writes:
> 
> > From: Vivien Didelot 
> > Date: Fri, 13 May 2016 21:28:28 -0400
> >
> >> Hi David,
> >> 
> >> Vivien Didelot  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 
> >>> Signed-off-by: Vivien Didelot 
> >> 
> >> 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

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

[Andrew Lunn]

On Tue, May 17, 2016 at 11:39:23AM -0400, Vivien Didelot wrote:
> Hi David,
> 
> David Miller  writes:
> 
> > From: Vivien Didelot 
> > Date: Fri, 13 May 2016 21:28:28 -0400
> >
> >> Hi David,
> >> 
> >> Vivien Didelot  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 
> >>> Signed-off-by: Vivien Didelot 
> >> 
> >> 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

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

[Vivien Didelot]

Hi David,

David Miller  writes:

> From: Vivien Didelot 
> Date: Fri, 13 May 2016 21:28:28 -0400
>
>> Hi David,
>> 
>> Vivien Didelot  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 
>>> Signed-off-by: Vivien Didelot 
>> 
>> 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

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

[Vivien Didelot]

Hi David,

David Miller  writes:

> From: Vivien Didelot 
> Date: Fri, 13 May 2016 21:28:28 -0400
>
>> Hi David,
>> 
>> Vivien Didelot  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 
>>> Signed-off-by: Vivien Didelot 
>> 
>> 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

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

[David Miller]

From: Vivien Didelot 
Date: Fri, 13 May 2016 21:28:28 -0400

> Hi David,
> 
> Vivien Didelot  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 
>> Signed-off-by: Vivien Didelot 
> 
> 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.

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

[David Miller]

From: Vivien Didelot 
Date: Fri, 13 May 2016 21:28:28 -0400

> Hi David,
> 
> Vivien Didelot  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 
>> Signed-off-by: Vivien Didelot 
> 
> 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.

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

[Vivien Didelot]

Hi David,

Vivien Didelot  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 
> Signed-off-by: Vivien Didelot 

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

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

[Vivien Didelot]

Hi David,

Vivien Didelot  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 
> Signed-off-by: Vivien Didelot 

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

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

[Vivien Didelot]

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 
Signed-off-by: Vivien Didelot 
---
 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(>bridge_work);
+   mutex_lock(>smi_mutex);
+   err = _mv88e6xxx_port_state(ps, port, stp_state);
+   mutex_unlock(>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(>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(>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(>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(>bridge_work, mv88e6xxx_bridge_work);
-
if (mv88e6xxx_has(ps, MV88E6XXX_FLAG_EEPROM))
mutex_init(>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

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

[Vivien Didelot]

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 
Signed-off-by: Vivien Didelot 
---
 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(>bridge_work);
+   mutex_lock(>smi_mutex);
+   err = _mv88e6xxx_port_state(ps, port, stp_state);
+   mutex_unlock(>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(>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(>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(>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(>bridge_work, mv88e6xxx_bridge_work);
-
if (mv88e6xxx_has(ps, MV88E6XXX_FLAG_EEPROM))
mutex_init(>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