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)