linux-kernel - Re: [PATCH v2 2/2] gpio: create the /sys/class/gpio mount point with GPIO

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: <2024101510-shifter-pursuable-ded5@gregkh>
Date: Tue, 15 Oct 2024 11:27:18 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Kent Gibson <warthog618@...il.com>, linux-kernel@...r.kernel.org,
	linux-gpio@...r.kernel.org,
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH v2 2/2] gpio: create the /sys/class/gpio mount point with
 GPIO_SYSFS disabled

On Tue, Oct 15, 2024 at 10:00:24AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> 
> User-space may want to use some kind of a compatibility layer for the
> deprecated GPIO sysfs ABI. This would typically involve mounting
> a fuse-based filesystem using the GPIO character device to emulate the
> sysfs behavior and layout.

Ick, no, don't mount a filesystem in /sys/class/ that's crazy, and
wrong.  See my other response for why.

> With GPIO_SYSFS disabled, the /sys/class/gpio directory doesn't exist
> and user-space cannot create it.

userspace should NOT be creating it.

> In order to facilitate moving away from
> the sysfs, add a new Kconfig option that indicates to GPIOLIB that is
> should create an always-empty directory where the GPIO class interface
> would exist and enable this option by default if GPIO_SYSFS is not
> selected.

No, either support a real sysfs api here, or don't.  Don't paper over
the api change by allowing this type of fake interface to live here,
that is just going to be a maintance nightmare.  Attempting to push the
"we don't support this user/kernel api anymore" off to a userspace
developer is not how to do kernel development.

> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> ---
>  drivers/gpio/Kconfig   | 18 ++++++++++++++++++
>  drivers/gpio/gpiolib.c |  6 ++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index efddc6455315..1a3535bda779 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -69,6 +69,24 @@ config GPIO_SYSFS
>  	  use the character device /dev/gpiochipN with the appropriate
>  	  ioctl() operations instead.
>  
> +config GPIO_SYSFS_CLASS_MOUNT_POINT
> +	bool "Create empty /sys/class/gpio directory" if EXPERT
> +	depends on !GPIO_SYSFS
> +	default y

Only "default y" if you can not boot without this enabled.  I doubt
that's the case here.  If it is the case here, then don't remove the
sysfs api in the first place.

And why is it being removed?  Who relies on it that can't live with it
being gone?


> +	help
> +	  Say Y here to create an empty /sys/class/gpio directory.
> +
> +	  User-space may want to use some kind of a compatibility layer for the
> +	  deprecated GPIO sysfs ABI. This would typically involve mounting
> +	  a fuse-based filesystem using the GPIO character device to emulate
> +	  the sysfs behavior and layout.
> +
> +	  This option makes GPIOLIB create an empty directory at /sys/class/gpio
> +	  where user-space can mount the sysfs replacement and avoid having to
> +	  change existing programs to adjust to different filesystem paths.
> +
> +	  If unsure, say Y.
> +
>  config GPIO_CDEV
>  	bool
>  	prompt "Character device (/dev/gpiochipN) support" if EXPERT
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 97346b746ef5..1c8bd765d8e1 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -4899,6 +4899,12 @@ static int __init gpiolib_dev_init(void)
>  		return ret;
>  	}
>  
> +#if IS_ENABLED(CONFIG_GPIO_SYSFS_CLASS_MOUNT_POINT)
> +	ret = sysfs_create_mount_point(class_kobj, "gpio");
> +	if (ret)
> +		pr_err("gpiolib: failed to create the GPIO class mountpoint\n");
> +#endif /* CONFIG_GPIO_SYSFS_CLASS_MOUNT_POINT */

Nit, I think we know what the #endif is for, it was only 3 lines above :)

thanks,

greg k-h