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

Vladimir Oltean
2024-09-04T14:58:42Z

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().