Re: [PATCH net-next v3 1/6] net: stmmac: Introduce separate files for FPE implementation

Re: [PATCH net-next v3 1/6] net: stmmac: Introduce separate files for FPE implementation

[Vladimir Oltean]

On Wed, Oct 23, 2024 at 03:05:21PM +0800, Furong Xu wrote:
> By moving FPE related code info separate files, FPE implementation
> becomes a separate module initially.
> No functional change intended.
> 
> Signed-off-by: Furong Xu <0x1207@gmail.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next v3 3/6] net: stmmac: Refactor FPE functions to generic version

[Vladimir Oltean]

On Wed, Oct 23, 2024 at 03:05:23PM +0800, Furong Xu wrote:
> -void dwmac5_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
> -			  u32 num_txq, u32 num_rxq,
> +void stmmac_fpe_configure(struct stmmac_priv *priv, u32 num_txq, u32 num_rxq,
>  			  bool tx_enable, bool pmac_enable)
>  {
> +	struct stmmac_fpe_cfg *cfg = &priv->fpe_cfg;
> +	const struct stmmac_fpe_reg *reg = cfg->reg;
> +	void __iomem *ioaddr = priv->ioaddr;
>  	u32 value;
>  
> +	if (!reg)
> +		return;

What are all these "if (!reg) return" checks protecting against?
At all call sites you ensure that priv->dma_cap.fpesel is true.
If defense against driver writers is necessary, check only once at boot
time that if priv->dma_cap.fpesel is true, we have a way to handle it
(priv->fpe_cfg.reg is set). Or are there still supported DWMAC variants
with the FPE hardware capability but without driver support?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index ab547430a717..7874a955ab60 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5944,8 +5944,7 @@ static void stmmac_common_interrupt(struct stmmac_priv *priv)
>  				      &priv->xstats, tx_cnt);
>  
>  	if (priv->dma_cap.fpesel) {
> -		int status = stmmac_fpe_irq_status(priv, priv->ioaddr,
> -						   priv->dev);
> +		int status = stmmac_fpe_irq_status(priv);
>  
>  		stmmac_fpe_event_status(priv, status);
>  	}

I think this coding pattern is illogical, and the code refactoring makes
it more apparent. stmmac_common_interrupt() does nothing with "status",
it just takes it as a return code from stmmac_fpe_irq_status(), and
passes it to stmmac_fpe_event_status(), both of which are in
stmmac_fpe.c. Why isn't there a direct tail call from one function to
the other, to simplify the external API exposed by stmmac_fpe.h?

> -- 
> 2.34.1
>