shutdown trigger on gpio_keys.X for armhf hardware

Bug #1347776 reported by Manoj Iyer
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
systemd
Fix Released
Medium
systemd (Ubuntu)
Fix Released
Undecided
Unassigned
Trusty
Fix Released
Undecided
Manoj Iyer
Utopic
Fix Released
Undecided
Unassigned

Bug Description

Some ARM board uses GPIO gpio_key.12 for power control (key=116). The proposed patch adds entry to logind's 70-power-switch.rules to initiate soft shutdown of the cartridge from ilo.

Here is the udevadm output for /dev/input/event0

sudo udevadm info --query=all --name=/dev/input/event0 --attribute-walk

Udevadm info starts with the device specified by the devpath and then
walks up the chain of parent devices. It prints for every device
found, all possible attributes in the udev rules key format.
A rule to match, can be composed by the attributes of the device
and the attributes from one single parent device.

  looking at device '/devices/soc.3/gpio_keys.12/input/input0/event0':
    KERNEL=="event0"
    SUBSYSTEM=="input"
    DRIVER==""

  looking at parent device '/devices/soc.3/gpio_keys.12/input/input0':
    KERNELS=="input0"
    SUBSYSTEMS=="input"
    DRIVERS==""
    ATTRS{name}=="gpio_keys.12"
    ATTRS{phys}=="gpio-keys/input0"
    ATTRS{uniq}==""
    ATTRS{properties}=="0"

  looking at parent device '/devices/soc.3/gpio_keys.12':
    KERNELS=="gpio_keys.12"
    SUBSYSTEMS=="platform"
    DRIVERS=="gpio-keys"
    ATTRS{keys}=="116"
    ATTRS{switches}==""
    ATTRS{disabled_keys}==""
    ATTRS{disabled_switches}==""

  looking at parent device '/devices/soc.3':
    KERNELS=="soc.3"
    SUBSYSTEMS=="platform"
    DRIVERS==""

Regarding the possibility of gpio_key.12 being used by other systems to map to some other trigger, I put in the check that the gpio_key.12 is associated with power control (keys=116). '116' is supposedly linux generic power control in DTS. There is no uniq idSystem or idVendor for device /dev/input/event0 as you can see from udevadm output, therefore I tried to use the best available combination as a safety check. This patch will enable power control for any system vendor (Other than the one the patch in intended for) that describes in DTS, the trigger gpio_key.12 as power control (116).

SRU Request
==========

[Impact]

* User won't be able to initiate a soft shutdown from the chassis manager.

[Test Case]

* To reproduce this bug, initiate a soft shutdown from the chassis manager, for example from ilo you could do
<ilo> set node power off shutdown <node number>

[Test Result]

== BEFORE PATCH ==

$ cat /lib/udev/rules.d/70-power-switch.rules
# This file is part of systemd.
#
# systemd is free software; you can redistribute it and/or modify it
# under the terms of the GNU Lesser General Public License as published by
# the Free Software Foundation; either version 2.1 of the License, or
# (at your option) any later version.

ACTION=="remove", GOTO="power_switch_end"

SUBSYSTEM=="input", KERNEL=="event*", SUBSYSTEMS=="acpi", TAG+="power-switch"
SUBSYSTEM=="input", KERNEL=="event*", KERNELS=="thinkpad_acpi", TAG+="power-switch"

LABEL="power_switch_end"
$

</>hpiLO-> set node power off shutdown c3n2

  c3: #Cartridge 3
    c3n2: #Node 2 Shutting node down gracefully

hpiLO-> show node list

Slot ID Proc Manufacturer Architecture Memory Power Health
---- ----- ---------------------- -------------------- ------ ----- ------
  3 c3n1 **** ARM Architecture 8 GB On OK
  3 c3n2 **** ARM Architecture 8 GB On OK
  3 c3n3 **** ARM Architecture 8 GB On OK
  3 c3n4 **** ARM Architecture 8 GB Off OK

hpiLO->

== AFTER PATCH ==

hpiLO-> set node power off shutdown c3n1

  c3: #Cartridge 3
    c3n1: #Node 1 Shutting node down gracefully

hpiLO-> show node list

Slot ID Proc Manufacturer Architecture Memory Power Health
---- ----- ---------------------- -------------------- ------ ----- ------
  3 c3n1 **** ARM Architecture 8 GB Off OK
  3 c3n2 **** ARM Architecture 8 GB On OK
  3 c3n3 **** ARM Architecture 8 GB On OK
  3 c3n4 **** ARM Architecture 8 GB Off OK

hpiLO->

[Regression Potential]

None.
Note: Regarding the possibility of gpio_key.12 being used by other systems to map to some other trigger, I put in the check that the gpio_key.12 is associated with power control (keys=116). '116' is supposedly linux generic power control in DTS.

Related branches

Revision history for this message
Manoj Iyer (manjo) wrote :
Revision history for this message
Ubuntu Foundations Team Bug Bot (crichton) wrote :

Thank you for taking the time to report this bug and helping to make Ubuntu better. It seems that your bug report is not filed about a specific source package though, rather it is just filed against Ubuntu in general. It is important that bug reports be filed about source packages so that people interested in the package can find the bugs about it. You can find some hints about determining what package your bug might be about at https://wiki.ubuntu.com/Bugs/FindRightPackage. You might also ask for help in the #ubuntu-bugs irc channel on Freenode.

To change the source package that this bug is filed about visit https://bugs.launchpad.net/ubuntu/+bug/1347776/+editstatus and add the package name in the text box next to the word Package.

[This is an automated message. I apologize if it reached you inappropriately; please just reply to this message indicating so.]

tags: added: bot-comment
Revision history for this message
Manoj Iyer (manjo) wrote :

SRU Request
==========

[Impact]

* User won't be able to initiate a soft shutdown from the chassis manager.

[Test Case]

* To reproduce this bug, initiate a soft shutdown from the chassis manager, for example from ilo you could do
<ilo> set node power off shutdown <node number>

[Test Result]

== BEFORE PATCH ==

$ cat /lib/udev/rules.d/70-power-switch.rules
# This file is part of systemd.
#
# systemd is free software; you can redistribute it and/or modify it
# under the terms of the GNU Lesser General Public License as published by
# the Free Software Foundation; either version 2.1 of the License, or
# (at your option) any later version.

ACTION=="remove", GOTO="power_switch_end"

SUBSYSTEM=="input", KERNEL=="event*", SUBSYSTEMS=="acpi", TAG+="power-switch"
SUBSYSTEM=="input", KERNEL=="event*", KERNELS=="thinkpad_acpi", TAG+="power-switch"

LABEL="power_switch_end"
$

</>hpiLO-> set node power off shutdown c3n2

  c3: #Cartridge 3
    c3n2: #Node 2 Shutting node down gracefully

hpiLO-> show node list

Slot ID Proc Manufacturer Architecture Memory Power Health
---- ----- ---------------------- -------------------- ------ ----- ------
  3 c3n1 **** ARM Architecture 8 GB On OK
  3 c3n2 **** ARM Architecture 8 GB On OK
  3 c3n3 **** ARM Architecture 8 GB On OK
  3 c3n4 **** ARM Architecture 8 GB Off OK

hpiLO->

== AFTER PATCH ==

hpiLO-> set node power off shutdown c3n1

  c3: #Cartridge 3
    c3n1: #Node 1 Shutting node down gracefully

hpiLO-> show node list

Slot ID Proc Manufacturer Architecture Memory Power Health
---- ----- ---------------------- -------------------- ------ ----- ------
  3 c3n1 **** ARM Architecture 8 GB Off OK
  3 c3n2 **** ARM Architecture 8 GB On OK
  3 c3n3 **** ARM Architecture 8 GB On OK
  3 c3n4 **** ARM Architecture 8 GB Off OK

hpiLO->

[Regression Potential]

None.
Note: Regarding the possibility of gpio_key.12 being used by other systems to map to some other trigger, I put in the check that the gpio_key.12 is associated with power control (keys=116). '116' is supposedly linux generic power control in DTS.

Martin Pitt (pitti)
affects: ubuntu → systemd (Ubuntu)
Changed in systemd (Ubuntu):
status: New → In Progress
Revision history for this message
Martin Pitt (pitti) wrote :

Committed to packaging git for utopic:

http://anonscm.debian.org/gitweb/?p=pkg-systemd/systemd.git;a=commitdiff;h=47c10f310

I also uploaded that fix to trusty's SRU review queue.

Changed in systemd (Ubuntu Utopic):
status: In Progress → Fix Committed
Changed in systemd (Ubuntu Trusty):
status: New → In Progress
assignee: nobody → Manoj Iyer (manjo)
Revision history for this message
Steve Langasek (vorlon) wrote : Please test proposed package

Hello Manoj, or anyone else affected,

Accepted systemd into trusty-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/systemd/204-5ubuntu20.4 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Changed in systemd (Ubuntu Trusty):
status: In Progress → Fix Committed
tags: added: verification-needed
Manoj Iyer (manjo)
description: updated
Revision history for this message
Kodamati Pradeep Vinesh Reddy (pradeep-reddy2) wrote :

With the package from the proposed it seems to be working fine. I have verified the same.

Thanks,
Pradeep.

tags: added: verification-done
removed: verification-needed
Revision history for this message
Manoj Iyer (manjo) wrote :

I tested the systemd package from proposed and it works as expected. Below are the test results:

ubuntu@ubuntu:~$ sudo apt-get dist-upgrade

Reading package lists... Done
Building dependency tree
Reading state information... Done
Calculating upgrade... Done
The following packages will be upgraded:
  accountsservice apport apt apt-transport-https apt-utils base-files bsdutils
  byobu cloud-init dbus dpkg file gcc-4.9-base gnupg gpgv grub-legacy-ec2
  initramfs-tools initramfs-tools-bin language-selector-common
  libaccountsservice0 libapt-inst1.5 libapt-pkg4.12 libblkid1
  libboost-iostreams1.54.0 libc-bin libc6 libcgmanager0 libdbus-1-3 libgcc1
  libjson-c2 libjson0 libmagic1 libmount1 libpam-systemd libssl1.0.0
  libsystemd-daemon0 libsystemd-login0 libtasn1-6 libudev1 libuuid1 libxml2
  mount multiarch-support net-tools openssl python3-apport python3-distupgrade
  python3-gi python3-problem-report resolvconf systemd-services tzdata
  ubuntu-release-upgrader-core udev upstart util-linux uuid-runtime

ubuntu@ubuntu:~$ cat /lib/udev/rules.d/70-power-switch.rules
# This file is part of systemd.
#
# systemd is free software; you can redistribute it and/or modify it
# under the terms of the GNU Lesser General Public License as published by
# the Free Software Foundation; either version 2.1 of the License, or
# (at your option) any later version.

ACTION=="remove", GOTO="power_switch_end"

SUBSYSTEM=="input", KERNEL=="event*", SUBSYSTEMS=="acpi", TAG+="power-switch"
SUBSYSTEM=="input", KERNEL=="event*", KERNELS=="thinkpad_acpi", TAG+="power-switch"
SUBSYSTEM=="input", KERNEL=="event*", SUBSYSTEMS=="platform", KERNELS=="gpio_keys.12", ATTRS{keys}=="116", PROGRAM="/bin/cat /proc/device-tree/model", RESULT=="HP ProLiant m800 Server Cartridge", TAG+="power-switch"

LABEL="power_switch_end"
ubuntu@ubuntu:~$
</>hpiLO-> show node list

Slot ID Proc Manufacturer Architecture Memory Power Health
---- ----- ---------------------- -------------------- ------ ----- ------
  3 c3n1 ************************************ 8 GB Off OK
  3 c3n2 ************************************ 8 GB On OK
  3 c3n3 ************************************ 8 GB Off OK
  3 c3n4 ************************************ 8 GB On OK

hpiLO->
hpiLO-> set node power off shutdown c3n4

  c3: #Cartridge 3
    c3n4: #Node 4 Shutting node down gracefully

hpiLO->

ubuntu@ubuntu:~$
Broadcast message from root@ubuntu
 (unknown) at 19:40 ...

The system is going down for power off NOW!
Connection to 192.168.17.25 closed by remote host.
Connection to 192.168.17.25 closed.
ubuntu@sm2:~$

hpiLO-> show node list

Slot ID Proc Manufacturer Architecture Memory Power Health
---- ----- ---------------------- -------------------- ------ ----- ------
  3 c3n1 ************************************ 8 GB Off OK
  3 c3n2 ************************************ 8 GB On OK
  3 c3n3 ************************************ 8 GB Off OK
  3 c3n4 ************************************ 8 GB Off OK

hpiLO->

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package systemd - 208-7ubuntu4

---------------
systemd (208-7ubuntu4) utopic; urgency=medium

  * Lower Breaks: to lvm2 again. Our lvm2 package has always used udev for
    device setup, and thus should be compatible with systemd, too.
 -- Martin Pitt <email address hidden> Thu, 07 Aug 2014 07:11:28 +0200

Changed in systemd (Ubuntu Utopic):
status: Fix Committed → Fix Released
Revision history for this message
In , dann frazier (dannf) wrote :

There are a couple of ARM-based server systems that use the gpio-keyboard and gpio-keyboard-polled drivers to send an event requesting a shutdown. The following rules DTRT, though obviously it'd be nice if a more generic (but still safe) rule could be devised.

SUBSYSTEM=="input", KERNEL=="event*", SUBSYSTEMS=="platform", KERNELS=="gpio-keys.6", PROGRAM="/bin/grep '^HP ProLiant m400 Server Cartridge$' /proc/device-tree/model", TAG+="power-switch"

SUBSYSTEM=="input", KERNEL=="event*", SUBSYSTEMS=="platform", KERNELS=="gpio_keys.12", ATTRS{keys}=="116", PROGRAM="/bin/cat /proc/device-tree/model", RESULT=="HP ProLiant m800 Server Cartridge", TAG+="power-switch"

You'll notice a couple of differences between the two. The m800 rule specifies a key attribute where the m400 rule does not. This is because the first uses a polled gpio via the gpio-keyboard-polled driver. This driver does not expose a "keys" attribute in sysfs. The m400 uses the gpio-keyboard driver, which does.

You'll also notice the grep vs. cat/RESULT methods. Not sure if upstream has a preference.

It might make sense to drop the model/pin matching of the m800 rule, and bind to any ATTR{keys}=="116" - this is defined as KEY_POWER in <linux/input.h>. Of course, that won't help the -polled variant.

Changed in systemd:
importance: Unknown → Medium
status: Unknown → Confirmed
Revision history for this message
In , dann frazier (dannf) wrote :

Note that, due to a firmware change, the m400 rule should use "gpio_keys" instead of "gpio-keys". Here is the new rule:

SUBSYSTEM=="input", KERNEL=="event*", SUBSYSTEMS=="platform", KERNELS=="gpio_keys.6", PROGRAM="/bin/grep '^HP ProLiant m400 Server Cartridge$' /proc/device-tree/model", TAG+="power-switch"

Revision history for this message
In , Kay Sievers (kaysievers) wrote :

(In reply to comment #1)
> Note that, due to a firmware change, the m400 rule should use "gpio_keys"
> instead of "gpio-keys". Here is the new rule:
>
> SUBSYSTEM=="input", KERNEL=="event*", SUBSYSTEMS=="platform",
> KERNELS=="gpio_keys.6", PROGRAM="/bin/grep '^HP ProLiant m400 Server
> Cartridge$' /proc/device-tree/model", TAG+="power-switch"

That's a neat hack, but in no way upstreamable.

Upstream udev dev will not read the device information from /proc, and will
also not call any external tools like grep.

This all needs to be solved properly on the kernel side, to export real devices
and allow efficient and reliable matching from userspace tools.

Revision history for this message
In , dann frazier (dannf) wrote :

Right, that wasn't intended to be an upstream patch submittal, just a description of the problem.

My best guess at a "good" solution would be something like this:

SUBSYSTEM=="input", KERNEL=="event*", SUBSYSTEMS=="platform", ATTRS{keys}=="116", TAG+="power-switch"

Which would obviously require some new support in gpio-keyboard-polled to expose the ATTRS{keys}. But I don't know if it is safe to assume that KEY_POWER is always a graceful powerdown request.

Revision history for this message
In , Lennart-poettering (lennart-poettering) wrote :

I think the proper way to fix this is by waiting for this kernel patch to be applied:

http://www.spinics.net/lists/linux-input/msg33007.html

With that in place we can then update logind to make use of it, and then simply add the udev tag to all keyboards that are discovered, which would then implicitly include any gpio kbd devices.

Without that kernel patch I am a bit concerned about listening to all of the system's keypresses, as that might be a bit too much.

Revision history for this message
Scott Kitterman (kitterman) wrote : Update Released

The verification of the Stable Release Update for systemd has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package systemd - 204-5ubuntu20.4

---------------
systemd (204-5ubuntu20.4) trusty-proposed; urgency=medium

  * Add HP ProLiant m800 Server Cartridge power control support. The cartridge
    uses gpio_keys.12 to emulate shutdown. (LP: #1347776)
 -- Manoj Iyer <email address hidden> Mon, 21 Jul 2014 17:58:06 -0500

Changed in systemd (Ubuntu Trusty):
status: Fix Committed → Fix Released
Revision history for this message
In , Lucas Magasweran (lucasrangit) wrote :

I added the keys sysfs attribute to gpio-keys-polled.c. Please try this patch (should apply on ubuntu-linux 4.3).

diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
index 11e77a9..3a8ee50 100644
--- a/drivers/input/keyboard/gpio_keys_polled.c
+++ b/drivers/input/keyboard/gpio_keys_polled.c
@@ -43,6 +43,85 @@ struct gpio_keys_polled_dev {
  struct gpio_keys_button_data data[0];
 };

+/**
+ * get_n_events_by_type() - returns maximum number of events per @type
+ * @type: type of button (%EV_KEY, %EV_SW)
+ *
+ * Return value of this function can be used to allocate bitmap
+ * large enough to hold all bits for given type.
+ */
+static inline int get_n_events_by_type(int type)
+{
+ BUG_ON(type != EV_SW && type != EV_KEY);
+
+ return (type == EV_KEY) ? KEY_CNT : SW_CNT;
+}
+
+/**
+ * gpio_keys_polled_attr_show_helper() - fill in stringified bitmap of buttons
+ * @ddata: pointer to drvdata
+ * @buf: buffer where stringified bitmap is written
+ * @type: button type (%EV_KEY, %EV_SW)
+ *
+ * Returns 0 on success or negative errno on failure.
+ */
+static ssize_t gpio_keys_polled_attr_show_helper(
+ struct gpio_keys_polled_dev *ddata,
+ char *buf, unsigned int type)
+{
+ int n_events = get_n_events_by_type(type);
+ unsigned long *bits;
+ ssize_t ret;
+ int i;
+
+ bits = kcalloc(BITS_TO_LONGS(n_events), sizeof(*bits), GFP_KERNEL);
+ if (!bits)
+ return -ENOMEM;
+
+ for (i = 0; i < ddata->pdata->nbuttons; i++) {
+ struct gpio_keys_button *bdata = &ddata->pdata->buttons[i];
+
+ if (bdata->type != type)
+ continue;
+
+ __set_bit(bdata->code, bits);
+ }
+
+ ret = scnprintf(buf, PAGE_SIZE - 1, "%*pbl", n_events, bits);
+ buf[ret++] = '\n';
+ buf[ret] = '\0';
+
+ kfree(bits);
+
+ return ret;
+}
+
+static ssize_t gpio_keys_polled_show_keys(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct gpio_keys_polled_dev *ddata = platform_get_drvdata(pdev);
+
+ return gpio_keys_polled_attr_show_helper(ddata, buf, EV_KEY);
+}
+
+/*
+ * ATTRIBUTES:
+ *
+ * /sys/devices/platform/gpio-keys-polled/keys [ro]
+ */
+static DEVICE_ATTR(keys, S_IRUGO, gpio_keys_polled_show_keys, NULL);
+
+static struct attribute *gpio_keys_polled_attrs[] = {
+ &dev_attr_keys.attr,
+ NULL,
+};
+
+static struct attribute_group gpio_keys_polled_attr_group = {
+ .attrs = gpio_keys_polled_attrs,
+};
+
 static void gpio_keys_polled_check_state(struct input_dev *input,
       struct gpio_keys_button *button,
       struct gpio_keys_button_data *bdata)
@@ -286,6 +365,13 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
  bdev->pdata = pdata;
  platform_set_drvdata(pdev, bdev);

+ error = sysfs_create_group(&pdev->dev.kobj, &gpio_keys_polled_attr_group);
+ if (error) {
+ dev_err(dev, "Unable to export keys, error: %d\n",
+ error);
+ return error;
+ }
+
  error = input_register_polled_device(poll_dev);
  if (error) {
   dev_err(dev, "unable to register polled device, err=%d\n",

Revision history for this message
Lucas Magasweran (lucasrangit) wrote :

gpio-keys-polled: add sysfs bitmask to export supported keycodes

Revision history for this message
In , Lennart-poettering (lennart-poettering) wrote :

This has been implemented a while back, see 405e116f57b1cf33d4ca0294ab045a9f709bbc96

Changed in systemd:
status: Confirmed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.