[v3,3/3] pinctrl: qcom: Don't allow protected pins to be requested
diff mbox series

Message ID 20180321165848.89751-4-swboyd@chromium.org
State New
Headers show
Series
  • Support qcom pinctrl protected pins
Related show

Commit Message

Stephen Boyd March 21, 2018, 4:58 p.m. UTC
From: Stephen Boyd <sboyd@codeaurora.org>

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>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 69 ++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 4 deletions(-)

Comments

Andy Shevchenko March 21, 2018, 6:07 p.m. UTC | #1
On Wed, 2018-03-21 at 09:58 -0700, Stephen Boyd wrote:
> From: Stephen Boyd <sboyd@codeaurora.org>
> 
> 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>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Hmm...

> +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_line_is_valid(chip, offset))
> +		return 0;
> +
> +	return -EINVAL;

Perhaps traditional pattern

if (!...)
 return -EINVAL;

return 0;

?

> +}

>  	seq_printf(s, " %dmA", msm_regval_to_drive(drive));
> -	seq_printf(s, " %s", pulls[pull]);
> +	seq_printf(s, " %s\n", pulls[pull]);

I had commented this once, but you ignored by some reason.

I would rather just move 
 seq_puts(s, "\n");
here.

The rationale behind, besides making diff more neat, is to reduce
possible burden in the future if someone would like to squeeze more data
in between.

> +		tmp = kmalloc_array(len, sizeof(tmp[0]), GFP_KERNEL);

sizeof(*tmp) ?

> +		if (!tmp)
> +			return -ENOMEM;
Stephen Boyd March 21, 2018, 8:04 p.m. UTC | #2
Quoting Andy Shevchenko (2018-03-21 11:07:09)
> On Wed, 2018-03-21 at 09:58 -0700, Stephen Boyd wrote:
> > +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_line_is_valid(chip, offset))
> > +             return 0;
> > +
> > +     return -EINVAL;
> 
> Perhaps traditional pattern
> 
> if (!...)
>  return -EINVAL;
> 
> return 0;
> 

Or ternary?

  return gpiochip_line_is_valid(chip, offset) ? 0 : -EINVAL;

> 
> > +}
> 
> >       seq_printf(s, " %dmA", msm_regval_to_drive(drive));
> > -     seq_printf(s, " %s", pulls[pull]);
> > +     seq_printf(s, " %s\n", pulls[pull]);
> 
> I had commented this once, but you ignored by some reason.
> 
> I would rather just move 
>  seq_puts(s, "\n");
> here.
> 
> The rationale behind, besides making diff more neat, is to reduce
> possible burden in the future if someone would like to squeeze more data
> in between.

Sure.

> 
> > +             tmp = kmalloc_array(len, sizeof(tmp[0]), GFP_KERNEL);
> 
> sizeof(*tmp) ?
> 

Ok.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd March 21, 2018, 8:17 p.m. UTC | #3
Quoting Stephen Boyd (2018-03-21 09:58:48)
> +       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->valid_mask, max_gpios);
> +               for (i = 0; i < len; i++)
> +                       set_bit(tmp[i], chip->valid_mask);

This leaks tmp too.. I'll fix that in v4 and resend tomorrow if there
aren't more comments.

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko March 21, 2018, 8:30 p.m. UTC | #4
On Wed, 2018-03-21 at 13:04 -0700, Stephen Boyd wrote:
> Quoting Andy Shevchenko (2018-03-21 11:07:09)
> > On Wed, 2018-03-21 at 09:58 -0700, Stephen Boyd wrote:
> > > 

> Or ternary?
> 
>   return gpiochip_line_is_valid(chip, offset) ? 0 : -EINVAL;

Fine with me!
Timur Tabi March 23, 2018, 12:16 a.m. UTC | #5
On 03/21/2018 11:58 AM, Stephen Boyd wrote:
> +static int msm_gpio_init_valid_mask(struct gpio_chip *chip,
> +				    struct msm_pinctrl *pctrl)
> +{
> +	int ret;
> +	unsigned int len, i;
> +	unsigned int max_gpios = pctrl->soc->ngpios;
> +
> +	/* 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) {

This needs to be ret <= max_gpios, otherwise it will fail if every GPIO 
is available.

And it should print an error message and return an error code if ret > 
max_gpios.
Timur Tabi March 23, 2018, 12:23 a.m. UTC | #6
On 03/22/2018 07:16 PM, Timur Tabi wrote:
> 
> This needs to be ret <= max_gpios, otherwise it will fail if every GPIO 
> is available.
> 
> And it should print an error message and return an error code if ret > 
> max_gpios.

Also, you don't allocate chip->valid_mask anywhere.
Timur Tabi March 23, 2018, 12:59 a.m. UTC | #7
On 03/22/2018 07:23 PM, Timur Tabi wrote:
> 
> Also, you don't allocate chip->valid_mask anywhere.

So I see now where it's allocated, but something is fishy.  I have three 
TLMMs on my chip:

[   67.107018] gpiochip_init_valid_mask:351 gpiochip->need_valid_mask=1
[   67.153747] gpiochip_init_valid_mask:356 gpiochip->ngpio=72
[   67.195324] gpiochip_init_valid_mask:361 
gpiochip->valid_mask=0000000070b1a4b6
[   68.532992] gpiochip_init_valid_mask:356 gpiochip->ngpio=44
[   68.574496] gpiochip_init_valid_mask:361 
gpiochip->valid_mask=000000002f33b8a3
[   68.709378] msm_gpio_init_valid_mask:837 ret=44 max_gpios=44 
chip->valid_mask=000000002f33b8a3
[   69.726502] gpiochip_init_valid_mask:351 gpiochip->need_valid_mask=1
[   69.772960] gpiochip_init_valid_mask:356 gpiochip->ngpio=54
[   69.814084] gpiochip_init_valid_mask:361 
gpiochip->valid_mask=000000001a53c932

Are these normal addresses for kcalloc() to return?  They're not even 
word-aligned.
Andy Shevchenko March 23, 2018, 11:34 a.m. UTC | #8
On Thu, 2018-03-22 at 19:59 -0500, Timur Tabi wrote:
> On 03/22/2018 07:23 PM, Timur Tabi wrote:
> > 
> > Also, you don't allocate chip->valid_mask anywhere.
> 
> So I see now where it's allocated, but something is fishy.

No, it seems you missed %p vs. %px discussion.

The pointers printed by %p nowadays are hashed values, not the original
ones.
Andy Shevchenko March 23, 2018, 11:36 a.m. UTC | #9
On Thu, 2018-03-22 at 19:16 -0500, Timur Tabi wrote:
> On 03/21/2018 11:58 AM, Stephen Boyd wrote:
> > +static int msm_gpio_init_valid_mask(struct gpio_chip *chip,
> > +				    struct msm_pinctrl *pctrl)
> > +{
> > +	int ret;
> > +	unsigned int len, i;
> > +	unsigned int max_gpios = pctrl->soc->ngpios;
> > +
> > +	/* 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) {
> 
> This needs to be ret <= max_gpios, otherwise it will fail if every
> GPIO 
> is available.
> 
> And it should print an error message and return an error code if ret
> > 
> max_gpios.

Perhaps makes sense to do the opposite condition

if (ret < 0 || ret > max_gpios) {
 ... error handling ...
}
Timur Tabi March 23, 2018, 2:03 p.m. UTC | #10
On 3/23/18 6:34 AM, Andy Shevchenko wrote:
> No, it seems you missed %p vs. %px discussion.
> 
> The pointers printed by %p nowadays are hashed values, not the original
> ones.

I totally forgot about that, thanks.
Stephen Boyd March 23, 2018, 4:21 p.m. UTC | #11
Quoting Andy Shevchenko (2018-03-23 04:36:04)
> On Thu, 2018-03-22 at 19:16 -0500, Timur Tabi wrote:
> > On 03/21/2018 11:58 AM, Stephen Boyd wrote:
> > > +static int msm_gpio_init_valid_mask(struct gpio_chip *chip,
> > > +                               struct msm_pinctrl *pctrl)
> > > +{
> > > +   int ret;
> > > +   unsigned int len, i;
> > > +   unsigned int max_gpios = pctrl->soc->ngpios;
> > > +
> > > +   /* 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) {
> > 
> > This needs to be ret <= max_gpios, otherwise it will fail if every
> > GPIO 
> > is available.
> > 
> > And it should print an error message and return an error code if ret
> > > 
> > max_gpios.
> 
> Perhaps makes sense to do the opposite condition
> 
> if (ret < 0 || ret > max_gpios) {
>  ... error handling ...
> }
> 

Indeed. I already rewrote it like this two days ago:

static int msm_gpio_init_valid_mask(struct gpio_chip *chip,
                                   struct msm_pinctrl *pctrl)
{
       int ret;
       unsigned int len, i;
       unsigned int max_gpios = pctrl->soc->ngpios;
       u16 *tmp;

       /* The number of GPIOs in the ACPI tables */
       len = ret = device_property_read_u16_array(pctrl->dev, "gpios",
ULL, 0);
       if (ret < 0)
               return 0;

       if (ret > max_gpios)
               return -EINVAL;

       tmp = kmalloc_array(len, sizeof(*tmp), GFP_KERNEL);
       if (!tmp)
               return -ENOMEM;

       ret = device_property_read_u16_array(pctrl->dev, "gpios", tmp,
en);
       if (ret < 0) {
               dev_err(pctrl->dev, "could not read list of GPIOs\n");
               goto out;
       }

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

out:
       kfree(tmp);
       return ret;
}

I'll send the updated patches now.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Timur Tabi March 23, 2018, 4:25 p.m. UTC | #12
On 03/23/2018 11:21 AM, Stephen Boyd wrote:
> I'll send the updated patches now.

Thanks.

BTW, I just discovered that the static globals in pinctrl-msm are 
breaking the ability to support more than one TLMM:

static struct pinctrl_desc msm_pinctrl_desc = {

static struct irq_chip msm_gpio_irq_chip = {

static struct msm_pinctrl *poweroff_pctrl;

I'm going to have to fix all of these.
Andy Shevchenko March 23, 2018, 4:31 p.m. UTC | #13
On Fri, Mar 23, 2018 at 6:21 PM, Stephen Boyd <swboyd@chromium.org> wrote:

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

Looking to this code I just realized it would be nice to have

{of,device}_property_read_bitmask() where bitmask_parse() is called.

Since it related to change a binding, I would really take couple of
days to hear other opinions.

In the above case, you need to supply a string, like

"1-5,16,18,25"
Andy Shevchenko March 23, 2018, 4:32 p.m. UTC | #14
On Fri, Mar 23, 2018 at 6:31 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Mar 23, 2018 at 6:21 PM, Stephen Boyd <swboyd@chromium.org> wrote:
>
>>        bitmap_zero(chip->valid_mask, max_gpios);
>>        for (i = 0; i < len; i++)
>>                set_bit(tmp[i], chip->valid_mask);
>
> Looking to this code I just realized it would be nice to have
>
> {of,device}_property_read_bitmask() where bitmask_parse() is called.
>
> Since it related to change a binding, I would really take couple of
> days to hear other opinions.
>
> In the above case, you need to supply a string, like
>
> "1-5,16,18,25"


s/bitmask/bitmap/g

Patch
diff mbox series

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 495432f3341b..bc9cda0da886 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_line_is_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_line_is_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,46 @@  static void msm_gpio_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+static int msm_gpio_init_valid_mask(struct gpio_chip *chip,
+				    struct msm_pinctrl *pctrl)
+{
+	int ret;
+	unsigned int len, i;
+	unsigned int max_gpios = pctrl->soc->ngpios;
+
+	/* 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->valid_mask, max_gpios);
+		for (i = 0; i < len; i++)
+			set_bit(tmp[i], chip->valid_mask);
+
+		return 0;
+	}
+
+	return 0;
+}
+
+static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
+{
+	return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
+}
+
 static int msm_gpio_init(struct msm_pinctrl *pctrl)
 {
 	struct gpio_chip *chip;
@@ -824,6 +877,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->need_valid_mask = msm_gpio_needs_valid_mask(pctrl);
 
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
@@ -831,6 +885,13 @@  static int msm_gpio_init(struct msm_pinctrl *pctrl)
 		return ret;
 	}
 
+	ret = msm_gpio_init_valid_mask(chip, pctrl);
+	if (ret) {
+		dev_err(pctrl->dev, "Failed to setup irq valid bits\n");
+		gpiochip_remove(&pctrl->chip);
+		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");