linux-kernel - Re: [PATCH 5/5] MAINTAINERS: Add Lukas Wunner as co-maintainer of thunderbolt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180910121308.GN14465@lahna.fi.intel.com>
Date:   Mon, 10 Sep 2018 15:13:08 +0300
From:   Mika Westerberg <mika.westerberg@...ux.intel.com>
To:     Lukas Wunner <lukas@...ner.de>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andreas Noever <andreas.noever@...il.com>,
        Michael Jamet <michael.jamet@...el.com>,
        Yehezkel Bernat <YehezkelShB@...il.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/5] MAINTAINERS: Add Lukas Wunner as co-maintainer of
 thunderbolt

On Mon, Sep 10, 2018 at 12:25:14PM +0200, Lukas Wunner wrote:
> On Mon, Sep 10, 2018 at 12:33:33PM +0300, Mika Westerberg wrote:
> > The reason for making the driver less verbose comes from direct feedback
> > from the community. For example:
> > 
> >   https://lkml.org/lkml/2017/10/31/864
> 
> I am not opposed to muting messages to KERN_DEBUG severity which merely
> report what the driver is doing such as:
> 
>     "control channel created"
>     "control channel starting..."
> 
> However messages should NOT be muted which report register contents
> or register changes unless those registers are *fully* documented and
> register changes are known to work *reliably*.  The URL you're referring
> to above provides an example where that's not the case:
> 
>     "disabling interrupt at register 0x38200 bit 12 (0xffffffff -> 0xffffefff)"
> 
> Something is broken here, the register was read as "all ones".
> This doesn't seem to work as reliable as it should and in that case
> please don't mute the message until we know it's fixed and always
> works.

That is actually already on my todo list. What happens here is that on
PCs the controller is hot-removed and the driver is removed but it tries
to touch hardware after it is already gone. We have a flag for that
already so I'm hoping to use that and skip touching hardware if it is
set.

> Also, it is quite customary and serves a useful purpose to report
> devices at KERN_INFO severity as they're enumerated.  E.g. the PCI
> bus logs messages for each enumerated device, pciehp logs the
> port's capabilities on probe, and so on.  Therefore please do not
> mute the enumeration of switches and their ports.  If you find the
> messages too noisy, feel free to condense the data reported for each
> port to 1 or 2 lines.

That I realized also myself after testing the patch bit more. We could
print some information that is useful upon device connect/disconnect.

But I don't think dumping the switch structure and ports are useful. I
would rather make it to follow USB and print the actual device that was
connected (this is the information read from DROM and which is exported
in sysfs as well).

So I'm experimenting patch that does something like this when device is
connected:

  thunderbolt 0-1: device 1:8003 connected
  thunderbolt 0-1: Apple, Inc Thunderbolt to FireWire Adapter

and when it is disconnected

  thunderbolt 0-1: device disconnected

> We currently print "Port ..." on enumeration, but use the syntax
> "<route string>:<port number>" for other port-related messages
> printed with tb_port_*().  It might be beneficial to use a single
> syntax consistently.

Since thunderbolt is now bus and all the devices include embedded struct
device we can use dev_*() just like everybody else is doing.