[PATCH 0/4] thunderbolt: XDomain and NHI improvements

[PATCH 0/4] thunderbolt: XDomain and NHI improvements

[Mika Westerberg]

Hi,

This small series improves the "data" path handling when doing host-to-host
connections over TBT/USB4 cable. First patch delays setting nodename upon
first connect to allow the userspace to fill in host name. Rest of the
series deal with the NHI (TBT/USB4 host interface) HopID allocation so that
by dropping the E2E workaround which was never used, we can use DMA rings
starting from 1 to transfer data over the TBT/USB4 fabric.

Mika Westerberg (4):
  thunderbolt: Build initial XDomain property block upon first connect
  thunderbolt: No need to warn if NHI hop_count != 12 or hop_count != 32
  thunderbolt: NHI can use HopIDs 1-7
  thunderbolt: Get rid of E2E workaround

 drivers/net/thunderbolt.c     |  4 +-
 drivers/thunderbolt/nhi.c     | 30 ++---------
 drivers/thunderbolt/switch.c  |  7 ++-
 drivers/thunderbolt/xdomain.c | 94 ++++++++++++++++++++---------------
 include/linux/thunderbolt.h   |  2 -
 5 files changed, 64 insertions(+), 73 deletions(-)

-- 
2.27.0.rc2

[PATCH 1/4] thunderbolt: Build initial XDomain property block upon first connect

[Mika Westerberg]

On a systems where the Thunderbolt controller is present all the time
the kernel nodename may not yet set by the userspace when the driver is
loaded. This means when another host is connected it may see the default
"(none)" hostname instead of the system real hostname.

For this reason build the initial XDomain property block only upon first
connect. This should make sure the userspace has had chance to set it up.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/xdomain.c | 94 ++++++++++++++++++++---------------
 1 file changed, 54 insertions(+), 40 deletions(-)

diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c
index 053f918e00e8..48907853732a 100644
--- a/drivers/thunderbolt/xdomain.c
+++ b/drivers/thunderbolt/xdomain.c
@@ -501,6 +501,55 @@ void tb_unregister_protocol_handler(struct tb_protocol_handler *handler)
 }
 EXPORT_SYMBOL_GPL(tb_unregister_protocol_handler);
 
+static int rebuild_property_block(void)
+{
+	u32 *block, len;
+	int ret;
+
+	ret = tb_property_format_dir(xdomain_property_dir, NULL, 0);
+	if (ret < 0)
+		return ret;
+
+	len = ret;
+
+	block = kcalloc(len, sizeof(u32), GFP_KERNEL);
+	if (!block)
+		return -ENOMEM;
+
+	ret = tb_property_format_dir(xdomain_property_dir, block, len);
+	if (ret) {
+		kfree(block);
+		return ret;
+	}
+
+	kfree(xdomain_property_block);
+	xdomain_property_block = block;
+	xdomain_property_block_len = len;
+	xdomain_property_block_gen++;
+
+	return 0;
+}
+
+static void finalize_property_block(void)
+{
+	const struct tb_property *nodename;
+
+	/*
+	 * On first XDomain connection we set up the the system
+	 * nodename. This delayed here because userspace may not have it
+	 * set when the driver is first probed.
+	 */
+	mutex_lock(&xdomain_lock);
+	nodename = tb_property_find(xdomain_property_dir, "deviceid",
+				    TB_PROPERTY_TYPE_TEXT);
+	if (!nodename) {
+		tb_property_add_text(xdomain_property_dir, "deviceid",
+				     utsname()->nodename);
+		rebuild_property_block();
+	}
+	mutex_unlock(&xdomain_lock);
+}
+
 static void tb_xdp_handle_request(struct work_struct *work)
 {
 	struct xdomain_request_work *xw = container_of(work, typeof(*xw), work);
@@ -529,6 +578,8 @@ static void tb_xdp_handle_request(struct work_struct *work)
 		goto out;
 	}
 
+	finalize_property_block();
+
 	switch (pkg->type) {
 	case PROPERTIES_REQUEST:
 		ret = tb_xdp_properties_response(tb, ctl, route, sequence, uuid,
@@ -1569,35 +1620,6 @@ bool tb_xdomain_handle_request(struct tb *tb, enum tb_cfg_pkg_type type,
 	return ret > 0;
 }
 
-static int rebuild_property_block(void)
-{
-	u32 *block, len;
-	int ret;
-
-	ret = tb_property_format_dir(xdomain_property_dir, NULL, 0);
-	if (ret < 0)
-		return ret;
-
-	len = ret;
-
-	block = kcalloc(len, sizeof(u32), GFP_KERNEL);
-	if (!block)
-		return -ENOMEM;
-
-	ret = tb_property_format_dir(xdomain_property_dir, block, len);
-	if (ret) {
-		kfree(block);
-		return ret;
-	}
-
-	kfree(xdomain_property_block);
-	xdomain_property_block = block;
-	xdomain_property_block_len = len;
-	xdomain_property_block_gen++;
-
-	return 0;
-}
-
 static int update_xdomain(struct device *dev, void *data)
 {
 	struct tb_xdomain *xd;
@@ -1702,8 +1724,6 @@ EXPORT_SYMBOL_GPL(tb_unregister_property_dir);
 
 int tb_xdomain_init(void)
 {
-	int ret;
-
 	xdomain_property_dir = tb_property_create_dir(NULL);
 	if (!xdomain_property_dir)
 		return -ENOMEM;
@@ -1712,22 +1732,16 @@ int tb_xdomain_init(void)
 	 * Initialize standard set of properties without any service
 	 * directories. Those will be added by service drivers
 	 * themselves when they are loaded.
+	 *
+	 * We also add node name later when first connection is made.
 	 */
 	tb_property_add_immediate(xdomain_property_dir, "vendorid",
 				  PCI_VENDOR_ID_INTEL);
 	tb_property_add_text(xdomain_property_dir, "vendorid", "Intel Corp.");
 	tb_property_add_immediate(xdomain_property_dir, "deviceid", 0x1);
-	tb_property_add_text(xdomain_property_dir, "deviceid",
-			     utsname()->nodename);
 	tb_property_add_immediate(xdomain_property_dir, "devicerv", 0x80000100);
 
-	ret = rebuild_property_block();
-	if (ret) {
-		tb_property_free_dir(xdomain_property_dir);
-		xdomain_property_dir = NULL;
-	}
-
-	return ret;
+	return 0;
 }
 
 void tb_xdomain_exit(void)
-- 
2.27.0.rc2

[PATCH 2/4] thunderbolt: No need to warn if NHI hop_count != 12 or hop_count != 32

[Mika Westerberg]

While Intel hardware typically has hop_count (Total Paths in the spec)
12 the USB4 spec allows this to be anything between 1 and 21 so no need
to warn about this. Simply log number of paths at debug level.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/nhi.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index d299dc168147..b617922b5b0a 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -1123,9 +1123,7 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	/* cannot fail - table is allocated bin pcim_iomap_regions */
 	nhi->iobase = pcim_iomap_table(pdev)[0];
 	nhi->hop_count = ioread32(nhi->iobase + REG_HOP_COUNT) & 0x3ff;
-	if (nhi->hop_count != 12 && nhi->hop_count != 32)
-		dev_warn(&pdev->dev, "unexpected hop count: %d\n",
-			 nhi->hop_count);
+	dev_dbg(&pdev->dev, "total paths: %d\n", nhi->hop_count);
 
 	nhi->tx_rings = devm_kcalloc(&pdev->dev, nhi->hop_count,
 				     sizeof(*nhi->tx_rings), GFP_KERNEL);
-- 
2.27.0.rc2

[PATCH 3/4] thunderbolt: NHI can use HopIDs 1-7

[Mika Westerberg]

NHI (The host interface adapter) is allowed to use HopIDs 1-7 as well so
relax the restriction in tb_port_alloc_hopid() to support this.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/switch.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index d7d60cd9226f..95b75a712ade 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -789,8 +789,11 @@ static int tb_port_alloc_hopid(struct tb_port *port, bool in, int min_hopid,
 		ida = &port->out_hopids;
 	}
 
-	/* HopIDs 0-7 are reserved */
-	if (min_hopid < TB_PATH_MIN_HOPID)
+	/*
+	 * NHI can use HopIDs 1-max for other adapters HopIDs 0-7 are
+	 * reserved.
+	 */
+	if (port->config.type != TB_TYPE_NHI && min_hopid < TB_PATH_MIN_HOPID)
 		min_hopid = TB_PATH_MIN_HOPID;
 
 	if (max_hopid < 0 || max_hopid > port_max_hopid)
-- 
2.27.0.rc2

[PATCH 4/4] thunderbolt: Get rid of E2E workaround

[Mika Westerberg]

The end-to-end (E2E) workaround is needed for Falcon Ridge (TBT 2)
controller when E2E is enabled for both ends of the host-to-host
connection. However, we never supported full E2E in the first place so
this code is not necessary at the moment. Further this allows us to use
all available rings for data except ring 0 which is reserved for the
control path.

The complete E2E flow control is explained in the USB4 spec so we may
add it back later if needed but at least the networking driver seems to
work fine without, and the higher level stack, like TCP will retransmit
lost packets anyway.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/net/thunderbolt.c   |  4 ++--
 drivers/thunderbolt/nhi.c   | 26 ++------------------------
 include/linux/thunderbolt.h |  2 --
 3 files changed, 4 insertions(+), 28 deletions(-)

diff --git a/drivers/net/thunderbolt.c b/drivers/net/thunderbolt.c
index dacb4f680fd4..a812726703a4 100644
--- a/drivers/net/thunderbolt.c
+++ b/drivers/net/thunderbolt.c
@@ -866,8 +866,8 @@ static int tbnet_open(struct net_device *dev)
 	eof_mask = BIT(TBIP_PDF_FRAME_END);
 
 	ring = tb_ring_alloc_rx(xd->tb->nhi, -1, TBNET_RING_SIZE,
-				RING_FLAG_FRAME | RING_FLAG_E2E, sof_mask,
-				eof_mask, tbnet_start_poll, net);
+				RING_FLAG_FRAME, sof_mask, eof_mask,
+				tbnet_start_poll, net);
 	if (!ring) {
 		netdev_err(dev, "failed to allocate Rx ring\n");
 		tb_ring_free(net->tx_ring.ring);
diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index b617922b5b0a..5f7489fa1327 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -24,12 +24,7 @@
 
 #define RING_TYPE(ring) ((ring)->is_tx ? "TX ring" : "RX ring")
 
-/*
- * Used to enable end-to-end workaround for missing RX packets. Do not
- * use this ring for anything else.
- */
-#define RING_E2E_UNUSED_HOPID	2
-#define RING_FIRST_USABLE_HOPID	TB_PATH_MIN_HOPID
+#define RING_FIRST_USABLE_HOPID	1
 
 /*
  * Minimal number of vectors when we use MSI-X. Two for control channel
@@ -440,7 +435,7 @@ static int nhi_alloc_hop(struct tb_nhi *nhi, struct tb_ring *ring)
 
 		/*
 		 * Automatically allocate HopID from the non-reserved
-		 * range 8 .. hop_count - 1.
+		 * range 1 .. hop_count - 1.
 		 */
 		for (i = RING_FIRST_USABLE_HOPID; i < nhi->hop_count; i++) {
 			if (ring->is_tx) {
@@ -496,10 +491,6 @@ static struct tb_ring *tb_ring_alloc(struct tb_nhi *nhi, u32 hop, int size,
 	dev_dbg(&nhi->pdev->dev, "allocating %s ring %d of size %d\n",
 		transmit ? "TX" : "RX", hop, size);
 
-	/* Tx Ring 2 is reserved for E2E workaround */
-	if (transmit && hop == RING_E2E_UNUSED_HOPID)
-		return NULL;
-
 	ring = kzalloc(sizeof(*ring), GFP_KERNEL);
 	if (!ring)
 		return NULL;
@@ -614,19 +605,6 @@ void tb_ring_start(struct tb_ring *ring)
 		flags = RING_FLAG_ENABLE | RING_FLAG_RAW;
 	}
 
-	if (ring->flags & RING_FLAG_E2E && !ring->is_tx) {
-		u32 hop;
-
-		/*
-		 * In order not to lose Rx packets we enable end-to-end
-		 * workaround which transfers Rx credits to an unused Tx
-		 * HopID.
-		 */
-		hop = RING_E2E_UNUSED_HOPID << REG_RX_OPTIONS_E2E_HOP_SHIFT;
-		hop &= REG_RX_OPTIONS_E2E_HOP_MASK;
-		flags |= hop | RING_FLAG_E2E_FLOW_CONTROL;
-	}
-
 	ring_iowrite64desc(ring, ring->descriptors_dma, 0);
 	if (ring->is_tx) {
 		ring_iowrite32desc(ring, ring->size, 12);
diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h
index ff397c0d5c07..5db2b11ab085 100644
--- a/include/linux/thunderbolt.h
+++ b/include/linux/thunderbolt.h
@@ -504,8 +504,6 @@ struct tb_ring {
 #define RING_FLAG_NO_SUSPEND	BIT(0)
 /* Configure the ring to be in frame mode */
 #define RING_FLAG_FRAME		BIT(1)
-/* Enable end-to-end flow control */
-#define RING_FLAG_E2E		BIT(2)
 
 struct ring_frame;
 typedef void (*ring_cb)(struct tb_ring *, struct ring_frame *, bool canceled);
-- 
2.27.0.rc2

Re: [PATCH 4/4] thunderbolt: Get rid of E2E workaround

[Yehezkel Bernat]

On Mon, Jun 15, 2020 at 4:02 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h
> index ff397c0d5c07..5db2b11ab085 100644
> --- a/include/linux/thunderbolt.h
> +++ b/include/linux/thunderbolt.h
> @@ -504,8 +504,6 @@ struct tb_ring {
>  #define RING_FLAG_NO_SUSPEND   BIT(0)
>  /* Configure the ring to be in frame mode */
>  #define RING_FLAG_FRAME                BIT(1)
> -/* Enable end-to-end flow control */
> -#define RING_FLAG_E2E          BIT(2)
>

Isn't it better to keep it (or mark it as reserved) so it'll not cause
compatibility issues with older versions of the driver or with Windows?

Re: [PATCH 4/4] thunderbolt: Get rid of E2E workaround

[Greg Kroah-Hartman]

On Mon, Jun 15, 2020 at 04:45:22PM +0300, Yehezkel Bernat wrote:
> On Mon, Jun 15, 2020 at 4:02 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h
> > index ff397c0d5c07..5db2b11ab085 100644
> > --- a/include/linux/thunderbolt.h
> > +++ b/include/linux/thunderbolt.h
> > @@ -504,8 +504,6 @@ struct tb_ring {
> >  #define RING_FLAG_NO_SUSPEND   BIT(0)
> >  /* Configure the ring to be in frame mode */
> >  #define RING_FLAG_FRAME                BIT(1)
> > -/* Enable end-to-end flow control */
> > -#define RING_FLAG_E2E          BIT(2)
> >
> 
> Isn't it better to keep it (or mark it as reserved) so it'll not cause
> compatibility issues with older versions of the driver or with Windows?


How can you have "older versions of the driver"?  All drivers are in the
kernel tree at the same time, you can't ever mix-and-match drivers and
kernels.

And how does Windows come into play here?

thanks,

greg k-h

Re: [PATCH 4/4] thunderbolt: Get rid of E2E workaround

[Yehezkel Bernat]

On Mon, Jun 15, 2020 at 4:51 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jun 15, 2020 at 04:45:22PM +0300, Yehezkel Bernat wrote:
> > On Mon, Jun 15, 2020 at 4:02 PM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > >
> > > diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h
> > > index ff397c0d5c07..5db2b11ab085 100644
> > > --- a/include/linux/thunderbolt.h
> > > +++ b/include/linux/thunderbolt.h
> > > @@ -504,8 +504,6 @@ struct tb_ring {
> > >  #define RING_FLAG_NO_SUSPEND   BIT(0)
> > >  /* Configure the ring to be in frame mode */
> > >  #define RING_FLAG_FRAME                BIT(1)
> > > -/* Enable end-to-end flow control */
> > > -#define RING_FLAG_E2E          BIT(2)
> > >
> >
> > Isn't it better to keep it (or mark it as reserved) so it'll not cause
> > compatibility issues with older versions of the driver or with Windows?
>
>
> How can you have "older versions of the driver"?  All drivers are in the
> kernel tree at the same time, you can't ever mix-and-match drivers and
> kernels.
>
> And how does Windows come into play here?
>

As much as I remember, this flag is sent as part of creating the
interdomain connection.
If we reuse this bit to something else, and the other host runs an
older kernel or
Windows, this seems to be an issue.
But maybe I don't remember it correctly.

Re: [PATCH 4/4] thunderbolt: Get rid of E2E workaround

[Mika Westerberg]

On Mon, Jun 15, 2020 at 05:18:38PM +0300, Yehezkel Bernat wrote:
> On Mon, Jun 15, 2020 at 4:51 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Jun 15, 2020 at 04:45:22PM +0300, Yehezkel Bernat wrote:
> > > On Mon, Jun 15, 2020 at 4:02 PM Mika Westerberg
> > > <mika.westerberg@linux.intel.com> wrote:
> > > >
> > > > diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h
> > > > index ff397c0d5c07..5db2b11ab085 100644
> > > > --- a/include/linux/thunderbolt.h
> > > > +++ b/include/linux/thunderbolt.h
> > > > @@ -504,8 +504,6 @@ struct tb_ring {
> > > >  #define RING_FLAG_NO_SUSPEND   BIT(0)
> > > >  /* Configure the ring to be in frame mode */
> > > >  #define RING_FLAG_FRAME                BIT(1)
> > > > -/* Enable end-to-end flow control */
> > > > -#define RING_FLAG_E2E          BIT(2)
> > > >
> > >
> > > Isn't it better to keep it (or mark it as reserved) so it'll not cause
> > > compatibility issues with older versions of the driver or with Windows?
> >
> >
> > How can you have "older versions of the driver"?  All drivers are in the
> > kernel tree at the same time, you can't ever mix-and-match drivers and
> > kernels.
> >
> > And how does Windows come into play here?
> >
> 
> As much as I remember, this flag is sent as part of creating the
> interdomain connection.
> If we reuse this bit to something else, and the other host runs an
> older kernel or
> Windows, this seems to be an issue.
> But maybe I don't remember it correctly.

We never send this flag anywhere. At the moment we do not announce
support the "full E2E" in the network driver. Basically this is dead
code what we remove.

Re: [PATCH 4/4] thunderbolt: Get rid of E2E workaround

[Yehezkel Bernat]

On Mon, Jun 15, 2020 at 5:22 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Mon, Jun 15, 2020 at 05:18:38PM +0300, Yehezkel Bernat wrote:
> > On Mon, Jun 15, 2020 at 4:51 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Mon, Jun 15, 2020 at 04:45:22PM +0300, Yehezkel Bernat wrote:
> > > > On Mon, Jun 15, 2020 at 4:02 PM Mika Westerberg
> > > > <mika.westerberg@linux.intel.com> wrote:
> > > > >
> > > > > diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h
> > > > > index ff397c0d5c07..5db2b11ab085 100644
> > > > > --- a/include/linux/thunderbolt.h
> > > > > +++ b/include/linux/thunderbolt.h
> > > > > @@ -504,8 +504,6 @@ struct tb_ring {
> > > > >  #define RING_FLAG_NO_SUSPEND   BIT(0)
> > > > >  /* Configure the ring to be in frame mode */
> > > > >  #define RING_FLAG_FRAME                BIT(1)
> > > > > -/* Enable end-to-end flow control */
> > > > > -#define RING_FLAG_E2E          BIT(2)
> > > > >
> > > >
> > > > Isn't it better to keep it (or mark it as reserved) so it'll not cause
> > > > compatibility issues with older versions of the driver or with Windows?
> > >
> > >
> > > How can you have "older versions of the driver"?  All drivers are in the
> > > kernel tree at the same time, you can't ever mix-and-match drivers and
> > > kernels.
> > >
> > > And how does Windows come into play here?
> > >
> >
> > As much as I remember, this flag is sent as part of creating the
> > interdomain connection.
> > If we reuse this bit to something else, and the other host runs an
> > older kernel or
> > Windows, this seems to be an issue.
> > But maybe I don't remember it correctly.
>
> We never send this flag anywhere. At the moment we do not announce
> support the "full E2E" in the network driver. Basically this is dead
> code what we remove.

OK, maybe we never sent it, but Windows driver does send such a flag somewhere.
This is the only way both sides can know both of them support it and that they
should start using it. I'd prefer at least leaving a comment here that mentions
this, so if someone goes to add flags in the future, they will know to
take it into consideration.

Re: [PATCH 4/4] thunderbolt: Get rid of E2E workaround

[Mika Westerberg]

On Mon, Jun 15, 2020 at 06:15:47PM +0300, Yehezkel Bernat wrote:
> On Mon, Jun 15, 2020 at 5:22 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > On Mon, Jun 15, 2020 at 05:18:38PM +0300, Yehezkel Bernat wrote:
> > > On Mon, Jun 15, 2020 at 4:51 PM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Mon, Jun 15, 2020 at 04:45:22PM +0300, Yehezkel Bernat wrote:
> > > > > On Mon, Jun 15, 2020 at 4:02 PM Mika Westerberg
> > > > > <mika.westerberg@linux.intel.com> wrote:
> > > > > >
> > > > > > diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h
> > > > > > index ff397c0d5c07..5db2b11ab085 100644
> > > > > > --- a/include/linux/thunderbolt.h
> > > > > > +++ b/include/linux/thunderbolt.h
> > > > > > @@ -504,8 +504,6 @@ struct tb_ring {
> > > > > >  #define RING_FLAG_NO_SUSPEND   BIT(0)
> > > > > >  /* Configure the ring to be in frame mode */
> > > > > >  #define RING_FLAG_FRAME                BIT(1)
> > > > > > -/* Enable end-to-end flow control */
> > > > > > -#define RING_FLAG_E2E          BIT(2)
> > > > > >
> > > > >
> > > > > Isn't it better to keep it (or mark it as reserved) so it'll not cause
> > > > > compatibility issues with older versions of the driver or with Windows?
> > > >
> > > >
> > > > How can you have "older versions of the driver"?  All drivers are in the
> > > > kernel tree at the same time, you can't ever mix-and-match drivers and
> > > > kernels.
> > > >
> > > > And how does Windows come into play here?
> > > >
> > >
> > > As much as I remember, this flag is sent as part of creating the
> > > interdomain connection.
> > > If we reuse this bit to something else, and the other host runs an
> > > older kernel or
> > > Windows, this seems to be an issue.
> > > But maybe I don't remember it correctly.
> >
> > We never send this flag anywhere. At the moment we do not announce
> > support the "full E2E" in the network driver. Basically this is dead
> > code what we remove.
> 
> OK, maybe we never sent it, but Windows driver does send such a flag somewhere.

It does yes but that is optional and we chose not to support it in
Linux TBT network driver.

> This is the only way both sides can know both of them support it and that they
> should start using it. I'd prefer at least leaving a comment here that mentions
> this, so if someone goes to add flags in the future, they will know to
> take it into consideration.

This flag here (RING_FLAG_E2E) is not exposed anywhere outside of
thunderbolt driver.

I think you are talking about the "prtstns" property in the network
driver. There we only set TBNET_MATCH_FRAGS_ID (bit 1). This is the
thing that get exposed to the other side of the connection and we never
announced support for full E2E.

Re: [PATCH 4/4] thunderbolt: Get rid of E2E workaround

[Yehezkel Bernat]

On Mon, Jun 15, 2020 at 6:32 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Mon, Jun 15, 2020 at 06:15:47PM +0300, Yehezkel Bernat wrote:
> > On Mon, Jun 15, 2020 at 5:22 PM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > >
> > > On Mon, Jun 15, 2020 at 05:18:38PM +0300, Yehezkel Bernat wrote:
> > > > On Mon, Jun 15, 2020 at 4:51 PM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Mon, Jun 15, 2020 at 04:45:22PM +0300, Yehezkel Bernat wrote:
> > > > > > On Mon, Jun 15, 2020 at 4:02 PM Mika Westerberg
> > > > > > <mika.westerberg@linux.intel.com> wrote:
> > > > > > >
> > > > > > > diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h
> > > > > > > index ff397c0d5c07..5db2b11ab085 100644
> > > > > > > --- a/include/linux/thunderbolt.h
> > > > > > > +++ b/include/linux/thunderbolt.h
> > > > > > > @@ -504,8 +504,6 @@ struct tb_ring {
> > > > > > >  #define RING_FLAG_NO_SUSPEND   BIT(0)
> > > > > > >  /* Configure the ring to be in frame mode */
> > > > > > >  #define RING_FLAG_FRAME                BIT(1)
> > > > > > > -/* Enable end-to-end flow control */
> > > > > > > -#define RING_FLAG_E2E          BIT(2)
> > > > > > >
> > > > > >
> > > > > > Isn't it better to keep it (or mark it as reserved) so it'll not cause
> > > > > > compatibility issues with older versions of the driver or with Windows?
> > > > >
> > > > >
> > > > > How can you have "older versions of the driver"?  All drivers are in the
> > > > > kernel tree at the same time, you can't ever mix-and-match drivers and
> > > > > kernels.
> > > > >
> > > > > And how does Windows come into play here?
> > > > >
> > > >
> > > > As much as I remember, this flag is sent as part of creating the
> > > > interdomain connection.
> > > > If we reuse this bit to something else, and the other host runs an
> > > > older kernel or
> > > > Windows, this seems to be an issue.
> > > > But maybe I don't remember it correctly.
> > >
> > > We never send this flag anywhere. At the moment we do not announce
> > > support the "full E2E" in the network driver. Basically this is dead
> > > code what we remove.
> >
> > OK, maybe we never sent it, but Windows driver does send such a flag somewhere.
>
> It does yes but that is optional and we chose not to support it in
> Linux TBT network driver.
>
> > This is the only way both sides can know both of them support it and that they
> > should start using it. I'd prefer at least leaving a comment here that mentions
> > this, so if someone goes to add flags in the future, they will know to
> > take it into consideration.
>
> This flag here (RING_FLAG_E2E) is not exposed anywhere outside of
> thunderbolt driver.
>
> I think you are talking about the "prtstns" property in the network
> driver. There we only set TBNET_MATCH_FRAGS_ID (bit 1). This is the
> thing that get exposed to the other side of the connection and we never
> announced support for full E2E.


Ah, yes, this one, Thanks!
As Windows driver uses it for flagging full-E2E, and we completely drop E2E
support here, it may worth to mention there that this is what bit 2 is used in
Windows so any reuse should consider the possible compatibility issue.

Re: [PATCH 4/4] thunderbolt: Get rid of E2E workaround

[Mika Westerberg]

On Mon, Jun 15, 2020 at 06:41:32PM +0300, Yehezkel Bernat wrote:
> > I think you are talking about the "prtstns" property in the network
> > driver. There we only set TBNET_MATCH_FRAGS_ID (bit 1). This is the
> > thing that get exposed to the other side of the connection and we never
> > announced support for full E2E.
> 
> 
> Ah, yes, this one, Thanks!
> As Windows driver uses it for flagging full-E2E, and we completely drop E2E
> support here, it may worth to mention there that this is what bit 2 is used in
> Windows so any reuse should consider the possible compatibility issue.

Note we only drop dead code in this patch. It is that workaround for
Falcon Ridge controller we actually never used.

I can add a comment to the network driver about the full E2E support
flag as a separate patch if you think it is useful.

The network protocol will be public soon I guess because USB4 spec
refers to "USB4 Inter-Domain Specification, Revision 1.0, [to be
published] – (USB4 Inter-Domain Specification)" so I would expect it to
be explained there as well.

Re: [PATCH 4/4] thunderbolt: Get rid of E2E workaround

[Yehezkel Bernat]

On Mon, Jun 15, 2020 at 6:55 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Mon, Jun 15, 2020 at 06:41:32PM +0300, Yehezkel Bernat wrote:
> > > I think you are talking about the "prtstns" property in the network
> > > driver. There we only set TBNET_MATCH_FRAGS_ID (bit 1). This is the
> > > thing that get exposed to the other side of the connection and we never
> > > announced support for full E2E.
> >
> >
> > Ah, yes, this one, Thanks!
> > As Windows driver uses it for flagging full-E2E, and we completely drop E2E
> > support here, it may worth to mention there that this is what bit 2 is used in
> > Windows so any reuse should consider the possible compatibility issue.
>
> Note we only drop dead code in this patch. It is that workaround for
> Falcon Ridge controller we actually never used.
>
> I can add a comment to the network driver about the full E2E support
> flag as a separate patch if you think it is useful.
>
> The network protocol will be public soon I guess because USB4 spec
> refers to "USB4 Inter-Domain Specification, Revision 1.0, [to be
> published] – (USB4 Inter-Domain Specification)" so I would expect it to
> be explained there as well.

I see. I leave it for your decision, then.
Thanks for bearing with me.

Re: [PATCH 4/4] thunderbolt: Get rid of E2E workaround

[Mika Westerberg]

On Mon, Jun 15, 2020 at 10:54:52PM +0300, Yehezkel Bernat wrote:
> On Mon, Jun 15, 2020 at 6:55 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > On Mon, Jun 15, 2020 at 06:41:32PM +0300, Yehezkel Bernat wrote:
> > > > I think you are talking about the "prtstns" property in the network
> > > > driver. There we only set TBNET_MATCH_FRAGS_ID (bit 1). This is the
> > > > thing that get exposed to the other side of the connection and we never
> > > > announced support for full E2E.
> > >
> > >
> > > Ah, yes, this one, Thanks!
> > > As Windows driver uses it for flagging full-E2E, and we completely drop E2E
> > > support here, it may worth to mention there that this is what bit 2 is used in
> > > Windows so any reuse should consider the possible compatibility issue.
> >
> > Note we only drop dead code in this patch. It is that workaround for
> > Falcon Ridge controller we actually never used.
> >
> > I can add a comment to the network driver about the full E2E support
> > flag as a separate patch if you think it is useful.
> >
> > The network protocol will be public soon I guess because USB4 spec
> > refers to "USB4 Inter-Domain Specification, Revision 1.0, [to be
> > published] – (USB4 Inter-Domain Specification)" so I would expect it to
> > be explained there as well.
> 
> I see. I leave it for your decision, then.
> Thanks for bearing with me.

OK, I think it makes sense to add the comment so I'll do that as
a separate patch (will probably go next week since I have some other
patches to deal with this week, and Friday is holiday in Finland).

Re: [PATCH 4/4] thunderbolt: Get rid of E2E workaround

[Mika Westerberg]

On Tue, Jun 16, 2020 at 02:55:25PM +0300, Mika Westerberg wrote:
> On Mon, Jun 15, 2020 at 10:54:52PM +0300, Yehezkel Bernat wrote:
> > On Mon, Jun 15, 2020 at 6:55 PM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > >
> > > On Mon, Jun 15, 2020 at 06:41:32PM +0300, Yehezkel Bernat wrote:
> > > > > I think you are talking about the "prtstns" property in the network
> > > > > driver. There we only set TBNET_MATCH_FRAGS_ID (bit 1). This is the
> > > > > thing that get exposed to the other side of the connection and we never
> > > > > announced support for full E2E.
> > > >
> > > >
> > > > Ah, yes, this one, Thanks!
> > > > As Windows driver uses it for flagging full-E2E, and we completely drop E2E
> > > > support here, it may worth to mention there that this is what bit 2 is used in
> > > > Windows so any reuse should consider the possible compatibility issue.
> > >
> > > Note we only drop dead code in this patch. It is that workaround for
> > > Falcon Ridge controller we actually never used.
> > >
> > > I can add a comment to the network driver about the full E2E support
> > > flag as a separate patch if you think it is useful.
> > >
> > > The network protocol will be public soon I guess because USB4 spec
> > > refers to "USB4 Inter-Domain Specification, Revision 1.0, [to be
> > > published] – (USB4 Inter-Domain Specification)" so I would expect it to
> > > be explained there as well.
> > 
> > I see. I leave it for your decision, then.
> > Thanks for bearing with me.
> 
> OK, I think it makes sense to add the comment so I'll do that as
> a separate patch (will probably go next week since I have some other
> patches to deal with this week, and Friday is holiday in Finland).

OK, I sent it now and can be found here:

  https://lore.kernel.org/netdev/20200622163022.53298-1-mika.westerberg@linux.intel.com/

Re: [PATCH 0/4] thunderbolt: XDomain and NHI improvements

[Mika Westerberg]

On Mon, Jun 15, 2020 at 04:01:35PM +0300, Mika Westerberg wrote:
> Hi,
> 
> This small series improves the "data" path handling when doing host-to-host
> connections over TBT/USB4 cable. First patch delays setting nodename upon
> first connect to allow the userspace to fill in host name. Rest of the
> series deal with the NHI (TBT/USB4 host interface) HopID allocation so that
> by dropping the E2E workaround which was never used, we can use DMA rings
> starting from 1 to transfer data over the TBT/USB4 fabric.
> 
> Mika Westerberg (4):
>   thunderbolt: Build initial XDomain property block upon first connect
>   thunderbolt: No need to warn if NHI hop_count != 12 or hop_count != 32
>   thunderbolt: NHI can use HopIDs 1-7
>   thunderbolt: Get rid of E2E workaround
> 
>  drivers/net/thunderbolt.c     |  4 +-
>  drivers/thunderbolt/nhi.c     | 30 ++---------
>  drivers/thunderbolt/switch.c  |  7 ++-
>  drivers/thunderbolt/xdomain.c | 94 ++++++++++++++++++++---------------
>  include/linux/thunderbolt.h   |  2 -
>  5 files changed, 64 insertions(+), 73 deletions(-)

Queued these for v5.9.