From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7BC60C0015E for ; Wed, 19 Jul 2023 19:58:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229637AbjGST6w (ORCPT ); Wed, 19 Jul 2023 15:58:52 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42880 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229681AbjGST6v (ORCPT ); Wed, 19 Jul 2023 15:58:51 -0400 Received: from out-21.mta0.migadu.com (out-21.mta0.migadu.com [91.218.175.21]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DA217B3 for ; Wed, 19 Jul 2023 12:58:49 -0700 (PDT) Message-ID: <9a1590bd-5dfc-94ad-645e-a0a499ae5b23@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1689796727; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=0X9JpDivKaFvl29MFTdl1MmoW2D7jWyZmoPkL8jU/wY=; b=g/U7HnDoDiCqmNZx9nygGmoTHsh/oR4gOBAp/GI+d29O4ufvQt6OR0CHo4r0LBcxZz9GnM /4jyWgrcpveBCEPEy5FQ5TPkk0pyFbmPiyrGZL1dAKoa+GM+RF9uf/7G/a5Eq/OqVKVBIQ 0hhXwg4DTFrgz5FZ2cobs56lIWG+5N8= Date: Thu, 20 Jul 2023 03:58:36 +0800 MIME-Version: 1.0 Subject: Re: [PATCH 2/6] PCI/VGA: Deal with PCI VGA compatible devices only To: Bjorn Helgaas Cc: Bjorn Helgaas , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Daniel Vetter , Sui Jingfeng , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, loongson-kernel@lists.loongnix.cn, Mario Limonciello References: <20230719182617.GA509912@bhelgaas> Content-Language: en-US X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Sui Jingfeng In-Reply-To: <20230719182617.GA509912@bhelgaas> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org Hi, On 2023/7/20 02:26, Bjorn Helgaas wrote: > On Tue, Jul 11, 2023 at 09:43:50PM +0800, Sui Jingfeng wrote: [...] >> Reviewed-by: Mario Limonciello >> Signed-off-by: Sui Jingfeng > 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. Yes, you are right. But I suddenly realized that this may deserve another patch, desperate trivial. What this version adds here is *same* before this patch set is applied. My explanation about the minor tweak being made before this version and previous version is that  I want to keep my patch *less distraction*. The major functional gains(benefit) is that we filter non VGA compatible devices out. As a start point, I should keep one patch do one thing (do one thing and do it well). On the other hand, even though the lest significant 8 but if pdev->class is really matter. I think I still need to wait the things(a bug emerged, for example) became clear. Instead of cleanup all potential problems with obvious motivation. I think Mario will accept my explanation. > [1] https://lore.kernel.org/r/20230718231400.GA496927@bhelgaas > [2] https://lore.kernel.org/all/5b6fdf65-b354-94a9-f883-be820157efad@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 >>