| 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
| ||
|
Message-ID: <53D0A7E4.1040707@aksignal.cz>
Date: Thu, 24 Jul 2014 08:29:56 +0200
From: Jiří Prchal <jiri.prchal@...ignal.cz>
To: Alexandre Courbot <gnurou@...il.com>,
Guenter Roeck <linux@...ck-us.net>
CC: "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
Linus Walleij <linus.walleij@...aro.org>,
Randy Dunlap <rdunlap@...radead.org>
Subject: Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name
Hi,
just to append to this: is in plan some kind of exporting named GPIOs from device tree to /sys/* or /dev/*? It would be
very useful not only for me. Because I thing what GPIO is used for what is hardware design dependent same as for example
what SPI chips are used. And SPI chips are in DT.
Dne 24.7.2014 v 07:44 Alexandre Courbot napsal(a):
> On Thu, Jul 24, 2014 at 3:12 AM, Guenter Roeck <linux@...ck-us.net> wrote:
>> gpiod_export_name is similar to gpiod_export, but lets the user
>> determine the name used to export a gpio pin.
>>
>> Currently, the pin name is determined by the chip driver with
>> the 'names' array in the gpio_chip data structure, or it is set
>> to gpioX, where X is the pin number, if no name is provided by
>> the chip driver.
>
> Oh, my. I did not even know about this 'names' array. This is pretty
> bad - a GPIO provider should not decide what its GPIOs are used for.
>
> Luckily for you, this creates a precedent that makes this patch more
> acceptable, in that it is not making the situation worse. Even though
> I consider both solutions to be bad, I actually prefer your
> gpiod_export_name() function to that 'names' array in gpio_chip...
>
>>
>> It is, however, desirable to be able to provide the pin name when
>> exporting the pin, for example from platform code. In other words,
>> it would be useful to move the naming decision from the pin provider
>> to the pin consumer. The gpio-pca953x driver provides this capability
>> as part of its platform data. Other drivers could be enhanced in a
>> similar way; however, this is not always possible or easy to accomplish.
>> For example, mfd client drivers such as gpio-ich already use platform
>> data to pass information from the mfd master driver to the client driver.
>> Overloading this platform data to also provide an array of gpio pin names
>> would be a challenge if not impossible.
>>
>> The alternative to use gpiod_export_link is also not always desirable,
>> since it only creates a named link to a different directory, meaning
>> the named gpio pin is not available in /sys/class/gpio but only
>> in some platform specific directory and thus not as generic as possible
>> and/or useful.
>>
>> A specific example for a use case is a gpio pin which reports AC power
>> loss to user space. Depending on the platform and platform variant,
>> the pin can be provided by various gpio chip drivers and pin numbers.
>> It would be very desirable to have a well defined location such as
>> /sys/class/gpio/ac_power_loss for this pin, so user space knows where
>> to find the attribute without knowledge of the underlying platform
>> variant or oher hardware details.
>
> As I explained on the other thread, I still encourage you to have
> these GPIOs under device nodes that give a hint of who is provided the
> GPIO (effectively exporting the (dev, function) tuple to user-space)
> instead of having them popping out under /sys/class/gpio where nobody
> knows where they come from and name collisions are much more likely.
>
> Your message sounds like you have no choice but have the named GPIO
> link under the gpiochip's device node, but this is not the case -
> gpio_export_link() let's you specify the device under which the link
> should appear. Make that device be your "scu" device and you can have
> a consistent sysfs path to access your GPIOs.
>
> Allowing GPIOs to pop up in the same directory with an arbitrary name
> is just a recipe for a mess. But that's a recipe that is already
> allowed thanks to that 'names' array. So I'm really confused about
> what to do with this patch. If you can do with gpio_export_link() (and
> I have not seen evidence of the contrary), please go that way instead.
>
>>
>> Signed-off-by: Guenter Roeck <linux@...ck-us.net>
>> ---
>> Applies to tip of linux-gpio/for-next.
>>
>> Documentation/gpio/sysfs.txt | 12 ++++++++----
>> drivers/gpio/gpiolib-sysfs.c | 23 ++++++++++++++++-------
>> include/linux/gpio/consumer.h | 9 +++++++++
>> 3 files changed, 33 insertions(+), 11 deletions(-)
>>
>> diff --git a/Documentation/gpio/sysfs.txt b/Documentation/gpio/sysfs.txt
>> index c2c3a97..8e301b2 100644
>> --- a/Documentation/gpio/sysfs.txt
>> +++ b/Documentation/gpio/sysfs.txt
>> @@ -125,7 +125,11 @@ requested using gpio_request():
>> /* export the GPIO to userspace */
>> int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
>>
>> - /* reverse gpio_export() */
>> + /* export named GPIO to userspace */
>> + int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
>> + bool direction_may_change);
>> +
>> + /* reverse gpio_export() / gpiod_export_name() */
>> void gpiod_unexport(struct gpio_desc *desc);
>>
>> /* create a sysfs link to an exported GPIO node */
>> @@ -136,9 +140,9 @@ requested using gpio_request():
>> int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
>>
>> After a kernel driver requests a GPIO, it may only be made available in
>> -the sysfs interface by gpiod_export(). The driver can control whether the
>> -signal direction may change. This helps drivers prevent userspace code
>> -from accidentally clobbering important system state.
>> +the sysfs interface by gpiod_export() or gpiod_export_name(). The driver
>> +can control whether the signal direction may change. This helps drivers
>> +prevent userspace code from accidentally clobbering important system state.
>>
>> This explicit exporting can help with debugging (by making some kinds
>> of experiments easier), or can provide an always-there interface that's
>> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
>> index be45a92..7d36230 100644
>> --- a/drivers/gpio/gpiolib-sysfs.c
>> +++ b/drivers/gpio/gpiolib-sysfs.c
>> @@ -523,13 +523,12 @@ static struct class gpio_class = {
>> *
>> * Returns zero on success, else an error.
>> */
>> -int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>> +int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
>> + bool direction_may_change)
>> {
>> unsigned long flags;
>> int status;
>> - const char *ioname = NULL;
>> struct device *dev;
>> - int offset;
>>
>> /* can't export until sysfs is available ... */
>> if (!gpio_class.p) {
>> @@ -560,10 +559,6 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>> direction_may_change = false;
>> spin_unlock_irqrestore(&gpio_lock, flags);
>>
>> - offset = gpio_chip_hwgpio(desc);
>> - if (desc->chip->names && desc->chip->names[offset])
>> - ioname = desc->chip->names[offset];
>> -
>> dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
>> desc, ioname ? ioname : "gpio%u",
>> desc_to_gpio(desc));
>> @@ -600,6 +595,20 @@ fail_unlock:
>> gpiod_dbg(desc, "%s: status %d\n", __func__, status);
>> return status;
>> }
>> +EXPORT_SYMBOL_GPL(gpiod_export_name);
>> +
>> +int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>> +{
>> + const char *ioname = NULL;
>> +
>> + if (desc) {
>> + int offset = gpio_chip_hwgpio(desc);
>> +
>> + if (desc->chip->names && desc->chip->names[offset])
>> + ioname = desc->chip->names[offset];
>
> I'd add a
>
> else
> ioname = "gpio%u";
>
> so all the name-chosing code is grouped in the same place. Then you
> can remove that same check from gpiod_export_name().
>
>> + }
>> + return gpiod_export_name(desc, ioname, direction_may_change);
>> +}
>> EXPORT_SYMBOL_GPL(gpiod_export);
>>
>> static int match_export(struct device *dev, const void *data)
>> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
>> index 05e53cc..986da3e 100644
>> --- a/include/linux/gpio/consumer.h
>> +++ b/include/linux/gpio/consumer.h
>> @@ -260,6 +260,8 @@ static inline int desc_to_gpio(const struct gpio_desc *desc)
>> #if IS_ENABLED(CONFIG_GPIOLIB) && IS_ENABLED(CONFIG_GPIO_SYSFS)
>>
>> int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
>> +int gpiod_export_name(struct gpio_desc *desc, const char *ioname,
>> + bool direction_may_change);
>> int gpiod_export_link(struct device *dev, const char *name,
>> struct gpio_desc *desc);
>> int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
>> @@ -273,6 +275,13 @@ static inline int gpiod_export(struct gpio_desc *desc,
>> return -ENOSYS;
>> }
>>
>> +static inline int gpiod_export_name(struct gpio_desc *desc,
>> + const char *ioname,
>> + bool direction_may_change)
>> +{
>> + return -ENOSYS;
>> +}
>> +
>> static inline int gpiod_export_link(struct device *dev, const char *name,
>> struct gpio_desc *desc)
>> {
>> --
>> 1.9.1
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists