[PATCH net-next 0/2] Couple of vlan cleanups

[PATCH net-next 0/2] Couple of vlan cleanups

[Gal Pressman]

A couple of vlan cleanups/improvements:
First patch replaces BUG() calls with WARN_ON_ONCE(). This fixes a
compilation warning/error from objtool, and in general the usage of
BUG() should be avoided.

The second patch uses the "kernel" way of testing whether an option is
configured as builtin/module, instead of open-coding it.

Gal Pressman (2):
  net: vlan: Replace BUG() with WARN_ON_ONCE() in vlan_dev_* stubs
  net: vlan: Use IS_ENABLED() helper for CONFIG_VLAN_8021Q guard

 include/linux/if_vlan.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
2.40.1

[PATCH net-next 1/2] net: vlan: Replace BUG() with WARN_ON_ONCE() in vlan_dev_* stubs

[Gal Pressman]

When CONFIG_VLAN_8021Q=n, a set of stub helpers are used, three of these
helpers use BUG() unconditionally.

This code should not be reached, as callers of these functions should
always check for is_vlan_dev() first, but the usage of BUG() is not
recommended, replace it with WARN_ON() instead.

This also resolves the following compilation error when:
* CONFIG_VLAN_8021Q=n
* CONFIG_OBJTOOL=y
* CONFIG_OBJTOOL_WERROR=y

drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.o: error: objtool: parse_mirred.isra.0+0x370: mlx5e_tc_act_vlan_add_push_action() missing __noreturn in .c/.h or NORETURN() in noreturns.h

Which is caused by the call to BUG(), which never returns.

Reviewed-by: Alex Lazar <alazar@nvidia.com>
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
Signed-off-by: Gal Pressman <gal@nvidia.com>
---
 include/linux/if_vlan.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 38456b42cdb5..9135cbe6ae92 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -254,19 +254,19 @@ vlan_for_each(struct net_device *dev,
 
 static inline struct net_device *vlan_dev_real_dev(const struct net_device *dev)
 {
-	BUG();
+	WARN_ON_ONCE(1);
 	return NULL;
 }
 
 static inline u16 vlan_dev_vlan_id(const struct net_device *dev)
 {
-	BUG();
+	WARN_ON_ONCE(1);
 	return 0;
 }
 
 static inline __be16 vlan_dev_vlan_proto(const struct net_device *dev)
 {
-	BUG();
+	WARN_ON_ONCE(1);
 	return 0;
 }
 
-- 
2.40.1

[PATCH net-next 2/2] net: vlan: Use IS_ENABLED() helper for CONFIG_VLAN_8021Q guard

[Gal Pressman]

The header currently tests the VLAN core with an explicit pair of 'if
defined' checks:
    #if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)

Instead, use IS_ENABLED() which is the kernel way to test whether an
option is configured as builtin/module.

This is purely cosmetic – no functional changes.

Reviewed-by: Alex Lazar <alazar@nvidia.com>
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
Signed-off-by: Gal Pressman <gal@nvidia.com>
---
 include/linux/if_vlan.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 9135cbe6ae92..981fa37be0bf 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -136,7 +136,7 @@ struct vlan_pcpu_stats {
 	u32			tx_dropped;
 };
 
-#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
+#if IS_ENABLED(CONFIG_VLAN_8021Q)
 
 extern struct net_device *__vlan_find_dev_deep_rcu(struct net_device *real_dev,
 					       __be16 vlan_proto, u16 vlan_id);
-- 
2.40.1

Re: [PATCH net-next 1/2] net: vlan: Replace BUG() with WARN_ON_ONCE() in vlan_dev_* stubs

[Jakub Kicinski]

On Tue, 10 Jun 2025 10:26:10 +0300 Gal Pressman wrote:
> This code should not be reached, as callers of these functions should
> always check for is_vlan_dev() first, but the usage of BUG() is not
> recommended, replace it with WARN_ON() instead.

Did you try something along the lines of:

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 38456b42cdb5..f24af2988fd9 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -81,7 +81,8 @@ extern void vlan_ioctl_set(int (*hook)(struct net *, void __user *));
 
 static inline bool is_vlan_dev(const struct net_device *dev)
 {
-        return dev->priv_flags & IFF_802_1Q_VLAN;
+        return IS_ENABLED(CONFIG_VLAN_8021Q) &&
+               dev->priv_flags & IFF_802_1Q_VLAN;
 }
 
 #define skb_vlan_tag_present(__skb)    (!!(__skb)->vlan_all)


This should let the compiler eliminate the dead code completely,
assuming it truly dead. We can still replace the BUG()s as 
a of cleanup but I think the above is strictly preferable as 
a solution to your problem?

Re: [PATCH net-next 1/2] net: vlan: Replace BUG() with WARN_ON_ONCE() in vlan_dev_* stubs

[Gal Pressman]

On 12/06/2025 3:15, Jakub Kicinski wrote:
> On Tue, 10 Jun 2025 10:26:10 +0300 Gal Pressman wrote:
>> This code should not be reached, as callers of these functions should
>> always check for is_vlan_dev() first, but the usage of BUG() is not
>> recommended, replace it with WARN_ON() instead.
> 
> Did you try something along the lines of:
> 
> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
> index 38456b42cdb5..f24af2988fd9 100644
> --- a/include/linux/if_vlan.h
> +++ b/include/linux/if_vlan.h
> @@ -81,7 +81,8 @@ extern void vlan_ioctl_set(int (*hook)(struct net *, void __user *));
>  
>  static inline bool is_vlan_dev(const struct net_device *dev)
>  {
> -        return dev->priv_flags & IFF_802_1Q_VLAN;
> +        return IS_ENABLED(CONFIG_VLAN_8021Q) &&
> +               dev->priv_flags & IFF_802_1Q_VLAN;
>  }
>  
>  #define skb_vlan_tag_present(__skb)    (!!(__skb)->vlan_all)
> 
> 
> This should let the compiler eliminate the dead code completely,
> assuming it truly dead. We can still replace the BUG()s as 
> a of cleanup but I think the above is strictly preferable as 
> a solution to your problem?

Makes sense.

If we're going in this direction, maybe just provide a stub
is_vlan_dev() for the !IS_ENABLED case?

Re: [PATCH net-next 1/2] net: vlan: Replace BUG() with WARN_ON_ONCE() in vlan_dev_* stubs

[Jakub Kicinski]

On Thu, 12 Jun 2025 13:32:33 +0300 Gal Pressman wrote:
> > This should let the compiler eliminate the dead code completely,
> > assuming it truly dead. We can still replace the BUG()s as 
> > a of cleanup but I think the above is strictly preferable as 
> > a solution to your problem?  
> 
> Makes sense.
> 
> If we're going in this direction, maybe just provide a stub
> is_vlan_dev() for the !IS_ENABLED case?

I guess it's a question of whether we prefer simpler expression
or fewer ifdefs. I picked fewer ifdefs, but no strong preference.