Re: [PATCH v3] net: stmmac: move the lock to struct plat_stmmacenet_data

[wang xiaolei <xiaolei.wang@windriver.com>]

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