netdev - Re: [PATCH net-next 07/11] net: hns3: adds debug messages to identify eth down cause

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 25 Jul 2019 21:59:08 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "tanhuazhong@...wei.com" <tanhuazhong@...wei.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "liuyonglong@...wei.com" <liuyonglong@...wei.com>
CC:     "lipeng321@...wei.com" <lipeng321@...wei.com>,
        "yisen.zhuang@...wei.com" <yisen.zhuang@...wei.com>,
        "salil.mehta@...wei.com" <salil.mehta@...wei.com>,
        "linuxarm@...wei.com" <linuxarm@...wei.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 07/11] net: hns3: adds debug messages to identify
 eth down cause

On Thu, 2019-07-25 at 20:28 +0800, liuyonglong wrote:
> 
> On 2019/7/25 3:12, Saeed Mahameed wrote:
> > On Wed, 2019-07-24 at 11:18 +0800, Huazhong Tan wrote:
> > > From: Yonglong Liu <liuyonglong@...wei.com>
> > > 
> > > Some times just see the eth interface have been down/up via
> > > dmesg, but can not know why the eth down. So adds some debug
> > > messages to identify the cause for this.
> > > 
> > 
> > I really don't like this. your default msg lvl has NETIF_MSG_IFDOWN
> > turned on .. dumping every single operation that happens on your
> > device
> > by default to kernel log is too much ! 
> > 
> > We should really consider using trace buffers with well defined
> > structures for vendor specific events. so we can use bpf filters
> > and
> > state of the art tools for netdev debugging.
> > 
> 
> We do this because we can just see a link down message in dmesg, and
> had
> take a long time to found the cause of link down, just because
> another
> user changed the settings.
> 
> We can change the net_open/net_stop/dcbnl_ops to msg_drv (not default
> turned on),  and want to keep the others default print to kernel log,
> is it acceptable?
> 

acceptable as long as debug information are kept off by default and
your driver doens't spam the kernel log.

you should use dynamic debug [1] and/or "off by default" msg lvls for
debugging information..

I couldn't find any rules regarding what to put in kernel log, Maybe
someone can share ?. but i vaguely remember that the recommendation
for device drivers is to put nothing, only error/warning messages.

[1] 
https://www.kernel.org/doc/html/v4.15/admin-guide/dynamic-debug-howto.html

> > > @@ -1593,6 +1603,11 @@ static int hns3_ndo_set_vf_vlan(struct
> > > net_device *netdev, int vf, u16 vlan,
> > >  	struct hnae3_handle *h = hns3_get_handle(netdev);
> > >  	int ret = -EIO;
> > >  
> > > +	if (netif_msg_ifdown(h))
> > 
> > why msg_ifdown ? looks like netif_msg_drv is more appropriate, for
> > many
> > of the cases in this patch.
> > 
> 
> This operation may cause link down, so we use msg_ifdown.
> 

ifdown isn't link down.. 

to be honest, I couldn't find any documentation explaining how/when to
use msg lvls, (i didn't look too deep though), by looking at other
drivers, my interpretations is:

ifdup (open/boot up flow)
ifdwon (close/teardown flow)
drv (driver based or dynamic flows) 
etc .. 

-Saeed.