leds-gpio on x86

leds-gpio on x86

[Vincent Pelletier]

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

Re: leds-gpio on x86

[Mika Westerberg]

On Wed, Aug 05, 2015 at 10:09:48PM +0200, Vincent Pelletier wrote:
> 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.

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.

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

Indeed toggling random GPIOs might break something ;-)

> 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 ?

Sounds correct to me.

> Which next step would you recommend ?

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

Re: leds-gpio on x86

[Vincent Pelletier]

On Thu, 6 Aug 2015 19:59:24 +0300, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> Does it list the GPIO controller itself in the DSDT? If it does not, you
> would need to patch that there as well.

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.

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

(sent off-list)

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

Thanks, I'll go this way.

Regards,
-- 
Vincent Pelletier

Re: leds-gpio on x86

[Mika Westerberg]

On Thu, Aug 06, 2015 at 07:17:27PM +0200, Vincent Pelletier wrote:
> On Thu, 6 Aug 2015 19:59:24 +0300, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > Does it list the GPIO controller itself in the DSDT? If it does not, you
> > would need to patch that there as well.
> 
> 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.

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.

> > Just in case, can you provide me the DSDT? I can take a look.
> 
> (sent off-list)

Got it, thanks.

Re: leds-gpio on x86

[Vincent Pelletier]

On Thu, 6 Aug 2015 20:35:58 +0300, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> 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.

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

Re: leds-gpio on x86

[Mika Westerberg]

On Thu, Aug 06, 2015 at 08:18:20PM +0200, Vincent Pelletier wrote:
> On Thu, 6 Aug 2015 20:35:58 +0300, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > 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.
> 
> 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).

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.

Re: leds-gpio on x86

[Vincent Pelletier]

On Fri, 7 Aug 2015 13:57:45 +0300, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> 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

Re: leds-gpio on x86

[Mika Westerberg]

On Sat, Aug 08, 2015 at 02:06:56PM +0200, Vincent Pelletier wrote:
> On Fri, 7 Aug 2015 13:57:45 +0300, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > 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

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

Re: leds-gpio on x86

[Vincent Pelletier]

On Tue, 11 Aug 2015 15:00:38 +0300, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> 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

Re: leds-gpio on x86

[Mika Westerberg]

On Tue, Aug 11, 2015 at 07:42:56PM +0200, Vincent Pelletier wrote:
> On Tue, 11 Aug 2015 15:00:38 +0300, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > 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.

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.

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

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.

Re: leds-gpio on x86

[Vincent Pelletier]

On Wed, 12 Aug 2015 15:53:14 +0300, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> Once your module gets loaded, it creates the "leds-gpio" platform device
> which in turn makes the leds-gpio driver to load.

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

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

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.

> > - 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.
> 
> Typically we get necessary information from ACPI or similar device
> description ;-)

Right :) .

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

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

Re: leds-gpio on x86

[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

Re: leds-gpio on x86

[Vincent Pelletier]

On Mon, 17 Aug 2015 23:08:23 +0200, Vincent Pelletier
<plr.vincent@gmail.com> wrote:
> How can I debug this further ?

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

Re: leds-gpio on x86

[Mika Westerberg]

On Tue, Aug 18, 2015 at 09:32:26AM +0200, Vincent Pelletier wrote:
> On Mon, 17 Aug 2015 23:08:23 +0200, Vincent Pelletier
> <plr.vincent@gmail.com> wrote:
> > How can I debug this further ?
> 
> 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.

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.

Re: leds-gpio on x86

[Vincent Pelletier]

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

On Tue, 18 Aug 2015 12:02:16 +0300, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> 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.

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

Re: leds-gpio on x86

[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