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
