Message ID | 20230412131520.52840-1-andriy.shevchenko@linux.intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp330251vqo; Wed, 12 Apr 2023 06:34:34 -0700 (PDT) X-Google-Smtp-Source: AKy350blCwd8zDr6ZBQLyYO/XvuOkG0uOMcfs6g3U0XSTVBJdapf5M8Ug1EGpsFUe5TE+9mLHp7a X-Received: by 2002:a17:902:d50a:b0:1a2:749:5f1a with SMTP id b10-20020a170902d50a00b001a207495f1amr3301957plg.26.1681306474317; Wed, 12 Apr 2023 06:34:34 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1681306474; cv=none; d=google.com; s=arc-20160816; b=AExRocx1pHb1IKLlwLFxN7Nk90LOvA/uo6WgIRVh/zKZnTuEJPkt7P5myM/9MIe48U tpTZ0UzndDu6FjCaXZi1dwVRq3XhE/Twx6sU/ONlss3fIx+r2UnN1syIinIFRD71e/nV YaAmHrpYPomUkPKOLs+bIDp/NE0vwxBTCSkC12nJZYXLM/f3pPIJ8oBjCWLFxAEkGdtD gn3I55QbkBb62frS1YpVaTwestn+MM+vTZdVOUy0LF2kjwy7NR3lcQl0x7ir1SlYqKrY TnFdZinzIWY7BHsUjM82yMvojJ110wPC9qxr216jYgIzvgkOMFLmEEXxsG9ieXeaJr4G ak8w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=PzsTylHjbi7Acj5i8JxAg5lSJEQ0XCFWrwj+5u/ZzNg=; b=u7pbv4bKIGoe5dOCrwZIpqkMuyV4GD5aI2rePEzBmWXgCFgbg3NtlntV6phteZXM7E O0d8H4lHQxUvtESXqRdNBgbK1zk0zUM/ThUgDNwCSCsi0/fPA7zjM7ezzZptPorHwB7i dymRkcMdSGCqmqElgkpAU2gfIc8SIbbUVgLLn4YFKg0xRTAkXeKthVTx7IWvacym15/N evGG1enYteF6yTPn/wUXc4YHV+31E7MRxrQMvlKin8Or25tFps3Uu4bxBP4x0RlAuxbh XFBiTtdqV8j7vHt9PVVnUUAYI1CUxMm+7ajEIxtpWUg5c1rkgYQ/1No9zk/TWoYh6fFb KexQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=kqycX832; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id a4-20020a170902b58400b001a6387faaa6si8321269pls.422.2023.04.12.06.34.22; Wed, 12 Apr 2023 06:34:34 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=kqycX832; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230032AbjDLNUH (ORCPT <rfc822;peter110.wang@gmail.com> + 99 others); Wed, 12 Apr 2023 09:20:07 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45790 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231488AbjDLNT7 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 12 Apr 2023 09:19:59 -0400 Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 891789EC2; Wed, 12 Apr 2023 06:19:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1681305580; x=1712841580; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=JYAs2/Rz4YUIstRy5EIsBG2WbHJqPyfD3jBpMgyj1MQ=; b=kqycX832qBMuSH6jqZx5cr8R/Qc5+yMS/RAlkD9m+UuxlZGXXKNKaBjo rKUUBLSZQzzHOmEKKP4Rlh4+2GomYvmOO6ZlqGWpjO7kSPHzKtkxP+uRn TqQosw8FZvGYJq9dff8y/gHRXtu/+x2Xqy61jO8TdY/LgWe0Xm9Qmad6M USRkWpwn/vdgpd7PkVptWGBJQPunT/akCmC8O6kRmS4FnPyLCX7Bjt0hF Jg/OoCOTIgutnR+sag9w4talka8FdMwETzZRGKObt0LZG0D2NpvsxdqdS 4qpxmRD5IqkXso12JITzSqGjz/QWCZT4tEcLBOJ149Srefx9hq6+EtVht A==; X-IronPort-AV: E=McAfee;i="6600,9927,10678"; a="343896511" X-IronPort-AV: E=Sophos;i="5.98,339,1673942400"; d="scan'208";a="343896511" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Apr 2023 06:15:22 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10678"; a="935111585" X-IronPort-AV: E=Sophos;i="5.98,339,1673942400"; d="scan'208";a="935111585" Received: from black.fi.intel.com ([10.237.72.28]) by fmsmga006.fm.intel.com with ESMTP; 12 Apr 2023 06:15:21 -0700 Received: by black.fi.intel.com (Postfix, from userid 1003) id B08E8172; Wed, 12 Apr 2023 16:15:24 +0300 (EEST) From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> To: =?utf-8?q?Pali_Roh=C3=A1r?= <pali@kernel.org>, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Bjorn Helgaas <bhelgaas@google.com>, Andy Shevchenko <andriy.shevchenko@linux.intel.com> Subject: [PATCH v1 1/1] PCI: of: Propagate firmware node Date: Wed, 12 Apr 2023 16:15:20 +0300 Message-Id: <20230412131520.52840-1-andriy.shevchenko@linux.intel.com> X-Mailer: git-send-email 2.40.0.1.gaa8946217a0b MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE, SPF_NONE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1762977617375314181?= X-GMAIL-MSGID: =?utf-8?q?1762977617375314181?= |
Series |
[v1,1/1] PCI: of: Propagate firmware node
|
|
Commit Message
Andy Shevchenko
April 12, 2023, 1:15 p.m. UTC
Propagate firmware node by using a specific API call, i.e. device_set_node().
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pci/of.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
Comments
On Wed, Apr 12, 2023 at 04:15:20PM +0300, Andy Shevchenko wrote: > Propagate firmware node by using a specific API call, i.e. device_set_node(). Can you add a line or two about *why* we should do this, e.g., is this headed toward some goal? Is it a simplification that's 100% equivalent (doesn't seem so, see below)? Seems like there's an underlying long-term effort to unify things from OF and ACPI, which seems like a good thing, but at the moment it's a little confusing to follow. For instance pci_set_of_node() seems like it ought to be sort of analogous to pci_set_acpi_fwnode(), but they look nothing alike. > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/pci/of.c | 19 +++++++------------ > 1 file changed, 7 insertions(+), 12 deletions(-) > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > index 196834ed44fe..4bba00dfbfc5 100644 > --- a/drivers/pci/of.c > +++ b/drivers/pci/of.c > @@ -18,19 +18,18 @@ > #ifdef CONFIG_PCI > void pci_set_of_node(struct pci_dev *dev) > { > + struct device_node *node; > + > if (!dev->bus->dev.of_node) > return; > - dev->dev.of_node = of_pci_find_child_device(dev->bus->dev.of_node, > - dev->devfn); > - if (dev->dev.of_node) > - dev->dev.fwnode = &dev->dev.of_node->fwnode; > + node = of_pci_find_child_device(dev->bus->dev.of_node, dev->devfn); > + device_set_node(&dev->dev, of_fwnode_handle(node)); This doesn't seem 100% equivalent. If of_pci_find_child_device() returns NULL, the previous code doesn't set dev->dev.fwnode, but the new code does. > } > > void pci_release_of_node(struct pci_dev *dev) > { > of_node_put(dev->dev.of_node); > - dev->dev.of_node = NULL; > - dev->dev.fwnode = NULL; > + device_set_node(&dev->dev, NULL); > } > > void pci_set_bus_of_node(struct pci_bus *bus) > @@ -45,17 +44,13 @@ void pci_set_bus_of_node(struct pci_bus *bus) > bus->self->external_facing = true; > } > > - bus->dev.of_node = node; > - > - if (bus->dev.of_node) > - bus->dev.fwnode = &bus->dev.of_node->fwnode; > + device_set_node(&bus->dev, of_fwnode_handle(node)); > } > > void pci_release_bus_of_node(struct pci_bus *bus) > { > of_node_put(bus->dev.of_node); > - bus->dev.of_node = NULL; > - bus->dev.fwnode = NULL; > + device_set_node(&bus->dev, NULL); > } > > struct device_node * __weak pcibios_get_phb_of_node(struct pci_bus *bus) > -- > 2.40.0.1.gaa8946217a0b >
On Wed, Apr 12, 2023 at 11:02:53AM -0500, Bjorn Helgaas wrote: > On Wed, Apr 12, 2023 at 04:15:20PM +0300, Andy Shevchenko wrote: > > Propagate firmware node by using a specific API call, i.e. device_set_node(). > > Can you add a line or two about *why* we should do this, e.g., is this > headed toward some goal? Because dereferencing the fwnode in struct device is preventing us from modifications of how fwnode looks like in the future. > Is it a simplification that's 100% > equivalent (doesn't seem so, see below)? To me it's an equivalent, I'll explain below. > Seems like there's an underlying long-term effort to unify things from > OF and ACPI, which seems like a good thing, but at the moment it's a > little confusing to follow. For instance pci_set_of_node() seems like > it ought to be sort of analogous to pci_set_acpi_fwnode(), but they > look nothing alike. Unification to some extent, but here is not a point of this change. ... > > + struct device_node *node; > > + > > if (!dev->bus->dev.of_node) > > return; > > - dev->dev.of_node = of_pci_find_child_device(dev->bus->dev.of_node, > > - dev->devfn); > > - if (dev->dev.of_node) > > - dev->dev.fwnode = &dev->dev.of_node->fwnode; > > + node = of_pci_find_child_device(dev->bus->dev.of_node, dev->devfn); > > + device_set_node(&dev->dev, of_fwnode_handle(node)); > > This doesn't seem 100% equivalent. If of_pci_find_child_device() > returns NULL, the previous code doesn't set dev->dev.fwnode, but the > new code does. Yes and this is not a problem. We create device with pci_alloc_dev() in both callers of the pci_setup_device() and the field is NULL anyway. So, the last condition there is a simple micro-optimisation.
On Thu, Apr 13, 2023 at 07:00:26PM +0300, Andy Shevchenko wrote: > On Wed, Apr 12, 2023 at 11:02:53AM -0500, Bjorn Helgaas wrote: > > On Wed, Apr 12, 2023 at 04:15:20PM +0300, Andy Shevchenko wrote: > > > Propagate firmware node by using a specific API call, i.e. device_set_node(). > > > > Can you add a line or two about *why* we should do this, e.g., is this > > headed toward some goal? > > Because dereferencing the fwnode in struct device is preventing us from > modifications of how fwnode looks like in the future. How do you want to express this in the commit log? Something like this? Insulate pci_set_of_node() and pci_set_bus_of_node() from possible changes to fwnode_handle implementation by using device_set_node() instead of open-coding dev->dev.fwnode assignments. > > Is it a simplification that's 100% > > equivalent (doesn't seem so, see below)? > > To me it's an equivalent, I'll explain below. > > > Seems like there's an underlying long-term effort to unify things from > > OF and ACPI, which seems like a good thing, but at the moment it's a > > little confusing to follow. For instance pci_set_of_node() seems like > > it ought to be sort of analogous to pci_set_acpi_fwnode(), but they > > look nothing alike. > > Unification to some extent, but here is not a point of this change. > > ... > > > > + struct device_node *node; > > > + > > > if (!dev->bus->dev.of_node) > > > return; > > > - dev->dev.of_node = of_pci_find_child_device(dev->bus->dev.of_node, > > > - dev->devfn); > > > - if (dev->dev.of_node) > > > - dev->dev.fwnode = &dev->dev.of_node->fwnode; > > > + node = of_pci_find_child_device(dev->bus->dev.of_node, dev->devfn); > > > + device_set_node(&dev->dev, of_fwnode_handle(node)); > > > > This doesn't seem 100% equivalent. If of_pci_find_child_device() > > returns NULL, the previous code doesn't set dev->dev.fwnode, but the > > new code does. > > Yes and this is not a problem. We create device with pci_alloc_dev() in both > callers of the pci_setup_device() and the field is NULL anyway. So, the last > condition there is a simple micro-optimisation. OK, makes sense, thanks. Bjorn
On Fri, Apr 14, 2023 at 01:55:45PM -0500, Bjorn Helgaas wrote: > On Thu, Apr 13, 2023 at 07:00:26PM +0300, Andy Shevchenko wrote: > > On Wed, Apr 12, 2023 at 11:02:53AM -0500, Bjorn Helgaas wrote: > > > On Wed, Apr 12, 2023 at 04:15:20PM +0300, Andy Shevchenko wrote: ... > > > > Propagate firmware node by using a specific API call, i.e. device_set_node(). > > > > > > Can you add a line or two about *why* we should do this, e.g., is this > > > headed toward some goal? > > > > Because dereferencing the fwnode in struct device is preventing us from > > modifications of how fwnode looks like in the future. > > How do you want to express this in the commit log? Something like > this? > > Insulate pci_set_of_node() and pci_set_bus_of_node() from possible > changes to fwnode_handle implementation by using device_set_node() > instead of open-coding dev->dev.fwnode assignments. Sounds good to me, thanks for the draft. I will do it in v2 this way.
diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 196834ed44fe..4bba00dfbfc5 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -18,19 +18,18 @@ #ifdef CONFIG_PCI void pci_set_of_node(struct pci_dev *dev) { + struct device_node *node; + if (!dev->bus->dev.of_node) return; - dev->dev.of_node = of_pci_find_child_device(dev->bus->dev.of_node, - dev->devfn); - if (dev->dev.of_node) - dev->dev.fwnode = &dev->dev.of_node->fwnode; + node = of_pci_find_child_device(dev->bus->dev.of_node, dev->devfn); + device_set_node(&dev->dev, of_fwnode_handle(node)); } void pci_release_of_node(struct pci_dev *dev) { of_node_put(dev->dev.of_node); - dev->dev.of_node = NULL; - dev->dev.fwnode = NULL; + device_set_node(&dev->dev, NULL); } void pci_set_bus_of_node(struct pci_bus *bus) @@ -45,17 +44,13 @@ void pci_set_bus_of_node(struct pci_bus *bus) bus->self->external_facing = true; } - bus->dev.of_node = node; - - if (bus->dev.of_node) - bus->dev.fwnode = &bus->dev.of_node->fwnode; + device_set_node(&bus->dev, of_fwnode_handle(node)); } void pci_release_bus_of_node(struct pci_bus *bus) { of_node_put(bus->dev.of_node); - bus->dev.of_node = NULL; - bus->dev.fwnode = NULL; + device_set_node(&bus->dev, NULL); } struct device_node * __weak pcibios_get_phb_of_node(struct pci_bus *bus)