bcm63xx gpio issue on 3.19

bcm63xx gpio issue on 3.19

[Nicolas Schichan]

Hello Alexandre,

Using the latest 3.19 kernel, the bcm63xx GPIO code under
arch/mips/bcm63xx/gpio.c is unable to register the gpio chip via
gpiochip_add(), as it returns -ENOMEM. The kcalloc call for the gpio_desc
array fails, as during prom code, it is too early for the kmalloc to work.

It looks like the issue is caused by your patch: "gpio: remove gpio_descs
global array"

Could you please advise on how to fix/workaround that ? (ideally while keeping
the possibility to invoke the gpiolib code from the setup/prom code).

Regards,

-- 
Nicolas Schichan
Freebox SAS

Re: bcm63xx gpio issue on 3.19

[Alexandre Courbot]

Hi Nicolas,

(adding the linux-gpio mailing-list and Linus W.)

On 03/10/2015 02:53 AM, Nicolas Schichan wrote:
>
> Hello Alexandre,
>
> Using the latest 3.19 kernel, the bcm63xx GPIO code under
> arch/mips/bcm63xx/gpio.c is unable to register the gpio chip via
> gpiochip_add(), as it returns -ENOMEM. The kcalloc call for the gpio_desc
> array fails, as during prom code, it is too early for the kmalloc to work.
>
> It looks like the issue is caused by your patch: "gpio: remove gpio_descs
> global array"

Indeed. This happens because we removed the global GPIO array and 
replaced it with a more flexible per-chip array of GPIOs. We were hoping 
that issues like this one would have been caught in -next, but sadly the 
problem with bcm63xx went unnoticed until now. :(

>
> Could you please advise on how to fix/workaround that ? (ideally while keeping
> the possibility to invoke the gpiolib code from the setup/prom code).

The only allocation performed by gpiochip_add() is the array of 
gpio_descs. Having this array pre-allocated in your early code (maybe by 
using a static array variable) and passing it to a gpiochip_add_early() 
function would do the trick.

However, it is not that simple since gpio_desc is a private structure 
which details (including its size) are not visible outside of drivers/gpio.

Another solution I could see would be to have a kernel config option 
that would make gpiolib "pre-allocate" a number of gpio descriptors as a 
static array for such cases - similar to the global GPIO array, but not 
as big.

Finally, we can also restore the global GPIO array as a config option 
for the few architectures that need it.

Of course, I would prefer a solution based on dynamic allocation - is 
there a kind a primitive memory allocator that we can use at this early 
stage of boot? I.e. would alloc_pages() maybe work?

How do other subsystems that rely on dynamic allocation for registering 
their resources handle this? I guess regulator must fall in the same 
use-case, doesn't it?

Re: bcm63xx gpio issue on 3.19

[Alexandre Courbot]

Hi Nicolas,

(adding the linux-gpio mailing-list and Linus W.)

On 03/10/2015 02:53 AM, Nicolas Schichan wrote:
>
> Hello Alexandre,
>
> Using the latest 3.19 kernel, the bcm63xx GPIO code under
> arch/mips/bcm63xx/gpio.c is unable to register the gpio chip via
> gpiochip_add(), as it returns -ENOMEM. The kcalloc call for the gpio_desc
> array fails, as during prom code, it is too early for the kmalloc to work.
>
> It looks like the issue is caused by your patch: "gpio: remove gpio_descs
> global array"

Indeed. This happens because we removed the global GPIO array and 
replaced it with a more flexible per-chip array of GPIOs. We were hoping 
that issues like this one would have been caught in -next, but sadly the 
problem with bcm63xx went unnoticed until now. :(

>
> Could you please advise on how to fix/workaround that ? (ideally while keeping
> the possibility to invoke the gpiolib code from the setup/prom code).

The only allocation performed by gpiochip_add() is the array of 
gpio_descs. Having this array pre-allocated in your early code (maybe by 
using a static array variable) and passing it to a gpiochip_add_early() 
function would do the trick.

However, it is not that simple since gpio_desc is a private structure 
which details (including its size) are not visible outside of drivers/gpio.

Another solution I could see would be to have a kernel config option 
that would make gpiolib "pre-allocate" a number of gpio descriptors as a 
static array for such cases - similar to the global GPIO array, but not 
as big.

Finally, we can also restore the global GPIO array as a config option 
for the few architectures that need it.

Of course, I would prefer a solution based on dynamic allocation - is 
there a kind a primitive memory allocator that we can use at this early 
stage of boot? I.e. would alloc_pages() maybe work?

How do other subsystems that rely on dynamic allocation for registering 
their resources handle this? I guess regulator must fall in the same 
use-case, doesn't it?

Re: bcm63xx gpio issue on 3.19

[Nicolas Schichan]

On 03/11/2015 06:23 AM, Alexandre Courbot wrote:
>> Could you please advise on how to fix/workaround that ? (ideally while keeping
>> the possibility to invoke the gpiolib code from the setup/prom code).
> 
> The only allocation performed by gpiochip_add() is the array of gpio_descs.
> Having this array pre-allocated in your early code (maybe by using a static
> array variable) and passing it to a gpiochip_add_early() function would do the
> trick.
> 
> However, it is not that simple since gpio_desc is a private structure which
> details (including its size) are not visible outside of drivers/gpio.
> 
> Another solution I could see would be to have a kernel config option that
> would make gpiolib "pre-allocate" a number of gpio descriptors as a static
> array for such cases - similar to the global GPIO array, but not as big.
> 
> Finally, we can also restore the global GPIO array as a config option for the
> few architectures that need it.
> 
> Of course, I would prefer a solution based on dynamic allocation - is there a
> kind a primitive memory allocator that we can use at this early stage of boot?
> I.e. would alloc_pages() maybe work?
> 
> How do other subsystems that rely on dynamic allocation for registering their
> resources handle this? I guess regulator must fall in the same use-case,
> doesn't it?

Hi Alexandre,

Moving the bcm63xx_gpio_init() call from board_prom_init() to
bcm63xx_register_devices() (an arch_initcall) is enough to get called when
kmalloc is working. If code poking GPIOs is invoked earlier by a board code,
it will have to move in the board_register_devices() function though there
doesn't seem to any problems with the mainline bcm63xx board code in that regard.

I can produce a patch for that if it is an accepted solution. It has the
advantage of not requiring changes to the gpiolib code.

Regards,

-- 
Nicolas Schichan
Freebox SAS

Re: bcm63xx gpio issue on 3.19

[Alexandre Courbot]

On 03/12/2015 02:49 AM, Nicolas Schichan wrote:
> On 03/11/2015 06:23 AM, Alexandre Courbot wrote:
>>> Could you please advise on how to fix/workaround that ? (ideally while keeping
>>> the possibility to invoke the gpiolib code from the setup/prom code).
>>
>> The only allocation performed by gpiochip_add() is the array of gpio_descs.
>> Having this array pre-allocated in your early code (maybe by using a static
>> array variable) and passing it to a gpiochip_add_early() function would do the
>> trick.
>>
>> However, it is not that simple since gpio_desc is a private structure which
>> details (including its size) are not visible outside of drivers/gpio.
>>
>> Another solution I could see would be to have a kernel config option that
>> would make gpiolib "pre-allocate" a number of gpio descriptors as a static
>> array for such cases - similar to the global GPIO array, but not as big.
>>
>> Finally, we can also restore the global GPIO array as a config option for the
>> few architectures that need it.
>>
>> Of course, I would prefer a solution based on dynamic allocation - is there a
>> kind a primitive memory allocator that we can use at this early stage of boot?
>> I.e. would alloc_pages() maybe work?
>>
>> How do other subsystems that rely on dynamic allocation for registering their
>> resources handle this? I guess regulator must fall in the same use-case,
>> doesn't it?
>
> Hi Alexandre,
>
> Moving the bcm63xx_gpio_init() call from board_prom_init() to
> bcm63xx_register_devices() (an arch_initcall) is enough to get called when
> kmalloc is working. If code poking GPIOs is invoked earlier by a board code,
> it will have to move in the board_register_devices() function though there
> doesn't seem to any problems with the mainline bcm63xx board code in that regard.
>
> I can produce a patch for that if it is an accepted solution. It has the
> advantage of not requiring changes to the gpiolib code.

If that works for you and doesn't break bcm63xx, then yes that would be 
great. GPIO chips are indeed devices, so moving their registration to 
bcm63xx_register_devices() seems to make sense.

The GPIO subsystem might require more dynamic allocations in the near 
future (including maybe when requesting GPIOs), but hopefully all 
consumers will be safe with that. We'll find a solution for early GPIO 
support if they aren't.

Re: bcm63xx gpio issue on 3.19

[Alexandre Courbot]

On 03/12/2015 02:49 AM, Nicolas Schichan wrote:
> On 03/11/2015 06:23 AM, Alexandre Courbot wrote:
>>> Could you please advise on how to fix/workaround that ? (ideally while keeping
>>> the possibility to invoke the gpiolib code from the setup/prom code).
>>
>> The only allocation performed by gpiochip_add() is the array of gpio_descs.
>> Having this array pre-allocated in your early code (maybe by using a static
>> array variable) and passing it to a gpiochip_add_early() function would do the
>> trick.
>>
>> However, it is not that simple since gpio_desc is a private structure which
>> details (including its size) are not visible outside of drivers/gpio.
>>
>> Another solution I could see would be to have a kernel config option that
>> would make gpiolib "pre-allocate" a number of gpio descriptors as a static
>> array for such cases - similar to the global GPIO array, but not as big.
>>
>> Finally, we can also restore the global GPIO array as a config option for the
>> few architectures that need it.
>>
>> Of course, I would prefer a solution based on dynamic allocation - is there a
>> kind a primitive memory allocator that we can use at this early stage of boot?
>> I.e. would alloc_pages() maybe work?
>>
>> How do other subsystems that rely on dynamic allocation for registering their
>> resources handle this? I guess regulator must fall in the same use-case,
>> doesn't it?
>
> Hi Alexandre,
>
> Moving the bcm63xx_gpio_init() call from board_prom_init() to
> bcm63xx_register_devices() (an arch_initcall) is enough to get called when
> kmalloc is working. If code poking GPIOs is invoked earlier by a board code,
> it will have to move in the board_register_devices() function though there
> doesn't seem to any problems with the mainline bcm63xx board code in that regard.
>
> I can produce a patch for that if it is an accepted solution. It has the
> advantage of not requiring changes to the gpiolib code.

If that works for you and doesn't break bcm63xx, then yes that would be 
great. GPIO chips are indeed devices, so moving their registration to 
bcm63xx_register_devices() seems to make sense.

The GPIO subsystem might require more dynamic allocations in the near 
future (including maybe when requesting GPIOs), but hopefully all 
consumers will be safe with that. We'll find a solution for early GPIO 
support if they aren't.

Re: bcm63xx gpio issue on 3.19

[Linus Walleij]

On Wed, Mar 11, 2015 at 6:49 PM, Nicolas Schichan <nschichan@freebox.fr> wrote:

> Moving the bcm63xx_gpio_init() call from board_prom_init() to
> bcm63xx_register_devices() (an arch_initcall) is enough to get called when
> kmalloc is working. If code poking GPIOs is invoked earlier by a board code,
> it will have to move in the board_register_devices() function though there
> doesn't seem to any problems with the mainline bcm63xx board code in that regard.
>
> I can produce a patch for that if it is an accepted solution. It has the
> advantage of not requiring changes to the gpiolib code.

It would be *awesome* if you can come up with a patch like this.
Because it makes our life so much more simple.

(Maybe there is already such a patch in my INBOX somewhere...)

Yours,
Linus Walleij