Re: [PATCH v2 2/2] gpio: create the /sys/class/gpio mount point with GPIO_SYSFS disabled

[Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>]

On Tue, Oct 15, 2024 at 10:00:24AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> ---
>  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