Re: [PATCH net v2 3/6] net: stmmac: Move double VLAN handling to a dedicated op

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi,

On 8/21/26 19:09, Ovidiu Panait wrote:
> The double VLAN bits (EDVLP, ESVL, DOVLTC) are currently handled inside
> update_vlan_hash(). This ties the double VLAN state to the hash filter
> update, even though the two features are independent: hash filtering is
> controlled by dma_cap.vlhash and double vlan by dma_cap.dvlan.
> 
> In preparation for removing double vlan support from dwmac4, move the
> double vlan logic into a separate update_dvlan_state() VLAN operation.
> 
> Signed-off-by: Ovidiu Panait <ovidiu.panait.rb@renesas.com>

This is some nice code simplification, no more is_double everywhere, thanks!

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

> ---
> v2 changes: None.
> 
>  drivers/net/ethernet/stmicro/stmmac/hwif.h    |  6 ++-
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c |  6 +--
>  .../net/ethernet/stmicro/stmmac/stmmac_vlan.c | 45 ++++++++-----------
>  3 files changed, 25 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> index 6f26dbf95ce1..66837caafa84 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> @@ -632,8 +632,8 @@ struct stmmac_est_ops {
>  
>  struct stmmac_vlan_ops {
>  	/* VLAN */
> -	void (*update_vlan_hash)(struct mac_device_info *hw, u32 hash,
> -				 bool is_double);
> +	void (*update_vlan_hash)(struct mac_device_info *hw, u32 hash);
> +	void (*update_dvlan_state)(struct mac_device_info *hw, bool enable);
>  	void (*enable_vlan)(struct mac_device_info *hw, u32 type);
>  	void (*rx_hw_vlan)(struct mac_device_info *hw, struct dma_desc *rx_desc,
>  			   struct sk_buff *skb);
> @@ -650,6 +650,8 @@ struct stmmac_vlan_ops {
>  
>  #define stmmac_update_vlan_hash(__priv, __args...) \
>  	stmmac_do_void_callback(__priv, vlan, update_vlan_hash, __args)
> +#define stmmac_update_dvlan_state(__priv, __args...) \
> +	stmmac_do_void_callback(__priv, vlan, update_dvlan_state, __args)
>  #define stmmac_enable_vlan(__priv, __args...) \
>  	stmmac_do_void_callback(__priv, vlan, enable_vlan, __args)
>  #define stmmac_rx_hw_vlan(__priv, __args...) \
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 880cf3fab913..802f9e67a4bc 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6820,10 +6820,10 @@ static int stmmac_vlan_update(struct stmmac_priv *priv, bool is_double)
>  	if (!netif_running(priv->dev))
>  		return 0;
>  
> -	if (!priv->dma_cap.dvlan)
> -		is_double = false;
> +	if (priv->dma_cap.dvlan)
> +		stmmac_update_dvlan_state(priv, priv->hw, is_double);
>  
> -	return stmmac_update_vlan_hash(priv, priv->hw, hash, is_double);
> +	return stmmac_update_vlan_hash(priv, priv->hw, hash);
>  }
>  
>  /* FIXME: This may need RXC to be running, but it may be called with BH
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c
> index 983a90cb9767..1e47ae62093e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c
> @@ -161,8 +161,20 @@ static void vlan_restore_hw_rx_fltr(struct net_device *dev,
>  		vlan_write_filter(dev, hw, i, hw->vlan_filter[i]);
>  }
>  
> -static void vlan_update_hash(struct mac_device_info *hw, u32 hash,
> -			     bool is_double)
> +static void vlan_update_dvlan_state(struct mac_device_info *hw, bool enable)
> +{
> +	void __iomem *ioaddr = hw->pcsr;
> +	u32 value;
> +
> +	value = readl(ioaddr + VLAN_TAG);
> +	if (enable)
> +		value |= VLAN_EDVLP | VLAN_ESVL | VLAN_DOVLTC;
> +	else
> +		value &= ~(VLAN_EDVLP | VLAN_ESVL | VLAN_DOVLTC);
> +	writel(value, ioaddr + VLAN_TAG);
> +}
> +
> +static void vlan_update_hash(struct mac_device_info *hw, u32 hash)
>  {
>  	void __iomem *ioaddr = hw->pcsr;
>  	u32 value;
> @@ -173,21 +185,9 @@ static void vlan_update_hash(struct mac_device_info *hw, u32 hash,
>  
>  	if (hash) {
>  		value |= VLAN_VTHM | VLAN_ETV;
> -		if (is_double) {
> -			value |= VLAN_EDVLP;
> -			value |= VLAN_ESVL;
> -			value |= VLAN_DOVLTC;
> -		} else {
> -			value &= ~VLAN_EDVLP;
> -			value &= ~VLAN_ESVL;
> -			value &= ~VLAN_DOVLTC;
> -		}
> -
>  		writel(value, ioaddr + VLAN_TAG);
>  	} else {
>  		value &= ~(VLAN_VTHM | VLAN_ETV);
> -		value &= ~(VLAN_EDVLP | VLAN_ESVL);
> -		value &= ~VLAN_DOVLTC;
>  		value &= ~VLAN_VID;
>  
>  		writel(value, ioaddr + VLAN_TAG);
> @@ -236,8 +236,7 @@ static void vlan_set_hw_mode(struct mac_device_info *hw)
>  	writel(value, ioaddr + VLAN_TAG);
>  }
>  
> -static void dwxgmac2_update_vlan_hash(struct mac_device_info *hw, u32 hash,
> -				      bool is_double)
> +static void dwxgmac2_update_vlan_hash(struct mac_device_info *hw, u32 hash)
>  {
>  	void __iomem *ioaddr = hw->pcsr;
>  
> @@ -253,15 +252,6 @@ static void dwxgmac2_update_vlan_hash(struct mac_device_info *hw, u32 hash,
>  		value = readl(ioaddr + VLAN_TAG);
>  
>  		value |= VLAN_VTHM | VLAN_ETV;
> -		if (is_double) {
> -			value |= VLAN_EDVLP;
> -			value |= VLAN_ESVL;
> -			value |= VLAN_DOVLTC;
> -		} else {
> -			value &= ~VLAN_EDVLP;
> -			value &= ~VLAN_ESVL;
> -			value &= ~VLAN_DOVLTC;
> -		}
>  
>  		value &= ~VLAN_VID;
>  		writel(value, ioaddr + VLAN_TAG);
> @@ -275,8 +265,6 @@ static void dwxgmac2_update_vlan_hash(struct mac_device_info *hw, u32 hash,
>  		value = readl(ioaddr + VLAN_TAG);
>  
>  		value &= ~(VLAN_VTHM | VLAN_ETV);
> -		value &= ~(VLAN_EDVLP | VLAN_ESVL);
> -		value &= ~VLAN_DOVLTC;
>  		value &= ~VLAN_VID;
>  
>  		writel(value, ioaddr + VLAN_TAG);
> @@ -285,6 +273,7 @@ static void dwxgmac2_update_vlan_hash(struct mac_device_info *hw, u32 hash,
>  
>  const struct stmmac_vlan_ops dwmac_vlan_ops = {
>  	.update_vlan_hash = vlan_update_hash,
> +	.update_dvlan_state = vlan_update_dvlan_state,
>  	.enable_vlan = vlan_enable,
>  	.add_hw_vlan_rx_fltr = vlan_add_hw_rx_fltr,
>  	.del_hw_vlan_rx_fltr = vlan_del_hw_rx_fltr,
> @@ -295,11 +284,13 @@ const struct stmmac_vlan_ops dwmac_vlan_ops = {
>  
>  const struct stmmac_vlan_ops dwxlgmac2_vlan_ops = {
>  	.update_vlan_hash = dwxgmac2_update_vlan_hash,
> +	.update_dvlan_state = vlan_update_dvlan_state,
>  	.enable_vlan = vlan_enable,
>  };
>  
>  const struct stmmac_vlan_ops dwxgmac210_vlan_ops = {
>  	.update_vlan_hash = dwxgmac2_update_vlan_hash,
> +	.update_dvlan_state = vlan_update_dvlan_state,
>  	.enable_vlan = vlan_enable,
>  	.add_hw_vlan_rx_fltr = vlan_add_hw_rx_fltr,
>  	.del_hw_vlan_rx_fltr = vlan_del_hw_rx_fltr,