| Message ID | 20230415170551.3939607-3-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3ff468ef987e38740de9ca0a811c55e11bfb2141 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Ocelot/Felix driver support for preemptible traffic classes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 18 this patch: 18 |
| netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
| netdev/build_clang | success | Errors and warnings before: 18 this patch: 18 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 18 this patch: 18 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 88 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Sat, Apr 15, 2023 at 08:05:46PM +0300, Vladimir Oltean wrote: > Unfortunately, the workarounds for the hardware bugs make it pointless > to keep fine-grained locking for the MAC Merge state of each port. > > Our vsc9959_cut_through_fwd() implementation requires > ocelot->fwd_domain_lock to be held, in order to serialize with changes > to the bridging domains and to port speed changes (which affect which > ports can be cut-through). Simultaneously, the traffic classes which can > be cut-through cannot be preemptible at the same time, and this will > depend on the MAC Merge layer state (which changes from threaded > interrupt context). > > Since vsc9959_cut_through_fwd() would have to hold the mm->lock of all > ports for a correct and race-free implementation with respect to > ocelot_mm_irq(), in practice it means that any time a port's mm->lock is > held, it would potentially block holders of ocelot->fwd_domain_lock. > > In the interest of simple locking rules, make all MAC Merge layer state > changes (and preemptible traffic class changes) be serialized by the > ocelot->fwd_domain_lock. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Simon Horman <simon.horman@corigine.com>
On 4/15/2023 10:05 AM, Vladimir Oltean wrote: > Unfortunately, the workarounds for the hardware bugs make it pointless > to keep fine-grained locking for the MAC Merge state of each port. > > Our vsc9959_cut_through_fwd() implementation requires > ocelot->fwd_domain_lock to be held, in order to serialize with changes > to the bridging domains and to port speed changes (which affect which > ports can be cut-through). Simultaneously, the traffic classes which can > be cut-through cannot be preemptible at the same time, and this will > depend on the MAC Merge layer state (which changes from threaded > interrupt context). > > Since vsc9959_cut_through_fwd() would have to hold the mm->lock of all > ports for a correct and race-free implementation with respect to > ocelot_mm_irq(), in practice it means that any time a port's mm->lock is > held, it would potentially block holders of ocelot->fwd_domain_lock. > > In the interest of simple locking rules, make all MAC Merge layer state > changes (and preemptible traffic class changes) be serialized by the > ocelot->fwd_domain_lock. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
diff --git a/drivers/net/ethernet/mscc/ocelot_mm.c b/drivers/net/ethernet/mscc/ocelot_mm.c index ddaf1fb05e48..d2df47e6f8f6 100644 --- a/drivers/net/ethernet/mscc/ocelot_mm.c +++ b/drivers/net/ethernet/mscc/ocelot_mm.c @@ -56,8 +56,6 @@ static void ocelot_mm_update_port_status(struct ocelot *ocelot, int port) enum ethtool_mm_verify_status verify_status; u32 val; - mutex_lock(&mm->lock); - val = ocelot_port_readl(ocelot_port, DEV_MM_STATUS); verify_status = ocelot_mm_verify_status(val); @@ -88,16 +86,18 @@ static void ocelot_mm_update_port_status(struct ocelot *ocelot, int port) } ocelot_port_writel(ocelot_port, val, DEV_MM_STATUS); - - mutex_unlock(&mm->lock); } void ocelot_mm_irq(struct ocelot *ocelot) { int port; + mutex_lock(&ocelot->fwd_domain_lock); + for (port = 0; port < ocelot->num_phys_ports; port++) ocelot_mm_update_port_status(ocelot, port); + + mutex_unlock(&ocelot->fwd_domain_lock); } EXPORT_SYMBOL_GPL(ocelot_mm_irq); @@ -107,14 +107,11 @@ int ocelot_port_set_mm(struct ocelot *ocelot, int port, { struct ocelot_port *ocelot_port = ocelot->ports[port]; u32 mm_enable = 0, verify_disable = 0, add_frag_size; - struct ocelot_mm_state *mm; int err; if (!ocelot->mm_supported) return -EOPNOTSUPP; - mm = &ocelot->mm[port]; - err = ethtool_mm_frag_size_min_to_add(cfg->tx_min_frag_size, &add_frag_size, extack); if (err) @@ -129,7 +126,7 @@ int ocelot_port_set_mm(struct ocelot *ocelot, int port, if (!cfg->verify_enabled) verify_disable = DEV_MM_CONFIG_VERIF_CONFIG_PRM_VERIFY_DIS; - mutex_lock(&mm->lock); + mutex_lock(&ocelot->fwd_domain_lock); ocelot_port_rmwl(ocelot_port, mm_enable, DEV_MM_CONFIG_ENABLE_CONFIG_MM_TX_ENA | @@ -148,7 +145,7 @@ int ocelot_port_set_mm(struct ocelot *ocelot, int port, QSYS_PREEMPTION_CFG, port); - mutex_unlock(&mm->lock); + mutex_unlock(&ocelot->fwd_domain_lock); return 0; } @@ -166,7 +163,7 @@ int ocelot_port_get_mm(struct ocelot *ocelot, int port, mm = &ocelot->mm[port]; - mutex_lock(&mm->lock); + mutex_lock(&ocelot->fwd_domain_lock); val = ocelot_port_readl(ocelot_port, DEV_MM_ENABLE_CONFIG); state->pmac_enabled = !!(val & DEV_MM_CONFIG_ENABLE_CONFIG_MM_RX_ENA); @@ -185,7 +182,7 @@ int ocelot_port_get_mm(struct ocelot *ocelot, int port, state->verify_status = mm->verify_status; state->tx_active = mm->tx_active; - mutex_unlock(&mm->lock); + mutex_unlock(&ocelot->fwd_domain_lock); return 0; } @@ -209,7 +206,6 @@ int ocelot_mm_init(struct ocelot *ocelot) u32 val; mm = &ocelot->mm[port]; - mutex_init(&mm->lock); ocelot_port = ocelot->ports[port]; /* Update initial status variable for the diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h index eb8e3935375d..9599be6a0a39 100644 --- a/include/soc/mscc/ocelot.h +++ b/include/soc/mscc/ocelot.h @@ -744,7 +744,6 @@ struct ocelot_mirror { }; struct ocelot_mm_state { - struct mutex lock; enum ethtool_mm_verify_status verify_status; bool tx_active; };
Unfortunately, the workarounds for the hardware bugs make it pointless to keep fine-grained locking for the MAC Merge state of each port. Our vsc9959_cut_through_fwd() implementation requires ocelot->fwd_domain_lock to be held, in order to serialize with changes to the bridging domains and to port speed changes (which affect which ports can be cut-through). Simultaneously, the traffic classes which can be cut-through cannot be preemptible at the same time, and this will depend on the MAC Merge layer state (which changes from threaded interrupt context). Since vsc9959_cut_through_fwd() would have to hold the mm->lock of all ports for a correct and race-free implementation with respect to ocelot_mm_irq(), in practice it means that any time a port's mm->lock is held, it would potentially block holders of ocelot->fwd_domain_lock. In the interest of simple locking rules, make all MAC Merge layer state changes (and preemptible traffic class changes) be serialized by the ocelot->fwd_domain_lock. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- Diff: patch is new. drivers/net/ethernet/mscc/ocelot_mm.c | 20 ++++++++------------ include/soc/mscc/ocelot.h | 1 - 2 files changed, 8 insertions(+), 13 deletions(-)