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

[Sai Prakash Ranjan <quic_saipraka@quicinc.com>]

On 4/28/2022 12:29 PM, Greg Kroah-Hartman wrote:
> On Thu, Apr 28, 2022 at 10:04:34AM +0530, Sai Prakash Ranjan wrote:
>> Hi Greg,
>>
>> On 4/15/2022 11:55 AM, Greg Kroah-Hartman wrote:
>>> On Thu, Mar 10, 2022 at 08:56:36AM +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.
>>>>
>>>> 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>
>>>> ---
>>>>
>>>> Changes in v6:
>>>>    * Disable CPU hotplug when CONFIG_HVC_DCC_SERIALIZE_SMP=y.
>>>>
>>>> Changes in v5:
>>>>    * Use get_cpu() and put_cpu() for CPU id check in preemptible context.
>>>>    * Revert back to build time Kconfig.
>>>>    * Remove unnecessary hotplug locks, they result in sleeping in atomic context bugs.
>>>>    * Add a comment for the spinlock.
>>>>
>>>> Changes in v4:
>>>>    * Use module parameter for runtime choice of enabling this feature.
>>>>    * 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/Kconfig   |  20 +++++
>>>>    drivers/tty/hvc/hvc_dcc.c | 175 +++++++++++++++++++++++++++++++++++++-
>>>>    2 files changed, 192 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/hvc/Kconfig b/drivers/tty/hvc/Kconfig
>>>> index 8d60e0ff67b4..62560cd0c04d 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 CPU 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 CPU core. Each core has its
>>>> +	  own DCC device registers, so when a CPU core reads or writes from/to
>>>> +	  DCC, it only accesses its own DCC device. Since kernel code can run on
>>>> +	  any CPU 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.
>>> Why are we documenting, and supporting, a closed source userspace tool
>>> with kernel changes?  Does this advertisement deserve to be in the
>>> kernel source tree?
>> Ok, I will remove the comment.
>>
>>> And why can't they just fix their tool if this is such a big issue?  Why
>>> does this only affect this one platform and not all other smp systems?
>> Hmm, this has been discussed in all the past versions of this series and still we
>> are at the same question :) I will write a small summary below which will cover
>> mostly relevant discussions we discussed till now and then I can point to it
>> whenever this question is asked again.
> No, it needs to go into the changelog text so that we know what we are
> reviewing and considering when you submit it.  Never refer back to some
> old conversation, how are we supposed to remember that?

True given the amount of patches you handle. I will be explicit and add more details in the next
version.

> So this all seems to be debugging-only code, and this config option
> should NEVER be turned on for a real system.  That makes much more
> sense, and is something that I don't recall anyone saying before.

Ah my bad, I thought it was known thing given the DCC driver was already
present.

> So make this very very explicit, both in the changelog, and in the
> Kconfig text, AND when the driver loads have it spit out a huge message
> in the kernel log saying that this is for debugging only and that no one
> should see this on a normal running system.  We have examples of other
> Kconfig options that do this at runtime, copy what they do so it's
> painfully obvious.  Like what is in clk_debug_init().

Sure, I will make these changes and post.

Thanks,
Sai