linux-kernel - Re: [PATCH] serial: imx: Avoid probe failure in case of missing gpiolib

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 1 Aug 2019 10:59:54 +0000
From:   Schrempf Frieder <frieder.schrempf@...tron.de>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
CC:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        "Pengutronix Kernel Team" <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        "NXP Linux Team" <linux-imx@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        Jiri Slaby <jslaby@...e.com>
Subject: Re: [PATCH] serial: imx: Avoid probe failure in case of missing
 gpiolib

On 01.08.19 11:55, Uwe Kleine-König wrote:
> On Thu, Aug 01, 2019 at 09:28:33AM +0000, Schrempf Frieder wrote:
>> Hi Uwe,
>>
>> On 01.08.19 10:48, Uwe Kleine-König wrote:
>>> On Thu, Aug 01, 2019 at 08:18:05AM +0000, Schrempf Frieder wrote:
>>>> From: Frieder Schrempf <frieder.schrempf@...tron.de>
>>>>
>>>> If CONFIG_GPIOLIB is not enabled, mctrl_gpio_init() will return
>>>> -ENOSYS and cause the probing of the imx UART to fail. As the
>>>> GPIOs are optional, we should continue probing in this case.
>>>
>>> Is this really still the case? On which version did you hit this
>>> problem?
>>
>> Yes, I think it is. I used v5.2.5, that already has d99482673f95.
>>
>>>
>>> I would expect that is gone with
>>> d99482673f950817b30caf3fcdfb31179b050ce1 if not earlier.
>>
>> I think this is a different problem. If CONFIG_GPIOLIB is disabled,
>> mctrl_gpio_init() returns -ENOSYS unconditionally here: [1]. The
>> existing patch (d99482673f95) seems to handle the case when
>> CONFIG_GPIOLIB is enabled, but no or not all GPIOs are given in the dtb.
> 
> Ah, I see.
> 
> I don't think we should handle this on a per-driver basis. So my
> suggestion is to drop the dummy implementation for mctrl_gpio if GPIOLIB
> is disabled. Then the behaviour should be consistant with the gpio stuff
> returning NULL in this case. (Or alternatively adapt the dummy
> implementation to shortcut and behave identically.)

I get your point, but it seems a bit strange to go down all the way from 
the driver to mctrl_gpio_init_noauto() and into the loop just to return 
an empty struct mctrl_gpios to the driver that will be looped over again 
on each call of mctrl_gpio_*().

So I would rather go with a variation of your second proposal and keep 
the dummy implementation, but let it return NULL instead of an error 
pointer, as all the mctrl_gpio_*() functions already seem to have a 
check for gpios == NULL.

What do you think?

> 
> (Having said that I don't like gpiolib's behaviour of returning NULL for
> the optional calls if it's disabled, but having mctrl_gpio behave
> differently is worse.)
> 
>> The sh-sci.c driver has a similar check to skip this case: [2].
> 
> This should than be dropped, too.
> 
> Best regards
> Uwe
> 
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/serial_mctrl_gpio.h#n121
>> [2]
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/sh-sci.c#n3290
>