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

Message ID 20200713085010.26394-1-fe@dev.tdt.de
State In Next
Commit f560cd502190a9fd0ca8db0a15c5cca7d9091d2c
Headers show
Series
  • [1/1] platform/x86: revert pcengines-apuv2 wire up simswitch gpio as led
Related show

Commit Message

Florian Eckert July 13, 2020, 8:50 a.m. UTC
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(-)

Comments

Enrico Weigelt, metux IT consult July 31, 2020, 1:45 p.m. UTC | #1
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
Enrico Weigelt, metux IT consult July 31, 2020, 2:01 p.m. UTC | #2
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
>

Patch
diff mbox series

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),
 	}
 };