[PATCH 1/1] platform/x86: revert pcengines-apuv2 wire up simswitch gpio as led

[PATCH 1/1] platform/x86: revert pcengines-apuv2 wire up simswitch gpio as led

[Florian Eckert]

This reverts commit 5037d4ddda31c2dbbb018109655f61054b1756dc.

Explanation why this does not work:
This change connects the simswap to the LED subsystem of the kernel.
From my point of view, it's nonsense. If we do it this way, then this
can be switched relatively easily via the LED subsystem (trigger:
none/default-on) and that is dangerous! If this is used, it would be
unfavorable, since there is also another trigger (trigger:
heartbeat/netdev).

Therefore, this simswap GPIO should remain in the GPIO
subsystem and be switched via it and not be connected to the LED
subsystem. To avoid the problems mentioned above. The LED subsystem is
not made for this and it is not a good compromise, but rather dangerous.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 drivers/platform/x86/pcengines-apuv2.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/platform/x86/pcengines-apuv2.c b/drivers/platform/x86/pcengines-apuv2.c
index 9b11ef1a401f..6aff6cf41414 100644
--- a/drivers/platform/x86/pcengines-apuv2.c
+++ b/drivers/platform/x86/pcengines-apuv2.c
@@ -78,7 +78,6 @@ static const struct gpio_led apu2_leds[] = {
 	{ .name = "apu:green:1" },
 	{ .name = "apu:green:2" },
 	{ .name = "apu:green:3" },
-	{ .name = "apu:simswap" },
 };
 
 static const struct gpio_led_platform_data apu2_leds_pdata = {
@@ -95,8 +94,6 @@ static struct gpiod_lookup_table gpios_led_table = {
 				NULL, 1, GPIO_ACTIVE_LOW),
 		GPIO_LOOKUP_IDX(AMD_FCH_GPIO_DRIVER_NAME, APU2_GPIO_LINE_LED3,
 				NULL, 2, GPIO_ACTIVE_LOW),
-		GPIO_LOOKUP_IDX(AMD_FCH_GPIO_DRIVER_NAME, APU2_GPIO_LINE_SIMSWAP,
-				NULL, 3, GPIO_ACTIVE_LOW),
 	}
 };
 
-- 
2.20.1

Re: [PATCH 1/1] platform/x86: revert pcengines-apuv2 wire up simswitch gpio as led

[Enrico Weigelt, metux IT consult]

On 13.07.20 10:50, Florian Eckert wrote:

Hello Florian,

> This reverts commit 5037d4ddda31c2dbbb018109655f61054b1756dc.

no, please dont.

> This change connects the simswap to the LED subsystem of the kernel.
> From my point of view, it's nonsense. If we do it this way, then this
> can be switched relatively easily via the LED subsystem (trigger:
> none/default-on) and that is dangerous! If this is used, it would be
> unfavorable, since there is also another trigger (trigger:
> heartbeat/netdev).

I don't think that potential silly abuse is a good argument. It if
would, we should also disallow things like "echo FOO > /dev/sda" :p

The reason for it wire'ing up was having an simple and easy to use
interface. Raw gpios do NOT meet this criteria: complicated to use and
not stable addressing (from userland PoV) - would require an extra
userland program just for that single specific task.

Yes, LED is not the optimal approach, same for other gpio-connected
switches, eg. relais or various multiplexers. But as long as we don't
have a really fitting subsystem, it's IMHO the best compromise we have
so far.

Actually, I've already been hacking on a better subsystem, which models
switchable inter-device connections. It's called portmux. But it's not
usable yet. Lets talk about this instead of just wildly dropping
existing functionality, that's used in the field.


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 1/1] platform/x86: revert pcengines-apuv2 wire up simswitch gpio as led

[Enrico Weigelt, metux IT consult]

On 31.07.20 15:45, Enrico Weigelt, metux IT consult wrote:

Addendum: there's even more functionality we'll loose

Individual LED lines can be made available to specific
unprivileged users / processes, eg. chmod or passing fd's.

For example, allowing a web application to pull the switch,
w/o ever becoming root. And that's indeed a practial use case,
which is used in the field.

--mtx


> On 13.07.20 10:50, Florian Eckert wrote:
> 
> Hello Florian,
> 
>> This reverts commit 5037d4ddda31c2dbbb018109655f61054b1756dc.
> 
> no, please dont.
> 
>> This change connects the simswap to the LED subsystem of the kernel.
>> From my point of view, it's nonsense. If we do it this way, then this
>> can be switched relatively easily via the LED subsystem (trigger:
>> none/default-on) and that is dangerous! If this is used, it would be
>> unfavorable, since there is also another trigger (trigger:
>> heartbeat/netdev).
> 
> I don't think that potential silly abuse is a good argument. It if
> would, we should also disallow things like "echo FOO > /dev/sda" :p
> 
> The reason for it wire'ing up was having an simple and easy to use
> interface. Raw gpios do NOT meet this criteria: complicated to use and
> not stable addressing (from userland PoV) - would require an extra
> userland program just for that single specific task.
> 
> Yes, LED is not the optimal approach, same for other gpio-connected
> switches, eg. relais or various multiplexers. But as long as we don't
> have a really fitting subsystem, it's IMHO the best compromise we have
> so far.
> 
> Actually, I've already been hacking on a better subsystem, which models
> switchable inter-device connections. It's called portmux. But it's not
> usable yet. Lets talk about this instead of just wildly dropping
> existing functionality, that's used in the field.
> 
> 
> --mtx
> 

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287