| Message ID | 20180110015848.11480-4-sboyd@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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.
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"?
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.
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".
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");
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(-)