We will change all of them to netif_msg_drv() which is default off Thanks for your reply!
On 2019/7/26 5:59, Saeed Mahameed wrote: > 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 <liuyongl...@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. >
