Re: [PATCHv4] tty: hvc: dcc: Bind driver to CPU core0 for reads and writes

From: Sai Prakash Ranjan
Date: Thu Feb 10 2022 - 11:49:38 EST

Hi,

On 2/10/2022 7:54 PM, Greg Kroah-Hartman wrote:
On Thu, Feb 10, 2022 at 07:26:32PM +0530, Sai Prakash Ranjan wrote:
From: Shanker Donthineni <shankerd@xxxxxxxxxxxxxx>

Some debuggers, such as Trace32 from Lauterbach GmbH, do not handle
reads/writes from/to DCC on secondary cores. Each core has its
own DCC device registers, so when a core reads or writes from/to DCC,
it only accesses its own DCC device. Since kernel code can run on
any core, every time the kernel wants to write to the console, it
might write to a different DCC.

In SMP mode, Trace32 creates multiple windows, and each window shows
the DCC output only from that core's DCC. The result is that console
output is either lost or scattered across windows.

Selecting this option will enable code that serializes all console
input and output to core 0. The DCC driver will create input and
output FIFOs that all cores will use. Reads and writes from/to DCC
are handled by a workqueue that runs only core 0.

Signed-off-by: Shanker Donthineni <shankerd@xxxxxxxxxxxxxx>
Acked-by: Adam Wallis <awallis@xxxxxxxxxxxxxx>
Signed-off-by: Timur Tabi <timur@xxxxxxxxxxxxxx>
Signed-off-by: Elliot Berman <eberman@xxxxxxxxxxxxxx>
Signed-off-by: Sai Prakash Ranjan <quic_saipraka@xxxxxxxxxxx>
---

Changes in v4:
* Use module parameter for runtime choice of enabling this feature.
No, this is not the 1990's, module parameters do not work and are not
sustainable. They operate on a code-level while you are modifying a
device-specific attribute here. Please make this per-device if you
really want to be able to somehow turn this on or off.

Can you please explain how is this a device-specific thing? I guess you mean something like a device
tree property but that is not what it is used for, it is for hardware description of a device and this is not a
HW description but a software feature. Arch information such as DCC existing only on ARM64 is already
implied in Kconfig when this driver was merged before. Anyone with an ARM64 device with DCC can use
this feature. So anyone be it Mediatek, Qualcomm or any others can use this and there is nothing device
specific here. We need something on code level which is why the earlier version had build time Kconfig but
you mentioned something about runtime choice, so I modified to use module parameter. I will move back
to build time configuration for next version unless you have something else in mind when you mean a
runtime choice?

* Use hotplug locks to avoid race between cpu online check and work schedule.
* Remove ifdefs and move to common ops.
* Remove unnecessary check for this configuration.
* Use macros for buf size instead of magic numbers.
* v3 - https://lore.kernel.org/lkml/20211213141013.21464-1-quic_saipraka@xxxxxxxxxxx/

Changes in v3:
* Handle case where core0 is not online.

Changes in v2:
* Checkpatch warning fixes.
* Use of IS_ENABLED macros instead of ifdefs.

---
drivers/tty/hvc/hvc_dcc.c | 177 +++++++++++++++++++++++++++++++++++++-
1 file changed, 174 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/hvc/hvc_dcc.c b/drivers/tty/hvc/hvc_dcc.c
index 8e0edb7d93fd..535b09441e55 100644
--- a/drivers/tty/hvc/hvc_dcc.c
+++ b/drivers/tty/hvc/hvc_dcc.c
@@ -2,19 +2,35 @@
/* Copyright (c) 2010, 2014 The Linux Foundation. All rights reserved. */
#include <linux/console.h>
+#include <linux/cpu.h>
+#include <linux/cpumask.h>
#include <linux/init.h>
+#include <linux/kfifo.h>
+#include <linux/moduleparam.h>
#include <linux/serial.h>
#include <linux/serial_core.h>
+#include <linux/spinlock.h>
#include <asm/dcc.h>
#include <asm/processor.h>
#include "hvc_console.h"
+static bool serialize_smp;
+module_param(serialize_smp, bool, 0444);
+MODULE_PARM_DESC(serialize_smp, "Serialize all DCC console input and output to CPU core 0");
+
/* DCC Status Bits */
#define DCC_STATUS_RX (1 << 30)
#define DCC_STATUS_TX (1 << 29)
+#define DCC_INBUF_SIZE 128
+#define DCC_OUTBUF_SIZE 1024
Why these random sizes? Why is one bigger than the other? Why are they
these specific numbers?

These are input and output kfifo sizes, it is a software construct and there is no specification as such.
As per kfifo documentation, size must be a power of 2. As for the sizes, IN_BUF size is less assuming
that amount of input data (RX) is usually less when compared to the output data (TX ) on the DCC console.
For ex, during boot the output kernel logs on the DCC console would be more than the input.
Given DCC console is very slow, we wouldn't want to make the sizes too large, hence 1024.
This configuration is well tested for years now and I would like to keep these numbers unless someone
else comes with some issue with these sizes.

+
+static DEFINE_SPINLOCK(dcc_lock);
What is this locking? Please document it (didn't checkpatch complain?)

Sure, I will document this and no checkpatch doesn't complain even with --strict option.


+static DEFINE_KFIFO(inbuf, unsigned char, DCC_INBUF_SIZE);
+static DEFINE_KFIFO(outbuf, unsigned char, DCC_OUTBUF_SIZE);
+
static void dcc_uart_console_putchar(struct uart_port *port, int ch)
{
while (__dcc_getstatus() & DCC_STATUS_TX)
@@ -67,24 +83,179 @@ static int hvc_dcc_get_chars(uint32_t vt, char *buf, int count)
return i;
}
+/*
+ * Check if the DCC is enabled. If serialize_smp module param is enabled,
+ * then we assume then this function will be called first on core0. That way,
+ * dcc_core0_available will be true only if it's available on core0.
+ */
static bool hvc_dcc_check(void)
{
unsigned long time = jiffies + (HZ / 10);
+ static bool dcc_core0_available;
+
+ /*
+ * If we're not on core 0, but we previously confirmed that DCC is
+ * active, then just return true.
+ */
+ if (serialize_smp && smp_processor_id() && dcc_core0_available)
Why are you checking smp_processor_id()? Are you sure it is safe to do
that here?

It is to check for non-zero CPU core as mentioned in the comment above the check.
On safety, thanks for that, I guess you meant about calling smp_processor_id() in preemptible
context bug, so I tested with CONFIG_DEBUG_PREEMPT=y and that is a premptible section
and makes my system unbootable. I'll use proper get_cpu() and put_cpu() around this check.



+ return true;
/* Write a test character to check if it is handled */
__dcc_putchar('\n');
while (time_is_after_jiffies(time)) {
- if (!(__dcc_getstatus() & DCC_STATUS_TX))
+ if (!(__dcc_getstatus() & DCC_STATUS_TX)) {
+ dcc_core0_available = true;
return true;
+ }
That's a hard busy loop, are you sure it will always exit?

How? Nothing is changed there from before except setting a variable and the loop never checks
for that variable.

Thanks,
Sai