devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] Support qcom pinctrl protected pins
@ 2018-01-10  1:58 Stephen Boyd
       [not found] ` <20180110015848.11480-1-sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
                   ` (2 more replies)
  0 siblings, 3 replies; 19+ messages in thread
From: Stephen Boyd @ 2018-01-10  1:58 UTC (permalink / raw)
  To: Linus Walleij
  Cc: linux-kernel, linux-arm-msm, linux-arm-kernel, Timur Tabi,
	Andy Shevchenko, Bjorn Andersson, linux-gpio, devicetree

This patchset proposes a solution to describing the valid
pins for a pin controller in a semi-generic way so that qcom
platforms can expose the pins that are really available.

Typically, this has been done by having drivers and firmware 
descriptions only use pins they know they have access to, and that
still works now because we no longer read the pin direction at
boot. But there are still some userspace drivers and debugfs facilities
that don't know what pins are available and attempt to read everything
they can. On qcom platforms, this may lead to a system hang, which isn't 
very nice behavior, even if root is the only user that can trigger it.

The proposal is to describe the valid pins and then not allow things to
cause problems by using the invalid pins. Obviously, the firmware may
mess this up, so this is mostly a nice to have feature or a safety net
so that things don't blow up easily.

Stephen Boyd (3):
  gpiolib: Export gpiochip_irqchip_irq_valid() to drivers
  dt-bindings: pinctrl: Add a ngpios-ranges property
  pinctrl: qcom: Don't allow protected pins to be requested

 .../bindings/pinctrl/qcom,msm8996-pinctrl.txt      |  6 ++
 drivers/gpio/gpiolib.c                             |  5 +-
 drivers/pinctrl/qcom/pinctrl-msm.c                 | 98 +++++++++++++++++++++-
 include/linux/gpio/driver.h                        |  3 +
 4 files changed, 106 insertions(+), 6 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 1/3] gpiolib: Export gpiochip_irqchip_irq_valid() to drivers
       [not found] ` <20180110015848.11480-1-sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
@ 2018-01-10  1:58   ` Stephen Boyd
  2018-01-10  6:16     ` Bjorn Andersson
  2018-01-10 13:22     ` Linus Walleij
  0 siblings, 2 replies; 19+ messages in thread
From: Stephen Boyd @ 2018-01-10  1:58 UTC (permalink / raw)
  To: Linus Walleij
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Timur Tabi,
	Andy Shevchenko, Bjorn Andersson,
	linux-gpio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA

Some pinctrl drivers can use the gpiochip irq valid information
to figure out if certain gpios are exposed to the kernel for
usage or not. Expose this API so we can use it in the
pinmux_ops::request ops.

Signed-off-by: Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/gpio/gpiolib.c      | 5 +++--
 include/linux/gpio/driver.h | 3 +++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index b80936a25caa..c18b7b60ea1d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1503,14 +1503,15 @@ static void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gpiochip)
 	gpiochip->irq.valid_mask = NULL;
 }
 
-static bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip,
-				       unsigned int offset)
+bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip,
+				unsigned int offset)
 {
 	/* No mask means all valid */
 	if (likely(!gpiochip->irq.valid_mask))
 		return true;
 	return test_bit(offset, gpiochip->irq.valid_mask);
 }
+EXPORT_SYMBOL_GPL(gpiochip_irqchip_irq_valid);
 
 /**
  * gpiochip_set_cascaded_irqchip() - connects a cascaded irqchip to a gpiochip
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 7258cd676df4..1ba9a331ec51 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -436,6 +436,9 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
 			     struct lock_class_key *lock_key,
 			     struct lock_class_key *request_key);
 
+bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip,
+				unsigned int offset);
+
 #ifdef CONFIG_LOCKDEP
 
 /*
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 2/3] dt-bindings: pinctrl: Add a ngpios-ranges property
  2018-01-10  1:58 [PATCH 0/3] Support qcom pinctrl protected pins Stephen Boyd
       [not found] ` <20180110015848.11480-1-sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
@ 2018-01-10  1:58 ` Stephen Boyd
  2018-01-10 12:54   ` Andy Shevchenko
       [not found]   ` <20180110015848.11480-3-sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
  2018-01-10  1:58 ` [PATCH 3/3] pinctrl: qcom: Don't allow protected pins to be requested Stephen Boyd
  2 siblings, 2 replies; 19+ messages in thread
From: Stephen Boyd @ 2018-01-10  1:58 UTC (permalink / raw)
  To: Linus Walleij
  Cc: linux-kernel, linux-arm-msm, linux-arm-kernel, Timur Tabi,
	Andy Shevchenko, Bjorn Andersson, linux-gpio, devicetree

Some qcom platforms make some GPIOs or pins unavailable for use
by non-secure operating systems, and thus reading or writing the
registers for those pins will cause access control issues.
Introduce a DT property to describe the set of GPIOs that are
available for use so that higher level OSes are able to know what
pins to avoid reading/writing.

Cc: <devicetree@vger.kernel.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---

I stuck this inside msm8996, but maybe it can go somewhere more generic?

 Documentation/devicetree/bindings/pinctrl/qcom,msm8996-pinctrl.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,msm8996-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,msm8996-pinctrl.txt
index aaf01e929eea..8354ab270486 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,msm8996-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,msm8996-pinctrl.txt
@@ -40,6 +40,12 @@ MSM8996 platform.
 	Definition: must be 2. Specifying the pin number and flags, as defined
 		    in <dt-bindings/gpio/gpio.h>
 
+- ngpios-ranges:
+	Usage: optional
+	Value type: <prop-encoded-array>
+	Definition: Tuples of GPIO ranges (base, size) indicating
+		    GPIOs available for use.
+
 Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for
 a general description of GPIO and interrupt bindings.
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 3/3] pinctrl: qcom: Don't allow protected pins to be requested
  2018-01-10  1:58 [PATCH 0/3] Support qcom pinctrl protected pins Stephen Boyd
       [not found] ` <20180110015848.11480-1-sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
  2018-01-10  1:58 ` [PATCH 2/3] dt-bindings: pinctrl: Add a ngpios-ranges property Stephen Boyd
@ 2018-01-10  1:58 ` Stephen Boyd
  2018-01-10  6:11   ` Bjorn Andersson
                     ` (2 more replies)
  2 siblings, 3 replies; 19+ messages in thread
From: Stephen Boyd @ 2018-01-10  1:58 UTC (permalink / raw)
  To: Linus Walleij
  Cc: linux-kernel, linux-arm-msm, linux-arm-kernel, Timur Tabi,
	Andy Shevchenko, Bjorn Andersson, linux-gpio, devicetree

Some qcom platforms make some GPIOs or pins unavailable for use
by non-secure operating systems, and thus reading or writing the
registers for those pins will cause access control issues and
reset the device. With a DT/ACPI property to describe the set of
pins that are available for use, parse the available pins and set
the irq valid bits for gpiolib to know what to consider 'valid'.
This should avoid any issues with gpiolib. Furthermore, implement
the pinmux_ops::request function so that pinmux can also make
sure to not use pins that are unavailable.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 98 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 94 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 7a960590ecaa..4a251268ebc4 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -105,6 +105,17 @@ static const struct pinctrl_ops msm_pinctrl_ops = {
 	.dt_free_map		= pinctrl_utils_free_map,
 };
 
+static int msm_pinmux_request(struct pinctrl_dev *pctldev, unsigned offset)
+{
+	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	struct gpio_chip *chip = &pctrl->chip;
+
+	if (gpiochip_irqchip_irq_valid(chip, offset))
+		return 0;
+
+	return -EINVAL;
+}
+
 static int msm_get_functions_count(struct pinctrl_dev *pctldev)
 {
 	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
@@ -166,6 +177,7 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
 }
 
 static const struct pinmux_ops msm_pinmux_ops = {
+	.request		= msm_pinmux_request,
 	.get_functions_count	= msm_get_functions_count,
 	.get_function_name	= msm_get_function_name,
 	.get_function_groups	= msm_get_function_groups,
@@ -506,6 +518,9 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
 		"pull up"
 	};
 
+	if (!gpiochip_irqchip_irq_valid(chip, offset))
+		return;
+
 	g = &pctrl->soc->groups[offset];
 	ctl_reg = readl(pctrl->regs + g->ctl_reg);
 
@@ -516,7 +531,7 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
 
 	seq_printf(s, " %-8s: %-3s %d", g->name, is_out ? "out" : "in", func);
 	seq_printf(s, " %dmA", msm_regval_to_drive(drive));
-	seq_printf(s, " %s", pulls[pull]);
+	seq_printf(s, " %s\n", pulls[pull]);
 }
 
 static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
@@ -524,10 +539,8 @@ static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	unsigned gpio = chip->base;
 	unsigned i;
 
-	for (i = 0; i < chip->ngpio; i++, gpio++) {
+	for (i = 0; i < chip->ngpio; i++, gpio++)
 		msm_gpio_dbg_show_one(s, NULL, chip, i, gpio);
-		seq_puts(s, "\n");
-	}
 }
 
 #else
@@ -808,6 +821,76 @@ static void msm_gpio_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+static int msm_gpio_init_irq_valid_mask(struct gpio_chip *chip,
+					struct msm_pinctrl *pctrl)
+{
+	int ret;
+	unsigned int len, i;
+	unsigned int max_gpios = pctrl->soc->ngpios;
+	struct device_node *np = pctrl->dev->of_node;
+
+	/* The number of GPIOs in the ACPI tables */
+	ret = device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0);
+	if (ret > 0 && ret < max_gpios) {
+		u16 *tmp;
+
+		len = ret;
+		tmp = kmalloc_array(len, sizeof(tmp[0]), GFP_KERNEL);
+		if (!tmp)
+			return -ENOMEM;
+
+		ret = device_property_read_u16_array(pctrl->dev, "gpios", tmp,
+						     len);
+		if (ret < 0) {
+			dev_err(pctrl->dev, "could not read list of GPIOs\n");
+			kfree(tmp);
+			return ret;
+		}
+
+		bitmap_zero(chip->irq_valid_mask, max_gpios);
+		for (i = 0; i < len; i++)
+			set_bit(tmp[i], chip->irq_valid_mask);
+
+		return 0;
+	}
+
+	/* If there's a DT ngpios-ranges property then add those ranges */
+	ret = of_property_count_u32_elems(np,  "ngpios-ranges");
+	if (ret > 0 && ret % 2 == 0 && ret / 2 < max_gpios) {
+		u32 start;
+		u32 count;
+
+		len = ret / 2;
+		bitmap_zero(chip->irq_valid_mask, max_gpios);
+
+		for (i = 0; i < len; i++) {
+			of_property_read_u32_index(np, "ngpios-ranges",
+						   i * 2, &start);
+			of_property_read_u32_index(np, "ngpios-ranges",
+						   i * 2 + 1, &count);
+			bitmap_set(chip->irq_valid_mask, start, count);
+		}
+	}
+
+	return 0;
+}
+
+static bool msm_gpio_needs_irq_valid_mask(struct msm_pinctrl *pctrl)
+{
+	int ret;
+	struct device_node *np = pctrl->dev->of_node;
+
+	ret = device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0);
+	if (ret > 0)
+		return true;
+
+	ret = of_property_count_u32_elems(np,  "ngpios-ranges");
+	if (ret > 0 && ret % 2 == 0)
+		return true;
+
+	return false;
+}
+
 static int msm_gpio_init(struct msm_pinctrl *pctrl)
 {
 	struct gpio_chip *chip;
@@ -824,6 +907,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	chip->parent = pctrl->dev;
 	chip->owner = THIS_MODULE;
 	chip->of_node = pctrl->dev->of_node;
+	chip->irq_need_valid_mask = msm_gpio_needs_irq_valid_mask(pctrl);
 
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
@@ -831,6 +915,12 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 		return ret;
 	}
 
+	ret = msm_gpio_init_irq_valid_mask(chip, pctrl);
+	if (ret) {
+		dev_err(pctrl->dev, "Failed to setup irq valid bits\n");
+		return ret;
+	}
+
 	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
 	if (ret) {
 		dev_err(pctrl->dev, "Failed to add pin range\n");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/3] pinctrl: qcom: Don't allow protected pins to be requested
  2018-01-10  1:58 ` [PATCH 3/3] pinctrl: qcom: Don't allow protected pins to be requested Stephen Boyd
@ 2018-01-10  6:11   ` Bjorn Andersson
  2018-01-22 13:55   ` Timur Tabi
  2018-01-25 20:48   ` Timur Tabi
  2 siblings, 0 replies; 19+ messages in thread
From: Bjorn Andersson @ 2018-01-10  6:11 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Linus Walleij, linux-kernel, linux-arm-msm, linux-arm-kernel,
	Timur Tabi, Andy Shevchenko, linux-gpio, devicetree

On Tue 09 Jan 17:58 PST 2018, Stephen Boyd wrote:

I like it, a few comment below though.

> +static int msm_gpio_init_irq_valid_mask(struct gpio_chip *chip,
> +					struct msm_pinctrl *pctrl)
> +{
> +	int ret;
> +	unsigned int len, i;
> +	unsigned int max_gpios = pctrl->soc->ngpios;
> +	struct device_node *np = pctrl->dev->of_node;
> +
> +	/* The number of GPIOs in the ACPI tables */
> +	ret = device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0);
> +	if (ret > 0 && ret < max_gpios) {
> +		u16 *tmp;
> +
> +		len = ret;
> +		tmp = kmalloc_array(len, sizeof(tmp[0]), GFP_KERNEL);
> +		if (!tmp)
> +			return -ENOMEM;
> +
> +		ret = device_property_read_u16_array(pctrl->dev, "gpios", tmp,
> +						     len);
> +		if (ret < 0) {
> +			dev_err(pctrl->dev, "could not read list of GPIOs\n");
> +			kfree(tmp);
> +			return ret;
> +		}
> +
> +		bitmap_zero(chip->irq_valid_mask, max_gpios);

The valid_mask is moving into the gpio_irq_chip, so this should be
chip->irq.valid_mask.

See dc7b0387ee89 ("gpio: Move irq_valid_mask into struct gpio_irq_chip")

> +		for (i = 0; i < len; i++)
> +			set_bit(tmp[i], chip->irq_valid_mask);
> +

You're leaking tmp here.

> +		return 0;
> +	}
> +
> +	/* If there's a DT ngpios-ranges property then add those ranges */
> +	ret = of_property_count_u32_elems(np,  "ngpios-ranges");
> +	if (ret > 0 && ret % 2 == 0 && ret / 2 < max_gpios) {
> +		u32 start;
> +		u32 count;
> +
> +		len = ret / 2;
> +		bitmap_zero(chip->irq_valid_mask, max_gpios);
> +
> +		for (i = 0; i < len; i++) {

If you take steps of 2, when looping from 0 to ret, your loop index will
have the value that you're passing as index into the read - without
duplicating it.

> +			of_property_read_u32_index(np, "ngpios-ranges",
> +						   i * 2, &start);
> +			of_property_read_u32_index(np, "ngpios-ranges",
> +						   i * 2 + 1, &count);
> +			bitmap_set(chip->irq_valid_mask, start, count);
> +		}
> +	}
> +
> +	return 0;
> +}
[..]
> @@ -824,6 +907,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>  	chip->parent = pctrl->dev;
>  	chip->owner = THIS_MODULE;
>  	chip->of_node = pctrl->dev->of_node;
> +	chip->irq_need_valid_mask = msm_gpio_needs_irq_valid_mask(pctrl);

chip->irq.need_valid_mask

>  
>  	ret = gpiochip_add_data(&pctrl->chip, pctrl);
>  	if (ret) {
> @@ -831,6 +915,12 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>  		return ret;
>  	}
>  
> +	ret = msm_gpio_init_irq_valid_mask(chip, pctrl);
> +	if (ret) {
> +		dev_err(pctrl->dev, "Failed to setup irq valid bits\n");

gpiochip_remove() here, to release resources allocated by
gpiochip_add_data.

> +		return ret;
> +	}
> +
>  	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
>  	if (ret) {
>  		dev_err(pctrl->dev, "Failed to add pin range\n");

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/3] gpiolib: Export gpiochip_irqchip_irq_valid() to drivers
  2018-01-10  1:58   ` [PATCH 1/3] gpiolib: Export gpiochip_irqchip_irq_valid() to drivers Stephen Boyd
@ 2018-01-10  6:16     ` Bjorn Andersson
  2018-01-10 13:22     ` Linus Walleij
  1 sibling, 0 replies; 19+ messages in thread
From: Bjorn Andersson @ 2018-01-10  6:16 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Linus Walleij, linux-kernel, linux-arm-msm, linux-arm-kernel,
	Timur Tabi, Andy Shevchenko, linux-gpio, devicetree

On Tue 09 Jan 17:58 PST 2018, Stephen Boyd wrote:

> Some pinctrl drivers can use the gpiochip irq valid information
> to figure out if certain gpios are exposed to the kernel for
> usage or not. Expose this API so we can use it in the
> pinmux_ops::request ops.
> 
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/gpio/gpiolib.c      | 5 +++--
>  include/linux/gpio/driver.h | 3 +++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index b80936a25caa..c18b7b60ea1d 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1503,14 +1503,15 @@ static void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gpiochip)
>  	gpiochip->irq.valid_mask = NULL;
>  }
>  
> -static bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip,
> -				       unsigned int offset)
> +bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip,
> +				unsigned int offset)
>  {
>  	/* No mask means all valid */
>  	if (likely(!gpiochip->irq.valid_mask))
>  		return true;
>  	return test_bit(offset, gpiochip->irq.valid_mask);
>  }
> +EXPORT_SYMBOL_GPL(gpiochip_irqchip_irq_valid);
>  
>  /**
>   * gpiochip_set_cascaded_irqchip() - connects a cascaded irqchip to a gpiochip
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index 7258cd676df4..1ba9a331ec51 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -436,6 +436,9 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
>  			     struct lock_class_key *lock_key,
>  			     struct lock_class_key *request_key);
>  
> +bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip,
> +				unsigned int offset);
> +
>  #ifdef CONFIG_LOCKDEP
>  
>  /*
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/3] dt-bindings: pinctrl: Add a ngpios-ranges property
  2018-01-10  1:58 ` [PATCH 2/3] dt-bindings: pinctrl: Add a ngpios-ranges property Stephen Boyd
@ 2018-01-10 12:54   ` Andy Shevchenko
       [not found]   ` <20180110015848.11480-3-sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
  1 sibling, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2018-01-10 12:54 UTC (permalink / raw)
  To: Stephen Boyd, Linus Walleij
  Cc: linux-kernel, linux-arm-msm, linux-arm-kernel, Timur Tabi,
	Bjorn Andersson, linux-gpio, devicetree

On Tue, 2018-01-09 at 17:58 -0800, Stephen Boyd wrote:
> Some qcom platforms make some GPIOs or pins unavailable for use
> by non-secure operating systems, and thus reading or writing the
> registers for those pins will cause access control issues.
> Introduce a DT property to describe the set of GPIOs that are
> available for use so that higher level OSes are able to know what
> pins to avoid reading/writing.
 
> +- ngpios-ranges:
> +	Usage: optional
> +	Value type: <prop-encoded-array>
> +	Definition: Tuples of GPIO ranges (base, size) indicating
> +		    GPIOs available for use.

Can be name more particular?

We have on one hand gpio-range-list for mapping, on the other this one
might become generic.

So, there are few options (at least?) to consider:
1) re-use gpio-ranges
2) add a valid property to gpio-ranges
3) rename ngpios-ranges to something like gpio-valid-ranges (I don't
like it so much either, but for me it looks more descriptive than
ngpios-ranges)


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/3] gpiolib: Export gpiochip_irqchip_irq_valid() to drivers
  2018-01-10  1:58   ` [PATCH 1/3] gpiolib: Export gpiochip_irqchip_irq_valid() to drivers Stephen Boyd
  2018-01-10  6:16     ` Bjorn Andersson
@ 2018-01-10 13:22     ` Linus Walleij
  1 sibling, 0 replies; 19+ messages in thread
From: Linus Walleij @ 2018-01-10 13:22 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: linux-kernel, linux-arm-msm, Linux ARM, Timur Tabi,
	Andy Shevchenko, Bjorn Andersson, linux-gpio,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Wed, Jan 10, 2018 at 2:58 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:

> Some pinctrl drivers can use the gpiochip irq valid information
> to figure out if certain gpios are exposed to the kernel for
> usage or not. Expose this API so we can use it in the
> pinmux_ops::request ops.
>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>

Makes a lot of sense.

Patch applied with Björn's ACK.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/3] dt-bindings: pinctrl: Add a ngpios-ranges property
       [not found]   ` <20180110015848.11480-3-sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
@ 2018-01-10 13:37     ` Linus Walleij
  2018-01-10 16:37       ` Stephen Boyd
  2018-01-11 16:33       ` Grant Likely
  0 siblings, 2 replies; 19+ messages in thread
From: Linus Walleij @ 2018-01-10 13:37 UTC (permalink / raw)
  To: Stephen Boyd, Rob Herring, Grant Likely
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, Linux ARM, Timur Tabi,
	Andy Shevchenko, Bjorn Andersson,
	linux-gpio-u79uwXL29TY76Z2rM5mHXA,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Wed, Jan 10, 2018 at 2:58 AM, Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:

> Some qcom platforms make some GPIOs or pins unavailable for use
> by non-secure operating systems, and thus reading or writing the
> registers for those pins will cause access control issues.
> Introduce a DT property to describe the set of GPIOs that are
> available for use so that higher level OSes are able to know what
> pins to avoid reading/writing.
>
> Cc: <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Signed-off-by: Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>

I like the idea, let's check what we think about the details regarding
naming and semantics, I need feedback from some DT people
in particular.

Paging in Grant on this as he might have some input.

> I stuck this inside msm8996, but maybe it can go somewhere more generic?

Yeah just put it in Documentation/devicetree/bindings/gpio/gpio.txt
Everyone and its dog doing GPIO reservations "from another world"
will need to use this.

> +- ngpios-ranges:
> +       Usage: optional
> +       Value type: <prop-encoded-array>
> +       Definition: Tuples of GPIO ranges (base, size) indicating
> +                   GPIOs available for use.
> +
>  Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for
>  a general description of GPIO and interrupt bindings.

I like the tuples syntax. That's fine. It's like gpio-ranges we have
already to map between pin controllers and GPIO.

I don't think we can reuse gpio-ranges because that is
exclusively for pin control ATM, it would be fine if the ranges
were for a specific device, like pin control does, like:

gpio-ranges = <&secure_world_thing 0 20 10>;

But you definately would need a node to tie it to, so that the
driver for that node can specify that it's gonna take the
GPIOs.

But I think the semantics should be the inverse. That you
point out "holes" with the lines we *can't* use.

We already support a generic property "ngpios" that says how
many of the GPIOs (counted from zero) that can be used,
so if those should be able to use this as a generic property it
is better with the inverse semantics and say that the
"reserved-gpio-ranges", "secureworld-gpio-ranges"
(or whatever we decide to call it) takes precedence over
ngpios so we don't end up in ambigous places.

Then, will it be possible to put the parsing, handling and
disablement of these ranges into drivers/gpio/gpiolib-of.c
where we handle the ranges today, or do we need to
do it in the individual drivers?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/3] dt-bindings: pinctrl: Add a ngpios-ranges property
  2018-01-10 13:37     ` Linus Walleij
@ 2018-01-10 16:37       ` Stephen Boyd
  2018-01-10 17:59         ` Andy Shevchenko
  2018-01-11 16:33       ` Grant Likely
  1 sibling, 1 reply; 19+ messages in thread
From: Stephen Boyd @ 2018-01-10 16:37 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Rob Herring, Grant Likely, linux-kernel, linux-arm-msm,
	Linux ARM, Timur Tabi, Andy Shevchenko, Bjorn Andersson,
	linux-gpio,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On 01/10, Linus Walleij wrote:
> On Wed, Jan 10, 2018 at 2:58 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> 
> > +- ngpios-ranges:
> > +       Usage: optional
> > +       Value type: <prop-encoded-array>
> > +       Definition: Tuples of GPIO ranges (base, size) indicating
> > +                   GPIOs available for use.
> > +
> >  Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for
> >  a general description of GPIO and interrupt bindings.
> 
> I like the tuples syntax. That's fine. It's like gpio-ranges we have
> already to map between pin controllers and GPIO.
> 
> I don't think we can reuse gpio-ranges because that is
> exclusively for pin control ATM, it would be fine if the ranges
> were for a specific device, like pin control does, like:
> 
> gpio-ranges = <&secure_world_thing 0 20 10>;
> 
> But you definately would need a node to tie it to, so that the
> driver for that node can specify that it's gonna take the
> GPIOs.
> 
> But I think the semantics should be the inverse. That you
> point out "holes" with the lines we *can't* use.

Ok. I can invert the logic and push it into the core part of the
code. I'll leave the ACPI part in the msm driver.

> 
> We already support a generic property "ngpios" that says how
> many of the GPIOs (counted from zero) that can be used,
> so if those should be able to use this as a generic property it
> is better with the inverse semantics and say that the
> "reserved-gpio-ranges", "secureworld-gpio-ranges"
> (or whatever we decide to call it) takes precedence over
> ngpios so we don't end up in ambigous places.
> 
> Then, will it be possible to put the parsing, handling and
> disablement of these ranges into drivers/gpio/gpiolib-of.c
> where we handle the ranges today, or do we need to
> do it in the individual drivers?
> 

I'll cook that up right now to do the inverse thing in the
gpiolib core code with a 'reserved-gpio-ranges' property. I
haven't looked in much detail, but I would hope that it would
work pretty easily. Should it be decoupled from the
GPIOLIB_IRQCHIP config? If the idea is generic, then it may not
be related to irq lines, but for the qcom driver it was all fine
because all three concepts: irq, gpios, and pins have a one to
one relationship. The only place it breaks down is if we have
more pins than gpios, in which case I punted and just considered
non-gpio pins as always valid.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/3] dt-bindings: pinctrl: Add a ngpios-ranges property
  2018-01-10 16:37       ` Stephen Boyd
@ 2018-01-10 17:59         ` Andy Shevchenko
  0 siblings, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2018-01-10 17:59 UTC (permalink / raw)
  To: Stephen Boyd, Linus Walleij
  Cc: Rob Herring, Grant Likely, linux-kernel, linux-arm-msm,
	Linux ARM, Timur Tabi, Bjorn Andersson, linux-gpio,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Wed, 2018-01-10 at 08:37 -0800, Stephen Boyd wrote:
> On 01/10, Linus Walleij wrote:
> > On Wed, Jan 10, 2018 at 2:58 AM, Stephen Boyd <sboyd@codeaurora.org>
> > wrote:

> for the qcom driver it was all fine
> because all three concepts: irq, gpios, and pins have a one to
> one relationship.

Just a side note: While it might be the case for most of the
controllers, don't assume it's a generic case.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/3] dt-bindings: pinctrl: Add a ngpios-ranges property
  2018-01-10 13:37     ` Linus Walleij
  2018-01-10 16:37       ` Stephen Boyd
@ 2018-01-11 16:33       ` Grant Likely
  2018-01-11 16:36         ` Timur Tabi
  1 sibling, 1 reply; 19+ messages in thread
From: Grant Likely @ 2018-01-11 16:33 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Stephen Boyd, Rob Herring, linux-kernel, linux-arm-msm,
	Linux ARM, Timur Tabi, Andy Shevchenko, Bjorn Andersson,
	linux-gpio,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Wed, Jan 10, 2018 at 1:37 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Jan 10, 2018 at 2:58 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>
>> Some qcom platforms make some GPIOs or pins unavailable for use
>> by non-secure operating systems, and thus reading or writing the
>> registers for those pins will cause access control issues.
>> Introduce a DT property to describe the set of GPIOs that are
>> available for use so that higher level OSes are able to know what
>> pins to avoid reading/writing.

What level of access control is implemented here? Is there access
control for each GPIO individually, or is it done by banks of GPIOs?
Just asking to make sure I understand the problem domain.

>>
>> Cc: <devicetree@vger.kernel.org>
>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>
> I like the idea, let's check what we think about the details regarding
> naming and semantics, I need feedback from some DT people
> in particular.
>
> Paging in Grant on this as he might have some input.
>
>> I stuck this inside msm8996, but maybe it can go somewhere more generic?
>
> Yeah just put it in Documentation/devicetree/bindings/gpio/gpio.txt
> Everyone and its dog doing GPIO reservations "from another world"
> will need to use this.
>
>> +- ngpios-ranges:
>> +       Usage: optional
>> +       Value type: <prop-encoded-array>
>> +       Definition: Tuples of GPIO ranges (base, size) indicating
>> +                   GPIOs available for use.
>> +
>>  Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for
>>  a general description of GPIO and interrupt bindings.
>
> I like the tuples syntax. That's fine. It's like gpio-ranges we have
> already to map between pin controllers and GPIO.
>
> I don't think we can reuse gpio-ranges because that is
> exclusively for pin control ATM, it would be fine if the ranges
> were for a specific device, like pin control does, like:
>
> gpio-ranges = <&secure_world_thing 0 20 10>;
>
> But you definately would need a node to tie it to, so that the
> driver for that node can specify that it's gonna take the
> GPIOs.
>
> But I think the semantics should be the inverse. That you
> point out "holes" with the lines we *can't* use.
>
> We already support a generic property "ngpios" that says how
> many of the GPIOs (counted from zero) that can be used,
> so if those should be able to use this as a generic property it
> is better with the inverse semantics and say that the
> "reserved-gpio-ranges", "secureworld-gpio-ranges"
> (or whatever we decide to call it) takes precedence over
> ngpios so we don't end up in ambigous places.

Heh, I just went down the same thought process on the naming before I
read the above. Yes I agree. The property name should have something
like "reserved" in it. I vote for "gpio-reserved-ranges" because it
puts the binding owner (gpio) at the front of the name, it indicates
that the list is unavailable GPIOs, and that the format is a set of
ranges.

The fiddly bit is it assumes the GPIOs are described by a single
number. It works fine as long as the GPIO controllers can use a single
cell to describe a gpio number (instead of having #gpio-cells = 3 with
the first cell being bank, the second being number in bank, and the
third being flags).

>
> Then, will it be possible to put the parsing, handling and
> disablement of these ranges into drivers/gpio/gpiolib-of.c
> where we handle the ranges today, or do we need to
> do it in the individual drivers?

I certainly would prefer parsing this in common code, and not in
individual drivers, but again it becomes hard for any driver using
multiple cells to describe the local GPIO number. I think the guidance
here needs to be that the property is relevant when the internal GPIO
number representation fits within a uint32, which realistically should
never be a problem.

g.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/3] dt-bindings: pinctrl: Add a ngpios-ranges property
  2018-01-11 16:33       ` Grant Likely
@ 2018-01-11 16:36         ` Timur Tabi
  2018-01-11 19:56           ` Grant Likely
  0 siblings, 1 reply; 19+ messages in thread
From: Timur Tabi @ 2018-01-11 16:36 UTC (permalink / raw)
  To: Grant Likely, Linus Walleij
  Cc: Stephen Boyd, Rob Herring, linux-kernel, linux-arm-msm,
	Linux ARM, Andy Shevchenko, Bjorn Andersson, linux-gpio,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On 01/11/2018 10:33 AM, Grant Likely wrote:
> What level of access control is implemented here? Is there access
> control for each GPIO individually, or is it done by banks of GPIOs?
> Just asking to make sure I understand the problem domain.

On our ACPI system, it's specific GPIOs.  Each GPIO is in its own 64k 
page, which is what allows us to block specific ones.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/3] dt-bindings: pinctrl: Add a ngpios-ranges property
  2018-01-11 16:36         ` Timur Tabi
@ 2018-01-11 19:56           ` Grant Likely
  0 siblings, 0 replies; 19+ messages in thread
From: Grant Likely @ 2018-01-11 19:56 UTC (permalink / raw)
  To: Timur Tabi
  Cc: Linus Walleij, Stephen Boyd, Rob Herring, linux-kernel,
	linux-arm-msm, Linux ARM, Andy Shevchenko, Bjorn Andersson,
	linux-gpio,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Thu, Jan 11, 2018 at 4:36 PM, Timur Tabi <timur@codeaurora.org> wrote:
> On 01/11/2018 10:33 AM, Grant Likely wrote:
>>
>> What level of access control is implemented here? Is there access
>> control for each GPIO individually, or is it done by banks of GPIOs?
>> Just asking to make sure I understand the problem domain.
>
>
> On our ACPI system, it's specific GPIOs.  Each GPIO is in its own 64k page,
> which is what allows us to block specific ones.

Okay, thanks.

g.

>
> --
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
> Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/3] pinctrl: qcom: Don't allow protected pins to be requested
  2018-01-10  1:58 ` [PATCH 3/3] pinctrl: qcom: Don't allow protected pins to be requested Stephen Boyd
  2018-01-10  6:11   ` Bjorn Andersson
@ 2018-01-22 13:55   ` Timur Tabi
  2018-01-22 20:03     ` Timur Tabi
  2018-01-25 21:51     ` Stephen Boyd
  2018-01-25 20:48   ` Timur Tabi
  2 siblings, 2 replies; 19+ messages in thread
From: Timur Tabi @ 2018-01-22 13:55 UTC (permalink / raw)
  To: Stephen Boyd, Linus Walleij
  Cc: linux-kernel, linux-arm-msm, linux-arm-kernel, Andy Shevchenko,
	Bjorn Andersson, linux-gpio, devicetree

On 1/9/18 7:58 PM, Stephen Boyd wrote:
> +		ret = device_property_read_u16_array(pctrl->dev, "gpios", tmp,
> +						     len);
> +		if (ret < 0) {
> +			dev_err(pctrl->dev, "could not read list of GPIOs\n");
> +			kfree(tmp);
> +			return ret;
> +		}

Just FYI, I'm still going to have to parse "gpios" in my 
pinctrl-qdf2xxx.c driver, even though you're also parsing it here. 
That's because I need to make sure that the msm_pingroup array only 
contains "approve" addresses in its ctl_reg fields.

+	for (i = 0; i < avail_gpios; i++) {
+		unsigned int gpio = gpios[i];
+
+		groups[gpio].npins = 1;
+		snprintf(names[i], NAME_SIZE, "gpio%u", gpio);
+		pins[gpio].name = names[i];
+		groups[gpio].name = names[i];
+
+		groups[gpio].ctl_reg = 0x10000 * gpio;
  		       ^^^^

I do this because I need to make sure that "unapproved" physical 
addresses are never store anywhere in groups[].  That way, it's 
impossible for the driver to cause an XPU violation -- the worst that 
can happen is a null pointer dereference.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/3] pinctrl: qcom: Don't allow protected pins to be requested
  2018-01-22 13:55   ` Timur Tabi
@ 2018-01-22 20:03     ` Timur Tabi
  2018-01-25 21:51     ` Stephen Boyd
  1 sibling, 0 replies; 19+ messages in thread
From: Timur Tabi @ 2018-01-22 20:03 UTC (permalink / raw)
  To: Stephen Boyd, Linus Walleij
  Cc: devicetree, linux-arm-msm, linux-kernel, Bjorn Andersson,
	linux-gpio, Andy Shevchenko, linux-arm-kernel

On 01/22/2018 07:55 AM, Timur Tabi wrote:
> 
> Just FYI, I'm still going to have to parse "gpios" in my 
> pinctrl-qdf2xxx.c driver, even though you're also parsing it here. 
> That's because I need to make sure that the msm_pingroup array only 
> contains "approve" addresses in its ctl_reg fields.

Also, my patch

[PATCH 3/3] [v7] pinctrl: qcom: qdf2xxx: add support for new ACPI HID 
QCOM8002

Applies on top of your patches as-is.

Also, what about

[PATCH 1/3] [v2] Revert "gpio: set up initial state from .get_direction()"

I think you still need this patch.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/3] pinctrl: qcom: Don't allow protected pins to be requested
  2018-01-10  1:58 ` [PATCH 3/3] pinctrl: qcom: Don't allow protected pins to be requested Stephen Boyd
  2018-01-10  6:11   ` Bjorn Andersson
  2018-01-22 13:55   ` Timur Tabi
@ 2018-01-25 20:48   ` Timur Tabi
  2 siblings, 0 replies; 19+ messages in thread
From: Timur Tabi @ 2018-01-25 20:48 UTC (permalink / raw)
  To: Stephen Boyd, Linus Walleij
  Cc: linux-kernel, linux-arm-msm, linux-arm-kernel, Andy Shevchenko,
	Bjorn Andersson, linux-gpio, devicetree

On 01/09/2018 07:58 PM, Stephen Boyd wrote:
> +static int msm_pinmux_request(struct pinctrl_dev *pctldev, unsigned offset)

"unsigned int", instead of just "unsigned"?

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/3] pinctrl: qcom: Don't allow protected pins to be requested
  2018-01-22 13:55   ` Timur Tabi
  2018-01-22 20:03     ` Timur Tabi
@ 2018-01-25 21:51     ` Stephen Boyd
  2018-01-25 21:53       ` Timur Tabi
  1 sibling, 1 reply; 19+ messages in thread
From: Stephen Boyd @ 2018-01-25 21:51 UTC (permalink / raw)
  To: Timur Tabi
  Cc: Linus Walleij, linux-kernel, linux-arm-msm, linux-arm-kernel,
	Andy Shevchenko, Bjorn Andersson, linux-gpio, devicetree

On 01/22, Timur Tabi wrote:
> On 1/9/18 7:58 PM, Stephen Boyd wrote:
> >+		ret = device_property_read_u16_array(pctrl->dev, "gpios", tmp,
> >+						     len);
> >+		if (ret < 0) {
> >+			dev_err(pctrl->dev, "could not read list of GPIOs\n");
> >+			kfree(tmp);
> >+			return ret;
> >+		}
> 
> Just FYI, I'm still going to have to parse "gpios" in my
> pinctrl-qdf2xxx.c driver, even though you're also parsing it here.
> That's because I need to make sure that the msm_pingroup array only
> contains "approve" addresses in its ctl_reg fields.
> 
> +	for (i = 0; i < avail_gpios; i++) {
> +		unsigned int gpio = gpios[i];
> +
> +		groups[gpio].npins = 1;
> +		snprintf(names[i], NAME_SIZE, "gpio%u", gpio);
> +		pins[gpio].name = names[i];
> +		groups[gpio].name = names[i];
> +
> +		groups[gpio].ctl_reg = 0x10000 * gpio;
>  		       ^^^^
> 
> I do this because I need to make sure that "unapproved" physical
> addresses are never store anywhere in groups[].  That way, it's
> impossible for the driver to cause an XPU violation -- the worst
> that can happen is a null pointer dereference.
> 

Sorry I don't get it. Is that some sort of hardening requirement?
If the framework doesn't cause those pins to be touched I fail to
see how it could hurt to have the other addresses listed. I'm
sure with some effort protected addresses could be crafted in
other ways to cause an XPU violation to the same place.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/3] pinctrl: qcom: Don't allow protected pins to be requested
  2018-01-25 21:51     ` Stephen Boyd
@ 2018-01-25 21:53       ` Timur Tabi
  0 siblings, 0 replies; 19+ messages in thread
From: Timur Tabi @ 2018-01-25 21:53 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Linus Walleij, linux-kernel, linux-arm-msm, linux-arm-kernel,
	Andy Shevchenko, Bjorn Andersson, linux-gpio, devicetree

On 01/25/2018 03:51 PM, Stephen Boyd wrote:
> Sorry I don't get it. Is that some sort of hardening requirement?
> If the framework doesn't cause those pins to be touched I fail to
> see how it could hurt to have the other addresses listed. I'm
> sure with some effort protected addresses could be crafted in
> other ways to cause an XPU violation to the same place.

It's for my own sanity.  By ensuring that those physical addresses are 
not ever present in the driver or any data structure, I can fend off, 
"Hey Timur, your gpio driver is causing XPU violations again, heh heh".

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2018-01-25 21:53 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-10  1:58 [PATCH 0/3] Support qcom pinctrl protected pins Stephen Boyd
     [not found] ` <20180110015848.11480-1-sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2018-01-10  1:58   ` [PATCH 1/3] gpiolib: Export gpiochip_irqchip_irq_valid() to drivers Stephen Boyd
2018-01-10  6:16     ` Bjorn Andersson
2018-01-10 13:22     ` Linus Walleij
2018-01-10  1:58 ` [PATCH 2/3] dt-bindings: pinctrl: Add a ngpios-ranges property Stephen Boyd
2018-01-10 12:54   ` Andy Shevchenko
     [not found]   ` <20180110015848.11480-3-sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2018-01-10 13:37     ` Linus Walleij
2018-01-10 16:37       ` Stephen Boyd
2018-01-10 17:59         ` Andy Shevchenko
2018-01-11 16:33       ` Grant Likely
2018-01-11 16:36         ` Timur Tabi
2018-01-11 19:56           ` Grant Likely
2018-01-10  1:58 ` [PATCH 3/3] pinctrl: qcom: Don't allow protected pins to be requested Stephen Boyd
2018-01-10  6:11   ` Bjorn Andersson
2018-01-22 13:55   ` Timur Tabi
2018-01-22 20:03     ` Timur Tabi
2018-01-25 21:51     ` Stephen Boyd
2018-01-25 21:53       ` Timur Tabi
2018-01-25 20:48   ` Timur Tabi

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).