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.
-Daniel
>
> Signed-off-by: Bjorn Helgaas <bhelg...@google.com>
> ---
> drivers/gpu/vga/vgaarb.c | 66
> ++++++++++++++++++++++++----------------------
> 1 file changed, 35 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> index 1887f19..3a8e9c6 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -31,6 +31,10 @@
>
> #define pr_fmt(fmt) "vgaarb: " fmt
>
> +#define vgaarb_dbg(dev, fmt, arg...) dev_dbg(dev, "vgaarb: " fmt, ##arg)
> +#define vgaarb_info(dev, fmt, arg...) dev_info(dev, "vgaarb: " fmt,
> ##arg)
> +#define vgaarb_err(dev, fmt, arg...) dev_err(dev, "vgaarb: " fmt, ##arg)
> +
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/pci.h>
> @@ -188,6 +192,7 @@ static void vga_check_first_use(void)
> static struct vga_device *__vga_tryget(struct vga_device *vgadev,
> unsigned int rsrc)
> {
> + struct device *dev = &vgadev->pdev->dev;
> unsigned int wants, legacy_wants, match;
> struct vga_device *conflict;
> unsigned int pci_bits;
> @@ -203,8 +208,8 @@ static struct vga_device *__vga_tryget(struct vga_device
> *vgadev,
> (vgadev->decodes & VGA_RSRC_LEGACY_MEM))
> rsrc |= VGA_RSRC_LEGACY_MEM;
>
> - pr_debug("%s: %d\n", __func__, rsrc);
> - pr_debug("%s: owns: %d\n", __func__, vgadev->owns);
> + vgaarb_dbg(dev, "%s: %d\n", __func__, rsrc);
> + vgaarb_dbg(dev, "%s: owns: %d\n", __func__, vgadev->owns);
>
> /* Check what resources we need to acquire */
> wants = rsrc & ~vgadev->owns;
> @@ -336,9 +341,10 @@ static struct vga_device *__vga_tryget(struct vga_device
> *vgadev,
>
> static void __vga_put(struct vga_device *vgadev, unsigned int rsrc)
> {
> + struct device *dev = &vgadev->pdev->dev;
> unsigned int old_locks = vgadev->locks;
>
> - pr_debug("%s\n", __func__);
> + vgaarb_dbg(dev, "%s\n", __func__);
>
> /* Update our counters, and account for equivalent legacy resources
> * if we decode them
> @@ -611,7 +617,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev
> *pdev)
> /* Allocate structure */
> vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL);
> if (vgadev == NULL) {
> - pr_err("failed to allocate pci device\n");
> + vgaarb_err(&pdev->dev, "failed to allocate VGA arbiter data\n");
> /*
> * What to do on allocation failure ? For now, let's just do
> * nothing, I'm not sure there is anything saner to be done.
> @@ -663,7 +669,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev
> *pdev)
> */
> if (vga_default == NULL &&
> ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) {
> - pr_info("setting as boot device: PCI:%s\n", pci_name(pdev));
> + vgaarb_info(&pdev->dev, "setting as boot VGA device\n");
> vga_set_default_device(pdev);
> }
>
> @@ -672,8 +678,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev
> *pdev)
> /* Add to the list */
> list_add(&vgadev->list, &vga_list);
> vga_count++;
> - pr_info("device added: PCI:%s,decodes=%s,owns=%s,locks=%s\n",
> - pci_name(pdev),
> + vgaarb_info(&pdev->dev, "VGA device added:
> decodes=%s,owns=%s,locks=%s\n",
> vga_iostate_to_str(vgadev->decodes),
> vga_iostate_to_str(vgadev->owns),
> vga_iostate_to_str(vgadev->locks));
> @@ -725,6 +730,7 @@ static bool vga_arbiter_del_pci_device(struct pci_dev
> *pdev)
> static inline void vga_update_device_decodes(struct vga_device *vgadev,
> int new_decodes)
> {
> + struct device *dev = &vgadev->pdev->dev;
> int old_decodes, decodes_removed, decodes_unlocked;
>
> old_decodes = vgadev->decodes;
> @@ -732,8 +738,7 @@ static inline void vga_update_device_decodes(struct
> vga_device *vgadev,
> decodes_unlocked = vgadev->locks & decodes_removed;
> vgadev->decodes = new_decodes;
>
> - pr_info("device changed decodes:
> PCI:%s,olddecodes=%s,decodes=%s:owns=%s\n",
> - pci_name(vgadev->pdev),
> + vgaarb_info(dev, "changed VGA decodes:
> olddecodes=%s,decodes=%s:owns=%s\n",
> vga_iostate_to_str(old_decodes),
> vga_iostate_to_str(vgadev->decodes),
> vga_iostate_to_str(vgadev->owns));
> @@ -754,7 +759,7 @@ static inline void vga_update_device_decodes(struct
> vga_device *vgadev,
> if (!(old_decodes & VGA_RSRC_LEGACY_MASK) &&
> new_decodes & VGA_RSRC_LEGACY_MASK)
> vga_decode_count++;
> - pr_debug("decoding count now is: %d\n", vga_decode_count);
> + vgaarb_dbg(dev, "decoding count now is: %d\n", vga_decode_count);
> }
>
> static void __vga_set_legacy_decoding(struct pci_dev *pdev,
> @@ -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.
> ret_val = -ENODEV;
> goto done;
> }
> +
> + pr_debug("%s ==> %04x:%02x:%02x.%x pdev %p\n", curr_pos,
> + domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
> + pdev);
> }
>
> vgadev = vgadev_find(pdev);
> pr_debug("vgadev %p\n", vgadev);
> if (vgadev == NULL) {
> if (pdev) {
> - pr_err("this pci device is not a vga device\n");
> + vgaarb_err(&pdev->dev, "not a VGA device\n");
> pci_dev_put(pdev);
> }
>
> @@ -1226,7 +1232,7 @@ static ssize_t vga_arb_write(struct file *file, const
> char __user *buf,
> }
> }
> if (i == MAX_USER_CARDS) {
> - pr_err("maximum user cards (%d) number reached!\n",
> + vgaarb_err(&pdev->dev, "maximum user cards (%d) number
> reached, ignoring this one!\n",
> MAX_USER_CARDS);
> pci_dev_put(pdev);
> /* XXX: which value to return? */
> @@ -1317,8 +1323,8 @@ static int vga_arb_release(struct inode *inode, struct
> file *file)
> uc = &priv->cards[i];
> if (uc->pdev == NULL)
> continue;
> - pr_debug("uc->io_cnt == %d, uc->mem_cnt == %d\n",
> - uc->io_cnt, uc->mem_cnt);
> + vgaarb_dbg(&uc->pdev->dev, "uc->io_cnt == %d, uc->mem_cnt ==
> %d\n",
> + uc->io_cnt, uc->mem_cnt);
> while (uc->io_cnt--)
> vga_put(uc->pdev, VGA_RSRC_LEGACY_IO);
> while (uc->mem_cnt--)
> @@ -1371,7 +1377,7 @@ static int pci_notify(struct notifier_block *nb,
> unsigned long action,
> struct pci_dev *pdev = to_pci_dev(dev);
> bool notify = false;
>
> - pr_debug("%s\n", __func__);
> + vgaarb_dbg(dev, "%s\n", __func__);
>
> /* For now we're only intereted in devices added and removed. I didn't
> * test this thing here, so someone needs to double check for the
> @@ -1408,6 +1414,7 @@ static int __init vga_arb_device_init(void)
> int rc;
> struct pci_dev *pdev;
> struct vga_device *vgadev;
> + struct device *dev;
>
> rc = misc_register(&vga_arb_device);
> if (rc < 0)
> @@ -1423,8 +1430,6 @@ static int __init vga_arb_device_init(void)
> PCI_ANY_ID, pdev)) != NULL)
> vga_arbiter_add_pci_device(pdev);
>
> - pr_info("loaded\n");
> -
> list_for_each_entry(vgadev, &vga_list, list) {
> #if defined(CONFIG_X86) || defined(CONFIG_IA64)
> /*
> @@ -1440,6 +1445,7 @@ static int __init vga_arb_device_init(void)
> int i;
>
> limit = screen_info.lfb_base + screen_info.lfb_size;
> + dev = &vgadev->pdev->dev;
>
> /* Does firmware framebuffer belong to us? */
> for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> @@ -1458,21 +1464,19 @@ static int __init vga_arb_device_init(void)
> continue;
>
> if (!vga_default_device())
> - pr_info("setting as boot device: PCI:%s\n",
> - pci_name(vgadev->pdev));
> + vgaarb_info(dev, "setting as boot device\n");
> else if (vgadev->pdev != vga_default_device())
> - pr_info("overriding boot device: PCI:%s\n",
> - pci_name(vgadev->pdev));
> + vgaarb_info(dev, "overriding boot device\n");
> vga_set_default_device(vgadev->pdev);
> }
> #endif
> if (vgadev->bridge_has_one_vga)
> - pr_info("bridge control possible %s\n",
> - pci_name(vgadev->pdev));
> + vgaarb_info(dev, "bridge control possible\n");
> else
> - pr_info("no bridge control possible %s\n",
> - pci_name(vgadev->pdev));
> + vgaarb_info(dev, "no bridge control possible\n");
> }
> +
> + pr_info("loaded\n");
> return rc;
> }
> subsys_initcall(vga_arb_device_init);
>
> _______________________________________________
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
