linux-kernel - Re: [PATCH NAND v2] mtd: nand: Replace printk() with appropriate pr

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] [day] [month] [year] [list]
Message-ID: <20180220202008.4a67d0e9@bbrezillon>
Date:   Tue, 20 Feb 2018 20:20:08 +0100
From:   Boris Brezillon <boris.brezillon@...tlin.com>
To:     Shreeya Patel <shreeya.patel23498@...il.com>
Cc:     boris.brezillon@...e-electrons.com, richard@....at,
        dwmw2@...radead.org, computersforpeace@...il.com,
        marek.vasut@...il.com, cyrille.pitchen@...ev4u.fr,
        maximlevitsky@...il.com, linux-mtd@...ts.infradead.org,
        linux-kernel@...r.kernel.org, ezequiel@...guardiasur.com.ar,
        outreachy-kernel@...glegroups.com
Subject: Re: [PATCH NAND v2] mtd: nand: Replace printk() with appropriate
 pr_*macro()

On Tue, 20 Feb 2018 23:07:18 +0530
Shreeya Patel <shreeya.patel23498@...il.com> wrote:

> On Tue, 2018-02-20 at 18:16 +0100, Boris Brezillon wrote:
> > On Tue, 20 Feb 2018 22:36:41 +0530
> > Shreeya Patel <shreeya.patel23498@...il.com> wrote:
> >   
> > > 
> > > On Mon, 2018-02-19 at 15:51 +0100, Boris Brezillon wrote:  
> > > > 
> > > > Hi Shreeya,
> > > > 
> > > > On Mon, 19 Feb 2018 18:43:45 +0530
> > > > Shreeya Patel <shreeya.patel23498@...il.com> wrote:
> > > >     
> > > > > 
> > > > > 
> > > > > The log levels embedded with the name are more concise than
> > > > > printk.
> > > > > Replace printks having a log level with the appropriate
> > > > > pr_*macro.
> > > > > 
> > > > > Signed-off-by: Shreeya Patel <shreeya.patel23498@...il.com>
> > > > > ---
> > > > > 
> > > > > Changes in v2:
> > > > >   -Merge previous patches of the patchset regarding replacement
> > > > > of printk with pr_*macro, into single patch.
> > > > > 
> > > > > 
> > > > >  drivers/mtd/nand/cs553x_nand.c   |  9 ++---
> > > > >  drivers/mtd/nand/diskonchip.c    | 76 +++++++++++++++++++++---
> > > > > ----
> > > > > ------------
> > > > >  drivers/mtd/nand/fsl_elbc_nand.c |  2 +-
> > > > >  drivers/mtd/nand/fsl_ifc_nand.c  |  2 +-
> > > > >  drivers/mtd/nand/mxc_nand.c      |  2 +-
> > > > >  drivers/mtd/nand/nand_bch.c      | 12 +++----
> > > > >  drivers/mtd/nand/nandsim.c       | 10 +++---
> > > > >  drivers/mtd/nand/r852.c          |  2 +-
> > > > >  drivers/mtd/nand/r852.h          |  6 ++--
> > > > >  drivers/mtd/nand/sm_common.c     |  5 ++-
> > > > >  10 files changed, 65 insertions(+), 61 deletions(-)
> > > > >     
> > > > [...]
> > > >     
> > > > > 
> > > > > 
> > > > >  
> > > > > diff --git a/drivers/mtd/nand/diskonchip.c
> > > > > b/drivers/mtd/nand/diskonchip.c
> > > > > index c3aa53c..b97d88c 100644
> > > > > --- a/drivers/mtd/nand/diskonchip.c
> > > > > +++ b/drivers/mtd/nand/diskonchip.c    
> > > > [...]
> > > >     
> > > > > 
> > > > > 
> > > > > @@ -438,7 +438,7 @@ static void __init
> > > > > doc2000_count_chips(struct
> > > > > mtd_info *mtd)
> > > > >  			break;
> > > > >  	}
> > > > >  	doc->chips_per_floor = i;
> > > > > -	printk(KERN_DEBUG "Detected %d chips per floor.\n",
> > > > > i);
> > > > > +	pr_info("Detected %d chips per floor.\n", i);    
> > > > Should be pr_debug() here.
> > > >     
> > > > > 
> > > > > 
> > > > >  }
> > > > >      
> > > > [...]
> > > >     
> > > > > 
> > > > > 
> > > > > diff --git a/drivers/mtd/nand/nandsim.c
> > > > > b/drivers/mtd/nand/nandsim.c
> > > > > index 246b439..4e5f817 100644
> > > > > --- a/drivers/mtd/nand/nandsim.c
> > > > > +++ b/drivers/mtd/nand/nandsim.c
> > > > > @@ -184,15 +184,15 @@ MODULE_PARM_DESC(bch,		 "En
> > > > > able
> > > > > BCH ecc and set how many bits should "
> > > > >  
> > > > >  /* Simulator's output macros (logging, debugging, warning,
> > > > > error)
> > > > > */
> > > > >  #define NS_LOG(args...) \
> > > > > -	do { if (log) printk(KERN_DEBUG NS_OUTPUT_PREFIX "
> > > > > log: "
> > > > > args); } while(0)
> > > > > +	do { if (log) pr_debug(NS_OUTPUT_PREFIX " log: "
> > > > > args); }
> > > > > while(0)    
> > > > You could define pr_fmt() to avoid passing NS_OUTPUT_PREFIX.
> > > > Something
> > > > like:
> > > > 
> > > > #define pr_fmt(fmt)	"[nandsim]" fmt
> > > > 
> > > > (remember to put this definition before include directives).
> > > > 
> > > > Then, all you have to do is
> > > > 
> > > > do { if (log) pr_debug(" log: " args); } while(0)
> > > >     
> > > > > 
> > > > > 
> > > > >  #define NS_DBG(args...) \
> > > > > -	do { if (dbg) printk(KERN_DEBUG NS_OUTPUT_PREFIX "
> > > > > debug:
> > > > > " args); } while(0)
> > > > > +	do { if (dbg) pr_debug(NS_OUTPUT_PREFIX " debug: "
> > > > > args);
> > > > > } while(0)
> > > > >  #define NS_WARN(args...) \
> > > > > -	do { printk(KERN_WARNING NS_OUTPUT_PREFIX " warning: "
> > > > > args); } while(0)
> > > > > +	do { pr_warn(NS_OUTPUT_PREFIX " warning: " args); }
> > > > > while(0)
> > > > >  #define NS_ERR(args...) \
> > > > > -	do { printk(KERN_ERR NS_OUTPUT_PREFIX " error: "
> > > > > args); }
> > > > > while(0)
> > > > > +	do { pr_err(NS_OUTPUT_PREFIX " error: " args); }
> > > > > while(0)
> > > > >  #define NS_INFO(args...) \
> > > > > -	do { printk(KERN_INFO NS_OUTPUT_PREFIX " " args); }
> > > > > while(0)
> > > > > +	do { pr_info(NS_OUTPUT_PREFIX " " args); } while(0)
> > > > >  
> > > > >  /* Busy-wait delay macros (microseconds, milliseconds) */
> > > > >  #define NS_UDELAY(us) \
> > > > > diff --git a/drivers/mtd/nand/r852.c b/drivers/mtd/nand/r852.c
> > > > > index fc9287a..3d54c6a 100644
> > > > > --- a/drivers/mtd/nand/r852.c
> > > > > +++ b/drivers/mtd/nand/r852.c
> > > > > @@ -935,7 +935,7 @@ static int  r852_probe(struct pci_dev
> > > > > *pci_dev,
> > > > > const struct pci_device_id *id)
> > > > >  		&dev->card_detect_work, 0);
> > > > >  
> > > > >  
> > > > > -	printk(KERN_NOTICE DRV_NAME ": driver loaded
> > > > > successfully\n");
> > > > > +	pr_notice(DRV_NAME ": driver loaded
> > > > > successfully\n");    
> > > > Same here:
> > > > 
> > > > #define pr_fmt(fmt)	DRV_NAME fmt    
> > > I am facing the following errors here.
> > > 
> > > 
> > > 
> > > In file included from drivers/mtd/nand/r852.c:22:0:
> > > drivers/mtd/nand/r852.h:148:0: warning: "pr_fmt" redefined
> > >  #define pr_fmt(fmt)  (DRV_NAME fmt)
> > >  ^
> > > In file included from ./include/linux/kernel.h:14:0,
> > >                  from drivers/mtd/nand/r852.c:10:
> > > ./include/linux/printk.h:287:0: note: this is the location of the
> > > previous definition
> > >  #define pr_fmt(fmt) fmt  
> > That's because you didn't define pr_fmt() before all the #include
> > directives in this driver. See the '#indef pr_fmt' statement in
> > printk.h
> > which is preventing redefinition of this symbol if the file including
> > printk.h (either directly or indirectly) already defines it.  
> 
> Yes, and that is why I did undef before defining it again in the r852.c
> file.
> Shouldn't it work in this manner?

It should compile (I'd need to see the diff to figure out why it fails
to compile in your case), but not necessarily do what you want.
When you use #undef/#define to redefine a macro the new definition
applies to everything that uses it in the code *after* the
point it's been redefined. One problem I can think of (but there
probably are others) is when some intermediate header files use
pr_xxx() inside macros/inline functions. In this case, you probably
want the prefix to be applied and that can only be done if you've
defined your own pr_fmt() before including printk.h.

Another reason to define pr_fmt() before including printk.h is that it
requires one line instead of 2 if you go for the #undef/#define
solution.

-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com