On Mon, May 13, 2024 at 09:43:45AM GMT, Xiaolei Wang wrote:
> Reinitialize the whole EST structure would also reset the mutex
> lock which is embedded in the EST structure, and then trigger
> the following warning. To address this, move the lock to struct
> stmmac_priv. We also need to reacquire the mutex lock when doing
> this initialization.
>
> DEBUG_LOCKS_WARN_ON(lock->magic != lock)
> WARNING: CPU: 3 PID: 505 at kernel/locking/mutex.c:587 __mutex_lock+0xd84/0x1068
> Modules linked in:
> CPU: 3 PID: 505 Comm: tc Not tainted 6.9.0-rc6-00053-g0106679839f7-dirty #29
> Hardware name: NXP i.MX8MPlus EVK board (DT)
> pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : __mutex_lock+0xd84/0x1068
> lr : __mutex_lock+0xd84/0x1068
> sp : ffffffc0864e3570
> x29: ffffffc0864e3570 x28: ffffffc0817bdc78 x27: 0000000000000003
> x26: ffffff80c54f1808 x25: ffffff80c9164080 x24: ffffffc080d723ac
> x23: 0000000000000000 x22: 0000000000000002 x21: 0000000000000000
> x20: 0000000000000000 x19: ffffffc083bc3000 x18: ffffffffffffffff
> x17: ffffffc08117b080 x16: 0000000000000002 x15: ffffff80d2d40000
> x14: 00000000000002da x13: ffffff80d2d404b8 x12: ffffffc082b5a5c8
> x11: ffffffc082bca680 x10: ffffffc082bb2640 x9 : ffffffc082bb2698
> x8 : 0000000000017fe8 x7 : c0000000ffffefff x6 : 0000000000000001
> x5 : ffffff8178fe0d48 x4 : 0000000000000000 x3 : 0000000000000027
> x2 : ffffff8178fe0d50 x1 : 0000000000000000 x0 : 0000000000000000
> Call trace:
> __mutex_lock+0xd84/0x1068
> mutex_lock_nested+0x28/0x34
> tc_setup_taprio+0x118/0x68c
> stmmac_setup_tc+0x50/0xf0
> taprio_change+0x868/0xc9c
>
> Fixes: b2aae654a479 ("net: stmmac: add mutex lock to protect est parameters")
> Signed-off-by: Xiaolei Wang <[email protected]>
> Reviewed-by: Simon Horman <[email protected]>
> Reviewed-by: Serge Semin <[email protected]>
Reviewed-by: Andrew Halaney <[email protected]>
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++
> .../net/ethernet/stmicro/stmmac/stmmac_ptp.c | 8 ++++----
> .../net/ethernet/stmicro/stmmac/stmmac_tc.c | 18 ++++++++++--------
> include/linux/stmmac.h | 1 -
> 4 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index dddcaa9220cc..64b21c83e2b8 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -261,6 +261,8 @@ struct stmmac_priv {
> struct stmmac_extra_stats xstats ____cacheline_aligned_in_smp;
> struct stmmac_safety_stats sstats;
> struct plat_stmmacenet_data *plat;
> + /* Protect est parameters */
> + struct mutex est_lock;
> struct dma_features dma_cap;
> struct stmmac_counters mmc;
> int hw_cap_support;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> index e04830a3a1fb..0c5aab6dd7a7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> @@ -70,11 +70,11 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
> /* If EST is enabled, disabled it before adjust ptp time. */
> if (priv->plat->est && priv->plat->est->enable) {
> est_rst = true;
> - mutex_lock(&priv->plat->est->lock);
> + mutex_lock(&priv->est_lock);
> priv->plat->est->enable = false;
> stmmac_est_configure(priv, priv, priv->plat->est,
> priv->plat->clk_ptp_rate);
> - mutex_unlock(&priv->plat->est->lock);
> + mutex_unlock(&priv->est_lock);
> }
>
> write_lock_irqsave(&priv->ptp_lock, flags);
> @@ -87,7 +87,7 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
> ktime_t current_time_ns, basetime;
> u64 cycle_time;
>
> - mutex_lock(&priv->plat->est->lock);
> + mutex_lock(&priv->est_lock);
> priv->ptp_clock_ops.gettime64(&priv->ptp_clock_ops, ¤t_time);
> current_time_ns = timespec64_to_ktime(current_time);
> time.tv_nsec = priv->plat->est->btr_reserve[0];
> @@ -104,7 +104,7 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
> priv->plat->est->enable = true;
> ret = stmmac_est_configure(priv, priv, priv->plat->est,
> priv->plat->clk_ptp_rate);
> - mutex_unlock(&priv->plat->est->lock);
> + mutex_unlock(&priv->est_lock);
> if (ret)
> netdev_err(priv->dev, "failed to configure EST\n");
> }
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index cce00719937d..620c16e9be3a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -1004,17 +1004,19 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
> if (!plat->est)
> return -ENOMEM;
>
> - mutex_init(&priv->plat->est->lock);
> + mutex_init(&priv->est_lock);
> } else {
> + mutex_lock(&priv->est_lock);
> memset(plat->est, 0, sizeof(*plat->est));
> + mutex_unlock(&priv->est_lock);
> }
>
> size = qopt->num_entries;
>
> - mutex_lock(&priv->plat->est->lock);
> + mutex_lock(&priv->est_lock);
> priv->plat->est->gcl_size = size;
> priv->plat->est->enable = qopt->cmd == TAPRIO_CMD_REPLACE;
> - mutex_unlock(&priv->plat->est->lock);
> + mutex_unlock(&priv->est_lock);
>
> for (i = 0; i < size; i++) {
> s64 delta_ns = qopt->entries[i].interval;
> @@ -1045,7 +1047,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
> priv->plat->est->gcl[i] = delta_ns | (gates << wid);
> }
>
> - mutex_lock(&priv->plat->est->lock);
> + mutex_lock(&priv->est_lock);
> /* Adjust for real system time */
> priv->ptp_clock_ops.gettime64(&priv->ptp_clock_ops, ¤t_time);
> current_time_ns = timespec64_to_ktime(current_time);
> @@ -1068,7 +1070,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
> tc_taprio_map_maxsdu_txq(priv, qopt);
>
> if (fpe && !priv->dma_cap.fpesel) {
> - mutex_unlock(&priv->plat->est->lock);
> + mutex_unlock(&priv->est_lock);
> return -EOPNOTSUPP;
> }
>
> @@ -1079,7 +1081,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>
> ret = stmmac_est_configure(priv, priv, priv->plat->est,
> priv->plat->clk_ptp_rate);
> - mutex_unlock(&priv->plat->est->lock);
> + mutex_unlock(&priv->est_lock);
> if (ret) {
> netdev_err(priv->dev, "failed to configure EST\n");
> goto disable;
> @@ -1096,7 +1098,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>
> disable:
> if (priv->plat->est) {
> - mutex_lock(&priv->plat->est->lock);
> + mutex_lock(&priv->est_lock);
> priv->plat->est->enable = false;
> stmmac_est_configure(priv, priv, priv->plat->est,
> priv->plat->clk_ptp_rate);
> @@ -1105,7 +1107,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
> priv->xstats.max_sdu_txq_drop[i] = 0;
> priv->xstats.mtl_est_txq_hlbf[i] = 0;
> }
> - mutex_unlock(&priv->plat->est->lock);
> + mutex_unlock(&priv->est_lock);
> }
>
> priv->plat->fpe_cfg->enable = false;
> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> index dfa1828cd756..c0d74f97fd18 100644
> --- a/include/linux/stmmac.h
> +++ b/include/linux/stmmac.h
> @@ -117,7 +117,6 @@ struct stmmac_axi {
>
> #define EST_GCL 1024
> struct stmmac_est {
> - struct mutex lock;
> int enable;
> u32 btr_reserve[2];
> u32 btr_offset[2];
> --
> 2.25.1
>