Re: [PATCH v2 4/4] staging: qlge: replace pr_err with netdev_err

[Benjamin Poirier]

On 2020-06-30 01:43 +0800, Coiby Xu wrote:
> On Mon, Jun 29, 2020 at 02:30:04PM +0900, Benjamin Poirier wrote:
> > On 2020-06-27 22:58 +0800, Coiby Xu wrote:
> > [...]
> > >  void ql_dump_qdev(struct ql_adapter *qdev)
> > >  {
> > > @@ -1611,99 +1618,100 @@ void ql_dump_qdev(struct ql_adapter *qdev)
> > >  #ifdef QL_CB_DUMP
> > >  void ql_dump_wqicb(struct wqicb *wqicb)
> > >  {
> > > -	pr_err("Dumping wqicb stuff...\n");
> > > -	pr_err("wqicb->len = 0x%x\n", le16_to_cpu(wqicb->len));
> > > -	pr_err("wqicb->flags = %x\n", le16_to_cpu(wqicb->flags));
> > > -	pr_err("wqicb->cq_id_rss = %d\n",
> > > -	       le16_to_cpu(wqicb->cq_id_rss));
> > > -	pr_err("wqicb->rid = 0x%x\n", le16_to_cpu(wqicb->rid));
> > > -	pr_err("wqicb->wq_addr = 0x%llx\n",
> > > -	       (unsigned long long)le64_to_cpu(wqicb->addr));
> > > -	pr_err("wqicb->wq_cnsmr_idx_addr = 0x%llx\n",
> > > -	       (unsigned long long)le64_to_cpu(wqicb->cnsmr_idx_addr));
> > > +	netdev_err(qdev->ndev, "Dumping wqicb stuff...\n");
> > 
> > drivers/staging/qlge/qlge_dbg.c:1621:13: error: âqdevâ undeclared (first use in this function); did you mean âcdevâ?
> > 1621 |  netdev_err(qdev->ndev, "Dumping wqicb stuff...\n");
> >      |             ^~~~
> >      |             cdev
> > 
> > [...]
> > and many more like that
> > 
> > Anyways, qlge_dbg.h is a dumpster. It has hundreds of lines of code
> > bitrotting away in ifdef land. See this comment from David Miller on the
> > topic of ifdef'ed debugging code:
> > https://lore.kernel.org/netdev/20200303.145916.1506066510928020193.davem@xxxxxxxxxxxxx/
> 
> Thank you for spotting this problem! This issue could be fixed by
> passing qdev to ql_dump_wqicb. Or are you suggesting we completely
> remove qlge_dbg since it's only for the purpose of debugging the driver
> by the developer?

At 2000 lines, qlge_dbg.c alone is larger than some entire ethernet
drivers. Most of what it does is dump kernel data structures or pci
memory mapped registers to dmesg. There are better facilities for that.
My thinking is not simply to delete qlge_dbg.c but to replace it, making
sure that most of the same information is still available. For data
structures, crash or drgn can be used; possibly with a script for the
latter which formats the data. For pci registers, they should be
included in the ethtool register dump and a patch added to ethtool to
pretty print them. That's what other drivers like e1000e do. For the
"coredump", devlink health can be used.

The qlge_force_coredump module option should also be removed. At the
moment, calling the ethtool register dump function with that option
enabled does a hexdump of a 176k struct to dmesg. That's shameful.

> 
> Btw, I'm curious how you make this compiling error occur. Do you manually trigger
> it via "QL_CB_DUMP=1 make M=drivers/staging/qlge" or use some automate
> tools?

I just uncommented the defines in qlge.h