Date: Mon, 5 Jun 2017 12:32:49 +0300
From: Mika Westerberg <mika.westerberg@linux.intel.com>
To: Lukas Wunner <lukas@wunner.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Andreas Noever <andreas.noever@gmail.com>,
        Michael Jamet <michael.jamet@intel.com>,
        Yehezkel Bernat <yehezkel.bernat@intel.com>,
        Amir Levy <amir.jer.levy@intel.com>, Andy Lutomirski <luto@kernel.org>,
        Mario.Limonciello@dell.com, Jared.Dominguez@dell.com,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 19/27] thunderbolt: Add new Thunderbolt PCI IDs
Message-ID: <20170605093249.GK3454@lahna.fi.intel.com>
References: <20170602140524.23367-1-mika.westerberg@linux.intel.com>
 <20170602140524.23367-20-mika.westerberg@linux.intel.com>
 <20170605081437.GA7519@wunner.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170605081437.GA7519@wunner.de>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
User-Agent: Mutt/1.7.1 (2016-10-04)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3553
Lines: 79

On Mon, Jun 05, 2017 at 10:14:37AM +0200, Lukas Wunner wrote:
> On Fri, Jun 02, 2017 at 05:05:16PM +0300, Mika Westerberg wrote:
> > --- a/drivers/thunderbolt/nhi.h
> > +++ b/drivers/thunderbolt/nhi.h
> > @@ -143,4 +143,21 @@ static inline int ring_tx(struct tb_ring *ring, struct ring_frame *frame)
> >  	return __ring_enqueue(ring, frame);
> >  }
> >  
> > +/*
> > + * PCI IDs used in this driver from Win Ridge forward. There is no
> > + * need for the PCI quirk anymore as we will use ICM also on Apple
> > + * hardware.
> > + */
> 
> I wrote a patch a few months ago which replaces the PCI quirk with
> device links:
> 
> https://github.com/l1k/linux/commit/8e2e7eaa1163
> 
> I was going to upstream this sometime this year, it probably would
> have been better if I had done that already but I wasn't expecting
> your series.
> 
> In any case, all Thunderbolt PCI device IDs can then be moved to a
> header in drivers/thunderbolt/, except for a few TB1 devices which
> are referenced by quirk_thunderbolt_hotplug_msi() and
> quirk_apple_poweroff_thunderbolt().

OK, but that should be a separate patch, right? On top of your device
links patch.

> As to using ICM on Apple hardware, I've heard from people with
> Alpine Ridge MacBook Pros that the native (i.e. non-ICM) driver
> at least probes fine.  I've yet to hear from folks who have
> actually tested it with any attached TB3 devices, but my expectation
> would be that it should work fine in native mode since the protocol
> seems to be the same.

I have one Mac with Alpine Ridge (well 4 Thunderbolt ports, 2 Alpine
Ridges) and it indeed works in native mode but it can tunnel only one
device, no display port.

However, starting ICM on them allows you to connect up to 6 devices
including display port. It also allows cross-domain connections where we
can implement things like Amir's networking driver.

That's the reason we did it this way - to get Thunderbolt working the
same way in Linux than it works on Macs running OS X :)

> I think I saw somewhere that you reset the root switch when reconfiguring
> the chip to run in ICM mode.  That's a problem because on Apple hardware,
> there's a (native, non-ICM) EFI NHI driver which sets up tunnels to any
> devices which are already attached on boot.  This can be used to boot
> from Thunderbolt-attached harddisks.  I imagine that resetting the chip
> to switch to ICM-mode will cause those already established tunnels to
> go away, essentially breaking booting from TB-attached disks.  That's
> a regression which we should try to avoid.

Yes, we reset the links and ICM re-establishes them the but during that
time the PCIe tunnel is down. However, it should not affect booting as
that is done in the EFI firmware as far as I can tell. If you have
rootfs on a TBT harddisk, it should not be problem either if your initrd
includes drivers for Thunderbolt and the disks you are using.

Unless I'm missing something.

> Ideally, the user should be able to choose whether to use ICM-mode or
> native mode.  This could be implemented via a module parameter.

I have nothing against that in principle but at least we should default
to the most functioning connection manager then, and I'm not sure it is
worth adding right now until we get some feedback from people actually
running ICM on Macs.

> I imagine it might even be possible to use native mode on non-Macs
> and this should be allowed as well.

While I guess it is doable, it is a path that is not tested at all so
you will get more problems than you are trying to solve, IMHO.