leds-gpio on x86

Hello,

I have recently acquired a NAS[1] which is basically an x86_64 with UEFI bios and adequate enclosure (drive trays and associated activity & error leds).

Not being too happy with stock firmware features, I am in the process of replacing it with a general distribution. The distribution works great with stock kernel, but I do not get the error leds (nor, but I'm less interested, the global power/error/usb leds).

Poking in the original firmware, I found a file declaring which SuperIO GPIO pins are used, and confirmed they do work by loading the appropriate gpio driver.

Now, I wonder how far I should go: I would like to use the leds subsystem for easy triggering, but leds-acpi finds nothing. A quick read shows this driver finds relevant devices using openfirmware enumeration functions. If I understand correctly, this would be devicetree on ARM/MIPS/... but ACPI tables on x86. After an uneducated look at disassembled DSDT, I think I found the SuperIO declaration:
Name (IO3B, 0x0290)
[...]
Device (SIO1)

  {
    Name (_HID, EisaId ("PNP0C02") /* PNP Motherboard Resources */)  // _HID: Hardware ID
    [...]
    Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
    {
      [...]
      If (IO3B)
      {
        CreateWordField (CRS, \_SB.PCI0.SBRG.SIO1._Y0A._MIN, GP30)  // _MIN: Minimum Base Address
        CreateWordField (CRS, \_SB.PCI0.SBRG.SIO1._Y0A._MAX, GP31)  // _MAX: Maximum Base Address
        CreateByteField (CRS, \_SB.PCI0.SBRG.SIO1._Y0A._LEN, GPL3)  // _LEN: Length
        GP30 = IO3B /* \IO3B */
        GP31 = IO3B /* \IO3B */
        GPL3 = IO3L /* \IO3L */
      }

(dmesg showing the SuperIO being based at 0x290 and this value not appearing elsewhere) which does not define any GPIO (no GpioIo() in the Device {} block), even less leds.

One option seems to patch this to declare leds, but it seems dangerous (from the examples[2] I found, the change needed is less than 100 lines for the 6 leds, but that's an awful lot of error possibilities for the ACPI illiterate I am).

Another option seems to develop a dumb module just declaring what the leds. This means I'll have to maintain an off-tree module (I do not see how it would detect that it's actually on this NAS model, so upstream inclusion seems unlikely).

Yet another option would be to extend an existing driver, to make it configurable (ex: via sysfs). This would be my favourite (it is clearer to see how it would get merged), but registration need several values (name, pin number, active high/low, maybe color ?) and I have no idea how clean the syntax would be.

Everything seems possible but I cannot tell which has better chances of success.

Is my analysis correct so far ?
Which next step would you recommend ?

[1] QNAP TS-651
[2] https://lwn.net/Articles/612062/

Vincent Pelletier

Does it list the GPIO controller itself in the DSDT? If it does not, you would need to patch that there as well.

Just in case, can you provide me the DSDT? I can take a look.

Indeed toggling random GPIOs might break something ;-)

Sounds correct to me.

I would just go for writing a custom board file that fills in platform data to leds-gpio.c driver.

I do see this in the dsdt:
Name (_HID, "INT33FC" /* Intel Baytrail GPIO Controller */) // _HID: Hardware ID Name (_CID, "INT33FC" /* Intel Baytrail GPIO Controller */) // _CID: Compatible ID

I do not know the distinction between a gpio controller and the chip which has the pins directly, though.

(sent off-list)

Thanks, I'll go this way.

Regards,

Vincent Pelletier

Do you know which GPIOs are used for leds?

Since the GPIO host controller is listed in the DSDT, I think patching the leds device should be pretty simple.

Got it, thanks.

That would be fantastic.

Green status: 62, green, active low
Red status: 63, red, active low
HDD racks: 70 to 75, red, active low
USB: 17, blue, active low

I tested all via sysfs with gpio-f7188x driver loaded (numbers above are the values I wrote to /sys/class/gpio/register).

"green status" led works minimally out of the box: static-on when the box is on, off otherwise (I would make it blink on S3, for example).

Regards,

Vincent Pelletier

Ah, this is different GPIO controller that is listed in the DSDT (the Baytrail GPIO host controller).

In order to get ACPI GPIO stuff work here you would need to first add the GPIO device to the ACPI namespace and then modify gpio-f7188x.c to probe it from there. You would also need to invent a _HID to the device in order to get it matched. Unfortunately using random _HID will make upstreaming the changes difficult.

I gave a shot at writing a module. I got to the point where it builds and loads, but it does not appear to do anything (no error returned modprobe, nothing in dmesg).

I have no idea if the ~4 lines of "actual" code (ie, not just data definition) are even in the good direction actually. I took
http://www.armadeus.com/wiki/index.php?title=GPIO_LEDS
as an example. This is for a platform definition for a devboard (arm ?), so it may be a wrong example. I took a look at other led drivers, and their complexity varies a lot.

Would you mind taking a look ?
https://github.com/vpelletier/linux/commits/ts651

For example, I have no idea how to explicitly depend on gpio-f7188x and leds-gpio.

Vincent Pelletier

One thing I noticed:

	qnap_tsx51_leds_platform_device = platform_device_register_resndata(NULL, "led-gpio", -1, NULL, 0,
		&qnap_tsx51_led_data, sizeof(qnap_tsx51_led_data));

The driver expects "leds-gpio" not "led-gpio".

And indeed, this is what was preventing proper detection. Very nice catch, thanks a lot.

Now, I see two more things I need to do and for which I have no idea:
- Somehow depend on gpio-f7188x and cause leds-gpio to get loaded (is it a dependence too ?).
Module writing documentation mention soft dependencies, but it feels wrong here.
- Somehow detect that it is actually a qnap of expected model (and, by extension, actually implement led count substraction).
I tried (and failed so far) to understand what the original firmware does. dmidecode does not bring something relevant. I have no idea what is typically done in this area.

Regards,

Vincent Pelletier

Once your module gets loaded, it creates the "leds-gpio" platform device which in turn makes the leds-gpio driver to load.

For gpio-f7188x you need to load it manually because it does not have any module strings modprobe can match with a device. You can create platform device for this in your qnap board file as well and then add MODULE_ALIAS() to the driver to get it loaded automatically.

Typically we get necessary information from ACPI or similar device description ;-)

You may check DMI strings in _init() of your board file and only create the platform devices if they match qnap. /sys/class/dmi/id/* should have something to differentiate it from others.

Oh, right, I somehow thought I had to modprobe it too and didn't check without. It does work.

Looking at MODULE_ALIAS, I found "request_module()". It works great.

I've also added declaration for both buttons, and moved my code to drivers/platform/x86/ as it is no longer leds-specific. I found that there are things like laptop-[brand-]specific modules there, so I guess it is reasonable.

Right :) .

Sadly, ACPI-careless OEM is DMI-careless too:

Handle 0x0000, DMI type 0, 24 bytes
BIOS Information
Vendor: American Megatrends Inc.
Version: 5.6.5
Release Date: 05/19/2014
Address: 0xF0000
Runtime Size: 64 kB
ROM Size: 1024 kB
Characteristics:
PCI is supported
BIOS is upgradeable
BIOS shadowing is allowed
Boot from CD is supported
Selectable boot is supported
BIOS ROM is socketed
EDD is supported
5.25"/1.2 MB floppy services are supported (int 13h) 3.5"/720 kB floppy services are supported (int 13h) 3.5"/2.88 MB floppy services are supported (int 13h) Print screen service is supported (int 5h) 8042 keyboard services are supported (int 9h) Serial services are supported (int 14h) Printer services are supported (int 17h) ACPI is supported
USB legacy is supported
BIOS boot specification is supported Targeted content distribution is supported UEFI is supported
BIOS Revision: 5.6

Handle 0x0001, DMI type 1, 27 bytes
System Information
Manufacturer: To be filled by O.E.M. Product Name: To be filled by O.E.M.
Version: To be filled by O.E.M. Serial Number: To be filled by O.E.M.
UUID: 03000200-0400-0500-0006-000700080009 Wake-up Type: Power Switch
SKU Number: To be filled by O.E.M.
Family: To be filled by O.E.M.

Handle 0x0002, DMI type 2, 15 bytes
Base Board Information
Manufacturer: AMI Corporation Product Name: Aptio CRB
Version: To be filled by O.E.M. Serial Number: To be filled by O.E.M. Asset Tag: To be filled by O.E.M. Features:
Board is a hosting board Board is replaceable
Location In Chassis: To be filled by O.E.M. Chassis Handle: 0x0003
Type: Motherboard
Contained Object Handles: 0

Handle 0x0003, DMI type 3, 25 bytes
Chassis Information
Manufacturer: To Be Filled By O.E.M.
Type: Desktop
Lock: Not Present
Version: To Be Filled By O.E.M.
Serial Number: To Be Filled By O.E.M.
Asset Tag: To Be Filled By O.E.M.
Boot-up State: Safe
Power Supply State: Safe
Thermal State: Safe
Security Status: None
OEM Information: 0x00000000
Height: Unspecified
Number Of Power Cords: 1
Contained Elements: 1
<OUT OF SPEC> (0)
SKU Number: To be filled by O.E.M.

BIOS block values look nice, but not specific enough. "Aptio CRB" seems to be an AMI trademark for their UEFI bios. UUID looks garbage (or rather, it looks too clean to be relevant).

So I guess /etc/modules is the way.

I think the module is now quite close to submission for broader review, thanks to your help.

Regards,

Vincent Pelletier

I'm getting more comfortable with leds, and troubles begin. When I enable timer-base led triggering and start even just two leds in short succession (in a shell script), I start seeing:

[ 1210.586990] Trying to free nonexistent resource <000000000000002e-000000000000002f>
[ 1211.227414] Trying to free nonexistent resource <000000000000002e-000000000000002f>
[ 1211.867890] Trying to free nonexistent resource <000000000000002e-000000000000002f>
[ 1212.508299] Trying to free nonexistent resource <000000000000002e-000000000000002f>
[ 1213.148734] Trying to free nonexistent resource <000000000000002e-000000000000002f>
[ 1213.789172] Trying to free nonexistent resource <000000000000002e-000000000000002f>
[ 1214.429607] Trying to free nonexistent resource <000000000000002e-000000000000002f>

2e-2f is the range of the SuperIO, which is requested & released by (at least) gpio-f7188x:

[  535.653716] gpio-f7188x: Found f71869a at 0x2e, revision 32

Unloading all other modules accessing the SuperIO (fintek-cir and f71882fg), I can still reproduce the message.

Also, when this error happens, tty screen can become corrupted (areas become blank, as if space char was replacing large rectangles). Unloading my module (which unregisters leds, in turn unregistering GPIO pins) clears the corruption ("hidden" chars appear again).

Enabling usb-host triggering, the messages appears without a single other led trigger set to something else than "none".

I do not speak C fluently enough to tell if this is a re-entry kind of problem (same module accessing GPIO pins right next to each other at about the same time by different timers).

How can I debug this further ?

Vincent Pelletier

I made a bit of progress: disabling all but one cpu (...which, in this case, just means disabling the second core) makes the error messages go away, and blinking works fine.

There is a lock taken in __request_region, and released around schedule() when muxed. I guess this means region request is just pushed into a queue when resource is busy, then scheduler is told to let whatever else available run.
I have no idea if the lock should be taken before or after remove_wait_queue after schedule call.

FWIW:

# cat /proc/cpuinfo
processor       : 0
vendor_id       : GenuineIntel
cpu family      : 6
model           : 55
model name      : Intel(R) Celeron(R) CPU  J1800  @ 2.41GHz
stepping        : 8
microcode       : 0x811
cpu MHz         : 2582.817
cache size      : 1024 KB
physical id     : 0
siblings        : 1
core id         : 0
cpu cores       : 1
apicid          : 0
initial apicid  : 0
fpu             : yes
fpu_exception   : yes
cpuid level     : 11
wp              : yes
flags           : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology nonstop_tsc aperfmperf pni pclmulqdq dtes64 monitor ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm sse4_1 sse4_2 movbe popcnt tsc_deadline_timer rdrand lahf_lm 3dnowprefetch ida arat epb dtherm tpr_shadow vnmi flexpriority ept vpid tsc_adjust smep erms
bugs            :
bogomips        : 4825.60
clflush size    : 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

I'm looking for some resource tracing mechanism, but haven't found one yet. I suspect that there is a driver I haven't identified yet which would allocate resources incorrectly (or in an incompatible way, at least).

Regards,

Vincent Pelletier

For each GPIO operation the f7188x driver calls
superio_enter()/superio_exit().

However, I don't think that is enough to prevent multiple threads accessing a GPIO at the same time.

One option is to add mutex to the private structure and then do something like this in each callback:

static int f7188x_gpio_get(...)

{
	mutex_lock(&sio->lock);
	
	/* request the resource and touch the hardware */

	mutex_unlock(&sio->lock);

	...
}

It is a bit weird that it needs to acquire/release the ioport region each time.

(CC'ing gpio-f7188x author)

Discussion summary:
I'm writing a minimal platform driver, declaring 8 GPIO leds and 2 GPIO input keys. When setting two leds to blink ("timer" trigger) in very short succession (ie, enable both next to each other in shell script), kernel emits:

[ 1210.586990] Trying to free nonexistent resource <000000000000002e-000000000000002f>
[ 1211.227414] Trying to free nonexistent resource <000000000000002e-000000000000002f>
[ 1211.867890] Trying to free nonexistent resource <000000000000002e-000000000000002f>
[ 1212.508299] Trying to free nonexistent resource <000000000000002e-000000000000002f>
[ 1213.148734] Trying to free nonexistent resource <000000000000002e-000000000000002f>
[ 1213.789172] Trying to free nonexistent resource <000000000000002e-000000000000002f>
[ 1214.429607] Trying to free nonexistent resource <000000000000002e-000000000000002f>

This is with 500ms on & 500ms off, which matches the aproximate 2 lines/s from this log.
This error goes away when running on a single core. 2e-2f is the f7188x GPIO address range.

There several modules which are using the same two IO addresses for the many function the SuperIO has, I assumed acquiring the IO range would prevent them to do any access once the first is loaded.

But as you point out, it seem to not be the case. I also noticed the fintek-cir driver (for IR sensor, also managed by GPIO) calls request_region in .probe and release_region() in .remove, which also goes in this direction.

Regards,

Vincent Pelletier

Hello,

I did a first pass on gpio-f7188x module, and pushed several commits in my current working branch:

https://github.com/vpelletier/linux/commits/ts651

This commit contains the relevant part of my changes:
gpio: gpio-f7188x: Use mutex for access serialisation.
(currently:
https://github.com/vpelletier/linux/commit/0fecf37bcee67d577f7e1e8e58afc463f452bcff
)
but "of course", it does not work: kernel Oops as soon as I trigger the concurrent access which caused the original issue. With a single active core, it works fine of course.

Sadly, the traceback is longer than on-screen info, despite being at 1920x1080 resolution with default console font. I recall reading about a project to display kernel errors as qr-codes, did it get somewhere ? Also, watchdog kicks in a few seconds after, complaining about hard lock on (at least) one core, and scrolling even more. I suspect both cores are hard locked, and have so far no idea what I did wrong:
- each acquire is followed by a release, with no code path around it
- gpio_device.can_sleep is true (as was the case before my patch)

...would I have somehow poked at another bug ?

Regards,

Vincent Pelletier