Re: [PATCH v2] vgaarb: Use dev_printk() when possible

[Daniel Vetter]

On Thu, Nov 17, 2016 at 02:20:59PM -0600, Bjorn Helgaas wrote:
> On Thu, Nov 17, 2016 at 07:59:21PM +0100, Daniel Vetter wrote:
> > On Thu, Nov 17, 2016 at 11:47:58AM -0600, Bjorn Helgaas wrote:
> > > Use dev_printk() when possible.  This makes messages more consistent with
> > > other device-related messages and, in some cases, adds useful information.
> > > This changes messages like this:
> > > 
> > >   vgaarb: failed to allocate pci device
> > >   vgaarb: setting as boot device: PCI:0000:01:00.0
> > >   vgaarb: device added: PCI:0000:01:00.0,decodes=io+mem,owns=io+mem,locks=none
> > >   vgaarb: bridge control possible 0000:01:00.0
> > > 
> > > to this:
> > > 
> > >   pci 0000:01:00.0: vgaarb: failed to allocate VGA arbiter data
> > >   pci 0000:01:00.0: vgaarb: setting as boot VGA device
> > >   pci 0000:01:00.0: vgaarb: VGA device added: decodes=io+mem,owns=io+mem,locks=none
> > >   pci 0000:01:00.0: vgaarb: bridge control possible
> > > 
> > > No functional change intended.
> > 
> > Found one more nit below where there was one relevant change that
> > shouldn't be here.
> 
> > > @@ -1189,24 +1194,25 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
> > >  				ret_val = -EPROTO;
> > >  				goto done;
> > >  			}
> > > -			pr_debug("%s ==> %x:%x:%x.%x\n", curr_pos,
> > > -				domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
> > > -
> > >  			pdev = pci_get_domain_bus_and_slot(domain, bus, devfn);
> > > -			pr_debug("pdev %p\n", pdev);
> > >  			if (!pdev) {
> > > -				pr_err("invalid PCI address %x:%x:%x\n",
> > > -					domain, bus, devfn);
> > > +				pr_err("invalid PCI address %04x:%02x:%02x.%x\n",
> > > +					domain, bus, PCI_SLOT(devfn),
> > > +					PCI_FUNC(devfn));
> > 
> > Userspace-triggerable dmesg spam is imo not good, this needs to stay at
> > debug level.
> 
> I did move these around slightly, but I don't *think* I changed
> anything from debug to err level.  Previously:
> 
>   pr_debug("%s ==> %x:%x:%x.%x\n", ...);
>   pdev = pci_get_domain_bus_and_slot(...);
>   pr_debug("pdev %p\n", pdev);
>   if (!pdev) {
>     pr_err("invalid PCI address %x:%x:%x\n", ...);
>   }
> 
> after my patch:
> 
>   pdev = pci_get_domain_bus_and_slot(...);
>   if (!pdev) {
>     pr_err("invalid PCI address %x:%x:%x\n", ...);
>   }
>   pr_debug("%s ==> %x:%x:%x.%x pdev %p\n", ...);
> 
> The pr_err() was there before.  I'd be glad to change that to a
> pr_debug() if you prefer.

Oh right, misread the diff. Applied your patch, but a s/pr_err/pr_debug
for anything userspace can trigger in the string parsing would be nice.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch