Messages in this thread | | | Date | Thu, 9 May 2024 12:10:56 +1100 | Subject | Re: [PATCH v3] net: stmmac: move the lock to struct plat_stmmacenet_data | From | wang xiaolei <> |
| |
On 5/8/24 8:56 PM, Serge Semin wrote: > CAUTION: This email comes from a non Wind River email account! > Do not click links or open attachments unless you recognize the sender and know the content is safe. > > On Wed, May 08, 2024 at 12:52:57PM +0800, 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 plat_stmmacenet_data. >> We also need to require 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 >> >> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> >> --- >> v1 -> v2: >> - move the lock to struct plat_stmmacenet_data >> v2 -> v3: >> - Add require the mutex lock for reinitialization >> >> .../net/ethernet/stmicro/stmmac/stmmac_ptp.c | 8 ++++---- >> .../net/ethernet/stmicro/stmmac/stmmac_tc.c | 18 ++++++++++-------- >> include/linux/stmmac.h | 2 +- >> 3 files changed, 15 insertions(+), 13 deletions(-) >> >> [...] >> >> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h >> index dfa1828cd756..316ff7eb8b33 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]; >> @@ -246,6 +245,7 @@ struct plat_stmmacenet_data { >> struct fwnode_handle *port_node; >> struct device_node *mdio_node; >> struct stmmac_dma_cfg *dma_cfg; >> + struct mutex lock; >> struct stmmac_est *est; >> struct stmmac_fpe_cfg *fpe_cfg; >> struct stmmac_safety_feature_cfg *safety_feat_cfg; > Seeing you are going to move things around I suggest to move the > entire stmmac_est instance out of the plat_stmmacenet_data structure > and place it in the stmmac_priv instead. Why? Because the EST configs > don't look as the platform config, but EST is enabled in runtime with > the settings retrieved for the TC TAPRIO feature also in runtime. So > it's better to have the EST-data preserved in the driver private date > instead of the platform data storage. You could move the structure > there and place the lock aside of it. Field name like "est_lock" might > be most suitable to be looking unified with the "ptp_lock" or > "aux_ts_lock". > > * The same, but with no lock-related thing should be done for the > * stmmac_safety_feature_cfg structure, > but it's unrelated to the subject...
I think this is good and I will send a v4 version out later, does anyone else have any other opinions?
thanks
xiaolei
> > -Serge(y) > >> -- >> 2.25.1 >> >>
| |