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
