Merge lp:~manjo/ubuntu/trusty/systemd/HP-m800 into lp:ubuntu/trusty/systemd

[Manoj Iyer]

HP ProLiant m800 Server Cartridge 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

ubuntu@c3n1:~$ 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==""

ubuntu@c3n1:~$

This patch was tested by me and verified to work on HP m800 cartridge at Canonical.

[Adam Conrad (adconrad)]

Requesting a review from Martin, who probably knows this bit better than I do. Is this safe to do, globally like this, or could it cause unintended weird consequences on other random platforms that happen to have something wired up to the same gpio?

[Manoj Iyer (manjo)]

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. It would have been nice if HP's fw had something like idSystem or idVendor attribute to /dev/input/event0 but that does not exist, in which case, 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).

[Martin Pitt (pitti)]

This looks ok to me as an SRU. But please file a bug report for it for SRU tracking/verification. Thanks!

review: Approve

[Martin Pitt (pitti)]

Note that a rule can still refer to the general DMI data, like this:

SUBSYSTEM=="input", KERNEL=="event*", SUBSYSTEMS=="platform", KERNELS=="gpio_keys.12", ATTR{[dmi/id]sys_vendor}=="HP*", ATTR{[dmi/id]product_name}=="SuperCalc 100[234]*", ATTRS{keys}=="116", TAG+="power-switch"

So it is easily possible to limit such a rule to particular systems.

review: Approve

[dann frazier (dannf)]

> Note that a rule can still refer to the general DMI data, like this:
>
> SUBSYSTEM=="input", KERNEL=="event*", SUBSYSTEMS=="platform",
> KERNELS=="gpio_keys.12", ATTR{[dmi/id]sys_vendor}=="HP*",
> ATTR{[dmi/id]product_name}=="SuperCalc 100[234]*", ATTRS{keys}=="116", TAG
> +="power-switch"
>
> So it is easily possible to limit such a rule to particular systems.

The systems in question don't provide DMI data. Do you know of an equivalent with device-tree? I don't see any device-tree attributes available in sysfs - the best system identifier I see is /proc/device-tree/model.

[Martin Pitt (pitti)]

dann frazier [2014-07-28 14:17 -0000]:
> The systems in question don't provide DMI data. Do you know of an
> equivalent with device-tree? I don't see any device-tree attributes
> available in sysfs - the best system identifier I see is
> /proc/device-tree/model.

Meh, too bad. I suppose an equivalent would be

  PROGRAM="/bin/grep FancyModel /proc/device-tree/model"

?

[Adam Conrad (adconrad)]

While having grep in a udev rule is ugly, I assume that would only trigger if the other conditions are met (something attached to that GPIO pin that we think we might want to play with), right? If so, I think it would be safer to do the device-tree model check than to just let the rule run free.

[Aalap Tripathy (aalap-tripathy)]

Sorry to be jumping in - Isn't this any help to isolate to the m800 cartridge ?
cat /proc/device-tree/model
HP ProLiant m800 Server Cartridge

[Martin Pitt (pitti)]

Setting my review status back to "needs fixing" until you can test the grep rule suggested above, and this is working for both the systems you are interested in. Thanks!

review: Needs Fixing

[Manoj Iyer (manjo)]

> Setting my review status back to "needs fixing" until you can test the grep
> rule suggested above, and this is working for both the systems you are
> interested in. Thanks!

I think we are confusing two different platforms, What dannf is talking about is a different platform. The SRU I proposed is for HP-m800 server cartridges. There wont be a generic solution that will address soft shutdown for both platforms. On m800 platform gpio_keys.12 is associated with power control (attribute 116 which is generic power control attr for linux).

[lp:~manjo/ubuntu/trusty/systemd/HP-m800]

75. By Manoj Iyer on 2014-08-05

check device tree for platform name

[Manoj Iyer (manjo)]

pitti/adam,

I have added a check to make sure the gpio_keys trigger are confined to m800 server cartridge by checking proc/device-tree/model. I think this solution is better than the PROGRAM='grep' that was suggested earlier.

Can you please review and accept? This fix was tested by me on m800 cartridge (c3n3) at canonical.

Thanks

[Martin Pitt (pitti)]

For efficiency reasons I'd like to avoid calling the two shells and the cat on every platform. This can be done more efficiently with

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

Then on any system which doesn't have a gpio_keys.12 there will be no additional program calls at all, and on that device it's a single cat (and if the file isn't pesent it'll just fail and RESULT will be empty).

Can you please double-check that?

review: Needs Fixing

[lp:~manjo/ubuntu/trusty/systemd/HP-m800]

76. By Manoj Iyer on 2014-08-05

limit device tree check to the rule, so that it does not impact other platforms or archs

[Manoj Iyer (manjo)]

== C3N3 ===

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

LABEL="power_switch_end"

ubuntu@ubuntu:~$ uname -a
Linux ubuntu 3.13.0-8-keystone #12-Ubuntu SMP Sat Jul 19 07:56:07 UTC 2014 armv7l armv7l armv7l GNU/Linux

ubuntu@ubuntu:~$ cat /proc/device-tree/model
HP ProLiant m800 Server Cartridgeubuntu@ubuntu:~$

== INITIATING SOFT SHUTDOWN FROM ILO ==
</>hpiLO-> set node power off shutdown c3n3

  c3: #Cartridge 3
    c3n3: #Node 3 Shutting node down gracefully

hpiLO->

== SYSTEM SHUTTING DOWN AS EXPECTED ==
ubuntu@ubuntu:~$ cat /proc/device-tree/model
HP ProLiant m800 Server Cartridgeubuntu@ubuntu:~$
Broadcast message from root@ubuntu
 (unknown) at 11:49 ...

The system is going down for power off NOW!
Connection to 192.168.17.24 closed by remote host.
Connection to 192.168.17.24 closed.

== CONFIRM WITH ILO THAT NODE IS POWERED DOWN ==
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 Off OK
  3 c3n4 * ARM Architecture 8 GB On OK

hpiLO->

[Martin Pitt (pitti)]

Splendid!

I massaged that into a proper debian/patches patch and added a bug ref to the changelog. Committed to packaging git for utopic:

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

and also uploaded to trusty's SRU review queue. I'm closing this MP now as it won't be literally merged into the UDD branches.

Thanks!

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2014-04-14 16:20:35 +0000
+++ debian/changelog	2014-08-05 15:39:10 +0000
@@ -1,3 +1,10 @@
+systemd (204-5ubuntu21) trusty; urgency=medium
+
+  * Add HP ProLiant m800 Server Cartridge power control support. The
+    cartridge uses gpio_keys.12 to emulate shutdown.
+
+ -- Manoj Iyer <[e-mail]>  Mon, 21 Jul 2014 17:58:06 -0500
+
 systemd (204-5ubuntu20) trusty; urgency=medium
 
   * systemd-logind.conf: Don't use the limit stanza which fails the

=== modified file 'src/login/70-power-switch.rules'
--- src/login/70-power-switch.rules	2013-03-13 10:46:40 +0000
+++ src/login/70-power-switch.rules	2014-08-05 15:39:10 +0000
@@ -7,6 +7,7 @@
 
 ACTION=="remove", GOTO="power_switch_end"
 
+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"
 SUBSYSTEM=="input", KERNEL=="event*", SUBSYSTEMS=="acpi", TAG+="power-switch"
 SUBSYSTEM=="input", KERNEL=="event*", KERNELS=="thinkpad_acpi", TAG+="power-switch"