Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Mon, 10 Apr 2023 10:00:23 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     Badhri Jagan Sridharan <badhri@google.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>,
        heikki.krogerus@linux.intel.com, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1] usb: typec: tcpm: Add kernel config to wrap around
 tcpm logs
Message-ID: <b1ac0ab9-e6f3-4301-a231-519752e4a735@roeck-us.net>
References: <20230410073134.488762-1-badhri@google.com>
 <2023041028-irritate-starless-a42f@gregkh>
 <CAPTae5KZ0zkaCWrtPYzyX8oTyvDhBhc-hvxyHn9VHZ32UOqJ3g@mail.gmail.com>
 <2023041004-antarctic-hardiness-524e@gregkh>
 <CAPTae5JFC8WUzjrMeiyw7tYfWpsZUQThrrvG_etx7Fb2KP6y6A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAPTae5JFC8WUzjrMeiyw7tYfWpsZUQThrrvG_etx7Fb2KP6y6A@mail.gmail.com>
Precedence: bulk

On Mon, Apr 10, 2023 at 02:00:08AM -0700, Badhri Jagan Sridharan wrote:
> On Mon, Apr 10, 2023 at 1:27 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Apr 10, 2023 at 01:08:55AM -0700, Badhri Jagan Sridharan wrote:
> > > On Mon, Apr 10, 2023 at 12:45 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Mon, Apr 10, 2023 at 07:31:34AM +0000, Badhri Jagan Sridharan wrote:
> > > > > This change adds CONFIG_TCPM_LOG_WRAPAROUND which when set allows the
> > > > > logs to be wrapped around. Additionally, when set, does not clear
> > > > > the TCPM logs when dumped.
> > > > >
> > > > > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > > > > ---
> > > > >  drivers/usb/typec/tcpm/Kconfig | 6 ++++++
> > > > >  drivers/usb/typec/tcpm/tcpm.c  | 9 +++++++--
> > > > >  2 files changed, 13 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/usb/typec/tcpm/Kconfig b/drivers/usb/typec/tcpm/Kconfig
> > > > > index e6b88ca4a4b9..4dd2b594dfc9 100644
> > > > > --- a/drivers/usb/typec/tcpm/Kconfig
> > > > > +++ b/drivers/usb/typec/tcpm/Kconfig
> > > > > @@ -18,6 +18,12 @@ config TYPEC_TCPCI
> > > > >       help
> > > > >         Type-C Port Controller driver for TCPCI-compliant controller.
> > > > >
> > > > > +config TCPM_LOG_WRAPAROUND
> > > > > +     bool "Enable TCPM log wraparound"
> > > > > +     help
> > > > > +       When set, wraps around TCPM logs and does not clear the logs when dumped. TCPM logs by
> > > > > +       default gets cleared when dumped and does not wraparound when full.
> > > >
> > > > Kconfig help text needs to be wrapped at the properly width.
> > >
> > > I assumed that the width is 100 characters, but it looks like it is
> > > 80. Will fix it in the next version.
> > > >
> > > > And you do not provide any hint here as to why this is not the default
> > > > option, or why someone would want this.
> > >
> > > "TCPM logs by default gets cleared when dumped and does not wraparound
> > > when full." was intended
> > > to convey why someone would want to set this. Perhaps it's not effective.
> > >
> > > Does the below look better:
> > > "TCPM logs by default gets cleared when dumped and does not wraparound
> > > when full. This can be overridden by setting this config.
> > > When the config is set, TCPM wraps around logs and does not clear the
> > > logs when dumped."
> > >
> > > Also, I could make this default if that's OK with Guenter.
> > >
> > > >
> > > > So, why is this not just the default operation anyway?  Why would you
> > > > ever want the logs cleared?
> > >
> > > I remember Guenter mentioning that he was finding it useful to not
> > > wrap around the logs to fix problems
> > > during tcpm_register_port (init sequence). IMHO wrapping around the
> > > logs helps to triage interoperability
> > > issues uncovered during testing. So both approaches have their own advantages.
> >
> > But as this is a build-time option, what would cause someone to choose
> > one over the other, and then when the system is running, they can't
> > change them?
> 
> During initial phases of bringup, it makes sense to not wrap around
> the logs, but, once bringup is done its most effective to wraparound
> so that interop issues reported by the end users can be triaged where
> TCPM logs are very effective.

Not really, because the problem tends to be the remote device
(or at least it used to be), not a driver under development.

> Also, without wrapping around, the logbuffer logs are completely stale
> after the user goes through a few USB connect and disconnect cycles
> till the system is rebooted.

Unless they are cleared.

Again, the premise is wrong here. The idea was to ensure that the
beginning of a problem is caught and available in the log, not its tail.
This includes "beginning" as the behavior immediately after boot regarding
an already connected device, and the behavior observed when inserting
a device into the running system. Again, in both cases it was most
important to catch the beginning of a problem, not its tail.

> If we don't want to pursue the Kconfig option, we should atleast make
> TCPM default to wrapping the logs around when full so we could
> maximise the use of the logbuffer contents.
> 

I don't really agree, but then I am not in a position to argue either.
Maybe the premise and reasons have changed since I wrote the driver.

Guenter

> >
> > That does not seem good at all.
> >
> > Why not just use tracing instead of this odd custom log buffer?  That's
> > a better solution overall, right?
> 
> Tracing is not enabled by default in most systems. End users don't
> want to retry and reproduce the failure to collect traces even if they
> could reproduce it consistently.
> So tracing was not proving handy here.
> 
> Thanks,
> Badhri
> 
> >
> > thanks,
> >
> > greg k-h