| Message ID | 20200615130139.83854-5-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 53f13319d13197dd1f4c8ce5fc1ef4c32509b4e2 |
| Headers | show |
| Series | thunderbolt: XDomain and NHI improvements | expand |
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?
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
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.
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.
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.
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.
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.
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.
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.
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).
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/
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);
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(-)