Re: [PATCH 1/3] thunderbolt: Make the driver less verbose

From: Lukas Wunner <lukas@wunner.de>
To: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: linux-kernel@vger.kernel.org,
	Andreas Noever <andreas.noever@gmail.com>,
	Michael Jamet <michael.jamet@intel.com>,
	Yehezkel Bernat <YehezkelShB@gmail.com>
Subject: Re: [PATCH 1/3] thunderbolt: Make the driver less verbose
Date: Thu, 6 Sep 2018 10:41:43 +0200	[thread overview]
Message-ID: <20180906084143.sah4njyft7z5squb@wunner.de> (raw)
In-Reply-To: <20180905095451.GI2283@lahna.fi.intel.com>

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.

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

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."

> Anything running on Alpine Ridge and higher does
> not require reverse-engineering (even on Apple systems) because those
> are already supported in the driver.

Long term it may be worthwhile to move to OS-controlled tunnel management
even when ICM-controlled tunnel management is available as the latter
might not support features of the former, e.g. correlation of PCI devices
with Thunderbolt ports:
https://github.com/l1k/linux/commit/d024c6e7e80e

Thanks,

Lukas