Message-ID: <2866a87b-d1d2-7e9e-57d6-fdcfcd62e27e@quicinc.com>
Date:   Thu, 10 Feb 2022 22:19:27 +0530
From:   Sai Prakash Ranjan <quic_saipraka@...cinc.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     Jiri Slaby <jirislaby@...nel.org>,
        Elliot Berman <quic_eberman@...cinc.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        "Shanker Donthineni" <shankerd@...eaurora.org>,
        Adam Wallis <awallis@...eaurora.org>,
        Timur Tabi <timur@...eaurora.org>,
        Elliot Berman <eberman@...eaurora.org>
Subject: Re: [PATCHv4] tty: hvc: dcc: Bind driver to CPU core0 for reads and
 writes

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@...eaurora.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.
>>
>> Signed-off-by: Shanker Donthineni <shankerd@...eaurora.org>
>> Acked-by: Adam Wallis <awallis@...eaurora.org>
>> Signed-off-by: Timur Tabi <timur@...eaurora.org>
>> Signed-off-by: Elliot Berman <eberman@...eaurora.org>
>> Signed-off-by: Sai Prakash Ranjan <quic_saipraka@...cinc.com>
>> ---
>>
>> 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@quicinc.com/
>>
>> 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

Powered by blists - more mailing lists