On Mon, 15 Feb 2016 15:42:01 +0000, Robert Shearman wrote: > +static const char *lwtunnel_encap_str(enum lwtunnel_encap_types encap_type) > +{ > + switch (encap_type) { > + case LWTUNNEL_ENCAP_MPLS: > + return "LWTUNNEL_ENCAP_MPLS"; > + case LWTUNNEL_ENCAP_IP: > + return "LWTUNNEL_ENCAP_IP"; > + case LWTUNNEL_ENCAP_ILA: > + return "LWTUNNEL_ENCAP_ILA"; > + case LWTUNNEL_ENCAP_IP6: > + return "LWTUNNEL_ENCAP_IP6"; > + case LWTUNNEL_ENCAP_NONE: > + case __LWTUNNEL_ENCAP_MAX: > + /* should not have got here */ > + break; > + } > + WARN_ON(1); > + return "LWTUNNEL_ENCAP_NONE"; > +} > + > +#endif /* CONFIG_MODULES */ > + > struct lwtunnel_state *lwtunnel_state_alloc(int encap_len) > { > struct lwtunnel_state *lws; > @@ -85,6 +109,14 @@ int lwtunnel_build_state(struct net_device *dev, u16 > encap_type, > ret = -EOPNOTSUPP; > rcu_read_lock(); > ops = rcu_dereference(lwtun_encaps[encap_type]); > +#ifdef CONFIG_MODULES > + if (!ops) { > + rcu_read_unlock(); > + request_module("rtnl-lwt-%s", lwtunnel_encap_str(encap_type));
Why the repeating of "lwt"/"LWTUNNEL" and the unnecessary "ENCAP"? Wouldn't be lwtunnel_encap_str returning just "MPLS" or "ILA" enough? I don't have any strong preference here, it just looks weird to me. Maybe there's a reason. Also, this doesn't affect IP lwtunnels, i.e. LWTUNNEL_ENCAP_IP and LWTUNNEL_ENCAP_IP6. Should we just return NULL from lwtunnel_encap_str in such cases (plus unknown encap_type) and WARN on the NULL here? Jiri -- Jiri Benc