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

[Greg Kroah-Hartman]

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@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 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@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/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?

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.

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().

thanks,

greg k-h