Re: [PATCH 5/6] staging: rtl8188eu: stop using DBG_88E

[Sudip Mukherjee <sudipm.mukherjee@gmail.com>]

On Sat, Jul 18, 2015 at 08:03:27PM +0200, Jakub Sitnicki wrote:
> On Sat, Jul 18, 2015 at 06:46 AM CEST, Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote:
> > On Fri, Jul 17, 2015 at 05:33:55PM +0200, Jakub Sitnicki wrote:
> >> On Thu, Jul 16, 2015 at 01:28 PM CEST, Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote:
> >> > Stop using DBG_88E which is a custom macro for printing debugging
> >> > messages. Instead start using pr_debug and in the process define
> >> > pr_fmt.
> >> 
> >> In the end, don't we want to use netdev_dbg() everywhere where we work
> >> with a struct net_device? And use dev_dbg() everywhere where we work
> >> with a struct device (or a struct usb_interface)?
> > Looks like in some places we can get net_device from usb_interface.
> >
> > struct dvobj_priv *dvobj = usb_get_intfdata(pusb_intf);
> > struct adapter *padapter = dvobj->if1;
> > struct net_device *pnetdev = padapter->pnetdev;
> 
> You're right providing that the net_device has been successfully
> allocated and hasn't been deallocated yet.  There are at least a couple
> of pr_debug() calls that couldn't be converted to netdev_dbg(), one in
> rtw_drv_init() and another in rtw_dev_remove(), because the net_device
> is not available, if I'm not wrong.
yes. That is why I said in some places.
> 
> >> At least that's how I understand commit 8f26b8376faa ("checkpatch:
> >> update suggested printk conversions") description:
> >> 
> >>     Direct conversion of printk(KERN_<LEVEL>...  to pr_<level> isn't the
> >>     preferred conversion when a struct net_device or struct device is
> >>     available.
> >> 
> >> Do you think it is worth going straight for netdev_dbg()/dev_dbg() to
> >> avoid redoing it later?
> > At the end it should be netdev_* or dev_* and if both are not available
> > then pr_*. Here my main intention was to remove the custom defined
> > macro. And while doing this it is easier to use a script to reduce the
> > chances of error. Now that the custom macro is out of the way we can
> > concentrate on converting it to netdev_* or dev_*.
> >> 
> >> >
> > <snip>
> >> >  
> >> > +#define pr_fmt(fmt) "R8188EU: " fmt
> >> >  #include <osdep_service.h>
> >> >  #include <drv_types.h>
> >> >  #include <recv_osdep.h>
> >> 
> >> If we're going to stay with pr_debug(), using KBUILD_MODNAME seems to be
> >> the convention among drivers when defining pr_fmt():
> >> 
> >> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > yes, KBUILD_MODNAME is the usual convention but you will also find many
> > places where that has not been used. Here I have used R8188EU to keep it
> > same for all the messages that will be printed from the other files of
> > this driver else it might be confusing for the user.
> 
> I see your point.  If consistent log prefix is the goal do you think it
> would make sense to change it to:
> 
> #define pr_fmt(fmt) DRIVER_PREFIX ": " fmt
> 
> ... so the prefix is not defined in two places?
Well, then we need to define DRIVER_PREFIX in some header file and by
norm all local header files should be included at the end. And pr_fmt
should be at the top so we will not have DRIVER_PREFIX at that point.

regards
sudip