[PATCH 0/4] Some pinctrl fixes for pinmux modules

[PATCH 0/4] Some pinctrl fixes for pinmux modules

[Tony Lindgren]

Hi,

Here are few fixes mostly to make it easier to develop
loadable pinmux modules.

Regards,

Tony

---

Tony Lindgren (4):
      pinctrl: Free debugfs entries when unloading a pinmux driver
      pinctrl: Fix pinmux_hog_maps when ctrl_dev_name is not set
      pinctrl: Add checks for empty names in pinmux_search_function
      pinctrl: Fix some pinmux typos


 Documentation/pinctrl.txt |    2 +-
 drivers/pinctrl/core.c    |   14 +++++++++++++-
 drivers/pinctrl/core.h    |    3 +++
 drivers/pinctrl/pinmux.c  |   23 ++++++++++++++---------
 4 files changed, 31 insertions(+), 11 deletions(-)

-- 
Signature

[PATCH 1/4] pinctrl: Free debugfs entries when unloading a pinmux driver

[Tony Lindgren]

Free debugfs entries when unloading a pinmux driver

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/pinctrl/core.c |   14 +++++++++++++-
 drivers/pinctrl/core.h |    3 +++
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 569bdb3..d9d35fc 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -510,10 +510,12 @@ static struct dentry *debugfs_root;
 
 static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev)
 {
-	static struct dentry *device_root;
+	struct dentry *device_root;
 
 	device_root = debugfs_create_dir(dev_name(pctldev->dev),
 					 debugfs_root);
+	pctldev->device_root = device_root;
+
 	if (IS_ERR(device_root) || !device_root) {
 		pr_warn("failed to create debugfs directory for %s\n",
 			dev_name(pctldev->dev));
@@ -529,6 +531,11 @@ static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev)
 	pinconf_init_device_debugfs(device_root, pctldev);
 }
 
+static void pinctrl_remove_device_debugfs(struct pinctrl_dev *pctldev)
+{
+	debugfs_remove_recursive(pctldev->device_root);
+}
+
 static void pinctrl_init_debugfs(void)
 {
 	debugfs_root = debugfs_create_dir("pinctrl", NULL);
@@ -553,6 +560,10 @@ static void pinctrl_init_debugfs(void)
 {
 }
 
+static void pinctrl_remove_device_debugfs(struct pinctrl_dev *pctldev)
+{
+}
+
 #endif
 
 /**
@@ -641,6 +652,7 @@ void pinctrl_unregister(struct pinctrl_dev *pctldev)
 	if (pctldev == NULL)
 		return;
 
+	pinctrl_remove_device_debugfs(pctldev);
 	pinmux_unhog_maps(pctldev);
 	/* TODO: check that no pinmuxes are still active? */
 	mutex_lock(&pinctrldev_list_mutex);
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 177a331..cfa86da 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -41,6 +41,9 @@ struct pinctrl_dev {
 	struct device *dev;
 	struct module *owner;
 	void *driver_data;
+#ifdef CONFIG_DEBUG_FS
+	struct dentry *device_root;
+#endif
 #ifdef CONFIG_PINMUX
 	struct mutex pinmux_hogs_lock;
 	struct list_head pinmux_hogs;

[PATCH 2/4] pinctrl: Fix pinmux_hog_maps when ctrl_dev_name is not set

[Tony Lindgren]

The ctrl_dev_name is optional for struct pinmux_map assuming
that ctrl_dev is set. Without this patch we can get:

Unable to handle kernel NULL pointer dereference at virtual address 00000000
...
(pinmux_hog_maps+0xa4/0x20c)
(pinctrl_register+0x2a4/0x378)
...

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/pinctrl/pinmux.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index a76a348..06b8943 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -992,9 +992,12 @@ int pinmux_hog_maps(struct pinctrl_dev *pctldev)
 
 	for (i = 0; i < pinmux_maps_num; i++) {
 		struct pinmux_map const *map = &pinmux_maps[i];
+		int match_found = 0;
 
-		if (((map->ctrl_dev == dev) ||
-		     !strcmp(map->ctrl_dev_name, devname)) &&
+		if (map->ctrl_dev_name && !strcmp(map->ctrl_dev_name, devname))
+				match_found = 1;
+
+		if (((map->ctrl_dev == dev) || match_found) &&
 		    map->hog_on_boot) {
 			/* OK time to hog! */
 			ret = pinmux_hog_map(pctldev, map);

[PATCH 3/4] pinctrl: Add checks for empty names in pinmux_search_function

[Tony Lindgren]

Otherwise we can get the following when dealing with
buggy data in a pinmux driver:

Unable to handle kernel NULL pointer dereference at virtual address 00000000
...
PC is at strcmp+0xc/0x34
LR is at pinmux_get+0x350/0x8f4
...

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/pinctrl/pinmux.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 06b8943..ffe633d 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -584,6 +584,13 @@ static int pinmux_search_function(struct pinctrl_dev *pctldev,
 							   selector);
 		int ret;
 
+		if (!fname) {
+			pr_warning("no name for function%i\n",
+				   selector);
+			selector++;
+			continue;
+		}
+
 		if (!strcmp(map->function, fname)) {
 			/* Found the function, check pin group */
 			ret = pinmux_check_pin_group(pctldev, selector,

[PATCH 4/4] pinctrl: Fix some pinmux typos

[Tony Lindgren]

Fix some pinmux typos so implementing pinmux drivers
is a bit easier.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 Documentation/pinctrl.txt |    2 +-
 drivers/pinctrl/pinmux.c  |    9 ++-------
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index 6727b92..4af17a0 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -1025,7 +1025,7 @@ it, disables and releases it, and muxes it in on the pins defined by group B:
 
 foo_switch()
 {
-	struct pinmux pmx;
+	struct pinmux *pmx;
 
 	/* Enable on position A */
 	pmx = pinmux_get(&device, "spi0-pos-A");
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index ffe633d..8acf54e 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -53,11 +53,6 @@ struct pinmux_group {
  * @dev: the device using this pinmux
  * @usecount: the number of active users of this mux setting, used to keep
  *	track of nested use cases
- * @pins: an array of discrete physical pins used in this mapping, taken
- *	from the global pin enumeration space (copied from pinmux map)
- * @num_pins: the number of pins in this mapping array, i.e. the number of
- *	elements in .pins so we can iterate over that array (copied from
- *	pinmux map)
  * @pctldev: pin control device handling this pinmux
  * @func_selector: the function selector for the pinmux device handling
  *	this pinmux
@@ -411,7 +406,7 @@ int __init pinmux_register_mappings(struct pinmux_map const *maps,
 }
 
 /**
- * acquire_pins() - acquire all the pins for a certain funcion on a pinmux
+ * acquire_pins() - acquire all the pins for a certain function on a pinmux
  * @pctldev: the device to take the pins on
  * @func_selector: the function selector to acquire the pins for
  * @group_selector: the group selector containing the pins to acquire
@@ -458,7 +453,7 @@ static int acquire_pins(struct pinctrl_dev *pctldev,
 
 /**
  * release_pins() - release pins taken by earlier acquirement
- * @pctldev: the device to free the pinx on
+ * @pctldev: the device to free the pins on
  * @group_selector: the group selector containing the pins to free
  */
 static void release_pins(struct pinctrl_dev *pctldev,

RE: [PATCH 2/4] pinctrl: Fix pinmux_hog_maps when ctrl_dev_name is not set

[Stephen Warren]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1350 bytes --]

Tony Lindgren wrote at Friday, January 20, 2012 9:17 AM:
> The ctrl_dev_name is optional for struct pinmux_map assuming
> that ctrl_dev is set. Without this patch we can get:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 00000000

> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c


> @@ -992,9 +992,12 @@ int pinmux_hog_maps(struct pinctrl_dev *pctldev)
> 
>  	for (i = 0; i < pinmux_maps_num; i++) {
>  		struct pinmux_map const *map = &pinmux_maps[i];
> +		int match_found = 0;
> 
> -		if (((map->ctrl_dev == dev) ||
> -		     !strcmp(map->ctrl_dev_name, devname)) &&
> +		if (map->ctrl_dev_name && !strcmp(map->ctrl_dev_name, devname))
> +				match_found = 1;
> +
> +		if (((map->ctrl_dev == dev) || match_found) &&
>  		    map->hog_on_boot) {
>  			/* OK time to hog! */
>  			ret = pinmux_hog_map(pctldev, map);

Wouldn't it be better if match_found was true for all matching cases,
in other words for the new code to be:

		int match_found = 0;

		if (map->ctrl_dev_name && !strcmp(map->ctrl_dev_name, devname))
				match_found = 1;
		if (map->ctrl_dev == dev)
				match_found = 1;
		if (match_found && map->hog_on_boot) {

-- 
nvpublic

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 3/4] pinctrl: Add checks for empty names in pinmux_search_function

[Stephen Warren]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1209 bytes --]

Tony Lindgren wrote at Friday, January 20, 2012 9:18 AM:
> Otherwise we can get the following when dealing with
> buggy data in a pinmux driver:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 00000000

> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
> index 06b8943..ffe633d 100644
> --- a/drivers/pinctrl/pinmux.c
> +++ b/drivers/pinctrl/pinmux.c
> @@ -584,6 +584,13 @@ static int pinmux_search_function(struct pinctrl_dev *pctldev,
>  							   selector);
>  		int ret;
> 
> +		if (!fname) {
> +			pr_warning("no name for function%i\n",
> +				   selector);
> +			selector++;
> +			continue;
> +		}
> +
>  		if (!strcmp(map->function, fname)) {
>  			/* Found the function, check pin group */
>  			ret = pinmux_check_pin_group(pctldev, selector,

Shouldn't this be BUG_ON(!fname)?

There are lots of other places that pmxops->get_function_name() is
called. Wouldn't it be better to enhance e.g. pinmux_check_ops() to
validate that all functions have a name during pinctrl registration?

-- 
nvpublic

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/4] pinctrl: Fix pinmux_hog_maps when ctrl_dev_name is not set

[Tony Lindgren]

* Stephen Warren <swarren@nvidia.com> [120120 08:24]:
> Tony Lindgren wrote at Friday, January 20, 2012 9:17 AM:
> > The ctrl_dev_name is optional for struct pinmux_map assuming
> > that ctrl_dev is set. Without this patch we can get:
> > 
> > Unable to handle kernel NULL pointer dereference at virtual address 00000000
> 
> > diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
> 
> 
> > @@ -992,9 +992,12 @@ int pinmux_hog_maps(struct pinctrl_dev *pctldev)
> > 
> >  	for (i = 0; i < pinmux_maps_num; i++) {
> >  		struct pinmux_map const *map = &pinmux_maps[i];
> > +		int match_found = 0;
> > 
> > -		if (((map->ctrl_dev == dev) ||
> > -		     !strcmp(map->ctrl_dev_name, devname)) &&
> > +		if (map->ctrl_dev_name && !strcmp(map->ctrl_dev_name, devname))
> > +				match_found = 1;
> > +
> > +		if (((map->ctrl_dev == dev) || match_found) &&
> >  		    map->hog_on_boot) {
> >  			/* OK time to hog! */
> >  			ret = pinmux_hog_map(pctldev, map);
> 
> Wouldn't it be better if match_found was true for all matching cases,
> in other words for the new code to be:
> 
> 		int match_found = 0;
> 
> 		if (map->ctrl_dev_name && !strcmp(map->ctrl_dev_name, devname))
> 				match_found = 1;
> 		if (map->ctrl_dev == dev)
> 				match_found = 1;
> 		if (match_found && map->hog_on_boot) {

OK will take a look, that would make the code easier to read.

Regards,

Tony

Re: [PATCH 3/4] pinctrl: Add checks for empty names in pinmux_search_function

[Tony Lindgren]

* Stephen Warren <swarren@nvidia.com> [120120 08:28]:
> Tony Lindgren wrote at Friday, January 20, 2012 9:18 AM:
> > Otherwise we can get the following when dealing with
> > buggy data in a pinmux driver:
> > 
> > Unable to handle kernel NULL pointer dereference at virtual address 00000000
> 
> > diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
> > index 06b8943..ffe633d 100644
> > --- a/drivers/pinctrl/pinmux.c
> > +++ b/drivers/pinctrl/pinmux.c
> > @@ -584,6 +584,13 @@ static int pinmux_search_function(struct pinctrl_dev *pctldev,
> >  							   selector);
> >  		int ret;
> > 
> > +		if (!fname) {
> > +			pr_warning("no name for function%i\n",
> > +				   selector);
> > +			selector++;
> > +			continue;
> > +		}
> > +
> >  		if (!strcmp(map->function, fname)) {
> >  			/* Found the function, check pin group */
> >  			ret = pinmux_check_pin_group(pctldev, selector,
> 
> Shouldn't this be BUG_ON(!fname)?
>
> There are lots of other places that pmxops->get_function_name() is
> called. Wouldn't it be better to enhance e.g. pinmux_check_ops() to
> validate that all functions have a name during pinctrl registration?

Maybe yeah. I'll take a look next week when I'm back home.

Tony

Re: [PATCH 1/4] pinctrl: Free debugfs entries when unloading a pinmux driver

[Linus Walleij]

On Fri, Jan 20, 2012 at 5:17 PM, Tony Lindgren <tony@atomide.com> wrote:

> Free debugfs entries when unloading a pinmux driver
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Thanks, patch applied.

Yours,
Linus Walleij

Re: [PATCH 4/4] pinctrl: Fix some pinmux typos

[Linus Walleij]

On Fri, Jan 20, 2012 at 5:17 PM, Tony Lindgren <tony@atomide.com> wrote:

> Fix some pinmux typos so implementing pinmux drivers
> is a bit easier.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Thanks for spotting these, patch applied.

Yours,
Linus Walleij

Re: [PATCH 0/4] Some pinctrl fixes for pinmux modules

[Linus Walleij]

On Fri, Jan 20, 2012 at 5:17 PM, Tony Lindgren <tony@atomide.com> wrote:

> Here are few fixes mostly to make it easier to develop
> loadable pinmux modules.

So applied 1/4 and 4/4 waiting for v2 of 2/4 and 3/4 in accordance
with Stephens comments.

Yours,
Linus Walleij

Re: [PATCH 2/4] pinctrl: Fix pinmux_hog_maps when ctrl_dev_name is not set

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [120120 09:06]:
> * Stephen Warren <swarren@nvidia.com> [120120 08:24]:
> > Tony Lindgren wrote at Friday, January 20, 2012 9:17 AM:
> > > The ctrl_dev_name is optional for struct pinmux_map assuming
> > > that ctrl_dev is set. Without this patch we can get:
> > > 
> > > Unable to handle kernel NULL pointer dereference at virtual address 00000000
> > 
> > > diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
> > 
> > 
> > > @@ -992,9 +992,12 @@ int pinmux_hog_maps(struct pinctrl_dev *pctldev)
> > > 
> > >  	for (i = 0; i < pinmux_maps_num; i++) {
> > >  		struct pinmux_map const *map = &pinmux_maps[i];
> > > +		int match_found = 0;
> > > 
> > > -		if (((map->ctrl_dev == dev) ||
> > > -		     !strcmp(map->ctrl_dev_name, devname)) &&
> > > +		if (map->ctrl_dev_name && !strcmp(map->ctrl_dev_name, devname))
> > > +				match_found = 1;
> > > +
> > > +		if (((map->ctrl_dev == dev) || match_found) &&
> > >  		    map->hog_on_boot) {
> > >  			/* OK time to hog! */
> > >  			ret = pinmux_hog_map(pctldev, map);
> > 
> > Wouldn't it be better if match_found was true for all matching cases,
> > in other words for the new code to be:
> > 
> > 		int match_found = 0;
> > 
> > 		if (map->ctrl_dev_name && !strcmp(map->ctrl_dev_name, devname))
> > 				match_found = 1;
> > 		if (map->ctrl_dev == dev)
> > 				match_found = 1;
> > 		if (match_found && map->hog_on_boot) {
> 
> OK will take a look, that would make the code easier to read.

Here's this one updated, looks like we can keep it readable
even without match_found if we continue the search early based
on !map->hog_on_boot.

Regards,

Tony


From: Tony Lindgren <tony@atomide.com>
Date: Fri, 20 Jan 2012 07:43:53 -0800
Subject: [PATCH] pinctrl: Fix pinmux_hog_maps when ctrl_dev_name is not set

The ctrl_dev_name is optional for struct pinmux_map assuming
that ctrl_dev is set. Without this patch we can get:

Unable to handle kernel NULL pointer dereference at virtual address 00000000
...
(pinmux_hog_maps+0xa4/0x20c)
(pinctrl_register+0x2a4/0x378)
...

Fix this by adding adding a test for map->ctrl_dev.
Additionally move the test for map->ctrl_dev earlier
to optimize out the loop a bit.

Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -993,9 +993,12 @@ int pinmux_hog_maps(struct pinctrl_dev *pctldev)
 	for (i = 0; i < pinmux_maps_num; i++) {
 		struct pinmux_map const *map = &pinmux_maps[i];
 
-		if (((map->ctrl_dev == dev) ||
-		     !strcmp(map->ctrl_dev_name, devname)) &&
-		    map->hog_on_boot) {
+		if (!map->hog_on_boot)
+			continue;
+
+		if ((map->ctrl_dev == dev) ||
+			(map->ctrl_dev_name &&
+				!strcmp(map->ctrl_dev_name, devname))) {
 			/* OK time to hog! */
 			ret = pinmux_hog_map(pctldev, map);
 			if (ret)

RE: [PATCH 2/4] pinctrl: Fix pinmux_hog_maps when ctrl_dev_name is not set

[Stephen Warren]

Tony Lindgren wrote at Tuesday, January 24, 2012 5:24 PM:
...
> The ctrl_dev_name is optional for struct pinmux_map assuming
> that ctrl_dev is set. Without this patch we can get:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> ...
> (pinmux_hog_maps+0xa4/0x20c)
> (pinctrl_register+0x2a4/0x378)
> ...
> 
> Fix this by adding adding a test for map->ctrl_dev.
> Additionally move the test for map->ctrl_dev earlier
> to optimize out the loop a bit.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Acked-by: Stephen Warren <swarren@nvidia.com>

-- 
nvpublic

Re: [PATCH 3/4] pinctrl: Add checks for empty names in pinmux_search_function

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [120120 09:09]:
> * Stephen Warren <swarren@nvidia.com> [120120 08:28]:
> > Tony Lindgren wrote at Friday, January 20, 2012 9:18 AM:
> > > Otherwise we can get the following when dealing with
> > > buggy data in a pinmux driver:
> > > 
> > > Unable to handle kernel NULL pointer dereference at virtual address 00000000
> > 
> > > diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
> > > index 06b8943..ffe633d 100644
> > > --- a/drivers/pinctrl/pinmux.c
> > > +++ b/drivers/pinctrl/pinmux.c
> > > @@ -584,6 +584,13 @@ static int pinmux_search_function(struct pinctrl_dev *pctldev,
> > >  							   selector);
> > >  		int ret;
> > > 
> > > +		if (!fname) {
> > > +			pr_warning("no name for function%i\n",
> > > +				   selector);
> > > +			selector++;
> > > +			continue;
> > > +		}
> > > +
> > >  		if (!strcmp(map->function, fname)) {
> > >  			/* Found the function, check pin group */
> > >  			ret = pinmux_check_pin_group(pctldev, selector,
> > 
> > Shouldn't this be BUG_ON(!fname)?
> >
> > There are lots of other places that pmxops->get_function_name() is
> > called. Wouldn't it be better to enhance e.g. pinmux_check_ops() to
> > validate that all functions have a name during pinctrl registration?
> 
> Maybe yeah. I'll take a look next week when I'm back home.

There's a little bit of a init order issue here to add checking for
the function names during registration. That's because we don't allocate
pctldev until after pinmux_check_ops() in pinctrl_register. This means
we can't currently call ops->list_functions until unless we change the
init order.

To fix this we need to allocate pctldev earlier, and then change the
*_check_ops functions to use pctldev, see below.

Regards,

Tony


From: Tony Lindgren <tony@atomide.com>
Date: Tue, 24 Jan 2012 16:28:08 -0800
Subject: [PATCH] pinctrl: Add checks for empty function names

This is needed as otherwise we can get the following when dealing
with buggy data in a pinmux driver for pinmux_search_function:

Unable to handle kernel NULL pointer dereference at virtual address 00000000
...
PC is at strcmp+0xc/0x34
LR is at pinmux_get+0x350/0x8f4
...

As we need pctldev initialized to call ops->list_functions, let's
initialize it before check_ops calls and pass the pctldev to the
check_ops functions. Do this for both pinmux and pinconf check_ops
functions.

Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -583,40 +583,40 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
 	if (pctldesc->name == NULL)
 		return NULL;
 
+	pctldev = kzalloc(sizeof(struct pinctrl_dev), GFP_KERNEL);
+	if (pctldev == NULL)
+		return NULL;
+
+	/* Initialize pin control device struct */
+	pctldev->owner = pctldesc->owner;
+	pctldev->desc = pctldesc;
+	pctldev->driver_data = driver_data;
+	INIT_RADIX_TREE(&pctldev->pin_desc_tree, GFP_KERNEL);
+	spin_lock_init(&pctldev->pin_desc_tree_lock);
+	INIT_LIST_HEAD(&pctldev->gpio_ranges);
+	mutex_init(&pctldev->gpio_ranges_lock);
+	pctldev->dev = dev;
+
 	/* If we're implementing pinmuxing, check the ops for sanity */
 	if (pctldesc->pmxops) {
-		ret = pinmux_check_ops(pctldesc->pmxops);
+		ret = pinmux_check_ops(pctldev);
 		if (ret) {
 			pr_err("%s pinmux ops lacks necessary functions\n",
 			       pctldesc->name);
-			return NULL;
+			goto out_err;
 		}
 	}
 
 	/* If we're implementing pinconfig, check the ops for sanity */
 	if (pctldesc->confops) {
-		ret = pinconf_check_ops(pctldesc->confops);
+		ret = pinconf_check_ops(pctldev);
 		if (ret) {
 			pr_err("%s pin config ops lacks necessary functions\n",
 			       pctldesc->name);
-			return NULL;
+			goto out_err;
 		}
 	}
 
-	pctldev = kzalloc(sizeof(struct pinctrl_dev), GFP_KERNEL);
-	if (pctldev == NULL)
-		return NULL;
-
-	/* Initialize pin control device struct */
-	pctldev->owner = pctldesc->owner;
-	pctldev->desc = pctldesc;
-	pctldev->driver_data = driver_data;
-	INIT_RADIX_TREE(&pctldev->pin_desc_tree, GFP_KERNEL);
-	spin_lock_init(&pctldev->pin_desc_tree_lock);
-	INIT_LIST_HEAD(&pctldev->gpio_ranges);
-	mutex_init(&pctldev->gpio_ranges_lock);
-	pctldev->dev = dev;
-
 	/* Register all the pins */
 	pr_debug("try to register %d pins on %s...\n",
 		 pctldesc->npins, pctldesc->name);
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -205,8 +205,10 @@ int pin_config_group_set(const char *dev_name, const char *pin_group,
 }
 EXPORT_SYMBOL(pin_config_group_set);
 
-int pinconf_check_ops(const struct pinconf_ops *ops)
+int pinconf_check_ops(struct pinctrl_dev *pctldev)
 {
+	const struct pinconf_ops *ops = pctldev->desc->confops;
+
 	/* We must be able to read out pin status */
 	if (!ops->pin_config_get && !ops->pin_config_group_get)
 		return -EINVAL;
--- a/drivers/pinctrl/pinconf.h
+++ b/drivers/pinctrl/pinconf.h
@@ -13,7 +13,7 @@
 
 #ifdef CONFIG_PINCONF
 
-int pinconf_check_ops(const struct pinconf_ops *ops);
+int pinconf_check_ops(struct pinctrl_dev *pctldev);
 void pinconf_init_device_debugfs(struct dentry *devroot,
 				 struct pinctrl_dev *pctldev);
 int pin_config_get_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
@@ -23,7 +23,7 @@ int pin_config_set_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
 
 #else
 
-static inline int pinconf_check_ops(const struct pinconf_ops *ops)
+static inline int pinconf_check_ops(struct pinctrl_dev *pctldev)
 {
 	return 0;
 }
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -904,8 +904,11 @@ void pinmux_disable(struct pinmux *pmx)
 }
 EXPORT_SYMBOL_GPL(pinmux_disable);
 
-int pinmux_check_ops(const struct pinmux_ops *ops)
+int pinmux_check_ops(struct pinctrl_dev *pctldev)
 {
+	const struct pinmux_ops *ops = pctldev->desc->pmxops;
+	unsigned selector = 0;
+
 	/* Check that we implement required operations */
 	if (!ops->list_functions ||
 	    !ops->get_function_name ||
@@ -914,6 +917,18 @@ int pinmux_check_ops(const struct pinmux_ops *ops)
 	    !ops->disable)
 		return -EINVAL;
 
+	/* Check that all functions registered have names */
+	while (ops->list_functions(pctldev, selector) >= 0) {
+		const char *fname = ops->get_function_name(pctldev,
+							   selector);
+		if (!fname) {
+			pr_err("pinmux ops has no name for function%u\n",
+				selector);
+			return -EINVAL;
+		}
+		selector++;
+	}
+
 	return 0;
 }
 
--- a/drivers/pinctrl/pinmux.h
+++ b/drivers/pinctrl/pinmux.h
@@ -12,7 +12,7 @@
  */
 #ifdef CONFIG_PINMUX
 
-int pinmux_check_ops(const struct pinmux_ops *ops);
+int pinmux_check_ops(struct pinctrl_dev *pctldev);
 void pinmux_init_device_debugfs(struct dentry *devroot,
 				struct pinctrl_dev *pctldev);
 void pinmux_init_debugfs(struct dentry *subsys_root);
@@ -21,7 +21,7 @@ void pinmux_unhog_maps(struct pinctrl_dev *pctldev);
 
 #else
 
-static inline int pinmux_check_ops(const struct pinmux_ops *ops)
+static inline int pinmux_check_ops(struct pinctrl_dev *pctldev)
 {
 	return 0;
 }

Re: [PATCH 2/4] pinctrl: Fix pinmux_hog_maps when ctrl_dev_name is not set

[Linus Walleij]

On Wed, Jan 25, 2012 at 1:23 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Tony Lindgren <tony@atomide.com> [120120 09:06]:

> Here's this one updated, looks like we can keep it readable
> even without match_found if we continue the search early based
> on !map->hog_on_boot.

Thanks, patch applied with Stephens ACK.

Yours,
Linus Walleij

Re: [PATCH 3/4] pinctrl: Add checks for empty names in pinmux_search_function

[Linus Walleij]

On Wed, Jan 25, 2012 at 11:08 PM, Tony Lindgren <tony@atomide.com> wrote:

> This is needed as otherwise we can get the following when dealing
> with buggy data in a pinmux driver for pinmux_search_function:
>
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> ...
> PC is at strcmp+0xc/0x34
> LR is at pinmux_get+0x350/0x8f4
> ...
>
> As we need pctldev initialized to call ops->list_functions, let's
> initialize it before check_ops calls and pass the pctldev to the
> check_ops functions. Do this for both pinmux and pinconf check_ops
> functions.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

This looks correct to me, so applied it and pushed to -next
for testing.

Yours,
Linus Walleij