| Message ID | 20201204170938.1415582-1-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: mscc: ocelot: install MAC addresses in .ndo_set_rx_mode from process context | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | fail | Patch does not apply to net |
| netdev/tree_selection | success | Clearly marked for net |
On 12/4/2020 9:09 AM, Vladimir Oltean wrote: > Currently ocelot_set_rx_mode calls ocelot_mact_learn directly, which has > a very nice ocelot_mact_wait_for_completion at the end. Introduced in > commit 639c1b2625af ("net: mscc: ocelot: Register poll timeout should be > wall time not attempts"), this function uses readx_poll_timeout which > triggers a lot of lockdep warnings and is also dangerous to use from > atomic context, leading to lockups and panics. > > Steen Hegelund added a poll timeout of 100 ms for checking the MAC > table, a duration which is clearly absurd to poll in atomic context. > So we need to defer the MAC table access to process context, which we do > via a dynamically allocated workqueue which contains all there is to > know about the MAC table operation it has to do. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> Did you want to have a Fixes tag to help identify how far back this needs to be back ported?
On Fri, Dec 04, 2020 at 09:34:54AM -0800, Florian Fainelli wrote: > On 12/4/2020 9:09 AM, Vladimir Oltean wrote: > > Currently ocelot_set_rx_mode calls ocelot_mact_learn directly, which has > > a very nice ocelot_mact_wait_for_completion at the end. Introduced in > > commit 639c1b2625af ("net: mscc: ocelot: Register poll timeout should be > > wall time not attempts"), this function uses readx_poll_timeout which > > triggers a lot of lockdep warnings and is also dangerous to use from > > atomic context, leading to lockups and panics. > > > > Steen Hegelund added a poll timeout of 100 ms for checking the MAC > > table, a duration which is clearly absurd to poll in atomic context. > > So we need to defer the MAC table access to process context, which we do > > via a dynamically allocated workqueue which contains all there is to > > know about the MAC table operation it has to do. > > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > Did you want to have a Fixes tag to help identify how far back this > needs to be back ported? I was on the fence about whether I should even target "net", considering that it's not a small patch. But the lockdep warnings are super annoying, I cannot do anything further on the ocelot switchdev driver with them. There's also a small concern I have that I should have taken a reference count on ocelot->dev using get_device, to avoid racing with the unbind. I think I'll send a v2 with this and a Fixes: tag.
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c index c65ae6f75a16..2f536692d61e 100644 --- a/drivers/net/ethernet/mscc/ocelot_net.c +++ b/drivers/net/ethernet/mscc/ocelot_net.c @@ -414,13 +414,82 @@ static int ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev) return NETDEV_TX_OK; } +enum ocelot_action_type { + OCELOT_MACT_LEARN, + OCELOT_MACT_FORGET, +}; + +struct ocelot_mact_work_ctx { + struct work_struct work; + struct ocelot *ocelot; + enum ocelot_action_type type; + union { + /* OCELOT_MACT_LEARN */ + struct { + int pgid; + enum macaccess_entry_type entry_type; + unsigned char addr[ETH_ALEN]; + u16 vid; + } learn; + /* OCELOT_MACT_FORGET */ + struct { + unsigned char addr[ETH_ALEN]; + u16 vid; + } forget; + }; +}; + +#define ocelot_work_to_ctx(x) \ + container_of((x), struct ocelot_mact_work_ctx, work) + +static void ocelot_mact_work(struct work_struct *work) +{ + struct ocelot_mact_work_ctx *w = ocelot_work_to_ctx(work); + struct ocelot *ocelot = w->ocelot; + + switch (w->type) { + case OCELOT_MACT_LEARN: + ocelot_mact_learn(ocelot, w->learn.pgid, w->learn.addr, + w->learn.vid, w->learn.entry_type); + break; + case OCELOT_MACT_FORGET: + ocelot_mact_forget(ocelot, w->forget.addr, w->forget.vid); + break; + default: + break; + }; + + kfree(w); +} + +static int ocelot_enqueue_mact_action(struct ocelot *ocelot, + const struct ocelot_mact_work_ctx *ctx) +{ + struct ocelot_mact_work_ctx *w = kmalloc(sizeof(*w), GFP_ATOMIC); + + if (!w) + return -ENOMEM; + + memcpy(w, ctx, sizeof(*w)); + w->ocelot = ocelot; + INIT_WORK(&w->work, ocelot_mact_work); + schedule_work(&w->work); + + return 0; +} + static int ocelot_mc_unsync(struct net_device *dev, const unsigned char *addr) { struct ocelot_port_private *priv = netdev_priv(dev); struct ocelot_port *ocelot_port = &priv->port; struct ocelot *ocelot = ocelot_port->ocelot; + struct ocelot_mact_work_ctx w; + + ether_addr_copy(w.forget.addr, addr); + w.forget.vid = ocelot_port->pvid_vlan.vid; + w.type = OCELOT_MACT_FORGET; - return ocelot_mact_forget(ocelot, addr, ocelot_port->pvid_vlan.vid); + return ocelot_enqueue_mact_action(ocelot, &w); } static int ocelot_mc_sync(struct net_device *dev, const unsigned char *addr) @@ -428,9 +497,15 @@ static int ocelot_mc_sync(struct net_device *dev, const unsigned char *addr) struct ocelot_port_private *priv = netdev_priv(dev); struct ocelot_port *ocelot_port = &priv->port; struct ocelot *ocelot = ocelot_port->ocelot; + struct ocelot_mact_work_ctx w; + + ether_addr_copy(w.learn.addr, addr); + w.learn.vid = ocelot_port->pvid_vlan.vid; + w.learn.pgid = PGID_CPU; + w.learn.entry_type = ENTRYTYPE_LOCKED; + w.type = OCELOT_MACT_LEARN; - return ocelot_mact_learn(ocelot, PGID_CPU, addr, - ocelot_port->pvid_vlan.vid, ENTRYTYPE_LOCKED); + return ocelot_enqueue_mact_action(ocelot, &w); } static void ocelot_set_rx_mode(struct net_device *dev)
Currently ocelot_set_rx_mode calls ocelot_mact_learn directly, which has a very nice ocelot_mact_wait_for_completion at the end. Introduced in commit 639c1b2625af ("net: mscc: ocelot: Register poll timeout should be wall time not attempts"), this function uses readx_poll_timeout which triggers a lot of lockdep warnings and is also dangerous to use from atomic context, leading to lockups and panics. Steen Hegelund added a poll timeout of 100 ms for checking the MAC table, a duration which is clearly absurd to poll in atomic context. So we need to defer the MAC table access to process context, which we do via a dynamically allocated workqueue which contains all there is to know about the MAC table operation it has to do. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- drivers/net/ethernet/mscc/ocelot_net.c | 81 +++++++++++++++++++++++++- 1 file changed, 78 insertions(+), 3 deletions(-)