Re: next/master boot: 265 boots: 17 failed, 184 passed with 64 offline (next-20190730)

[Stephen Boyd <swboyd@chromium.org>]

Quoting Linus Walleij (2019-07-31 01:48:38)
> On Tue, Jul 30, 2019 at 3:41 PM Mark Brown <broonie@kernel.org> wrote:
> 
> > Today's -next fails to boot on QDF2400 systems:
> 
> Is this a devicetree or ACPI system? Which devicetree in that case?
> If it is ACPI I assume one had to look into DSDTs?
> 
> But I looked into this!
> 
> > 08:56:36.026587  [    4.339966] pc : __memset+0x80/0x188
> > 08:56:36.026867  [    4.343524] lr : msm_gpio_init_valid_mask+0x134/0x1a4
> 
> Aha. I think this only worked out of chance before.
> 
> What the Qualcomm driver does is exploit that .init_valid_mask() gets called
> even if .need_valid_mask in gpio_chip is not set to true,
> and this is why the bug appears in
> msm_gpio_init_valid_mask(), I'm pretty much sure it is the
> bitmap_zero(chip->valid_mask, max_gpios);
> call towards the end of the function that gets turned
> into:
> 08:56:36.114798  [    4.433713]  __memset+0x80/0x188
> 
> And this causes the crash.
> 
> This is then because chip->valid_mask has not been allocated, and that
> is because .need_valid_mask is not set. This is set like so:
> 
> static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
> {
>     if (pctrl->soc->reserved_gpios)
>         return true;
> 
>     return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
> }

Some of the code here is new. I guess the arm64 laptop stuff is making
changes.

> 
> First comes from the driver, second is for ACPI I think. It looks
> like a bit dangerous way to do it for ACPI, what if an OF pin controller
> has some "gpios" property? Oh well.
> 
> Apparently this only happens on ACPI systems because I tested it
> myself on a DT system.
> 
> Another cause may be this from the call site inside gpiolib:
> 
> static int gpiochip_alloc_valid_mask(struct gpio_chip *gc)
> {
>     if (IS_ENABLED(CONFIG_OF_GPIO))
>         gc->need_valid_mask = of_gpio_need_valid_mask(gc);
>     if (!gc->need_valid_mask)
>         return 0;
> 
>     gc->valid_mask = gpiochip_allocate_mask(gc);
>     if (!gc->valid_mask)
>         return -ENOMEM;
>     return 0;
> }
> 
> So if OF and ACPI is activated at the same time (can that happen?)

Yes, OF and ACPI can be compiled into the same kernel. Also, ACPI does
some interesting things when CONFIG_OF is enabled by looking for some
ACPI magic that basically says "match the DT compatible string embedded
in this ACPI thing". Quite scary!

> then the OF code will bail out I suppose and this happens when the
> ACPI side of things try to use the mask it didn't allocate. The ACPI
> codepath in msm_gpio_init_valid_mask() seems to try to set the
> mask even if there is zero GPIOs as well... dereferencing the NULL
> pointer in chip->valid_mask.
> 
> Could it be that this is a ACPI system with zero protected GPIOs?
> It doesn't seem like the code will survive that. It seems written
> under the assumption that
> 
> It's a bit of a mess.
> 
> Can some qcom ACPI people take linux-next for a ride and tell me
> what I should do?
> 
> Lee: do you know if linux-next boots fine on your ACPI machine?
> 
> Timur/Stephen: any ideas?
> 

I think the code changed in commit f626d6dfb709 ("gpio: of: Break out
OF-only code"). Now it unconditionally overwrites the chip's
need_valid_mask member when CONFIG_OF is enabled. How about only
overwriting it to 'true' when it really needs it? That way, the gpio
provider can have a say. I originally wrote this by having
of_gpio_need_valid_mask() modify the chip directly, but I like this
approach instead where we mark it as const in that function and then
only set it to true if it's actually needed.

-----8<----
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index b10d04dd9296..e39b4290b80c 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -87,7 +87,7 @@ static struct gpio_desc *of_xlate_and_get_gpiod_flags(struct gpio_chip *chip,
  * @dev: the device for the GPIO provider
  * @return: true if the valid mask needs to be set
  */
-bool of_gpio_need_valid_mask(struct gpio_chip *gc)
+bool of_gpio_need_valid_mask(const struct gpio_chip *gc)
 {
 	int size;
 	struct device_node *np = gc->of_node;
diff --git a/drivers/gpio/gpiolib-of.h b/drivers/gpio/gpiolib-of.h
index 34954921d96e..454d1658ee2d 100644
--- a/drivers/gpio/gpiolib-of.h
+++ b/drivers/gpio/gpiolib-of.h
@@ -16,7 +16,7 @@ struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
 int of_gpiochip_add(struct gpio_chip *gc);
 void of_gpiochip_remove(struct gpio_chip *gc);
 int of_gpio_get_count(struct device *dev, const char *con_id);
-bool of_gpio_need_valid_mask(struct gpio_chip *gc);
+bool of_gpio_need_valid_mask(const struct gpio_chip *gc);
 #else
 static inline struct gpio_desc *of_find_gpio(struct device *dev,
 					     const char *con_id,
@@ -36,7 +36,7 @@ static inline int of_gpio_get_count(struct device *dev, const char *con_id)
 {
 	return 0;
 }
-static inline bool of_gpio_need_valid_mask(struct gpio_chip *gc)
+static inline bool of_gpio_need_valid_mask(const struct gpio_chip *gc)
 {
 	return false;
 }
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d45c9a2285f0..e7153c81fdaa 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -362,8 +362,8 @@ static unsigned long *gpiochip_allocate_mask(struct gpio_chip *chip)
 
 static int gpiochip_alloc_valid_mask(struct gpio_chip *gc)
 {
-	if (IS_ENABLED(CONFIG_OF_GPIO))
-		gc->need_valid_mask = of_gpio_need_valid_mask(gc);
+	if (of_gpio_need_valid_mask(gc))
+		gc->need_valid_mask = true;
 	if (!gc->need_valid_mask)
 		return 0;
 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel