Message-ID: <20180906104746.GW2283@lahna.fi.intel.com>
Date:   Thu, 6 Sep 2018 13:47:46 +0300
From:   Mika Westerberg <mika.westerberg@...ux.intel.com>
To:     Lukas Wunner <lukas@...ner.de>
Cc:     linux-kernel@...r.kernel.org,
        Andreas Noever <andreas.noever@...il.com>,
        Michael Jamet <michael.jamet@...el.com>,
        Yehezkel Bernat <YehezkelShB@...il.com>
Subject: Re: [PATCH 1/3] thunderbolt: Make the driver less verbose

On Thu, Sep 06, 2018 at 10:41:43AM +0200, Lukas Wunner wrote:
> On Wed, Sep 05, 2018 at 12:54:51PM +0300, Mika Westerberg wrote:
> > On Wed, Sep 05, 2018 at 11:05:10AM +0200, Lukas Wunner wrote:
> > > On Mon, Sep 03, 2018 at 04:33:02PM +0300, Mika Westerberg wrote:
> > > > Currently the driver logs quite a lot to the system message buffer even
> > > > when doing normal operations. This information is not useful for
> > > > ordinary users and might even annoy some.
> > > 
> > > No, the verbose logging is done on purpose to aid us in reverse-engineering
> > > the protocol.  For example ...
> > > 
> > > > -	tb_port_info(port, "  Unknown1: %#x Unknown2: %#x Unknown3: %#x\n",
> > > > -		     hop->unknown1, hop->unknown2, hop->unknown3);
> > > 
> > > ... why do you think we're logging these seemingly stupid unknown
> > > bitfields?  Because whenever someone posts dmesg output they
> > > inadvertantly post the contents of those unknown fields and we can
> > > then google the value of those fields on various controllers and
> > > machines and deduce their possible meaning.
> > 
> > And the majority of people get tons of completely useless messages
> > filling their dmesgs? No, I don't think that's a good thing.
> 
> As you know the OS-controlled tunnel manager is far from feature
> complete.  I think the "majority of people" are perfectly willing
> to accept verbose logging if it helps us fill in those gaps.
> 
> You make it sound like logfiles are spammed all the time, but
> messages are in fact only logged on driver initialization and hotplug.

And every time the controller runtime suspends/resumes. Every
suspend/resume. Every time ring gets initialized/teared when you connect
cable between computers. It is way too much for a driver that is part of
production operating system.

> We wouldn't be in this situation if we had a proper Thunderbolt spec.
> It wasn't *my* idea to keep it under wraps.

Please don't start that again.

I can't affect when the spec is released, really. I'm just a poor driver
guy trying to get this working as good as possible.

> So please leave the messages at info level so as not to hinder our work.
> 
> As for your claim that the "majority of people" find the messages
> useless", I rather suspect that you, personally, find them useless
> because you complained about noisiness of the driver before:
> 
>    "I don't think we want to log anything with info level to be honest.
>     The driver currently already is pretty noisy so adding even more
>     information there just makes it worse ;-)
>     I would rather convert debugging information to use tracepoints and
>     get rid of the tb_*_info() things completely."
>     https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1401181.html
> 
> And guess what I responded:
> 
>    "The noisiness has value in that it helps with reverse-engineering:
>     Just google for dmesg output and check what other machines are
>     reporting for unknown registers. :-)
>     If there was public documentation available or Intel would be okay
>     with answering specific questions (as you've done with the 0x30
>     attribute id), then the value obviously diminishes."

It is not just me, see here:

https://lkml.org/lkml/2017/10/31/864

And I fully agree.

Andreas was fine at the time to silence the driver. I just did not have
time to do that until now.

Andreas, let me know if you have objections about this patch.

Powered by blists - more mailing lists