[PATCH v3 1/3] ACPI / debugger: Add kernel flushing support

[PATCH v3 1/3] ACPI / debugger: Add kernel flushing support

[Lv Zheng]

This patch adds debugger log flushing support in kernel via .ioctl()
callback. The in-kernel flushing is more efficient, because it reduces
useless log IOs by bypassing log user_read/kern_write during the flush
period.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: linux-api@vger.kernel.org
---
 drivers/acpi/acpi_dbg.c          |   85 ++++++++++++++++++++++++++++++++++++--
 include/linux/acpi.h             |    1 +
 include/uapi/linux/acpi-ioctls.h |   21 ++++++++++
 3 files changed, 103 insertions(+), 4 deletions(-)
 create mode 100644 include/uapi/linux/acpi-ioctls.h

diff --git a/drivers/acpi/acpi_dbg.c b/drivers/acpi/acpi_dbg.c
index dee8692..a5f4457 100644
--- a/drivers/acpi/acpi_dbg.c
+++ b/drivers/acpi/acpi_dbg.c
@@ -46,6 +46,8 @@
 #define ACPI_AML_KERN		(ACPI_AML_IN_KERN | ACPI_AML_OUT_KERN)
 #define ACPI_AML_BUSY		(ACPI_AML_USER | ACPI_AML_KERN)
 #define ACPI_AML_OPEN		(ACPI_AML_OPENED | ACPI_AML_CLOSED)
+#define ACPI_AML_FLUSHING_LOG	0x0040 /* flushing log output */
+#define ACPI_AML_WAITING_CMD	0x0080 /* waiting for cmd input */
 
 struct acpi_aml_io {
 	wait_queue_head_t wait;
@@ -120,6 +122,16 @@ static inline bool __acpi_aml_busy(void)
 	return false;
 }
 
+static inline bool __acpi_aml_waiting_cmd(void)
+{
+	return !!(acpi_aml_io.flags & ACPI_AML_WAITING_CMD);
+}
+
+static inline bool __acpi_aml_flushing_log(void)
+{
+	return !!(acpi_aml_io.flags & ACPI_AML_FLUSHING_LOG);
+}
+
 static inline bool __acpi_aml_opened(void)
 {
 	if (acpi_aml_io.flags & ACPI_AML_OPEN)
@@ -152,6 +164,26 @@ static bool acpi_aml_busy(void)
 	return ret;
 }
 
+static inline bool acpi_aml_waiting_cmd(void)
+{
+	bool ret;
+
+	mutex_lock(&acpi_aml_io.lock);
+	ret = __acpi_aml_waiting_cmd();
+	mutex_unlock(&acpi_aml_io.lock);
+	return ret;
+}
+
+static inline bool acpi_aml_flushing_log(void)
+{
+	bool ret;
+
+	mutex_lock(&acpi_aml_io.lock);
+	ret = __acpi_aml_flushing_log();
+	mutex_unlock(&acpi_aml_io.lock);
+	return ret;
+}
+
 static bool acpi_aml_used(void)
 {
 	bool ret;
@@ -183,7 +215,8 @@ static bool acpi_aml_kern_writable(void)
 
 	mutex_lock(&acpi_aml_io.lock);
 	ret = !__acpi_aml_access_ok(ACPI_AML_OUT_KERN) ||
-	      __acpi_aml_writable(&acpi_aml_io.out_crc, ACPI_AML_OUT_KERN);
+	      __acpi_aml_writable(&acpi_aml_io.out_crc, ACPI_AML_OUT_KERN) ||
+	      __acpi_aml_flushing_log();
 	mutex_unlock(&acpi_aml_io.lock);
 	return ret;
 }
@@ -264,6 +297,9 @@ static int acpi_aml_write_kern(const char *buf, int len)
 	int n;
 	char *p;
 
+	if (acpi_aml_flushing_log())
+		return len;
+
 	ret = acpi_aml_lock_write(crc, ACPI_AML_OUT_KERN);
 	if (ret < 0)
 		return ret;
@@ -458,9 +494,18 @@ static int acpi_aml_wait_command_ready(bool single_step,
 	else
 		acpi_os_printf("\n%1c ", ACPI_DEBUGGER_COMMAND_PROMPT);
 
+	mutex_lock(&acpi_aml_io.lock);
+	acpi_aml_io.flags |= ACPI_AML_WAITING_CMD;
+	wake_up_interruptible(&acpi_aml_io.wait);
+	mutex_unlock(&acpi_aml_io.lock);
+
 	status = acpi_os_get_line(buffer, length, NULL);
 	if (ACPI_FAILURE(status))
 		return -EINVAL;
+
+	mutex_lock(&acpi_aml_io.lock);
+	acpi_aml_io.flags &= ~ACPI_AML_WAITING_CMD;
+	mutex_unlock(&acpi_aml_io.lock);
 	return 0;
 }
 
@@ -593,9 +638,11 @@ static int acpi_aml_read_user(char __user *buf, int len)
 	smp_rmb();
 	p = &crc->buf[crc->tail];
 	n = min(len, circ_count_to_end(crc));
-	if (copy_to_user(buf, p, n)) {
-		ret = -EFAULT;
-		goto out;
+	if (!acpi_aml_flushing_log()) {
+		if (copy_to_user(buf, p, n)) {
+			ret = -EFAULT;
+			goto out;
+		}
 	}
 	/* sync tail after removing logs */
 	smp_mb();
@@ -731,10 +778,40 @@ static unsigned int acpi_aml_poll(struct file *file, poll_table *wait)
 	return masks;
 }
 
+static int acpi_aml_flush(void)
+{
+	int ret;
+
+	/*
+	 * Discard output buffer and put the driver into a state waiting
+	 * for the new user input.
+	 */
+	mutex_lock(&acpi_aml_io.lock);
+	acpi_aml_io.flags |= ACPI_AML_FLUSHING_LOG;
+	mutex_unlock(&acpi_aml_io.lock);
+
+	ret = wait_event_interruptible(acpi_aml_io.wait,
+		acpi_aml_waiting_cmd());
+	(void)acpi_aml_read_user(NULL, ACPI_AML_BUF_SIZE);
+
+	mutex_lock(&acpi_aml_io.lock);
+	acpi_aml_io.flags &= ~ACPI_AML_FLUSHING_LOG;
+	mutex_unlock(&acpi_aml_io.lock);
+	return ret;
+}
+
+static long acpi_aml_ioctl(struct file *file,
+			   unsigned int cmd, unsigned long arg)
+{
+	return cmd == ACPI_IOCTL_DEBUGGER_FLUSH ?
+	       acpi_aml_flush() : -EINVAL;
+}
+
 static const struct file_operations acpi_aml_operations = {
 	.read		= acpi_aml_read,
 	.write		= acpi_aml_write,
 	.poll		= acpi_aml_poll,
+	.unlocked_ioctl	= acpi_aml_ioctl,
 	.open		= acpi_aml_open,
 	.release	= acpi_aml_release,
 	.llseek		= generic_file_llseek,
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 08235a6..9354fb8 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -26,6 +26,7 @@
 #include <linux/resource_ext.h>
 #include <linux/device.h>
 #include <linux/property.h>
+#include <uapi/linux/acpi-ioctls.h>
 
 #ifndef _LINUX
 #define _LINUX
diff --git a/include/uapi/linux/acpi-ioctls.h b/include/uapi/linux/acpi-ioctls.h
new file mode 100644
index 0000000..71b891a
--- /dev/null
+++ b/include/uapi/linux/acpi-ioctls.h
@@ -0,0 +1,21 @@
+/*
+ * ACPI IOCTL collections
+ *
+ * Copyright (C) 2016, Intel Corporation
+ * Authors: Lv Zheng <lv.zheng@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _UAPI_LINUX_ACPI_IOCTLS_H
+#define _UAPI_LINUX_ACPI_IOCTLS_H
+
+#include <linux/ioctl.h>
+
+#define ACPI_IOCTL_IDENT		'a'
+
+#define ACPI_IOCTL_DEBUGGER_FLUSH	_IO(ACPI_IOCTL_IDENT, 0x80)
+
+#endif /* _UAPI_LINUX_ACPI_IOCTLS_H */
-- 
1.7.10

[PATCH v4 1/3] ACPI / debugger: Add kernel flushing support

[Lv Zheng]

This patch adds debugger output flushing support in kernel via .ioctl()
callback. The in-kernel flushing is more efficient, because it reduces
useless log IOs by bypassing log user_read/kern_write during the flush
period.

This mechanism is useful for the batch mode. Scripts can integrate a batch
mode acpidbg instance to perform AML debugger functionalities.

As the batch mode always starts from a new command write, it thus requires
the kernel debugger driver to drop the old input/output first. The old
input is automatically dropped by acpi_os_get_line() via an error returning
value, but the output are remained in acpi_dbg output buffers and should be
dropped prior than reading the new command, otherwise, the old output can
be read out by the batch mode instance and the result of the batch mode
command will be messed up.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: linux-api@vger.kernel.org
---
 drivers/acpi/acpi_dbg.c          |   85 ++++++++++++++++++++++++++++++++++++--
 include/linux/acpi.h             |    1 +
 include/uapi/linux/acpi-ioctls.h |   21 ++++++++++
 3 files changed, 103 insertions(+), 4 deletions(-)
 create mode 100644 include/uapi/linux/acpi-ioctls.h

diff --git a/drivers/acpi/acpi_dbg.c b/drivers/acpi/acpi_dbg.c
index dee8692..a5f4457 100644
--- a/drivers/acpi/acpi_dbg.c
+++ b/drivers/acpi/acpi_dbg.c
@@ -46,6 +46,8 @@
 #define ACPI_AML_KERN		(ACPI_AML_IN_KERN | ACPI_AML_OUT_KERN)
 #define ACPI_AML_BUSY		(ACPI_AML_USER | ACPI_AML_KERN)
 #define ACPI_AML_OPEN		(ACPI_AML_OPENED | ACPI_AML_CLOSED)
+#define ACPI_AML_FLUSHING_LOG	0x0040 /* flushing log output */
+#define ACPI_AML_WAITING_CMD	0x0080 /* waiting for cmd input */
 
 struct acpi_aml_io {
 	wait_queue_head_t wait;
@@ -120,6 +122,16 @@ static inline bool __acpi_aml_busy(void)
 	return false;
 }
 
+static inline bool __acpi_aml_waiting_cmd(void)
+{
+	return !!(acpi_aml_io.flags & ACPI_AML_WAITING_CMD);
+}
+
+static inline bool __acpi_aml_flushing_log(void)
+{
+	return !!(acpi_aml_io.flags & ACPI_AML_FLUSHING_LOG);
+}
+
 static inline bool __acpi_aml_opened(void)
 {
 	if (acpi_aml_io.flags & ACPI_AML_OPEN)
@@ -152,6 +164,26 @@ static bool acpi_aml_busy(void)
 	return ret;
 }
 
+static inline bool acpi_aml_waiting_cmd(void)
+{
+	bool ret;
+
+	mutex_lock(&acpi_aml_io.lock);
+	ret = __acpi_aml_waiting_cmd();
+	mutex_unlock(&acpi_aml_io.lock);
+	return ret;
+}
+
+static inline bool acpi_aml_flushing_log(void)
+{
+	bool ret;
+
+	mutex_lock(&acpi_aml_io.lock);
+	ret = __acpi_aml_flushing_log();
+	mutex_unlock(&acpi_aml_io.lock);
+	return ret;
+}
+
 static bool acpi_aml_used(void)
 {
 	bool ret;
@@ -183,7 +215,8 @@ static bool acpi_aml_kern_writable(void)
 
 	mutex_lock(&acpi_aml_io.lock);
 	ret = !__acpi_aml_access_ok(ACPI_AML_OUT_KERN) ||
-	      __acpi_aml_writable(&acpi_aml_io.out_crc, ACPI_AML_OUT_KERN);
+	      __acpi_aml_writable(&acpi_aml_io.out_crc, ACPI_AML_OUT_KERN) ||
+	      __acpi_aml_flushing_log();
 	mutex_unlock(&acpi_aml_io.lock);
 	return ret;
 }
@@ -264,6 +297,9 @@ static int acpi_aml_write_kern(const char *buf, int len)
 	int n;
 	char *p;
 
+	if (acpi_aml_flushing_log())
+		return len;
+
 	ret = acpi_aml_lock_write(crc, ACPI_AML_OUT_KERN);
 	if (ret < 0)
 		return ret;
@@ -458,9 +494,18 @@ static int acpi_aml_wait_command_ready(bool single_step,
 	else
 		acpi_os_printf("\n%1c ", ACPI_DEBUGGER_COMMAND_PROMPT);
 
+	mutex_lock(&acpi_aml_io.lock);
+	acpi_aml_io.flags |= ACPI_AML_WAITING_CMD;
+	wake_up_interruptible(&acpi_aml_io.wait);
+	mutex_unlock(&acpi_aml_io.lock);
+
 	status = acpi_os_get_line(buffer, length, NULL);
 	if (ACPI_FAILURE(status))
 		return -EINVAL;
+
+	mutex_lock(&acpi_aml_io.lock);
+	acpi_aml_io.flags &= ~ACPI_AML_WAITING_CMD;
+	mutex_unlock(&acpi_aml_io.lock);
 	return 0;
 }
 
@@ -593,9 +638,11 @@ static int acpi_aml_read_user(char __user *buf, int len)
 	smp_rmb();
 	p = &crc->buf[crc->tail];
 	n = min(len, circ_count_to_end(crc));
-	if (copy_to_user(buf, p, n)) {
-		ret = -EFAULT;
-		goto out;
+	if (!acpi_aml_flushing_log()) {
+		if (copy_to_user(buf, p, n)) {
+			ret = -EFAULT;
+			goto out;
+		}
 	}
 	/* sync tail after removing logs */
 	smp_mb();
@@ -731,10 +778,40 @@ static unsigned int acpi_aml_poll(struct file *file, poll_table *wait)
 	return masks;
 }
 
+static int acpi_aml_flush(void)
+{
+	int ret;
+
+	/*
+	 * Discard output buffer and put the driver into a state waiting
+	 * for the new user input.
+	 */
+	mutex_lock(&acpi_aml_io.lock);
+	acpi_aml_io.flags |= ACPI_AML_FLUSHING_LOG;
+	mutex_unlock(&acpi_aml_io.lock);
+
+	ret = wait_event_interruptible(acpi_aml_io.wait,
+		acpi_aml_waiting_cmd());
+	(void)acpi_aml_read_user(NULL, ACPI_AML_BUF_SIZE);
+
+	mutex_lock(&acpi_aml_io.lock);
+	acpi_aml_io.flags &= ~ACPI_AML_FLUSHING_LOG;
+	mutex_unlock(&acpi_aml_io.lock);
+	return ret;
+}
+
+static long acpi_aml_ioctl(struct file *file,
+			   unsigned int cmd, unsigned long arg)
+{
+	return cmd == ACPI_IOCTL_DEBUGGER_FLUSH ?
+	       acpi_aml_flush() : -EINVAL;
+}
+
 static const struct file_operations acpi_aml_operations = {
 	.read		= acpi_aml_read,
 	.write		= acpi_aml_write,
 	.poll		= acpi_aml_poll,
+	.unlocked_ioctl	= acpi_aml_ioctl,
 	.open		= acpi_aml_open,
 	.release	= acpi_aml_release,
 	.llseek		= generic_file_llseek,
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 08235a6..9354fb8 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -26,6 +26,7 @@
 #include <linux/resource_ext.h>
 #include <linux/device.h>
 #include <linux/property.h>
+#include <uapi/linux/acpi-ioctls.h>
 
 #ifndef _LINUX
 #define _LINUX
diff --git a/include/uapi/linux/acpi-ioctls.h b/include/uapi/linux/acpi-ioctls.h
new file mode 100644
index 0000000..71b891a
--- /dev/null
+++ b/include/uapi/linux/acpi-ioctls.h
@@ -0,0 +1,21 @@
+/*
+ * ACPI IOCTL collections
+ *
+ * Copyright (C) 2016, Intel Corporation
+ * Authors: Lv Zheng <lv.zheng@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _UAPI_LINUX_ACPI_IOCTLS_H
+#define _UAPI_LINUX_ACPI_IOCTLS_H
+
+#include <linux/ioctl.h>
+
+#define ACPI_IOCTL_IDENT		'a'
+
+#define ACPI_IOCTL_DEBUGGER_FLUSH	_IO(ACPI_IOCTL_IDENT, 0x80)
+
+#endif /* _UAPI_LINUX_ACPI_IOCTLS_H */
-- 
1.7.10

Re: [PATCH v4 1/3] ACPI / debugger: Add kernel flushing support

[Rafael J. Wysocki]

On Tuesday, July 26, 2016 07:01:33 PM Lv Zheng wrote:
> This patch adds debugger output flushing support in kernel via .ioctl()
> callback. The in-kernel flushing is more efficient, because it reduces
> useless log IOs by bypassing log user_read/kern_write during the flush
> period.
> 
> This mechanism is useful for the batch mode.

Is it only useful or is it required?

Also the batch mode is introduced by the remaining patches in the series,
isn't it?

> Scripts can integrate a batch mode acpidbg instance to perform AML debugger
> functionalities.

This sentence is not parsable for me.  What does it mean, really?

> As the batch mode always starts from a new command write, it thus requires
> the kernel debugger driver to drop the old input/output first.

What does "the old" mean here?

> The old input is automatically dropped by acpi_os_get_line() via an error
> returning value,

I can't parse this too, sorry.

> but the output are remained in acpi_dbg output buffers and should be
> dropped prior than reading the new command, otherwise, the old output can
> be read out by the batch mode instance and the result of the batch mode
> command will be messed up.

Can you give an example here for clarity, please?

Thanks,
Rafael

RE: [PATCH v4 1/3] ACPI / debugger: Add kernel flushing support

[Zheng, Lv]

Hi, Rafael

> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Subject: Re: [PATCH v4 1/3] ACPI / debugger: Add kernel flushing support
> 
> On Tuesday, July 26, 2016 07:01:33 PM Lv Zheng wrote:
> > This patch adds debugger output flushing support in kernel via .ioctl()
> > callback. The in-kernel flushing is more efficient, because it reduces
> > useless log IOs by bypassing log user_read/kern_write during the flush
> > period.
> >
> > This mechanism is useful for the batch mode.
> 
> Is it only useful or is it required?

Should be required.

> 
> Also the batch mode is introduced by the remaining patches in the series,
> isn't it?

No, the batch mode is already in the upstream.
It's acpidbg -b "debugger commands".
For example:
acpidbg "-b namespace"
It's currently working.

> 
> > Scripts can integrate a batch mode acpidbg instance to perform AML debugger
> > functionalities.
> 
> This sentence is not parsable for me.  What does it mean, really?

By default, just like acpiexec, acpidbg is a shell like interactive utility.
Running acpidbg enters an interactive acpidbg shell.
So it cannot be used by the shell scripts.

With acpidbg -b option, acpidbg executes 1 command and exits.
So it can be used in a script like:

#!/bin/sh

acpidbg -b "find _LID"
acpidbg -b "namespace"

So that validators can use "acpidbg -b" to create recursive test cases.

> 
> > As the batch mode always starts from a new command write, it thus requires
> > the kernel debugger driver to drop the old input/output first.
> 
> What does "the old" mean here?

There are 2 cases, requiring the "flush" operation to be performed before an "acpidbg -b" execution.

A. The first case requiring flush is "prompt string":
After the kernel is booted, for the first acpidbg execution, user may choose to use it in either batch mode or in interactive mode.

A.1. First use is batch mode, for example, "acpidbg -b namespace"

1. acpi_dbg kernel driver outputs prompt strings into the output buffer.
2. acpidbg user program writes "namespace" command to the acpi_dbg kernel driver's input buffer.
3. acpi_dbg kernel driver reads the input buffer.
   It then obtains the "namespace" command and starts to execute it.
4. acpi_dbg kernel driver put the command result into the output buffer.
   And wait the userspace acpidbg to read the output.
5. acpidbg user program reads the acpi_dbg kernel driver's output buffer.
   It then obtains both the unexpected "prompt string" and the expected command result.

If there is a "flush" between 1 and 2, acpidbg user program won't obtain the unexpected "prompt string".

A.2. First use is interactive mode

However, it is not ensured that the "prompt string" can always appear before the command result.
For example:
1. User runs acpidbg with the interactive mode
2. acpi_dbg kernel driver outputs prompt strings into the output buffer.
3. acpidbg user program reads the output buffer and get the "prompt string".
4. User exits acpidbg.
5. User runs acpidbg with the batch mode.
In this case, "prompt string" has already been read by the 1st "acpidbg interactive execution"
Thus the 2nd "acpidbg batch execution" won't read unexpected "prompt string".

There is no harm to have a "flush" between 4 and 5.

Conclusion:
Whether there is "prompt string" in the output buffer depends on whether the "acpidbg -b" is the first use.
While running “flush" operation before batch mode execution can always drain the output buffer. 

B. The second case requiring "flush" is the old output:

The old output can be remained in the output buffer because we allow "asynchronous termination" of acpi_dbg IO.

The use case is:

1. User runs acpidbg by executing a command in either interactive mode or in batch mode.
2. acpi_dbg kernel driver put the command result into the output buffer.
3. When the output buffer is full, acpi_dbg kernel driver blocks, to wait for the userspace to read the result.
4. Instead of reading the result, userspace can close the acpi_dbg IO.
   User can achieve this by typing "ctrl+C" in acpidbg user program.
5. User runs acpidbg with the batch mode.
   It then obtains both the unexpected "old output", "prompt string" and the expected command result.

If there is a "flush" between 4 and 5, acpidbg user program won't obtain the unexpected "old output" and "prompt string".

> 
> > The old input is automatically dropped by acpi_os_get_line() via an error
> > returning value,
> 
> I can't parse this too, sorry.

OK, so we don't need this statement.
The flush is mainly used to drain the output buffer.
Because it can wrong output for the batch mode.

> 
> > but the output are remained in acpi_dbg output buffers and should be
> > dropped prior than reading the new command, otherwise, the old output can
> > be read out by the batch mode instance and the result of the batch mode
> > command will be messed up.
> 
> Can you give an example here for clarity, please?

Please find the 2 cases in A.1. and B.

Thanks and best regards
Lv