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

[Vladimir Oltean]

On Thu, Dec 10, 2020 at 09:55:16AM -0800, Florian Fainelli wrote:
> 
> 
> 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

I don't have a strong opinion, considering that I have not actually
tried this. I do tend to agree with Florian about registering an 8021q
interface, as long as there aren't any other complications associated
with it. As for the syntax, I am not sure if "eth0.<VID>" is universal
enough to make an ABI out of it.