[PATCH v5 1/4] PCI/VGA: Use unsigned type for the io_state variable

[PATCH v5 1/4] PCI/VGA: Use unsigned type for the io_state variable

[Sui Jingfeng]

The io_state variable in the vga_arb_write() function is declared with
unsigned int type, while the vga_str_to_iostate() function takes int *
type. To keep them consistent, replace the third argument of
vga_str_to_iostate() function with the unsigned int * type.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/pci/vgaarb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 5a696078b382..c1bc6c983932 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -77,7 +77,7 @@ static const char *vga_iostate_to_str(unsigned int iostate)
 	return "none";
 }
 
-static int vga_str_to_iostate(char *buf, int str_size, int *io_state)
+static int vga_str_to_iostate(char *buf, int str_size, unsigned int *io_state)
 {
 	/* we could in theory hand out locks on IO and mem
 	 * separately to userspace but it can cause deadlocks */
-- 
2.25.1

[PATCH v5 2/4] PCI/VGA: Deal only with VGA class devices

[Sui Jingfeng]

Deal only with the VGA devcie(pdev->class == 0x0300), so replace the
pci_get_subsys() function with pci_get_class(). Filter the non-PCI display
device(pdev->class != 0x0300) out. There no need to process the non-display
PCI device.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/pci/vgaarb.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index c1bc6c983932..22a505e877dc 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -754,10 +754,6 @@ 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) {
@@ -1500,7 +1496,9 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
 	struct pci_dev *pdev = to_pci_dev(dev);
 	bool notify = false;
 
-	vgaarb_dbg(dev, "%s\n", __func__);
+	/* Only deal with VGA class devices */
+	if (pdev->class != PCI_CLASS_DISPLAY_VGA << 8)
+		return 0;
 
 	/* 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
@@ -1510,6 +1508,8 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
 	else if (action == BUS_NOTIFY_DEL_DEVICE)
 		notify = vga_arbiter_del_pci_device(pdev);
 
+	vgaarb_dbg(dev, "%s: action = %lu\n", __func__, action);
+
 	if (notify)
 		vga_arbiter_notify_clients();
 	return 0;
@@ -1534,8 +1534,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)
@@ -1545,11 +1545,13 @@ static int __init vga_arb_device_init(void)
 
 	/* We add all PCI devices satisfying VGA class in the arbiter by
 	 * default */
-	pdev = NULL;
-	while ((pdev =
-		pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
-			       PCI_ANY_ID, pdev)) != NULL)
+	while (1) {
+		pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev);
+		if (!pdev)
+			break;
+
 		vga_arbiter_add_pci_device(pdev);
+	}
 
 	pr_info("loaded\n");
 	return rc;
-- 
2.25.1

[PATCH v5 3/4] PCI/VGA: Tidy up the code and comment format

[Sui Jingfeng]

This patch replaces the leading space with a tab and removes the double
blank line, no functional change.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/pci/vgaarb.c   | 108 ++++++++++++++++++++++++-----------------
 include/linux/vgaarb.h |   4 +-
 2 files changed, 65 insertions(+), 47 deletions(-)

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 22a505e877dc..ceb914245383 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -61,7 +61,6 @@ static bool vga_arbiter_used;
 static DEFINE_SPINLOCK(vga_lock);
 static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue);
 
-
 static const char *vga_iostate_to_str(unsigned int iostate)
 {
 	/* Ignore VGA_RSRC_IO and VGA_RSRC_MEM */
@@ -79,8 +78,10 @@ static const char *vga_iostate_to_str(unsigned int iostate)
 
 static int vga_str_to_iostate(char *buf, int str_size, unsigned int *io_state)
 {
-	/* we could in theory hand out locks on IO and mem
-	 * separately to userspace but it can cause deadlocks */
+	/*
+	 * We could in theory hand out locks on IO and mem
+	 * separately to userspace but it can cause deadlocks
+	 */
 	if (strncmp(buf, "none", 4) == 0) {
 		*io_state = VGA_RSRC_NONE;
 		return 1;
@@ -99,7 +100,7 @@ static int vga_str_to_iostate(char *buf, int str_size, unsigned int *io_state)
 	return 1;
 }
 
-/* this is only used a cookie - it should not be dereferenced */
+/* This is only used as cookie, it should not be dereferenced */
 static struct pci_dev *vga_default;
 
 /* Find somebody in our list */
@@ -193,14 +194,17 @@ int vga_remove_vgacon(struct pci_dev *pdev)
 #endif
 EXPORT_SYMBOL(vga_remove_vgacon);
 
-/* If we don't ever use VGA arb 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 arb 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
-	 * VGA arb has occurred and to try and disable resources
-	 * if they can */
+	/*
+	 * We should inform all GPUs in the system that
+	 * vgaarb has occurred and to try and disable resources if they can
+	 */
 	if (!vga_arbiter_used) {
 		vga_arbiter_used = true;
 		vga_arbiter_notify_clients();
@@ -216,7 +220,8 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
 	unsigned int pci_bits;
 	u32 flags = 0;
 
-	/* Account for "normal" resources to lock. If we decode the legacy,
+	/*
+	 * Account for "normal" resources to lock. If we decode the legacy,
 	 * counterpart, we need to request it as well
 	 */
 	if ((rsrc & VGA_RSRC_NORMAL_IO) &&
@@ -236,7 +241,8 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
 	if (wants == 0)
 		goto lock_them;
 
-	/* We don't need to request a legacy resource, we just enable
+	/*
+	 * We don't need to request a legacy resource, we just enable
 	 * appropriate decoding and go
 	 */
 	legacy_wants = wants & VGA_RSRC_LEGACY_MASK;
@@ -252,7 +258,8 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
 		if (vgadev == conflict)
 			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
 		 * together since there is only a single bit controlling
@@ -263,13 +270,15 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
 			lwants = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM;
 		}
 
-		/* Check if the guy has a lock on the resource. If he does,
+		/*
+		 * Check if the guy has a lock on the resource. If he does,
 		 * return the conflicting entry
 		 */
 		if (conflict->locks & lwants)
 			return conflict;
 
-		/* Ok, now check if it owns the resource we want.  We can
+		/*
+		 * Ok, now check if it owns the resource we want.  We can
 		 * lock resources that are not decoded, therefore a device
 		 * can own resources it doesn't decode.
 		 */
@@ -277,14 +286,16 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
 		if (!match)
 			continue;
 
-		/* looks like he doesn't have a lock, we can steal
+		/*
+		 * Looks like he doesn't have a lock, we can steal
 		 * them from him
 		 */
 
 		flags = 0;
 		pci_bits = 0;
 
-		/* If we can't control legacy resources via the bridge, we
+		/*
+		 * If we can't control legacy resources via the bridge, we
 		 * also need to disable normal decoding.
 		 */
 		if (!conflict->bridge_has_one_vga) {
@@ -311,7 +322,8 @@ 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 dude, 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".
@@ -353,7 +365,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
+	/*
+	 * Update our counters, and account for equivalent legacy resources
 	 * if we decode them
 	 */
 	if ((rsrc & VGA_RSRC_NORMAL_IO) && vgadev->io_norm_cnt > 0) {
@@ -371,7 +384,8 @@ static void __vga_put(struct vga_device *vgadev, unsigned int rsrc)
 	if ((rsrc & VGA_RSRC_LEGACY_MEM) && vgadev->mem_lock_cnt > 0)
 		vgadev->mem_lock_cnt--;
 
-	/* Just clear lock bits, we do lazy operations so we don't really
+	/*
+	 * Just clear lock bits, we do lazy operations so we don't really
 	 * have to bother about anything else at this point
 	 */
 	if (vgadev->io_lock_cnt == 0)
@@ -379,7 +393,8 @@ static void __vga_put(struct vga_device *vgadev, unsigned int rsrc)
 	if (vgadev->mem_lock_cnt == 0)
 		vgadev->locks &= ~VGA_RSRC_LEGACY_MEM;
 
-	/* Kick the wait queue in case somebody was waiting if we actually
+	/*
+	 * Kick the wait queue in case somebody was waiting if we actually
 	 * released something
 	 */
 	if (old_locks != vgadev->locks)
@@ -447,8 +462,8 @@ int vga_get(struct pci_dev *pdev, unsigned int rsrc, int interruptible)
 		if (conflict == NULL)
 			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
@@ -665,7 +680,7 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
 	}
 
 	/*
-	 * vgadev has neither IO nor MEM enabled.  If we haven't found any
+	 * Vgadev has neither IO nor MEM enabled.  If we haven't found any
 	 * other VGA devices, it is the best candidate so far.
 	 */
 	if (!boot_vga)
@@ -706,7 +721,7 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev)
 			bus = same_bridge_vgadev->pdev->bus;
 			bridge = bus->self;
 
-			/* see if the share a bridge with this device */
+			/* See if the share a bridge with this device */
 			if (new_bridge == bridge) {
 				/*
 				 * If their direct parent bridge is the same
@@ -777,9 +792,10 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
 	vgadev->decodes = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM |
 			  VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
 
-	/* by default mark it as decoding */
+	/* By default, mark it as decoding */
 	vga_decode_count++;
-	/* Mark that we "own" resources based on our enables, we will
+	/*
+	 * Mark that we "own" resources based on our enables, we will
 	 * clear that below if the bridge isn't forwarding
 	 */
 	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
@@ -860,7 +876,7 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
 	return ret;
 }
 
-/* this is called with the lock */
+/* This is called with the lock */
 static inline void vga_update_device_decodes(struct vga_device *vgadev,
 					     int new_decodes)
 {
@@ -877,7 +893,7 @@ static inline void vga_update_device_decodes(struct vga_device *vgadev,
 		vga_iostate_to_str(vgadev->decodes),
 		vga_iostate_to_str(vgadev->owns));
 
-	/* if we removed locked decodes, lock count goes to zero, and release */
+	/* If we removed locked decodes, lock count goes to zero, and release */
 	if (decodes_unlocked) {
 		if (decodes_unlocked & VGA_RSRC_LEGACY_IO)
 			vgadev->io_lock_cnt = 0;
@@ -886,7 +902,7 @@ static inline void vga_update_device_decodes(struct vga_device *vgadev,
 		__vga_put(vgadev, decodes_unlocked);
 	}
 
-	/* change decodes counter */
+	/* Change decodes counter */
 	if (old_decodes & VGA_RSRC_LEGACY_MASK &&
 	    !(new_decodes & VGA_RSRC_LEGACY_MASK))
 		vga_decode_count--;
@@ -910,14 +926,15 @@ static void __vga_set_legacy_decoding(struct pci_dev *pdev,
 	if (vgadev == NULL)
 		goto bail;
 
-	/* don't let userspace futz with kernel driver decodes */
+	/* Don't let userspace futz with kernel driver decodes */
 	if (userspace && vgadev->set_decode)
 		goto bail;
 
-	/* update the device decodes + counter */
+	/* Update the device decodes + counter */
 	vga_update_device_decodes(vgadev, decodes);
 
-	/* XXX if somebody is going from "doesn't decode" to "decodes" state
+	/*
+	 * 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 ...
 	 */
@@ -952,9 +969,9 @@ EXPORT_SYMBOL(vga_set_legacy_decoding);
  * @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
+ * 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.
@@ -984,7 +1001,6 @@ int vga_client_register(struct pci_dev *pdev,
 bail:
 	spin_unlock_irqrestore(&vga_lock, flags);
 	return ret;
-
 }
 EXPORT_SYMBOL(vga_client_register);
 
@@ -1075,7 +1091,6 @@ static int vga_pci_str_to_vars(char *buf, int count, unsigned int *domain,
 	int n;
 	unsigned int slot, func;
 
-
 	n = sscanf(buf, "PCI:%x:%x:%x.%x", domain, bus, &slot, &func);
 	if (n != 4)
 		return 0;
@@ -1310,7 +1325,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
 		curr_pos += 7;
 		remaining -= 7;
 		pr_debug("client 0x%p called 'target'\n", priv);
-		/* if target is default */
+		/* If target is default */
 		if (!strncmp(curr_pos, "default", 7))
 			pdev = pci_dev_get(vga_default_device());
 		else {
@@ -1427,7 +1442,6 @@ static int vga_arb_open(struct inode *inode, struct file *file)
 	priv->cards[0].io_cnt = 0;
 	priv->cards[0].mem_cnt = 0;
 
-
 	return 0;
 }
 
@@ -1461,7 +1475,7 @@ static int vga_arb_release(struct inode *inode, struct file *file)
 }
 
 /*
- * callback any registered clients to let them know we have a
+ * Callback any registered clients to let them know we have a
  * change in VGA cards
  */
 static void vga_arbiter_notify_clients(void)
@@ -1500,9 +1514,11 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
 	if (pdev->class != PCI_CLASS_DISPLAY_VGA << 8)
 		return 0;
 
-	/* For now we're only intereted in devices added and removed. I didn't
+	/*
+	 * 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
-	 * cases of hotplugable vga cards. */
+	 * cases of hotplugable vga cards.
+	 */
 	if (action == BUS_NOTIFY_ADD_DEVICE)
 		notify = vga_arbiter_add_pci_device(pdev);
 	else if (action == BUS_NOTIFY_DEL_DEVICE)
@@ -1543,8 +1559,10 @@ 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 */
+	/*
+	 * We add all PCI devices satisfying VGA class in the arbiter by
+	 * default
+	 */
 	while (1) {
 		pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev);
 		if (!pdev)
diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
index b4b9137f9792..6d5465f8c3f2 100644
--- a/include/linux/vgaarb.h
+++ b/include/linux/vgaarb.h
@@ -96,7 +96,7 @@ static inline int vga_client_register(struct pci_dev *pdev,
 static inline int vga_get_interruptible(struct pci_dev *pdev,
 					unsigned int rsrc)
 {
-       return vga_get(pdev, rsrc, 1);
+	return vga_get(pdev, rsrc, 1);
 }
 
 /**
@@ -111,7 +111,7 @@ static inline int vga_get_interruptible(struct pci_dev *pdev,
 static inline int vga_get_uninterruptible(struct pci_dev *pdev,
 					  unsigned int rsrc)
 {
-       return vga_get(pdev, rsrc, 0);
+	return vga_get(pdev, rsrc, 0);
 }
 
 static inline void vga_client_unregister(struct pci_dev *pdev)
-- 
2.25.1

Re: [PATCH v5 3/4] PCI/VGA: Tidy up the code and comment format

[Andi Shyti]

Hi Sui,

On Fri, Jun 09, 2023 at 07:24:16PM +0800, Sui Jingfeng wrote:
> This patch replaces the leading space with a tab and removes the double
> blank line, no functional change.

You mainly fixed comment style, though and it's not written here.

No need to resend for me as you also wrote it in the title. But
next time please list all the changes in the commit log.

> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> 

Andi

> ---
>  drivers/pci/vgaarb.c   | 108 ++++++++++++++++++++++++-----------------
>  include/linux/vgaarb.h |   4 +-
>  2 files changed, 65 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 22a505e877dc..ceb914245383 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -61,7 +61,6 @@ static bool vga_arbiter_used;
>  static DEFINE_SPINLOCK(vga_lock);
>  static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue);
>  
> -
>  static const char *vga_iostate_to_str(unsigned int iostate)
>  {
>  	/* Ignore VGA_RSRC_IO and VGA_RSRC_MEM */
> @@ -79,8 +78,10 @@ static const char *vga_iostate_to_str(unsigned int iostate)
>  
>  static int vga_str_to_iostate(char *buf, int str_size, unsigned int *io_state)
>  {
> -	/* we could in theory hand out locks on IO and mem
> -	 * separately to userspace but it can cause deadlocks */
> +	/*
> +	 * We could in theory hand out locks on IO and mem
> +	 * separately to userspace but it can cause deadlocks
> +	 */
>  	if (strncmp(buf, "none", 4) == 0) {
>  		*io_state = VGA_RSRC_NONE;
>  		return 1;
> @@ -99,7 +100,7 @@ static int vga_str_to_iostate(char *buf, int str_size, unsigned int *io_state)
>  	return 1;
>  }
>  
> -/* this is only used a cookie - it should not be dereferenced */
> +/* This is only used as cookie, it should not be dereferenced */
>  static struct pci_dev *vga_default;
>  
>  /* Find somebody in our list */
> @@ -193,14 +194,17 @@ int vga_remove_vgacon(struct pci_dev *pdev)
>  #endif
>  EXPORT_SYMBOL(vga_remove_vgacon);
>  
> -/* If we don't ever use VGA arb 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 arb 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
> -	 * VGA arb has occurred and to try and disable resources
> -	 * if they can */
> +	/*
> +	 * We should inform all GPUs in the system that
> +	 * vgaarb has occurred and to try and disable resources if they can
> +	 */
>  	if (!vga_arbiter_used) {
>  		vga_arbiter_used = true;
>  		vga_arbiter_notify_clients();
> @@ -216,7 +220,8 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
>  	unsigned int pci_bits;
>  	u32 flags = 0;
>  
> -	/* Account for "normal" resources to lock. If we decode the legacy,
> +	/*
> +	 * Account for "normal" resources to lock. If we decode the legacy,
>  	 * counterpart, we need to request it as well
>  	 */
>  	if ((rsrc & VGA_RSRC_NORMAL_IO) &&
> @@ -236,7 +241,8 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
>  	if (wants == 0)
>  		goto lock_them;
>  
> -	/* We don't need to request a legacy resource, we just enable
> +	/*
> +	 * We don't need to request a legacy resource, we just enable
>  	 * appropriate decoding and go
>  	 */
>  	legacy_wants = wants & VGA_RSRC_LEGACY_MASK;
> @@ -252,7 +258,8 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
>  		if (vgadev == conflict)
>  			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
>  		 * together since there is only a single bit controlling
> @@ -263,13 +270,15 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
>  			lwants = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM;
>  		}
>  
> -		/* Check if the guy has a lock on the resource. If he does,
> +		/*
> +		 * Check if the guy has a lock on the resource. If he does,
>  		 * return the conflicting entry
>  		 */
>  		if (conflict->locks & lwants)
>  			return conflict;
>  
> -		/* Ok, now check if it owns the resource we want.  We can
> +		/*
> +		 * Ok, now check if it owns the resource we want.  We can
>  		 * lock resources that are not decoded, therefore a device
>  		 * can own resources it doesn't decode.
>  		 */
> @@ -277,14 +286,16 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
>  		if (!match)
>  			continue;
>  
> -		/* looks like he doesn't have a lock, we can steal
> +		/*
> +		 * Looks like he doesn't have a lock, we can steal
>  		 * them from him
>  		 */
>  
>  		flags = 0;
>  		pci_bits = 0;
>  
> -		/* If we can't control legacy resources via the bridge, we
> +		/*
> +		 * If we can't control legacy resources via the bridge, we
>  		 * also need to disable normal decoding.
>  		 */
>  		if (!conflict->bridge_has_one_vga) {
> @@ -311,7 +322,8 @@ 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 dude, 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".
> @@ -353,7 +365,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
> +	/*
> +	 * Update our counters, and account for equivalent legacy resources
>  	 * if we decode them
>  	 */
>  	if ((rsrc & VGA_RSRC_NORMAL_IO) && vgadev->io_norm_cnt > 0) {
> @@ -371,7 +384,8 @@ static void __vga_put(struct vga_device *vgadev, unsigned int rsrc)
>  	if ((rsrc & VGA_RSRC_LEGACY_MEM) && vgadev->mem_lock_cnt > 0)
>  		vgadev->mem_lock_cnt--;
>  
> -	/* Just clear lock bits, we do lazy operations so we don't really
> +	/*
> +	 * Just clear lock bits, we do lazy operations so we don't really
>  	 * have to bother about anything else at this point
>  	 */
>  	if (vgadev->io_lock_cnt == 0)
> @@ -379,7 +393,8 @@ static void __vga_put(struct vga_device *vgadev, unsigned int rsrc)
>  	if (vgadev->mem_lock_cnt == 0)
>  		vgadev->locks &= ~VGA_RSRC_LEGACY_MEM;
>  
> -	/* Kick the wait queue in case somebody was waiting if we actually
> +	/*
> +	 * Kick the wait queue in case somebody was waiting if we actually
>  	 * released something
>  	 */
>  	if (old_locks != vgadev->locks)
> @@ -447,8 +462,8 @@ int vga_get(struct pci_dev *pdev, unsigned int rsrc, int interruptible)
>  		if (conflict == NULL)
>  			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
> @@ -665,7 +680,7 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
>  	}
>  
>  	/*
> -	 * vgadev has neither IO nor MEM enabled.  If we haven't found any
> +	 * Vgadev has neither IO nor MEM enabled.  If we haven't found any
>  	 * other VGA devices, it is the best candidate so far.
>  	 */
>  	if (!boot_vga)
> @@ -706,7 +721,7 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev)
>  			bus = same_bridge_vgadev->pdev->bus;
>  			bridge = bus->self;
>  
> -			/* see if the share a bridge with this device */
> +			/* See if the share a bridge with this device */
>  			if (new_bridge == bridge) {
>  				/*
>  				 * If their direct parent bridge is the same
> @@ -777,9 +792,10 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
>  	vgadev->decodes = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM |
>  			  VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
>  
> -	/* by default mark it as decoding */
> +	/* By default, mark it as decoding */
>  	vga_decode_count++;
> -	/* Mark that we "own" resources based on our enables, we will
> +	/*
> +	 * Mark that we "own" resources based on our enables, we will
>  	 * clear that below if the bridge isn't forwarding
>  	 */
>  	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> @@ -860,7 +876,7 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
>  	return ret;
>  }
>  
> -/* this is called with the lock */
> +/* This is called with the lock */
>  static inline void vga_update_device_decodes(struct vga_device *vgadev,
>  					     int new_decodes)
>  {
> @@ -877,7 +893,7 @@ static inline void vga_update_device_decodes(struct vga_device *vgadev,
>  		vga_iostate_to_str(vgadev->decodes),
>  		vga_iostate_to_str(vgadev->owns));
>  
> -	/* if we removed locked decodes, lock count goes to zero, and release */
> +	/* If we removed locked decodes, lock count goes to zero, and release */
>  	if (decodes_unlocked) {
>  		if (decodes_unlocked & VGA_RSRC_LEGACY_IO)
>  			vgadev->io_lock_cnt = 0;
> @@ -886,7 +902,7 @@ static inline void vga_update_device_decodes(struct vga_device *vgadev,
>  		__vga_put(vgadev, decodes_unlocked);
>  	}
>  
> -	/* change decodes counter */
> +	/* Change decodes counter */
>  	if (old_decodes & VGA_RSRC_LEGACY_MASK &&
>  	    !(new_decodes & VGA_RSRC_LEGACY_MASK))
>  		vga_decode_count--;
> @@ -910,14 +926,15 @@ static void __vga_set_legacy_decoding(struct pci_dev *pdev,
>  	if (vgadev == NULL)
>  		goto bail;
>  
> -	/* don't let userspace futz with kernel driver decodes */
> +	/* Don't let userspace futz with kernel driver decodes */
>  	if (userspace && vgadev->set_decode)
>  		goto bail;
>  
> -	/* update the device decodes + counter */
> +	/* Update the device decodes + counter */
>  	vga_update_device_decodes(vgadev, decodes);
>  
> -	/* XXX if somebody is going from "doesn't decode" to "decodes" state
> +	/*
> +	 * 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 ...
>  	 */
> @@ -952,9 +969,9 @@ EXPORT_SYMBOL(vga_set_legacy_decoding);
>   * @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
> + * 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.
> @@ -984,7 +1001,6 @@ int vga_client_register(struct pci_dev *pdev,
>  bail:
>  	spin_unlock_irqrestore(&vga_lock, flags);
>  	return ret;
> -
>  }
>  EXPORT_SYMBOL(vga_client_register);
>  
> @@ -1075,7 +1091,6 @@ static int vga_pci_str_to_vars(char *buf, int count, unsigned int *domain,
>  	int n;
>  	unsigned int slot, func;
>  
> -
>  	n = sscanf(buf, "PCI:%x:%x:%x.%x", domain, bus, &slot, &func);
>  	if (n != 4)
>  		return 0;
> @@ -1310,7 +1325,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
>  		curr_pos += 7;
>  		remaining -= 7;
>  		pr_debug("client 0x%p called 'target'\n", priv);
> -		/* if target is default */
> +		/* If target is default */
>  		if (!strncmp(curr_pos, "default", 7))
>  			pdev = pci_dev_get(vga_default_device());
>  		else {
> @@ -1427,7 +1442,6 @@ static int vga_arb_open(struct inode *inode, struct file *file)
>  	priv->cards[0].io_cnt = 0;
>  	priv->cards[0].mem_cnt = 0;
>  
> -
>  	return 0;
>  }
>  
> @@ -1461,7 +1475,7 @@ static int vga_arb_release(struct inode *inode, struct file *file)
>  }
>  
>  /*
> - * callback any registered clients to let them know we have a
> + * Callback any registered clients to let them know we have a
>   * change in VGA cards
>   */
>  static void vga_arbiter_notify_clients(void)
> @@ -1500,9 +1514,11 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
>  	if (pdev->class != PCI_CLASS_DISPLAY_VGA << 8)
>  		return 0;
>  
> -	/* For now we're only intereted in devices added and removed. I didn't
> +	/*
> +	 * 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
> -	 * cases of hotplugable vga cards. */
> +	 * cases of hotplugable vga cards.
> +	 */
>  	if (action == BUS_NOTIFY_ADD_DEVICE)
>  		notify = vga_arbiter_add_pci_device(pdev);
>  	else if (action == BUS_NOTIFY_DEL_DEVICE)
> @@ -1543,8 +1559,10 @@ 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 */
> +	/*
> +	 * We add all PCI devices satisfying VGA class in the arbiter by
> +	 * default
> +	 */
>  	while (1) {
>  		pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev);
>  		if (!pdev)
> diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
> index b4b9137f9792..6d5465f8c3f2 100644
> --- a/include/linux/vgaarb.h
> +++ b/include/linux/vgaarb.h
> @@ -96,7 +96,7 @@ static inline int vga_client_register(struct pci_dev *pdev,
>  static inline int vga_get_interruptible(struct pci_dev *pdev,
>  					unsigned int rsrc)
>  {
> -       return vga_get(pdev, rsrc, 1);
> +	return vga_get(pdev, rsrc, 1);
>  }
>  
>  /**
> @@ -111,7 +111,7 @@ static inline int vga_get_interruptible(struct pci_dev *pdev,
>  static inline int vga_get_uninterruptible(struct pci_dev *pdev,
>  					  unsigned int rsrc)
>  {
> -       return vga_get(pdev, rsrc, 0);
> +	return vga_get(pdev, rsrc, 0);
>  }
>  
>  static inline void vga_client_unregister(struct pci_dev *pdev)
> -- 
> 2.25.1

Re: [PATCH v5 3/4] PCI/VGA: Tidy up the code and comment format

[Sui Jingfeng]

Hi,

On 2023/6/9 20:00, Andi Shyti wrote:
> Hi Sui,
>
> On Fri, Jun 09, 2023 at 07:24:16PM +0800, Sui Jingfeng wrote:
>> This patch replaces the leading space with a tab and removes the double
>> blank line, no functional change.
> You mainly fixed comment style, though and it's not written here.
>
> No need to resend for me as you also wrote it in the title. But
> next time please list all the changes in the commit log.

OK, I remember this.

This patch is trivial, that's fine.

There are probably more complex patch in future, probably will send to 
you if the checkpatch.pl script  mandate it.

  :-)

Thanks a lot really.

>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
>
> Andi
>
>> ---
>>   drivers/pci/vgaarb.c   | 108 ++++++++++++++++++++++++-----------------
>>   include/linux/vgaarb.h |   4 +-
>>   2 files changed, 65 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
>> index 22a505e877dc..ceb914245383 100644
>> --- a/drivers/pci/vgaarb.c
>> +++ b/drivers/pci/vgaarb.c
>> @@ -61,7 +61,6 @@ static bool vga_arbiter_used;
>>   static DEFINE_SPINLOCK(vga_lock);
>>   static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue);
>>   
>> -
>>   static const char *vga_iostate_to_str(unsigned int iostate)
>>   {
>>   	/* Ignore VGA_RSRC_IO and VGA_RSRC_MEM */
>> @@ -79,8 +78,10 @@ static const char *vga_iostate_to_str(unsigned int iostate)
>>   
>>   static int vga_str_to_iostate(char *buf, int str_size, unsigned int *io_state)
>>   {
>> -	/* we could in theory hand out locks on IO and mem
>> -	 * separately to userspace but it can cause deadlocks */
>> +	/*
>> +	 * We could in theory hand out locks on IO and mem
>> +	 * separately to userspace but it can cause deadlocks
>> +	 */
>>   	if (strncmp(buf, "none", 4) == 0) {
>>   		*io_state = VGA_RSRC_NONE;
>>   		return 1;
>> @@ -99,7 +100,7 @@ static int vga_str_to_iostate(char *buf, int str_size, unsigned int *io_state)
>>   	return 1;
>>   }
>>   
>> -/* this is only used a cookie - it should not be dereferenced */
>> +/* This is only used as cookie, it should not be dereferenced */
>>   static struct pci_dev *vga_default;
>>   
>>   /* Find somebody in our list */
>> @@ -193,14 +194,17 @@ int vga_remove_vgacon(struct pci_dev *pdev)
>>   #endif
>>   EXPORT_SYMBOL(vga_remove_vgacon);
>>   
>> -/* If we don't ever use VGA arb 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 arb 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
>> -	 * VGA arb has occurred and to try and disable resources
>> -	 * if they can */
>> +	/*
>> +	 * We should inform all GPUs in the system that
>> +	 * vgaarb has occurred and to try and disable resources if they can
>> +	 */
>>   	if (!vga_arbiter_used) {
>>   		vga_arbiter_used = true;
>>   		vga_arbiter_notify_clients();
>> @@ -216,7 +220,8 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
>>   	unsigned int pci_bits;
>>   	u32 flags = 0;
>>   
>> -	/* Account for "normal" resources to lock. If we decode the legacy,
>> +	/*
>> +	 * Account for "normal" resources to lock. If we decode the legacy,
>>   	 * counterpart, we need to request it as well
>>   	 */
>>   	if ((rsrc & VGA_RSRC_NORMAL_IO) &&
>> @@ -236,7 +241,8 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
>>   	if (wants == 0)
>>   		goto lock_them;
>>   
>> -	/* We don't need to request a legacy resource, we just enable
>> +	/*
>> +	 * We don't need to request a legacy resource, we just enable
>>   	 * appropriate decoding and go
>>   	 */
>>   	legacy_wants = wants & VGA_RSRC_LEGACY_MASK;
>> @@ -252,7 +258,8 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
>>   		if (vgadev == conflict)
>>   			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
>>   		 * together since there is only a single bit controlling
>> @@ -263,13 +270,15 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
>>   			lwants = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM;
>>   		}
>>   
>> -		/* Check if the guy has a lock on the resource. If he does,
>> +		/*
>> +		 * Check if the guy has a lock on the resource. If he does,
>>   		 * return the conflicting entry
>>   		 */
>>   		if (conflict->locks & lwants)
>>   			return conflict;
>>   
>> -		/* Ok, now check if it owns the resource we want.  We can
>> +		/*
>> +		 * Ok, now check if it owns the resource we want.  We can
>>   		 * lock resources that are not decoded, therefore a device
>>   		 * can own resources it doesn't decode.
>>   		 */
>> @@ -277,14 +286,16 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
>>   		if (!match)
>>   			continue;
>>   
>> -		/* looks like he doesn't have a lock, we can steal
>> +		/*
>> +		 * Looks like he doesn't have a lock, we can steal
>>   		 * them from him
>>   		 */
>>   
>>   		flags = 0;
>>   		pci_bits = 0;
>>   
>> -		/* If we can't control legacy resources via the bridge, we
>> +		/*
>> +		 * If we can't control legacy resources via the bridge, we
>>   		 * also need to disable normal decoding.
>>   		 */
>>   		if (!conflict->bridge_has_one_vga) {
>> @@ -311,7 +322,8 @@ 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 dude, 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".
>> @@ -353,7 +365,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
>> +	/*
>> +	 * Update our counters, and account for equivalent legacy resources
>>   	 * if we decode them
>>   	 */
>>   	if ((rsrc & VGA_RSRC_NORMAL_IO) && vgadev->io_norm_cnt > 0) {
>> @@ -371,7 +384,8 @@ static void __vga_put(struct vga_device *vgadev, unsigned int rsrc)
>>   	if ((rsrc & VGA_RSRC_LEGACY_MEM) && vgadev->mem_lock_cnt > 0)
>>   		vgadev->mem_lock_cnt--;
>>   
>> -	/* Just clear lock bits, we do lazy operations so we don't really
>> +	/*
>> +	 * Just clear lock bits, we do lazy operations so we don't really
>>   	 * have to bother about anything else at this point
>>   	 */
>>   	if (vgadev->io_lock_cnt == 0)
>> @@ -379,7 +393,8 @@ static void __vga_put(struct vga_device *vgadev, unsigned int rsrc)
>>   	if (vgadev->mem_lock_cnt == 0)
>>   		vgadev->locks &= ~VGA_RSRC_LEGACY_MEM;
>>   
>> -	/* Kick the wait queue in case somebody was waiting if we actually
>> +	/*
>> +	 * Kick the wait queue in case somebody was waiting if we actually
>>   	 * released something
>>   	 */
>>   	if (old_locks != vgadev->locks)
>> @@ -447,8 +462,8 @@ int vga_get(struct pci_dev *pdev, unsigned int rsrc, int interruptible)
>>   		if (conflict == NULL)
>>   			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
>> @@ -665,7 +680,7 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
>>   	}
>>   
>>   	/*
>> -	 * vgadev has neither IO nor MEM enabled.  If we haven't found any
>> +	 * Vgadev has neither IO nor MEM enabled.  If we haven't found any
>>   	 * other VGA devices, it is the best candidate so far.
>>   	 */
>>   	if (!boot_vga)
>> @@ -706,7 +721,7 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev)
>>   			bus = same_bridge_vgadev->pdev->bus;
>>   			bridge = bus->self;
>>   
>> -			/* see if the share a bridge with this device */
>> +			/* See if the share a bridge with this device */
>>   			if (new_bridge == bridge) {
>>   				/*
>>   				 * If their direct parent bridge is the same
>> @@ -777,9 +792,10 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
>>   	vgadev->decodes = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM |
>>   			  VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
>>   
>> -	/* by default mark it as decoding */
>> +	/* By default, mark it as decoding */
>>   	vga_decode_count++;
>> -	/* Mark that we "own" resources based on our enables, we will
>> +	/*
>> +	 * Mark that we "own" resources based on our enables, we will
>>   	 * clear that below if the bridge isn't forwarding
>>   	 */
>>   	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
>> @@ -860,7 +876,7 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
>>   	return ret;
>>   }
>>   
>> -/* this is called with the lock */
>> +/* This is called with the lock */
>>   static inline void vga_update_device_decodes(struct vga_device *vgadev,
>>   					     int new_decodes)
>>   {
>> @@ -877,7 +893,7 @@ static inline void vga_update_device_decodes(struct vga_device *vgadev,
>>   		vga_iostate_to_str(vgadev->decodes),
>>   		vga_iostate_to_str(vgadev->owns));
>>   
>> -	/* if we removed locked decodes, lock count goes to zero, and release */
>> +	/* If we removed locked decodes, lock count goes to zero, and release */
>>   	if (decodes_unlocked) {
>>   		if (decodes_unlocked & VGA_RSRC_LEGACY_IO)
>>   			vgadev->io_lock_cnt = 0;
>> @@ -886,7 +902,7 @@ static inline void vga_update_device_decodes(struct vga_device *vgadev,
>>   		__vga_put(vgadev, decodes_unlocked);
>>   	}
>>   
>> -	/* change decodes counter */
>> +	/* Change decodes counter */
>>   	if (old_decodes & VGA_RSRC_LEGACY_MASK &&
>>   	    !(new_decodes & VGA_RSRC_LEGACY_MASK))
>>   		vga_decode_count--;
>> @@ -910,14 +926,15 @@ static void __vga_set_legacy_decoding(struct pci_dev *pdev,
>>   	if (vgadev == NULL)
>>   		goto bail;
>>   
>> -	/* don't let userspace futz with kernel driver decodes */
>> +	/* Don't let userspace futz with kernel driver decodes */
>>   	if (userspace && vgadev->set_decode)
>>   		goto bail;
>>   
>> -	/* update the device decodes + counter */
>> +	/* Update the device decodes + counter */
>>   	vga_update_device_decodes(vgadev, decodes);
>>   
>> -	/* XXX if somebody is going from "doesn't decode" to "decodes" state
>> +	/*
>> +	 * 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 ...
>>   	 */
>> @@ -952,9 +969,9 @@ EXPORT_SYMBOL(vga_set_legacy_decoding);
>>    * @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
>> + * 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.
>> @@ -984,7 +1001,6 @@ int vga_client_register(struct pci_dev *pdev,
>>   bail:
>>   	spin_unlock_irqrestore(&vga_lock, flags);
>>   	return ret;
>> -
>>   }
>>   EXPORT_SYMBOL(vga_client_register);
>>   
>> @@ -1075,7 +1091,6 @@ static int vga_pci_str_to_vars(char *buf, int count, unsigned int *domain,
>>   	int n;
>>   	unsigned int slot, func;
>>   
>> -
>>   	n = sscanf(buf, "PCI:%x:%x:%x.%x", domain, bus, &slot, &func);
>>   	if (n != 4)
>>   		return 0;
>> @@ -1310,7 +1325,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
>>   		curr_pos += 7;
>>   		remaining -= 7;
>>   		pr_debug("client 0x%p called 'target'\n", priv);
>> -		/* if target is default */
>> +		/* If target is default */
>>   		if (!strncmp(curr_pos, "default", 7))
>>   			pdev = pci_dev_get(vga_default_device());
>>   		else {
>> @@ -1427,7 +1442,6 @@ static int vga_arb_open(struct inode *inode, struct file *file)
>>   	priv->cards[0].io_cnt = 0;
>>   	priv->cards[0].mem_cnt = 0;
>>   
>> -
>>   	return 0;
>>   }
>>   
>> @@ -1461,7 +1475,7 @@ static int vga_arb_release(struct inode *inode, struct file *file)
>>   }
>>   
>>   /*
>> - * callback any registered clients to let them know we have a
>> + * Callback any registered clients to let them know we have a
>>    * change in VGA cards
>>    */
>>   static void vga_arbiter_notify_clients(void)
>> @@ -1500,9 +1514,11 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
>>   	if (pdev->class != PCI_CLASS_DISPLAY_VGA << 8)
>>   		return 0;
>>   
>> -	/* For now we're only intereted in devices added and removed. I didn't
>> +	/*
>> +	 * 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
>> -	 * cases of hotplugable vga cards. */
>> +	 * cases of hotplugable vga cards.
>> +	 */
>>   	if (action == BUS_NOTIFY_ADD_DEVICE)
>>   		notify = vga_arbiter_add_pci_device(pdev);
>>   	else if (action == BUS_NOTIFY_DEL_DEVICE)
>> @@ -1543,8 +1559,10 @@ 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 */
>> +	/*
>> +	 * We add all PCI devices satisfying VGA class in the arbiter by
>> +	 * default
>> +	 */
>>   	while (1) {
>>   		pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev);
>>   		if (!pdev)
>> diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
>> index b4b9137f9792..6d5465f8c3f2 100644
>> --- a/include/linux/vgaarb.h
>> +++ b/include/linux/vgaarb.h
>> @@ -96,7 +96,7 @@ static inline int vga_client_register(struct pci_dev *pdev,
>>   static inline int vga_get_interruptible(struct pci_dev *pdev,
>>   					unsigned int rsrc)
>>   {
>> -       return vga_get(pdev, rsrc, 1);
>> +	return vga_get(pdev, rsrc, 1);
>>   }
>>   
>>   /**
>> @@ -111,7 +111,7 @@ static inline int vga_get_interruptible(struct pci_dev *pdev,
>>   static inline int vga_get_uninterruptible(struct pci_dev *pdev,
>>   					  unsigned int rsrc)
>>   {
>> -       return vga_get(pdev, rsrc, 0);
>> +	return vga_get(pdev, rsrc, 0);
>>   }
>>   
>>   static inline void vga_client_unregister(struct pci_dev *pdev)
>> -- 
>> 2.25.1

-- 
Jingfeng

[PATCH v5 4/4] PCI/VGA: Replace full MIT license text with SPDX identifier

[Sui Jingfeng]

Per Documentation/process/license-rules.rst, the SPDX MIT identifier is
equivalent to including the entire MIT license text from
LICENSES/preferred/MIT.

Replace the MIT license text with the equivalent SPDX identifier.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 include/linux/vgaarb.h | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
index 6d5465f8c3f2..97129a1bbb7d 100644
--- a/include/linux/vgaarb.h
+++ b/include/linux/vgaarb.h
@@ -1,3 +1,5 @@
+/* SPDX-License-Identifier: MIT */
+
 /*
  * The VGA aribiter manages VGA space routing and VGA resource decode to
  * allow multiple VGA devices to be used in a system in a safe way.
@@ -5,27 +7,6 @@
  * (C) Copyright 2005 Benjamin Herrenschmidt <benh@kernel.crashing.org>
  * (C) Copyright 2007 Paulo R. Zanoni <przanoni@gmail.com>
  * (C) Copyright 2007, 2009 Tiago Vignatti <vignatti@freedesktop.org>
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the "Software"),
- * to deal in the Software without restriction, including without limitation
- * the rights to use, copy, modify, merge, publish, distribute, sublicense,
- * and/or sell copies of the Software, and to permit persons to whom the
- * Software is furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice (including the next
- * paragraph) shall be included in all copies or substantial portions of the
- * Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
- * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS
- * IN THE SOFTWARE.
- *
  */
 
 #ifndef LINUX_VGA_H
-- 
2.25.1

Re: [PATCH v5 4/4] PCI/VGA: Replace full MIT license text with SPDX identifier

[Andi Shyti]

On Fri, Jun 09, 2023 at 07:24:17PM +0800, Sui Jingfeng wrote:
> Per Documentation/process/license-rules.rst, the SPDX MIT identifier is
> equivalent to including the entire MIT license text from
> LICENSES/preferred/MIT.
> 
> Replace the MIT license text with the equivalent SPDX identifier.
> 
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> 

Andi

Re: [PATCH v5 1/4] PCI/VGA: Use unsigned type for the io_state variable

[Andi Shyti]

Hi Sui,

On Fri, Jun 09, 2023 at 07:24:14PM +0800, Sui Jingfeng wrote:
> The io_state variable in the vga_arb_write() function is declared with
> unsigned int type, while the vga_str_to_iostate() function takes int *
> type. To keep them consistent, replace the third argument of
> vga_str_to_iostate() function with the unsigned int * type.
> 
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> 

Andi

> ---
>  drivers/pci/vgaarb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 5a696078b382..c1bc6c983932 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -77,7 +77,7 @@ static const char *vga_iostate_to_str(unsigned int iostate)
>  	return "none";
>  }
>  
> -static int vga_str_to_iostate(char *buf, int str_size, int *io_state)
> +static int vga_str_to_iostate(char *buf, int str_size, unsigned int *io_state)
>  {
>  	/* we could in theory hand out locks on IO and mem
>  	 * separately to userspace but it can cause deadlocks */
> -- 
> 2.25.1

Re: [PATCH v5 1/4] PCI/VGA: Use unsigned type for the io_state variable

[Sui Jingfeng]

Hi,

On 2023/6/9 19:48, Andi Shyti wrote:
> Hi Sui,
>
> On Fri, Jun 09, 2023 at 07:24:14PM +0800, Sui Jingfeng wrote:
>> The io_state variable in the vga_arb_write() function is declared with
>> unsigned int type, while the vga_str_to_iostate() function takes int *
>> type. To keep them consistent, replace the third argument of
>> vga_str_to_iostate() function with the unsigned int * type.
>>
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
Thanks a lot.
> Andi
>
>> ---
>>   drivers/pci/vgaarb.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
>> index 5a696078b382..c1bc6c983932 100644
>> --- a/drivers/pci/vgaarb.c
>> +++ b/drivers/pci/vgaarb.c
>> @@ -77,7 +77,7 @@ static const char *vga_iostate_to_str(unsigned int iostate)
>>   	return "none";
>>   }
>>   
>> -static int vga_str_to_iostate(char *buf, int str_size, int *io_state)
>> +static int vga_str_to_iostate(char *buf, int str_size, unsigned int *io_state)
>>   {
>>   	/* we could in theory hand out locks on IO and mem
>>   	 * separately to userspace but it can cause deadlocks */
>> -- 
>> 2.25.1

-- 
Jingfeng