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@xxxxxxxxxxxxxxx/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
