Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Andres Salomon <dilinger@queued.net>]

On Fri, 1 Apr 2011 17:58:44 -0600
Grant Likely <grant.likely@secretlab.ca> wrote:

> On Fri, Apr 1, 2011 at 5:52 PM, Samuel Ortiz <sameo@linux.intel.com>
> wrote:
> > On Fri, Apr 01, 2011 at 11:56:35AM -0600, Grant Likely wrote:
> >> On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon
> >> <dilinger@queued.net> wrote:
> >> > On Fri, 1 Apr 2011 13:20:31 +0200
> >> > Samuel Ortiz <sameo@linux.intel.com> 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.
> > 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).