On Wed, Aug 30, 2023 at 07:15:28PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng <[email protected]>
>
> 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" <[email protected]>
> Signed-off-by: Sui Jingfeng <[email protected]>
> ---
> 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
>
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 <[email protected]>
>>
>> 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" <[email protected]>
>> Signed-off-by: Sui Jingfeng <[email protected]>
>> ---
>> 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
>>
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