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

[Sui Jingfeng]

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@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > Signed-off-by: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
> > ---
> >   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