Re: [PATCH 2/2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio)

[Stefan Roese <sr@denx.de>]

Hi Andy,

On 08.08.19 15:48, Andy Shevchenko wrote:
> On Thu, Aug 08, 2019 at 03:25:43PM +0200, Stefan Roese wrote:
>> This patch fixes a backward compatibility issue, when boards use the
>> old style GPIO suffix "-gpio" instead of the new "-gpios". This
>> potential problem has been introduced by commit d99482673f95 ("serial:
>> mctrl_gpio: Check if GPIO property exisits before requesting it").
>>
>> This patch now fixes this issue by iterating over all supported GPIO
>> suffixes by using the newly introduced for_each_gpio_suffix() helper.
>>
>> Also, the string buffer is now allocated on the stack to avoid the
>> problem of allocation in a loop and its potential failure.
> 
>>   	for (i = 0; i < UART_GPIO_MAX; i++) {
>>   		enum gpiod_flags flags;
>> -		char *gpio_str;
>> +		const char *suffix;
>> +		char gpio_str[32];	/* 32 is max size of property name */
> 
> Hmm... don't we have some define for the maximum length of property?

I've come up with this assumption from this code (identical comment):

https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib-of.c#L293

(and other places in drivers/gpio/*)
  
> Or maybe we can still continue using kasprintf() approach?

Frankly, I was feeling a bit uncomfortable with this memory allocation
in a loop. And Pavel also commented on this:

https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2066286.html

So I would really prefer to move this buffer to the stack instead.
  
>>   		bool present;
>> +		int k;
>> +
>> +		/*
>> +		 * Check if GPIO property exists and continue if not. Iterate
>> +		 * over all supported GPIO suffixes (foo-gpios vs. foo-gpio).
>> +		 */
>> +		for_each_gpio_suffix(k, suffix) {
>> +			snprintf(gpio_str, sizeof(gpio_str), "%s-%s",
>> +				 mctrl_gpios_desc[i].name, suffix);
>> +
>> +			present = device_property_present(dev, gpio_str);
>> +			if (present)
>> +				break;
>> +		}
>>   
>> -		/* Check if GPIO property exists and continue if not */
>> -		gpio_str = kasprintf(GFP_KERNEL, "%s-gpios",
>> -				     mctrl_gpios_desc[i].name);
>> -		if (!gpio_str)
>> -			continue;
>> -
> 
>> -		present = device_property_present(dev, gpio_str);
> 
> Because there is no more explicit assignment of present outside of the loop,
> the compiler may warn about uninitialized variable in use...
> 
>> -		kfree(gpio_str);
> 
>>   		if (!present)
> 
> ...here.

My compiler does not warn (MIPS GCC 8.1) but I see your point. I'll add
an initialization in the next version.

Thanks,
Stefan