| Message ID | 20230808223412.1743176-1-sui.jingfeng@linux.dev (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix typos, comments and copyright | expand |
On Wed, Aug 09, 2023 at 06:34:01AM +0800, Sui Jingfeng wrote: > From: Sui Jingfeng <suijingfeng@loongson.cn> > > v1: > * Various improve. > v2: > * More fixes, optimizations and improvements. > > Sui Jingfeng (11): > PCI/VGA: Use unsigned type for the io_state variable > PCI: Add the pci_get_class_masked() helper > PCI/VGA: Deal with VGA class devices I dropped these two patches, at least for now. There's no other use of pci_get_class_masked(), and the VGA use seems to be mostly optimization and possibly some behavior change that isn't 100% clear yet. If it's important, we can look at it again later. > PCI/VGA: Drop the inline in the vga_update_device_decodes() function. > PCI/VGA: Move the new_state assignment out of the loop > PCI/VGA: Fix two typos in the comments of pci_notify() > PCI/VGA: vga_client_register() return -ENODEV on failure, not -1 > PCI/VGA: Fix a typo to the comment of vga_default > PCI/VGA: Fix a typo to the comments in vga_str_to_iostate() function > PCI/VGA: Tidy up the code and comment format > PCI/VGA: Replace full MIT license text with SPDX identifier > > drivers/pci/search.c | 30 ++++++ > drivers/pci/vgaarb.c | 233 +++++++++++++++++++++++++---------------- > include/linux/pci.h | 7 ++ > include/linux/vgaarb.h | 27 +---- > 4 files changed, 185 insertions(+), 112 deletions(-) I applied the rest of the patches on pci/vga for v6.5. I updated the commit logs and tweaked some of the patches. I squashed all the typo fixes together and added several more that I had done previously but not posted. The diff between your original v2 posting and the current pci/vga branch is attached. Most of it is additional typo fixes, but if you look closely you'll see: - The omission of the pci_get_class_masked() patches (in vga_arbiter_add_pci_device(), pci_notify(), vga_arb_device_init()) - The tweaks I did to: vga_update_device_decodes() vga_client_register() vga_arbiter_notify_clients() I dropped the Reviewed-bys from the typo fixes because I didn't want to extend them to everything that got squashed together. Happy to add them back if anybody wants to look again. The branch is at https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=vga Bjorn diff --git a/drivers/pci/search.c b/drivers/pci/search.c index f1c15aea868b..b4c138a6ec02 100644 --- a/drivers/pci/search.c +++ b/drivers/pci/search.c @@ -334,36 +334,6 @@ struct pci_dev *pci_get_device(unsigned int vendor, unsigned int device, } EXPORT_SYMBOL(pci_get_device); -/** - * pci_get_class_masked - begin or continue searching for a PCI device by class and mask - * @class: search for a PCI device with this class designation - * @from: Previous PCI device found in search, or %NULL for new search. - * - * Iterates through the list of known PCI devices. If a PCI device is - * found with a matching @class, the reference count to the device is - * incremented and a pointer to its device structure is returned. - * Otherwise, %NULL is returned. - * A new search is initiated by passing %NULL as the @from argument. - * Otherwise if @from is not %NULL, searches continue from next device - * on the global list. The reference count for @from is always decremented - * if it is not %NULL. - */ -struct pci_dev *pci_get_class_masked(unsigned int class, unsigned int mask, - struct pci_dev *from) -{ - struct pci_device_id id = { - .vendor = PCI_ANY_ID, - .device = PCI_ANY_ID, - .subvendor = PCI_ANY_ID, - .subdevice = PCI_ANY_ID, - .class_mask = mask, - .class = class, - }; - - return pci_get_dev_by_id(&id, from); -} -EXPORT_SYMBOL(pci_get_class_masked); - /** * pci_get_class - begin or continue searching for a PCI device by class * @class: search for a PCI device with this class designation diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c index a2f6e0e6b634..5e6b1eb54c64 100644 --- a/drivers/pci/vgaarb.c +++ b/drivers/pci/vgaarb.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: MIT /* - * vgaarb.c: Implements the VGA arbitration. For details refer to + * vgaarb.c: Implements VGA arbitration. For details refer to * Documentation/gpu/vgaarbiter.rst * * (C) Copyright 2005 Benjamin Herrenschmidt <benh@kernel.crashing.org> @@ -42,9 +42,9 @@ static void vga_arbiter_notify_clients(void); struct vga_device { struct list_head list; struct pci_dev *pdev; - unsigned int decodes; /* what does it decodes */ - unsigned int owns; /* what does it owns */ - unsigned int locks; /* what does it locks */ + unsigned int decodes; /* what it decodes */ + unsigned int owns; /* what it owns */ + unsigned int locks; /* what it locks */ unsigned int io_lock_cnt; /* legacy IO lock count */ unsigned int mem_lock_cnt; /* legacy MEM lock count */ unsigned int io_norm_cnt; /* normal IO count */ @@ -116,20 +116,18 @@ static struct vga_device *vgadev_find(struct pci_dev *pdev) /** * vga_default_device - return the default VGA device, for vgacon * - * This can be defined by the platform. The default implementation - * is rather dumb and will probably only work properly on single - * vga card setups and/or x86 platforms. + * This can be defined by the platform. The default implementation is + * rather dumb and will probably only work properly on single VGA card + * setups and/or x86 platforms. * - * If your VGA default device is not PCI, you'll have to return - * NULL here. In this case, I assume it will not conflict with - * any PCI card. If this is not true, I'll have to define two archs - * hooks for enabling/disabling the VGA default device if that is - * possible. This may be a problem with real _ISA_ VGA cards, in - * addition to a PCI one. I don't know at this point how to deal - * with that card. Can theirs IOs be disabled at all ? If not, then - * I suppose it's a matter of having the proper arch hook telling - * us about it, so we basically never allow anybody to succeed a - * vga_get()... + * If your VGA default device is not PCI, you'll have to return NULL here. + * In this case, I assume it will not conflict with any PCI card. If this + * is not true, I'll have to define two arch hooks for enabling/disabling + * the VGA default device if that is possible. This may be a problem with + * real _ISA_ VGA cards, in addition to a PCI one. I don't know at this + * point how to deal with that card. Can their IOs be disabled at all? If + * not, then I suppose it's a matter of having the proper arch hook telling + * us about it, so we basically never allow anybody to succeed a vga_get(). */ struct pci_dev *vga_default_device(void) { @@ -147,14 +145,13 @@ void vga_set_default_device(struct pci_dev *pdev) } /** - * vga_remove_vgacon - deactivete vga console + * vga_remove_vgacon - deactivate VGA console * - * Unbind and unregister vgacon in case pdev is the default vga - * device. Can be called by gpu drivers on initialization to make - * sure vga register access done by vgacon will not disturb the - * device. + * Unbind and unregister vgacon in case pdev is the default VGA device. + * Can be called by GPU drivers on initialization to make sure VGA register + * access done by vgacon will not disturb the device. * - * @pdev: pci device. + * @pdev: PCI device. */ #if !defined(CONFIG_VGA_CONSOLE) int vga_remove_vgacon(struct pci_dev *pdev) @@ -194,14 +191,15 @@ int vga_remove_vgacon(struct pci_dev *pdev) EXPORT_SYMBOL(vga_remove_vgacon); /* - * If we don't ever use vgaarb, we should avoid turning off anything anywhere. - * Due to old X servers getting confused about the boot device not being VGA. + * If we don't ever use VGA arbitration, we should avoid turning off + * anything anywhere due to old X servers getting confused about the boot + * device not being VGA. */ static void vga_check_first_use(void) { /* - * We should inform all GPUs in the system that - * vgaarb has occurred and to try and disable resources if they can + * Inform all GPUs in the system that VGA arbitration has occurred + * so they can disable resources if possible. */ if (!vga_arbiter_used) { vga_arbiter_used = true; @@ -241,13 +239,13 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, /* * We don't need to request a legacy resource, we just enable - * appropriate decoding and go + * appropriate decoding and go. */ legacy_wants = wants & VGA_RSRC_LEGACY_MASK; if (legacy_wants == 0) goto enable_them; - /* Ok, we don't, let's find out how we need to kick off */ + /* Ok, we don't, let's find out who we need to kick off */ list_for_each_entry(conflict, &vga_list, list) { unsigned int lwants = legacy_wants; unsigned int change_bridge = 0; @@ -257,11 +255,11 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, continue; /* - * We have a possible conflict. before we go further, we must + * We have a possible conflict. Before we go further, we must * check if we sit on the same bus as the conflicting device. - * if we don't, then we must tie both IO and MEM resources + * If we don't, then we must tie both IO and MEM resources * together since there is only a single bit controlling - * VGA forwarding on P2P bridges + * VGA forwarding on P2P bridges. */ if (vgadev->pdev->bus != conflict->pdev->bus) { change_bridge = 1; @@ -270,14 +268,14 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, /* * Check if the guy has a lock on the resource. If he does, - * return the conflicting entry + * return the conflicting entry. */ if (conflict->locks & lwants) return conflict; /* * Ok, now check if it owns the resource we want. We can - * lock resources that are not decoded, therefore a device + * lock resources that are not decoded; therefore a device * can own resources it doesn't decode. */ match = lwants & conflict->owns; @@ -285,8 +283,8 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, continue; /* - * Looks like he doesn't have a lock, we can steal - * them from him + * Looks like he doesn't have a lock, we can steal them + * from him. */ flags = 0; @@ -321,7 +319,7 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, enable_them: /* - * Ok dude, we got it, everybody conflicting has been disabled, let's + * Ok, we got it, everybody conflicting has been disabled, let's * enable us. Mark any bits in "owns" regardless of whether we * decoded them. We can lock resources we don't decode, therefore * we must track them via "owns". @@ -364,8 +362,8 @@ static void __vga_put(struct vga_device *vgadev, unsigned int rsrc) vgaarb_dbg(dev, "%s\n", __func__); /* - * Update our counters, and account for equivalent legacy resources - * if we decode them + * Update our counters and account for equivalent legacy resources + * if we decode them. */ if ((rsrc & VGA_RSRC_NORMAL_IO) && vgadev->io_norm_cnt > 0) { vgadev->io_norm_cnt--; @@ -384,7 +382,7 @@ static void __vga_put(struct vga_device *vgadev, unsigned int rsrc) /* * Just clear lock bits, we do lazy operations so we don't really - * have to bother about anything else at this point + * have to bother about anything else at this point. */ if (vgadev->io_lock_cnt == 0) vgadev->locks &= ~VGA_RSRC_LEGACY_IO; @@ -393,23 +391,23 @@ static void __vga_put(struct vga_device *vgadev, unsigned int rsrc) /* * Kick the wait queue in case somebody was waiting if we actually - * released something + * released something. */ if (old_locks != vgadev->locks) wake_up_all(&vga_wait_queue); } /** - * vga_get - acquire & locks VGA resources - * @pdev: pci device of the VGA card or NULL for the system default + * vga_get - acquire & lock VGA resources + * @pdev: PCI device of the VGA card or NULL for the system default * @rsrc: bit mask of resources to acquire and lock * @interruptible: blocking should be interruptible by signals ? * - * This function acquires VGA resources for the given card and mark those - * resources locked. If the resource requested are "normal" (and not legacy) + * Acquire VGA resources for the given card and mark those resources + * locked. If the resources requested are "normal" (and not legacy) * resources, the arbiter will first check whether the card is doing legacy - * decoding for that type of resource. If yes, the lock is "converted" into a - * legacy resource lock. + * decoding for that type of resource. If yes, the lock is "converted" into + * a legacy resource lock. * * The arbiter will first look for all VGA cards that might conflict and disable * their IOs and/or Memory access, including VGA forwarding on P2P bridges if @@ -441,7 +439,7 @@ int vga_get(struct pci_dev *pdev, unsigned int rsrc, int interruptible) int rc = 0; vga_check_first_use(); - /* The one who calls us should check for this, but lets be sure... */ + /* The caller should check for this, but let's be sure */ if (pdev == NULL) pdev = vga_default_device(); if (pdev == NULL) @@ -461,11 +459,11 @@ int vga_get(struct pci_dev *pdev, unsigned int rsrc, int interruptible) break; /* - * We have a conflict, we wait until somebody kicks the + * We have a conflict; we wait until somebody kicks the * work queue. Currently we have one work queue that we * kick each time some resources are released, but it would - * be fairly easy to have a per device one so that we only - * need to attach to the conflicting device + * be fairly easy to have a per-device one so that we only + * need to attach to the conflicting device. */ init_waitqueue_entry(&wait, current); add_wait_queue(&vga_wait_queue, &wait); @@ -487,12 +485,12 @@ EXPORT_SYMBOL(vga_get); /** * vga_tryget - try to acquire & lock legacy VGA resources - * @pdev: pci devivce of VGA card or NULL for system default + * @pdev: PCI device of VGA card or NULL for system default * @rsrc: bit mask of resources to acquire and lock * - * This function performs the same operation as vga_get(), but will return an - * error (-EBUSY) instead of blocking if the resources are already locked by - * another card. It can be called in any context + * Perform the same operation as vga_get(), but return an error (-EBUSY) + * instead of blocking if the resources are already locked by another card. + * Can be called in any context. * * On success, release the VGA resource again with vga_put(). * @@ -508,7 +506,7 @@ static int vga_tryget(struct pci_dev *pdev, unsigned int rsrc) vga_check_first_use(); - /* The one who calls us should check for this, but lets be sure... */ + /* The caller should check for this, but let's be sure */ if (pdev == NULL) pdev = vga_default_device(); if (pdev == NULL) @@ -528,20 +526,20 @@ static int vga_tryget(struct pci_dev *pdev, unsigned int rsrc) /** * vga_put - release lock on legacy VGA resources - * @pdev: pci device of VGA card or NULL for system default - * @rsrc: but mask of resource to release + * @pdev: PCI device of VGA card or NULL for system default + * @rsrc: bit mask of resource to release * - * This fuction releases resources previously locked by vga_get() or - * vga_tryget(). The resources aren't disabled right away, so that a subsequence - * vga_get() on the same card will succeed immediately. Resources have a - * counter, so locks are only released if the counter reaches 0. + * Release resources previously locked by vga_get() or vga_tryget(). The + * resources aren't disabled right away, so that a subsequent vga_get() on + * the same card will succeed immediately. Resources have a counter, so + * locks are only released if the counter reaches 0. */ void vga_put(struct pci_dev *pdev, unsigned int rsrc) { struct vga_device *vgadev; unsigned long flags; - /* The one who calls us should check for this, but lets be sure... */ + /* The caller should check for this, but let's be sure */ if (pdev == NULL) pdev = vga_default_device(); if (pdev == NULL) @@ -709,20 +707,20 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) return; } - /* okay iterate the new devices bridge hierarachy */ + /* Iterate the new device's bridge hierarchy */ new_bus = vgadev->pdev->bus; while (new_bus) { new_bridge = new_bus->self; - /* go through list of devices already registered */ + /* Go through list of devices already registered */ list_for_each_entry(same_bridge_vgadev, &vga_list, list) { bus = same_bridge_vgadev->pdev->bus; bridge = bus->self; - /* See if the share a bridge with this device */ + /* See if it shares a bridge with this device */ if (new_bridge == bridge) { /* - * If their direct parent bridge is the same + * If its direct parent bridge is the same * as any bridge of this device then it can't * be used for that device. */ @@ -730,10 +728,10 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) } /* - * Now iterate the previous devices bridge hierarchy. - * If the new devices parent bridge is in the other - * devices hierarchy then we can't use it to control - * this device + * Now iterate the previous device's bridge hierarchy. + * If the new device's parent bridge is in the other + * device's hierarchy, we can't use it to control this + * device. */ while (bus) { bridge = bus->self; @@ -754,10 +752,9 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) } /* - * Currently, we assume that the "initial" setup of the system is - * not sane, that is we come up with conflicting devices and let - * the arbiter's client decides if devices decodes or not legacy - * things. + * Currently, we assume that the "initial" setup of the system is not sane, + * that is, we come up with conflicting devices and let the arbiter's + * client decide if devices decodes legacy things or not. */ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) { @@ -767,12 +764,16 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) struct pci_dev *bridge; u16 cmd; + /* Only deal with VGA class devices */ + if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA) + return false; + /* Allocate structure */ vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL); if (vgadev == NULL) { vgaarb_err(&pdev->dev, "failed to allocate VGA arbiter data\n"); /* - * What to do on allocation failure ? For now, let's just do + * What to do on allocation failure? For now, let's just do * nothing, I'm not sure there is anything saner to be done. */ return false; @@ -792,9 +793,10 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) /* By default, mark it as decoding */ vga_decode_count++; + /* * Mark that we "own" resources based on our enables, we will - * clear that below if the bridge isn't forwarding + * clear that below if the bridge isn't forwarding. */ pci_read_config_word(pdev, PCI_COMMAND, &cmd); if (cmd & PCI_COMMAND_IO) @@ -874,7 +876,7 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev) return ret; } -/* This is called with the lock */ +/* Called with the lock */ static void vga_update_device_decodes(struct vga_device *vgadev, unsigned int new_decodes) { @@ -885,8 +887,7 @@ static void vga_update_device_decodes(struct vga_device *vgadev, vgadev->decodes = new_decodes; - vgaarb_info(dev, - "VGA decodes changed: olddecodes=%s,decodes=%s:owns=%s\n", + vgaarb_info(dev, "VGA decodes changed: 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)); @@ -932,9 +933,9 @@ static void __vga_set_legacy_decoding(struct pci_dev *pdev, vga_update_device_decodes(vgadev, decodes); /* - * XXX if somebody is going from "doesn't decode" to "decodes" state - * here, additional care must be taken as we may have pending owner - * ship of non-legacy region ... + * XXX If somebody is going from "doesn't decode" to "decodes" + * state here, additional care must be taken as we may have pending + * ownership of non-legacy region. */ bail: spin_unlock_irqrestore(&vga_lock, flags); @@ -942,10 +943,10 @@ static void __vga_set_legacy_decoding(struct pci_dev *pdev, /** * vga_set_legacy_decoding - * @pdev: pci device of the VGA card + * @pdev: PCI device of the VGA card * @decodes: bit mask of what legacy regions the card decodes * - * Indicates to the arbiter if the card decodes legacy VGA IOs, legacy VGA + * Indicate to the arbiter if the card decodes legacy VGA IOs, legacy VGA * Memory, both, or none. All cards default to both, the card driver (fbdev for * example) should tell the arbiter if it has disabled legacy decoding, so the * card can be left out of the arbitration process (and can be safe to take @@ -959,44 +960,42 @@ EXPORT_SYMBOL(vga_set_legacy_decoding); /** * vga_client_register - register or unregister a VGA arbitration client - * @pdev: pci device of the VGA client - * @set_decode: vga decode change callback + * @pdev: PCI device of the VGA client + * @set_decode: VGA decode change callback * * Clients have two callback mechanisms they can use. * * @set_decode callback: If a client can disable its GPU VGA resource, it * will get a callback from this to set the encode/decode state. * - * Rationale: we cannot disable VGA decode resources unconditionally, some - * single GPU laptops seem to require ACPI or BIOS access to the VGA registers - * to control things like backlights etc. Hopefully newer multi-GPU laptops do - * something saner, and desktops won't have any special ACPI for this. The - * driver will get a callback when VGA arbitration is first used by userspace - * since some older X servers have issues. + * Rationale: we cannot disable VGA decode resources unconditionally + * because some single GPU laptops seem to require ACPI or BIOS access to + * the VGA registers to control things like backlights etc. Hopefully newer + * multi-GPU laptops do something saner, and desktops won't have any + * special ACPI for this. The driver will get a callback when VGA + * arbitration is first used by userspace since some older X servers have + * issues. * - * This function does not check whether a client for @pdev has been registered - * already. + * Does not check whether a client for @pdev has been registered already. * - * To unregister just call vga_client_unregister(). + * To unregister, call vga_client_unregister(). * * Returns: 0 on success, -ENODEV on failure */ int vga_client_register(struct pci_dev *pdev, unsigned int (*set_decode)(struct pci_dev *pdev, bool decode)) { - int ret = -ENODEV; - struct vga_device *vgadev; unsigned long flags; + struct vga_device *vgadev; spin_lock_irqsave(&vga_lock, flags); vgadev = vgadev_find(pdev); - if (vgadev) { + if (vgadev) vgadev->set_decode = set_decode; - ret = 0; - } spin_unlock_irqrestore(&vga_lock, flags); - - return ret; + if (!vgadev) + return -ENODEV; + return 0; } EXPORT_SYMBOL(vga_client_register); @@ -1005,13 +1004,13 @@ EXPORT_SYMBOL(vga_client_register); * * Semantics is: * - * open : open user instance of the arbitrer. by default, it's + * open : Open user instance of the arbiter. By default, it's * attached to the default VGA device of the system. * - * close : close user instance, release locks + * close : Close user instance, release locks * - * read : return a string indicating the status of the target. - * an IO state string is of the form {io,mem,io+mem,none}, + * read : Return a string indicating the status of the target. + * An IO state string is of the form {io,mem,io+mem,none}, * mc and ic are respectively mem and io lock counts (for * debugging/diagnostic only). "decodes" indicate what the * card currently decodes, "owns" indicates what is currently @@ -1025,7 +1024,7 @@ EXPORT_SYMBOL(vga_client_register); * write : write a command to the arbiter. List of commands is: * * target <card_ID> : switch target to card <card_ID> (see below) - * lock <io_state> : acquires locks on target ("none" is invalid io_state) + * lock <io_state> : acquire locks on target ("none" is invalid io_state) * trylock <io_state> : non-blocking acquire locks on target * unlock <io_state> : release locks on target * unlock all : release all locks on target held by this user @@ -1042,23 +1041,21 @@ EXPORT_SYMBOL(vga_client_register); * Note about locks: * * The driver keeps track of which user has what locks on which card. It - * supports stacking, like the kernel one. This complexifies the implementation + * supports stacking, like the kernel one. This complicates the implementation * a bit, but makes the arbiter more tolerant to userspace problems and able * to properly cleanup in all cases when a process dies. * Currently, a max of 16 cards simultaneously can have locks issued from * userspace for a given user (file descriptor instance) of the arbiter. * * If the device is hot-unplugged, there is a hook inside the module to notify - * they being added/removed in the system and automatically added/removed in + * it being added/removed in the system and automatically added/removed in * the arbiter. */ #define MAX_USER_CARDS CONFIG_VGA_ARB_MAX_GPUS #define PCI_INVALID_CARD ((struct pci_dev *)-1UL) -/* - * Each user has an array of these, tracking which cards have locks - */ +/* Each user has an array of these, tracking which cards have locks */ struct vga_arb_user_card { struct pci_dev *pdev; unsigned int mem_cnt; @@ -1077,9 +1074,8 @@ static DEFINE_SPINLOCK(vga_user_lock); /* - * This function gets a string in the format: "PCI:domain:bus:dev.fn" and - * returns the respective values. If the string is not in this format, - * it returns 0. + * Take a string in the format: "PCI:domain:bus:dev.fn" and return the + * respective values. If the string is not in this format, return 0. */ static int vga_pci_str_to_vars(char *buf, int count, unsigned int *domain, unsigned int *bus, unsigned int *devfn) @@ -1111,7 +1107,7 @@ static ssize_t vga_arb_read(struct file *file, char __user *buf, if (lbuf == NULL) return -ENOMEM; - /* Protects vga_list */ + /* Protect vga_list */ spin_lock_irqsave(&vga_lock, flags); /* If we are targeting the default, use it */ @@ -1126,15 +1122,15 @@ static ssize_t vga_arb_read(struct file *file, char __user *buf, vgadev = vgadev_find(pdev); if (vgadev == NULL) { /* - * Wow, it's not in the list, that shouldn't happen, - * let's fix us up and return invalid card + * Wow, it's not in the list, that shouldn't happen, let's + * fix us up and return invalid card. */ spin_unlock_irqrestore(&vga_lock, flags); len = sprintf(lbuf, "invalid"); goto done; } - /* Fill the buffer with infos */ + /* Fill the buffer with info */ len = snprintf(lbuf, 1024, "count:%d,PCI:%s,decodes=%s,owns=%s,locks=%s(%u:%u)\n", vga_decode_count, pci_name(pdev), @@ -1180,7 +1176,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf, if (copy_from_user(kbuf, buf, count)) return -EFAULT; curr_pos = kbuf; - kbuf[count] = '\0'; /* Just to make sure... */ + kbuf[count] = '\0'; if (strncmp(curr_pos, "lock ", 5) == 0) { curr_pos += 5; @@ -1205,7 +1201,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf, vga_get_uninterruptible(pdev, io_state); - /* Update the client's locks lists... */ + /* Update the client's locks lists */ for (i = 0; i < MAX_USER_CARDS; i++) { if (priv->cards[i].pdev == pdev) { if (io_state & VGA_RSRC_LEGACY_IO) @@ -1372,7 +1368,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf, vgaarb_dbg(&pdev->dev, "maximum user cards (%d) number reached, ignoring this one!\n", MAX_USER_CARDS); pci_dev_put(pdev); - /* XXX: which value to return? */ + /* XXX: Which value to return? */ ret_val = -ENOMEM; goto done; } @@ -1433,7 +1429,7 @@ static int vga_arb_open(struct inode *inode, struct file *file) list_add(&priv->list, &vga_user_list); spin_unlock_irqrestore(&vga_user_lock, flags); - /* Set the client' lists of locks */ + /* Set the client's lists of locks */ priv->target = vga_default_device(); /* Maybe this is still null! */ priv->cards[0].pdev = priv->target; priv->cards[0].io_cnt = 0; @@ -1472,68 +1468,32 @@ static int vga_arb_release(struct inode *inode, struct file *file) } /* - * Callback any registered clients to let them know we have a - * change in VGA cards + * Callback any registered clients to let them know we have a change in VGA + * cards. */ static void vga_arbiter_notify_clients(void) { struct vga_device *vgadev; unsigned long flags; - bool state; + unsigned int new_decodes; + bool new_state; if (!vga_arbiter_used) return; - state = (vga_count > 1) ? false : true; + new_state = (vga_count > 1) ? false : true; spin_lock_irqsave(&vga_lock, flags); list_for_each_entry(vgadev, &vga_list, list) { if (vgadev->set_decode) { - unsigned int decodes; - - decodes = vgadev->set_decode(vgadev->pdev, state); - vga_update_device_decodes(vgadev, decodes); + new_decodes = vgadev->set_decode(vgadev->pdev, + new_state); + vga_update_device_decodes(vgadev, new_decodes); } } spin_unlock_irqrestore(&vga_lock, flags); } -/* - * The PCI Class Code spec implies that only VGA devices with programming - * interface 0x00 can depend on the legacy VGA address range. VGA devices - * with programming interface 0x01 are 8514-compatible controllers. Since - * VGA devices with programming interface 0x00 is VGA compatible, the 'vga' - * suffix here should refer to the VGA-compatible devices after a strict - * reading of that specification. But considering the fact that there - * probably don't has a 8514-compatible controller that could be used with - * upstream kernel anymore, we would like to just ignore the programming - * interface byte. - * - * Besides, there do exist non VGA-compatible display controllers in the - * world and hardware vendors may abandon the old VGA standard someday. - * The meaning of 'vga' suffix here may change to evolve with time. - * - * A strict understanding of 'vga' certainly should be VGA-compatible, While - * a relaxed understanding of 'vga' would be PCI devices that are able to - * display. Currently, we just keep aligned to the previous behavior. - * Deal with VGA class devices. - */ -static bool pci_dev_is_vga(struct pci_dev *pdev) -{ - if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) - return true; - - /* - * The PCI_CLASS_NOT_DEFINED_VGA is defined to provide backward - * compatibility for devices that were built before the class code - * field was defined. - */ - if ((pdev->class >> 8) == PCI_CLASS_NOT_DEFINED_VGA) - return true; - - return false; -} - static int pci_notify(struct notifier_block *nb, unsigned long action, void *data) { @@ -1543,9 +1503,6 @@ static int pci_notify(struct notifier_block *nb, unsigned long action, vgaarb_dbg(dev, "%s\n", __func__); - if (!pci_dev_is_vga(pdev)) - return 0; - /* * For now, we're only interested in devices added and removed. * I didn't test this thing here, so someone needs to double check @@ -1580,8 +1537,8 @@ static struct miscdevice vga_arb_device = { static int __init vga_arb_device_init(void) { - struct pci_dev *pdev = NULL; int rc; + struct pci_dev *pdev; rc = misc_register(&vga_arb_device); if (rc < 0) @@ -1589,22 +1546,12 @@ static int __init vga_arb_device_init(void) bus_register_notifier(&pci_bus_type, &pci_notifier); - /* - * We add all PCI devices satisfying VGA class in the arbiter - * by default, but we ignore the programming interface byte - * intentionally. - */ - do { - pdev = pci_get_class_masked(PCI_CLASS_DISPLAY_VGA << 8, 0xFFFF00, pdev); - if (pdev && pci_dev_is_vga(pdev)) - vga_arbiter_add_pci_device(pdev); - } while (pdev); - - do { - pdev = pci_get_class_masked(PCI_CLASS_NOT_DEFINED_VGA << 8, 0xFFFF00, pdev); - if (pdev && pci_dev_is_vga(pdev)) - vga_arbiter_add_pci_device(pdev); - } while (pdev); + /* Add all VGA class PCI devices by default */ + pdev = NULL; + while ((pdev = + pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, + PCI_ANY_ID, pdev)) != NULL) + vga_arbiter_add_pci_device(pdev); pr_info("loaded\n"); return rc; diff --git a/include/linux/pci.h b/include/linux/pci.h index 14dc5ab4a1d2..c69a2cc1f412 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1180,9 +1180,6 @@ struct pci_dev *pci_get_slot(struct pci_bus *bus, unsigned int devfn); struct pci_dev *pci_get_domain_bus_and_slot(int domain, unsigned int bus, unsigned int devfn); struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from); -struct pci_dev *pci_get_class_masked(unsigned int class, unsigned int mask, - struct pci_dev *from); - int pci_dev_present(const struct pci_device_id *ids); int pci_bus_read_config_byte(struct pci_bus *bus, unsigned int devfn, @@ -1898,10 +1895,6 @@ static inline struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from) { return NULL; } -static inline struct pci_dev *pci_get_class_masked(unsigned int class, - unsigned int mask, - struct pci_dev *from) -{ return NULL; } static inline int pci_dev_present(const struct pci_device_id *ids) { return 0; }
Hi, Nice cleanup, thanks a lot. On 2023/8/24 06:29, Bjorn Helgaas wrote: > On Wed, Aug 09, 2023 at 06:34:01AM +0800, Sui Jingfeng wrote: >> From: Sui Jingfeng <suijingfeng@loongson.cn> >> >> v1: >> * Various improve. >> v2: >> * More fixes, optimizations and improvements. >> >> Sui Jingfeng (11): >> PCI/VGA: Use unsigned type for the io_state variable >> PCI: Add the pci_get_class_masked() helper >> PCI/VGA: Deal with VGA class devices > I dropped these two patches, at least for now. There's no other use > of pci_get_class_masked(), and the VGA use seems to be mostly > optimization and possibly some behavior change that isn't 100% clear > yet. If it's important, we can look at it again later. OK, no problem. >> PCI/VGA: Drop the inline in the vga_update_device_decodes() function. >> PCI/VGA: Move the new_state assignment out of the loop >> PCI/VGA: Fix two typos in the comments of pci_notify() >> PCI/VGA: vga_client_register() return -ENODEV on failure, not -1 >> PCI/VGA: Fix a typo to the comment of vga_default >> PCI/VGA: Fix a typo to the comments in vga_str_to_iostate() function >> PCI/VGA: Tidy up the code and comment format >> PCI/VGA: Replace full MIT license text with SPDX identifier >> >> drivers/pci/search.c | 30 ++++++ >> drivers/pci/vgaarb.c | 233 +++++++++++++++++++++++++---------------- >> include/linux/pci.h | 7 ++ >> include/linux/vgaarb.h | 27 +---- >> 4 files changed, 185 insertions(+), 112 deletions(-) > I applied the rest of the patches on pci/vga for v6.5. > > I updated the commit logs and tweaked some of the patches. > > I squashed all the typo fixes together and added several more that I > had done previously but not posted. The diff between your original v2 > posting and the current pci/vga branch is attached. Most of it is > additional typo fixes, but if you look closely you'll see: > > - The omission of the pci_get_class_masked() patches (in > vga_arbiter_add_pci_device(), pci_notify(), vga_arb_device_init()) > > - The tweaks I did to: > > vga_update_device_decodes() > vga_client_register() > vga_arbiter_notify_clients() > > I dropped the Reviewed-bys from the typo fixes because I didn't want > to extend them to everything that got squashed together. Happy to add > them back if anybody wants to look again. > > The branch is at https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=vga > > Bjorn > > diff --git a/drivers/pci/search.c b/drivers/pci/search.c > index f1c15aea868b..b4c138a6ec02 100644 > --- a/drivers/pci/search.c > +++ b/drivers/pci/search.c > @@ -334,36 +334,6 @@ struct pci_dev *pci_get_device(unsigned int vendor, unsigned int device, > } > EXPORT_SYMBOL(pci_get_device); > > -/** > - * pci_get_class_masked - begin or continue searching for a PCI device by class and mask > - * @class: search for a PCI device with this class designation > - * @from: Previous PCI device found in search, or %NULL for new search. > - * > - * Iterates through the list of known PCI devices. If a PCI device is > - * found with a matching @class, the reference count to the device is > - * incremented and a pointer to its device structure is returned. > - * Otherwise, %NULL is returned. > - * A new search is initiated by passing %NULL as the @from argument. > - * Otherwise if @from is not %NULL, searches continue from next device > - * on the global list. The reference count for @from is always decremented > - * if it is not %NULL. > - */ > -struct pci_dev *pci_get_class_masked(unsigned int class, unsigned int mask, > - struct pci_dev *from) > -{ > - struct pci_device_id id = { > - .vendor = PCI_ANY_ID, > - .device = PCI_ANY_ID, > - .subvendor = PCI_ANY_ID, > - .subdevice = PCI_ANY_ID, > - .class_mask = mask, > - .class = class, > - }; > - > - return pci_get_dev_by_id(&id, from); > -} > -EXPORT_SYMBOL(pci_get_class_masked); > - > /** > * pci_get_class - begin or continue searching for a PCI device by class > * @class: search for a PCI device with this class designation > diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c > index a2f6e0e6b634..5e6b1eb54c64 100644 > --- a/drivers/pci/vgaarb.c > +++ b/drivers/pci/vgaarb.c > @@ -1,6 +1,6 @@ > // SPDX-License-Identifier: MIT > /* > - * vgaarb.c: Implements the VGA arbitration. For details refer to > + * vgaarb.c: Implements VGA arbitration. For details refer to > * Documentation/gpu/vgaarbiter.rst > * > * (C) Copyright 2005 Benjamin Herrenschmidt <benh@kernel.crashing.org> > @@ -42,9 +42,9 @@ static void vga_arbiter_notify_clients(void); > struct vga_device { > struct list_head list; > struct pci_dev *pdev; > - unsigned int decodes; /* what does it decodes */ > - unsigned int owns; /* what does it owns */ > - unsigned int locks; /* what does it locks */ > + unsigned int decodes; /* what it decodes */ > + unsigned int owns; /* what it owns */ > + unsigned int locks; /* what it locks */ > unsigned int io_lock_cnt; /* legacy IO lock count */ > unsigned int mem_lock_cnt; /* legacy MEM lock count */ > unsigned int io_norm_cnt; /* normal IO count */ > @@ -116,20 +116,18 @@ static struct vga_device *vgadev_find(struct pci_dev *pdev) > /** > * vga_default_device - return the default VGA device, for vgacon > * > - * This can be defined by the platform. The default implementation > - * is rather dumb and will probably only work properly on single > - * vga card setups and/or x86 platforms. > + * This can be defined by the platform. The default implementation is > + * rather dumb and will probably only work properly on single VGA card > + * setups and/or x86 platforms. > * > - * If your VGA default device is not PCI, you'll have to return > - * NULL here. In this case, I assume it will not conflict with > - * any PCI card. If this is not true, I'll have to define two archs > - * hooks for enabling/disabling the VGA default device if that is > - * possible. This may be a problem with real _ISA_ VGA cards, in > - * addition to a PCI one. I don't know at this point how to deal > - * with that card. Can theirs IOs be disabled at all ? If not, then > - * I suppose it's a matter of having the proper arch hook telling > - * us about it, so we basically never allow anybody to succeed a > - * vga_get()... > + * If your VGA default device is not PCI, you'll have to return NULL here. > + * In this case, I assume it will not conflict with any PCI card. If this > + * is not true, I'll have to define two arch hooks for enabling/disabling > + * the VGA default device if that is possible. This may be a problem with > + * real _ISA_ VGA cards, in addition to a PCI one. I don't know at this > + * point how to deal with that card. Can their IOs be disabled at all? If > + * not, then I suppose it's a matter of having the proper arch hook telling > + * us about it, so we basically never allow anybody to succeed a vga_get(). > */ > struct pci_dev *vga_default_device(void) > { > @@ -147,14 +145,13 @@ void vga_set_default_device(struct pci_dev *pdev) > } > > /** > - * vga_remove_vgacon - deactivete vga console > + * vga_remove_vgacon - deactivate VGA console > * > - * Unbind and unregister vgacon in case pdev is the default vga > - * device. Can be called by gpu drivers on initialization to make > - * sure vga register access done by vgacon will not disturb the > - * device. > + * Unbind and unregister vgacon in case pdev is the default VGA device. > + * Can be called by GPU drivers on initialization to make sure VGA register > + * access done by vgacon will not disturb the device. > * > - * @pdev: pci device. > + * @pdev: PCI device. > */ > #if !defined(CONFIG_VGA_CONSOLE) > int vga_remove_vgacon(struct pci_dev *pdev) > @@ -194,14 +191,15 @@ int vga_remove_vgacon(struct pci_dev *pdev) > EXPORT_SYMBOL(vga_remove_vgacon); > > /* > - * If we don't ever use vgaarb, we should avoid turning off anything anywhere. > - * Due to old X servers getting confused about the boot device not being VGA. > + * If we don't ever use VGA arbitration, we should avoid turning off > + * anything anywhere due to old X servers getting confused about the boot > + * device not being VGA. > */ > static void vga_check_first_use(void) > { > /* > - * We should inform all GPUs in the system that > - * vgaarb has occurred and to try and disable resources if they can > + * Inform all GPUs in the system that VGA arbitration has occurred > + * so they can disable resources if possible. > */ > if (!vga_arbiter_used) { > vga_arbiter_used = true; > @@ -241,13 +239,13 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, > > /* > * We don't need to request a legacy resource, we just enable > - * appropriate decoding and go > + * appropriate decoding and go. > */ > legacy_wants = wants & VGA_RSRC_LEGACY_MASK; > if (legacy_wants == 0) > goto enable_them; > > - /* Ok, we don't, let's find out how we need to kick off */ > + /* Ok, we don't, let's find out who we need to kick off */ > list_for_each_entry(conflict, &vga_list, list) { > unsigned int lwants = legacy_wants; > unsigned int change_bridge = 0; > @@ -257,11 +255,11 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, > continue; > > /* > - * We have a possible conflict. before we go further, we must > + * We have a possible conflict. Before we go further, we must > * check if we sit on the same bus as the conflicting device. > - * if we don't, then we must tie both IO and MEM resources > + * If we don't, then we must tie both IO and MEM resources > * together since there is only a single bit controlling > - * VGA forwarding on P2P bridges > + * VGA forwarding on P2P bridges. > */ > if (vgadev->pdev->bus != conflict->pdev->bus) { > change_bridge = 1; > @@ -270,14 +268,14 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, > > /* > * Check if the guy has a lock on the resource. If he does, > - * return the conflicting entry > + * return the conflicting entry. > */ > if (conflict->locks & lwants) > return conflict; > > /* > * Ok, now check if it owns the resource we want. We can > - * lock resources that are not decoded, therefore a device > + * lock resources that are not decoded; therefore a device > * can own resources it doesn't decode. > */ > match = lwants & conflict->owns; > @@ -285,8 +283,8 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, > continue; > > /* > - * Looks like he doesn't have a lock, we can steal > - * them from him > + * Looks like he doesn't have a lock, we can steal them > + * from him. > */ > > flags = 0; > @@ -321,7 +319,7 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, > > enable_them: > /* > - * Ok dude, we got it, everybody conflicting has been disabled, let's > + * Ok, we got it, everybody conflicting has been disabled, let's > * enable us. Mark any bits in "owns" regardless of whether we > * decoded them. We can lock resources we don't decode, therefore > * we must track them via "owns". > @@ -364,8 +362,8 @@ static void __vga_put(struct vga_device *vgadev, unsigned int rsrc) > vgaarb_dbg(dev, "%s\n", __func__); > > /* > - * Update our counters, and account for equivalent legacy resources > - * if we decode them > + * Update our counters and account for equivalent legacy resources > + * if we decode them. > */ > if ((rsrc & VGA_RSRC_NORMAL_IO) && vgadev->io_norm_cnt > 0) { > vgadev->io_norm_cnt--; > @@ -384,7 +382,7 @@ static void __vga_put(struct vga_device *vgadev, unsigned int rsrc) > > /* > * Just clear lock bits, we do lazy operations so we don't really > - * have to bother about anything else at this point > + * have to bother about anything else at this point. > */ > if (vgadev->io_lock_cnt == 0) > vgadev->locks &= ~VGA_RSRC_LEGACY_IO; > @@ -393,23 +391,23 @@ static void __vga_put(struct vga_device *vgadev, unsigned int rsrc) > > /* > * Kick the wait queue in case somebody was waiting if we actually > - * released something > + * released something. > */ > if (old_locks != vgadev->locks) > wake_up_all(&vga_wait_queue); > } > > /** > - * vga_get - acquire & locks VGA resources > - * @pdev: pci device of the VGA card or NULL for the system default > + * vga_get - acquire & lock VGA resources > + * @pdev: PCI device of the VGA card or NULL for the system default > * @rsrc: bit mask of resources to acquire and lock > * @interruptible: blocking should be interruptible by signals ? > * > - * This function acquires VGA resources for the given card and mark those > - * resources locked. If the resource requested are "normal" (and not legacy) > + * Acquire VGA resources for the given card and mark those resources > + * locked. If the resources requested are "normal" (and not legacy) > * resources, the arbiter will first check whether the card is doing legacy > - * decoding for that type of resource. If yes, the lock is "converted" into a > - * legacy resource lock. > + * decoding for that type of resource. If yes, the lock is "converted" into > + * a legacy resource lock. > * > * The arbiter will first look for all VGA cards that might conflict and disable > * their IOs and/or Memory access, including VGA forwarding on P2P bridges if > @@ -441,7 +439,7 @@ int vga_get(struct pci_dev *pdev, unsigned int rsrc, int interruptible) > int rc = 0; > > vga_check_first_use(); > - /* The one who calls us should check for this, but lets be sure... */ > + /* The caller should check for this, but let's be sure */ > if (pdev == NULL) > pdev = vga_default_device(); > if (pdev == NULL) > @@ -461,11 +459,11 @@ int vga_get(struct pci_dev *pdev, unsigned int rsrc, int interruptible) > break; > > /* > - * We have a conflict, we wait until somebody kicks the > + * We have a conflict; we wait until somebody kicks the > * work queue. Currently we have one work queue that we > * kick each time some resources are released, but it would > - * be fairly easy to have a per device one so that we only > - * need to attach to the conflicting device > + * be fairly easy to have a per-device one so that we only > + * need to attach to the conflicting device. > */ > init_waitqueue_entry(&wait, current); > add_wait_queue(&vga_wait_queue, &wait); > @@ -487,12 +485,12 @@ EXPORT_SYMBOL(vga_get); > > /** > * vga_tryget - try to acquire & lock legacy VGA resources > - * @pdev: pci devivce of VGA card or NULL for system default > + * @pdev: PCI device of VGA card or NULL for system default > * @rsrc: bit mask of resources to acquire and lock > * > - * This function performs the same operation as vga_get(), but will return an > - * error (-EBUSY) instead of blocking if the resources are already locked by > - * another card. It can be called in any context > + * Perform the same operation as vga_get(), but return an error (-EBUSY) > + * instead of blocking if the resources are already locked by another card. > + * Can be called in any context. > * > * On success, release the VGA resource again with vga_put(). > * > @@ -508,7 +506,7 @@ static int vga_tryget(struct pci_dev *pdev, unsigned int rsrc) > > vga_check_first_use(); > > - /* The one who calls us should check for this, but lets be sure... */ > + /* The caller should check for this, but let's be sure */ > if (pdev == NULL) > pdev = vga_default_device(); > if (pdev == NULL) > @@ -528,20 +526,20 @@ static int vga_tryget(struct pci_dev *pdev, unsigned int rsrc) > > /** > * vga_put - release lock on legacy VGA resources > - * @pdev: pci device of VGA card or NULL for system default > - * @rsrc: but mask of resource to release > + * @pdev: PCI device of VGA card or NULL for system default > + * @rsrc: bit mask of resource to release > * > - * This fuction releases resources previously locked by vga_get() or > - * vga_tryget(). The resources aren't disabled right away, so that a subsequence > - * vga_get() on the same card will succeed immediately. Resources have a > - * counter, so locks are only released if the counter reaches 0. > + * Release resources previously locked by vga_get() or vga_tryget(). The > + * resources aren't disabled right away, so that a subsequent vga_get() on > + * the same card will succeed immediately. Resources have a counter, so > + * locks are only released if the counter reaches 0. > */ > void vga_put(struct pci_dev *pdev, unsigned int rsrc) > { > struct vga_device *vgadev; > unsigned long flags; > > - /* The one who calls us should check for this, but lets be sure... */ > + /* The caller should check for this, but let's be sure */ > if (pdev == NULL) > pdev = vga_default_device(); > if (pdev == NULL) > @@ -709,20 +707,20 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) > return; > } > > - /* okay iterate the new devices bridge hierarachy */ > + /* Iterate the new device's bridge hierarchy */ > new_bus = vgadev->pdev->bus; > while (new_bus) { > new_bridge = new_bus->self; > > - /* go through list of devices already registered */ > + /* Go through list of devices already registered */ > list_for_each_entry(same_bridge_vgadev, &vga_list, list) { > bus = same_bridge_vgadev->pdev->bus; > bridge = bus->self; > > - /* See if the share a bridge with this device */ > + /* See if it shares a bridge with this device */ > if (new_bridge == bridge) { > /* > - * If their direct parent bridge is the same > + * If its direct parent bridge is the same > * as any bridge of this device then it can't > * be used for that device. > */ > @@ -730,10 +728,10 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) > } > > /* > - * Now iterate the previous devices bridge hierarchy. > - * If the new devices parent bridge is in the other > - * devices hierarchy then we can't use it to control > - * this device > + * Now iterate the previous device's bridge hierarchy. > + * If the new device's parent bridge is in the other > + * device's hierarchy, we can't use it to control this > + * device. > */ > while (bus) { > bridge = bus->self; > @@ -754,10 +752,9 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) > } > > /* > - * Currently, we assume that the "initial" setup of the system is > - * not sane, that is we come up with conflicting devices and let > - * the arbiter's client decides if devices decodes or not legacy > - * things. > + * Currently, we assume that the "initial" setup of the system is not sane, > + * that is, we come up with conflicting devices and let the arbiter's > + * client decide if devices decodes legacy things or not. > */ > static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) > { > @@ -767,12 +764,16 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) > struct pci_dev *bridge; > u16 cmd; > > + /* Only deal with VGA class devices */ > + if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA) > + return false; > + > /* Allocate structure */ > vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL); > if (vgadev == NULL) { > vgaarb_err(&pdev->dev, "failed to allocate VGA arbiter data\n"); > /* > - * What to do on allocation failure ? For now, let's just do > + * What to do on allocation failure? For now, let's just do > * nothing, I'm not sure there is anything saner to be done. > */ > return false; > @@ -792,9 +793,10 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) > > /* By default, mark it as decoding */ > vga_decode_count++; > + > /* > * Mark that we "own" resources based on our enables, we will > - * clear that below if the bridge isn't forwarding > + * clear that below if the bridge isn't forwarding. > */ > pci_read_config_word(pdev, PCI_COMMAND, &cmd); > if (cmd & PCI_COMMAND_IO) > @@ -874,7 +876,7 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev) > return ret; > } > > -/* This is called with the lock */ > +/* Called with the lock */ > static void vga_update_device_decodes(struct vga_device *vgadev, > unsigned int new_decodes) > { > @@ -885,8 +887,7 @@ static void vga_update_device_decodes(struct vga_device *vgadev, > > vgadev->decodes = new_decodes; > > - vgaarb_info(dev, > - "VGA decodes changed: olddecodes=%s,decodes=%s:owns=%s\n", > + vgaarb_info(dev, "VGA decodes changed: 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)); > @@ -932,9 +933,9 @@ static void __vga_set_legacy_decoding(struct pci_dev *pdev, > vga_update_device_decodes(vgadev, decodes); > > /* > - * XXX if somebody is going from "doesn't decode" to "decodes" state > - * here, additional care must be taken as we may have pending owner > - * ship of non-legacy region ... > + * XXX If somebody is going from "doesn't decode" to "decodes" > + * state here, additional care must be taken as we may have pending > + * ownership of non-legacy region. > */ > bail: > spin_unlock_irqrestore(&vga_lock, flags); > @@ -942,10 +943,10 @@ static void __vga_set_legacy_decoding(struct pci_dev *pdev, > > /** > * vga_set_legacy_decoding > - * @pdev: pci device of the VGA card > + * @pdev: PCI device of the VGA card > * @decodes: bit mask of what legacy regions the card decodes > * > - * Indicates to the arbiter if the card decodes legacy VGA IOs, legacy VGA > + * Indicate to the arbiter if the card decodes legacy VGA IOs, legacy VGA > * Memory, both, or none. All cards default to both, the card driver (fbdev for > * example) should tell the arbiter if it has disabled legacy decoding, so the > * card can be left out of the arbitration process (and can be safe to take > @@ -959,44 +960,42 @@ EXPORT_SYMBOL(vga_set_legacy_decoding); > > /** > * vga_client_register - register or unregister a VGA arbitration client > - * @pdev: pci device of the VGA client > - * @set_decode: vga decode change callback > + * @pdev: PCI device of the VGA client > + * @set_decode: VGA decode change callback > * > * Clients have two callback mechanisms they can use. > * > * @set_decode callback: If a client can disable its GPU VGA resource, it > * will get a callback from this to set the encode/decode state. > * > - * Rationale: we cannot disable VGA decode resources unconditionally, some > - * single GPU laptops seem to require ACPI or BIOS access to the VGA registers > - * to control things like backlights etc. Hopefully newer multi-GPU laptops do > - * something saner, and desktops won't have any special ACPI for this. The > - * driver will get a callback when VGA arbitration is first used by userspace > - * since some older X servers have issues. > + * Rationale: we cannot disable VGA decode resources unconditionally > + * because some single GPU laptops seem to require ACPI or BIOS access to > + * the VGA registers to control things like backlights etc. Hopefully newer > + * multi-GPU laptops do something saner, and desktops won't have any > + * special ACPI for this. The driver will get a callback when VGA > + * arbitration is first used by userspace since some older X servers have > + * issues. > * > - * This function does not check whether a client for @pdev has been registered > - * already. > + * Does not check whether a client for @pdev has been registered already. > * > - * To unregister just call vga_client_unregister(). > + * To unregister, call vga_client_unregister(). > * > * Returns: 0 on success, -ENODEV on failure > */ > int vga_client_register(struct pci_dev *pdev, > unsigned int (*set_decode)(struct pci_dev *pdev, bool decode)) > { > - int ret = -ENODEV; > - struct vga_device *vgadev; > unsigned long flags; > + struct vga_device *vgadev; > > spin_lock_irqsave(&vga_lock, flags); > vgadev = vgadev_find(pdev); > - if (vgadev) { > + if (vgadev) > vgadev->set_decode = set_decode; > - ret = 0; > - } > spin_unlock_irqrestore(&vga_lock, flags); > - > - return ret; > + if (!vgadev) > + return -ENODEV; > + return 0; > } > EXPORT_SYMBOL(vga_client_register); > > @@ -1005,13 +1004,13 @@ EXPORT_SYMBOL(vga_client_register); > * > * Semantics is: > * > - * open : open user instance of the arbitrer. by default, it's > + * open : Open user instance of the arbiter. By default, it's > * attached to the default VGA device of the system. > * > - * close : close user instance, release locks > + * close : Close user instance, release locks > * > - * read : return a string indicating the status of the target. > - * an IO state string is of the form {io,mem,io+mem,none}, > + * read : Return a string indicating the status of the target. > + * An IO state string is of the form {io,mem,io+mem,none}, > * mc and ic are respectively mem and io lock counts (for > * debugging/diagnostic only). "decodes" indicate what the > * card currently decodes, "owns" indicates what is currently > @@ -1025,7 +1024,7 @@ EXPORT_SYMBOL(vga_client_register); > * write : write a command to the arbiter. List of commands is: > * > * target <card_ID> : switch target to card <card_ID> (see below) > - * lock <io_state> : acquires locks on target ("none" is invalid io_state) > + * lock <io_state> : acquire locks on target ("none" is invalid io_state) > * trylock <io_state> : non-blocking acquire locks on target > * unlock <io_state> : release locks on target > * unlock all : release all locks on target held by this user > @@ -1042,23 +1041,21 @@ EXPORT_SYMBOL(vga_client_register); > * Note about locks: > * > * The driver keeps track of which user has what locks on which card. It > - * supports stacking, like the kernel one. This complexifies the implementation > + * supports stacking, like the kernel one. This complicates the implementation > * a bit, but makes the arbiter more tolerant to userspace problems and able > * to properly cleanup in all cases when a process dies. > * Currently, a max of 16 cards simultaneously can have locks issued from > * userspace for a given user (file descriptor instance) of the arbiter. > * > * If the device is hot-unplugged, there is a hook inside the module to notify > - * they being added/removed in the system and automatically added/removed in > + * it being added/removed in the system and automatically added/removed in > * the arbiter. > */ > > #define MAX_USER_CARDS CONFIG_VGA_ARB_MAX_GPUS > #define PCI_INVALID_CARD ((struct pci_dev *)-1UL) > > -/* > - * Each user has an array of these, tracking which cards have locks > - */ > +/* Each user has an array of these, tracking which cards have locks */ > struct vga_arb_user_card { > struct pci_dev *pdev; > unsigned int mem_cnt; > @@ -1077,9 +1074,8 @@ static DEFINE_SPINLOCK(vga_user_lock); > > > /* > - * This function gets a string in the format: "PCI:domain:bus:dev.fn" and > - * returns the respective values. If the string is not in this format, > - * it returns 0. > + * Take a string in the format: "PCI:domain:bus:dev.fn" and return the > + * respective values. If the string is not in this format, return 0. > */ > static int vga_pci_str_to_vars(char *buf, int count, unsigned int *domain, > unsigned int *bus, unsigned int *devfn) > @@ -1111,7 +1107,7 @@ static ssize_t vga_arb_read(struct file *file, char __user *buf, > if (lbuf == NULL) > return -ENOMEM; > > - /* Protects vga_list */ > + /* Protect vga_list */ > spin_lock_irqsave(&vga_lock, flags); > > /* If we are targeting the default, use it */ > @@ -1126,15 +1122,15 @@ static ssize_t vga_arb_read(struct file *file, char __user *buf, > vgadev = vgadev_find(pdev); > if (vgadev == NULL) { > /* > - * Wow, it's not in the list, that shouldn't happen, > - * let's fix us up and return invalid card > + * Wow, it's not in the list, that shouldn't happen, let's > + * fix us up and return invalid card. > */ > spin_unlock_irqrestore(&vga_lock, flags); > len = sprintf(lbuf, "invalid"); > goto done; > } > > - /* Fill the buffer with infos */ > + /* Fill the buffer with info */ > len = snprintf(lbuf, 1024, > "count:%d,PCI:%s,decodes=%s,owns=%s,locks=%s(%u:%u)\n", > vga_decode_count, pci_name(pdev), > @@ -1180,7 +1176,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf, > if (copy_from_user(kbuf, buf, count)) > return -EFAULT; > curr_pos = kbuf; > - kbuf[count] = '\0'; /* Just to make sure... */ > + kbuf[count] = '\0'; > > if (strncmp(curr_pos, "lock ", 5) == 0) { > curr_pos += 5; > @@ -1205,7 +1201,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf, > > vga_get_uninterruptible(pdev, io_state); > > - /* Update the client's locks lists... */ > + /* Update the client's locks lists */ > for (i = 0; i < MAX_USER_CARDS; i++) { > if (priv->cards[i].pdev == pdev) { > if (io_state & VGA_RSRC_LEGACY_IO) > @@ -1372,7 +1368,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf, > vgaarb_dbg(&pdev->dev, "maximum user cards (%d) number reached, ignoring this one!\n", > MAX_USER_CARDS); > pci_dev_put(pdev); > - /* XXX: which value to return? */ > + /* XXX: Which value to return? */ > ret_val = -ENOMEM; > goto done; > } > @@ -1433,7 +1429,7 @@ static int vga_arb_open(struct inode *inode, struct file *file) > list_add(&priv->list, &vga_user_list); > spin_unlock_irqrestore(&vga_user_lock, flags); > > - /* Set the client' lists of locks */ > + /* Set the client's lists of locks */ > priv->target = vga_default_device(); /* Maybe this is still null! */ > priv->cards[0].pdev = priv->target; > priv->cards[0].io_cnt = 0; > @@ -1472,68 +1468,32 @@ static int vga_arb_release(struct inode *inode, struct file *file) > } > > /* > - * Callback any registered clients to let them know we have a > - * change in VGA cards > + * Callback any registered clients to let them know we have a change in VGA > + * cards. > */ > static void vga_arbiter_notify_clients(void) > { > struct vga_device *vgadev; > unsigned long flags; > - bool state; > + unsigned int new_decodes; > + bool new_state; > > if (!vga_arbiter_used) > return; > > - state = (vga_count > 1) ? false : true; > + new_state = (vga_count > 1) ? false : true; > > spin_lock_irqsave(&vga_lock, flags); > list_for_each_entry(vgadev, &vga_list, list) { > if (vgadev->set_decode) { > - unsigned int decodes; > - > - decodes = vgadev->set_decode(vgadev->pdev, state); > - vga_update_device_decodes(vgadev, decodes); > + new_decodes = vgadev->set_decode(vgadev->pdev, > + new_state); > + vga_update_device_decodes(vgadev, new_decodes); > } > } > spin_unlock_irqrestore(&vga_lock, flags); > } > > -/* > - * The PCI Class Code spec implies that only VGA devices with programming > - * interface 0x00 can depend on the legacy VGA address range. VGA devices > - * with programming interface 0x01 are 8514-compatible controllers. Since > - * VGA devices with programming interface 0x00 is VGA compatible, the 'vga' > - * suffix here should refer to the VGA-compatible devices after a strict > - * reading of that specification. But considering the fact that there > - * probably don't has a 8514-compatible controller that could be used with > - * upstream kernel anymore, we would like to just ignore the programming > - * interface byte. > - * > - * Besides, there do exist non VGA-compatible display controllers in the > - * world and hardware vendors may abandon the old VGA standard someday. > - * The meaning of 'vga' suffix here may change to evolve with time. > - * > - * A strict understanding of 'vga' certainly should be VGA-compatible, While > - * a relaxed understanding of 'vga' would be PCI devices that are able to > - * display. Currently, we just keep aligned to the previous behavior. > - * Deal with VGA class devices. > - */ > -static bool pci_dev_is_vga(struct pci_dev *pdev) > -{ > - if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) > - return true; > - > - /* > - * The PCI_CLASS_NOT_DEFINED_VGA is defined to provide backward > - * compatibility for devices that were built before the class code > - * field was defined. > - */ > - if ((pdev->class >> 8) == PCI_CLASS_NOT_DEFINED_VGA) > - return true; > - > - return false; > -} > - > static int pci_notify(struct notifier_block *nb, unsigned long action, > void *data) > { > @@ -1543,9 +1503,6 @@ static int pci_notify(struct notifier_block *nb, unsigned long action, > > vgaarb_dbg(dev, "%s\n", __func__); > > - if (!pci_dev_is_vga(pdev)) > - return 0; > - > /* > * For now, we're only interested in devices added and removed. > * I didn't test this thing here, so someone needs to double check > @@ -1580,8 +1537,8 @@ static struct miscdevice vga_arb_device = { > > static int __init vga_arb_device_init(void) > { > - struct pci_dev *pdev = NULL; > int rc; > + struct pci_dev *pdev; > > rc = misc_register(&vga_arb_device); > if (rc < 0) > @@ -1589,22 +1546,12 @@ static int __init vga_arb_device_init(void) > > bus_register_notifier(&pci_bus_type, &pci_notifier); > > - /* > - * We add all PCI devices satisfying VGA class in the arbiter > - * by default, but we ignore the programming interface byte > - * intentionally. > - */ > - do { > - pdev = pci_get_class_masked(PCI_CLASS_DISPLAY_VGA << 8, 0xFFFF00, pdev); > - if (pdev && pci_dev_is_vga(pdev)) > - vga_arbiter_add_pci_device(pdev); > - } while (pdev); > - > - do { > - pdev = pci_get_class_masked(PCI_CLASS_NOT_DEFINED_VGA << 8, 0xFFFF00, pdev); > - if (pdev && pci_dev_is_vga(pdev)) > - vga_arbiter_add_pci_device(pdev); > - } while (pdev); > + /* Add all VGA class PCI devices by default */ > + pdev = NULL; > + while ((pdev = > + pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, > + PCI_ANY_ID, pdev)) != NULL) > + vga_arbiter_add_pci_device(pdev); > > pr_info("loaded\n"); > return rc; > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 14dc5ab4a1d2..c69a2cc1f412 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1180,9 +1180,6 @@ struct pci_dev *pci_get_slot(struct pci_bus *bus, unsigned int devfn); > struct pci_dev *pci_get_domain_bus_and_slot(int domain, unsigned int bus, > unsigned int devfn); > struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from); > -struct pci_dev *pci_get_class_masked(unsigned int class, unsigned int mask, > - struct pci_dev *from); > - > int pci_dev_present(const struct pci_device_id *ids); > > int pci_bus_read_config_byte(struct pci_bus *bus, unsigned int devfn, > @@ -1898,10 +1895,6 @@ static inline struct pci_dev *pci_get_class(unsigned int class, > struct pci_dev *from) > { return NULL; } > > -static inline struct pci_dev *pci_get_class_masked(unsigned int class, > - unsigned int mask, > - struct pci_dev *from) > -{ return NULL; } > > static inline int pci_dev_present(const struct pci_device_id *ids) > { return 0; }
From: Sui Jingfeng <suijingfeng@loongson.cn> v1: * Various improve. v2: * More fixes, optimizations and improvements. Sui Jingfeng (11): PCI/VGA: Use unsigned type for the io_state variable PCI: Add the pci_get_class_masked() helper PCI/VGA: Deal with VGA class devices PCI/VGA: Drop the inline in the vga_update_device_decodes() function. PCI/VGA: Move the new_state assignment out of the loop PCI/VGA: Fix two typos in the comments of pci_notify() PCI/VGA: vga_client_register() return -ENODEV on failure, not -1 PCI/VGA: Fix a typo to the comment of vga_default PCI/VGA: Fix a typo to the comments in vga_str_to_iostate() function PCI/VGA: Tidy up the code and comment format PCI/VGA: Replace full MIT license text with SPDX identifier drivers/pci/search.c | 30 ++++++ drivers/pci/vgaarb.c | 233 +++++++++++++++++++++++++---------------- include/linux/pci.h | 7 ++ include/linux/vgaarb.h | 27 +---- 4 files changed, 185 insertions(+), 112 deletions(-) base-commit: 69286072664490a366f3331f9496fe78efaca603