* [PATCH v3 2/3] pinctrl: msm: Use init_valid_mask exported function
2018-10-02 8:27 [PATCH v3 1/3] gpiolib: Add init_valid_mask exported function Ricardo Ribalda Delgado
@ 2018-10-02 8:27 ` Ricardo Ribalda Delgado
2018-10-02 8:27 ` [PATCH v3 3/3] gpiolib: Show correct direction from the beginning Ricardo Ribalda Delgado
2018-10-02 9:14 ` [PATCH v3 1/3] gpiolib: Add init_valid_mask exported function Linus Walleij
2 siblings, 0 replies; 5+ messages in thread
From: Ricardo Ribalda Delgado @ 2018-10-02 8:27 UTC (permalink / raw)
To: Linus Walleij, Timur Tabi, swboyd, linux-gpio, LKML, Jeffrey Hugo
Cc: Ricardo Ribalda Delgado
The current code produces XPU violation if get_direction is called just
after the initialization.
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
drivers/pinctrl/qcom/pinctrl-msm.c | 79 ++++++++++++++----------------
1 file changed, 37 insertions(+), 42 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 5d72ffad32c2..ce1ade47ea37 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -566,6 +566,42 @@ static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
#define msm_gpio_dbg_show NULL
#endif
+static int msm_gpio_init_valid_mask(struct gpio_chip *chip)
+{
+ struct msm_pinctrl *pctrl = gpiochip_get_data(chip);
+ 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", NULL,
+ 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, len);
+ 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;
+}
+
static const struct gpio_chip msm_gpio_template = {
.direction_input = msm_gpio_direction_input,
.direction_output = msm_gpio_direction_output,
@@ -575,6 +611,7 @@ static const struct gpio_chip msm_gpio_template = {
.request = gpiochip_generic_request,
.free = gpiochip_generic_free,
.dbg_show = msm_gpio_dbg_show,
+ .init_valid_mask = msm_gpio_init_valid_mask,
};
/* For dual-edge interrupts in software, since some hardware has no
@@ -855,41 +892,6 @@ 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;
- u16 *tmp;
-
- /* The number of GPIOs in the ACPI tables */
- len = ret = device_property_read_u16_array(pctrl->dev, "gpios", NULL, 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, len);
- 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;
-}
-
static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
{
return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
@@ -926,13 +928,6 @@ 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;
- }
-
/*
* For DeviceTree-supported systems, the gpio core checks the
* pinctrl's device node for the "gpio-ranges" property.
--
2.19.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v3 3/3] gpiolib: Show correct direction from the beginning
2018-10-02 8:27 [PATCH v3 1/3] gpiolib: Add init_valid_mask exported function Ricardo Ribalda Delgado
2018-10-02 8:27 ` [PATCH v3 2/3] pinctrl: msm: Use " Ricardo Ribalda Delgado
@ 2018-10-02 8:27 ` Ricardo Ribalda Delgado
2018-10-02 12:29 ` Timur Tabi
2018-10-02 9:14 ` [PATCH v3 1/3] gpiolib: Add init_valid_mask exported function Linus Walleij
2 siblings, 1 reply; 5+ messages in thread
From: Ricardo Ribalda Delgado @ 2018-10-02 8:27 UTC (permalink / raw)
To: Linus Walleij, Timur Tabi, swboyd, linux-gpio, LKML, Jeffrey Hugo
Cc: Ricardo Ribalda Delgado
Current code assumes that the direction is input if direction_input
function is set.
This might not be the case on GPIOs with programmable direction.
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
drivers/gpio/gpiolib.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6925196136ce..eaadbcb5c0f8 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1344,20 +1344,6 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
spin_unlock_irqrestore(&gpio_lock, flags);
- for (i = 0; i < chip->ngpio; i++) {
- struct gpio_desc *desc = &gdev->descs[i];
-
- desc->gdev = gdev;
-
- /* REVISIT: most hardware initializes GPIOs as inputs (often
- * with pullups enabled) so power usage is minimized. Linux
- * code should set the gpio direction first thing; but until
- * it does, and in case chip->get_direction is not set, we may
- * expose the wrong direction in sysfs.
- */
- desc->flags = !chip->direction_input ? (1 << FLAG_IS_OUT) : 0;
- }
-
#ifdef CONFIG_PINCTRL
INIT_LIST_HEAD(&gdev->pin_ranges);
#endif
@@ -1374,6 +1360,25 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
if (status)
goto err_remove_irqchip_mask;
+ for (i = 0; i < chip->ngpio; i++) {
+ struct gpio_desc *desc = &gdev->descs[i];
+
+ desc->gdev = gdev;
+
+ /* REVISIT: most hardware initializes GPIOs as inputs (often
+ * with pullups enabled) so power usage is minimized. Linux
+ * code should set the gpio direction first thing; but until
+ * it does, and in case chip->get_direction is not set, we may
+ * expose the wrong direction in sysfs.
+ */
+ if (chip->get_direction && gpiochip_line_is_valid(chip, i))
+ desc->flags = !chip->get_direction(chip, i) ?
+ (1 << FLAG_IS_OUT) : 0;
+ else
+ desc->flags = !chip->direction_input ?
+ (1 << FLAG_IS_OUT) : 0;
+ }
+
status = gpiochip_add_irqchip(chip, lock_key, request_key);
if (status)
goto err_remove_chip;
--
2.19.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v3 1/3] gpiolib: Add init_valid_mask exported function
2018-10-02 8:27 [PATCH v3 1/3] gpiolib: Add init_valid_mask exported function Ricardo Ribalda Delgado
2018-10-02 8:27 ` [PATCH v3 2/3] pinctrl: msm: Use " Ricardo Ribalda Delgado
2018-10-02 8:27 ` [PATCH v3 3/3] gpiolib: Show correct direction from the beginning Ricardo Ribalda Delgado
@ 2018-10-02 9:14 ` Linus Walleij
2 siblings, 0 replies; 5+ messages in thread
From: Linus Walleij @ 2018-10-02 9:14 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: Timur Tabi, Stephen Boyd, open list:GPIO SUBSYSTEM,
linux-kernel@vger.kernel.org, jhugo
On Tue, Oct 2, 2018 at 10:27 AM Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> Add a function that allows initializing the valid_mask from
> gpiochip_add_data.
>
> This prevents race conditions during gpiochip initialization.
>
> If the function is not exported, then the old behaviour is respected,
> this is, set all gpios as valid.
>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
This is a very appetizing patch set.
I think patches 1 & 2 should be applied for sure even if
we don't apply patch 3, simply because it is way more
elegant.
Looking forward to see some test on Qualcomm's hardware
for this!
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 5+ messages in thread