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

[suijingfeng]

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