MFD: core assumes that all children are platform devices

MFD: core assumes that all children are platform devices

[Jean-François Dagenais]

Hi,

I have a pci driver that registers with UIO for it's operations. As a consequence, the pci device
instance has a child device of class uio.

My driver also declares a ds1wm instance that has it's register interface at an offset in BAR0
of the pci device, as an MFD cell.

When I call mfd_remove_devices, MFD proceeds to enumerate ALL the parent device's chilren
and assumes that they are MFD cells, and thus platform_device, which is not true in my case.
(...uio is a child of the parent pci device)

I had (luckily or unluckily) not seen signs of this broken assumption on certain setups I have
used, but in my current setup, this page-faults every time now.

This is a major thing and I have not found the assumption documented anywhere.

I could first declare a new child device on my pci device and then declare it as the parent to
the mfd cells...

Or, is there a way for the mfd-core, as it's doing the "for each child device", to recognize
non-MFD-cell children and skip them?

/jfd

[PATCH] mfd: core - make sure children are cells during mfd_remove_devices

[Jean-François Dagenais]

The MFD core assumes that all children of a parent MFD device are platform
devices and cells. This may not always be the case.

For example a PCI driver may register it's BAR0 with UIO (that creates a child
device) and also declare MFD cells which use resources of the PCI device. In
such a scenario, mfd_remove_devices would fall upon the UIO device, treat it
as a platform_device (which its not), then proceed to dereference the cell
pointer, which is then arbitrary memory.

This commit make mfd_remove_device use the bus_for_each_dev to find the true
platform devices under a given parent, and further checks if the cell pointer
is set to really make sure the given device is what we think it is.

Signed-off-by: Jean-François Dagenais <jeff.dagenais@gmail.com>
---
 drivers/mfd/mfd-core.c |   26 +++++++++++++++++++-------
 1 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 0902523..e82f02e 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -180,15 +180,23 @@ int mfd_add_devices(struct device *parent, int id,
 }
 EXPORT_SYMBOL(mfd_add_devices);
 
+struct mfd_remove_devices_fn_data {
+	struct device *parent;
+	atomic_t *cnts;
+};
+
 static int mfd_remove_devices_fn(struct device *dev, void *c)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	const struct mfd_cell *cell = mfd_get_cell(pdev);
-	atomic_t **usage_count = c;
+	struct mfd_remove_devices_fn_data *rm_data = c;
+
+	if(rm_data->parent != dev->parent || !cell)
+		return 0;
 
 	/* find the base address of usage_count pointers (for freeing) */
-	if (!*usage_count || (cell->usage_count < *usage_count))
-		*usage_count = cell->usage_count;
+	if (!rm_data->cnts || (cell->usage_count < rm_data->cnts))
+		rm_data->cnts = cell->usage_count;
 
 	platform_device_unregister(pdev);
 	return 0;
@@ -196,10 +204,14 @@ 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(parent, &cnts, mfd_remove_devices_fn);
-	kfree(cnts);
+	struct mfd_remove_devices_fn_data fn_data = {
+		.parent = parent,
+		.cnts = NULL,
+	};
+
+	bus_for_each_dev(&platform_bus_type, NULL,
+	                 &fn_data, mfd_remove_devices_fn);
+	kfree(fn_data.cnts);
 }
 EXPORT_SYMBOL(mfd_remove_devices);
 
-- 
1.7.6

Re: MFD: core assumes that all children are platform devices

[Guenter Roeck]

On Mon, Nov 28, 2011 at 11:23:00AM -0500, Jean-François Dagenais wrote:
> Hi,
> 
> I have a pci driver that registers with UIO for it's operations. As a consequence, the pci device
> instance has a child device of class uio.
> 
> My driver also declares a ds1wm instance that has it's register interface at an offset in BAR0
> of the pci device, as an MFD cell.
> 
> When I call mfd_remove_devices, MFD proceeds to enumerate ALL the parent device's chilren
> and assumes that they are MFD cells, and thus platform_device, which is not true in my case.
> (...uio is a child of the parent pci device)
> 
> I had (luckily or unluckily) not seen signs of this broken assumption on certain setups I have
> used, but in my current setup, this page-faults every time now.
> 
> This is a major thing and I have not found the assumption documented anywhere.
> 
> I could first declare a new child device on my pci device and then declare it as the parent to
> the mfd cells...
> 
I had the same problem, with a Multi-function USB device. Took me a while to figure out that
mfd_remove_devices() removed the USB child devices when I used the USB device as MFD parent device.

My solution was to do what you suggested - my MFD probe function now creates a platform device
to be used as MFD parent device. Works nicely, it doesn't require much additional code,
and I think it is cleaner than other possible solutions (at least the ones I came up with).
We'll see how it flies with the MFD and USB maintainers once I submit the patch ;).

> Or, is there a way for the mfd-core, as it's doing the "for each child device", to recognize
> non-MFD-cell children and skip them?
> 
Looking at your proposed patch, I personally prefer my solution. Of course it would be nice
if it was documented that MFD parent devices MUST be dedicated (platform) devices and must not
have any non-MFD child devices. This would be a simple documentation patch and avoid making
assumptions on MFD child device removal.

Guenter 

Re: MFD: core assumes that all children are platform devices

[Jean-François Dagenais]

Sent from my iPod

On 2011-11-29, at 1:57 PM, Guenter Roeck <guenter.roeck@ericsson.com> wrote:

> On Mon, Nov 28, 2011 at 11:23:00AM -0500, Jean-François Dagenais wrote:
>> Hi,
>> 
>> I have a pci driver that registers with UIO for it's operations. As a consequence, the pci device
>> instance has a child device of class uio.
>> 
>> My driver also declares a ds1wm instance that has it's register interface at an offset in BAR0
>> of the pci device, as an MFD cell.
>> 
>> When I call mfd_remove_devices, MFD proceeds to enumerate ALL the parent device's chilren
>> and assumes that they are MFD cells, and thus platform_device, which is not true in my case.
>> (...uio is a child of the parent pci device)
>> 
>> I had (luckily or unluckily) not seen signs of this broken assumption on certain setups I have
>> used, but in my current setup, this page-faults every time now.
>> 
>> This is a major thing and I have not found the assumption documented anywhere.
>> 
>> I could first declare a new child device on my pci device and then declare it as the parent to
>> the mfd cells...
>> 
> I had the same problem, with a Multi-function USB device. Took me a while to figure out that
> mfd_remove_devices() removed the USB child devices when I used the USB device as MFD parent device.
> 
> My solution was to do what you suggested - my MFD probe function now creates a platform device
> to be used as MFD parent device. Works nicely, it doesn't require much additional code,
> and I think it is cleaner than other possible solutions (at least the ones I came up with).
> We'll see how it flies with the MFD and USB maintainers once I submit the patch ;).
> 
>> Or, is there a way for the mfd-core, as it's doing the "for each child device", to recognize
>> non-MFD-cell children and skip them?
>> 
> Looking at your proposed patch, I personally prefer my solution.
I'd like to see your patch... You mean to say that you changed your driver, or the mfd core? If it's your driver's probe function, then the problem still exists out there and imposing an extra device alloc simply as a container is in my opinion both a hassle and a waste of resource. At the very least, this task should be done by the mfd core. It still seems pointless though, when you can filter out which children are the ones to remove, like what my patch does.
> Of course it would be nice
> if it was documented that MFD parent devices MUST be dedicated (platform) devices and must not
> have any non-MFD child devices. This would be a simple documentation patch and avoid making
> assumptions on MFD child device removal.
Well the patch uses already present assumptions, it just checks
for them.

What's good is that my patch doesn't prevent you from using a container platform device either.

We'll see what Samuel thinks...
> 
> Guenter 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Thanks for your response!

Re: MFD: core assumes that all children are platform devices

[Guenter Roeck]

On Tue, 2011-11-29 at 16:40 -0500, Jean-François Dagenais wrote:
> 
> Sent from my iPod
> 
> On 2011-11-29, at 1:57 PM, Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> 
> > On Mon, Nov 28, 2011 at 11:23:00AM -0500, Jean-François Dagenais wrote:
> >> Hi,
> >> 
> >> I have a pci driver that registers with UIO for it's operations. As a consequence, the pci device
> >> instance has a child device of class uio.
> >> 
> >> My driver also declares a ds1wm instance that has it's register interface at an offset in BAR0
> >> of the pci device, as an MFD cell.
> >> 
> >> When I call mfd_remove_devices, MFD proceeds to enumerate ALL the parent device's chilren
> >> and assumes that they are MFD cells, and thus platform_device, which is not true in my case.
> >> (...uio is a child of the parent pci device)
> >> 
> >> I had (luckily or unluckily) not seen signs of this broken assumption on certain setups I have
> >> used, but in my current setup, this page-faults every time now.
> >> 
> >> This is a major thing and I have not found the assumption documented anywhere.
> >> 
> >> I could first declare a new child device on my pci device and then declare it as the parent to
> >> the mfd cells...
> >> 
> > I had the same problem, with a Multi-function USB device. Took me a while to figure out that
> > mfd_remove_devices() removed the USB child devices when I used the USB device as MFD parent device.
> > 
> > My solution was to do what you suggested - my MFD probe function now creates a platform device
> > to be used as MFD parent device. Works nicely, it doesn't require much additional code,
> > and I think it is cleaner than other possible solutions (at least the ones I came up with).
> > We'll see how it flies with the MFD and USB maintainers once I submit the patch ;).
> > 
> >> Or, is there a way for the mfd-core, as it's doing the "for each child device", to recognize
> >> non-MFD-cell children and skip them?
> >> 
> > Looking at your proposed patch, I personally prefer my solution.
> I'd like to see your patch... You mean to say that you changed your driver, or the mfd core? If it's your driver's probe function, then the problem still exists out there and imposing an extra device alloc simply as a container is in my opinion both a hassle and a waste of resource. At the very least, this task should be done by the mfd core. It still seems pointless though, when you can filter out which children are the ones to remove, like what my patch does.

I did not touch the mfd core; I am in general not in favor of changing
core code if I can avoid it.

You can clone my driver from git://github.com/groeck/diolan.git. The mfd
core code is in diolan-u2c-core.c. Not in shape for submission, and the
SPI part is completely untested, but the MFD code should be ok.

> > Of course it would be nice
> > if it was documented that MFD parent devices MUST be dedicated (platform) devices and must not
> > have any non-MFD child devices. This would be a simple documentation patch and avoid making
> > assumptions on MFD child device removal.
> Well the patch uses already present assumptions, it just checks
> for them.
> 
The assumption I referred to was your assumption or expectation that
mfd_remove_devices() only removes a subset of child devices, not all of
them.

> What's good is that my patch doesn't prevent you from using a container platform device either.
> 
> We'll see what Samuel thinks...

Agreed. Either way, I think I'll stick with my approach - if nothing
else for backward compatibility. I would not want to have a driver which
depends on changed semantics of a kernel API function call.

Thanks,
Guenter

Re: MFD: core assumes that all children are platform devices

[Jean-Francois Dagenais]

On Nov 29, 2011, at 18:02, Guenter Roeck wrote:

> On Tue, 2011-11-29 at 16:40 -0500, Jean-François Dagenais wrote:
>> 
>> Sent from my iPod
>> 
>> On 2011-11-29, at 1:57 PM, Guenter Roeck <guenter.roeck@ericsson.com> wrote:
>> 
>>> On Mon, Nov 28, 2011 at 11:23:00AM -0500, Jean-François Dagenais wrote:
>>>> Hi,
>>>> ...
>>> Looking at your proposed patch, I personally prefer my solution.
>> I'd like to see your patch... You mean to say that you changed your driver, or the mfd core? If it's your driver's probe function, then the problem still exists out there and imposing an extra device alloc simply as a container is in my opinion both a hassle and a waste of resource. At the very least, this task should be done by the mfd core. It still seems pointless though, when you can filter out which children are the ones to remove, like what my patch does.
> 
> I did not touch the mfd core; I am in general not in favor of changing
> core code if I can avoid it.
I generally agree, only this time I felt it was a flaw in mfd_remove_devices.
> 
> You can clone my driver from git://github.com/groeck/diolan.git. The mfd
> core code is in diolan-u2c-core.c. Not in shape for submission, and the
> SPI part is completely untested, but the MFD code should be ok.
Thanks.
> 
>>> Of course it would be nice
>>> if it was documented that MFD parent devices MUST be dedicated (platform) devices and must not
>>> have any non-MFD child devices. This would be a simple documentation patch and avoid making
>>> assumptions on MFD child device removal.
>> Well the patch uses already present assumptions, it just checks
>> for them.
>> 
> The assumption I referred to was your assumption or expectation that
> mfd_remove_devices() only removes a subset of child devices, not all of
> them.
I think it's safe to say that the current non-documented assumption is that all children ARE cells. Which
I don't even believe is intentional.

When you think about it, in general the inverse of a API (i.e. add/remove, register/unregister)
should make all the efforts necessary to undo what IT knows it did, in this case, add_devices added
cells, but remove_devices removes ALL child devices. This is a mistake in terms of API sanity and it's
exactly what my patch addresses.
> 
>> What's good is that my patch doesn't prevent you from using a container platform device either.
>> 
>> We'll see what Samuel thinks...
> 
> Agreed. Either way, I think I'll stick with my approach - if nothing
> else for backward compatibility. I would not want to have a driver which
> depends on changed semantics of a kernel API function call.
Fair enough, I would do the same in your position.
> 
> Thanks,
> Guenter
> 
>