Messages in this thread | | | From | Saeed Mahameed <> | Subject | Re: [PATCH net-next 07/11] net: hns3: adds debug messages to identify eth down cause | Date | Thu, 25 Jul 2019 21:59:08 +0000 |
| |
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@huawei.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.
| |