| Message ID | 20180110015848.11480-1-sboyd@codeaurora.org |
|---|---|
| Headers | show |
| Series | Support qcom pinctrl protected pins | expand |
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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".