Re: [PATCH v4 1/8] x86/mm/pat: use pr_info() and friends

["Luis R. Rodriguez" <mcgrof@suse.com>]

On Thu, Apr 30, 2015 at 03:56:41PM +0200, Borislav Petkov wrote:
> On Wed, Apr 29, 2015 at 02:44:20PM -0700, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > 
> 
> Please add a commit message, albeit a single trivial sentence.
> 
> > Cc: Andy Walls <awalls@md.metrocast.net>
> > Cc: Doug Ledford <dledford@redhat.com>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Juergen Gross <jgross@suse.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Borislav Petkov <bp@suse.de>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: linux-kernel@vger.kernel.org
> > Cc: x86@kernel.org
> > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> > ---
> >  arch/x86/mm/pat.c | 47 +++++++++++++++++++++++------------------------
> >  1 file changed, 23 insertions(+), 24 deletions(-)
> > 
> > diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> > index 372ad42..917155d 100644
> > --- a/arch/x86/mm/pat.c
> > +++ b/arch/x86/mm/pat.c
> 
> #define pr_fmt(fmt) "PAT: " fmt
> 
> or so, so that it has a prefix and lookers can know where the message
> comes from.

Sure, I didn't as I thought this was already automatically going to be
added for is, after I added the build complained about it being
defined. I checked an we udef it and redefine it for some other x86
code so will do the same. I'll use "x86-PAT".

> > @@ -39,7 +39,7 @@ int __read_mostly pat_enabled = 1;
> >  static inline void pat_disable(const char *reason)
> >  {
> >  	pat_enabled = 0;
> > -	printk(KERN_INFO "%s\n", reason);
> > +	pr_info("%s\n", reason);
> >  }
> >  
> >  static int __init nopat(char *str)
> > @@ -211,8 +211,7 @@ void pat_init(void)
> >  			 * switched to PAT on the boot CPU. We have no way to
> >  			 * undo PAT.
> >  			 */
> > -			printk(KERN_ERR "PAT enabled, "
> > -			       "but not supported by secondary CPU\n");
> > +			pr_err("PAT enabled, but not supported by secondary CPU\n");
> >  			BUG();
> >  		}
> >  	}
> > @@ -451,9 +450,9 @@ int reserve_memtype(u64 start, u64 end, enum page_cache_mode req_type,
> >  
> >  	err = rbt_memtype_check_insert(new, new_type);
> >  	if (err) {
> > -		printk(KERN_INFO "reserve_memtype failed [mem %#010Lx-%#010Lx], track %s, req %s\n",
> > -		       start, end - 1,
> > -		       cattr_name(new->type), cattr_name(req_type));
> > +		pr_info("reserve_memtype failed [mem %#010Lx-%#010Lx], track %s, req %s\n",
> > +			start, end - 1,
> > +			cattr_name(new->type), cattr_name(req_type));
> >  		kfree(new);
> >  		spin_unlock(&memtype_lock);
> >  
> > @@ -462,9 +461,9 @@ int reserve_memtype(u64 start, u64 end, enum page_cache_mode req_type,
> >  
> >  	spin_unlock(&memtype_lock);
> >  
> > -	dprintk("reserve_memtype added [mem %#010Lx-%#010Lx], track %s, req %s, ret %s\n",
> > -		start, end - 1, cattr_name(new->type), cattr_name(req_type),
> > -		new_type ? cattr_name(*new_type) : "-");
> 
> This was using "debugpat"...
> 
> > +	pr_debug("reserve_memtype added [mem %#010Lx-%#010Lx], track %s, req %s, ret %s\n",
> > +		 start, end - 1, cattr_name(new->type), cattr_name(req_type),
> > +		 new_type ? cattr_name(*new_type) : "-");
> 
> ... and now that pr_debug() ugliness which I can't enable at boot time.
> From looking at Documentation/dynamic-debug-howto.txt apparently I can
> using dyndbg= but let's not change that.

OK sure, I'll leave this and mention the few remaining non-converted stragglers
on the commit log, I'll also add a "x86-PAT-debug" prefix to dprintk().

> >  	return err;
> >  }
> > @@ -497,14 +496,15 @@ int free_memtype(u64 start, u64 end)
> >  	spin_unlock(&memtype_lock);
> >  
> >  	if (!entry) {
> > -		printk(KERN_INFO "%s:%d freeing invalid memtype [mem %#010Lx-%#010Lx]\n",
> > -		       current->comm, current->pid, start, end - 1);
> > +		pr_info("%s:%d freeing invalid memtype [mem %#010Lx-%#010Lx]\n",
> > +			current->comm, current->pid, start, end - 1);
> >  		return -EINVAL;
> >  	}
> >  
> >  	kfree(entry);
> >  
> > -	dprintk("free_memtype request [mem %#010Lx-%#010Lx]\n", start, end - 1);
> > +	pr_debug("free_memtype request [mem %#010Lx-%#010Lx]\n",
> > +		 start, end - 1);
> 
> Ditto.

OK, fixed.

> >  
> >  	return 0;
> >  }
> > @@ -628,8 +628,8 @@ static inline int range_is_allowed(unsigned long pfn, unsigned long size)
> >  
> >  	while (cursor < to) {
> >  		if (!devmem_is_allowed(pfn)) {
> > -			printk(KERN_INFO "Program %s tried to access /dev/mem between [mem %#010Lx-%#010Lx], PAT prevents it\n",
> > -			       current->comm, from, to - 1);
> > +			pr_info("Program %s tried to access /dev/mem between [mem %#010Lx-%#010Lx], PAT prevents it\n",
> > +				current->comm, from, to - 1);
> >  			return 0;
> >  		}
> >  		cursor += PAGE_SIZE;
> > @@ -698,8 +698,7 @@ int kernel_map_sync_memtype(u64 base, unsigned long size,
> >  				size;
> >  
> >  	if (ioremap_change_attr((unsigned long)__va(base), id_sz, pcm) < 0) {
> > -		printk(KERN_INFO "%s:%d ioremap_change_attr failed %s "
> > -			"for [mem %#010Lx-%#010Lx]\n",
> > +		pr_info("%s:%d ioremap_change_attr failed %s for [mem %#010Lx-%#010Lx]\n",
> >  			current->comm, current->pid,
> >  			cattr_name(pcm),
> >  			base, (unsigned long long)(base + size-1));
> > @@ -734,7 +733,7 @@ static int reserve_pfn_range(u64 paddr, unsigned long size, pgprot_t *vma_prot,
> >  
> >  		pcm = lookup_memtype(paddr);
> >  		if (want_pcm != pcm) {
> > -			printk(KERN_WARNING "%s:%d map pfn RAM range req %s for [mem %#010Lx-%#010Lx], got %s\n",
> > +			pr_warn("%s:%d map pfn RAM range req %s for [mem %#010Lx-%#010Lx], got %s\n",
> >  				current->comm, current->pid,
> >  				cattr_name(want_pcm),
> >  				(unsigned long long)paddr,
> > @@ -755,13 +754,13 @@ static int reserve_pfn_range(u64 paddr, unsigned long size, pgprot_t *vma_prot,
> >  		if (strict_prot ||
> >  		    !is_new_memtype_allowed(paddr, size, want_pcm, pcm)) {
> >  			free_memtype(paddr, paddr + size);
> > -			printk(KERN_ERR "%s:%d map pfn expected mapping type %s"
> > -				" for [mem %#010Lx-%#010Lx], got %s\n",
> > -				current->comm, current->pid,
> > -				cattr_name(want_pcm),
> > -				(unsigned long long)paddr,
> > -				(unsigned long long)(paddr + size - 1),
> > -				cattr_name(pcm));
> > +			pr_err(KERN_ERR "%s:%d map pfn expected mapping type %s"
> 
> KERN_ERR forgotten.
> 
> Use checkpatch but don't take it too seriously :-)

That was a fat finger rather. Fixed, thanks.

 Luis