On Tue, Jul 11, 2023 at 09:43:50PM +0800, Sui Jingfeng wrote: > From: Sui Jingfeng <suijingf...@loongson.cn> > > Currently, vgaarb only cares about PCI VGA-compatible class devices. > > While vga_arbiter_del_pci_device() gets called unbalanced when some PCI > device is about to be removed. This happens even during the boot process.
The previous code calls vga_arbiter_add_pci_device() for every device (every device present at boot and also every hot-added device). It only allocates a vga_device if pdev->class is 0x0300XX. It calls vga_arbiter_del_pci_device() for every device removal. It does nothing unless it finds a vga_device. This seems symmetric and reasonable to me. Did you observe a problem with it? > Another reason is that the vga_arb_device_init() function is not efficient. > Since we only care about VGA-compatible devices (pdev->class == 0x030000), > We could filter the unqualified devices out in the vga_arb_device_init() > function. While the current implementation is to search all PCI devices > in a system, this is not necessary. 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.limoncie...@amd.com> > Signed-off-by: Sui Jingfeng <suijingf...@loongson.cn> 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-be820157e...@amd.com/ > --- > drivers/pci/vgaarb.c | 25 +++++++++++++------------ > 1 file changed, 13 insertions(+), 12 deletions(-) > > diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c > index c1bc6c983932..021116ed61cb 100644 > --- a/drivers/pci/vgaarb.c > +++ b/drivers/pci/vgaarb.c > @@ -754,10 +754,6 @@ static bool vga_arbiter_add_pci_device(struct pci_dev > *pdev) > struct pci_dev *bridge; > u16 cmd; > > - /* Only deal with VGA class devices */ > - if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA) > - return false; > - > /* Allocate structure */ > vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL); > if (vgadev == NULL) { > @@ -1502,6 +1498,10 @@ static int pci_notify(struct notifier_block *nb, > unsigned long action, > > vgaarb_dbg(dev, "%s\n", __func__); > > + /* Deal with VGA compatible devices only */ > + if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA) > + return 0; > + > /* For now we're only intereted in devices added and removed. I didn't > * test this thing here, so someone needs to double check for the > * cases of hotplugable vga cards. */ > @@ -1534,8 +1534,8 @@ static struct miscdevice vga_arb_device = { > > static int __init vga_arb_device_init(void) > { > + struct pci_dev *pdev = NULL; > int rc; > - struct pci_dev *pdev; > > rc = misc_register(&vga_arb_device); > if (rc < 0) > @@ -1543,13 +1543,14 @@ static int __init vga_arb_device_init(void) > > bus_register_notifier(&pci_bus_type, &pci_notifier); > > - /* We add all PCI devices satisfying VGA class in the arbiter by > - * default */ > - pdev = NULL; > - while ((pdev = > - pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, > - PCI_ANY_ID, pdev)) != NULL) > - vga_arbiter_add_pci_device(pdev); > + /* > + * We add all PCI VGA compatible devices in the arbiter by default > + */ > + do { > + pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev); > + if (pdev) > + vga_arbiter_add_pci_device(pdev); > + } while (pdev); > > pr_info("loaded\n"); > return rc; > -- > 2.25.1 >
