[PATCH RFC] mfd: Stop setting refcounting pointers in original mfd_cell arrays

[PATCH RFC] mfd: Stop setting refcounting pointers in original mfd_cell arrays

[Geert Uytterhoeven]

Commit 1e29af62f2b285bd18685da93c3ce8c33ca2d1db ("mfd: Add refcounting
support to mfd_cells") had to drop the "const" keyword on the "cell"
parameter of mfd_add_devices(), as it added the refcounting pointers
to the objects of the passed mfd_cell array itself.

However, the mfd core code operates on copies of the mfd_cell objects,
so there's no need to modify the originally passed objects.

Hence, move the setting of the refcounting pointers from mfd_add_devices()
to mfd_platform_add_cell(), where the copy of the mfd_cell objects is made.
mfd_clone_cell() can just pass (a copy of) the original usage_count
pointer.

This allows to make the "cell" parameter of mfd_add_devices() "const"
again, and avoids future race conditions when registering multiple
instances of the same device in parallel.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Ever since this "const" keyword was removed, I was wondering what was
really going on with these reference counts. My main use case involved
multiple instances of the same device, where the registration of each
successive instance would overwrite the refcounting pointer of the
previous one.
Finally I found some time to dive into it, and create this patch.
If it is accepted, I'll create a follow-up patch to add
"const" keywords to the various mfd_cell arrays.

Note that there's another issue in mfd_add_devices(): if the first call to
mfd_add_device() fails, the array of atomic_t objects will never be freed,
as no child devices have been registered to the parent yet, and thus
mfd_remove_devices() won't find anything to remove nor free.

 drivers/mfd/mfd-core.c   |   17 +++++++++--------
 include/linux/mfd/core.h |    2 +-
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index f421586f29fb..8736f4539bc0 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -63,7 +63,8 @@ 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)
+				 const struct mfd_cell *cell,
+				 atomic_t *usage_count)
 {
 	if (!cell)
 		return 0;
@@ -72,11 +73,12 @@ static int mfd_platform_add_cell(struct platform_device *pdev,
 	if (!pdev->mfd_cell)
 		return -ENOMEM;
 
+	pdev->mfd_cell->usage_count = usage_count;
 	return 0;
 }
 
 static int mfd_add_device(struct device *parent, int id,
-			  const struct mfd_cell *cell,
+			  const struct mfd_cell *cell, atomic_t *usage_count,
 			  struct resource *mem_base,
 			  int irq_base, struct irq_domain *domain)
 {
@@ -115,7 +117,7 @@ static int mfd_add_device(struct device *parent, int id,
 			goto fail_res;
 	}
 
-	ret = mfd_platform_add_cell(pdev, cell);
+	ret = mfd_platform_add_cell(pdev, cell, usage_count);
 	if (ret)
 		goto fail_res;
 
@@ -180,7 +182,7 @@ fail_alloc:
 }
 
 int mfd_add_devices(struct device *parent, int id,
-		    struct mfd_cell *cells, int n_devs,
+		    const struct mfd_cell *cells, int n_devs,
 		    struct resource *mem_base,
 		    int irq_base, struct irq_domain *domain)
 {
@@ -195,8 +197,7 @@ int mfd_add_devices(struct device *parent, int id,
 
 	for (i = 0; i < n_devs; i++) {
 		atomic_set(&cnts[i], 0);
-		cells[i].usage_count = &cnts[i];
-		ret = mfd_add_device(parent, id, cells + i, mem_base,
+		ret = mfd_add_device(parent, id, cells + i, cnts + i, mem_base,
 				     irq_base, domain);
 		if (ret)
 			break;
@@ -259,8 +260,8 @@ int mfd_clone_cell(const char *cell, const char **clones, size_t n_clones)
 	for (i = 0; i < n_clones; i++) {
 		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, NULL, 0,
-				   NULL))
+		if (mfd_add_device(pdev->dev.parent, -1, &cell_entry,
+				   cell_entry.usage_count, NULL, 0, NULL))
 			dev_err(dev, "failed to create platform device '%s'\n",
 					clones[i]);
 	}
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index cebe97ee98b8..60ced604664f 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -98,7 +98,7 @@ static inline const struct mfd_cell *mfd_get_cell(struct platform_device *pdev)
 }
 
 extern int mfd_add_devices(struct device *parent, int id,
-			   struct mfd_cell *cells, int n_devs,
+			   const struct mfd_cell *cells, int n_devs,
 			   struct resource *mem_base,
 			   int irq_base, struct irq_domain *irq_domain);
 
-- 
1.7.9.5

Re: [PATCH RFC] mfd: Stop setting refcounting pointers in original mfd_cell arrays

[Geert Uytterhoeven]

On Tue, Oct 29, 2013 at 10:03 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Note that there's another issue in mfd_add_devices(): if the first call to
> mfd_add_device() fails, the array of atomic_t objects will never be freed,
> as no child devices have been registered to the parent yet, and thus
> mfd_remove_devices() won't find anything to remove nor free.

This was actually not that difficult to fix, so I've just sent out a patch.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC] mfd: Stop setting refcounting pointers in original mfd_cell arrays

[Lee Jones]

On Tue, 29 Oct 2013, Geert Uytterhoeven wrote:

> Commit 1e29af62f2b285bd18685da93c3ce8c33ca2d1db ("mfd: Add refcounting
> support to mfd_cells") had to drop the "const" keyword on the "cell"
> parameter of mfd_add_devices(), as it added the refcounting pointers
> to the objects of the passed mfd_cell array itself.
> 
> However, the mfd core code operates on copies of the mfd_cell objects,
> so there's no need to modify the originally passed objects.
> 
> Hence, move the setting of the refcounting pointers from mfd_add_devices()
> to mfd_platform_add_cell(), where the copy of the mfd_cell objects is made.
> mfd_clone_cell() can just pass (a copy of) the original usage_count
> pointer.
> 
> This allows to make the "cell" parameter of mfd_add_devices() "const"
> again, and avoids future race conditions when registering multiple
> instances of the same device in parallel.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Ever since this "const" keyword was removed, I was wondering what was
> really going on with these reference counts. My main use case involved
> multiple instances of the same device, where the registration of each
> successive instance would overwrite the refcounting pointer of the
> previous one.
> Finally I found some time to dive into it, and create this patch.
> If it is accepted, I'll create a follow-up patch to add
> "const" keywords to the various mfd_cell arrays.
> 
> Note that there's another issue in mfd_add_devices(): if the first call to
> mfd_add_device() fails, the array of atomic_t objects will never be freed,
> as no child devices have been registered to the parent yet, and thus
> mfd_remove_devices() won't find anything to remove nor free.
> 
>  drivers/mfd/mfd-core.c   |   17 +++++++++--------
>  include/linux/mfd/core.h |    2 +-
>  2 files changed, 10 insertions(+), 9 deletions(-)

Hmm... I didn't see my reply to this, but it has been applied for a
while.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog