In preparation for splitting fujitsu-laptop into two separate modules, I
prepared two more cleanup series to minimize the amount of code being
moved around. This first series contains all patches that touch ACPI in
some way, which is why I am CCing linux-acpi as per Rafael's previous
advice.
This patch series was based on platform-driver-x86/for-next and tested
on a Lifebook S7020.
drivers/platform/x86/Kconfig | 10 --
drivers/platform/x86/fujitsu-laptop.c | 187 +++++++++-------------------------
2 files changed, 48 insertions(+), 149 deletions(-)
--
2.13.1
Using a dedicated Kconfig option for enabling debugging means the user
may be forced to recompile their kernel in order to gather debugging
information, which is inconvenient. Replace custom debugging
infrastructure with standard logging functions, taking advantage of
dynamic debug. Replace a pr_info() call inside an ACPI callback with an
acpi_handle_info() call.
The following mapping was used:
- FUJLAPTOP_DBG_ERROR -> acpi_handle_err()
- FUJLAPTOP_DBG_WARN -> acpi_handle_info() / dev_info()
- FUJLAPTOP_DBG_INFO -> acpi_handle_debug()
- FUJLAPTOP_DBG_TRACE -> acpi_handle_debug() / dev_dbg()
This means that some events which used to only be logged when the user
explicitly requested it will now be logged by default:
- ACPI method evaluation errors,
- unknown ACPI notification codes,
- unknown hotkey scancodes.
The first type of events should happen rarely, if ever at all. The rest
is interesting from driver development perspective as their presence in
the logs will mean the driver is unaware of certain events, handling of
which should be implemented.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/Kconfig | 10 -----
drivers/platform/x86/fujitsu-laptop.c | 78 +++++++++++++----------------------
2 files changed, 28 insertions(+), 60 deletions(-)
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 49a1d012f025..980419081ed7 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -195,16 +195,6 @@ config FUJITSU_LAPTOP
If you have a Fujitsu laptop, say Y or M here.
-config FUJITSU_LAPTOP_DEBUG
- bool "Verbose debug mode for Fujitsu Laptop Extras"
- depends on FUJITSU_LAPTOP
- default n
- ---help---
- Enables extra debug output from the fujitsu extras driver, at the
- expense of a slight increase in driver size.
-
- If you are not sure, say N here.
-
config FUJITSU_TABLET
tristate "Fujitsu Tablet Extras"
depends on ACPI
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 0861be36305d..d50872d96e63 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -110,22 +110,6 @@
#define MAX_HOTKEY_RINGBUFFER_SIZE 100
-/* Debugging */
-#define FUJLAPTOP_DBG_ERROR 0x0001
-#define FUJLAPTOP_DBG_WARN 0x0002
-#define FUJLAPTOP_DBG_INFO 0x0004
-#define FUJLAPTOP_DBG_TRACE 0x0008
-
-#ifdef CONFIG_FUJITSU_LAPTOP_DEBUG
-#define vdbg_printk(a_dbg_level, format, arg...) \
- do { if (dbg_level & a_dbg_level) \
- printk(KERN_DEBUG pr_fmt("%s: " format), __func__, ## arg); \
- } while (0)
-#else
-#define vdbg_printk(a_dbg_level, format, arg...) \
- do { } while (0)
-#endif
-
/* Device controlling the backlight and associated keys */
struct fujitsu_bl {
struct input_dev *input;
@@ -152,10 +136,6 @@ struct fujitsu_laptop {
static struct acpi_device *fext;
-#ifdef CONFIG_FUJITSU_LAPTOP_DEBUG
-static u32 dbg_level = 0x03;
-#endif
-
/* Fujitsu ACPI interface function */
static int call_fext_func(struct acpi_device *device,
@@ -174,12 +154,13 @@ static int call_fext_func(struct acpi_device *device,
status = acpi_evaluate_integer(device->handle, "FUNC", &arg_list,
&value);
if (ACPI_FAILURE(status)) {
- vdbg_printk(FUJLAPTOP_DBG_ERROR, "Failed to evaluate FUNC\n");
+ acpi_handle_err(device->handle, "Failed to evaluate FUNC\n");
return -ENODEV;
}
- vdbg_printk(FUJLAPTOP_DBG_TRACE, "FUNC 0x%x (args 0x%x, 0x%x, 0x%x) returned 0x%x\n",
- func, op, feature, state, (int)value);
+ acpi_handle_debug(device->handle,
+ "FUNC 0x%x (args 0x%x, 0x%x, 0x%x) returned 0x%x\n",
+ func, op, feature, state, (int)value);
return value;
}
@@ -206,16 +187,16 @@ static int set_lcd_level(struct acpi_device *device, int level)
break;
}
- vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via %s [%d]\n",
- method, level);
+ acpi_handle_debug(device->handle, "set lcd level via %s [%d]\n", method,
+ level);
if (level < 0 || level >= priv->max_brightness)
return -EINVAL;
status = acpi_execute_simple_method(device->handle, method, level);
if (ACPI_FAILURE(status)) {
- vdbg_printk(FUJLAPTOP_DBG_ERROR, "Failed to evaluate %s\n",
- method);
+ acpi_handle_err(device->handle, "Failed to evaluate %s\n",
+ method);
return -ENODEV;
}
@@ -230,7 +211,7 @@ static int get_lcd_level(struct acpi_device *device)
unsigned long long state = 0;
acpi_status status = AE_OK;
- vdbg_printk(FUJLAPTOP_DBG_TRACE, "get lcd level via GBLL\n");
+ acpi_handle_debug(device->handle, "get lcd level via GBLL\n");
status = acpi_evaluate_integer(device->handle, "GBLL", NULL, &state);
if (ACPI_FAILURE(status))
@@ -247,7 +228,7 @@ static int get_max_brightness(struct acpi_device *device)
unsigned long long state = 0;
acpi_status status = AE_OK;
- vdbg_printk(FUJLAPTOP_DBG_TRACE, "get max lcd level via RBLL\n");
+ acpi_handle_debug(device->handle, "get max lcd level via RBLL\n");
status = acpi_evaluate_integer(device->handle, "RBLL", NULL, &state);
if (ACPI_FAILURE(status))
@@ -440,8 +421,8 @@ static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
int oldb, newb;
if (event != ACPI_FUJITSU_NOTIFY_CODE1) {
- vdbg_printk(FUJLAPTOP_DBG_WARN,
- "unsupported event [0x%x]\n", event);
+ acpi_handle_info(device->handle, "unsupported event [0x%x]\n",
+ event);
sparse_keymap_report_event(priv->input, -1, 1, true);
return;
}
@@ -450,8 +431,8 @@ static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
get_lcd_level(device);
newb = priv->brightness_level;
- vdbg_printk(FUJLAPTOP_DBG_TRACE, "brightness button event [%i -> %i]\n",
- oldb, newb);
+ acpi_handle_debug(device->handle,
+ "brightness button event [%i -> %i]\n", oldb, newb);
if (oldb == newb)
return;
@@ -797,7 +778,8 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
while (call_fext_func(device, FUNC_BUTTONS, 0x1, 0x0, 0x0) != 0
&& (i++) < MAX_HOTKEY_RINGBUFFER_SIZE)
; /* No action, result is discarded */
- vdbg_printk(FUJLAPTOP_DBG_INFO, "Discarded %i ringbuffer entries\n", i);
+ acpi_handle_debug(device->handle, "Discarded %i ringbuffer entries\n",
+ i);
priv->flags_supported = call_fext_func(device, FUNC_FLAGS, 0x0, 0x0,
0x0);
@@ -812,8 +794,8 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
0x0);
/* Suspect this is a keymap of the application panel, print it */
- pr_info("BTNI: [0x%x]\n", call_fext_func(device,
- FUNC_BUTTONS, 0x0, 0x0, 0x0));
+ acpi_handle_info(device->handle, "BTNI: [0x%x]\n",
+ call_fext_func(device, FUNC_BUTTONS, 0x0, 0x0, 0x0));
/* Sync backlight power status */
if (fujitsu_bl && fujitsu_bl->bl_device &&
@@ -847,14 +829,14 @@ static void acpi_fujitsu_laptop_press(struct acpi_device *device, int scancode)
struct fujitsu_laptop *priv = acpi_driver_data(device);
if (priv->scancode_count == ARRAY_SIZE(priv->scancode_buf)) {
- vdbg_printk(FUJLAPTOP_DBG_WARN,
- "Could not push scancode [0x%x]\n", scancode);
+ dev_info(&priv->input->dev, "Could not push scancode [0x%x]\n",
+ scancode);
return;
}
priv->scancode_buf[priv->scancode_count++] = scancode;
sparse_keymap_report_event(priv->input, scancode, 1, false);
- vdbg_printk(FUJLAPTOP_DBG_TRACE,
- "Push scancode into buffer [0x%x]\n", scancode);
+ dev_dbg(&priv->input->dev, "Push scancode into buffer [0x%x]\n",
+ scancode);
}
static void acpi_fujitsu_laptop_release(struct acpi_device *device)
@@ -865,8 +847,8 @@ static void acpi_fujitsu_laptop_release(struct acpi_device *device)
while (priv->scancode_count > 0) {
scancode = priv->scancode_buf[--priv->scancode_count];
sparse_keymap_report_event(priv->input, scancode, 0, false);
- vdbg_printk(FUJLAPTOP_DBG_TRACE,
- "Pop scancode from buffer [0x%x]\n", scancode);
+ dev_dbg(&priv->input->dev, "Pop scancode from buffer [0x%x]\n",
+ scancode);
}
}
@@ -877,8 +859,8 @@ static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
unsigned int irb;
if (event != ACPI_FUJITSU_NOTIFY_CODE1) {
- vdbg_printk(FUJLAPTOP_DBG_WARN,
- "Unsupported event [0x%x]\n", event);
+ acpi_handle_info(device->handle, "Unsupported event [0x%x]\n",
+ event);
sparse_keymap_report_event(priv->input, -1, 1, true);
return;
}
@@ -896,8 +878,8 @@ static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
else if (scancode == 0)
acpi_fujitsu_laptop_release(device);
else
- vdbg_printk(FUJLAPTOP_DBG_WARN,
- "Unknown GIRB result [%x]\n", irb);
+ acpi_handle_info(device->handle,
+ "Unknown GIRB result [%x]\n", irb);
}
/* On some models (first seen on the Skylake-based Lifebook
@@ -999,10 +981,6 @@ module_param(use_alt_lcd_levels, int, 0644);
MODULE_PARM_DESC(use_alt_lcd_levels, "Interface used for setting LCD brightness level (-1 = auto, 0 = force SBLL, 1 = force SBL2)");
module_param(disable_brightness_adjust, bool, 0644);
MODULE_PARM_DESC(disable_brightness_adjust, "Disable LCD brightness adjustment");
-#ifdef CONFIG_FUJITSU_LAPTOP_DEBUG
-module_param_named(debug, dbg_level, uint, 0644);
-MODULE_PARM_DESC(debug, "Sets debug level bit-mask");
-#endif
MODULE_AUTHOR("Jonathan Woithe, Peter Gruber, Tony Vroon");
MODULE_DESCRIPTION("Fujitsu laptop extras support");
--
2.13.1
No formatting is needed when setting ACPI device name and class, so
switch to using strcpy() for this purpose.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index c64d5305ff37..06d04500dac0 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -411,8 +411,8 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
return -ENOMEM;
fujitsu_bl = priv;
- sprintf(acpi_device_name(device), "%s", ACPI_FUJITSU_BL_DEVICE_NAME);
- sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS);
+ strcpy(acpi_device_name(device), ACPI_FUJITSU_BL_DEVICE_NAME);
+ strcpy(acpi_device_class(device), ACPI_FUJITSU_CLASS);
device->driver_data = priv;
error = acpi_fujitsu_bl_input_setup(device);
@@ -800,9 +800,8 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
WARN_ONCE(fext, "More than one FUJ02E3 ACPI device was found. Driver may not work as intended.");
fext = device;
- sprintf(acpi_device_name(device), "%s",
- ACPI_FUJITSU_LAPTOP_DEVICE_NAME);
- sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS);
+ strcpy(acpi_device_name(device), ACPI_FUJITSU_LAPTOP_DEVICE_NAME);
+ strcpy(acpi_device_class(device), ACPI_FUJITSU_CLASS);
device->driver_data = priv;
error = acpi_fujitsu_laptop_input_setup(device);
--
2.13.1
acpi_ns_initialize_devices(), which is called during system-wide ACPI
initialization, already detects and calls all _INI methods belonging to
objects present in ACPI tables. There is no need to call these methods
again every time the module is loaded because they only initialize
status flags and hotkey-related variables; status flags are effectively
constants, hotkey-related variables may be assigned non-zero values
before acpi_fujitsu_laptop_add() is called, but that does not really
matter as we drain the scancodes queued in the firmware's ring buffer
before doing anything else.
Remove sections of code which invoke and check evaluation status of the
_INI methods belonging to the ACPI devices handled by the driver.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 16 ----------------
1 file changed, 16 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index b9f3ede4d567..0861be36305d 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -421,14 +421,6 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
pr_info("ACPI: %s [%s]\n",
acpi_device_name(device), acpi_device_bid(device));
- if (acpi_has_method(device->handle, METHOD_NAME__INI)) {
- vdbg_printk(FUJLAPTOP_DBG_INFO, "Invoking _INI\n");
- if (ACPI_FAILURE
- (acpi_evaluate_object
- (device->handle, METHOD_NAME__INI, NULL, NULL)))
- pr_err("_INI Method failed\n");
- }
-
if (get_max_brightness(device) <= 0)
priv->max_brightness = FUJITSU_LCD_N_LEVELS;
get_lcd_level(device);
@@ -801,14 +793,6 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
pr_info("ACPI: %s [%s]\n",
acpi_device_name(device), acpi_device_bid(device));
- if (acpi_has_method(device->handle, METHOD_NAME__INI)) {
- vdbg_printk(FUJLAPTOP_DBG_INFO, "Invoking _INI\n");
- if (ACPI_FAILURE
- (acpi_evaluate_object
- (device->handle, METHOD_NAME__INI, NULL, NULL)))
- pr_err("_INI Method failed\n");
- }
-
i = 0;
while (call_fext_func(device, FUNC_BUTTONS, 0x1, 0x0, 0x0) != 0
&& (i++) < MAX_HOTKEY_RINGBUFFER_SIZE)
--
2.13.1
Do not check whether the pointer passed to ACPI add callbacks is NULL as
it is earlier dereferenced anyway in the bus-level probe callback,
acpi_device_probe().
Do not check the value of acpi_disabled in fujitsu_init(), because it is
already done by acpi_bus_register_driver(), which is the first function
called by fujitsu_init().
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 54cb7ae541d4..c64d5305ff37 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -406,9 +406,6 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
return -ENODEV;
- if (!device)
- return -EINVAL;
-
priv = devm_kzalloc(&device->dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
return -ENOMEM;
@@ -796,9 +793,6 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
int error;
int i;
- if (!device)
- return -EINVAL;
-
priv = devm_kzalloc(&device->dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
return -ENOMEM;
@@ -993,9 +987,6 @@ static int __init fujitsu_init(void)
{
int ret;
- if (acpi_disabled)
- return -ENODEV;
-
ret = acpi_bus_register_driver(&acpi_fujitsu_bl_driver);
if (ret)
return ret;
--
2.13.1
Calling acpi_bus_update_power() for ACPI devices FUJ02B1 and FUJ02E3 is
pointless as they are not power manageable (neither _PS0 nor _PR0 is
defined for any of them), which causes their power state to be inherited
from their parent devices. Given the ACPI paths of these two devices
(\_SB.PCI0.LPCB.FJEX, \_SB.FEXT), their parent devices are also not
power manageable. These parent devices will thus have their power state
initialized to ACPI_STATE_D0, which in turn causes the power state for
both FUJ02B1 and FUJ02E3 to always be ACPI_STATE_D0 ("on").
Remove relevant acpi_bus_update_power() calls along with parts of debug
messages that they were supposed to have an effect on.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 24 ++++--------------------
1 file changed, 4 insertions(+), 20 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 5c0dc2126313..b9f3ede4d567 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -400,7 +400,6 @@ static int fujitsu_backlight_register(struct acpi_device *device)
static int acpi_fujitsu_bl_add(struct acpi_device *device)
{
struct fujitsu_bl *priv;
- int state = 0;
int error;
if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
@@ -419,15 +418,8 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
if (error)
return error;
- error = acpi_bus_update_power(device->handle, &state);
- if (error) {
- pr_err("Error reading power state\n");
- return error;
- }
-
- pr_info("ACPI: %s [%s] (%s)\n",
- acpi_device_name(device), acpi_device_bid(device),
- !device->power.state ? "on" : "off");
+ pr_info("ACPI: %s [%s]\n",
+ acpi_device_name(device), acpi_device_bid(device));
if (acpi_has_method(device->handle, METHOD_NAME__INI)) {
vdbg_printk(FUJLAPTOP_DBG_INFO, "Invoking _INI\n");
@@ -788,7 +780,6 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
static int acpi_fujitsu_laptop_add(struct acpi_device *device)
{
struct fujitsu_laptop *priv;
- int state = 0;
int error;
int i;
@@ -807,15 +798,8 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
if (error)
return error;
- error = acpi_bus_update_power(device->handle, &state);
- if (error) {
- pr_err("Error reading power state\n");
- return error;
- }
-
- pr_info("ACPI: %s [%s] (%s)\n",
- acpi_device_name(device), acpi_device_bid(device),
- !device->power.state ? "on" : "off");
+ pr_info("ACPI: %s [%s]\n",
+ acpi_device_name(device), acpi_device_bid(device));
if (acpi_has_method(device->handle, METHOD_NAME__INI)) {
vdbg_printk(FUJLAPTOP_DBG_INFO, "Invoking _INI\n");
--
2.13.1
In the case of brightness-related FUJ02B1 ACPI device, initializing the
input device associated with it identically as acpi-video initializes
its input device makes sense. However, using the same data for the
input device associated with the FUJ02E3 ACPI device makes little sense,
because the latter has nothing to do with video and assigning an
arbitrary product ID to it is redundant.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 06d04500dac0..5c0dc2126313 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -555,13 +555,12 @@ static int acpi_fujitsu_laptop_input_setup(struct acpi_device *device)
if (!priv->input)
return -ENOMEM;
- snprintf(priv->phys, sizeof(priv->phys), "%s/video/input0",
+ snprintf(priv->phys, sizeof(priv->phys), "%s/input0",
acpi_device_hid(device));
priv->input->name = acpi_device_name(device);
priv->input->phys = priv->phys;
priv->input->id.bustype = BUS_HOST;
- priv->input->id.product = 0x06;
dmi_check_system(fujitsu_laptop_dmi_table);
ret = sparse_keymap_setup(priv->input, keymap, NULL);
--
2.13.1
All ACPI device notify callbacks are invoked using acpi_os_execute(),
which causes the supplied callback to be queued to a static workqueue
which always executes on CPU 0. This means that there is no possibility
for any ACPI device notify callback to be concurrently executed on
multiple CPUs, which in the case of fujitsu-laptop means that using a
locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
of concurrent pushing and popping exists.
Instead of a kfifo, use a fixed-size integer table for storing pushed
scancodes and an associated variable holding the number of scancodes
currently stored in that table. Change debug messages so that they no
longer contain the term "ringbuffer".
In order to simplify implementation, hotkey input device behavior is
slightly changed (from FIFO to LIFO). The order of release events
generated by the input device should not matter from end user
perspective as upon releasing any hotkey the firmware only produces a
single event which means "all hotkeys were released".
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 54 +++++++++--------------------------
1 file changed, 14 insertions(+), 40 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 1c6fdd952c75..54cb7ae541d4 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -58,7 +58,6 @@
#include <linux/fb.h>
#include <linux/input.h>
#include <linux/input/sparse-keymap.h>
-#include <linux/kfifo.h>
#include <linux/leds.h>
#include <linux/platform_device.h>
#include <linux/slab.h>
@@ -110,7 +109,6 @@
#define KEY5_CODE 0x420
#define MAX_HOTKEY_RINGBUFFER_SIZE 100
-#define RINGBUFFERSIZE 40
/* Debugging */
#define FUJLAPTOP_DBG_ERROR 0x0001
@@ -146,8 +144,8 @@ struct fujitsu_laptop {
struct input_dev *input;
char phys[32];
struct platform_device *pf_device;
- struct kfifo fifo;
- spinlock_t fifo_lock;
+ int scancode_buf[40];
+ int scancode_count;
int flags_supported;
int flags_state;
};
@@ -813,23 +811,14 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS);
device->driver_data = priv;
- /* kfifo */
- spin_lock_init(&priv->fifo_lock);
- error = kfifo_alloc(&priv->fifo, RINGBUFFERSIZE * sizeof(int),
- GFP_KERNEL);
- if (error) {
- pr_err("kfifo_alloc failed\n");
- goto err_stop;
- }
-
error = acpi_fujitsu_laptop_input_setup(device);
if (error)
- goto err_free_fifo;
+ return error;
error = acpi_bus_update_power(device->handle, &state);
if (error) {
pr_err("Error reading power state\n");
- goto err_free_fifo;
+ return error;
}
pr_info("ACPI: %s [%s] (%s)\n",
@@ -877,62 +866,47 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
error = acpi_fujitsu_laptop_leds_register(device);
if (error)
- goto err_free_fifo;
+ return error;
error = fujitsu_laptop_platform_add(device);
if (error)
- goto err_free_fifo;
+ return error;
return 0;
-
-err_free_fifo:
- kfifo_free(&priv->fifo);
-err_stop:
- return error;
}
static int acpi_fujitsu_laptop_remove(struct acpi_device *device)
{
- struct fujitsu_laptop *priv = acpi_driver_data(device);
-
fujitsu_laptop_platform_remove(device);
- kfifo_free(&priv->fifo);
-
return 0;
}
static void acpi_fujitsu_laptop_press(struct acpi_device *device, int scancode)
{
struct fujitsu_laptop *priv = acpi_driver_data(device);
- int status;
- status = kfifo_in_locked(&priv->fifo, (unsigned char *)&scancode,
- sizeof(scancode), &priv->fifo_lock);
- if (status != sizeof(scancode)) {
+ if (priv->scancode_count == ARRAY_SIZE(priv->scancode_buf)) {
vdbg_printk(FUJLAPTOP_DBG_WARN,
"Could not push scancode [0x%x]\n", scancode);
return;
}
+ priv->scancode_buf[priv->scancode_count++] = scancode;
sparse_keymap_report_event(priv->input, scancode, 1, false);
vdbg_printk(FUJLAPTOP_DBG_TRACE,
- "Push scancode into ringbuffer [0x%x]\n", scancode);
+ "Push scancode into buffer [0x%x]\n", scancode);
}
static void acpi_fujitsu_laptop_release(struct acpi_device *device)
{
struct fujitsu_laptop *priv = acpi_driver_data(device);
- int scancode, status;
-
- while (true) {
- status = kfifo_out_locked(&priv->fifo,
- (unsigned char *)&scancode,
- sizeof(scancode), &priv->fifo_lock);
- if (status != sizeof(scancode))
- return;
+ int scancode;
+
+ while (priv->scancode_count > 0) {
+ scancode = priv->scancode_buf[--priv->scancode_count];
sparse_keymap_report_event(priv->input, scancode, 0, false);
vdbg_printk(FUJLAPTOP_DBG_TRACE,
- "Pop scancode from ringbuffer [0x%x]\n", scancode);
+ "Pop scancode from buffer [0x%x]\n", scancode);
}
}
--
2.13.1
Hi Michel
On Fri, Jun 16, 2017 at 06:40:51AM +0200, Micha?? K??pie?? wrote:
> In preparation for splitting fujitsu-laptop into two separate modules, I
> prepared two more cleanup series to minimize the amount of code being
> moved around. This first series contains all patches that touch ACPI in
> some way, which is why I am CCing linux-acpi as per Rafael's previous
> advice.
>
> This patch series was based on platform-driver-x86/for-next and tested
> on a Lifebook S7020.
As far as I can tell this series looks good. It is, as you said, another
step towards the splitting of this driver into two separate modules. I have
a few brief comments.
Patch 1:
- The argument for this change assumes that the execution methodology of
the acpi_os_execute() remains as is, since this is what guarantees the
absence of concurrency concerns. If it is fair to assume that we don't
have to worry about such things which may never happen then there's no
problem with this patch.
Patch 2:
- No problem.
Patch 3:
- No problem.
Patch 4:
- Presumedly the change in device path doesn't count as an API change.
Given that, I have no objection since the argument put forward makes
sense.
Patch 5:
- I am not overly familiar with the power management infrastructure within
the ACPI subsystem, but the presented argument seems sensible to me.
This patch is therefore fine if the given assumptions hold.
Patch 6:
- This seems fair.
Patch 7:
- The motivations which lead to the creation of the debug Kconfig option
are now moot. The elimination of this and the custom logging code is
therefore entirely justified.
To summarise, I see no major issues with this patch set so long as the minor
details noted above are of no consequence. Applying this will assist with
the end-goal of splitting the driver into two modules since it will minimise
the changes needed at the time the split is carried out.
Reviewed-by: Jonathan Woithe <[email protected]>
Regards
jonathan
On Fri, Jun 16, 2017 at 06:40:52AM +0200, Michał Kępień wrote:
> All ACPI device notify callbacks are invoked using acpi_os_execute(),
> which causes the supplied callback to be queued to a static workqueue
> which always executes on CPU 0. This means that there is no possibility
> for any ACPI device notify callback to be concurrently executed on
> multiple CPUs, which in the case of fujitsu-laptop means that using a
> locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
> only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
> of concurrent pushing and popping exists.
Was the kfifo causing a problem currently or for the migration to separate
modules? Is this purely a simplification?
Rafael, the above rationale appears sound to me. Do you have any concerns?
...
> -#define RINGBUFFERSIZE 40
>
> /* Debugging */
> #define FUJLAPTOP_DBG_ERROR 0x0001
> @@ -146,8 +144,8 @@ struct fujitsu_laptop {
> struct input_dev *input;
> char phys[32];
> struct platform_device *pf_device;
> - struct kfifo fifo;
> - spinlock_t fifo_lock;
> + int scancode_buf[40];
Do we know why 40 was used here? A single use magic number is fine, but it would
be good to document why it is what it is if we know.
--
Darren Hart
VMware Open Source Technology Center
On Fri, Jun 16, 2017 at 06:40:56AM +0200, Michał Kępień wrote:
> Calling acpi_bus_update_power() for ACPI devices FUJ02B1 and FUJ02E3 is
> pointless as they are not power manageable (neither _PS0 nor _PR0 is
> defined for any of them), which causes their power state to be inherited
> from their parent devices. Given the ACPI paths of these two devices
> (\_SB.PCI0.LPCB.FJEX, \_SB.FEXT), their parent devices are also not
> power manageable. These parent devices will thus have their power state
> initialized to ACPI_STATE_D0, which in turn causes the power state for
> both FUJ02B1 and FUJ02E3 to always be ACPI_STATE_D0 ("on").
>
How confident are we that all implementations of these two ACPI devices lack
_PS0 and _PR0 ?
--
Darren Hart
VMware Open Source Technology Center
On Wed, Jun 21, 2017 at 11:15:43AM -0700, Darren Hart wrote:
> On Fri, Jun 16, 2017 at 06:40:52AM +0200, Micha?? K??pie?? wrote:
> > -#define RINGBUFFERSIZE 40
> >
> > /* Debugging */
> > #define FUJLAPTOP_DBG_ERROR 0x0001
> > @@ -146,8 +144,8 @@ struct fujitsu_laptop {
> > struct input_dev *input;
> > char phys[32];
> > struct platform_device *pf_device;
> > - struct kfifo fifo;
> > - spinlock_t fifo_lock;
> > + int scancode_buf[40];
>
> Do we know why 40 was used here? A single use magic number is fine, but it
> would be good to document why it is what it is if we know.
I am unsure as to why 40 was chosen as the size. The support for the
hotkeys was contributed by Peter Gruber in 2008 who had a Fujitsu laptop
which featured this functionality (mine doesn't). I obviously can't speak
for Peter but I suspect that he simply picked a number which seemed
reasonable at the time. I am unaware of any specific reason why the size
was set at 40, and to be honest never throught to ask at the time the patch
was proposed.
If we need a reason to justify the value of 40, I guess the best option is
"because it seems reasonable".
I think the buffer size could probably be reduced a little without impacting
on functionality. I suspect the value was chosen so as to be well above the
number of events which could be generated ahead of a button release (which
effectively releases all buttons held at that stage). The number of hot
keys is limited (I don't think any model has more than 5), so reducing the
buffer somewhat appears safe (perhaps to 32 if there were any advantages to
having the size be a power of two). There seems little point doing this in
the name of memory saving since the amount saved is trivially small.
Looking through my correspondance with Peter from 2008 (which was largely
off-list for reasons I do not recall), it seems the reason the kfifo was
used was due to Peter's concern about concurrent processing of mulitple
events across different CPUs. Since at the time we couldn't determine
whether this was a real issue Peter took the safe route and used a kfifo.
If the assumption that it cannot happen is known to hold in all cases there
is clearly no need for the more complex construct (at least on the basis of
concurrency arguments).
I've given some more thought to the following point (from the original patch
submission):
> In order to simplify implementation, hotkey input device behavior is
> slightly changed (from FIFO to LIFO). The order of release events
> generated by the input device should not matter from end user
> perspective as upon releasing any hotkey the firmware only produces a
> single event which means "all hotkeys were released".
This will effectively alter the order the button events are seen by
userspace though, won't it? It would mean that (for example) a user who
presses (and holds) the "media" key before "email" will end up with "email"
being acted on first. This may surprise them. Then again, I suppose it
could be argued that such usage is unusual and therefore is likely to be
rare - and therefore not worth bothering about.
Regards
jonathan
On Thu, Jun 22, 2017 at 09:20:21AM +0930, Jonathan Woithe wrote:
> On Wed, Jun 21, 2017 at 11:15:43AM -0700, Darren Hart wrote:
> > On Fri, Jun 16, 2017 at 06:40:52AM +0200, Micha?? K??pie?? wrote:
> > > -#define RINGBUFFERSIZE 40
> > >
> > > /* Debugging */
> > > #define FUJLAPTOP_DBG_ERROR 0x0001
> > > @@ -146,8 +144,8 @@ struct fujitsu_laptop {
> > > struct input_dev *input;
> > > char phys[32];
> > > struct platform_device *pf_device;
> > > - struct kfifo fifo;
> > > - spinlock_t fifo_lock;
> > > + int scancode_buf[40];
> >
> > Do we know why 40 was used here? A single use magic number is fine, but it
> > would be good to document why it is what it is if we know.
>
> I am unsure as to why 40 was chosen as the size. The support for the
> hotkeys was contributed by Peter Gruber in 2008 who had a Fujitsu laptop
> which featured this functionality (mine doesn't). I obviously can't speak
> for Peter but I suspect that he simply picked a number which seemed
> reasonable at the time. I am unaware of any specific reason why the size
> was set at 40, and to be honest never throught to ask at the time the patch
> was proposed.
>
> If we need a reason to justify the value of 40, I guess the best option is
> "because it seems reasonable".
That's not surprising, and it's fine. If we know, it's worth documenting. If
not, well, we do what we can :-)
>
> I think the buffer size could probably be reduced a little without impacting
> on functionality. I suspect the value was chosen so as to be well above the
> number of events which could be generated ahead of a button release (which
> effectively releases all buttons held at that stage). The number of hot
> keys is limited (I don't think any model has more than 5), so reducing the
> buffer somewhat appears safe (perhaps to 32 if there were any advantages to
> having the size be a power of two). There seems little point doing this in
> the name of memory saving since the amount saved is trivially small.
>
It's not a problem as far as I'm concerned. Plenty more lower hanging fruit if
people want to reduce memory footprint.
> Looking through my correspondance with Peter from 2008 (which was largely
> off-list for reasons I do not recall), it seems the reason the kfifo was
> used was due to Peter's concern about concurrent processing of mulitple
> events across different CPUs. Since at the time we couldn't determine
> whether this was a real issue Peter took the safe route and used a kfifo.
> If the assumption that it cannot happen is known to hold in all cases there
> is clearly no need for the more complex construct (at least on the basis of
> concurrency arguments).
>
> I've given some more thought to the following point (from the original patch
> submission):
> > In order to simplify implementation, hotkey input device behavior is
> > slightly changed (from FIFO to LIFO). The order of release events
> > generated by the input device should not matter from end user
> > perspective as upon releasing any hotkey the firmware only produces a
> > single event which means "all hotkeys were released".
>
> This will effectively alter the order the button events are seen by
> userspace though, won't it? It would mean that (for example) a user who
> presses (and holds) the "media" key before "email" will end up with "email"
> being acted on first. This may surprise them. Then again, I suppose it
> could be argued that such usage is unusual and therefore is likely to be
> rare - and therefore not worth bothering about.
Michal noted there is only one event to release all keys so the order wouldn't
be noticed in userspace. Has this been confirmed with testing?
--
Darren Hart
VMware Open Source Technology Center
Hi Darren
On Wed, Jun 21, 2017 at 07:44:13PM -0700, Darren Hart wrote:
> > I think the buffer size could probably be reduced a little without impacting
> > on functionality. I suspect the value was chosen so as to be well above the
> > number of events which could be generated ahead of a button release (which
> > effectively releases all buttons held at that stage). The number of hot
> > keys is limited (I don't think any model has more than 5), so reducing the
> > buffer somewhat appears safe (perhaps to 32 if there were any advantages to
> > having the size be a power of two). There seems little point doing this in
> > the name of memory saving since the amount saved is trivially small.
>
> It's not a problem as far as I'm concerned. Plenty more lower hanging
> fruit if people want to reduce memory footprint.
Indeed. I'm happy to leave it at 40.
> > I've given some more thought to the following point (from the original patch
> > submission):
> > > In order to simplify implementation, hotkey input device behavior is
> > > slightly changed (from FIFO to LIFO). The order of release events
> > > generated by the input device should not matter from end user
> > > perspective as upon releasing any hotkey the firmware only produces a
> > > single event which means "all hotkeys were released".
> >
> > This will effectively alter the order the button events are seen by
> > userspace though, won't it? It would mean that (for example) a user who
> > presses (and holds) the "media" key before "email" will end up with "email"
> > being acted on first. This may surprise them. Then again, I suppose it
> > could be argued that such usage is unusual and therefore is likely to be
> > rare - and therefore not worth bothering about.
>
> Michal noted there is only one event to release all keys so the order wouldn't
> be noticed in userspace. Has this been confirmed with testing?
I can't test this because my Fujitsu doesn't have these hotkeys.
Regarding the order, the original code sent "press" events to userspace in
the order that the keys were pressed. When the single "release all keys"
event is received, a "release" event ws sent to userspace for each button
which was pressed, in the order they were pressed.
If userspace acted on the button release event then the order of the
synthesised "release" events could be significant to userspace, even if they
are all generated at the same time in response to the first "release" event.
By way of example, let's say there are two hotkeys, "A" and "B". Let us also
say that the user does the following:
- Press and hold hotkey "A"
- Press and hold hotkey "B"
- Release one of these hotkeys
The events seen by userspace with the original code would be "A-press",
"B-press", "A-release", "B-release". With the revised code the order of the
release events would be reversed compared to the previous behaviour. That
is, unless I've misunderstood how sparse_keymap_report_event() works.
Regards
jonathan
> On Fri, Jun 16, 2017 at 06:40:52AM +0200, Michał Kępień wrote:
> > All ACPI device notify callbacks are invoked using acpi_os_execute(),
> > which causes the supplied callback to be queued to a static workqueue
> > which always executes on CPU 0. This means that there is no possibility
> > for any ACPI device notify callback to be concurrently executed on
> > multiple CPUs, which in the case of fujitsu-laptop means that using a
> > locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
> > only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
> > of concurrent pushing and popping exists.
>
> Was the kfifo causing a problem currently or for the migration to separate
> modules? Is this purely a simplification?
It is just another step in stripping fujitsu-laptop down to its bare
essentials. If my reasoning quoted above is correct, using a locked
kfifo needlessly suggests potential concurrency issues, but it is
definitely not an error.
--
Best regards,
Michał Kępień
> Hi Darren
>
> On Wed, Jun 21, 2017 at 07:44:13PM -0700, Darren Hart wrote:
> > > I think the buffer size could probably be reduced a little without impacting
> > > on functionality. I suspect the value was chosen so as to be well above the
> > > number of events which could be generated ahead of a button release (which
> > > effectively releases all buttons held at that stage). The number of hot
> > > keys is limited (I don't think any model has more than 5), so reducing the
> > > buffer somewhat appears safe (perhaps to 32 if there were any advantages to
> > > having the size be a power of two). There seems little point doing this in
> > > the name of memory saving since the amount saved is trivially small.
> >
> > It's not a problem as far as I'm concerned. Plenty more lower hanging
> > fruit if people want to reduce memory footprint.
>
> Indeed. I'm happy to leave it at 40.
>
> > > I've given some more thought to the following point (from the original patch
> > > submission):
> > > > In order to simplify implementation, hotkey input device behavior is
> > > > slightly changed (from FIFO to LIFO). The order of release events
> > > > generated by the input device should not matter from end user
> > > > perspective as upon releasing any hotkey the firmware only produces a
> > > > single event which means "all hotkeys were released".
> > >
> > > This will effectively alter the order the button events are seen by
> > > userspace though, won't it? It would mean that (for example) a user who
> > > presses (and holds) the "media" key before "email" will end up with "email"
> > > being acted on first. This may surprise them. Then again, I suppose it
> > > could be argued that such usage is unusual and therefore is likely to be
> > > rare - and therefore not worth bothering about.
> >
> > Michal noted there is only one event to release all keys so the order wouldn't
> > be noticed in userspace. Has this been confirmed with testing?
I may have worded this in a confusing way, sorry for that. What I
wanted to convey is that the driver currently produces release events in
FIFO order, but that order is not imposed by the firmware.
>
> I can't test this because my Fujitsu doesn't have these hotkeys.
>
> Regarding the order, the original code sent "press" events to userspace in
> the order that the keys were pressed. When the single "release all keys"
> event is received, a "release" event ws sent to userspace for each button
> which was pressed, in the order they were pressed.
>
> If userspace acted on the button release event then the order of the
> synthesised "release" events could be significant to userspace, even if they
> are all generated at the same time in response to the first "release" event.
>
> By way of example, let's say there are two hotkeys, "A" and "B". Let us also
> say that the user does the following:
> - Press and hold hotkey "A"
> - Press and hold hotkey "B"
> - Release one of these hotkeys
>
> The events seen by userspace with the original code would be "A-press",
> "B-press", "A-release", "B-release". With the revised code the order of the
> release events would be reversed compared to the previous behaviour. That
> is, unless I've misunderstood how sparse_keymap_report_event() works.
All you wrote above is correct and this patch does change the order of
release events sent to userspace when multiple hotkeys are pressed
simultaneously. The question is: is it relevant for any practical
scenario?
Anyway, I find this matter to be of secondary importance. The primary
objective of this patch is to get rid of the kfifo. If anyone has
strong feelings about the change in event ordering, I will be happy to
revert to FIFO in v2.
--
Best regards,
Michał Kępień
> On Fri, Jun 16, 2017 at 06:40:56AM +0200, Michał Kępień wrote:
> > Calling acpi_bus_update_power() for ACPI devices FUJ02B1 and FUJ02E3 is
> > pointless as they are not power manageable (neither _PS0 nor _PR0 is
> > defined for any of them), which causes their power state to be inherited
> > from their parent devices. Given the ACPI paths of these two devices
> > (\_SB.PCI0.LPCB.FJEX, \_SB.FEXT), their parent devices are also not
> > power manageable. These parent devices will thus have their power state
> > initialized to ACPI_STATE_D0, which in turn causes the power state for
> > both FUJ02B1 and FUJ02E3 to always be ACPI_STATE_D0 ("on").
> >
>
> How confident are we that all implementations of these two ACPI devices lack
> _PS0 and _PR0 ?
I looked at DSDT dumps of four different Fujitsu laptops released in the
past ten years or so for which at least one of these two ACPI devices is
present and found no traces of either of these methods being defined for
them. I do not think we have a way of ensuring that the above holds
true for every other model out there, but I will point out that
fujitsu-laptop is the only user of acpi_bus_update_power() outside of
drivers/acpi.
--
Best regards,
Michał Kępień
On Fri, Jun 16, 2017 at 06:40:51AM +0200, Michał Kępień wrote:
> In preparation for splitting fujitsu-laptop into two separate modules, I
> prepared two more cleanup series to minimize the amount of code being
> moved around. This first series contains all patches that touch ACPI in
> some way, which is why I am CCing linux-acpi as per Rafael's previous
> advice.
>
> This patch series was based on platform-driver-x86/for-next and tested
> on a Lifebook S7020.
I'm satisfied with the responses to all questions on this series. Queued to
testing. Thanks Michal and Jonathan.
--
Darren Hart
VMware Open Source Technology Center
On Thu, Jun 22, 2017 at 10:46:19PM +0200, Michał Kępień wrote:
> > The events seen by userspace with the original code would be "A-press",
> > "B-press", "A-release", "B-release". With the revised code the order of the
> > release events would be reversed compared to the previous behaviour. That
> > is, unless I've misunderstood how sparse_keymap_report_event() works.
>
> All you wrote above is correct and this patch does change the order of
> release events sent to userspace when multiple hotkeys are pressed
> simultaneously. The question is: is it relevant for any practical
> scenario?
>
> Anyway, I find this matter to be of secondary importance. The primary
> objective of this patch is to get rid of the kfifo. If anyone has
> strong feelings about the change in event ordering, I will be happy to
> revert to FIFO in v2.
This all looks reasonable to me, I don't see anything requiring a respin.
--
Darren Hart
VMware Open Source Technology Center
On Thu, Jun 22, 2017 at 11:02:35PM +0200, Michał Kępień wrote:
> > On Fri, Jun 16, 2017 at 06:40:56AM +0200, Michał Kępień wrote:
> > > Calling acpi_bus_update_power() for ACPI devices FUJ02B1 and FUJ02E3 is
> > > pointless as they are not power manageable (neither _PS0 nor _PR0 is
> > > defined for any of them), which causes their power state to be inherited
> > > from their parent devices. Given the ACPI paths of these two devices
> > > (\_SB.PCI0.LPCB.FJEX, \_SB.FEXT), their parent devices are also not
> > > power manageable. These parent devices will thus have their power state
> > > initialized to ACPI_STATE_D0, which in turn causes the power state for
> > > both FUJ02B1 and FUJ02E3 to always be ACPI_STATE_D0 ("on").
> > >
> >
> > How confident are we that all implementations of these two ACPI devices lack
> > _PS0 and _PR0 ?
>
> I looked at DSDT dumps of four different Fujitsu laptops released in the
> past ten years or so for which at least one of these two ACPI devices is
> present and found no traces of either of these methods being defined for
> them. I do not think we have a way of ensuring that the above holds
> true for every other model out there, but I will point out that
> fujitsu-laptop is the only user of acpi_bus_update_power() outside of
> drivers/acpi.
OK, thanks. Queueing to testing.
--
Darren Hart
VMware Open Source Technology Center
On Thu, Jun 22, 2017 at 04:58:09PM -0700, Darren Hart wrote:
> On Thu, Jun 22, 2017 at 10:46:19PM +0200, Micha?? K??pie?? wrote:
> > > The events seen by userspace with the original code would be "A-press",
> > > "B-press", "A-release", "B-release". With the revised code the order of the
> > > release events would be reversed compared to the previous behaviour. That
> > > is, unless I've misunderstood how sparse_keymap_report_event() works.
> >
> > All you wrote above is correct and this patch does change the order of
> > release events sent to userspace when multiple hotkeys are pressed
> > simultaneously. The question is: is it relevant for any practical
> > scenario?
> >
> > Anyway, I find this matter to be of secondary importance. The primary
> > objective of this patch is to get rid of the kfifo. If anyone has
> > strong feelings about the change in event ordering, I will be happy to
> > revert to FIFO in v2.
>
> This all looks reasonable to me, I don't see anything requiring a respin.
I agree it is of seconary importance. To me, using LIFO release order is
counter-intuitive, but it's the sort of question that if put to 100 people
you'll get a 50/50 split of opinions.
Especially since the whole "multiple buttons held at once" scenario is
rather unusual we can go with switching the order if others don't see a
problem with the behavioural change.
Regards
jonathan
On Thu, Jun 22, 2017 at 04:58:43PM -0700, Darren Hart wrote:
> On Thu, Jun 22, 2017 at 11:02:35PM +0200, Micha?? K??pie?? wrote:
> > > On Fri, Jun 16, 2017 at 06:40:56AM +0200, Micha?? K??pie?? wrote:
> > > > Calling acpi_bus_update_power() for ACPI devices FUJ02B1 and FUJ02E3 is
> > > > pointless as they are not power manageable (neither _PS0 nor _PR0 is
> > > > defined for any of them), which causes their power state to be inherited
> > > > from their parent devices. Given the ACPI paths of these two devices
> > > > (\_SB.PCI0.LPCB.FJEX, \_SB.FEXT), their parent devices are also not
> > > > power manageable. These parent devices will thus have their power state
> > > > initialized to ACPI_STATE_D0, which in turn causes the power state for
> > > > both FUJ02B1 and FUJ02E3 to always be ACPI_STATE_D0 ("on").
> > > >
> > >
> > > How confident are we that all implementations of these two ACPI devices lack
> > > _PS0 and _PR0 ?
> >
> > I looked at DSDT dumps of four different Fujitsu laptops released in the
> > past ten years or so for which at least one of these two ACPI devices is
> > present and found no traces of either of these methods being defined for
> > them. I do not think we have a way of ensuring that the above holds
> > true for every other model out there, but I will point out that
> > fujitsu-laptop is the only user of acpi_bus_update_power() outside of
> > drivers/acpi.
>
> OK, thanks. Queueing to testing.
Thanks. In case it was missed, I supplied my reviewed-by message and
sign-off in an earlier post.
Regards
jonathan
On Fri, Jun 23, 2017 at 09:46:59AM +0930, Jonathan Woithe wrote:
> Thanks. In case it was missed, I supplied my reviewed-by message and
> sign-off in an earlier post.
Yup, got it - thanks!
--
Darren Hart
VMware Open Source Technology Center
On Fri, Jun 23, 2017 at 09:44:58AM +0930, Jonathan Woithe wrote:
> On Thu, Jun 22, 2017 at 04:58:09PM -0700, Darren Hart wrote:
> > On Thu, Jun 22, 2017 at 10:46:19PM +0200, Micha?? K??pie?? wrote:
> > > > The events seen by userspace with the original code would be "A-press",
> > > > "B-press", "A-release", "B-release". With the revised code the order of the
> > > > release events would be reversed compared to the previous behaviour. That
> > > > is, unless I've misunderstood how sparse_keymap_report_event() works.
> > >
> > > All you wrote above is correct and this patch does change the order of
> > > release events sent to userspace when multiple hotkeys are pressed
> > > simultaneously. The question is: is it relevant for any practical
> > > scenario?
> > >
> > > Anyway, I find this matter to be of secondary importance. The primary
> > > objective of this patch is to get rid of the kfifo. If anyone has
> > > strong feelings about the change in event ordering, I will be happy to
> > > revert to FIFO in v2.
> >
> > This all looks reasonable to me, I don't see anything requiring a respin.
>
> I agree it is of seconary importance. To me, using LIFO release order is
> counter-intuitive, but it's the sort of question that if put to 100 people
> you'll get a 50/50 split of opinions.
>
> Especially since the whole "multiple buttons held at once" scenario is
> rather unusual we can go with switching the order if others don't see a
> problem with the behavioural change.
Yes. If anyone notices the implementation difference, I'll be rather surprised.
If they do, we can convert back to FIFO as you say.
--
Darren Hart
VMware Open Source Technology Center
On Wednesday, June 21, 2017 11:15:43 AM Darren Hart wrote:
> On Fri, Jun 16, 2017 at 06:40:52AM +0200, Michał Kępień wrote:
> > All ACPI device notify callbacks are invoked using acpi_os_execute(),
> > which causes the supplied callback to be queued to a static workqueue
> > which always executes on CPU 0. This means that there is no possibility
> > for any ACPI device notify callback to be concurrently executed on
> > multiple CPUs, which in the case of fujitsu-laptop means that using a
> > locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
> > only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
> > of concurrent pushing and popping exists.
>
> Was the kfifo causing a problem currently or for the migration to separate
> modules? Is this purely a simplification?
>
> Rafael, the above rationale appears sound to me. Do you have any concerns?
I actually do.
While this is the case today, making the driver code depend on it in a hard way
sort of makes it difficult to change in the future if need be.
Thanks,
Rafael
On Sat, Jun 24, 2017 at 02:25:46AM +0200, Rafael Wysocki wrote:
> On Wednesday, June 21, 2017 11:15:43 AM Darren Hart wrote:
> > On Fri, Jun 16, 2017 at 06:40:52AM +0200, Michał Kępień wrote:
> > > All ACPI device notify callbacks are invoked using acpi_os_execute(),
> > > which causes the supplied callback to be queued to a static workqueue
> > > which always executes on CPU 0. This means that there is no possibility
> > > for any ACPI device notify callback to be concurrently executed on
> > > multiple CPUs, which in the case of fujitsu-laptop means that using a
> > > locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
> > > only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
> > > of concurrent pushing and popping exists.
> >
> > Was the kfifo causing a problem currently or for the migration to separate
> > modules? Is this purely a simplification?
> >
> > Rafael, the above rationale appears sound to me. Do you have any concerns?
>
> I actually do.
>
> While this is the case today, making the driver code depend on it in a hard way
> sort of makes it difficult to change in the future if need be.
OK, if we aren't guaranteed for this to run on CPU 0 in the future, and this
will be annoying to debug if it does changes, let's skip the kfifo change.
I have removed this patch, and fixed up the merge conflicts of the remaining 6
patches here:
http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/fujitsu
Michal / Jonathan, would you please review and let me know if this is what you
would have done / approve the rebase?
Thanks,
--
Darren Hart
VMware Open Source Technology Center
On Mon, Jun 26, 2017 at 05:07:18PM -0700, Darren Hart wrote:
> On Sat, Jun 24, 2017 at 02:25:46AM +0200, Rafael Wysocki wrote:
> > > Rafael, the above rationale appears sound to me. Do you have any concerns?
> >
> > I actually do.
> >
> > While this is the case today, making the driver code depend on it in a hard way
> > sort of makes it difficult to change in the future if need be.
>
> OK, if we aren't guaranteed for this to run on CPU 0 in the future, and
> this will be annoying to debug if it does changes, let's skip the kfifo
> change.
>
> I have removed this patch, and fixed up the merge conflicts of the
> remaining 6 patches here:
>
> http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/fujitsu
>
> Michal / Jonathan, would you please review and let me know if this is what
> you would have done / approve the rebase?
The rebase looks reasonable to me.
Regards
jonathan
> On Sat, Jun 24, 2017 at 02:25:46AM +0200, Rafael Wysocki wrote:
> > On Wednesday, June 21, 2017 11:15:43 AM Darren Hart wrote:
> > > On Fri, Jun 16, 2017 at 06:40:52AM +0200, Michał Kępień wrote:
> > > > All ACPI device notify callbacks are invoked using acpi_os_execute(),
> > > > which causes the supplied callback to be queued to a static workqueue
> > > > which always executes on CPU 0. This means that there is no possibility
> > > > for any ACPI device notify callback to be concurrently executed on
> > > > multiple CPUs, which in the case of fujitsu-laptop means that using a
> > > > locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
> > > > only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
> > > > of concurrent pushing and popping exists.
> > >
> > > Was the kfifo causing a problem currently or for the migration to separate
> > > modules? Is this purely a simplification?
> > >
> > > Rafael, the above rationale appears sound to me. Do you have any concerns?
> >
> > I actually do.
> >
> > While this is the case today, making the driver code depend on it in a hard way
> > sort of makes it difficult to change in the future if need be.
>
> OK, if we aren't guaranteed for this to run on CPU 0 in the future, and this
> will be annoying to debug if it does changes, let's skip the kfifo change.
>
> I have removed this patch, and fixed up the merge conflicts of the remaining 6
> patches here:
>
> http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/fujitsu
>
> Michal / Jonathan, would you please review and let me know if this is what you
> would have done / approve the rebase?
The only issue I can see is that the push/pop debug messages in the last
patch contain the word "buffer" instead of the original "ringbuffer".
The dropped kfifo patch changed the wording from "ringbuffer" to
"buffer" as applying it means there is no ringbuffer any more, but since
it was not applied in the end, I guess the original wording should stay
in place.
The rest looks good to me.
--
Best regards,
Michał Kępień
On Wed, Jun 28, 2017 at 06:30:44AM +0200, Michał Kępień wrote:
> > On Sat, Jun 24, 2017 at 02:25:46AM +0200, Rafael Wysocki wrote:
> > > On Wednesday, June 21, 2017 11:15:43 AM Darren Hart wrote:
> > > > On Fri, Jun 16, 2017 at 06:40:52AM +0200, Michał Kępień wrote:
> > > > > All ACPI device notify callbacks are invoked using acpi_os_execute(),
> > > > > which causes the supplied callback to be queued to a static workqueue
> > > > > which always executes on CPU 0. This means that there is no possibility
> > > > > for any ACPI device notify callback to be concurrently executed on
> > > > > multiple CPUs, which in the case of fujitsu-laptop means that using a
> > > > > locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
> > > > > only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
> > > > > of concurrent pushing and popping exists.
> > > >
> > > > Was the kfifo causing a problem currently or for the migration to separate
> > > > modules? Is this purely a simplification?
> > > >
> > > > Rafael, the above rationale appears sound to me. Do you have any concerns?
> > >
> > > I actually do.
> > >
> > > While this is the case today, making the driver code depend on it in a hard way
> > > sort of makes it difficult to change in the future if need be.
> >
> > OK, if we aren't guaranteed for this to run on CPU 0 in the future, and this
> > will be annoying to debug if it does changes, let's skip the kfifo change.
> >
> > I have removed this patch, and fixed up the merge conflicts of the remaining 6
> > patches here:
> >
> > http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/fujitsu
> >
> > Michal / Jonathan, would you please review and let me know if this is what you
> > would have done / approve the rebase?
>
> The only issue I can see is that the push/pop debug messages in the last
> patch contain the word "buffer" instead of the original "ringbuffer".
> The dropped kfifo patch changed the wording from "ringbuffer" to
> "buffer" as applying it means there is no ringbuffer any more, but since
> it was not applied in the end, I guess the original wording should stay
> in place.
>
> The rest looks good to me.
Good catch, thank you. The testing branch has been rebased to reflect these
changes.
--
Darren Hart
VMware Open Source Technology Center