Re: [PATCH net-next v7 3/7] net: stmmac: refactor FPE verification process

[Vladimir Oltean]

On Wed, Sep 04, 2024 at 05:21:18PM +0800, Furong Xu wrote:
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 3072ad33b105..e2f933353f40 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -969,17 +969,30 @@ static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
>  static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
>  {
>  	struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg;
> -	enum stmmac_fpe_state *lo_state = &fpe_cfg->lo_fpe_state;
> -	enum stmmac_fpe_state *lp_state = &fpe_cfg->lp_fpe_state;
> -	bool *hs_enable = &fpe_cfg->hs_enable;
> +	unsigned long flags;
>  
> -	if (is_up && *hs_enable) {
> -		stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg,
> -					MPACKET_VERIFY);
> +	del_timer_sync(&fpe_cfg->verify_timer);

Interesting comments in include/linux/timer.h:
 * Do not use in new code. Use timer_delete_sync() instead.

Also, interesting comment in the timer_delete_sync() kernel-doc:
 Callers must prevent restarting of the timer, otherwise this function is meaningless.

I don't think you have any restart prevention mechanism. So between the
timer deletion and the spin_lock_irqsave(), another thread has enough
time to acquire fpe_cfg->lock first, and run stmmac_fpe_verify_timer_arm().