On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon <dilinger@xxxxxxxxxx> wrote:
> On Fri, 1 Apr 2011 13:20:31 +0200
> Samuel Ortiz <sameo@xxxxxxxxxxxxxxx> wrote:
>
>> Hi Grant,
>>
>> On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
> [...]
>> > 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.
>
>
>
>> 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.
>>
>> 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.
>
>
>
>>
>> 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(-)
>>
>> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
>> index d01574d..8abe510 100644
>> --- a/drivers/mfd/mfd-core.c
>> +++ b/drivers/mfd/mfd-core.c
>> @@ -75,9 +75,16 @@ static int mfd_add_device(struct device *parent,
>> int id,
>> pdev->dev.parent = parent;
>>
>> - ret = platform_device_add_data(pdev, cell, sizeof(*cell));
>> - if (ret)
>> - goto fail_res;
>> + if (cell->mfd_data_size > 0) {
>> + ret = platform_device_add_data(pdev,
>> + cell->mfd_data,
>> cell->mfd_data_size);
>> + if (ret)
>> + goto fail_res;
>> + } else {
>> + ret = platform_device_add_data(pdev, cell,
>> sizeof(*cell));
>> + if (ret)
>> + goto fail_res;
>> + }
>>
>> for (r = 0; r < cell->num_resources; r++) {
>> res[r].name = cell->resources[r].name;
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
