linux-kernel - Re: [PATCH v2] gpiolib: Take MUX usage into account

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]
Message-ID: <f568ca31-c9f0-0dc2-be12-22de25891794@gmx.net>
Date:   Tue, 13 Aug 2019 18:32:23 +0200
From:   Stefan Wahren <wahrenst@....net>
To:     "Fried, Ramon" <ramon.fried@...ux.intel.com>,
        linus.walleij@...aro.org, bgolaszewski@...libre.com
Cc:     linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] gpiolib: Take MUX usage into account

Am 13.08.19 um 08:10 schrieb Fried, Ramon:
>
> On 8/13/2019 08:38, Stefan Wahren wrote:
>> Hi Ramon,
>>
>> On 13.08.19 03:42, Ramon Fried wrote:
>>> From: Stefan Wahren <stefan.wahren@...e.com>
>>>
>>> The user space like gpioinfo only see the GPIO usage but not the
>>> MUX usage (e.g. I2C or SPI usage) of a pin. As a user we want to
>>> know which
>>> pin is free/safe to use. So take the MUX usage of strict pinmux
>>> controllers
>>> into account to get a more realistic view for ioctl
>>> GPIO_GET_LINEINFO_IOCTL.
>>>
>>> Signed-off-by: Stefan Wahren <stefan.wahren@...e.com>
>>> Tested-by: Ramon Fried <rfried.dev@...il.com>
>>> Signed-off-by: Ramon Fried <rfried.dev@...il.com>
>>> ---
>>> v2: Address review from linus:
>>> * ** Please notive logic was reversed **
>>> * renamed pinctrl_gpio_is_in_use() to pinctrl_gpio_can_use_line()
>>> * renamed pinmux_is_in_use() to pinmux_can_be_used_for_gpio()
>>> * changed dev_err to dev_dbg (Linus suggested removing it altogether, I
>>>    find it better to keep it for debug).
>> thanks for taking care of this.
>>>   drivers/gpio/gpiolib.c           |  3 ++-
>>>   drivers/pinctrl/core.c           | 28 ++++++++++++++++++++++++++++
>>>   drivers/pinctrl/pinmux.c         | 27 +++++++++++++++++++++++++++
>>>   drivers/pinctrl/pinmux.h         |  8 ++++++++
>>>   include/linux/pinctrl/consumer.h |  6 ++++++
>>>   5 files changed, 71 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>> index f497003f119c..52937bf8e514 100644
>>> --- a/drivers/gpio/gpiolib.c
>>> +++ b/drivers/gpio/gpiolib.c
>>> @@ -1084,7 +1084,8 @@ static long gpio_ioctl(struct file *filp,
>>> unsigned int cmd, unsigned long arg)
>>>               test_bit(FLAG_IS_HOGGED, &desc->flags) ||
>>>               test_bit(FLAG_USED_AS_IRQ, &desc->flags) ||
>>>               test_bit(FLAG_EXPORT, &desc->flags) ||
>>> -            test_bit(FLAG_SYSFS, &desc->flags))
>>> +            test_bit(FLAG_SYSFS, &desc->flags) ||
>>> +            !pinctrl_gpio_can_use_line(chip->base +
>>> lineinfo.line_offset))
>>>               lineinfo.flags |= GPIOLINE_FLAG_KERNEL;
>>>           if (test_bit(FLAG_IS_OUT, &desc->flags))
>>>               lineinfo.flags |= GPIOLINE_FLAG_IS_OUT;
>>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>>> index b70df27874d1..2bbd8ee93507 100644
>>> --- a/drivers/pinctrl/core.c
>>> +++ b/drivers/pinctrl/core.c
>>> @@ -736,6 +736,34 @@ int pinctrl_get_group_selector(struct
>>> pinctrl_dev *pctldev,
>>>       return -EINVAL;
>>>   }
>>>   +bool pinctrl_gpio_can_use_line(unsigned gpio)
>>> +{
>>> +    struct pinctrl_dev *pctldev;
>>> +    struct pinctrl_gpio_range *range;
>>> +    bool result;
>>> +    int pin;
>>> +
>>> +    /*
>>> +     * Try to obtain GPIO range, if it fails
>>> +     * we're probably dealing with GPIO driver
>>> +     * without a backing pin controller - bail out.
>>> +     */
>>> +    if (pinctrl_get_device_gpio_range(gpio, &pctldev, &range))
>>> +        return true;
>>> +
>>> +    mutex_lock(&pctldev->mutex);
>>> +
>>> +    /* Convert to the pin controllers number space */
>>> +    pin = gpio_to_pin(range, gpio);
>>> +
>>> +    result = pinmux_can_be_used_for_gpio(pctldev, pin);
>>> +
>>> +    mutex_unlock(&pctldev->mutex);
>>> +
>>> +    return result;
>>> +}
>>> +EXPORT_SYMBOL_GPL(pinctrl_gpio_can_use_line);
>>> +
>>>   /**
>>>    * pinctrl_gpio_request() - request a single pin to be used as GPIO
>>>    * @gpio: the GPIO pin number from the GPIO subsystem number space
>>> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
>>> index 020e54f843f9..7e42a5738d82 100644
>>> --- a/drivers/pinctrl/pinmux.c
>>> +++ b/drivers/pinctrl/pinmux.c
>>> @@ -70,6 +70,33 @@ int pinmux_validate_map(const struct pinctrl_map
>>> *map, int i)
>>>       return 0;
>>>   }
>>>   +/**
>>> + * pinmux_can_be_used_for_gpio() - check if a specific pin
>>> + *    is either muxed to a different function or used as gpio.
>>> + *
>>> + * @pin: the pin number in the global pin space
>>> + *
>>> + * Controllers not defined as strict will always return true,
>>> + * menaning that the gpio can be used.
>>> + */
>>> +bool pinmux_can_be_used_for_gpio(struct pinctrl_dev *pctldev,
>>> unsigned pin)
>>> +{
>>> +    struct pin_desc *desc = pin_desc_get(pctldev, pin);
>>> +    const struct pinmux_ops *ops = pctldev->desc->pmxops;
>>> +
>>> +    if (!desc) {
>>> +        dev_dbg(pctldev->dev,
>>> +            "pin %u is not registered so it cannot be requested\n",
>>> +            pin);
>>> +        return true;
>> This return value looks strange to me.
>
> Basically, it's just the reversed return value you returned in the
> original patch,
> It means in this context that if the pin is not owned by a
> pin-controller it can be used for GPIO.
As long as the provided pin is valid. Btw shouldn't we change the logic
in the debug message?
>
> Thanks,
> Ramon.
>
>>
>> Stefan
>>