[PATCH v2 00/11] Fix typos, comments and copyright

[PATCH v2 00/11] Fix typos, comments and copyright

[Sui Jingfeng]

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
-- 
2.34.1

[PATCH v2 01/11] PCI/VGA: Use unsigned type for the io_state variable

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

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, this patch replaceis the third argument of
vga_str_to_iostate() function with 'unsigned int *' type.

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
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.34.1

Re: [PATCH v2 01/11] PCI/VGA: Use unsigned type for the io_state variable

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

On Wed, 9 Aug 2023, Sui Jingfeng wrote:

> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 
> 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, this patch replaceis the third argument of
> vga_str_to_iostate() function with 'unsigned int *' type.
> 
> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
> 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 */

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[PATCH v2 02/11] PCI: Add the pci_get_class_masked() helper

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

Because there is no good way to get the mask member used to searching for
devices that conform to a specific PCI class code, an application needs to
process all PCI display devices can achieve its goal as follows:

pdev = NULL;
do {
	pdev = pci_get_class_masked(PCI_BASE_CLASS_DISPLAY << 16, 0xFF0000, pdev);
	if (pdev)
		do_something_for_pci_display_device(pdev);
} while (pdev);

While previously, we just can not ignore Sub-Class code and the Programming
Interface byte when do the searching.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/pci/search.c | 30 ++++++++++++++++++++++++++++++
 include/linux/pci.h  |  7 +++++++
 2 files changed, 37 insertions(+)

diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index b4c138a6ec02..f1c15aea868b 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -334,6 +334,36 @@ 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/include/linux/pci.h b/include/linux/pci.h
index 0ff7500772e6..b20e7ba844bf 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1180,6 +1180,9 @@ 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,
@@ -1895,6 +1898,10 @@ 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; }
-- 
2.34.1

Re: [PATCH v2 02/11] PCI: Add the pci_get_class_masked() helper

[Ilpo Järvinen]

On Wed, 9 Aug 2023, Sui Jingfeng wrote:

> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 
> Because there is no good way to get the mask member used to searching for
> devices that conform to a specific PCI class code, an application needs to
> process all PCI display devices can achieve its goal as follows:

This is mixing old and new way in a single sentence (which is confusing)?
 
> pdev = NULL;
> do {
> 	pdev = pci_get_class_masked(PCI_BASE_CLASS_DISPLAY << 16, 0xFF0000, pdev);
> 	if (pdev)
> 		do_something_for_pci_display_device(pdev);
> } while (pdev);
> 
> While previously, we just can not ignore Sub-Class code and the Programming

cannot

> Interface byte when do the searching.

doing the search.

> 
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/pci/search.c | 30 ++++++++++++++++++++++++++++++
>  include/linux/pci.h  |  7 +++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index b4c138a6ec02..f1c15aea868b 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -334,6 +334,36 @@ 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

No double spaces in kernel comments. Perhaps your editor might be adding 
them on reflow (might be configurable to not do that).

> + * 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.

Use kerneldoc's Return: section for describing return value.

> + */
> +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/include/linux/pci.h b/include/linux/pci.h
> index 0ff7500772e6..b20e7ba844bf 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1180,6 +1180,9 @@ 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,
> @@ -1895,6 +1898,10 @@ 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; }
> 

-- 
 i.

Re: [PATCH v2 02/11] PCI: Add the pci_get_class_masked() helper

[suijingfeng]

Hi,


On 2023/8/9 22:01, Ilpo Järvinen wrote:
> On Wed, 9 Aug 2023, Sui Jingfeng wrote:
>
>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>
>> Because there is no good way to get the mask member used to searching for
>> devices that conform to a specific PCI class code, an application needs to
>> process all PCI display devices can achieve its goal as follows:
> This is mixing old and new way in a single sentence (which is confusing)?


Thanks for reviewing, but I can't understand this sentence.
Are you telling me that my description have grammar problem or something else?


I means that before apply this patch, we don't have a function can be used
to get all PCI(e) devices in a system by matching against its the PCI base class code only,
while keep the Sub-Class code and the Programming Interface ignored.
By supply a mask as argument, such thing become possible.

If an application want to process all PCI display devices in the system,
then it can achieve its goal by calling pci_get_class_masked() function.


>> pdev = NULL;
>> do {
>> 	pdev = pci_get_class_masked(PCI_BASE_CLASS_DISPLAY << 16, 0xFF0000, pdev);
>> 	if (pdev)
>> 		do_something_for_pci_display_device(pdev);
>> } while (pdev);
>>
>> While previously, we just can not ignore Sub-Class code and the Programming
> cannot
>
>> Interface byte when do the searching.
> doing the search.


OK, will be fixed at the next version.


>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   drivers/pci/search.c | 30 ++++++++++++++++++++++++++++++
>>   include/linux/pci.h  |  7 +++++++
>>   2 files changed, 37 insertions(+)
>>
>> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
>> index b4c138a6ec02..f1c15aea868b 100644
>> --- a/drivers/pci/search.c
>> +++ b/drivers/pci/search.c
>> @@ -334,6 +334,36 @@ 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
> No double spaces in kernel comments. Perhaps your editor might be adding
> them on reflow (might be configurable to not do that).
>
>> + * 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.
> Use kerneldoc's Return: section for describing return value.
>
>> + */
>> +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/include/linux/pci.h b/include/linux/pci.h
>> index 0ff7500772e6..b20e7ba844bf 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1180,6 +1180,9 @@ 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,
>> @@ -1895,6 +1898,10 @@ 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; }
>>

Re: [PATCH v2 02/11] PCI: Add the pci_get_class_masked() helper

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5586 bytes --]

On Thu, 10 Aug 2023, suijingfeng wrote:

> Hi,
> 
> 
> On 2023/8/9 22:01, Ilpo Järvinen wrote:
> > On Wed, 9 Aug 2023, Sui Jingfeng wrote:
> > 
> > > From: Sui Jingfeng <suijingfeng@loongson.cn>
> > > 
> > > Because there is no good way to get the mask member used to searching for
> > > devices that conform to a specific PCI class code, an application needs to
> > > process all PCI display devices can achieve its goal as follows:
> > This is mixing old and new way in a single sentence (which is confusing)?
> 
> 
> Thanks for reviewing, but I can't understand this sentence.
> Are you telling me that my description have grammar problem or something else?

I think it's a bit of both.

> I means that before apply this patch, we don't have a function can be used
> to get all PCI(e) devices in a system by matching against its the PCI base
> class code only,
> while keep the Sub-Class code and the Programming Interface ignored.
> By supply a mask as argument, such thing become possible.

This explanation you put into this reply is much easier to follow and 
understand. I recommend you'd use it to replace the unclear fragment 
above. So something along the lines of:

  There is no function that can be used to get all PCI(e) devices in a 
  system by matching against its the PCI base class code only, while keep 
  the Sub-Class code and the Programming Interface ignored.

  Add pci_get_class_masked() to allow supplying a mask for the get.

  [After this you can put the explanining code block+its intro if you 
   want]

-- 
 i.

> If an application want to process all PCI display devices in the system,
> then it can achieve its goal by calling pci_get_class_masked() function.
> 
> 
> > > pdev = NULL;
> > > do {
> > > 	pdev = pci_get_class_masked(PCI_BASE_CLASS_DISPLAY << 16, 0xFF0000,
> > > pdev);
> > > 	if (pdev)
> > > 		do_something_for_pci_display_device(pdev);
> > > } while (pdev);
> > > 
> > > While previously, we just can not ignore Sub-Class code and the
> > > Programming
> > cannot
> > 
> > > Interface byte when do the searching.
> > doing the search.
> 
> 
> OK, will be fixed at the next version.
> 
> 
> > > Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> > > ---
> > >   drivers/pci/search.c | 30 ++++++++++++++++++++++++++++++
> > >   include/linux/pci.h  |  7 +++++++
> > >   2 files changed, 37 insertions(+)
> > > 
> > > diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> > > index b4c138a6ec02..f1c15aea868b 100644
> > > --- a/drivers/pci/search.c
> > > +++ b/drivers/pci/search.c
> > > @@ -334,6 +334,36 @@ 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
> > No double spaces in kernel comments. Perhaps your editor might be adding
> > them on reflow (might be configurable to not do that).
> > 
> > > + * 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.
> > Use kerneldoc's Return: section for describing return value.
> > 
> > > + */
> > > +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/include/linux/pci.h b/include/linux/pci.h
> > > index 0ff7500772e6..b20e7ba844bf 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -1180,6 +1180,9 @@ 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,
> > > @@ -1895,6 +1898,10 @@ 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; }
> > > 
> 

[PATCH v2 03/11] PCI/VGA: Deal with VGA class devices

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

vgaarb only cares about PCI(e) VGA devices (pdev->class == 0x0300XX)
Currently, hence we only need to add VGA devices has its class code equals
to 0x0300 to the arbiter. To keep align with the previous behavior. we
ignore the programming interface byte (the least significant 8 bits)
intentionally.

After apply this patch, We will filter the unqualified devices out in the
vga_arb_device_init() function. While the current implementation is to
search all PCI devices in a system, this is not efficient. This also means
that deleting a PCI device no longer needs to walk the list.

Note that the major contribution of this patch is optimization.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/pci/vgaarb.c | 68 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 56 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index c1bc6c983932..8742a51d450f 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) {
@@ -1493,6 +1489,42 @@ static void vga_arbiter_notify_clients(void)
 	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)
 {
@@ -1502,6 +1534,9 @@ 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 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. */
@@ -1534,8 +1569,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)
@@ -1543,13 +1578,22 @@ 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 */
-	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);
+	/*
+	 * 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);
 
 	pr_info("loaded\n");
 	return rc;
-- 
2.34.1

[PATCH v2 04/11] PCI/VGA: Drop the inline in the vga_update_device_decodes() function.

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

The vga_update_device_decodes() function is not performance-critical.
So drop the inline. This patch also makes the parameter consistent with
the argument, using the 'unsigned int' type instead of the 'signed' type
to store the decode.

Change the second argument of the vga_update_device_decodes() function
to 'unsigned int' type.

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

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 8742a51d450f..dc10a262fb5e 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -860,24 +860,24 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
 	return ret;
 }
 
-/* this is called with the lock */
-static inline void vga_update_device_decodes(struct vga_device *vgadev,
-					     int new_decodes)
+/* This is called with the lock */
+static void vga_update_device_decodes(struct vga_device *vgadev,
+				      unsigned int new_decodes)
 {
 	struct device *dev = &vgadev->pdev->dev;
-	int old_decodes, decodes_removed, decodes_unlocked;
+	unsigned int old_decodes = vgadev->decodes;
+	unsigned int decodes_removed = ~new_decodes & old_decodes;
+	unsigned int decodes_unlocked = vgadev->locks & decodes_removed;
 
-	old_decodes = vgadev->decodes;
-	decodes_removed = ~new_decodes & old_decodes;
-	decodes_unlocked = vgadev->locks & decodes_removed;
 	vgadev->decodes = new_decodes;
 
-	vgaarb_info(dev, "changed VGA decodes: olddecodes=%s,decodes=%s:owns=%s\n",
-		vga_iostate_to_str(old_decodes),
-		vga_iostate_to_str(vgadev->decodes),
-		vga_iostate_to_str(vgadev->owns));
+	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));
 
-	/* 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;
-- 
2.34.1

Re: [PATCH v2 04/11] PCI/VGA: Drop the inline in the vga_update_device_decodes() function.

[Ilpo Järvinen]

On Wed, 9 Aug 2023, Sui Jingfeng wrote:

> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 
> The vga_update_device_decodes() function is not performance-critical.
> So drop the inline. This patch also makes the parameter consistent with
> the argument, using the 'unsigned int' type instead of the 'signed' type
> to store the decode.

Use imperative form, Don't start with "This patch" but directly what 
follows after those words.
 
> Change the second argument of the vga_update_device_decodes() function
> to 'unsigned int' type.

Somehow it feels all these 3 changes should be in separate patches, I 
don't see how they're related at all with each other other than they touch 
roughly the same area.

-- 
 i.

> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/pci/vgaarb.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 8742a51d450f..dc10a262fb5e 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -860,24 +860,24 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
>  	return ret;
>  }
>  
> -/* this is called with the lock */
> -static inline void vga_update_device_decodes(struct vga_device *vgadev,
> -					     int new_decodes)
> +/* This is called with the lock */
> +static void vga_update_device_decodes(struct vga_device *vgadev,
> +				      unsigned int new_decodes)
>  {
>  	struct device *dev = &vgadev->pdev->dev;
> -	int old_decodes, decodes_removed, decodes_unlocked;
> +	unsigned int old_decodes = vgadev->decodes;
> +	unsigned int decodes_removed = ~new_decodes & old_decodes;
> +	unsigned int decodes_unlocked = vgadev->locks & decodes_removed;
>  
> -	old_decodes = vgadev->decodes;
> -	decodes_removed = ~new_decodes & old_decodes;
> -	decodes_unlocked = vgadev->locks & decodes_removed;
>  	vgadev->decodes = new_decodes;
>  
> -	vgaarb_info(dev, "changed VGA decodes: olddecodes=%s,decodes=%s:owns=%s\n",
> -		vga_iostate_to_str(old_decodes),
> -		vga_iostate_to_str(vgadev->decodes),
> -		vga_iostate_to_str(vgadev->owns));
> +	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));
>  
> -	/* 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;
> 

[PATCH v2 05/11] PCI/VGA: Move the new_state assignment out of the loop

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

In the vga_arbiter_notify_clients() function, the value of the 'new_state'
variable will be 'false' on systems that have more than one VGA device.
The value will be 'true' if there is only one VGA device or no VGA device
at all. Hence, its value is not relevant to the iteration of the loop.

So move the assignment clause out of the loop. For a system with multiple
video cards, this patch saves unnecessary assignment.

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

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index dc10a262fb5e..6883067a802a 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -1468,22 +1468,20 @@ static void vga_arbiter_notify_clients(void)
 {
 	struct vga_device *vgadev;
 	unsigned long flags;
-	uint32_t new_decodes;
-	bool new_state;
+	bool state;
 
 	if (!vga_arbiter_used)
 		return;
 
+	state = (vga_count > 1) ? false : true;
+
 	spin_lock_irqsave(&vga_lock, flags);
 	list_for_each_entry(vgadev, &vga_list, list) {
-		if (vga_count > 1)
-			new_state = false;
-		else
-			new_state = true;
 		if (vgadev->set_decode) {
-			new_decodes = vgadev->set_decode(vgadev->pdev,
-							 new_state);
-			vga_update_device_decodes(vgadev, new_decodes);
+			unsigned int decodes;
+
+			decodes = vgadev->set_decode(vgadev->pdev, state);
+			vga_update_device_decodes(vgadev, decodes);
 		}
 	}
 	spin_unlock_irqrestore(&vga_lock, flags);
-- 
2.34.1

Re: [PATCH v2 05/11] PCI/VGA: Move the new_state assignment out of the loop

[Ilpo Järvinen]

On Wed, 9 Aug 2023, Sui Jingfeng wrote:

> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 
> In the vga_arbiter_notify_clients() function, the value of the 'new_state'
> variable will be 'false' on systems that have more than one VGA device.
> The value will be 'true' if there is only one VGA device or no VGA device
> at all. Hence, its value is not relevant to the iteration of the loop.
> 
> So move the assignment clause out of the loop. For a system with multiple
> video cards, this patch saves unnecessary assignment.
> 
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/pci/vgaarb.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index dc10a262fb5e..6883067a802a 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -1468,22 +1468,20 @@ static void vga_arbiter_notify_clients(void)
>  {
>  	struct vga_device *vgadev;
>  	unsigned long flags;
> -	uint32_t new_decodes;
> -	bool new_state;
> +	bool state;
>  
>  	if (!vga_arbiter_used)
>  		return;
>  
> +	state = (vga_count > 1) ? false : true;
> +

Is it safe to move this outside of the lock?

This would be enough (no need for ?: construct):

state = vga_count <= 1;

Or if you want to keep it as > 1:

state = !(vga_count > 1);

>  	spin_lock_irqsave(&vga_lock, flags);
>  	list_for_each_entry(vgadev, &vga_list, list) {
> -		if (vga_count > 1)
> -			new_state = false;
> -		else
> -			new_state = true;
>  		if (vgadev->set_decode) {
> -			new_decodes = vgadev->set_decode(vgadev->pdev,
> -							 new_state);
> -			vga_update_device_decodes(vgadev, new_decodes);
> +			unsigned int decodes;
> +
> +			decodes = vgadev->set_decode(vgadev->pdev, state);
> +			vga_update_device_decodes(vgadev, decodes);
>  		}
>  	}
>  	spin_unlock_irqrestore(&vga_lock, flags);
> 


-- 
 i.

[PATCH v2 06/11] PCI/VGA: Fix two typos in the comments of pci_notify()

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

1) s/intereted/interested
2) s/hotplugable/hot-pluggable

While at it, convert the comments to the conventional multi-line style,
and rewrap to fill 78 columns.

Fixes: deb2d2ecd43d ("PCI/GPU: implement VGA arbitration on Linux")
Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/pci/vgaarb.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 6883067a802a..811510253553 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -1535,9 +1535,11 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
 	if (!pci_dev_is_vga(pdev))
 		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
-	 * cases of hotplugable vga cards. */
+	/*
+	 * For now, we're only interested in devices added and removed.
+	 * I didn't test this thing here, so someone needs to double check
+	 * for the cases of hot-pluggable VGA cards.
+	 */
 	if (action == BUS_NOTIFY_ADD_DEVICE)
 		notify = vga_arbiter_add_pci_device(pdev);
 	else if (action == BUS_NOTIFY_DEL_DEVICE)
-- 
2.34.1

Re: [PATCH v2 06/11] PCI/VGA: Fix two typos in the comments of pci_notify()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1468 bytes --]

On Wed, 9 Aug 2023, Sui Jingfeng wrote:

> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 
> 1) s/intereted/interested
> 2) s/hotplugable/hot-pluggable
> 
> While at it, convert the comments to the conventional multi-line style,
> and rewrap to fill 78 columns.
> 
> Fixes: deb2d2ecd43d ("PCI/GPU: implement VGA arbitration on Linux")
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/pci/vgaarb.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 6883067a802a..811510253553 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -1535,9 +1535,11 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
>  	if (!pci_dev_is_vga(pdev))
>  		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
> -	 * cases of hotplugable vga cards. */
> +	/*
> +	 * For now, we're only interested in devices added and removed.
> +	 * I didn't test this thing here, so someone needs to double check
> +	 * for the cases of hot-pluggable VGA cards.
> +	 */
>  	if (action == BUS_NOTIFY_ADD_DEVICE)
>  		notify = vga_arbiter_add_pci_device(pdev);
>  	else if (action == BUS_NOTIFY_DEL_DEVICE)

Don't use Fixes tag for comment changes. After removing it, feel free to 
add:

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.

Re: [PATCH v2 06/11] PCI/VGA: Fix two typos in the comments of pci_notify()

[suijingfeng]

Hi,


On 2023/8/9 22:12, Ilpo Järvinen wrote:
> On Wed, 9 Aug 2023, Sui Jingfeng wrote:
>
>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>
>> 1) s/intereted/interested
>> 2) s/hotplugable/hot-pluggable
>>
>> While at it, convert the comments to the conventional multi-line style,
>> and rewrap to fill 78 columns.
>>
>> Fixes: deb2d2ecd43d ("PCI/GPU: implement VGA arbitration on Linux")
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   drivers/pci/vgaarb.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
>> index 6883067a802a..811510253553 100644
>> --- a/drivers/pci/vgaarb.c
>> +++ b/drivers/pci/vgaarb.c
>> @@ -1535,9 +1535,11 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
>>   	if (!pci_dev_is_vga(pdev))
>>   		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
>> -	 * cases of hotplugable vga cards. */
>> +	/*
>> +	 * For now, we're only interested in devices added and removed.
>> +	 * I didn't test this thing here, so someone needs to double check
>> +	 * for the cases of hot-pluggable VGA cards.
>> +	 */
>>   	if (action == BUS_NOTIFY_ADD_DEVICE)
>>   		notify = vga_arbiter_add_pci_device(pdev);
>>   	else if (action == BUS_NOTIFY_DEL_DEVICE)
> Don't use Fixes tag for comment changes. After removing it, feel free to
> add:


OK, I will remove the Fixes tag for comment changes at next version.
Thanks a lot. Also for other patches in this series.


> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>
>

[PATCH v2 07/11] PCI/VGA: vga_client_register() return -ENODEV on failure, not -1

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

Fixes: 934f992c763a ("drm/i915: Recognise non-VGA display devices")
Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/pci/vgaarb.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 811510253553..a6b8c0def35d 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -964,7 +964,7 @@ EXPORT_SYMBOL(vga_set_legacy_decoding);
  *
  * To unregister just call vga_client_unregister().
  *
- * Returns: 0 on success, -1 on failure
+ * 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))
@@ -975,16 +975,13 @@ int vga_client_register(struct pci_dev *pdev,
 
 	spin_lock_irqsave(&vga_lock, flags);
 	vgadev = vgadev_find(pdev);
-	if (!vgadev)
-		goto bail;
-
-	vgadev->set_decode = set_decode;
-	ret = 0;
-
-bail:
+	if (vgadev) {
+		vgadev->set_decode = set_decode;
+		ret = 0;
+	}
 	spin_unlock_irqrestore(&vga_lock, flags);
-	return ret;
 
+	return ret;
 }
 EXPORT_SYMBOL(vga_client_register);
 
-- 
2.34.1

Re: [PATCH v2 07/11] PCI/VGA: vga_client_register() return -ENODEV on failure, not -1

[Ilpo Järvinen]

On Wed, 9 Aug 2023, Sui Jingfeng wrote:

> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 

Changelog body is missing.

> Fixes: 934f992c763a ("drm/i915: Recognise non-VGA display devices")
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/pci/vgaarb.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 811510253553..a6b8c0def35d 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -964,7 +964,7 @@ EXPORT_SYMBOL(vga_set_legacy_decoding);
>   *
>   * To unregister just call vga_client_unregister().
>   *
> - * Returns: 0 on success, -1 on failure
> + * Returns: 0 on success, -ENODEV on failure

So this is the true substance of this change??

It doesn't warrant Fixes tag which requires a real problem to fix. An 
incorrect comment is not enough.

I think the shortlog is a bit misleading as is because it doesn't in any 
way indicate the problem is only in a comment.

>   */
>  int vga_client_register(struct pci_dev *pdev,
>  		unsigned int (*set_decode)(struct pci_dev *pdev, bool decode))
> @@ -975,16 +975,13 @@ int vga_client_register(struct pci_dev *pdev,
>  
>  	spin_lock_irqsave(&vga_lock, flags);
>  	vgadev = vgadev_find(pdev);
> -	if (!vgadev)
> -		goto bail;
> -
> -	vgadev->set_decode = set_decode;
> -	ret = 0;
> -
> -bail:
> +	if (vgadev) {
> +		vgadev->set_decode = set_decode;
> +		ret = 0;
> +	}
>  	spin_unlock_irqrestore(&vga_lock, flags);
> -	return ret;
>  
> +	return ret;

No logic changes in this at all? I don't think it belongs to the same 
patch. I'm not sure if the new logic is improvement anyway. I'd prefer to 
initialize ret = 0 instead:

	int ret = 0;
	...
	if (!vgadev) {
		err = -ENODEV;
		goto unlock;
	}
	...
unlock:
	...

-- 
 i.

Re: [PATCH v2 07/11] PCI/VGA: vga_client_register() return -ENODEV on failure, not -1

[suijingfeng]

Hi,


On 2023/8/9 21:52, Ilpo Järvinen wrote:
> On Wed, 9 Aug 2023, Sui Jingfeng wrote:
>
>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>
> Changelog body is missing.


I thought that probably the Fixes tag could be taken as the body of this commit,
since there are no warnings when I check the whole series with checkpatch.pl.


>> Fixes: 934f992c763a ("drm/i915: Recognise non-VGA display devices")
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   drivers/pci/vgaarb.c | 15 ++++++---------
>>   1 file changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
>> index 811510253553..a6b8c0def35d 100644
>> --- a/drivers/pci/vgaarb.c
>> +++ b/drivers/pci/vgaarb.c
>> @@ -964,7 +964,7 @@ EXPORT_SYMBOL(vga_set_legacy_decoding);
>>    *
>>    * To unregister just call vga_client_unregister().
>>    *
>> - * Returns: 0 on success, -1 on failure
>> + * Returns: 0 on success, -ENODEV on failure
> So this is the true substance of this change??
>
Yes.


> It doesn't warrant Fixes tag which requires a real problem to fix. An
> incorrect comment is not enough.
>
> I think the shortlog is a bit misleading as is because it doesn't in any
> way indicate the problem is only in a comment.

But it's that commit(934f992c763a) alter the return value of vga_client_register(),
which make the commit and code don't match anymore.


>>    */
>>   int vga_client_register(struct pci_dev *pdev,
>>   		unsigned int (*set_decode)(struct pci_dev *pdev, bool decode))
>> @@ -975,16 +975,13 @@ int vga_client_register(struct pci_dev *pdev,
>>   
>>   	spin_lock_irqsave(&vga_lock, flags);
>>   	vgadev = vgadev_find(pdev);
>> -	if (!vgadev)
>> -		goto bail;
>> -
>> -	vgadev->set_decode = set_decode;
>> -	ret = 0;
>> -
>> -bail:
>> +	if (vgadev) {
>> +		vgadev->set_decode = set_decode;
>> +		ret = 0;
>> +	}
>>   	spin_unlock_irqrestore(&vga_lock, flags);
>> -	return ret;
>>   
>> +	return ret;
> No logic changes in this at all? I don't think it belongs to the same
> patch. I'm not sure if the new logic is improvement anyway.


Yes, the new logic is just improvement, no function change.
Strict speaking, you are right. One patch do one thing.


>   I'd prefer to
> initialize ret = 0 instead:
>
> 	int ret = 0;
> 	...
> 	if (!vgadev) {
> 		err = -ENODEV;
> 		goto unlock;
> 	}
> 	...
> unlock:
> 	...
>

But this is same as the original coding style, no fundamental improve.
The key point is to make the wrapped code between the spin_lock_irqsave() and spin_unlock_irqrestore() compact.
my patch remove the necessary 'goto' statement and the 'bail' label.
After apply my patch, the vga_client_register() function became as this:

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;

     spin_lock_irqsave(&vga_lock, flags);
     vgadev = vgadev_find(pdev);
     if (vgadev) {
         vgadev->set_decode = set_decode;
         ret = 0;
     }
     spin_unlock_irqrestore(&vga_lock, flags);

     return ret;
}

Re: [PATCH v2 07/11] PCI/VGA: vga_client_register() return -ENODEV on failure, not -1

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2923 bytes --]

On Thu, 10 Aug 2023, suijingfeng wrote:
> On 2023/8/9 21:52, Ilpo Järvinen wrote:
> > On Wed, 9 Aug 2023, Sui Jingfeng wrote:
> > 
> > > From: Sui Jingfeng <suijingfeng@loongson.cn>
> > > 
> > Changelog body is missing.
> 
> 
> I thought that probably the Fixes tag could be taken as the body of this
> commit,
> since there are no warnings when I check the whole series with checkpatch.pl.
> 
> 
> > > Fixes: 934f992c763a ("drm/i915: Recognise non-VGA display devices")
> > > Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> > > ---
> > >   drivers/pci/vgaarb.c | 15 ++++++---------
> > >   1 file changed, 6 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> > > index 811510253553..a6b8c0def35d 100644
> > > --- a/drivers/pci/vgaarb.c
> > > +++ b/drivers/pci/vgaarb.c
> > > @@ -964,7 +964,7 @@ EXPORT_SYMBOL(vga_set_legacy_decoding);
> > >    *
> > >    * To unregister just call vga_client_unregister().
> > >    *
> > > - * Returns: 0 on success, -1 on failure
> > > + * Returns: 0 on success, -ENODEV on failure
> > So this is the true substance of this change??
> > 
> Yes.
> 
> 
> > It doesn't warrant Fixes tag which requires a real problem to fix. An
> > incorrect comment is not enough.
> > 
> > I think the shortlog is a bit misleading as is because it doesn't in any
> > way indicate the problem is only in a comment.
> 
> But it's that commit(934f992c763a) alter the return value of
> vga_client_register(),
> which make the commit and code don't match anymore.

This is useful information, no point in withholding it which forces 
others to figure it out by looking that commit up so put that detail into 
the changelog body.

> >   I'd prefer to
> > initialize ret = 0 instead:
> > 
> > 	int ret = 0;
> > 	...
> > 	if (!vgadev) {
> > 		err = -ENODEV;
> > 		goto unlock;
> > 	}
> > 	...
> > unlock:
> > 	...
> > 
> 
> But this is same as the original coding style, no fundamental improve.
> The key point is to make the wrapped code between the spin_lock_irqsave() and
> spin_unlock_irqrestore() compact.
> my patch remove the necessary 'goto' statement and the 'bail' label.
> After apply my patch, the vga_client_register() function became as this:
> 
> 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;
> 
>     spin_lock_irqsave(&vga_lock, flags);
>     vgadev = vgadev_find(pdev);
>     if (vgadev) {
>         vgadev->set_decode = set_decode;
>         ret = 0;
>     }
>     spin_unlock_irqrestore(&vga_lock, flags);
> 
>     return ret;
> }

I'm not too attached to either of the ways around since there's no 
correctness issues here. Feel free to ignore my alternative suggestion
(make the separate patch out of it in anycase).

-- 
 i.

Re: [PATCH v2 07/11] PCI/VGA: vga_client_register() return -ENODEV on failure, not -1

[Sui Jingfeng]

Hi,


On 2023/8/10 20:13, Ilpo Järvinen wrote:
> On Thu, 10 Aug 2023, suijingfeng wrote:
>> On 2023/8/9 21:52, Ilpo Järvinen wrote:
>>> On Wed, 9 Aug 2023, Sui Jingfeng wrote:
>>>
>>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>
>>> Changelog body is missing.
>>
>> I thought that probably the Fixes tag could be taken as the body of this
>> commit,
>> since there are no warnings when I check the whole series with checkpatch.pl.
>>
>>
>>>> Fixes: 934f992c763a ("drm/i915: Recognise non-VGA display devices")
>>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>> ---
>>>>    drivers/pci/vgaarb.c | 15 ++++++---------
>>>>    1 file changed, 6 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
>>>> index 811510253553..a6b8c0def35d 100644
>>>> --- a/drivers/pci/vgaarb.c
>>>> +++ b/drivers/pci/vgaarb.c
>>>> @@ -964,7 +964,7 @@ EXPORT_SYMBOL(vga_set_legacy_decoding);
>>>>     *
>>>>     * To unregister just call vga_client_unregister().
>>>>     *
>>>> - * Returns: 0 on success, -1 on failure
>>>> + * Returns: 0 on success, -ENODEV on failure
>>> So this is the true substance of this change??
>>>
>> Yes.
>>
>>
>>> It doesn't warrant Fixes tag which requires a real problem to fix. An
>>> incorrect comment is not enough.
>>>
>>> I think the shortlog is a bit misleading as is because it doesn't in any
>>> way indicate the problem is only in a comment.
>> But it's that commit(934f992c763a) alter the return value of
>> vga_client_register(),
>> which make the commit and code don't match anymore.
> This is useful information, no point in withholding it which forces
> others to figure it out by looking that commit up so put that detail into
> the changelog body.
>
>>>    I'd prefer to
>>> initialize ret = 0 instead:
>>>
>>> 	int ret = 0;
>>> 	...
>>> 	if (!vgadev) {
>>> 		err = -ENODEV;
>>> 		goto unlock;
>>> 	}
>>> 	...
>>> unlock:
>>> 	...
>>>
>> But this is same as the original coding style, no fundamental improve.
>> The key point is to make the wrapped code between the spin_lock_irqsave() and
>> spin_unlock_irqrestore() compact.
>> my patch remove the necessary 'goto' statement and the 'bail' label.
>> After apply my patch, the vga_client_register() function became as this:
>>
>> 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;
>>
>>      spin_lock_irqsave(&vga_lock, flags);
>>      vgadev = vgadev_find(pdev);
>>      if (vgadev) {
>>          vgadev->set_decode = set_decode;
>>          ret = 0;
>>      }
>>      spin_unlock_irqrestore(&vga_lock, flags);
>>
>>      return ret;
>> }
> I'm not too attached to either of the ways around since there's no
> correctness issues here. Feel free to ignore my alternative suggestion
> (make the separate patch out of it in anycase).


OK, will be done at the next version.

[PATCH v2 08/11] PCI/VGA: Fix a typo to the comment of vga_default

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

Fixes: deb2d2ecd43d ("PCI/GPU: implement VGA arbitration on Linux")
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 a6b8c0def35d..d80d92e8012b 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -99,7 +99,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 a cookie, it should not be dereferenced */
 static struct pci_dev *vga_default;
 
 /* Find somebody in our list */
-- 
2.34.1

Re: [PATCH v2 08/11] PCI/VGA: Fix a typo to the comment of vga_default

[Ilpo Järvinen]

On Wed, 9 Aug 2023, Sui Jingfeng wrote:

> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 

Please add changelog text.

> Fixes: deb2d2ecd43d ("PCI/GPU: implement VGA arbitration on Linux")
> 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 a6b8c0def35d..d80d92e8012b 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -99,7 +99,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 a cookie, it should not be dereferenced */
>  static struct pci_dev *vga_default;
>  
>  /* Find somebody in our list */

Again, no Fixes tag for comment corrections.

-- 
 i.

[PATCH v2 09/11] PCI/VGA: Fix a typo to the comments in vga_str_to_iostate() function

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

s/chekcing/checking

While at it, convert the comments to the conventional multi-line style,
and rewrap to fill 78 columns.

Fixes: deb2d2ecd43d ("PCI/GPU: implement VGA arbitration on Linux")
Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/pci/vgaarb.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index d80d92e8012b..9f5cf6a6e3a2 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -79,14 +79,16 @@ 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 */
+	/*
+	 * In theory, we could hand out locks on IO and MEM separately to
+	 * userspace, but this can cause deadlocks.
+	 */
 	if (strncmp(buf, "none", 4) == 0) {
 		*io_state = VGA_RSRC_NONE;
 		return 1;
 	}
 
-	/* XXX We're not chekcing the str_size! */
+	/* XXX We're not checking the str_size! */
 	if (strncmp(buf, "io+mem", 6) == 0)
 		goto both;
 	else if (strncmp(buf, "io", 2) == 0)
-- 
2.34.1

[PATCH v2 10/11] PCI/VGA: Tidy up the code and comment format

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

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

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/pci/vgaarb.c   | 90 ++++++++++++++++++++++++------------------
 include/linux/vgaarb.h |  4 +-
 2 files changed, 53 insertions(+), 41 deletions(-)

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 9f5cf6a6e3a2..a2f6e0e6b634 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -30,14 +30,13 @@
 #include <linux/vt.h>
 #include <linux/console.h>
 #include <linux/acpi.h>
-
 #include <linux/uaccess.h>
-
 #include <linux/vgaarb.h>
 
 static void vga_arbiter_notify_clients(void);
+
 /*
- * We keep a list of all vga devices in the system to speed
+ * We keep a list of all VGA devices in the system to speed
  * up the various operations of the arbiter
  */
 struct vga_device {
@@ -61,7 +60,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 */
@@ -195,14 +193,16 @@ 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 vgaarb, 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();
@@ -218,7 +218,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) &&
@@ -238,7 +239,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;
@@ -254,7 +256,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
@@ -265,13 +268,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.
 		 */
@@ -279,14 +284,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) {
@@ -313,7 +320,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".
@@ -355,7 +363,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) {
@@ -373,7 +382,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)
@@ -381,7 +391,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)
@@ -449,8 +460,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
@@ -667,7 +678,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)
@@ -708,7 +719,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
@@ -779,9 +790,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);
@@ -888,7 +900,7 @@ static 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--;
@@ -912,14 +924,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 ...
 	 */
@@ -954,9 +967,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.
@@ -1074,7 +1087,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;
@@ -1113,7 +1125,8 @@ static ssize_t vga_arb_read(struct file *file, char __user *buf,
 	/* Find card vgadev structure */
 	vgadev = vgadev_find(pdev);
 	if (vgadev == NULL) {
-		/* Wow, it's not in the list, that shouldn't happen,
+		/*
+		 * 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);
@@ -1309,7 +1322,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 {
@@ -1426,7 +1439,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;
 }
 
@@ -1460,7 +1472,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)
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.34.1

Re: [PATCH v2 10/11] PCI/VGA: Tidy up the code and comment format

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3349 bytes --]

On Wed, 9 Aug 2023, Sui Jingfeng wrote:

> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 
> This patch replaces the leading space with a tab and removes the double
> blank line and fix various typos, no functional change.
> 
> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/pci/vgaarb.c   | 90 ++++++++++++++++++++++++------------------
>  include/linux/vgaarb.h |  4 +-
>  2 files changed, 53 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 9f5cf6a6e3a2..a2f6e0e6b634 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -30,14 +30,13 @@
>  #include <linux/vt.h>
>  #include <linux/console.h>
>  #include <linux/acpi.h>
> -
>  #include <linux/uaccess.h>
> -
>  #include <linux/vgaarb.h>
>  
>  static void vga_arbiter_notify_clients(void);
> +
>  /*
> - * We keep a list of all vga devices in the system to speed
> + * We keep a list of all VGA devices in the system to speed
>   * up the various operations of the arbiter
>   */
>  struct vga_device {
> @@ -61,7 +60,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 */
> @@ -195,14 +193,16 @@ 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 vgaarb, 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();
> @@ -218,7 +218,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) &&
> @@ -238,7 +239,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;
> @@ -254,7 +256,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

Before

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[PATCH v2 11/11] PCI/VGA: Replace full MIT license text with SPDX identifier

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

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>
---
 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.34.1

Re: [PATCH v2 00/11] Fix typos, comments and copyright

[Bjorn Helgaas]

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; }

Re: [PATCH v2 00/11] Fix typos, comments and copyright

[suijingfeng]

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; }