| Message ID | 20211208063847.27174-1-quic_saipraka@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,PATCHv2] tty: hvc: dcc: Bind driver to core0 for reads and writes | expand |
On Wed, Dec 08, 2021 at 12:08:47PM +0530, Sai Prakash Ranjan wrote: > From: Shanker Donthineni <shankerd@codeaurora.org> > > 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. > > Link: https://lore.kernel.org/lkml/1435344756-20901-1-git-send-email-timur@codeaurora.org/ > Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org> > Acked-by: Adam Wallis <awallis@codeaurora.org> > Signed-off-by: Timur Tabi <timur@codeaurora.org> > Signed-off-by: Elliot Berman <eberman@codeaurora.org> > Signed-off-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com> > --- > > Resending this v2 since earlier one had a typo in the variable type. > > Changes in v2: > * Checkpatch warning fixes. > * Use of IS_ENABLED macros instead of ifdefs. > > I also thought of making it depends on !HOTPLUG_CPU since it is broken > in case core0 is hotplugged off, but apparently HOTPLUG_CPU kconfig > has weird dependency issues, i.e., gets selected by CONFIG_PM and others. > So it will be almost like this feature won't be selectable at all if > I add !HOTPLUG_CPU kconfig dependency. Also HVC_DCC is a debug feature > where we need Trace32 like tools to access DCC windows in which case > these shortcomings can be expected since manual intervention is required > anyways for attaching a core to Trace32, so it won't matter much. But your code will break on systems when cpu 0 goes away, so this isn't going to work well at all. Please make this work for any cpu or handle the case when the cpu it is running on goes away. Also, this REALLY looks like you are trying to fix the kernel for a crazy userspace program. Why not fix the userspace program instead? Isn't that easier and then that way it will work for any kernel version? thanks, greg k-h
On 12/8/2021 12:30 PM, Greg Kroah-Hartman wrote: > On Wed, Dec 08, 2021 at 12:08:47PM +0530, Sai Prakash Ranjan wrote: >> From: Shanker Donthineni <shankerd@codeaurora.org> >> >> 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. >> >> Link: https://lore.kernel.org/lkml/1435344756-20901-1-git-send-email-timur@codeaurora.org/ >> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org> >> Acked-by: Adam Wallis <awallis@codeaurora.org> >> Signed-off-by: Timur Tabi <timur@codeaurora.org> >> Signed-off-by: Elliot Berman <eberman@codeaurora.org> >> Signed-off-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com> >> --- >> >> Resending this v2 since earlier one had a typo in the variable type. >> >> Changes in v2: >> * Checkpatch warning fixes. >> * Use of IS_ENABLED macros instead of ifdefs. >> >> I also thought of making it depends on !HOTPLUG_CPU since it is broken >> in case core0 is hotplugged off, but apparently HOTPLUG_CPU kconfig >> has weird dependency issues, i.e., gets selected by CONFIG_PM and others. >> So it will be almost like this feature won't be selectable at all if >> I add !HOTPLUG_CPU kconfig dependency. Also HVC_DCC is a debug feature >> where we need Trace32 like tools to access DCC windows in which case >> these shortcomings can be expected since manual intervention is required >> anyways for attaching a core to Trace32, so it won't matter much. > But your code will break on systems when cpu 0 goes away, so this isn't > going to work well at all. Please make this work for any cpu or handle > the case when the cpu it is running on goes away. It breaks as in we won't see the kernel logs in the userspace tools like Trace32 when CPU0 goes off, it isn't any different from detaching cores from Trace32 manually. There is no way to auto migrate to other CPU in such tools, you have to manually attach core to corresponding CPUs anyways. How do you suppose we write code in kernel such that it auto migrate in such external tools? Even if I did migrate to other CPU in kernel handling the hotplug case, it doesn't change anything, I can try to handle the !HOTPLUG_CPU, i.e., untangle Kconfig mess although its out of scope if you insist, might need some time to get through Kconfig things sorted. > Also, this REALLY looks like you are trying to fix the kernel for a > crazy userspace program. Why not fix the userspace program instead? > Isn't that easier and then that way it will work for any kernel version? This isn't a bug fix but mostly a feature where we wouldn't want to open multiple windows to see the logs. Would you do the same thing for your usual uart console? i.e., open all the 8 console (for 8 cpus) or 100s in case of 100s of CPU cores and look through each of them? I think no, so similarly anyone wouldn't want to open 8/100 windows of Trace32 or other userspace application which provides DCC functionality while debugging, it's not good at all. Also such feature requests do not go through for such tools, also there isn't a requirement for them since DCC is per core which is what they implement, its the **kernel which runs SMP** which needs to handle this so it makes it easier for the user to look at **kernel logs**, not Trace32 logs. Also what would you do for other applications like Trace32? Send this feature request to all those tools out there? How would that work? Thanks, Sai
diff --git a/drivers/tty/hvc/Kconfig b/drivers/tty/hvc/Kconfig index 8d60e0ff67b4..c0754a2e3fe4 100644 --- a/drivers/tty/hvc/Kconfig +++ b/drivers/tty/hvc/Kconfig @@ -87,6 +87,26 @@ config HVC_DCC driver. This console is used through a JTAG only on ARM. If you don't have a JTAG then you probably don't want this option. +config HVC_DCC_SERIALIZE_SMP + bool "Use DCC only on core 0" + depends on SMP && HVC_DCC + help + Some debuggers, such as Trace32 from Lauterbach GmbH, do not handle + reads/writes from/to DCC on more than one core. 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. + config HVC_RISCV_SBI bool "RISC-V SBI console support" depends on RISCV_SBI_V01 diff --git a/drivers/tty/hvc/hvc_dcc.c b/drivers/tty/hvc/hvc_dcc.c index 8e0edb7d93fd..b0ea2d4499da 100644 --- a/drivers/tty/hvc/hvc_dcc.c +++ b/drivers/tty/hvc/hvc_dcc.c @@ -3,8 +3,10 @@ #include <linux/console.h> #include <linux/init.h> +#include <linux/kfifo.h> #include <linux/serial.h> #include <linux/serial_core.h> +#include <linux/spinlock.h> #include <asm/dcc.h> #include <asm/processor.h> @@ -67,26 +69,177 @@ static int hvc_dcc_get_chars(uint32_t vt, char *buf, int count) return i; } +/* + * Check if the DCC is enabled. If CONFIG_HVC_DCC_SERIALIZE_SMP is enabled, + * then we assume then this function will be called first on core 0. That + * way, dcc_core0_available will be true only if it's available on core 0. + */ 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 (IS_ENABLED(CONFIG_HVC_DCC_SERIALIZE_SMP) && smp_processor_id() && + dcc_core0_available) + 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)) { + if (IS_ENABLED(CONFIG_HVC_DCC_SERIALIZE_SMP)) + dcc_core0_available = true; return true; + } } return false; } +#if defined(CONFIG_HVC_DCC_SERIALIZE_SMP) + +static void dcc_put_work_fn(struct work_struct *work); +static void dcc_get_work_fn(struct work_struct *work); +static DECLARE_WORK(dcc_pwork, dcc_put_work_fn); +static DECLARE_WORK(dcc_gwork, dcc_get_work_fn); +static DEFINE_SPINLOCK(dcc_lock); +static DEFINE_KFIFO(inbuf, unsigned char, 128); +static DEFINE_KFIFO(outbuf, unsigned char, 1024); + +/* + * Workqueue function that writes the output FIFO to the DCC on core 0. + */ +static void dcc_put_work_fn(struct work_struct *work) +{ + unsigned char ch; + unsigned long irqflags; + + spin_lock_irqsave(&dcc_lock, irqflags); + + /* While there's data in the output FIFO, write it to the DCC */ + while (kfifo_get(&outbuf, &ch)) + hvc_dcc_put_chars(0, &ch, 1); + + /* While we're at it, check for any input characters */ + while (!kfifo_is_full(&inbuf)) { + if (!hvc_dcc_get_chars(0, &ch, 1)) + break; + kfifo_put(&inbuf, ch); + } + + spin_unlock_irqrestore(&dcc_lock, irqflags); +} + +/* + * Workqueue function that reads characters from DCC and puts them into the + * input FIFO. + */ +static void dcc_get_work_fn(struct work_struct *work) +{ + unsigned char ch; + unsigned long irqflags; + + /* + * Read characters from DCC and put them into the input FIFO, as + * long as there is room and we have characters to read. + */ + spin_lock_irqsave(&dcc_lock, irqflags); + + while (!kfifo_is_full(&inbuf)) { + if (!hvc_dcc_get_chars(0, &ch, 1)) + break; + kfifo_put(&inbuf, ch); + } + spin_unlock_irqrestore(&dcc_lock, irqflags); +} + +/* + * Write characters directly to the DCC if we're on core 0 and the FIFO + * is empty, or write them to the FIFO if we're not. + */ +static int hvc_dcc0_put_chars(u32 vt, const char *buf, int count) +{ + int len; + unsigned long irqflags; + + spin_lock_irqsave(&dcc_lock, irqflags); + if (smp_processor_id() || (!kfifo_is_empty(&outbuf))) { + len = kfifo_in(&outbuf, buf, count); + spin_unlock_irqrestore(&dcc_lock, irqflags); + /* + * We just push data to the output FIFO, so schedule the + * workqueue that will actually write that data to DCC. + */ + schedule_work_on(0, &dcc_pwork); + return len; + } + + /* + * If we're already on core 0, and the FIFO is empty, then just + * write the data to DCC. + */ + len = hvc_dcc_put_chars(vt, buf, count); + spin_unlock_irqrestore(&dcc_lock, irqflags); + + return len; +} + +/* + * Read characters directly from the DCC if we're on core 0 and the FIFO + * is empty, or read them from the FIFO if we're not. + */ +static int hvc_dcc0_get_chars(u32 vt, char *buf, int count) +{ + int len; + unsigned long irqflags; + + spin_lock_irqsave(&dcc_lock, irqflags); + + if (smp_processor_id() || (!kfifo_is_empty(&inbuf))) { + len = kfifo_out(&inbuf, buf, count); + spin_unlock_irqrestore(&dcc_lock, irqflags); + + /* + * If the FIFO was empty, there may be characters in the DCC + * that we haven't read yet. Schedule a workqueue to fill + * the input FIFO, so that the next time this function is + * called, we'll have data. + */ + if (!len) + schedule_work_on(0, &dcc_gwork); + + return len; + } + + /* + * If we're already on core 0, and the FIFO is empty, then just + * read the data from DCC. + */ + len = hvc_dcc_get_chars(vt, buf, count); + spin_unlock_irqrestore(&dcc_lock, irqflags); + + return len; +} + +static const struct hv_ops hvc_dcc_get_put_ops = { + .get_chars = hvc_dcc0_get_chars, + .put_chars = hvc_dcc0_put_chars, +}; + +#else + static const struct hv_ops hvc_dcc_get_put_ops = { .get_chars = hvc_dcc_get_chars, .put_chars = hvc_dcc_put_chars, }; +#endif + static int __init hvc_dcc_console_init(void) { int ret;