[4/4] gpio: Remove VLA from stmpe driver

Message ID 20180310001021.6437-5-labbott@redhat.com
State New
Headers show
Series
  • VLA removal from the GPIO subsystem
Related show

Commit Message

Laura Abbott March 10, 2018, 12:10 a.m.
The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621)

This patch replaces a VLA with an appropriate call to kmalloc_array.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
 drivers/gpio/gpio-stmpe.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Phil Reid March 13, 2018, 9:13 a.m. | #1
On 10/03/2018 08:10, Laura Abbott wrote:
> The new challenge is to remove VLAs from the kernel
> (see https://lkml.org/lkml/2018/3/7/621)
> 
> This patch replaces a VLA with an appropriate call to kmalloc_array.
> 
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
>   drivers/gpio/gpio-stmpe.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
> index f8d7d1cd8488..b7854850bcdb 100644
> --- a/drivers/gpio/gpio-stmpe.c
> +++ b/drivers/gpio/gpio-stmpe.c
> @@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>   	struct stmpe *stmpe = stmpe_gpio->stmpe;
>   	u8 statmsbreg;
>   	int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
> -	u8 status[num_banks];
> +	u8 *status;
>   	int ret;
>   	int i;
>   
> +	status = kmalloc_array(num_banks, sizeof(*status), GFP_ATOMIC);
> +	if (!status)
> +		return IRQ_NONE;
> +
>   	/*
>   	 * the stmpe_block_read() call below, imposes to set statmsbreg
>   	 * with the register located at the lowest address. As STMPE1600
> @@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>   		}
>   	}
>   
> +	kfree(status);
>   	return IRQ_HANDLED;
>   }
>   
> 

Doing this in an irq handler seems wrong.
Perhaps better if a buffer is pre-allocated in stmpe_gpio
Laura Abbott March 14, 2018, 12:18 a.m. | #2
On 03/13/2018 02:13 AM, Phil Reid wrote:
> On 10/03/2018 08:10, Laura Abbott wrote:
>> The new challenge is to remove VLAs from the kernel
>> (see https://lkml.org/lkml/2018/3/7/621)
>>
>> This patch replaces a VLA with an appropriate call to kmalloc_array.
>>
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>>   drivers/gpio/gpio-stmpe.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
>> index f8d7d1cd8488..b7854850bcdb 100644
>> --- a/drivers/gpio/gpio-stmpe.c
>> +++ b/drivers/gpio/gpio-stmpe.c
>> @@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>>       struct stmpe *stmpe = stmpe_gpio->stmpe;
>>       u8 statmsbreg;
>>       int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
>> -    u8 status[num_banks];
>> +    u8 *status;
>>       int ret;
>>       int i;
>> +    status = kmalloc_array(num_banks, sizeof(*status), GFP_ATOMIC);
>> +    if (!status)
>> +        return IRQ_NONE;
>> +
>>       /*
>>        * the stmpe_block_read() call below, imposes to set statmsbreg
>>        * with the register located at the lowest address. As STMPE1600
>> @@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>>           }
>>       }
>> +    kfree(status);
>>       return IRQ_HANDLED;
>>   }
>>
> 
> Doing this in an irq handler seems wrong.
> Perhaps better if a buffer is pre-allocated in stmpe_gpio
> 
> 

Sure, I can pre-allocate the buffer in the probe.

Thanks,
Laura
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laura Abbott March 14, 2018, 1:16 a.m. | #3
On 03/13/2018 05:18 PM, Laura Abbott wrote:
> On 03/13/2018 02:13 AM, Phil Reid wrote:
>> On 10/03/2018 08:10, Laura Abbott wrote:
>>> The new challenge is to remove VLAs from the kernel
>>> (see https://lkml.org/lkml/2018/3/7/621)
>>>
>>> This patch replaces a VLA with an appropriate call to kmalloc_array.
>>>
>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>> ---
>>>   drivers/gpio/gpio-stmpe.c | 7 ++++++-
>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
>>> index f8d7d1cd8488..b7854850bcdb 100644
>>> --- a/drivers/gpio/gpio-stmpe.c
>>> +++ b/drivers/gpio/gpio-stmpe.c
>>> @@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>>>       struct stmpe *stmpe = stmpe_gpio->stmpe;
>>>       u8 statmsbreg;
>>>       int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
>>> -    u8 status[num_banks];
>>> +    u8 *status;
>>>       int ret;
>>>       int i;
>>> +    status = kmalloc_array(num_banks, sizeof(*status), GFP_ATOMIC);
>>> +    if (!status)
>>> +        return IRQ_NONE;
>>> +
>>>       /*
>>>        * the stmpe_block_read() call below, imposes to set statmsbreg
>>>        * with the register located at the lowest address. As STMPE1600
>>> @@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>>>           }
>>>       }
>>> +    kfree(status);
>>>       return IRQ_HANDLED;
>>>   }
>>>
>>
>> Doing this in an irq handler seems wrong.
>> Perhaps better if a buffer is pre-allocated in stmpe_gpio
>>
>>
> 
> Sure, I can pre-allocate the buffer in the probe.
> 
> Thanks,
> Laura

Actually I wonder if there would be concurrency problems if we
tried to pre-allocate a global buffer. But the IRQ handler
calls stmpe_block_read which takes a mutex to sleep so I think
doing the (small) kmalloc should be fine and I can change it to
a GFP_KERNEL.

Thanks,
Laura
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phil Reid March 14, 2018, 2:55 a.m. | #4
On 14/03/2018 09:16, Laura Abbott wrote:
> On 03/13/2018 05:18 PM, Laura Abbott wrote:
>> On 03/13/2018 02:13 AM, Phil Reid wrote:
>>> On 10/03/2018 08:10, Laura Abbott wrote:
>>>> The new challenge is to remove VLAs from the kernel
>>>> (see https://lkml.org/lkml/2018/3/7/621)
>>>>
>>>> This patch replaces a VLA with an appropriate call to kmalloc_array.
>>>>
>>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>>> ---
>>>>   drivers/gpio/gpio-stmpe.c | 7 ++++++-
>>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
>>>> index f8d7d1cd8488..b7854850bcdb 100644
>>>> --- a/drivers/gpio/gpio-stmpe.c
>>>> +++ b/drivers/gpio/gpio-stmpe.c
>>>> @@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>>>>       struct stmpe *stmpe = stmpe_gpio->stmpe;
>>>>       u8 statmsbreg;
>>>>       int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
>>>> -    u8 status[num_banks];
>>>> +    u8 *status;
>>>>       int ret;
>>>>       int i;
>>>> +    status = kmalloc_array(num_banks, sizeof(*status), GFP_ATOMIC);
>>>> +    if (!status)
>>>> +        return IRQ_NONE;
>>>> +
>>>>       /*
>>>>        * the stmpe_block_read() call below, imposes to set statmsbreg
>>>>        * with the register located at the lowest address. As STMPE1600
>>>> @@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>>>>           }
>>>>       }
>>>> +    kfree(status);
>>>>       return IRQ_HANDLED;
>>>>   }
>>>>
>>>
>>> Doing this in an irq handler seems wrong.
>>> Perhaps better if a buffer is pre-allocated in stmpe_gpio
>>
>> Sure, I can pre-allocate the buffer in the probe.
> 
> Actually I wonder if there would be concurrency problems if we
> tried to pre-allocate a global buffer. But the IRQ handler
> calls stmpe_block_read which takes a mutex to sleep so I think
> doing the (small) kmalloc should be fine and I can change it to
> a GFP_KERNEL.
> 
I'm no expert on this driver, but I wouldn't think there'd be any concurrency
problem if the buffer is only used by the irq handler.
It should never get called concurrently for the same device.

As to the impact, I'll admit I've really got no idea how much potential overhead a
kmalloc has compared to a mutex for the linux kernel.
Just a general rule of thumb, that memory allocations are usually expensive.

Patch

diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
index f8d7d1cd8488..b7854850bcdb 100644
--- a/drivers/gpio/gpio-stmpe.c
+++ b/drivers/gpio/gpio-stmpe.c
@@ -369,10 +369,14 @@  static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
 	struct stmpe *stmpe = stmpe_gpio->stmpe;
 	u8 statmsbreg;
 	int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
-	u8 status[num_banks];
+	u8 *status;
 	int ret;
 	int i;
 
+	status = kmalloc_array(num_banks, sizeof(*status), GFP_ATOMIC);
+	if (!status)
+		return IRQ_NONE;
+
 	/*
 	 * the stmpe_block_read() call below, imposes to set statmsbreg
 	 * with the register located at the lowest address. As STMPE1600
@@ -424,6 +428,7 @@  static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
 		}
 	}
 
+	kfree(status);
 	return IRQ_HANDLED;
 }