[PATCH] gpio: Do not accept gpio chip additions before gpiolib initialization

[PATCH] gpio: Do not accept gpio chip additions before gpiolib initialization

[Guenter Roeck]

Since commit ff2b13592299 ("gpio: make the gpiochip a real device"),
attempts to add a gpio chip prior to gpiolib initialization cause the
system to crash. Dump a warning to the console and return an error
if the situation is encountered.

Fixes: ff2b13592299 ("gpio: make the gpiochip a real device")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/gpio/gpiolib.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 72065532c1c7..ac66219f0611 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -68,6 +68,7 @@ LIST_HEAD(gpio_devices);
 static void gpiochip_free_hogs(struct gpio_chip *chip);
 static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip);
 
+static bool gpiolib_initialized;
 
 static inline void desc_set_label(struct gpio_desc *d, const char *label)
 {
@@ -457,6 +458,9 @@ static void gpiodevice_release(struct device *dev)
  * the gpio framework's arch_initcall().  Otherwise sysfs initialization
  * for GPIOs will fail rudely.
  *
+ * gpiochip_add_data() must only be called after gpiolib initialization,
+ * ie after core_initcall().
+ *
  * If chip->base is negative, this requests dynamic assignment of
  * a range of valid GPIOs.
  */
@@ -468,6 +472,11 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data)
 	int		base = chip->base;
 	struct gpio_device *gdev;
 
+	if (!gpiolib_initialized) {
+		WARN(1, "gpiolib not initialized\n");
+		return -EPROBE_DEFER;
+	}
+
 	/*
 	 * First: allocate and populate the internal stat container, and
 	 * set up the struct device.
@@ -2829,6 +2838,8 @@ static int __init gpiolib_dev_init(void)
 	if (ret < 0) {
 		pr_err("gpiolib: failed to allocate char dev region\n");
 		bus_unregister(&gpio_bus_type);
+	} else {
+		gpiolib_initialized = true;
 	}
 	return ret;
 }
-- 
2.5.0

Re: [PATCH] gpio: Do not accept gpio chip additions before gpiolib initialization

[Alexandre Courbot]

On Wed, Mar 30, 2016 at 3:20 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> Since commit ff2b13592299 ("gpio: make the gpiochip a real device"),
> attempts to add a gpio chip prior to gpiolib initialization cause the
> system to crash. Dump a warning to the console and return an error
> if the situation is encountered.

Mmm I see the problem but this could seriously delay the availability
of some GPIOs that are useful for early system boot.

I have not followed the GPIO device patches as closely as I should
have, but shouldn't you be able to register a GPIO chip without
immediately presenting it to user-space, for internal kernel needs? If
gpiolib is not initialized, then device-related operations would be
skipped, and gpiolib_dev_init() could then parse the list of
registered chips and fix them up when it gets called.

Again, I'm speaking without real knowledge here, but that pattern
seems more resilent to me.

Re: [PATCH] gpio: Do not accept gpio chip additions before gpiolib initialization

[Guenter Roeck]

On 03/30/2016 01:37 AM, Alexandre Courbot wrote:
> On Wed, Mar 30, 2016 at 3:20 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> Since commit ff2b13592299 ("gpio: make the gpiochip a real device"),
>> attempts to add a gpio chip prior to gpiolib initialization cause the
>> system to crash. Dump a warning to the console and return an error
>> if the situation is encountered.
>
> Mmm I see the problem but this could seriously delay the availability
> of some GPIOs that are useful for early system boot.
>
> I have not followed the GPIO device patches as closely as I should
> have, but shouldn't you be able to register a GPIO chip without
> immediately presenting it to user-space, for internal kernel needs? If
> gpiolib is not initialized, then device-related operations would be
> skipped, and gpiolib_dev_init() could then parse the list of
> registered chips and fix them up when it gets called.
>
> Again, I'm speaking without real knowledge here, but that pattern
> seems more resilent to me.
>
You are absolutely right, but my knowledge of gpiolib is not good enough
to make that change. See this as a band-gap; it is better than just crashing.

Guenter

Re: [PATCH] gpio: Do not accept gpio chip additions before gpiolib initialization

[Alexandre Courbot]

On Wed, Mar 30, 2016 at 6:16 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 03/30/2016 01:37 AM, Alexandre Courbot wrote:
>>
>> On Wed, Mar 30, 2016 at 3:20 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>> Since commit ff2b13592299 ("gpio: make the gpiochip a real device"),
>>> attempts to add a gpio chip prior to gpiolib initialization cause the
>>> system to crash. Dump a warning to the console and return an error
>>> if the situation is encountered.
>>
>>
>> Mmm I see the problem but this could seriously delay the availability
>> of some GPIOs that are useful for early system boot.
>>
>> I have not followed the GPIO device patches as closely as I should
>> have, but shouldn't you be able to register a GPIO chip without
>> immediately presenting it to user-space, for internal kernel needs? If
>> gpiolib is not initialized, then device-related operations would be
>> skipped, and gpiolib_dev_init() could then parse the list of
>> registered chips and fix them up when it gets called.
>>
>> Again, I'm speaking without real knowledge here, but that pattern
>> seems more resilent to me.
>>
> You are absolutely right, but my knowledge of gpiolib is not good enough
> to make that change. See this as a band-gap; it is better than just
> crashing.

Actually, the following may be simpler:

Why not add a check in gpiochip_add_data() that will directly call
gpiolib_dev_init() if required? Then gpiolib_dev_init() could also
check whether it has already been called in that context and become a
no-op for when it is later called from core_initcall. Is there
anything that would prevents this from being a viable fix?

Re: [PATCH] gpio: Do not accept gpio chip additions before gpiolib initialization

[Guenter Roeck]

On 03/30/2016 10:57 PM, Alexandre Courbot wrote:
> On Wed, Mar 30, 2016 at 6:16 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 03/30/2016 01:37 AM, Alexandre Courbot wrote:
>>>
>>> On Wed, Mar 30, 2016 at 3:20 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> Since commit ff2b13592299 ("gpio: make the gpiochip a real device"),
>>>> attempts to add a gpio chip prior to gpiolib initialization cause the
>>>> system to crash. Dump a warning to the console and return an error
>>>> if the situation is encountered.
>>>
>>>
>>> Mmm I see the problem but this could seriously delay the availability
>>> of some GPIOs that are useful for early system boot.
>>>
>>> I have not followed the GPIO device patches as closely as I should
>>> have, but shouldn't you be able to register a GPIO chip without
>>> immediately presenting it to user-space, for internal kernel needs? If
>>> gpiolib is not initialized, then device-related operations would be
>>> skipped, and gpiolib_dev_init() could then parse the list of
>>> registered chips and fix them up when it gets called.
>>>
>>> Again, I'm speaking without real knowledge here, but that pattern
>>> seems more resilent to me.
>>>
>> You are absolutely right, but my knowledge of gpiolib is not good enough
>> to make that change. See this as a band-gap; it is better than just
>> crashing.
>
> Actually, the following may be simpler:
>
> Why not add a check in gpiochip_add_data() that will directly call
> gpiolib_dev_init() if required? Then gpiolib_dev_init() could also
> check whether it has already been called in that context and become a
> no-op for when it is later called from core_initcall. Is there
> anything that would prevents this from being a viable fix?
>
That was my first solution. Unfortunately, it doesn't work. It appears
that the calls made by gpiolib_dev_init() have dependencies themselves.
Though maybe I messed up - feel free to try yourself.

As mentioned in the other thread, I started looking into the solution
you suggested above. It should work, but it will take (me) a while
to implement it. Until then, guess we'll see more breakage.

Guenter

Re: [PATCH] gpio: Do not accept gpio chip additions before gpiolib initialization

[Guenter Roeck]

On Tue, Mar 29, 2016 at 11:20:38AM -0700, Guenter Roeck wrote:
> Since commit ff2b13592299 ("gpio: make the gpiochip a real device"),
> attempts to add a gpio chip prior to gpiolib initialization cause the
> system to crash. Dump a warning to the console and return an error
> if the situation is encountered.
> 
> Fixes: ff2b13592299 ("gpio: make the gpiochip a real device")
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Please drop this one; I think the other series I sent out earlier today
is much better.

Guenter

> ---
>  drivers/gpio/gpiolib.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 72065532c1c7..ac66219f0611 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -68,6 +68,7 @@ LIST_HEAD(gpio_devices);
>  static void gpiochip_free_hogs(struct gpio_chip *chip);
>  static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip);
>  
> +static bool gpiolib_initialized;
>  
>  static inline void desc_set_label(struct gpio_desc *d, const char *label)
>  {
> @@ -457,6 +458,9 @@ static void gpiodevice_release(struct device *dev)
>   * the gpio framework's arch_initcall().  Otherwise sysfs initialization
>   * for GPIOs will fail rudely.
>   *
> + * gpiochip_add_data() must only be called after gpiolib initialization,
> + * ie after core_initcall().
> + *
>   * If chip->base is negative, this requests dynamic assignment of
>   * a range of valid GPIOs.
>   */
> @@ -468,6 +472,11 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data)
>  	int		base = chip->base;
>  	struct gpio_device *gdev;
>  
> +	if (!gpiolib_initialized) {
> +		WARN(1, "gpiolib not initialized\n");
> +		return -EPROBE_DEFER;
> +	}
> +
>  	/*
>  	 * First: allocate and populate the internal stat container, and
>  	 * set up the struct device.
> @@ -2829,6 +2838,8 @@ static int __init gpiolib_dev_init(void)
>  	if (ret < 0) {
>  		pr_err("gpiolib: failed to allocate char dev region\n");
>  		bus_unregister(&gpio_bus_type);
> +	} else {
> +		gpiolib_initialized = true;
>  	}
>  	return ret;
>  }
> -- 
> 2.5.0
>