| Message ID | 20180321165848.89751-4-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
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.
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.
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!
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.
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.
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.
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.
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 ... }
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.
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.
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.
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"
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
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");