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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux SPI]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux