gpio shutdown trigger for ProLiant m400 cartridges

[dann frazier]

Similar to the m800 case in LP: #1347776, udev/systemd also needs to know how to trigger a graceful shutdown on ProLiant m400 cartridges.

[Impact]
Without this change, the power down button will not initiate an OS shutdown on this platform.
[Test Case]
Push button. Watch poweroff occur.
[Regression Potential]
The new rule is pretty tightly bound to the m400 system - so the risk of this rule matching and impacting behavior on a different platform is minimal. This will presumably cause new code to run and read /proc/device-tree/model on other platforms - but this is already done for the m800 system in both utopic and trusty.

[Alberto Salvia Novella (es20490446e)]

It's a usability issue that doesn't limit the functionality of a core package.

[dann frazier (dannf)]

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.

[Ubuntu Foundations Team Bug Bot (crichton)]

The attachment "patch for utopic's systemd" seems to be a debdiff. The ubuntu-sponsors team has been subscribed to the bug report so that they can review and hopefully sponsor the debdiff. If the attachment isn't a patch, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are member of the ~ubuntu-sponsors, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issue please contact him.]

[dann frazier (dannf)]

The vendor has requested we use the string "gpio_keys" instead of "gpio-keys" to match current convention. Updated patches attached.

[dann frazier (dannf)]

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"

[Kay Sievers (kaysievers)]

(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.

[dann frazier (dannf)]

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.

[Lennart-poettering (lennart-poettering)]

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.

[Martin Pitt (pitti)]

Dann,

I think it's more consistent to use the same rule structure as in ProLiant-m800-power-button.patch, i. e. "cat" and RESULT=. If you want to use grep, then please use "grep -q". I'm also ok with using grep in both rules, but we should just use one form.

Also, please merge this into the previous patch, and rename to ProLiant-power-button.patch; there might be more platforms, and we don't want dozens of these very closely related micro-patches. That will make patch review/changing, easier and reduce patch interdependencies.

Otherwise this LGTM. Please don't upload the utopic one, I'll apply your final patch for trusty to the Debian/Ubuntu packaging git and then upload with some other changes soon. (I suppose the pressing part is trusty, not utopic?)

Thanks!

[dann frazier (dannf)]

Martin,
 Thanks for the review. This squashes the proliant support patches into one and uses the RESULT== format consistently. This patch is against trusty, but you should be able to use the included ProLiant-power-button.patch patch directly in utopic.

 It is true that trusty is more pressing, was just trying to follow "dev-release-first" protocol. And no risk of me uploading anything, I'm not an Ubuntu developer :)

[Martin Pitt (pitti)]

Utopic fix committed to git: http://anonscm.debian.org/gitweb/?p=pkg-systemd/systemd.git;a=commitdiff;h=65952fad

[Martin Pitt (pitti)]

Trusty fix uploaded to SRU review queue. Unsubscribing sponsors. Thanks!

[Launchpad Janitor (janitor)]

This bug was fixed in the package systemd - 208-8ubuntu1

---------------
systemd (208-8ubuntu1) utopic; urgency=medium

  [ Martin Pitt ]
  * Merge with Debian unstable.
  * debian/gbp.conf: Put ubuntu tags in the ubuntu/ namespace.

  [ Dann Frazier ]
  * Add support for HP ProLiant m400 Server Cartridge soft powerdown
    (LP: #1354306)

systemd (208-8) unstable; urgency=medium

  [ Martin Pitt ]
  * Fix duplicate line in copyright. (Closes: #756899)
  * Drop --disable-xattr configure option for udeb, does not exist any more.
  * Add Turkish debconf translations. Thanks Mert Dirik! (Closes: #757498)
  * Backport fix for lazy session-activation on non-seat0 seats.
    (LP: #1355331)

  [ Michael Biebl ]
  * Use "kmod static-nodes --output=/proc/self/fd/1" in make_static_nodes() as
    we can't rely on /dev/stdout to exist at this point during boot.
    (Closes: #757830)
  * Fix udev SysV init script and d-i start script to not write to
    /sys/kernel/uevent_helper unconditionally to not fail on a kernel with
    CONFIG_UEVENT_HELPER unset. (Closes: #756312)
  * Add Breaks: kmod (<< 14) to udev to make sure we have a kmod version
    supporting the static-nodes command.
  * Add Breaks: systemd (<< 208) to udev to avoid partial upgrades. Newer udev
    versions rely on kmod-static-nodes.service being provided by systemd.
    (Closes: #757777)
  * Updated upstream v208-stable patch series to 53b1b6c.
  * Cherry-pick upstream fix to ignore temporary dpkg files. (Closes: #757302)
  * Make emergency.service conflict with rescue.service.
    Otherwise if rescue mode is selected during boot and the emergency mode
    is triggered (e.g. via a broken fstab entry), we have two sulogin
    processes fighting over the tty. (Closes: #757072)
  * Stop syslog.socket when entering emergency mode as otherwise every log
    message triggers the start of the syslog service and its dependencies
    which conflicts with emergency.target. (Closes: #755581)
 -- Martin Pitt <email address hidden> Thu, 21 Aug 2014 07:58:30 +0200

[Brian Murray (brian-murray)]

Hello dann, 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.5 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!

[Scott Kitterman (kitterman)]

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.

[Launchpad Janitor (janitor)]

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

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

  * Add support for HP ProLiant m400 Server Cartridge soft powerdown
    (LP: #1354306)
 -- dann frazier <email address hidden> Fri, 08 Aug 2014 13:45:25 +0200

[Lennart-poettering (lennart-poettering)]

This has been implemented a while back, see 405e116f57b1cf33d4ca0294ab045a9f709bbc96