| Message ID | 20220209101823.1270489-1-horatiu.vultur@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: lan966x: Fix when CONFIG_IPV6 is not set | expand |
On Wed, Feb 09, 2022 at 11:18:23AM +0100, Horatiu Vultur wrote: > When CONFIG_IPV6 is not set, then the compilation of the lan966x driver > fails with the following error: > > drivers/net/ethernet/microchip/lan966x/lan966x_main.c:444: undefined > reference to `ipv6_mc_check_mld' > > The fix consists in adding #ifdef around this code. It might be better to add a stub function for when IPv6 is disabled. We try to avoid #if in C code. Andrew
On Wed, 9 Feb 2022 14:54:23 +0100 Andrew Lunn wrote: > On Wed, Feb 09, 2022 at 11:18:23AM +0100, Horatiu Vultur wrote: > > When CONFIG_IPV6 is not set, then the compilation of the lan966x driver compilation or linking? > > fails with the following error: > > > > drivers/net/ethernet/microchip/lan966x/lan966x_main.c:444: undefined > > reference to `ipv6_mc_check_mld' > > > > The fix consists in adding #ifdef around this code. > > It might be better to add a stub function for when IPv6 is > disabled. We try to avoid #if in C code. If it's linking we can do: if (IS_ENABLED(CONFIG_IPV6) && skb->protocol == htons(ETH_P_IPV6) && ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr) && !ipv6_mc_check_mld(skb)) return false; But beware that IPV6 can be a module, you may need a Kconfig dependency.
The 02/09/2022 18:06, Jakub Kicinski wrote: Hi Andrew, Jakub > > On Wed, 9 Feb 2022 14:54:23 +0100 Andrew Lunn wrote: > > On Wed, Feb 09, 2022 at 11:18:23AM +0100, Horatiu Vultur wrote: > > > When CONFIG_IPV6 is not set, then the compilation of the lan966x driver > > compilation or linking? It is a linking error. I will fix in the next version > > > > fails with the following error: > > > > > > drivers/net/ethernet/microchip/lan966x/lan966x_main.c:444: undefined > > > reference to `ipv6_mc_check_mld' > > > > > > The fix consists in adding #ifdef around this code. > > > > It might be better to add a stub function for when IPv6 is > > disabled. We try to avoid #if in C code. What do you think if I do something like this in the lan966x_main.h --- #if IS_ENABLED(CONFIG_IPV6) static inline bool lan966x_hw_offload_ipv6(struct sk_buff *skb) { if (skb->protocol == htons(ETH_P_IPV6) && ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr) && !ipv6_mc_check_mld(skb)) return false; return true; } #else static inline bool lan966x_hw_offload_ipv6(struct sk_buff *skb) { return false; } #endif --- And then in lan966x_main.c just call this function. > > If it's linking we can do: > > if (IS_ENABLED(CONFIG_IPV6) && > skb->protocol == htons(ETH_P_IPV6) && > ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr) && > !ipv6_mc_check_mld(skb)) > return false; > > But beware that IPV6 can be a module, you may need a Kconfig dependency. I was also looking at other drivers on how they use 'ipv6_mc_check_mld'. Then I have seen that drivers/net/amt.c and net/bridge/br_multicast.c they wrap this function with #if. But then there is net/batman-adv/multicast.c which doesn't do that and it can compile and link without CONFIG_IPV6 and I just don't see how that is working.
> What do you think if I do something like this in the lan966x_main.h > > --- > #if IS_ENABLED(CONFIG_IPV6) > static inline bool lan966x_hw_offload_ipv6(struct sk_buff *skb) > { > if (skb->protocol == htons(ETH_P_IPV6) && > ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr) && > !ipv6_mc_check_mld(skb)) > return false; > > return true; > } > #else > static inline bool lan966x_hw_offload_ipv6(struct sk_buff *skb) > { > return false; > } > #endif > --- The reason we prefer not to use #if is that it reduced compile testing coverage. The block of code inside gets compiled a lot less. > And then in lan966x_main.c just call this function. > > > > > If it's linking we can do: > > > > if (IS_ENABLED(CONFIG_IPV6) && > > skb->protocol == htons(ETH_P_IPV6) && > > ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr) && > > !ipv6_mc_check_mld(skb)) > > return false; Jakub solution results in the code always being compiled, but the IS_ENABLED(CONFIG_IPV6) gets turned into a constant 0 or 1. The optimizer can then remove the whole block of code in the 0 case. > I was also looking at other drivers on how they use 'ipv6_mc_check_mld'. > Then I have seen that drivers/net/amt.c and net/bridge/br_multicast.c > they wrap this function with #if. > But then there is net/batman-adv/multicast.c which doesn't do that and > it can compile and link without CONFIG_IPV6 and I just don't see how > that is working. Maybe it is to do with this at the end of net/ip6/Makefile ifneq ($(CONFIG_IPV6),) obj-$(CONFIG_NET_UDP_TUNNEL) += ip6_udp_tunnel.o obj-y += mcast_snoop.o endif
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c index d62484f14564..526dc41e98f8 100644 --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c @@ -439,10 +439,12 @@ static bool lan966x_hw_offload(struct lan966x *lan966x, u32 port, ip_hdr(skb)->protocol == IPPROTO_IGMP) return false; +#if IS_ENABLED(CONFIG_IPV6) if (skb->protocol == htons(ETH_P_IPV6) && ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr) && !ipv6_mc_check_mld(skb)) return false; +#endif return true; }
When CONFIG_IPV6 is not set, then the compilation of the lan966x driver fails with the following error: drivers/net/ethernet/microchip/lan966x/lan966x_main.c:444: undefined reference to `ipv6_mc_check_mld' The fix consists in adding #ifdef around this code. Fixes: 47aeea0d57e80c ("net: lan966x: Implement the callback SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED") Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> --- drivers/net/ethernet/microchip/lan966x/lan966x_main.c | 2 ++ 1 file changed, 2 insertions(+)