Received: by 2002:ac0:bc90:0:0:0:0:0 with SMTP id a16csp3059902img; Mon, 25 Mar 2019 02:58:35 -0700 (PDT) X-Google-Smtp-Source: APXvYqw6GozUkvT25bZd7JglqUwmIVmleQ0TxesWPccqL1HopYbsB4J/QVQNeL0GA1JWOHUnXPpo X-Received: by 2002:a17:902:8d97:: with SMTP id v23mr16602655plo.298.1553507915035; Mon, 25 Mar 2019 02:58:35 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1553507915; cv=none; d=google.com; s=arc-20160816; b=0tNlBNFFLAog945l1AhOUCWJn/ahYyEXGuZ+Ps6YC7JmadHo5uXinzPrRO1z42mIhL fzKdMwdYHYVnsjoCPPmBvZ2rvBrtQS8E29pjVIXGP6yEGrjsn5PYsC8J08poVeykPlCd 6DV7tAmJmET7vL+X47ZypD/GHzCHlzVICz3TsjJXrLzn5ns38jQTPbVcKEKnWVSmdsjf hGZil3hw4pyR5HZQHEsSJVV0yDHKzVctW1FBLDE/EeqXcIPv9zW1AL7eNRRedYIDv7Jg VBOMD1C6OnXVMa2dLlgZ11vHU7SYbCLe0dZqCK8mRDhcPJsVFhRqtHrRpBI7QpRcUQey XGFA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:organization:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date; bh=2DgvEHZQcexSm2LNStt7brbtpLrylq3+f/XWZw7w6zE=; b=yms1V+pF2Zn6RvJRhFUW+qGyhfr0xJL/Fsm/8sTqgQivCI9tGI0L4mZThOGiG59PC0 JN8rNKlQooNNAJa/I/Pw5EToq7iy+lV3Yuw6mNOMXDDa8JHWMkugG29mXBJDrtqyKqxh HxdJz0HbJOmL4lDFOmEIWnDzjvXTS0BPp4jHrRzuFtEcyF9q4WEIRDAlF7stwpwNiwpW WTJVJpYgHADdIQGdl2yt5ImQVwMk7/o5wwstVsxCkMBDcta/qiBjpJUpy7v6XIOAvWVQ 7CYyxJ0Z9zQ0f5LOitFHruEmC4tDc5f0EumUeqxaAqsuVQlgGwxEauSo7z8CSOJAo3Ng n+VQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id d40si14178343pla.114.2019.03.25.02.58.20; Mon, 25 Mar 2019 02:58:35 -0700 (PDT) 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; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730470AbfCYJ5i (ORCPT + 99 others); Mon, 25 Mar 2019 05:57:38 -0400 Received: from mga18.intel.com ([134.134.136.126]:28708 "EHLO mga18.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730216AbfCYJ5i (ORCPT ); Mon, 25 Mar 2019 05:57:38 -0400 X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Mar 2019 02:57:37 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,256,1549958400"; d="scan'208";a="331732903" Received: from lahna.fi.intel.com (HELO lahna) ([10.237.72.157]) by fmsmga005.fm.intel.com with SMTP; 25 Mar 2019 02:57:34 -0700 Received: by lahna (sSMTP sendmail emulation); Mon, 25 Mar 2019 11:57:33 +0200 Date: Mon, 25 Mar 2019 11:57:33 +0200 From: Mika Westerberg To: Lukas Wunner Cc: linux-kernel@vger.kernel.org, Michael Jamet , Yehezkel Bernat , Andreas Noever , "David S . Miller" , Andy Shevchenko , netdev@vger.kernel.org Subject: Re: [PATCH v2 17/28] thunderbolt: Add support for full PCIe daisy chains Message-ID: <20190325095733.GC3622@lahna.fi.intel.com> References: <20190206131738.43696-1-mika.westerberg@linux.intel.com> <20190206131738.43696-18-mika.westerberg@linux.intel.com> <20190324113144.wubme46hby7rj6r2@wunner.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190324113144.wubme46hby7rj6r2@wunner.de> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo User-Agent: Mutt/1.11.3 (2019-02-01) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Mar 24, 2019 at 12:31:44PM +0100, Lukas Wunner wrote: > On Wed, Feb 06, 2019 at 04:17:27PM +0300, Mika Westerberg wrote: > > @@ -63,6 +71,16 @@ static void tb_discover_tunnels(struct tb_switch *sw) > > } > > } > > > > +static void tb_switch_authorize(struct work_struct *work) > > +{ > > + struct tb_switch *sw = container_of(work, typeof(*sw), work); > > + > > + mutex_lock(&sw->tb->lock); > > + if (!sw->is_unplugged) > > + tb_domain_approve_switch(sw->tb, sw); > > + mutex_unlock(&sw->tb->lock); > > +} > > + > > You're establishing PCI tunnels by having tb_scan_port() schedule > tb_switch_authorize() via a work item, which in turn calls > tb_domain_approve_switch(), which in turn calls tb_approve_switch(), > which in turn calls tb_tunnel_pci(). > > This seems kind of like a roundabout way of doing things, in particular > since all switches are hardcoded to be automatically authorized. > > Why don't you just invoke tb_tunnel_pci() directly from tb_scan_port()? Indeed, it does not make much sense to schedule separate work item just for this. I'm will remove it in v3. However, instead of always creating PCIe tunnels I'm going to propose that we implement the "user" security level in the software connection manager by default. While DMA protection relies on IOMMU, doing this allows user to turn off PCIe tunneling completely (or implement their own whitelisting of known good devices for example). > And why is the work item needed? I'm also confused that the work item > has been present in struct tb_switch for 2 years but is put to use only > now. Yes, you are right - the work item here is not needed. It is actually remnant from the original patch series. I'll cook a patch removing it. > > -static void tb_activate_pcie_devices(struct tb *tb) > > +static int tb_tunnel_pci(struct tb *tb, struct tb_switch *sw) > > { > [...] > > + /* > > + * Look up available down port. Since we are chaining, it is > > + * typically found right above this switch. > > + */ > > + down = NULL; > > + parent_sw = tb_to_switch(sw->dev.parent); > > + while (parent_sw) { > > + down = tb_find_unused_down_port(parent_sw); > > + if (down) > > + break; > > + parent_sw = tb_to_switch(parent_sw->dev.parent); > > + } > > The problem I see here is that there's no guarantee that the switch > on which you're selecting a down port is actually itself connected > with a PCI tunnel. E.g., allocation of a tunnel to that parent > switch may have failed. In that case you end up establishing a > tunnel between that parent switch and the newly connected switch > but the tunnel is of no use. Since this is going through tb_domain_approve_switch() it does not allow PCIe tunnel creation if the parent is not authorized first. > It would seem more logical to me to walk down the chain of newly > connected switches and try to establish a PCI tunnel to each of > them in order. By deferring tunnel establishment to a work item, > I think the tunnels may be established in an arbitrary order, right? The workqueue is ordered so AFAIK they should be run in the order the hotplug happened. In any case I'm going to remove the work item so this should not be an issue.