diff mbox series

[-next,1/5] PCI: Add the pci_is_vga() helper

Message ID 20230830111532.444535-2-sui.jingfeng@linux.dev (mailing list archive)
State Accepted
Delegated to: Bjorn Helgaas
Headers show
Series Add the pci_is_vga() helper and use it | expand

Commit Message

Sui Jingfeng Aug. 30, 2023, 11:15 a.m. UTC 
From: Sui Jingfeng <suijingfeng@loongson.cn>

The PCI code and ID assignment specification defined four types of
display controllers for the display base class(03h), and the devices
with 0x00h sub-class code are VGA devices. VGA devices with programming
interface 0x00 is VGA-compatible, VGA devices with programming interface
0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
is defined to provide backward compatibility for devices that were built
before the class code field was defined. Hence, introduce the pci_is_vga()
helper, let it handle the details for us. It returns true if the PCI(e)
device being tested belongs to the VGA devices category.

Cc: "Maciej W. Rozycki" <macro@orcam.me.uk>
Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 include/linux/pci.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Bjorn Helgaas Oct. 5, 2023, 10:51 p.m. UTC | #1 
On Wed, Aug 30, 2023 at 07:15:28PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 
> The PCI code and ID assignment specification defined four types of
> display controllers for the display base class(03h), and the devices
> with 0x00h sub-class code are VGA devices. VGA devices with programming

I can update this with the spec details (PCI Code and Assignment spec
r1.15, secs 1.1 and 1.4).

> interface 0x00 is VGA-compatible, VGA devices with programming interface
> 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
> is defined to provide backward compatibility for devices that were built
> before the class code field was defined. Hence, introduce the pci_is_vga()
> helper, let it handle the details for us. It returns true if the PCI(e)
> device being tested belongs to the VGA devices category.
>
> Cc: "Maciej W. Rozycki" <macro@orcam.me.uk>
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  include/linux/pci.h | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index cf6e0b057752..ace727001911 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -713,6 +713,33 @@ static inline bool pci_is_bridge(struct pci_dev *dev)
>  		dev->hdr_type == PCI_HEADER_TYPE_CARDBUS;
>  }
>  
> +/**
> + * The PCI code and ID assignment specification defined four types of
> + * display controllers for the display base class(03h), and the devices
> + * with 0x00h sub-class code are VGA devices. VGA devices with programming
> + * interface 0x00 is VGA-compatible, VGA devices with programming interface
> + * 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
> + * is defined to provide backward compatibility for devices that were built
> + * before the class code field was defined. This means that it belong to the
> + * VGA devices category also.
> + *
> + * Returns:
> + * true if the PCI device is a VGA device, false otherwise.
> + */
> +static inline bool pci_is_vga(struct pci_dev *pdev)
> +{
> +	if (!pdev)
> +		return false;
> +
> +	if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
> +		return true;
> +
> +	if ((pdev->class >> 8) == PCI_CLASS_NOT_DEFINED_VGA)
> +		return true;

Are you seeing a problem that will be fixed by this series, i.e., a
PCI_CLASS_NOT_DEFINED_VGA device that we currently don't handle
correctly?

I think this makes sense per the spec, but there's always a risk of
breaking something, so it's nice if the change actually *fixes*
something to make that risk worthwhile.

> +	return false;
> +}
> +
>  #define for_each_pci_bridge(dev, bus)				\
>  	list_for_each_entry(dev, &bus->devices, bus_list)	\
>  		if (!pci_is_bridge(dev)) {} else
> -- 
> 2.34.1
>
Sui Jingfeng Oct. 6, 2023, 11:40 a.m. UTC | #2 
Hi,


On 2023/10/6 06:51, Bjorn Helgaas wrote:
> On Wed, Aug 30, 2023 at 07:15:28PM +0800, Sui Jingfeng wrote:
>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>
>> The PCI code and ID assignment specification defined four types of
>> display controllers for the display base class(03h), and the devices
>> with 0x00h sub-class code are VGA devices. VGA devices with programming
> I can update this with the spec details (PCI Code and Assignment spec
> r1.15, secs 1.1 and 1.4).
>
>> interface 0x00 is VGA-compatible, VGA devices with programming interface
>> 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
>> is defined to provide backward compatibility for devices that were built
>> before the class code field was defined. Hence, introduce the pci_is_vga()
>> helper, let it handle the details for us. It returns true if the PCI(e)
>> device being tested belongs to the VGA devices category.
>>
>> Cc: "Maciej W. Rozycki" <macro@orcam.me.uk>
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   include/linux/pci.h | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index cf6e0b057752..ace727001911 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -713,6 +713,33 @@ static inline bool pci_is_bridge(struct pci_dev *dev)
>>   		dev->hdr_type == PCI_HEADER_TYPE_CARDBUS;
>>   }
>>   
>> +/**
>> + * The PCI code and ID assignment specification defined four types of
>> + * display controllers for the display base class(03h), and the devices
>> + * with 0x00h sub-class code are VGA devices. VGA devices with programming
>> + * interface 0x00 is VGA-compatible, VGA devices with programming interface
>> + * 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
>> + * is defined to provide backward compatibility for devices that were built
>> + * before the class code field was defined. This means that it belong to the
>> + * VGA devices category also.
>> + *
>> + * Returns:
>> + * true if the PCI device is a VGA device, false otherwise.
>> + */
>> +static inline bool pci_is_vga(struct pci_dev *pdev)
>> +{
>> +	if (!pdev)
>> +		return false;
>> +
>> +	if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
>> +		return true;
>> +
>> +	if ((pdev->class >> 8) == PCI_CLASS_NOT_DEFINED_VGA)
>> +		return true;
> Are you seeing a problem that will be fixed by this series, i.e., a
> PCI_CLASS_NOT_DEFINED_VGA device that we currently don't handle
> correctly?

No, I write it according to the spec.
But I'm sure that the boot_vga will not be shown at sysfs for a PCI_CLASS_NOT_DEFINED_VGA device.


> I think this makes sense per the spec, but there's always a risk of
> breaking something, so it's nice if the change actually *fixes*
> something to make that risk worthwhile.


Maciej mentioned that PCI_CLASS_NOT_DEFINED_VGA device should also be handled in the past.
see [1]. But if no one interested in PCI_CLASS_NOT_DEFINED_VGA nowaday, then I guess
the gains of this patch may not deserve the time and risk. But I don't mind if someone
would like pick it up for other purpose.

Thanks for the reviewing. :-)

[1] https://lkml.org/lkml/2023/6/18/315


>> +	return false;
>> +}
>> +
>>   #define for_each_pci_bridge(dev, bus)				\
>>   	list_for_each_entry(dev, &bus->devices, bus_list)	\
>>   		if (!pci_is_bridge(dev)) {} else
>> -- 
>> 2.34.1
>>
Maciej W. Rozycki Oct. 6, 2023, 12:10 p.m. UTC | #3 
On Fri, 6 Oct 2023, Sui Jingfeng wrote:

> > I think this makes sense per the spec, but there's always a risk of
> > breaking something, so it's nice if the change actually *fixes*
> > something to make that risk worthwhile.
> 
> 
> Maciej mentioned that PCI_CLASS_NOT_DEFINED_VGA device should also be handled
> in the past.
> see [1]. But if no one interested in PCI_CLASS_NOT_DEFINED_VGA nowaday, then I
> guess
> the gains of this patch may not deserve the time and risk. But I don't mind if
> someone
> would like pick it up for other purpose.

 Well, if we need to determine for whatever purpose whether a PCI/e device 
presents a VGA programming interface, then I think we ought to do this in 
a complete manner.  I'm not sure offhand what could possibly break if we 
write our code according to specs and include PCI_CLASS_NOT_DEFINED_VGA 
devices in the class.

 Yes, I'm aware they won't be the latest and greatest, but they may still 
be there out there in service.  For one I continue using my 30 years old 
Trident 8900C ISA VGA adapter with the most recent Linux kernel.  The card 
serves its purpose, mostly as a glass TTY, so why should I replace it?

 Of course there are broken devices out there regardless, which won't work 
as we expect without special handling or sometimes at all even.  It does 
not mean we should refrain from making the best effort for good compliant 
devices and assume in advance that something will break even if we write 
our code according to the relevant specs.  I'd say do write according to 
specs and only try to sort out the situation somehow if something actually 
does break.

 In any case if we actually do choose to ignore PCI_CLASS_NOT_DEFINED_VGA 
devices, then I wanted to make sure we do it deliberately rather than from 
the lack of awareness of the existence of such devices.

  Maciej
diff mbox series

Patch

diff --git a/include/linux/pci.h b/include/linux/pci.h
index cf6e0b057752..ace727001911 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -713,6 +713,33 @@  static inline bool pci_is_bridge(struct pci_dev *dev)
 		dev->hdr_type == PCI_HEADER_TYPE_CARDBUS;
 }
 
+/**
+ * The PCI code and ID assignment specification defined four types of
+ * display controllers for the display base class(03h), and the devices
+ * with 0x00h sub-class code are VGA devices. VGA devices with programming
+ * interface 0x00 is VGA-compatible, VGA devices with programming interface
+ * 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
+ * is defined to provide backward compatibility for devices that were built
+ * before the class code field was defined. This means that it belong to the
+ * VGA devices category also.
+ *
+ * Returns:
+ * true if the PCI device is a VGA device, false otherwise.
+ */
+static inline bool pci_is_vga(struct pci_dev *pdev)
+{
+	if (!pdev)
+		return false;
+
+	if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
+		return true;
+
+	if ((pdev->class >> 8) == PCI_CLASS_NOT_DEFINED_VGA)
+		return true;
+
+	return false;
+}
+
 #define for_each_pci_bridge(dev, bus)				\
 	list_for_each_entry(dev, &bus->devices, bus_list)	\
 		if (!pci_is_bridge(dev)) {} else