diff mbox series

gpiolib sysfs access when CONFIG_GPIO_CDEV is not set

Message ID CAHNNwZAucoc00gJrUsPRMpFc9U2r+os6NJfc1axsGh0m6ES=xQ@mail.gmail.com
State New
Headers show
Series gpiolib sysfs access when CONFIG_GPIO_CDEV is not set | expand

Commit Message

Nicolas Schichan Nov. 4, 2020, 5:26 p.m. UTC
Hello,

Following an update to the 5.10-rc1 kernel, I found out that trying to
export a GPIO using /sys/class/gpio/export fails with the kernel
reporting a message in the kernel logs:

# echo 41 >/sys/class/gpio/export
[   46.761394] kobject_add_internal failed for gpio (error: -2 parent:
gpiochip2)
sh: write error: No such file or directory
#

I have tracked it to the fact that I have CONFIG_GPIO_CDEV is disabled in
my kernel config: Enabling CONFIG_GPIO_CDEV made export work again.

Enabling CONFIG_GPIO_CDEV and commenting all the code in
gpiolib_cdev_register() except the final "return 0;" made the issue
appear again, leading me to think that the issue is related to
something that is done cdev_device_add() must be done to fix the
issue.

Looking at the code of cdev_device_add() I was able to determine that
the device_add() call made there is required to get the gpiolib sysfs
export to work again.

In the end I have done this (which I won't even pretend is the proper
way to fix this), and sysfs attributes are finally working without
CONFIG_GPIO_CDEV:


If this is the preferred solution I can send a proper patch.

Best Regards,

Comments

Kent Gibson Nov. 5, 2020, 8:39 a.m. UTC | #1
On Wed, Nov 04, 2020 at 06:26:54PM +0100, Nicolas Schichan wrote:
> Hello,

> 

> Following an update to the 5.10-rc1 kernel, I found out that trying to

> export a GPIO using /sys/class/gpio/export fails with the kernel

> reporting a message in the kernel logs:

> 

> # echo 41 >/sys/class/gpio/export

> [   46.761394] kobject_add_internal failed for gpio (error: -2 parent:

> gpiochip2)

> sh: write error: No such file or directory

> #

> 

> I have tracked it to the fact that I have CONFIG_GPIO_CDEV is disabled in

> my kernel config: Enabling CONFIG_GPIO_CDEV made export work again.

> 


Hi Nicolas,

Thanks for the report and investigation.

Just checking - the CONFIG_GPIO_CDEV defaults to enabled, so you had
explicitly disabled it?

> Enabling CONFIG_GPIO_CDEV and commenting all the code in

> gpiolib_cdev_register() except the final "return 0;" made the issue

> appear again, leading me to think that the issue is related to

> something that is done cdev_device_add() must be done to fix the

> issue.

> 

> Looking at the code of cdev_device_add() I was able to determine that

> the device_add() call made there is required to get the gpiolib sysfs

> export to work again.

> 


So the sysfs init, and the remainder of gpiochip_setup_dev(), relies on the
cdev init to perform the device_add() - I missed that :(.

> In the end I have done this (which I won't even pretend is the proper

> way to fix this), and sysfs attributes are finally working without

> CONFIG_GPIO_CDEV:

> 


I'd prefer this dependency was made more explicit, so I'd be inclined to
relocate the ifdef CONFIG_GPIO_CDEV block from gpiolib-cdev.h into
gpiolib.c by adding helper functions that call either the
gpiolib_cdev_register/unregister or the device_add/del dependent on
CONFIG_GPIO_CDEV.

I'll try to get a patch out shortly.

Thanks,
Kent.

> diff --git a/drivers/gpio/gpiolib-cdev.h b/drivers/gpio/gpiolib-cdev.h

> index cb41dd757338..dd72bd0e4af4 100644

> --- a/drivers/gpio/gpiolib-cdev.h

> +++ b/drivers/gpio/gpiolib-cdev.h

> @@ -16,7 +16,7 @@ void gpiolib_cdev_unregister(struct gpio_device *gdev);

> 

>  static inline int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)

>  {

> -    return 0;

> +    return device_add(&gdev->dev);

>  }

> 

>  static inline void gpiolib_cdev_unregister(struct gpio_device *gdev)

> 

> If this is the preferred solution I can send a proper patch.

> 

> Best Regards,

> 

> -- 

> Nicolas Schichan
Nicolas Schichan Nov. 5, 2020, 12:07 p.m. UTC | #2
On Thu, Nov 5, 2020 at 9:39 AM Kent Gibson <warthog618@gmail.com> wrote:
[...]
> Hi Nicolas,

>

> Thanks for the report and investigation.

>

> Just checking - the CONFIG_GPIO_CDEV defaults to enabled, so you had

> explicitly disabled it?


Hello Kent,

Yes, we have no users of the character device gpio interface on our
platform, so I have disabled it explicitly when updating the defconfig
to the 5.10-rc1 kernel.

Regards,

-- 
Nicolas Schichan
Kent Gibson Nov. 5, 2020, 12:35 p.m. UTC | #3
On Thu, Nov 05, 2020 at 01:07:43PM +0100, Nicolas Schichan wrote:
> On Thu, Nov 5, 2020 at 9:39 AM Kent Gibson <warthog618@gmail.com> wrote:
> [...]
> > Hi Nicolas,
> >
> > Thanks for the report and investigation.
> >
> > Just checking - the CONFIG_GPIO_CDEV defaults to enabled, so you had
> > explicitly disabled it?
> 
> Hello Kent,
> 
> Yes, we have no users of the character device gpio interface on our
> platform, so I have disabled it explicitly when updating the defconfig
> to the 5.10-rc1 kernel.
> 

Thanks for confirming that - I was concerned that it had somehow been
disabled unintentionally - which would be a bug in itself.

Cheers,
Kent.
Linus Walleij Nov. 10, 2020, 1:43 p.m. UTC | #4
On Wed, Nov 4, 2020 at 6:27 PM Nicolas Schichan <nschichan@freebox.fr> wrote:

> I have tracked it to the fact that I have CONFIG_GPIO_CDEV is disabled in

> my kernel config: Enabling CONFIG_GPIO_CDEV made export work again.


We have deliberately made it hard to remove the character device
because we want to encourage people to use it.

> Yes, we have no users of the character device gpio interface on our

> platform, so I have disabled it explicitly when updating the defconfig

> to the 5.10-rc1 kernel.


Please consider getting users of the character device, since if they
are using the sysfs they are using an explicitly obsoleted interface.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib-cdev.h b/drivers/gpio/gpiolib-cdev.h
index cb41dd757338..dd72bd0e4af4 100644
--- a/drivers/gpio/gpiolib-cdev.h
+++ b/drivers/gpio/gpiolib-cdev.h
@@ -16,7 +16,7 @@  void gpiolib_cdev_unregister(struct gpio_device *gdev);

 static inline int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
 {
-    return 0;
+    return device_add(&gdev->dev);
 }

 static inline void gpiolib_cdev_unregister(struct gpio_device *gdev)