From: Andres Salomon <hidden> Date: 2011-02-03 04:08:19
No need to explicitly set the cell's platform_data/data_size.
In this case, move the various platform_data pointers
to driver_data. All of the clients which make use of it
are also changed.
Signed-off-by: Andres Salomon <redacted>
---
drivers/dma/timb_dma.c | 2 +-
drivers/gpio/timbgpio.c | 5 +-
drivers/i2c/busses/i2c-ocores.c | 2 +-
drivers/i2c/busses/i2c-xiic.c | 2 +-
drivers/media/radio/radio-timb.c | 2 +-
drivers/media/video/timblogiw.c | 2 +-
drivers/mfd/timberdale.c | 81 +++++++++++++-------------------------
drivers/net/ks8842.c | 2 +-
drivers/spi/xilinx_spi.c | 2 +-
9 files changed, 36 insertions(+), 64 deletions(-)
From: Grant Likely <hidden> Date: 2011-03-31 23:05:29
On Wed, Feb 02, 2011 at 08:08:12PM -0800, Andres Salomon wrote:
quoted hunk
No need to explicitly set the cell's platform_data/data_size.
In this case, move the various platform_data pointers
to driver_data. All of the clients which make use of it
are also changed.
Signed-off-by: Andres Salomon <redacted>
---
drivers/dma/timb_dma.c | 2 +-
drivers/gpio/timbgpio.c | 5 +-
drivers/i2c/busses/i2c-ocores.c | 2 +-
drivers/i2c/busses/i2c-xiic.c | 2 +-
drivers/media/radio/radio-timb.c | 2 +-
drivers/media/video/timblogiw.c | 2 +-
drivers/mfd/timberdale.c | 81 +++++++++++++-------------------------
drivers/net/ks8842.c | 2 +-
drivers/spi/xilinx_spi.c | 2 +-
9 files changed, 36 insertions(+), 64 deletions(-)
Hold the phone. I know this has already been merged, but this isn't correct.
drvdata is under the ownership of the driver, which means it should
*not* be set when .probe() gets called. It is for driver private data
to do with as it needs, usually for internal state.
Gah. Not all devices instantiated via mfd will be an mfd device,
which means that the driver may very well expect an *entirely
different* platform_device pointer; which further means a very high
potential of incorrectly dereferenced structures (as evidenced by a
patch series that is not bisectable). For instance, the xilinx ip
cores are used by more than just mfd.
This is really bad. Since this is new in .39-rc1, I really think this
series needs to be reverted. Samuel, can you look at this please?
g.
quoted hunk
struct timb_dma *td;
struct resource *iomem;
int irq;
From: Samuel Ortiz <hidden> Date: 2011-04-01 11:20:44
Hi Grant,
On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
On Wed, Feb 02, 2011 at 08:08:12PM -0800, Andres Salomon wrote:
quoted
No need to explicitly set the cell's platform_data/data_size.
In this case, move the various platform_data pointers
to driver_data. All of the clients which make use of it
are also changed.
Signed-off-by: Andres Salomon <redacted>
---
drivers/dma/timb_dma.c | 2 +-
drivers/gpio/timbgpio.c | 5 +-
drivers/i2c/busses/i2c-ocores.c | 2 +-
drivers/i2c/busses/i2c-xiic.c | 2 +-
drivers/media/radio/radio-timb.c | 2 +-
drivers/media/video/timblogiw.c | 2 +-
drivers/mfd/timberdale.c | 81 +++++++++++++-------------------------
drivers/net/ks8842.c | 2 +-
drivers/spi/xilinx_spi.c | 2 +-
9 files changed, 36 insertions(+), 64 deletions(-)
Hold the phone. I know this has already been merged, but this isn't correct.
drvdata is under the ownership of the driver, which means it should
*not* be set when .probe() gets called. It is for driver private data
to do with as it needs, usually for internal state.
We didn't merge that version of the patchset, but one getting the
platform_data pointer from mfd_get_data(). That introduces the issue you're
talking about below.
Gah. Not all devices instantiated via mfd will be an mfd device,
which means that the driver may very well expect an *entirely
different* platform_device pointer; which further means a very high
potential of incorrectly dereferenced structures (as evidenced by a
patch series that is not bisectable). For instance, the xilinx ip
cores are used by more than just mfd.
I agree. Since the vast majority of the MFD subdevices are MFD specific IPs, I
overlooked that part. The impacted drivers are the timberdale and the DaVinci
voice codec ones.
To fix that problem I propose 2 alternatives:
1) When declaring the sub devices cells, the MFD driver should specify an
mfd_data_size value for sub devices that are not MFD specific. It's the MFD
driver responsibility to set the cell properly, and the non MFD specific
drivers are kept MFD agnostic.
See my patch below for the timberdale case.
2) Revert the mfd_get_data() call for getting sub devices platform data
pointers. That was introduced to ease the MFD cell sharing work, so if we take
this route we'll need the cs5535 MFD driver to pass its cells as platform_data
pointer. Andres, can you confirm that this would be fine for the
mfd_clone_cell() routine to keep working ?
Patch for solution 1:
drivers/mfd/mfd-core.c | 13 ++++++++++---
drivers/mfd/timberdale.c | 11 +++++++++++
include/linux/mfd/core.h | 1 +
drivers/i2c/busses/i2c-ocores.c | 3 +--
drivers/i2c/busses/i2c-xiic.c | 3 +--
drivers/net/ks8842.c | 3 +--
drivers/spi/xilinx_spi.c | 3 +--
7 files changed, 26 insertions(+), 11 deletions(-)
@@ -35,6 +35,7 @@ struct mfd_cell {/* mfd_data can be used to pass data to client drivers */void*mfd_data;+size_tmfd_data_size;/**Theseresourcescanbespecifiedrelativetotheparentdevice.
From: Andres Salomon <hidden> Date: 2011-04-01 17:48:09
On Fri, 1 Apr 2011 13:20:31 +0200
Samuel Ortiz [off-list ref] wrote:
Hi Grant,
On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
[...]
quoted
Gah. Not all devices instantiated via mfd will be an mfd device,
which means that the driver may very well expect an *entirely
different* platform_device pointer; which further means a very high
potential of incorrectly dereferenced structures (as evidenced by a
patch series that is not bisectable). For instance, the xilinx ip
cores are used by more than just mfd.
I agree. Since the vast majority of the MFD subdevices are MFD
specific IPs, I overlooked that part. The impacted drivers are the
timberdale and the DaVinci voice codec ones.
Can you please provide pointers to what you're referring to? The only
code that I could find that created platform devices prefixed with
'timb-' or named 'xilinx_spi' was drivers/mfd/timberdale.c.
To fix that problem I propose 2 alternatives:
1) When declaring the sub devices cells, the MFD driver should
specify an mfd_data_size value for sub devices that are not MFD
specific. It's the MFD driver responsibility to set the cell
properly, and the non MFD specific drivers are kept MFD agnostic.
See my patch below for the timberdale case.
2) Revert the mfd_get_data() call for getting sub devices platform
data pointers. That was introduced to ease the MFD cell sharing work,
so if we take this route we'll need the cs5535 MFD driver to pass its
cells as platform_data pointer. Andres, can you confirm that this
would be fine for the mfd_clone_cell() routine to keep working ?
It would break mfd_clone_cell, as it uses mfd_get_cell to grab the one
to clone. We could change it to accept the cell as an argument. It
would also break mfd_cell_enable/disable, of course.
From: Grant Likely <hidden> Date: 2011-04-01 17:57:03
On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon [off-list ref] wrote:
On Fri, 1 Apr 2011 13:20:31 +0200
Samuel Ortiz [off-list ref] wrote:
quoted
Hi Grant,
On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
[...]
quoted
quoted
Gah. Not all devices instantiated via mfd will be an mfd device,
which means that the driver may very well expect an *entirely
different* platform_device pointer; which further means a very high
potential of incorrectly dereferenced structures (as evidenced by a
patch series that is not bisectable). For instance, the xilinx ip
cores are used by more than just mfd.
I agree. Since the vast majority of the MFD subdevices are MFD
specific IPs, I overlooked that part. The impacted drivers are the
timberdale and the DaVinci voice codec ones.
Another option is you could do this for MFD devices:
struct mfd_device {
struct platform_devce pdev;
struct mfd_cell *cell;
};
However, that requires that drivers using the mfd_cell will *never*
get instantiated outside of the mfd infrastructure, and there is no
way to protect against this so it is probably a bad idea.
Or, mfd_cell could be added to platform_device directly which would
*by far* be the safest option at the cost of every platform_device
having a mostly unused mfd_cell pointer. Not a significant cost in my
opinion.
One last option is I'm prototyping a way to add type-safe structure
pointers to a device, but that requires nasty CPP tricks and it's not
complete yet. The cure might be worse than the disease here.
g.
Can you please provide pointers to what you're referring to? The only
code that I could find that created platform devices prefixed with
'timb-' or named 'xilinx_spi' was drivers/mfd/timberdale.c.
quoted
To fix that problem I propose 2 alternatives:
1) When declaring the sub devices cells, the MFD driver should
specify an mfd_data_size value for sub devices that are not MFD
specific. It's the MFD driver responsibility to set the cell
properly, and the non MFD specific drivers are kept MFD agnostic.
See my patch below for the timberdale case.
This approach worries me because it changes the behaviour on a
per-device basis. That could be difficult to maintain a mental model
for. I'd rather see consistent behaviour.
quoted
2) Revert the mfd_get_data() call for getting sub devices platform
data pointers. That was introduced to ease the MFD cell sharing work,
so if we take this route we'll need the cs5535 MFD driver to pass its
cells as platform_data pointer. Andres, can you confirm that this
would be fine for the mfd_clone_cell() routine to keep working ?
It would break mfd_clone_cell, as it uses mfd_get_cell to grab the one
to clone. We could change it to accept the cell as an argument. It
would also break mfd_cell_enable/disable, of course.
From: Grant Likely <hidden> Date: 2011-04-01 18:01:20
On Fri, Apr 1, 2011 at 11:56 AM, Grant Likely [off-list ref] wrote:
On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon [off-list ref] wrote:
quoted
On Fri, 1 Apr 2011 13:20:31 +0200
Samuel Ortiz [off-list ref] wrote:
quoted
Hi Grant,
On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
[...]
quoted
quoted
Gah. Not all devices instantiated via mfd will be an mfd device,
which means that the driver may very well expect an *entirely
different* platform_device pointer; which further means a very high
potential of incorrectly dereferenced structures (as evidenced by a
patch series that is not bisectable). For instance, the xilinx ip
cores are used by more than just mfd.
I agree. Since the vast majority of the MFD subdevices are MFD
specific IPs, I overlooked that part. The impacted drivers are the
timberdale and the DaVinci voice codec ones.
Another option is you could do this for MFD devices:
struct mfd_device {
struct platform_devce pdev;
struct mfd_cell *cell;
};
However, that requires that drivers using the mfd_cell will *never*
get instantiated outside of the mfd infrastructure, and there is no
way to protect against this so it is probably a bad idea.
Or, mfd_cell could be added to platform_device directly which would
*by far* be the safest option at the cost of every platform_device
having a mostly unused mfd_cell pointer. Not a significant cost in my
opinion.
One last option is I'm prototyping a way to add type-safe structure
pointers to a device, but that requires nasty CPP tricks and it's not
complete yet. The cure might be worse than the disease here.
And yet another option is to create a mfd_bus_type, but that probably
isn't helpful since the one of the purposes of MFDs is that it is a
collection of non-detectable memory mapped devices that
platform_bus_type is intended to handle.
g.
From: Samuel Ortiz <hidden> Date: 2011-04-01 18:26:22
On Fri, Apr 01, 2011 at 10:47:56AM -0700, Andres Salomon wrote:
On Fri, 1 Apr 2011 13:20:31 +0200
Samuel Ortiz [off-list ref] wrote:
quoted
Hi Grant,
On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
[...]
quoted
quoted
Gah. Not all devices instantiated via mfd will be an mfd device,
which means that the driver may very well expect an *entirely
different* platform_device pointer; which further means a very high
potential of incorrectly dereferenced structures (as evidenced by a
patch series that is not bisectable). For instance, the xilinx ip
cores are used by more than just mfd.
I agree. Since the vast majority of the MFD subdevices are MFD
specific IPs, I overlooked that part. The impacted drivers are the
timberdale and the DaVinci voice codec ones.
Can you please provide pointers to what you're referring to? The only
code that I could find that created platform devices prefixed with
'timb-' or named 'xilinx_spi' was drivers/mfd/timberdale.c.
The xilinx-spi, ocores-i2c, i2c-xiic drivers and to some extend the
ks8842 ethernet driver are generic IPs that the timberdale SOC happens to
use. So I agree it's extremely unlikely that anyone could come up with a
platform that would be re-using e.g. the timb-radio IP, but I think it's less
unikely for more generic IPs such as the xilinx-spi one.
quoted
To fix that problem I propose 2 alternatives:
1) When declaring the sub devices cells, the MFD driver should
specify an mfd_data_size value for sub devices that are not MFD
specific. It's the MFD driver responsibility to set the cell
properly, and the non MFD specific drivers are kept MFD agnostic.
See my patch below for the timberdale case.
2) Revert the mfd_get_data() call for getting sub devices platform
data pointers. That was introduced to ease the MFD cell sharing work,
so if we take this route we'll need the cs5535 MFD driver to pass its
cells as platform_data pointer. Andres, can you confirm that this
would be fine for the mfd_clone_cell() routine to keep working ?
It would break mfd_clone_cell, as it uses mfd_get_cell to grab the one
to clone. We could change it to accept the cell as an argument. It
would also break mfd_cell_enable/disable, of course.
I'm talking about reverting the default behaviour of passing the MFD cell as
the platform data, and going back to the cell definitions setting their
platform_data pointer explicitely. In that case, the cs5535 driver would have
to do something like:
{
.name = "cs5535-acpi",
.num_resources = 1,
.resources = &cs5535_mfd_resources[ACPI_BAR],
+ .platform_data = &cs5535_mfd_cells[ACPI_BAR],
.enable = cs5535_mfd_res_enable,
.disable = cs5535_mfd_res_disable,
mfd_get_cell would then return &cs5535_mfd_cells[ACPI_BAR].
This fix would put all sub devices drivers back to an MFD agnostic state,
although the vast majority of them will certainly never be found anywhere else
than in their current MFD SoC. That's why I'm still not sure which way to go
to fix that problem.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
From: Samuel Ortiz <hidden> Date: 2011-04-01 23:52:51
On Fri, Apr 01, 2011 at 11:56:35AM -0600, Grant Likely wrote:
On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon [off-list ref] wrote:
quoted
On Fri, 1 Apr 2011 13:20:31 +0200
Samuel Ortiz [off-list ref] wrote:
quoted
Hi Grant,
On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
[...]
quoted
quoted
Gah. Not all devices instantiated via mfd will be an mfd device,
which means that the driver may very well expect an *entirely
different* platform_device pointer; which further means a very high
potential of incorrectly dereferenced structures (as evidenced by a
patch series that is not bisectable). For instance, the xilinx ip
cores are used by more than just mfd.
I agree. Since the vast majority of the MFD subdevices are MFD
specific IPs, I overlooked that part. The impacted drivers are the
timberdale and the DaVinci voice codec ones.
Another option is you could do this for MFD devices:
struct mfd_device {
struct platform_devce pdev;
struct mfd_cell *cell;
};
However, that requires that drivers using the mfd_cell will *never*
get instantiated outside of the mfd infrastructure, and there is no
way to protect against this so it is probably a bad idea.
Or, mfd_cell could be added to platform_device directly which would
*by far* be the safest option at the cost of every platform_device
having a mostly unused mfd_cell pointer. Not a significant cost in my
opinion.
I thought about this one, but I had the impression people would want to kill
me for adding an MFD specific pointer to platform_device. I guess it's worth
giving it a try since it would be a simple and safe solution.
I'll look at it later this weekend.
Thanks for the input.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
From: Grant Likely <hidden> Date: 2011-04-01 23:59:08
On Fri, Apr 1, 2011 at 5:52 PM, Samuel Ortiz [off-list ref] wrote:
On Fri, Apr 01, 2011 at 11:56:35AM -0600, Grant Likely wrote:
quoted
On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon [off-list ref] wrote:
quoted
On Fri, 1 Apr 2011 13:20:31 +0200
Samuel Ortiz [off-list ref] wrote:
quoted
Hi Grant,
On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
[...]
quoted
quoted
Gah. Not all devices instantiated via mfd will be an mfd device,
which means that the driver may very well expect an *entirely
different* platform_device pointer; which further means a very high
potential of incorrectly dereferenced structures (as evidenced by a
patch series that is not bisectable). For instance, the xilinx ip
cores are used by more than just mfd.
I agree. Since the vast majority of the MFD subdevices are MFD
specific IPs, I overlooked that part. The impacted drivers are the
timberdale and the DaVinci voice codec ones.
Another option is you could do this for MFD devices:
struct mfd_device {
struct platform_devce pdev;
struct mfd_cell *cell;
};
However, that requires that drivers using the mfd_cell will *never*
get instantiated outside of the mfd infrastructure, and there is no
way to protect against this so it is probably a bad idea.
Or, mfd_cell could be added to platform_device directly which would
*by far* be the safest option at the cost of every platform_device
having a mostly unused mfd_cell pointer. Not a significant cost in my
opinion.
I thought about this one, but I had the impression people would want to kill
me for adding an MFD specific pointer to platform_device. I guess it's worth
giving it a try since it would be a simple and safe solution.
I'll look at it later this weekend.
Thanks for the input.
[cc'ing gregkh because we're talking about modifying struct platform_device]
I'll back you up on this one. It is a far better solution than the
alternatives. At least with mfd, it covers a large set of devices. I
think there is a strong argument for doing this. Or alternatively,
the particular interesting fields from mfd_cell could be added to
platform_device. What information do child devices need access to?
g.
From: Andres Salomon <hidden> Date: 2011-04-02 00:10:17
On Fri, 1 Apr 2011 17:58:44 -0600
Grant Likely [off-list ref] wrote:
On Fri, Apr 1, 2011 at 5:52 PM, Samuel Ortiz [off-list ref]
wrote:
quoted
On Fri, Apr 01, 2011 at 11:56:35AM -0600, Grant Likely wrote:
quoted
On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon
[off-list ref] wrote:
quoted
On Fri, 1 Apr 2011 13:20:31 +0200
Samuel Ortiz [off-list ref] wrote:
quoted
Hi Grant,
On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
[...]
quoted
quoted
Gah. Not all devices instantiated via mfd will be an mfd
device, which means that the driver may very well expect an
*entirely different* platform_device pointer; which further
means a very high potential of incorrectly dereferenced
structures (as evidenced by a patch series that is not
bisectable). For instance, the xilinx ip cores are used by
more than just mfd.
I agree. Since the vast majority of the MFD subdevices are MFD
specific IPs, I overlooked that part. The impacted drivers are
the timberdale and the DaVinci voice codec ones.
Another option is you could do this for MFD devices:
struct mfd_device {
struct platform_devce pdev;
struct mfd_cell *cell;
};
However, that requires that drivers using the mfd_cell will *never*
get instantiated outside of the mfd infrastructure, and there is no
way to protect against this so it is probably a bad idea.
Or, mfd_cell could be added to platform_device directly which would
*by far* be the safest option at the cost of every platform_device
having a mostly unused mfd_cell pointer. Not a significant cost
in my opinion.
I thought about this one, but I had the impression people would
want to kill me for adding an MFD specific pointer to
platform_device. I guess it's worth giving it a try since it would
be a simple and safe solution. I'll look at it later this weekend.
Thanks for the input.
[cc'ing gregkh because we're talking about modifying struct
platform_device]
I'll back you up on this one. It is a far better solution than the
alternatives. At least with mfd, it covers a large set of devices. I
think there is a strong argument for doing this. Or alternatively,
the particular interesting fields from mfd_cell could be added to
platform_device. What information do child devices need access to?
This was one of the things I was originally tempted to do (adding
mfd fields to platform_device). I didn't think it would fly.
I can look at this stuff or help out once I have a stable internet
connection and I'm all moved in to my new place (which should be
Wednesday).
From: Samuel Ortiz <hidden> Date: 2011-04-04 10:03:23
On Fri, Apr 01, 2011 at 05:58:44PM -0600, Grant Likely wrote:
On Fri, Apr 1, 2011 at 5:52 PM, Samuel Ortiz [off-list ref] wrote:
quoted
On Fri, Apr 01, 2011 at 11:56:35AM -0600, Grant Likely wrote:
quoted
On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon [off-list ref] wrote:
quoted
On Fri, 1 Apr 2011 13:20:31 +0200
Samuel Ortiz [off-list ref] wrote:
quoted
Hi Grant,
On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
[...]
quoted
quoted
Gah. Not all devices instantiated via mfd will be an mfd device,
which means that the driver may very well expect an *entirely
different* platform_device pointer; which further means a very high
potential of incorrectly dereferenced structures (as evidenced by a
patch series that is not bisectable). For instance, the xilinx ip
cores are used by more than just mfd.
I agree. Since the vast majority of the MFD subdevices are MFD
specific IPs, I overlooked that part. The impacted drivers are the
timberdale and the DaVinci voice codec ones.
Another option is you could do this for MFD devices:
struct mfd_device {
struct platform_devce pdev;
struct mfd_cell *cell;
};
However, that requires that drivers using the mfd_cell will *never*
get instantiated outside of the mfd infrastructure, and there is no
way to protect against this so it is probably a bad idea.
Or, mfd_cell could be added to platform_device directly which would
*by far* be the safest option at the cost of every platform_device
having a mostly unused mfd_cell pointer. Not a significant cost in my
opinion.
I thought about this one, but I had the impression people would want to kill
me for adding an MFD specific pointer to platform_device. I guess it's worth
giving it a try since it would be a simple and safe solution.
I'll look at it later this weekend.
Thanks for the input.
[cc'ing gregkh because we're talking about modifying struct platform_device]
I'll back you up on this one. It is a far better solution than the
alternatives. At least with mfd, it covers a large set of devices. I
think there is a strong argument for doing this. Or alternatively,
the particular interesting fields from mfd_cell could be added to
platform_device. What information do child devices need access to?
In some cases, they need the whole cell to clone it. So I'm up for adding an
mfd_cell pointer to the platform_device structure.
Below is a tentative patch. This is a first step and would fix all
regressions. I tried to keep the MFD dependencies as small as possible, which
is why I placed the pdev->mfd_cell building code in mfd-core.c
The second step would be to get rid of mfd_get_data() and have all subdrivers
going back to the regular platform_data way. They would no longer be dependant
on the MFD code except for those who really need it. In that case they could
just call mfd_get_cell() and get full access to their MFD cell.
---
drivers/mfd/mfd-core.c | 27 ++++++++++++++++++++++-----
include/linux/mfd/core.h | 7 +++++--
include/linux/platform_device.h | 5 +++++
3 files changed, 32 insertions(+), 7 deletions(-)
From: Grant Likely <hidden> Date: 2011-04-05 03:04:40
On Mon, Apr 04, 2011 at 12:03:15PM +0200, Samuel Ortiz wrote:
On Fri, Apr 01, 2011 at 05:58:44PM -0600, Grant Likely wrote:
quoted
On Fri, Apr 1, 2011 at 5:52 PM, Samuel Ortiz [off-list ref] wrote:
quoted
On Fri, Apr 01, 2011 at 11:56:35AM -0600, Grant Likely wrote:
quoted
On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon [off-list ref] wrote:
quoted
On Fri, 1 Apr 2011 13:20:31 +0200
Samuel Ortiz [off-list ref] wrote:
quoted
Hi Grant,
On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
[...]
quoted
quoted
Gah. Not all devices instantiated via mfd will be an mfd device,
which means that the driver may very well expect an *entirely
different* platform_device pointer; which further means a very high
potential of incorrectly dereferenced structures (as evidenced by a
patch series that is not bisectable). For instance, the xilinx ip
cores are used by more than just mfd.
I agree. Since the vast majority of the MFD subdevices are MFD
specific IPs, I overlooked that part. The impacted drivers are the
timberdale and the DaVinci voice codec ones.
Another option is you could do this for MFD devices:
struct mfd_device {
struct platform_devce pdev;
struct mfd_cell *cell;
};
However, that requires that drivers using the mfd_cell will *never*
get instantiated outside of the mfd infrastructure, and there is no
way to protect against this so it is probably a bad idea.
Or, mfd_cell could be added to platform_device directly which would
*by far* be the safest option at the cost of every platform_device
having a mostly unused mfd_cell pointer. Not a significant cost in my
opinion.
I thought about this one, but I had the impression people would want to kill
me for adding an MFD specific pointer to platform_device. I guess it's worth
giving it a try since it would be a simple and safe solution.
I'll look at it later this weekend.
Thanks for the input.
[cc'ing gregkh because we're talking about modifying struct platform_device]
I'll back you up on this one. It is a far better solution than the
alternatives. At least with mfd, it covers a large set of devices. I
think there is a strong argument for doing this. Or alternatively,
the particular interesting fields from mfd_cell could be added to
platform_device. What information do child devices need access to?
In some cases, they need the whole cell to clone it. So I'm up for adding an
mfd_cell pointer to the platform_device structure.
Below is a tentative patch. This is a first step and would fix all
regressions. I tried to keep the MFD dependencies as small as possible, which
is why I placed the pdev->mfd_cell building code in mfd-core.c
Okay.
The second step would be to get rid of mfd_get_data() and have all subdrivers
going back to the regular platform_data way. They would no longer be dependant
on the MFD code except for those who really need it. In that case they could
just call mfd_get_cell() and get full access to their MFD cell.
The revert to platform_data needs to happen ASAP though. If this
second step isn't ready really quickly, then the current patches
should be reverted to give some breathing room for creating the
replacement patches. However, it's not such a rush if the below
patch really does eliminate all of the nastiness of the original
series. (I haven't looked and a rolled up diff of the first series and
this change, so I don't know for sure).
In principle I agree with this patch. Some comments below.
g.
'sizeof(*cell) is a teensy bit safer. Also, this can be more concise:
static int mfd_platform_add_cell(struct platform_device *pdev,
const struct mfd_cell *cell)
{
if (!cell)
return 0;
pdev->mfd_cell = kmemdup(cell, sizeof(*cell), GFP_KERNEL);
return pdev->mfd_cell ? 0 : -ENOMEM;
}
@@ -75,7 +90,7 @@ static int mfd_add_device(struct device *parent, int id, pdev->dev.parent = parent;- ret = platform_device_add_data(pdev, cell, sizeof(*cell));+ ret = mfd_platform_add_cell(pdev, cell); if (ret) goto fail_res;
@@ -104,17 +119,17 @@ static int mfd_add_device(struct device *parent, int id, if (!cell->ignore_resource_conflicts) { ret = acpi_check_resource_conflict(res); if (ret)- goto fail_res;+ goto fail_cell; } } ret = platform_device_add_resources(pdev, res, cell->num_resources); if (ret)- goto fail_res;+ goto fail_cell; ret = platform_device_add(pdev); if (ret)- goto fail_res;+ goto fail_cell; if (cell->pm_runtime_no_callbacks) pm_runtime_no_callbacks(&pdev->dev);
@@ -123,7 +138,8 @@ static int mfd_add_device(struct device *parent, int id, return 0;-/* platform_device_del(pdev); */+fail_cell:+ kfree(pdev->mfd_cell);
Looks like kfreeing the cell should become part of the
platform_device_release() function. Which would remove it from here,
and also ...
From: Samuel Ortiz <hidden> Date: 2011-04-06 15:23:30
On Mon, Apr 04, 2011 at 09:04:29PM -0600, Grant Likely wrote:
quoted
The second step would be to get rid of mfd_get_data() and have all subdrivers
going back to the regular platform_data way. They would no longer be dependant
on the MFD code except for those who really need it. In that case they could
just call mfd_get_cell() and get full access to their MFD cell.
The revert to platform_data needs to happen ASAP though. If this
second step isn't ready really quickly, then the current patches
should be reverted to give some breathing room for creating the
replacement patches. However, it's not such a rush if the below
patch really does eliminate all of the nastiness of the original
series. (I haven't looked and a rolled up diff of the first series and
this change, so I don't know for sure).
I am done reverting these changes, with a final patch getting rid of
mfd_get_data. See
git://git.kernel.org/pub/scm/linux/kernel/git/sameo/mfd-2.6.git for-linus
I still need to give it a second review before pushing it to lkml for
comments. It's 20 patches long, so I'm not entirely sure Linus would take that
at that point.
Pushing patch #1 would be enough for fixing the issues introduced by the
original patchset, so I'm leaning toward pushing it and leaving the 19 other
patches for the next merge window.
In principle I agree with this patch. Some comments below.
Thanks for the comments. I think I addressed all of them in patch #1:
---
drivers/base/platform.c | 1 +
drivers/mfd/mfd-core.c | 15 +++++++++++++--
include/linux/device.h | 3 +++
include/linux/mfd/core.h | 7 +++++--
4 files changed, 22 insertions(+), 4 deletions(-)
What is a "MFD cell pointer" and why is it needed in struct device?
An MFD cell is an MFD instantiated device.
MFD (Multi Function Device) drivers instantiate platform devices. Those
devices drivers sometimes need a platform data pointer, sometimes an MFD
specific pointer, and sometimes both. Also, some of those drivers have been
implemented as MFD sub drivers, while others know nothing about MFD and just
expect a plain platform_data pointer.
We've been faced with the problem of being able to pass both MFD related data
and a platform_data pointer to some of those drivers. Squeezing the MFD bits
in the sub driver platform_data pointer doesn't work for drivers that know
nothing about MFDs. It also adds an additional dependency on the MFD API to
all MFD sub drivers. That prevents any of those drivers to eventually be used
as plain platform device drivers.
So, adding an MFD cell pointer to the device structure allows us to cleanly
pass both pieces of information, while keeping all the MFD sub drivers
independant from the MFD core if they want/can.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
What is a "MFD cell pointer" and why is it needed in struct device?
An MFD cell is an MFD instantiated device.
MFD (Multi Function Device) drivers instantiate platform devices. Those
devices drivers sometimes need a platform data pointer, sometimes an MFD
specific pointer, and sometimes both. Also, some of those drivers have been
implemented as MFD sub drivers, while others know nothing about MFD and just
expect a plain platform_data pointer.
We've been faced with the problem of being able to pass both MFD related data
and a platform_data pointer to some of those drivers. Squeezing the MFD bits
in the sub driver platform_data pointer doesn't work for drivers that know
nothing about MFDs. It also adds an additional dependency on the MFD API to
all MFD sub drivers. That prevents any of those drivers to eventually be used
as plain platform device drivers.
So, adding an MFD cell pointer to the device structure allows us to cleanly
pass both pieces of information, while keeping all the MFD sub drivers
independant from the MFD core if they want/can.
Why isn't an MFD the parent of its component devices?
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
From: Samuel Ortiz <hidden> Date: 2011-04-06 17:51:24
Hi Ben,
On Wed, Apr 06, 2011 at 06:16:49PM +0100, Ben Hutchings wrote:
quoted
So, adding an MFD cell pointer to the device structure allows us to cleanly
pass both pieces of information, while keeping all the MFD sub drivers
independant from the MFD core if they want/can.
Why isn't an MFD the parent of its component devices?
It actually is. How would that help here ?
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
What is a "MFD cell pointer" and why is it needed in struct device?
An MFD cell is an MFD instantiated device.
MFD (Multi Function Device) drivers instantiate platform devices. Those
devices drivers sometimes need a platform data pointer, sometimes an MFD
specific pointer, and sometimes both. Also, some of those drivers have been
implemented as MFD sub drivers, while others know nothing about MFD and just
expect a plain platform_data pointer.
That sounds like a bug in those drivers, why not fix them to properly
pass in the correct pointer?
We've been faced with the problem of being able to pass both MFD related data
and a platform_data pointer to some of those drivers. Squeezing the MFD bits
in the sub driver platform_data pointer doesn't work for drivers that know
nothing about MFDs. It also adds an additional dependency on the MFD API to
all MFD sub drivers. That prevents any of those drivers to eventually be used
as plain platform device drivers.
Then they shouldn't be "plain" platform drivers, that should only be
reserved for drivers that are the "lowest" type. Just make them MFD
devices and go from there.
So, adding an MFD cell pointer to the device structure allows us to cleanly
pass both pieces of information, while keeping all the MFD sub drivers
independant from the MFD core if they want/can.
They shouldn't be "independant", make them "dependant" and go from
there.
thanks,
greg k-h
From: Ben Hutchings <hidden> Date: 2011-04-06 18:07:15
On Wed, 2011-04-06 at 19:51 +0200, Samuel Ortiz wrote:
Hi Ben,
On Wed, Apr 06, 2011 at 06:16:49PM +0100, Ben Hutchings wrote:
quoted
quoted
So, adding an MFD cell pointer to the device structure allows us to cleanly
pass both pieces of information, while keeping all the MFD sub drivers
independant from the MFD core if they want/can.
Why isn't an MFD the parent of its component devices?
It actually is. How would that help here ?
I was thinking you could encode the component address in the
platform_device name (just as the bus address is the name of a normal
bus device). That plus the parent device pointer would be sufficient
information to look up the mfd_cell.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
What is a "MFD cell pointer" and why is it needed in struct
device?
An MFD cell is an MFD instantiated device.
MFD (Multi Function Device) drivers instantiate platform devices.
Those devices drivers sometimes need a platform data pointer,
sometimes an MFD specific pointer, and sometimes both. Also, some
of those drivers have been implemented as MFD sub drivers, while
others know nothing about MFD and just expect a plain platform_data
pointer.
That sounds like a bug in those drivers, why not fix them to properly
pass in the correct pointer?
I'm still trying to understand if this is a theoretical problem, or if
Grant has actually experienced a regression. His mention of bisecting
made it sound like the latter was the case, but I've yet to hear of
specifically what drivers this was breaking. Samuel described some
potential driver breakage, but nothing concrete.
I do agree that this needs a better solution, given the theoretical
breakage.
quoted
We've been faced with the problem of being able to pass both MFD
related data and a platform_data pointer to some of those drivers.
Squeezing the MFD bits in the sub driver platform_data pointer
doesn't work for drivers that know nothing about MFDs. It also adds
an additional dependency on the MFD API to all MFD sub drivers.
That prevents any of those drivers to eventually be used as plain
platform device drivers.
Then they shouldn't be "plain" platform drivers, that should only be
reserved for drivers that are the "lowest" type. Just make them MFD
devices and go from there.
The problem is of mixing "plain" platform devices and MFD devices.
It's reasonable to assume that different hardware may be using
one method or the other to create devices; in order to maintain
compatibility with the driver, one either needs to use a plain platform
device. Alternatively, if an MFD-specific device class is created,
then MFD devices would start showing up in weird places.
quoted
So, adding an MFD cell pointer to the device structure allows us to
cleanly pass both pieces of information, while keeping all the MFD
sub drivers independant from the MFD core if they want/can.
They shouldn't be "independant", make them "dependant" and go from
there.
thanks,
greg k-h
On Wed, Apr 06, 2011 at 11:25:57AM -0700, Andres Salomon wrote:
quoted
quoted
We've been faced with the problem of being able to pass both MFD
related data and a platform_data pointer to some of those drivers.
Squeezing the MFD bits in the sub driver platform_data pointer
doesn't work for drivers that know nothing about MFDs. It also adds
an additional dependency on the MFD API to all MFD sub drivers.
That prevents any of those drivers to eventually be used as plain
platform device drivers.
Then they shouldn't be "plain" platform drivers, that should only be
reserved for drivers that are the "lowest" type. Just make them MFD
devices and go from there.
The problem is of mixing "plain" platform devices and MFD devices.
Then don't do that.
It's reasonable to assume that different hardware may be using
one method or the other to create devices; in order to maintain
compatibility with the driver, one either needs to use a plain platform
device. Alternatively, if an MFD-specific device class is created,
then MFD devices would start showing up in weird places.
Then fix it. Lots of other drivers handle different "bus types" just
fine (look at the EHCI USB driver for an example.) Don't polute the
driver core just because you don't want to fix up the individual driver
issues that are quite obvious.
thanks,
greg k-h
What is a "MFD cell pointer" and why is it needed in struct device?
An MFD cell is an MFD instantiated device.
MFD (Multi Function Device) drivers instantiate platform devices. Those
devices drivers sometimes need a platform data pointer, sometimes an MFD
specific pointer, and sometimes both. Also, some of those drivers have been
implemented as MFD sub drivers, while others know nothing about MFD and just
expect a plain platform_data pointer.
That sounds like a bug in those drivers, why not fix them to properly
pass in the correct pointer?
Because they're drivers for generic IPs, not MFD ones. By forcing them to use
MFD specific structure and APIs, we make it more difficult for platform code
to instantiate them.
The timberdale MFD for example is built with a Xilinx SPI controller, and a
Micrel ks8842 ethernet switch IP. Forcing those devices into being MFD devices
would mean any platform willing to instantiate them would have to use the MFD
APIs. That sounds a bit artificial to me.
Although there is currently no drivers instantiated by both an MFD driver
and some platform code, Grant complaint about the Xilinx SPI driver moving
from a platform driver to an MFD one makes sense to me.
quoted
So, adding an MFD cell pointer to the device structure allows us to cleanly
pass both pieces of information, while keeping all the MFD sub drivers
independant from the MFD core if they want/can.
They shouldn't be "independant",
Excuse my poor spelling.
make them "dependant" and go from there.
That's what the code currently does.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
From: Felipe Balbi <hidden> Date: 2011-04-06 18:59:15
Hi,
On Wed, Apr 06, 2011 at 08:47:34PM +0200, Samuel Ortiz wrote:
quoted
quoted
quoted
What is a "MFD cell pointer" and why is it needed in struct device?
An MFD cell is an MFD instantiated device.
MFD (Multi Function Device) drivers instantiate platform devices. Those
devices drivers sometimes need a platform data pointer, sometimes an MFD
specific pointer, and sometimes both. Also, some of those drivers have been
implemented as MFD sub drivers, while others know nothing about MFD and just
expect a plain platform_data pointer.
That sounds like a bug in those drivers, why not fix them to properly
pass in the correct pointer?
Because they're drivers for generic IPs, not MFD ones. By forcing them to use
MFD specific structure and APIs, we make it more difficult for platform code
to instantiate them.
I agree. What I do on those cases is to have a simple platform_device
for the core IP driver and use platform_device_id tables to do runtime
checks of the small differences. If one platform X doesn't use a
platform_bus, it uses e.g. PCI, then you make a PCI "bridge" which
allocates a platform_device with the correct name and adds that to the
driver model.
See [1] (for the core driver) and [2] (for a PCI bridge driver) for an
example of what I'm talking about.
The timberdale MFD for example is built with a Xilinx SPI controller, and a
Micrel ks8842 ethernet switch IP. Forcing those devices into being MFD devices
would mean any platform willing to instantiate them would have to use the MFD
APIs. That sounds a bit artificial to me.
do they share any address space ? If they do, then you'd need something
to synchronize, right ? If they don't, then you just add two separate
devices, they don't have to be MFD.
Although there is currently no drivers instantiated by both an MFD driver
and some platform code, Grant complaint about the Xilinx SPI driver moving
from a platform driver to an MFD one makes sense to me.
On Wed, Apr 06, 2011 at 09:59:02PM +0300, Felipe Balbi wrote:
Hi,
On Wed, Apr 06, 2011 at 08:47:34PM +0200, Samuel Ortiz wrote:
quoted
quoted
quoted
quoted
What is a "MFD cell pointer" and why is it needed in struct device?
An MFD cell is an MFD instantiated device.
MFD (Multi Function Device) drivers instantiate platform devices. Those
devices drivers sometimes need a platform data pointer, sometimes an MFD
specific pointer, and sometimes both. Also, some of those drivers have been
implemented as MFD sub drivers, while others know nothing about MFD and just
expect a plain platform_data pointer.
That sounds like a bug in those drivers, why not fix them to properly
pass in the correct pointer?
Because they're drivers for generic IPs, not MFD ones. By forcing them to use
MFD specific structure and APIs, we make it more difficult for platform code
to instantiate them.
I agree. What I do on those cases is to have a simple platform_device
for the core IP driver and use platform_device_id tables to do runtime
checks of the small differences. If one platform X doesn't use a
platform_bus, it uses e.g. PCI, then you make a PCI "bridge" which
allocates a platform_device with the correct name and adds that to the
driver model.
See [1] (for the core driver) and [2] (for a PCI bridge driver) for an
example of what I'm talking about.
Yes, thanks for providing a real example, this is the best way to handle
this.
thanks,
greg k-h
From: Grant Likely <hidden> Date: 2011-04-07 08:04:22
On Wed, Apr 06, 2011 at 11:38:54AM -0700, Greg KH wrote:
On Wed, Apr 06, 2011 at 11:25:57AM -0700, Andres Salomon wrote:
quoted
quoted
quoted
We've been faced with the problem of being able to pass both MFD
related data and a platform_data pointer to some of those drivers.
Squeezing the MFD bits in the sub driver platform_data pointer
doesn't work for drivers that know nothing about MFDs. It also adds
an additional dependency on the MFD API to all MFD sub drivers.
That prevents any of those drivers to eventually be used as plain
platform device drivers.
Then they shouldn't be "plain" platform drivers, that should only be
reserved for drivers that are the "lowest" type. Just make them MFD
devices and go from there.
The problem is of mixing "plain" platform devices and MFD devices.
Then don't do that.
From my perspective, MFD devices are little more than a bag of
platform_devices, with the MFD layer provides infrastructure for
managing it. It isn't that there are 'plain' platform device and
'mfd' devices. There are only platform_devices, but some of the
drivers use additional data stored in a struct mfd.
Personally, I'm not thrilled with the approach of using struct mfd, or
more specifically making it available to drivers, but on the ugly
scale it isn't very high.
However, the changes on how struct mfd is passed that were merged in
2.6.39 were actively dangerous and are going to be reverted. Yet
a method is still needed to pass the struct mfd in a safe way. I
don't have a problem with adding the mfd pointer to struct
platform_device, even if it should just be a stop gap to something
better.
Independently, I have been experimenting with typesafe methods for
attaching data to devices which may very well be the long term
approach, but for the short term I see no problem with adding the mfd
pointer, particularly because it is by far safer than any of the other
immediately available options.
g.
From: Felipe Balbi <hidden> Date: 2011-04-07 08:09:32
Hi,
On Wed, Apr 06, 2011 at 03:09:00PM -0700, Greg KH wrote:
On Wed, Apr 06, 2011 at 09:59:02PM +0300, Felipe Balbi wrote:
quoted
Hi,
On Wed, Apr 06, 2011 at 08:47:34PM +0200, Samuel Ortiz wrote:
quoted
quoted
quoted
quoted
What is a "MFD cell pointer" and why is it needed in struct device?
An MFD cell is an MFD instantiated device.
MFD (Multi Function Device) drivers instantiate platform devices. Those
devices drivers sometimes need a platform data pointer, sometimes an MFD
specific pointer, and sometimes both. Also, some of those drivers have been
implemented as MFD sub drivers, while others know nothing about MFD and just
expect a plain platform_data pointer.
That sounds like a bug in those drivers, why not fix them to properly
pass in the correct pointer?
Because they're drivers for generic IPs, not MFD ones. By forcing them to use
MFD specific structure and APIs, we make it more difficult for platform code
to instantiate them.
I agree. What I do on those cases is to have a simple platform_device
for the core IP driver and use platform_device_id tables to do runtime
checks of the small differences. If one platform X doesn't use a
platform_bus, it uses e.g. PCI, then you make a PCI "bridge" which
allocates a platform_device with the correct name and adds that to the
driver model.
See [1] (for the core driver) and [2] (for a PCI bridge driver) for an
example of what I'm talking about.
Yes, thanks for providing a real example, this is the best way to handle
this.
no problem.
ps: that's the driver for the USB3 controller which will come on OMAP5.
Driver being validate on a pre-silicon platform right now :-D In a few
weeks I'll send the driver for integration.
--
balbi
From: Samuel Ortiz <hidden> Date: 2011-04-07 13:40:36
Hi Felipe,
On Wed, Apr 06, 2011 at 09:59:02PM +0300, Felipe Balbi wrote:
Hi,
On Wed, Apr 06, 2011 at 08:47:34PM +0200, Samuel Ortiz wrote:
quoted
quoted
quoted
quoted
What is a "MFD cell pointer" and why is it needed in struct device?
An MFD cell is an MFD instantiated device.
MFD (Multi Function Device) drivers instantiate platform devices. Those
devices drivers sometimes need a platform data pointer, sometimes an MFD
specific pointer, and sometimes both. Also, some of those drivers have been
implemented as MFD sub drivers, while others know nothing about MFD and just
expect a plain platform_data pointer.
That sounds like a bug in those drivers, why not fix them to properly
pass in the correct pointer?
Because they're drivers for generic IPs, not MFD ones. By forcing them to use
MFD specific structure and APIs, we make it more difficult for platform code
to instantiate them.
I agree. What I do on those cases is to have a simple platform_device
for the core IP driver and use platform_device_id tables to do runtime
checks of the small differences. If one platform X doesn't use a
platform_bus, it uses e.g. PCI, then you make a PCI "bridge" which
allocates a platform_device with the correct name and adds that to the
driver model.
I see, thanks.
Below is a patch for the Xilinx SPI example. Although this would fix the
issue, we'd still have to do that on device per device basis. I had a similar
solution where MFD drivers would set a flag for sub drivers that don't need
any of the MFD bits. In that case the MFD core code would just forward the
platform data, instead of embedding it through an MFD cell.
Cheers,
Samuel.
---
drivers/mfd/timberdale.c | 8 ++++----
drivers/spi/xilinx_spi.c | 19 ++++++++++++++++++-
include/linux/mfd/core.h | 3 +++
3 files changed, 25 insertions(+), 5 deletions(-)
@@ -471,7 +471,11 @@ static int __devinit xilinx_spi_probe(struct platform_device *dev)structspi_master*master;u8i;-pdata=mfd_get_data(dev);+if(platform_get_device_id(dev)&&+platform_get_device_id(dev)->driver_data&MFD_PLATFORM_DEVICE)+pdata=mfd_get_data(dev);+else+pdata=dev->dev.platform_data;if(pdata){num_cs=pdata->num_chipselect;little_endian=pdata->little_endian;
@@ -530,6 +534,18 @@ static int __devexit xilinx_spi_remove(struct platform_device *dev)/* work with hotplug and coldplug */MODULE_ALIAS("platform:"XILINX_SPI_NAME);+staticconststructplatform_device_idxilinx_spi_id_table[]={+{+.name=XILINX_SPI_NAME,+},+{+.name="mfd_xilinx_spi",+.driver_data=MFD_PLATFORM_DEVICE,+},+{},/* Terminating Entry */+};+MODULE_DEVICE_TABLE(platform,xilinx_spi_id_table);+staticstructplatform_driverxilinx_spi_driver={.probe=xilinx_spi_probe,.remove=__devexit_p(xilinx_spi_remove),
From: Grant Likely <hidden> Date: 2011-04-07 14:35:30
On Thu, Apr 07, 2011 at 03:40:23PM +0200, Samuel Ortiz wrote:
Hi Felipe,
On Wed, Apr 06, 2011 at 09:59:02PM +0300, Felipe Balbi wrote:
quoted
Hi,
On Wed, Apr 06, 2011 at 08:47:34PM +0200, Samuel Ortiz wrote:
quoted
quoted
quoted
quoted
What is a "MFD cell pointer" and why is it needed in struct device?
An MFD cell is an MFD instantiated device.
MFD (Multi Function Device) drivers instantiate platform devices. Those
devices drivers sometimes need a platform data pointer, sometimes an MFD
specific pointer, and sometimes both. Also, some of those drivers have been
implemented as MFD sub drivers, while others know nothing about MFD and just
expect a plain platform_data pointer.
That sounds like a bug in those drivers, why not fix them to properly
pass in the correct pointer?
Because they're drivers for generic IPs, not MFD ones. By forcing them to use
MFD specific structure and APIs, we make it more difficult for platform code
to instantiate them.
I agree. What I do on those cases is to have a simple platform_device
for the core IP driver and use platform_device_id tables to do runtime
checks of the small differences. If one platform X doesn't use a
platform_bus, it uses e.g. PCI, then you make a PCI "bridge" which
allocates a platform_device with the correct name and adds that to the
driver model.
I see, thanks.
Below is a patch for the Xilinx SPI example. Although this would fix the
issue, we'd still have to do that on device per device basis. I had a similar
solution where MFD drivers would set a flag for sub drivers that don't need
any of the MFD bits. In that case the MFD core code would just forward the
platform data, instead of embedding it through an MFD cell.
platform_data is already a fiddly bit where you don't know what
structure type platform_data points at; it is implicitly known and
easy to get wrong. This solution makes me *very* nervous
because it would become even easier to get a mismatch on the
platform_data pointer type.
g.
@@ -471,7 +471,11 @@ static int __devinit xilinx_spi_probe(struct platform_device *dev)structspi_master*master;u8i;-pdata=mfd_get_data(dev);+if(platform_get_device_id(dev)&&+platform_get_device_id(dev)->driver_data&MFD_PLATFORM_DEVICE)+pdata=mfd_get_data(dev);+else+pdata=dev->dev.platform_data;if(pdata){num_cs=pdata->num_chipselect;little_endian=pdata->little_endian;
@@ -530,6 +534,18 @@ static int __devexit xilinx_spi_remove(struct platform_device *dev)/* work with hotplug and coldplug */MODULE_ALIAS("platform:"XILINX_SPI_NAME);+staticconststructplatform_device_idxilinx_spi_id_table[]={+{+.name=XILINX_SPI_NAME,+},+{+.name="mfd_xilinx_spi",+.driver_data=MFD_PLATFORM_DEVICE,+},+{},/* Terminating Entry */+};+MODULE_DEVICE_TABLE(platform,xilinx_spi_id_table);+staticstructplatform_driverxilinx_spi_driver={.probe=xilinx_spi_probe,.remove=__devexit_p(xilinx_spi_remove),
From: Samuel Ortiz <hidden> Date: 2011-04-07 15:03:31
On Thu, Apr 07, 2011 at 07:35:15AM -0700, Grant Likely wrote:
quoted
Below is a patch for the Xilinx SPI example. Although this would fix the
issue, we'd still have to do that on device per device basis. I had a similar
solution where MFD drivers would set a flag for sub drivers that don't need
any of the MFD bits. In that case the MFD core code would just forward the
platform data, instead of embedding it through an MFD cell.
platform_data is already a fiddly bit where you don't know what
structure type platform_data points at; it is implicitly known and
easy to get wrong. This solution makes me *very* nervous
because it would become even easier to get a mismatch on the
platform_data pointer type.
How would that be more error prone than say a board file instantiating a
platform device after having set the platform_data pointer to point to an
implicitely know structure reference ?
Cheers,
Samuel.
P.S.: Would you be ok with something like the patch below ?
@@ -471,7 +471,11 @@ static int __devinit xilinx_spi_probe(struct platform_device *dev)structspi_master*master;u8i;-pdata=mfd_get_data(dev);+if(platform_get_device_id(dev)&&+platform_get_device_id(dev)->driver_data&MFD_PLATFORM_DEVICE)+pdata=mfd_get_data(dev);+else+pdata=dev->dev.platform_data;if(pdata){num_cs=pdata->num_chipselect;little_endian=pdata->little_endian;
@@ -530,6 +534,18 @@ static int __devexit xilinx_spi_remove(struct platform_device *dev)/* work with hotplug and coldplug */MODULE_ALIAS("platform:"XILINX_SPI_NAME);+staticconststructplatform_device_idxilinx_spi_id_table[]={+{+.name=XILINX_SPI_NAME,+},+{+.name="mfd_xilinx_spi",+.driver_data=MFD_PLATFORM_DEVICE,+},+{},/* Terminating Entry */+};+MODULE_DEVICE_TABLE(platform,xilinx_spi_id_table);+staticstructplatform_driverxilinx_spi_driver={.probe=xilinx_spi_probe,.remove=__devexit_p(xilinx_spi_remove),
Move this down to by the of_node pointer. May as well collect all the
supplemental data about the device in the same place.
So, okay. wow. I have *no* idea what I was smoking at this point in
time. The of_node pointer is in struct device which is definitely not
the place to put the mfd_cell pointer (and you probably though I was
crazy when I suggested it). Greg was totally right to complain about
moving it into struct device. Sorry for causing trouble.
Move it back into struct platform_device and you should be good. I
just talked to greg, and there should be any issues with locating it
there.
g.
From: Grant Likely <hidden> Date: 2011-04-07 18:06:58
On Thu, Apr 07, 2011 at 05:03:23PM +0200, Samuel Ortiz wrote:
On Thu, Apr 07, 2011 at 07:35:15AM -0700, Grant Likely wrote:
quoted
quoted
Below is a patch for the Xilinx SPI example. Although this would fix the
issue, we'd still have to do that on device per device basis. I had a similar
solution where MFD drivers would set a flag for sub drivers that don't need
any of the MFD bits. In that case the MFD core code would just forward the
platform data, instead of embedding it through an MFD cell.
platform_data is already a fiddly bit where you don't know what
structure type platform_data points at; it is implicitly known and
easy to get wrong. This solution makes me *very* nervous
because it would become even easier to get a mismatch on the
platform_data pointer type.
How would that be more error prone than say a board file instantiating a
platform device after having set the platform_data pointer to point to an
implicitely know structure reference ?
Yes, platform_data is already troublesome, but at least current
convention is a 1:1 relationship between driver and platform_data
type. I still hate it and want something better, but it is what we
have. The problem with what having a different platform_data pointer
depending on the instantiation means that it adds yet another level of
decision that needs to be made and is very easy to get wrong.
So, yes, platform_data is bad. I don't want to see it get any worse.
Cheers,
Samuel.
P.S.: Would you be ok with something like the patch below ?
Not really because it requires the driver to make the correct decision
about the platform_data type depending on the driver name. It is easy
to get wrong and the compiler cannot help you catch it.
I've talked with Greg, and adding the mfd_cell pointer to
platform_device will be okay in the short term. In the long term I'm
looking at creating a better way of attaching type-safe data to
devices that will pretty much eliminate this issue.
@@ -471,7 +471,11 @@ static int __devinit xilinx_spi_probe(struct platform_device *dev)structspi_master*master;u8i;-pdata=mfd_get_data(dev);+if(platform_get_device_id(dev)&&+platform_get_device_id(dev)->driver_data&MFD_PLATFORM_DEVICE)+pdata=mfd_get_data(dev);+else+pdata=dev->dev.platform_data;if(pdata){num_cs=pdata->num_chipselect;little_endian=pdata->little_endian;
@@ -530,6 +534,18 @@ static int __devexit xilinx_spi_remove(struct platform_device *dev)/* work with hotplug and coldplug */MODULE_ALIAS("platform:"XILINX_SPI_NAME);+staticconststructplatform_device_idxilinx_spi_id_table[]={+{+.name=XILINX_SPI_NAME,+},+{+.name="mfd_xilinx_spi",+.driver_data=MFD_PLATFORM_DEVICE,+},+{},/* Terminating Entry */+};+MODULE_DEVICE_TABLE(platform,xilinx_spi_id_table);+staticstructplatform_driverxilinx_spi_driver={.probe=xilinx_spi_probe,.remove=__devexit_p(xilinx_spi_remove),