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:   Lee Jones <lee.jones@linaro.org>
To:     broonie@kernel.org, linus.walleij@linaro.org,
        daniel.thompson@linaro.org, arnd@arndb.de
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        baohua@kernel.org, stephan@gerhold.net,
        Lee Jones <lee.jones@linaro.org>
Subject: [PATCH 1/2] mfd: mfd-core: Allocate reference counting memory directly to the platform device
Date:   Fri, 18 Oct 2019 13:26:46 +0100
Message-Id: <20191018122647.3849-2-lee.jones@linaro.org>
In-Reply-To: <20191018122647.3849-1-lee.jones@linaro.org>
References: <20191018122647.3849-1-lee.jones@linaro.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

MFD provides reference counting (for the 2 consumers who actually use it!)
via mfd_cell's 'usage_count' member.  However, since MFD cells become
read-only (const), MFD needs to allocate writable memory and assign it to
'usage_count' before first registration.  It currently does this by
allocating enough memory for all requested child devices (yes, even disabled
ones - but we'll get to that) and assigning the base pointer plus sub-device
index to each device in the cell.

The difficulty comes when trying to free that memory.  During the removal of
the parent device, MFD unregisters each child device, keeping a tally on the
lowest memory location pointed to by a child device's 'usage_count'.  Once
all of the children are unregistered, the lowest memory location must be the
base address of the previously allocated array, right?

Well yes, until we try to honour the disabling of devices via Device Tree
for instance.  If the first child device in the provided batch is disabled,
simply skipping registration (and consequentially deregistration) will mean
that the first device's 'usage_count' pointer will not be accounted for when
attempting to find the base.  In which case, MFD will assume the first non-
disabled 'usage_count' pointer is the base and subsequently attempt to
erroneously free it.

We can avoid all of this hoop jumping by simply allocating memory to each
single child device before it is considered read-only.  We can then free
it on a per-device basis during deregistration.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/mfd-core.c | 42 ++++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 23276a80e3b4..eafdadd58e8b 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -61,9 +61,10 @@ int mfd_cell_disable(struct platform_device *pdev)
 EXPORT_SYMBOL(mfd_cell_disable);
 
 static int mfd_platform_add_cell(struct platform_device *pdev,
-				 const struct mfd_cell *cell,
-				 atomic_t *usage_count)
+				 const struct mfd_cell *cell)
 {
+	atomic_t *usage_count;
+
 	if (!cell)
 		return 0;
 
@@ -71,7 +72,14 @@ static int mfd_platform_add_cell(struct platform_device *pdev,
 	if (!pdev->mfd_cell)
 		return -ENOMEM;
 
+	usage_count = kcalloc(1, sizeof(*usage_count), GFP_KERNEL);
+	if (!usage_count) {
+		kfree(pdev->mfd_cell);
+		return -ENOMEM;
+	}
+
 	pdev->mfd_cell->usage_count = usage_count;
+
 	return 0;
 }
 
@@ -134,7 +142,7 @@ static inline void mfd_acpi_add_device(const struct mfd_cell *cell,
 #endif
 
 static int mfd_add_device(struct device *parent, int id,
-			  const struct mfd_cell *cell, atomic_t *usage_count,
+			  const struct mfd_cell *cell,
 			  struct resource *mem_base,
 			  int irq_base, struct irq_domain *domain)
 {
@@ -196,7 +204,7 @@ static int mfd_add_device(struct device *parent, int id,
 			goto fail_alias;
 	}
 
-	ret = mfd_platform_add_cell(pdev, cell, usage_count);
+	ret = mfd_platform_add_cell(pdev, cell);
 	if (ret)
 		goto fail_alias;
 
@@ -286,16 +294,9 @@ int mfd_add_devices(struct device *parent, int id,
 {
 	int i;
 	int ret;
-	atomic_t *cnts;
-
-	/* initialize reference counting for all cells */
-	cnts = kcalloc(n_devs, sizeof(*cnts), GFP_KERNEL);
-	if (!cnts)
-		return -ENOMEM;
 
 	for (i = 0; i < n_devs; i++) {
-		atomic_set(&cnts[i], 0);
-		ret = mfd_add_device(parent, id, cells + i, cnts + i, mem_base,
+		ret = mfd_add_device(parent, id, cells + i, mem_base,
 				     irq_base, domain);
 		if (ret)
 			goto fail;
@@ -306,17 +307,15 @@ int mfd_add_devices(struct device *parent, int id,
 fail:
 	if (i)
 		mfd_remove_devices(parent);
-	else
-		kfree(cnts);
+
 	return ret;
 }
 EXPORT_SYMBOL(mfd_add_devices);
 
-static int mfd_remove_devices_fn(struct device *dev, void *c)
+static int mfd_remove_devices_fn(struct device *dev, void *data)
 {
 	struct platform_device *pdev;
 	const struct mfd_cell *cell;
-	atomic_t **usage_count = c;
 
 	if (dev->type != &mfd_dev_type)
 		return 0;
@@ -327,9 +326,7 @@ static int mfd_remove_devices_fn(struct device *dev, void *c)
 	regulator_bulk_unregister_supply_alias(dev, cell->parent_supplies,
 					       cell->num_parent_supplies);
 
-	/* find the base address of usage_count pointers (for freeing) */
-	if (!*usage_count || (cell->usage_count < *usage_count))
-		*usage_count = cell->usage_count;
+	kfree(cell->usage_count);
 
 	platform_device_unregister(pdev);
 	return 0;
@@ -337,10 +334,7 @@ static int mfd_remove_devices_fn(struct device *dev, void *c)
 
 void mfd_remove_devices(struct device *parent)
 {
-	atomic_t *cnts = NULL;
-
-	device_for_each_child_reverse(parent, &cnts, mfd_remove_devices_fn);
-	kfree(cnts);
+	device_for_each_child_reverse(parent, NULL, mfd_remove_devices_fn);
 }
 EXPORT_SYMBOL(mfd_remove_devices);
 
@@ -404,7 +398,7 @@ int mfd_clone_cell(const char *cell, const char **clones, size_t n_clones)
 		cell_entry.name = clones[i];
 		/* don't give up if a single call fails; just report error */
 		if (mfd_add_device(pdev->dev.parent, -1, &cell_entry,
-				   cell_entry.usage_count, NULL, 0, NULL))
+				   NULL, 0, NULL))
 			dev_err(dev, "failed to create platform device '%s'\n",
 					clones[i]);
 	}
-- 
2.17.1