Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 11 Feb 2019 10:52:03 +0200
From:   Mika Westerberg <mika.westerberg@linux.intel.com>
To:     Lukas Wunner <lukas@wunner.de>
Cc:     linux-kernel@vger.kernel.org,
        Michael Jamet <michael.jamet@intel.com>,
        Yehezkel Bernat <YehezkelShB@gmail.com>,
        Andreas Noever <andreas.noever@gmail.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Subject: Re: [PATCH v2 15/28] thunderbolt: Deactivate all paths before
 restarting them
Message-ID: <20190211085203.GV7875@lahna.fi.intel.com>
References: <20190206131738.43696-1-mika.westerberg@linux.intel.com>
 <20190206131738.43696-16-mika.westerberg@linux.intel.com>
 <20190211063555.2bozvqen2jmskox4@wunner.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190211063555.2bozvqen2jmskox4@wunner.de>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Feb 11, 2019 at 07:35:55AM +0100, Lukas Wunner wrote:
> On Wed, Feb 06, 2019 at 04:17:25PM +0300, Mika Westerberg wrote:
> > We can't be sure the paths are actually properly deactivated when a
> > tunnel is restarted after resume.
> 
> Why can't we be sure?  Please provide proper reasoning.

IIRC the reason was that when you suspend, then reconfigure parts of the
topology and resume, establishing the tunnel again went wrong. I'll
update the changelog with a better reasoning.

> > So instead of marking all paths as
> > inactive we go ahead and deactivate them explicitly.
> 
> This seems like a bad idea if the root partition is on a Thunderbolt-
> attached drive, the system is waking from hibernate and the EFI NHI
> driver has already established a tunnel to that drive.  It would seem
> more appropriate to discover tunnels already existing on resume from
> system sleep and then attempt to establish any others that might be
> missing.

That's what we do in the patch following, no? We discover the EFI
created paths and use that information to re-establish tunnels upon S3
resume and also when they are torn down.

> > @@ -183,8 +183,15 @@ int tb_tunnel_restart(struct tb_tunnel *tunnel)
> >  
> >  	tb_tunnel_info(tunnel, "activating\n");
> >  
> > +	/* Make sure all paths are properly disabled before enable them again */
> 
> This isn't proper English, s/enable/enabling/.

Thanks, I'll fix it up.