| Message ID | 20230615145243.37095-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:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp702921vqr;
Thu, 15 Jun 2023 08:04:45 -0700 (PDT)
X-Google-Smtp-Source:
ACHHUZ5/rTVgEyPPmYYSB6cwhpsczaT7+PHT1+9NipzSVlBYfJ3iuhjA6MLW9pGSQwk0lKmts1JL
X-Received: by 2002:aca:2402:0:b0:398:268a:1d00 with SMTP id
n2-20020aca2402000000b00398268a1d00mr13771206oic.51.1686841485220;
Thu, 15 Jun 2023 08:04:45 -0700 (PDT)
ARC-Seal: i=1; a=rsa-sha256; t=1686841485; cv=none;
d=google.com; s=arc-20160816;
b=CPoKTr5Ezfej2Shti6Q9CoZTPHEfy0nYhVfUK1F6jIkkyQmN1GzEkncoubzyC9x/gJ
A8byrjjidl+VZDJh9UQVAADkolmU/Hrr8qwhqVHdFPAqO1euScisleWM6lrMgLBxeRLh
8SpRNGejrhaxdT96qUsmgYoghTMD4BsMhhmDFSHlNnaAWady0sLjLmeeeN73rt3jFZid
rjKN52ZHJ07ovFRiHLEbaCeFVTq4jk0Y70WChShbx6BgD9YtRtEq7mFsc+b80EkeRiCE
kKRCdrAOGrMcJbJlcEkwk45ftMCP9aq9mB1IziE6tuK65/A1Jlcsel+gx6iUC8T9Kbmb
/SYA==
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=KHPqfT4dcvJIc+i+qcRgO8TxFWbq+iT0+uaPb4r5XC4=;
b=CvqB89y/1QANaRtCa9o5sjIZJ+2PCDpp6LDtN66YFsBH1Q1t/aKA4FOvclhz95wrgN
TSTzQwIs/OliZDaNah/F5W5xIZ2373vZiXAyKmV7UC5J0HwlNRTsXvPBkWzAqDNvbv5P
LmYsWhTwXpUZxGD7GSr0gn0LLLNaHXHgJtKQge6Jge4bKI0y4pejWlN/HrQNeOSo9mWn
++J9u1+8ho9qCGlMv3eYZ+pNgkSzMIAap+1s0OTLbBVvrKARw2/7D/cNcBJZYyIFYsL0
rCN3+IOo31JHJKZhMqVaDOen59liE+qlx01j6Y0+s3FkRp8d+kF37FhQEvfpUjOgWTxe
n0bg==
ARC-Authentication-Results: i=1; mx.google.com;
dkim=pass header.i=@intel.com header.s=Intel header.b=QTHbk0y2;
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
n7-20020a638f07000000b00546bab11218si12851966pgd.439.2023.06.15.08.04.09;
Thu, 15 Jun 2023 08:04:45 -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=QTHbk0y2;
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 S1344660AbjFOOwm (ORCPT <rfc822;n2h9z4@gmail.com> + 99 others);
Thu, 15 Jun 2023 10:52:42 -0400
Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35852 "EHLO
lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
with ESMTP id S232137AbjFOOwk (ORCPT
<rfc822;linux-kernel@vger.kernel.org>);
Thu, 15 Jun 2023 10:52:40 -0400
Received: from mga12.intel.com (mga12.intel.com [192.55.52.136])
by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A032110D8;
Thu, 15 Jun 2023 07:52:39 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple;
d=intel.com; i=@intel.com; q=dns/txt; s=Intel;
t=1686840759; x=1718376759;
h=from:to:cc:subject:date:message-id:mime-version:
content-transfer-encoding;
bh=kCi7DHYNcv6IoROHXZh3QF9cEVVIQC3uWI9Yr69isrA=;
b=QTHbk0y2cSFWbnqYp1F7tuLYIdXSusxk1TSP/EQkg4ppbOohxP/xhjci
ZvVvqWYxj8trUL6/e0ZQ9oKsuk8QkOYuHr67fJSkH1Db+a9Ic8Otj8LJF
K1pULPKjwwOCwly1SQL+lZe79Efxv2G3i7A7gCySsL+aE/i+Z2C/TLVji
ETSP6QDhLpO1kCj+fgPX1yPoIJF3wgugUxAWq0BxHU6kjY+AZBy9QJx3l
OxrA1slJehXucNXI6bibE3e4iP5UO9zLyfVh4QKojKlmEW0+XBLZvkY3A
Jo/lEnDUV09MEZDlQF9L9GqQ+g9+UmbKdS5hYfTtYsZbSDFy6GhpxPTTb
A==;
X-IronPort-AV: E=McAfee;i="6600,9927,10742"; a="338559714"
X-IronPort-AV: E=Sophos;i="6.00,245,1681196400";
d="scan'208";a="338559714"
Received: from orsmga003.jf.intel.com ([10.7.209.27])
by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;
15 Jun 2023 07:52:39 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=McAfee;i="6600,9927,10742"; a="662823914"
X-IronPort-AV: E=Sophos;i="6.00,245,1681196400";
d="scan'208";a="662823914"
Received: from black.fi.intel.com ([10.237.72.28])
by orsmga003.jf.intel.com with ESMTP; 15 Jun 2023 07:52:37 -0700
Received: by black.fi.intel.com (Postfix, from userid 1003)
id 1AB8B379; Thu, 15 Jun 2023 17:52:45 +0300 (EEST)
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Rob Herring <robh@kernel.org>, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
Cc: Rob Herring <robh+dt@kernel.org>,
Frank Rowand <frowand.list@gmail.com>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Subject: [PATCH v1 1/1] of/platform: Propagate firmware node by calling
device_set_node()
Date: Thu, 15 Jun 2023 17:52:43 +0300
Message-Id: <20230615145243.37095-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_PASS,
SPF_NONE,T_SCC_BODY_TEXT_LINE 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?1768781497110144401?=
X-GMAIL-MSGID: =?utf-8?q?1768781497110144401?=
|
| Series |
[v1,1/1] of/platform: Propagate firmware node by calling device_set_node()
|
|
Commit Message
Andy Shevchenko
June 15, 2023, 2:52 p.m. UTC
Insulate of_device_alloc() and of_amba_device_create() from possible
changes to fwnode_handle implementation by using device_set_node()
instead of open-coding dev->dev.fwnode assignments.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/of/platform.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
Comments
On Thu, Jun 15, 2023 at 05:52:43PM +0300, Andy Shevchenko wrote: > Insulate of_device_alloc() and of_amba_device_create() from possible > changes to fwnode_handle implementation by using device_set_node() > instead of open-coding dev->dev.fwnode assignments. Side note. When I preparing this change I have noticed a lot of dev_set_name(... dev_name()) in the code which seems to me problematic in two ways: 1) (minor) the dev_set_name() may fail, no checks are there; 2) (major?) the above construction leaks memory. Is it on purpose (esp. second point)? If no, can it be fixed? Note, I'm not familiar with OF platform code, so I would help reviewing the change, but that's it.
On Thu, Jun 15, 2023 at 05:59:52PM +0300, Andy Shevchenko wrote: > On Thu, Jun 15, 2023 at 05:52:43PM +0300, Andy Shevchenko wrote: > > Insulate of_device_alloc() and of_amba_device_create() from possible > > changes to fwnode_handle implementation by using device_set_node() > > instead of open-coding dev->dev.fwnode assignments. > > Side note. When I preparing this change I have noticed a lot of > > dev_set_name(... dev_name()) Plus dev_set_name(dev, ...) ... dev_set_name(dev, ...) on the same device will also give a memory leak. > in the code which seems to me problematic in two ways: > 1) (minor) the dev_set_name() may fail, no checks are there; > 2) (major?) the above construction leaks memory. > > Is it on purpose (esp. second point)? If no, can it be fixed? > Note, I'm not familiar with OF platform code, so I would help > reviewing the change, but that's it.
On Thu, Jun 15, 2023 at 06:01:17PM +0300, Andy Shevchenko wrote: > On Thu, Jun 15, 2023 at 05:59:52PM +0300, Andy Shevchenko wrote: > > On Thu, Jun 15, 2023 at 05:52:43PM +0300, Andy Shevchenko wrote: > > > Insulate of_device_alloc() and of_amba_device_create() from possible > > > changes to fwnode_handle implementation by using device_set_node() > > > instead of open-coding dev->dev.fwnode assignments. > > > > Side note. When I preparing this change I have noticed a lot of > > > > dev_set_name(... dev_name()) > > Plus > > dev_set_name(dev, ...) > ... > dev_set_name(dev, ...) > > on the same device will also give a memory leak. Ah, seems false alarm, the kobject_set_name_vargs() frees the old one. Sorry for the noise for second point. But the first one still applies. > > in the code which seems to me problematic in two ways: > > 1) (minor) the dev_set_name() may fail, no checks are there; > > 2) (major?) the above construction leaks memory. > > > > Is it on purpose (esp. second point)? If no, can it be fixed? > > Note, I'm not familiar with OF platform code, so I would help > > reviewing the change, but that's it.
On Thu, Jun 15, 2023 at 06:03:52PM +0300, Andy Shevchenko wrote: > On Thu, Jun 15, 2023 at 06:01:17PM +0300, Andy Shevchenko wrote: > > On Thu, Jun 15, 2023 at 05:59:52PM +0300, Andy Shevchenko wrote: > > > On Thu, Jun 15, 2023 at 05:52:43PM +0300, Andy Shevchenko wrote: > > > > Insulate of_device_alloc() and of_amba_device_create() from possible > > > > changes to fwnode_handle implementation by using device_set_node() > > > > instead of open-coding dev->dev.fwnode assignments. > > > > > > Side note. When I preparing this change I have noticed a lot of > > > > > > dev_set_name(... dev_name()) > > > > Plus > > > > dev_set_name(dev, ...) > > ... > > dev_set_name(dev, ...) > > > > on the same device will also give a memory leak. > > Ah, seems false alarm, the kobject_set_name_vargs() frees the old one. > Sorry for the noise for second point. But the first one still applies. > > > > in the code which seems to me problematic in two ways: > > > 1) (minor) the dev_set_name() may fail, no checks are there; Is there anything besides a memory alloc failure? What will print a message already. Wouldn't we fail a bit later on when adding the device anyways? In a rough count, 92 out of 500 cases check the return of dev_set_name(). Rob
On Thu, 15 Jun 2023 17:52:43 +0300, Andy Shevchenko wrote: > Insulate of_device_alloc() and of_amba_device_create() from possible > changes to fwnode_handle implementation by using device_set_node() > instead of open-coding dev->dev.fwnode assignments. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/of/platform.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > Applied, thanks!
On Thu, Jun 15, 2023 at 10:44:52AM -0600, Rob Herring wrote: > On Thu, Jun 15, 2023 at 06:03:52PM +0300, Andy Shevchenko wrote: > > On Thu, Jun 15, 2023 at 06:01:17PM +0300, Andy Shevchenko wrote: > > > On Thu, Jun 15, 2023 at 05:59:52PM +0300, Andy Shevchenko wrote: > > > > On Thu, Jun 15, 2023 at 05:52:43PM +0300, Andy Shevchenko wrote: ... > > > > in the code which seems to me problematic in two ways: > > > > 1) (minor) the dev_set_name() may fail, no checks are there; > > Is there anything besides a memory alloc failure? What will print a > message already. Wouldn't we fail a bit later on when adding the > device anyways? I don't see how we fail. Any pointers? > In a rough count, 92 out of 500 cases check the return of > dev_set_name(). Yeah, because it was new finding about that. Some static analyzers complain nowadays about this.
On Thu, Jun 15, 2023 at 08:13:52PM +0300, Andy Shevchenko wrote: > On Thu, Jun 15, 2023 at 10:44:52AM -0600, Rob Herring wrote: > > On Thu, Jun 15, 2023 at 06:03:52PM +0300, Andy Shevchenko wrote: > > > On Thu, Jun 15, 2023 at 06:01:17PM +0300, Andy Shevchenko wrote: > > > > On Thu, Jun 15, 2023 at 05:59:52PM +0300, Andy Shevchenko wrote: > > > > > On Thu, Jun 15, 2023 at 05:52:43PM +0300, Andy Shevchenko wrote: ... > > > > > in the code which seems to me problematic in two ways: > > > > > 1) (minor) the dev_set_name() may fail, no checks are there; > > > > Is there anything besides a memory alloc failure? What will print a > > message already. Wouldn't we fail a bit later on when adding the > > device anyways? > > I don't see how we fail. Any pointers? Okay, code in question: /* subsystems can specify simple device enumeration */ if (!dev_name(dev) && dev->bus && dev->bus->dev_name) dev_set_name(dev, "%s%u", dev->bus->dev_name, dev->id); if (!dev_name(dev)) { error = -EINVAL; goto name_error; }
diff --git a/drivers/of/platform.c b/drivers/of/platform.c index 78ae84187449..051e29b7ad2b 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -140,8 +140,8 @@ struct platform_device *of_device_alloc(struct device_node *np, } } - dev->dev.of_node = of_node_get(np); - dev->dev.fwnode = &np->fwnode; + /* setup generic device info */ + device_set_node(&dev->dev, of_fwnode_handle(np)); dev->dev.parent = parent ? : &platform_bus; if (bus_id) @@ -239,8 +239,7 @@ static struct amba_device *of_amba_device_create(struct device_node *node, dev->dev.dma_mask = &dev->dev.coherent_dma_mask; /* setup generic device info */ - dev->dev.of_node = of_node_get(node); - dev->dev.fwnode = &node->fwnode; + device_set_node(&dev->dev, of_fwnode_handle(node)); dev->dev.parent = parent ? : &platform_bus; dev->dev.platform_data = platform_data; if (bus_id)