Re: [PATCH] net/netconsole: Support VLAN for netconsole

[Florian Fainelli]

On 12/10/2020 2:07 AM, Libing Zhou wrote:
> During kernel startup phase, current netconsole doesn’t support VLAN
> since there is no VLAN interface setup already.
> 
> This patch provides VLAN ID and PCP as optional boot/module parameters
> to support VLAN environment, thus kernel startup log can be retrieved
> via VLAN.
> 
> Signed-off-by: Libing Zhou <libing.zhou@xxxxxxxxxxxxxxx>
> ---
>  Documentation/networking/netconsole.rst | 10 ++++-
>  drivers/net/netconsole.c                |  3 +-
>  include/linux/netpoll.h                 |  3 ++
>  net/core/netpoll.c                      | 58 ++++++++++++++++++++++++-
>  4 files changed, 70 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/networking/netconsole.rst b/Documentation/networking/netconsole.rst
> index 1f5c4a04027c..a08387fcc3f0 100644
> --- a/Documentation/networking/netconsole.rst
> +++ b/Documentation/networking/netconsole.rst
> @@ -13,6 +13,8 @@ IPv6 support by Cong Wang <xiyou.wangcong@xxxxxxxxx>, Jan 1 2013
>  
>  Extended console support by Tejun Heo <tj@xxxxxxxxxx>, May 1 2015
>  
> +VLAN support by Libing Zhou <libing.zhou@xxxxxxxxxxxxxxx>, Dec 8 2020
> +
>  Please send bug reports to Matt Mackall <mpm@xxxxxxxxxxx>
>  Satyam Sharma <satyam.sharma@xxxxxxxxx>, and Cong Wang <xiyou.wangcong@xxxxxxxxx>
>  
> @@ -34,7 +36,7 @@ Sender and receiver configuration:
>  It takes a string configuration parameter "netconsole" in the
>  following format::
>  
> - netconsole=[+][src-port]@[src-ip]/[<dev>],[tgt-port]@<tgt-ip>/[tgt-macaddr]
> + netconsole=[+][src-port]@[src-ip]/[<dev>],[tgt-port]@<tgt-ip>/[tgt-macaddr][-V<vid:pcp>]
>  
>     where
>  	+             if present, enable extended console support
> @@ -44,11 +46,17 @@ following format::
>  	tgt-port      port for logging agent (6666)
>  	tgt-ip        IP address for logging agent
>  	tgt-macaddr   ethernet MAC address for logging agent (broadcast)
> +	-V            if present, enable VLAN support
> +	vid:pcp       VLAN identifier and priority code point
>  
>  Examples::
>  
>   linux netconsole=4444@10.0.0.1/eth1,9353@10.0.0.2/12:34:56:78:9a:bc
>  
> +or using VLAN::
> +
> + linux netconsole=4444@10.0.0.1/eth1,9353@10.0.0.2/12:34:56:78:9a:bc-V100:1
> +
>  or::
>  
>   insmod netconsole netconsole=@/,@10.0.0.2/
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 92001f7af380..f0690cd6a744 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -36,7 +36,6 @@
>  #include <linux/inet.h>
>  #include <linux/configfs.h>
>  #include <linux/etherdevice.h>
> -
>  MODULE_AUTHOR("Maintainer: Matt Mackall <mpm@xxxxxxxxxxx>");
>  MODULE_DESCRIPTION("Console driver for network interfaces");
>  MODULE_LICENSE("GPL");
> @@ -46,7 +45,7 @@ MODULE_LICENSE("GPL");
>  
>  static char config[MAX_PARAM_LENGTH];
>  module_param_string(netconsole, config, MAX_PARAM_LENGTH, 0);
> -MODULE_PARM_DESC(netconsole, " netconsole=[src-port]@[src-ip]/[dev],[tgt-port]@<tgt-ip>/[tgt-macaddr]");
> +MODULE_PARM_DESC(netconsole, " netconsole=[src-port]@[src-ip]/[dev],[tgt-port]@<tgt-ip>/[tgt-macaddr][-V<vid:pcp>]");
>  
>  static bool oops_only = false;
>  module_param(oops_only, bool, 0600);
> diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
> index e6a2d72e0dc7..8ab3f25cadae 100644
> --- a/include/linux/netpoll.h
> +++ b/include/linux/netpoll.h
> @@ -31,6 +31,9 @@ struct netpoll {
>  	bool ipv6;
>  	u16 local_port, remote_port;
>  	u8 remote_mac[ETH_ALEN];
> +	bool vlan_present;
> +	u16 vlan_id;
> +	u8 pcp;
>  };
>  
>  struct netpoll_info {
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 2338753e936b..077a7aec51ae 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -478,6 +478,14 @@ void netpoll_send_udp(struct netpoll *np, const char *msg, int len)
>  
>  	skb->dev = np->dev;
>  
> +	if (np->vlan_present) {
> +		skb->vlan_proto = htons(ETH_P_8021Q);
> +
> +		/* htons for tci is done in __vlan_insert_inner_tag, not here */
> +		skb->vlan_tci = (np->pcp << VLAN_PRIO_SHIFT) + (np->vlan_id & VLAN_VID_MASK);
> +		skb->vlan_present = 1;
> +	}

This does not seem to be the way to go around this, I would rather
specifying eth0.<VID> on the netconsole parameters and automatically
create a VLAN interface from that which would ensure that everything
works properly and that the VLAN interface is linked to its lower device
properly.

If you prefer your current syntax, that is probably fine, too but you
should consider registering a VLAN device when you parse appropriate
options.
-- 
Florian