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

[Pavel Machek <pavel@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 2546 bytes --]

On Mon 2019-08-12 13:53:07, Andy Shevchenko wrote:
> On Thu, Aug 08, 2019 at 03:59:36PM +0200, Stefan Roese wrote:
> > 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/*)
> 
> I tried hard to find an evidence of this in Linux kernel, I assume that comes
> from DT compiler or something, but fail. Linux kernel OF properties handling is
> written in the assumption of arbitrary length of the property name.
> 
> It might be that my hard was not hard at all and I missed something.
> 
> > > 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
> 
> If memory allocator fails, it's a big issue, and what will happen next probably
> much less important.

Not... really. With "too big" allocations, it will fail.

Anyway, my point is that allocating in a loop for this is slow and
ugly. If we don't have a maximum property length, we should probably
invent some. I mean, we can agree that 64KB property name is not okay,
right?


								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]