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

[Vladimir Oltean]

On Thu, Sep 05, 2024 at 03:02:24PM +0800, Furong Xu wrote:
>  struct stmmac_fpe_cfg {
> -	bool enable;				/* FPE enable */
> -	bool hs_enable;				/* FPE handshake enable */
> -	enum stmmac_fpe_state lp_fpe_state;	/* Link Partner FPE state */
> -	enum stmmac_fpe_state lo_fpe_state;	/* Local station FPE state */
> +	/* Serialize access to MAC Merge state between ethtool requests
> +	 * and link state updates.
> +	 */
> +	spinlock_t lock;
> +
>  	u32 fpe_csr;				/* MAC_FPE_CTRL_STS reg cache */
> +	struct timer_list verify_timer;
> +	struct ethtool_mm_state state;

I don't know what to say about keeping a full-blown struct
ethtool_mm_state copy in struct stmmac_fpe_cfg.

You don't populate two of its members: tx_active and tx_min_frag_size,
and thus they would be invalid if anyone tried to access them. And two
more of its member variables are populated with driver-constant values:
max_verify_time and rx_min_frag_size.

This leaves only verify_time, verify_status, pmac_enabled, tx_enabled,
verify_enabled. Maybe it would be better to just open-code these
variables directly in struct stmmac_fpe_cfg.