LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Grant Likely <grant.likely@secretlab.ca>
To: Samuel Ortiz <sameo@linux.intel.com>
Cc: Andres Salomon <dilinger@queued.net>,
	linux-kernel@vger.kernel.org,
	Mark Brown <broonie@opensource.wolfsonmicro.com>,
	khali@linux-fr.org, ben-linux@fluff.org,
	Peter Korsgaard <jacmet@sunsite.dk>,
	Mauro Carvalho Chehab <mchehab@infradead.org>,
	David Brownell <dbrownell@users.sourceforge.net>,
	linux-i2c@vger.kernel.org, linux-media@vger.kernel.org,
	netdev@vger.kernel.org, spi-devel-general@lists.sourceforge.net,
	Mocean Laboratories <info@mocean-labs.com>,
	Greg Kroah-Hartman <gregkh@suse.de>
Subject: Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers
Date: Mon, 4 Apr 2011 21:04:29 -0600	[thread overview]
Message-ID: <20110405030428.GB29522@ponder.secretlab.ca> (raw)
In-Reply-To: <20110404100314.GC2751@sortiz-mobl>

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:
> > 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?
> 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.

> 
> --- 
>  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(-)
> 
> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> index d01574d..c0fc1c0 100644
> --- a/drivers/mfd/mfd-core.c
> +++ b/drivers/mfd/mfd-core.c
> @@ -18,6 +18,21 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/slab.h>
>  
> +static int mfd_platform_add_cell(struct platform_device *pdev, const struct mfd_cell *cell)
> +{
> +	struct mfd_cell *c;
> +
> +	if (cell == NULL)
> +		return 0;
> +
> +	c = kmemdup(cell, sizeof(struct mfd_cell), GFP_KERNEL);
> +	if (c == NULL)
> +		return -ENOMEM;
> +
> +	pdev->mfd_cell = c;
> +	return 0;
> +}

'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;
}

> +
>  int mfd_cell_enable(struct platform_device *pdev)
>  {
>  	const struct mfd_cell *cell = mfd_get_cell(pdev);
> @@ -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 ...

>  fail_res:
>  	kfree(res);
>  fail_device:
> @@ -171,6 +187,7 @@ static int mfd_remove_devices_fn(struct device *dev, void *c)
>  	if (!*usage_count || (cell->usage_count < *usage_count))
>  		*usage_count = cell->usage_count;
>  
> +	kfree(pdev->mfd_cell);

... from here.

>  	platform_device_unregister(pdev);
>  	return 0;
>  }
> diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> index ad1b19a..0e4d3a6 100644
> --- a/include/linux/mfd/core.h
> +++ b/include/linux/mfd/core.h
> @@ -86,7 +86,7 @@ extern int mfd_clone_cell(const char *cell, const char **clones,
>   */
>  static inline const struct mfd_cell *mfd_get_cell(struct platform_device *pdev)
>  {
> -	return pdev->dev.platform_data;
> +	return pdev->mfd_cell;
>  }
>  
>  /*
> @@ -95,7 +95,10 @@ static inline const struct mfd_cell *mfd_get_cell(struct platform_device *pdev)
>   */
>  static inline void *mfd_get_data(struct platform_device *pdev)
>  {
> -	return mfd_get_cell(pdev)->mfd_data;
> +	if (pdev->mfd_cell != NULL)
> +		return mfd_get_cell(pdev)->mfd_data;
> +	else
> +		return pdev->dev.platform_data;

Blech!  Yeah, this should become consistent that platform data
*always* comes from pdev->dev.platform_data.

>  }
>  
>  extern int mfd_add_devices(struct device *parent, int id,
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index d96db98..734d254 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -14,6 +14,8 @@
>  #include <linux/device.h>
>  #include <linux/mod_devicetable.h>
>  
> +struct mfd_cell;
> +
>  struct platform_device {
>  	const char	* name;
>  	int		id;
> @@ -23,6 +25,9 @@ struct platform_device {
>  
>  	const struct platform_device_id	*id_entry;
>  
> +	/* MFD cell pointer */
> +	struct mfd_cell	*mfd_cell;
> +

Move this down to by the of_node pointer.  May as well collect all the
supplemental data about the device in the same place.

>  	/* arch specific additions */
>  	struct pdev_archdata	archdata;
>  };
> 
> -- 
> Intel Open Source Technology Centre
> http://oss.intel.com/

  reply	other threads:[~2011-04-05  3:04 UTC|newest]

Thread overview: 65+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-02-03  3:54 [RFC] [PATCH 0/19] mfd sharing support Andres Salomon
2011-02-03  3:55 ` [PATCH 01/19] mfd-core: unconditionally add mfd_cell to every platform_device Andres Salomon
2011-02-03  3:58 ` [PATCH 02/19] jz4740: mfd_cell is now implicitly available to drivers Andres Salomon
2011-02-03  8:09   ` Jean Delvare
2011-02-03  4:01 ` [PATCH 03/19] ab3550: " Andres Salomon
2011-02-04  8:20   ` Mattias Wallin
2011-02-03  4:03 ` [PATCH 04/19] ab3100: " Andres Salomon
2011-02-03 12:52   ` Linus Walleij
2011-02-03  4:04 ` [PATCH 05/19] asic3: " Andres Salomon
2011-02-03  4:05 ` [PATCH 06/19] htc-pasic3: " Andres Salomon
2011-02-03  4:08 ` [PATCH 07/19] timberdale: " Andres Salomon
2011-03-31 23:05   ` Grant Likely
2011-04-01 11:20     ` Samuel Ortiz
2011-04-01 17:47       ` Andres Salomon
2011-04-01 17:56         ` Grant Likely
2011-04-01 18:00           ` Grant Likely
2011-04-01 23:52           ` Samuel Ortiz
2011-04-01 23:58             ` Grant Likely
2011-04-02  0:10               ` Andres Salomon
2011-04-04 10:03               ` Samuel Ortiz
2011-04-05  3:04                 ` Grant Likely [this message]
2011-04-06 15:23                   ` Samuel Ortiz
2011-04-06 15:58                     ` Greg KH
2011-04-06 17:05                       ` Samuel Ortiz
2011-04-06 17:16                         ` Ben Hutchings
2011-04-06 17:51                           ` Samuel Ortiz
2011-04-06 18:07                             ` Ben Hutchings
2011-04-06 17:56                         ` Greg KH
2011-04-06 18:25                           ` Andres Salomon
2011-04-06 18:38                             ` Greg KH
2011-04-07  8:04                               ` Grant Likely
2011-04-06 18:47                           ` Samuel Ortiz
2011-04-06 18:59                             ` Felipe Balbi
2011-04-06 22:09                               ` Greg KH
2011-04-07  8:09                                 ` Felipe Balbi
2011-04-07 13:40                               ` Samuel Ortiz
2011-04-07 14:35                                 ` Grant Likely
2011-04-07 15:03                                   ` Samuel Ortiz
2011-04-07 18:06                                     ` Grant Likely
2011-04-07 16:24                   ` Grant Likely
2011-04-01 18:26         ` Samuel Ortiz
2011-02-03  4:09 ` [PATCH 08/19] t7166xb: " Andres Salomon
2011-02-03  4:11 ` [PATCH 09/19] wl1273: " Andres Salomon
2011-02-03  4:12 ` [PATCH 10/19] sh_mobile_sdhi: " Andres Salomon
2011-02-03  4:13 ` [PATCH 11/19] tc6393xb: " Andres Salomon
2011-02-03  4:15 ` [PATCH 12/19] twl4030: " Andres Salomon
2011-02-03  6:05   ` Dmitry Torokhov
2011-02-03  6:39     ` Andres Salomon
2011-02-03  6:53       ` Dmitry Torokhov
2011-02-03  7:03         ` Andres Salomon
2011-02-03  9:31           ` Mark Brown
2011-02-05  2:39             ` Andres Salomon
2011-02-05  3:25               ` Andres Salomon
2011-02-03 12:23           ` Peter Ujfalusi
2011-02-04 10:41           ` Uwe Kleine-König
2011-02-03  4:16 ` [PATCH 13/19] tc6387xb: " Andres Salomon
2011-02-03  4:17 ` [PATCH 14/19] janz: " Andres Salomon
2011-02-03  4:20 ` [PATCH 15/19] mc13xxx: " Andres Salomon
2011-02-04  9:34   ` Uwe Kleine-König
2011-02-04 10:13     ` Uwe Kleine-König
2011-02-04 10:16     ` Andres Salomon
2011-02-03  4:21 ` [PATCH 16/19] mfd-core: drop platform_data/data_size from core mfd_cell struct Andres Salomon
2011-02-03  4:22 ` [PATCH 17/19] mfd-core: add refcounting support to mfd_cells Andres Salomon
2011-02-03  4:23 ` [PATCH 18/19] mfd-core: add platform_device sharing support for mfd Andres Salomon
2011-02-03  4:26 ` [PATCH 19/19] cs5535-mfd: add sharing for acpi/pms cells Andres Salomon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20110405030428.GB29522@ponder.secretlab.ca \
    --to=grant.likely@secretlab.ca \
    --cc=ben-linux@fluff.org \
    --cc=broonie@opensource.wolfsonmicro.com \
    --cc=dbrownell@users.sourceforge.net \
    --cc=dilinger@queued.net \
    --cc=gregkh@suse.de \
    --cc=info@mocean-labs.com \
    --cc=jacmet@sunsite.dk \
    --cc=khali@linux-fr.org \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@infradead.org \
    --cc=netdev@vger.kernel.org \
    --cc=sameo@linux.intel.com \
    --cc=spi-devel-general@lists.sourceforge.net \
    --subject='Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).