Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   "Chang, Junxiao" <junxiao.chang@intel.com>
To:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC:     "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "rafael@kernel.org" <rafael@kernel.org>,
        "Li, Lili" <lili.li@intel.com>
Subject: RE: [PATCH] platform: release resource itself instead of resource
 tree
Thread-Topic: [PATCH] platform: release resource itself instead of resource
 tree
Thread-Index: AQHU+y/55Via5j+fREqcQbqrpZ0MZqaDO8ow
Date:   Thu, 30 May 2019 03:50:32 +0000
Message-ID: <840F6BCBBBA89F46BAD0D7D6EF39E6E350F220C7@SHSMSX107.ccr.corp.intel.com>
References: <1556173458-9318-1-git-send-email-junxiao.chang@intel.com>
In-Reply-To: <1556173458-9318-1-git-send-email-junxiao.chang@intel.com>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-version: 11.0.600.7
dlp-reaction: no-action
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Any update?

This issue could be reproduced in one intel platform. To simulate the issue, adding following code could reproduce the issue.
Without the fix, device 2's resource will be released but the device is still registered.
With the fix, by cat /proc/iomem, device 2's resource is still there after device 1 is unregistered, this is expected.

Any comment is welcome, thanks,
Junxiao

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 4d17298..6832833 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1500,3 +1500,63 @@ void __init early_platform_cleanup(void)
 	}
 }
 
+static struct resource resource1[] = {
+	{
+		.start = 0xfed1a000,
+		.end = 0xfed1afff,
+		.flags = IORESOURCE_MEM,
+	}
+};
+
+static struct resource resource2[] = {
+	{
+		.start = 0xfed1a200,
+		.end = 0xfed1a2ff,
+		.flags = IORESOURCE_MEM,
+	}
+};
+
+static int __init simulate_insmod_rmmod(void)
+{
+	struct platform_device *pdev1, *pdev2;
+
+	const struct platform_device_info pdevinfo1 = {
+		.parent = NULL,
+		.name = "device1",
+		.id = -1,
+		.res = resource1,
+		.num_res = 1,
+		};
+
+	const struct platform_device_info pdevinfo2 = {
+		.parent = NULL,
+		.name = "device2",
+		.id = -1,
+		.res = resource2,
+		.num_res = 1,
+	};
+
+	// Register platform test device 1, resource 0xfed1a000 ~ 0xfed1afff
+	pdev1 = platform_device_register_full(&pdevinfo1);
+	if (IS_ERR(pdev1)) {
+		printk("Unable to register device 1\n");
+		return -1;
+	}
+
+	// Register platform test device 2, resource 0xfed1a200 ~ 0xfed1a2ff
+	pdev2 = platform_device_register_full(&pdevinfo2);
+	if (IS_ERR(pdev2)) {
+		printk("Unable to register device 2\n");
+		platform_device_unregister(pdev1);
+		return -1;
+	}
+
+
+	// Now platform device 2 resource should be device 1 resource's child
+
+	// Unregister device 1 only
+	platform_device_unregister(pdev1);
+
+	return 0;
+}
+fs_initcall(simulate_insmod_rmmod);

-----Original Message-----
From: Chang, Junxiao 
Sent: Thursday, April 25, 2019 2:24 PM
To: linux-kernel@vger.kernel.org
Cc: gregkh@linuxfoundation.org; rafael@kernel.org; Chang, Junxiao <junxiao.chang@intel.com>; Li, Lili <lili.li@intel.com>
Subject: [PATCH] platform: release resource itself instead of resource tree

From: Junxiao Chang <junxiao.chang@intel.com>

When platform device is deleted or there is error in adding device, platform device resources should be released. Currently API release_resource is used to release platform device resources.
However, this API releases not only platform resource itself but also its child resources. It might release resources which are still in use. Calling remove_resource only releases current resource itself, not resource tree, it moves its child resources to up level.

For example, platform device 1 and device 2 are registered, then only device 1 is unregistered in below code:

  ...
  // Register platform test device 1, resource 0xfed1a000 ~ 0xfed1afff
  pdev1 = platform_device_register_full(&pdevinfo1);

  // Register platform test device 2, resource 0xfed1a200 ~ 0xfed1a2ff
  pdev2 = platform_device_register_full(&pdevinfo2);

  // Now platform device 2 resource should be device 1 resource's child

  // Unregister device 1 only
  platform_device_unregister(pdev1);
  ...

Platform device 2 resource will be released as well because its parent resource(device 1's resource) is released, this is not expected.
If using API remove_resource, device 2 resource will not be released.

This change fixed an intel pmc platform device resource issue when intel pmc ipc kernel module is inserted/removed for twice.

Signed-off-by: Junxiao Chang <junxiao.chang@intel.com>
---
 drivers/base/platform.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c index dab0a5a..5fd1a41 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -461,7 +461,7 @@ int platform_device_add(struct platform_device *pdev)
 	while (--i >= 0) {
 		struct resource *r = &pdev->resource[i];
 		if (r->parent)
-			release_resource(r);
+			remove_resource(r);
 	}
 
  err_out:
@@ -492,7 +492,7 @@ void platform_device_del(struct platform_device *pdev)
 		for (i = 0; i < pdev->num_resources; i++) {
 			struct resource *r = &pdev->resource[i];
 			if (r->parent)
-				release_resource(r);
+				remove_resource(r);
 		}
 	}
 }
--
2.7.4