Re: [Intel-gfx] [PATCH v3 3/4] PCI/VGA: only deal with VGA class devices

[Sui Jingfeng]

Hi,

On 2023/6/9 03:12, Bjorn Helgaas wrote:
> Start with verb and capitalize to match ("Deal only with ...")
>
> On Thu, Jun 08, 2023 at 07:43:21PM +0800, Sui Jingfeng wrote:
> > From: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
> >
> > vgaarb only deal with the VGA devcie(pdev->class == 0x0300), so replace the
> > pci_get_subsys() function with pci_get_class(). Filter the non pci display
> > device out. There no need to process the non display PCI device.
> s/non pci/non-PCI/
> s/non display/non-display/

Will be fixed at next version, thanks for the strict checking,

I will try to bring this rigorous to the other patch of myself in the 
future.

> This is fine, and I'll merge this, but someday I would like to get rid
> of the bus_register_notifier() and call vga_arbiter_add_pci_device()
> and vga_arbiter_del_pci_device() directly from the PCI core.
>
> Or if you wanted to do that now, that would be even better :)

I think, I probably should only do what I'm good at.


The "good at" here means that It's under prepared,

already matured with thinking(or consideration) by myself.

And also including testing(on two or three architecture).


That idea is belong to you, I would like to see how does it going to 
happen only.

> Bjorn
>
> > Signed-off-by: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
> > ---
> >   drivers/pci/vgaarb.c | 22 ++++++++++++----------
> >   1 file changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> > index 7f0347f4f6fd..b0bf4952a95d 100644
> > --- a/drivers/pci/vgaarb.c
> > +++ b/drivers/pci/vgaarb.c
> > @@ -756,10 +756,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) {
> > @@ -1499,7 +1495,9 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
> >   	struct pci_dev *pdev = to_pci_dev(dev);
> >   	bool notify = false;
> >   
> > -	vgaarb_dbg(dev, "%s\n", __func__);
> > +	/* Only deal with VGA class devices */
> > +	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
> > @@ -1509,6 +1507,8 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
> >   	else if (action == BUS_NOTIFY_DEL_DEVICE)
> >   		notify = vga_arbiter_del_pci_device(pdev);
> >   
> > +	vgaarb_dbg(dev, "%s: action = %lu\n", __func__, action);
> > +
> >   	if (notify)
> >   		vga_arbiter_notify_clients();
> >   	return 0;
> > @@ -1533,8 +1533,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)
> > @@ -1545,11 +1545,13 @@ static int __init vga_arb_device_init(void)
> >   	/* 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)
> > +	while (1) {
> > +		pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev);
> > +		if (!pdev)
> > +			break;
> > +
> >   		vga_arbiter_add_pci_device(pdev);
> > +	};
> >   
> >   	pr_info("loaded\n");
> >   	return rc;
> > --
> > 2.25.1
--
Jingfeng