Re: [PATCH 2/6] PCI/VGA: Deal with PCI VGA compatible devices only

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 2023/7/20 02:26, Bjorn Helgaas wrote:
Optimization is fine, but the most important thing here is to be clear
about what functional change this patch makes.  As I mentioned at [1],
if this patch affects the class codes accepted, please make that clear
here.

Reviewed-by: Mario Limonciello<mario.limonciello@xxxxxxx>
Signed-off-by: Sui Jingfeng<suijingfeng@xxxxxxxxxxx>
I do not see Mario's Reviewed-by on the list.  I do see Mario's
Reviewed-by [2] for a previous version, but that version added this in
pci_notify():

   + if (pdev->class != PCI_CLASS_DISPLAY_VGA << 8)
   +   return 0;

while this version adds:

   + if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
   +   return 0;

It's OK to carry a review to future versions if there are
insignificant changes, but this is a functional change that seems
significant to me.  The first matches only 0x030000, while the second
discards the low eight bits so it matches 0x0300XX.

[1]https://lore.kernel.org/r/20230718231400.GA496927@bhelgaas
[2]https://lore.kernel.org/all/5b6fdf65-b354-94a9-f883-be820157efad@xxxxxxx/

Yes,  you are right. As you already told me at [1]:


According to the "PCI Code and ID Assignment" spec, r1.15, sec 1.4,

only mentions 0x0300 programming interface 0x00 as decoding

the legacy VGA addresses.


If the programming interface is 0x01, then it is a 8514-compatible controller.

It is petty old card,  about 30 years old(I think, it is nearly obsolete for now).

I never have a chance to see such a card in real life.


Yes, we should adopt first matches method here. That is:

  + if (pdev->class != PCI_CLASS_DISPLAY_VGA << 8)
  +   return 0;

It seems that we are more rigorous to deal the VGA-compatible devices as illustrated by above code here.

But who the still have that card (8514-compatible) and the hardware to using such a card today ?


Please consider that the pci_dev_attrs_are_visible() function[3] also ignore the

programming interface (the least significant 8 bits).

Therefore, at this version of my vgaarb cleanup patch set.

I choose to keep the original filtering rule,

but do the necessary optimization only which I think is meaningful.


In the future, we may want to expand VGAARB to deal all PCI display class devices, with another patch.

if (pdev->class >> 16 == PCI_BASE_CLASS_DISPLAY)

         // accept

else

      // return immediately.


Then, we will have a more good chance to clarify the programmer interface.

Is this explanation feasible and reasonable, Bjorn and Mario ?


[3] https://elixir.bootlin.com/linux/latest/source/drivers/pci/pci-sysfs.c#L1551





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux