Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add /dev/gpiomem device for rootless user GPIO access #1112

Merged
merged 1 commit into from Aug 21, 2015

Conversation

Wren6991
Copy link
Contributor

Currently, users need root privileges to use GPIO. This causes issues - for example, if they want to do some data logging, they will then need root permissions to edit the recorded data, and it will be put in root's home folder rather than theirs. Instead of just opening IDLE and doing import RPi.GPIO, they need to open a console, do sudo idle3 &, and close the console. etc.

This commit adds a character device, /dev/gpiomem, which maps the GPIO register page when a user mmaps it. An mmap interface is simplest at the moment because there are large and popular existing libraries which are just mmapping /dev/mem, and this means minimal code changes for the maintainers, although in the long term we would like a proper kernel-side driver rather than the existing userspace pseudo-drivers.

This allows better control of GPIO permissions, for instance there can be a gpio user group, which can give users access to GPIO without giving them the whole of physical memory.

@notro
Copy link
Contributor

notro commented Aug 20, 2015

Do you know why these libraries want to write directly to the gpio registers instead of using the sysfs interface? Is it to speed up setting multiple gpio's in one go?

@Ferroin
Copy link
Contributor

Ferroin commented Aug 20, 2015

On 2015-08-20 09:13, notro wrote:

Do you know why these libraries want to write directly to the gpio
registers instead of using the sysfs interface? Is it to speed up
setting multiple gpio's in go?
I'd be willing to bet that it also has something to do with the fact
that it is easier to safely allow non-root users to map segments of
iomem than it is to give them selective access to sysfs, and also allows
(theoretically) higher performance. That and there are things out there
that are timing sensitive enough that they can't afford the overhead of
dealing with the sysfs interface.

@P33M
Copy link
Contributor

P33M commented Aug 20, 2015

There are (historical, Pi-only) tools for manipulating the GPIOs that rely on /dev/mem access. They do this for reasons of performance, mainly - the sysfs interface is slow when you want to bitbash at kHz from userspace.

As Luke says, this ends up with most things being run as root, which is Bad (tm).

This is a halfway step to deprecating userspace tools that whack registers via mmap()'ed peripheral memory spaces - at least now we can control user/group accesses and prevent arbitrary reads outside the GPIO register file (which has its own 4k segment in the bus address space) and have a low-overhead userspace code change to update these applications to use this equivalent but safer interface.

@Wren6991
Copy link
Contributor Author

Hi Jonathan, thanks for spotting those, everything should be working now. I've only tested on a Pi 2 but raspi-gpio works with minimal changes. I've squashed in a new commit and will now go and find more Pis to test on.

As Jonathan says, the reason users are currently using /dev/mem is partly performance - there is a nice generic sysfs interface but mmapping /dev/mem allows them to go orders of magnitude faster (i.e. potentially toggling pins at MHz instead of kHz).

Also, I don't believe the sysfs interface allows setting of pullups.

@Wren6991
Copy link
Contributor Author

Thanks Phil, I've corrected that now.

A question: currently the driver is being loaded via a device tree overlay, but if we want to make /dev/gpiomem preferred over /dev/mem then we want it to be enabled by default. After a quick discussion with @pelwell we think the gpiomem node should go in both bcm2708.dtsi and bcm2709.dtsi to avoid polluting bcm2708_common.dtsi, which is meant to be a generic hardware description, and would be one more thing that ARCH_BCM2835 would have to patch out.

Thoughts?

@pelwell
Copy link
Contributor

pelwell commented Aug 20, 2015

(Context: @notro has a pending PR to include bcm2708_common.dtsi from bcm2835.dtsi)

@Wren6991
Copy link
Contributor Author

Squashed into one commit. What do people think?

@pelwell
Copy link
Contributor

pelwell commented Aug 21, 2015

The concept doesn't really fit with the spirit of Device Tree (including the GPIO block twice, giving it a different compatible string to cause a particular driver to be loaded) but we are doing it to allow the various user-space GPIO libraries to be cleaned up a bit by not requiring root privilege, in a way that doesn't require changes to the board support code or to /etc/modules.

Given the above, I'm happy with it.

@Wren6991
Copy link
Contributor Author

I totally agree, but it's a positive step away from the current state of everyone running as root and mapping /dev/mem willy nilly.

I've sent a pull request to raspi-gpio and a patch to the maintainer of RPi.GPIO to add support for /dev/gpiomem.

@Wren6991
Copy link
Contributor Author

@popcornmix what do you think?

@popcornmix
Copy link
Collaborator

Yes, I'm okay with this. It is ugly, but less so than running python as root to use gpio libraries.

@pelwell
Copy link
Contributor

pelwell commented Aug 21, 2015

Luke, do you want to make any changes following popcornmix's comments?

@Wren6991
Copy link
Contributor Author

Thanks for that Dom, I've removed those useless gotos. I'm happy if you are!

@pelwell
Copy link
Contributor

pelwell commented Aug 21, 2015

drivers/char/broadcom/bcm2835-gpiomem.c: In function "bcm2835_gpiomem_probe":
drivers/char/broadcom/bcm2835-gpiomem.c:204:2: warning: format "%x" expects argument of type "unsigned int", but argument 3 has type "long unsigned int" [-Wformat=]
  dev_info(inst->dev, "Initialised: Registers at 0x%08x",
  ^

@Wren6991
Copy link
Contributor Author

diff --git a/drivers/char/broadcom/bcm2835-gpiomem.c b/drivers/char/broadcom/bcm2835-gpiomem.c
index 0d46050..0085e13 100644
--- a/drivers/char/broadcom/bcm2835-gpiomem.c
+++ b/drivers/char/broadcom/bcm2835-gpiomem.c
@@ -201,7 +201,7 @@ static int bcm2835_gpiomem_probe(struct platform_device *pdev)
                inst->gpio_regs_phys = GPIO_BASE;
        }

-       dev_info(inst->dev, "Initialised: Registers at 0x%08x",
+       dev_info(inst->dev, "Initialised: Registers at 0x%08lx",
                inst->gpio_regs_phys);

        return 0;

Wow. Sorry, Phil.

Signed-off-by: Luke Wren <luke@raspberrypi.org>
pelwell added a commit that referenced this pull request Aug 21, 2015
Add /dev/gpiomem device for rootless user GPIO access
@pelwell pelwell merged commit 1be0d57 into raspberrypi:rpi-4.1.y Aug 21, 2015
@P33M
Copy link
Contributor

P33M commented Aug 21, 2015

This thread's been blowing up my email notifications for the last four hours. Go to bed, Luke.

popcornmix added a commit to raspberrypi/firmware that referenced this pull request Sep 10, 2015
See: raspberrypi/linux#756

kernel: spi-bcm2835: merge upstream patches allowing DMA transfers
See: raspberrypi/linux#1085

kernel: rpisense-fb: add low-light mode and gamma control
See: raspberrypi/linux#1104

kernel: bcm2708-dmaengine: Use more DMA channels (but not 12)
See:  raspberrypi/linux#1113

kernel: Add /dev/gpiomem device for rootless user GPIO access
See: raspberrypi/linux#1112

kernel: Add RaspiDAC3 support

kernel: Rpi 4.1.y spi bcm2835 patches clock-polarity issue
See: raspberrypi/linux#1125

kernel: BCM270X_DT: Add SDIO overlay
popcornmix added a commit to Hexxeh/rpi-firmware that referenced this pull request Sep 10, 2015
See: raspberrypi/linux#756

kernel: spi-bcm2835: merge upstream patches allowing DMA transfers
See: raspberrypi/linux#1085

kernel: rpisense-fb: add low-light mode and gamma control
See: raspberrypi/linux#1104

kernel: bcm2708-dmaengine: Use more DMA channels (but not 12)
See:  raspberrypi/linux#1113

kernel: Add /dev/gpiomem device for rootless user GPIO access
See: raspberrypi/linux#1112

kernel: Add RaspiDAC3 support

kernel: Rpi 4.1.y spi bcm2835 patches clock-polarity issue
See: raspberrypi/linux#1125

kernel: BCM270X_DT: Add SDIO overlay
@dotdoom
Copy link

dotdoom commented Dec 29, 2015

Any chance /dev/gpiomem gets PWM control registers range (base + 0x200000)?

@popcornmix
Copy link
Collaborator

Unlikely. If we add PWM, then why not clock manager, or dma, or interrupt any other peripherals.
This was reluctantly added as teaching resources were using python libraries to access gpio and the requirement for running as root was setting a bad example.
We don't want to extend this to allow more hacky peripheral drivers from userland. I suspect any pwm accesses should go through a proper kernel driver.

@BTBlueSkies
Copy link

I'd like to say thanks for the new /dev/gpiomem addition and add an user comment. While I understand the last comment about if you add PWM, then why not clock manager or dma, etc.. I'd like to add that as a user of this board, when I envision the applications for the board, I am mostly thinking of the functions supplied on the GPIO header. The SPI, the I2C, the PWM and so forth. So far it has been burdensome and hard to properly make use of these features and keep good security practices intact.

I'd vote that at a minimum, raw access via GPIOMEM should be maintained and extended to the scope of the GPIO pinmap. Performance is often key, especially since these products are often presented as good for robotics etc; thus degrading performance via a layer of kernel modules such as sysfs is not only going to not serve the vision of the product, it will be a disservice to the user base.

Just a friendly 2 cents. Smiles.

@jgarzagu
Copy link

I support what BTBlueSkies says. SPI, I2C, PWM and GPIO are the main peripherals most people use for hardware projects. We can access SPI and I2C with the proper drivers but we are missing PWM. If we could get PWM registers that would be awesome. I can help writing/modifying the driver, but because this driver is already supported in the Raspbian image I would prefer that the change was done into this driver.

Thanks.

@Wren6991
Copy link
Contributor Author

This patch probably won't be updated, because it is a hack. It solved an
immediate problem but is not a final answer - userspace pseudo-drivers are
not the way to do things. A proper driver for GPIO + PWM would be better
long-term than userspace poking, so if you have any suggestions for this
then that would be useful.

If you want to patch this driver yourself, you should just be able to add
the PWM page address to gpiomem.c, as long as nothing else is on that page.

On Mon, May 16, 2016 at 3:37 AM, Jorge Garza notifications@github.com
wrote:

I support what BTBlueSkies says. SPI, I2C, PWM and GPIO are the main
peripherals most people use for hardware projects. We can access SPI and
I2C with the proper drivers but we are missing PWM. If we could get PWM
registers that would be awesome. I can help writing/modifying the driver,
but because this driver is already supported in the Raspbian image I would
prefer that the change was done into this driver.

Thanks.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#1112 (comment)

@bennuttall
Copy link

We've just added a C library called pigpio which runs as a daemon. It's a good way of doing GPIO in any language (rather than writing to /dev/mem or gpiomem), as you can just communicate using sockets, and the actual GPIO stuff is handled for you (and saves you from reimplementing it all).

I assume the likes of RPi.GPIO will still do it using gpiomem.

I don't know whether pigpio can be pushed all the way to the kernel, but its presence in Raspbian provides a method of doing GPIO without this hack.

popcornmix pushed a commit that referenced this pull request Jun 21, 2016
Further testing with false negatives suppressed by commit 293e242
("rcu: Remove superfluous versions of rcu_read_lock_sched_held()")
identified another unprotected use of RCU from the idle loop.  Because RCU
actively ignores idle-loop code (for energy-efficiency reasons, among
other things), using RCU from the idle loop can result in too-short
grace periods, in turn resulting in arbitrary misbehavior.

The resulting lockdep-RCU splat is as follows:

------------------------------------------------------------------------

===============================
[ INFO: suspicious RCU usage. ]
4.6.0-rc5-next-20160426+ #1112 Not tainted
-------------------------------
include/trace/events/ipi.h:35 suspicious rcu_dereference_check() usage!

other info that might help us debug this:

RCU used illegally from idle CPU!
rcu_scheduler_active = 1, debug_locks = 0
RCU used illegally from extended quiescent state!
no locks held by swapper/0/0.

stack backtrace:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.6.0-rc5-next-20160426+ #1112
Hardware name: Generic OMAP4 (Flattened Device Tree)
[<c0110308>] (unwind_backtrace) from [<c010c3a8>] (show_stack+0x10/0x14)
[<c010c3a8>] (show_stack) from [<c047fec8>] (dump_stack+0xb0/0xe4)
[<c047fec8>] (dump_stack) from [<c010dcfc>] (smp_cross_call+0xbc/0x188)
[<c010dcfc>] (smp_cross_call) from [<c01c9e28>] (generic_exec_single+0x9c/0x15c)
[<c01c9e28>] (generic_exec_single) from [<c01ca0a0>] (smp_call_function_single_async+0 x38/0x9c)
[<c01ca0a0>] (smp_call_function_single_async) from [<c0603728>] (cpuidle_coupled_poke_others+0x8c/0xa8)
[<c0603728>] (cpuidle_coupled_poke_others) from [<c0603c10>] (cpuidle_enter_state_coupled+0x26c/0x390)
[<c0603c10>] (cpuidle_enter_state_coupled) from [<c0183c74>] (cpu_startup_entry+0x198/0x3a0)
[<c0183c74>] (cpu_startup_entry) from [<c0b00c0c>] (start_kernel+0x354/0x3c8)
[<c0b00c0c>] (start_kernel) from [<8000807c>] (0x8000807c)

------------------------------------------------------------------------

Reported-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Tested-by: Tony Lindgren <tony@atomide.com>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: <linux-omap@vger.kernel.org>
Cc: <linux-arm-kernel@lists.infradead.org>
popcornmix pushed a commit that referenced this pull request Sep 19, 2016
Further testing with false negatives suppressed by commit 293e242
("rcu: Remove superfluous versions of rcu_read_lock_sched_held()")
identified a few more unprotected uses of RCU from the idle loop.
Because RCU actively ignores idle-loop code (for energy-efficiency
reasons, among other things), using RCU from the idle loop can result
in too-short grace periods, in turn resulting in arbitrary misbehavior.

The affected function is rpm_suspend().

The resulting lockdep-RCU splat is as follows:

------------------------------------------------------------------------

Warning from omap3

===============================
[ INFO: suspicious RCU usage. ]
4.6.0-rc5-next-20160426+ #1112 Not tainted
-------------------------------
include/trace/events/rpm.h:63 suspicious rcu_dereference_check() usage!

other info that might help us debug this:

RCU used illegally from idle CPU!
rcu_scheduler_active = 1, debug_locks = 0
RCU used illegally from extended quiescent state!
1 lock held by swapper/0/0:
 #0:  (&(&dev->power.lock)->rlock){-.-...}, at: [<c052ee24>] __pm_runtime_suspend+0x54/0x84

stack backtrace:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.6.0-rc5-next-20160426+ #1112
Hardware name: Generic OMAP36xx (Flattened Device Tree)
[<c0110308>] (unwind_backtrace) from [<c010c3a8>] (show_stack+0x10/0x14)
[<c010c3a8>] (show_stack) from [<c047fec8>] (dump_stack+0xb0/0xe4)
[<c047fec8>] (dump_stack) from [<c052d7b4>] (rpm_suspend+0x604/0x7e4)
[<c052d7b4>] (rpm_suspend) from [<c052ee34>] (__pm_runtime_suspend+0x64/0x84)
[<c052ee34>] (__pm_runtime_suspend) from [<c04bf3bc>] (omap2_gpio_prepare_for_idle+0x5c/0x70)
[<c04bf3bc>] (omap2_gpio_prepare_for_idle) from [<c01255e8>] (omap_sram_idle+0x140/0x244)
[<c01255e8>] (omap_sram_idle) from [<c0126b48>] (omap3_enter_idle_bm+0xfc/0x1ec)
[<c0126b48>] (omap3_enter_idle_bm) from [<c0601db8>] (cpuidle_enter_state+0x80/0x3d4)
[<c0601db8>] (cpuidle_enter_state) from [<c0183c74>] (cpu_startup_entry+0x198/0x3a0)
[<c0183c74>] (cpu_startup_entry) from [<c0b00c0c>] (start_kernel+0x354/0x3c8)
[<c0b00c0c>] (start_kernel) from [<8000807c>] (0x8000807c)

------------------------------------------------------------------------

Reported-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Tested-by: Tony Lindgren <tony@atomide.com>
Tested-by: Guenter Roeck <linux@roeck-us.net>
[ rjw: Subject ]
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
neuschaefer pushed a commit to neuschaefer/raspi-binary-firmware that referenced this pull request Feb 27, 2017
See: raspberrypi/linux#756

kernel: spi-bcm2835: merge upstream patches allowing DMA transfers
See: raspberrypi/linux#1085

kernel: rpisense-fb: add low-light mode and gamma control
See: raspberrypi/linux#1104

kernel: bcm2708-dmaengine: Use more DMA channels (but not 12)
See:  raspberrypi/linux#1113

kernel: Add /dev/gpiomem device for rootless user GPIO access
See: raspberrypi/linux#1112

kernel: Add RaspiDAC3 support

kernel: Rpi 4.1.y spi bcm2835 patches clock-polarity issue
See: raspberrypi/linux#1125

kernel: BCM270X_DT: Add SDIO overlay
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants