Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1451789 > unrolled thread

[PATCH v5 0/8] thunderbolt: Introducing Thunderbolt(TM) networking

Back to article view | Back to linux.kernel

Contents

  [PATCH v5 0/8] thunderbolt: Introducing Thunderbolt(TM) networking Amir Levy <amir.jer.levy@intel.com> - 2016-07-28 10:20 +0200
    [PATCH v5 2/8] thunderbolt: Updating the register definitions Amir Levy <amir.jer.levy@intel.com> - 2016-07-28 10:20 +0200
    [PATCH v5 8/8] thunderbolt: Adding maintainer entry Amir Levy <amir.jer.levy@intel.com> - 2016-07-28 10:20 +0200
    [PATCH v5 7/8] thunderbolt: Networking doc Amir Levy <amir.jer.levy@intel.com> - 2016-07-28 10:20 +0200
    Re: [PATCH v5 5/8] thunderbolt: Networking state machine Lukas Wunner <lukas@wunner.de> - 2016-07-28 13:40 +0200
      RE: [PATCH v5 5/8] thunderbolt: Networking state machine "Levy, Amir (Jer)" <amir.jer.levy@intel.com> - 2016-07-31 14:00 +0200
    Re: [PATCH v5 4/8] thunderbolt: Communication with the ICM  (firmware) Stephen Hemminger <stephen@networkplumber.org> - 2016-07-29 23:10 +0200
      Re: [PATCH v5 4/8] thunderbolt: Communication with the ICM (firmware) Greg KH <gregkh@linuxfoundation.org> - 2016-07-29 23:50 +0200
        RE: [PATCH v5 4/8] thunderbolt: Communication with the ICM  (firmware) "Levy, Amir (Jer)" <amir.jer.levy@intel.com> - 2016-07-31 12:20 +0200
    Re: [PATCH v5 4/8] thunderbolt: Communication with the ICM  (firmware) Stephen Hemminger <stephen@networkplumber.org> - 2016-07-29 23:10 +0200
      RE: [PATCH v5 4/8] thunderbolt: Communication with the ICM  (firmware) "Levy, Amir (Jer)" <amir.jer.levy@intel.com> - 2016-07-31 14:10 +0200
    Re: [PATCH v5 6/8] thunderbolt: Networking transmit and receive Stephen Hemminger <stephen@networkplumber.org> - 2016-07-29 23:20 +0200
      RE: [PATCH v5 6/8] thunderbolt: Networking transmit and receive "Levy, Amir (Jer)" <amir.jer.levy@intel.com> - 2016-07-31 12:20 +0200
        Re: [PATCH v5 6/8] thunderbolt: Networking transmit and receive David Miller <davem@davemloft.net> - 2016-07-31 20:50 +0200
          RE: [PATCH v5 6/8] thunderbolt: Networking transmit and receive "Levy, Amir (Jer)" <amir.jer.levy@intel.com> - 2016-08-01 06:40 +0200

#1451789 — [PATCH v5 0/8] thunderbolt: Introducing Thunderbolt(TM) networking

This is version 5 of Thunderbolt(TM) driver for non-Apple hardware.

Changes since v4:
 - Added Amir Levy as maintainer of thunderbolt/icm
 - Replaced private uuid definitions with uuid_be

These patches were pushed to GitHub where they can be reviewed more
comfortably with green/red highlighting:
	https://github.com/01org/thunderbolt-software-kernel-tree

Daemon code:
	https://github.com/01org/thunderbolt-software-daemon

For reference, here's a link to version 4:
[v4]:	https://lkml.org/lkml/2016/7/18/171

Amir Levy (8):
  thunderbolt: Macro rename
  thunderbolt: Updating the register definitions
  thunderbolt: Kconfig for Thunderbolt(TM) networking
  thunderbolt: Communication with the ICM (firmware)
  thunderbolt: Networking state machine
  thunderbolt: Networking transmit and receive
  thunderbolt: Networking doc
  thunderbolt: Adding maintainer entry

 Documentation/00-INDEX                   |    2 +
 Documentation/thunderbolt-networking.txt |  135 ++
 MAINTAINERS                              |    8 +-
 drivers/thunderbolt/Kconfig              |   25 +-
 drivers/thunderbolt/Makefile             |    3 +-
 drivers/thunderbolt/icm/Makefile         |   28 +
 drivers/thunderbolt/icm/icm_nhi.c        | 1641 +++++++++++++++++++++
 drivers/thunderbolt/icm/icm_nhi.h        |   93 ++
 drivers/thunderbolt/icm/net.c            | 2267 ++++++++++++++++++++++++++++++
 drivers/thunderbolt/icm/net.h            |  270 ++++
 drivers/thunderbolt/nhi_regs.h           |  115 +-
 11 files changed, 4578 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/thunderbolt-networking.txt
 create mode 100644 drivers/thunderbolt/icm/Makefile
 create mode 100644 drivers/thunderbolt/icm/icm_nhi.c
 create mode 100644 drivers/thunderbolt/icm/icm_nhi.h
 create mode 100644 drivers/thunderbolt/icm/net.c
 create mode 100644 drivers/thunderbolt/icm/net.h

-- 
2.7.4

[toc] | [next] | [standalone]

#1451791 — [PATCH v5 2/8] thunderbolt: Updating the register definitions

Adding more Thunderbolt(TM) register definitions
and some helper macros.

Signed-off-by: Amir Levy <amir.jer.levy@intel.com>
---
 drivers/thunderbolt/nhi_regs.h | 109 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 109 insertions(+)

diff --git a/drivers/thunderbolt/nhi_regs.h b/drivers/thunderbolt/nhi_regs.h
index 75cf069..b8e961f 100644
--- a/drivers/thunderbolt/nhi_regs.h
+++ b/drivers/thunderbolt/nhi_regs.h
@@ -9,6 +9,11 @@
 
 #include <linux/types.h>
 
+#define NHI_MMIO_BAR 0
+
+#define TBT_RING_MIN_NUM_BUFFERS	2
+#define TBT_RING_MAX_FRAME_SIZE		(4 * 1024)
+
 enum ring_flags {
 	RING_FLAG_ISOCH_ENABLE = 1 << 27, /* TX only? */
 	RING_FLAG_E2E_FLOW_CONTROL = 1 << 28,
@@ -39,6 +44,33 @@ struct ring_desc {
 	u32 time; /* write zero */
 } __packed;
 
+/**
+ * struct tbt_buf_desc - TX/RX ring buffer descriptor.
+ * This is same as struct ring_desc, but without the use of bitfields and
+ * with explicit endianity.
+ */
+struct tbt_buf_desc {
+	__le64 phys;
+	__le32 attributes;
+	__le32 time;
+};
+
+#define DESC_ATTR_LEN_SHIFT		0
+#define DESC_ATTR_LEN_MASK		GENMASK(11, DESC_ATTR_LEN_SHIFT)
+#define DESC_ATTR_EOF_SHIFT		12
+#define DESC_ATTR_EOF_MASK		GENMASK(15, DESC_ATTR_EOF_SHIFT)
+#define DESC_ATTR_SOF_SHIFT		16
+#define DESC_ATTR_SOF_MASK		GENMASK(19, DESC_ATTR_SOF_SHIFT)
+#define DESC_ATTR_TX_ISOCH_DMA_EN	BIT(20)	/* TX */
+#define DESC_ATTR_RX_CRC_ERR		BIT(20)	/* RX after use */
+#define DESC_ATTR_DESC_DONE		BIT(21)
+#define DESC_ATTR_REQ_STS		BIT(22)	/* TX and RX before use */
+#define DESC_ATTR_RX_BUF_OVRN_ERR	BIT(22)	/* RX after use */
+#define DESC_ATTR_INT_EN		BIT(23)
+#define DESC_ATTR_OFFSET_SHIFT		24
+#define DESC_ATTR_OFFSET_MASK		GENMASK(31, DESC_ATTR_OFFSET_SHIFT)
+
+
 /* NHI registers in bar 0 */
 
 /*
@@ -60,6 +92,30 @@ struct ring_desc {
  */
 #define REG_RX_RING_BASE	0x08000
 
+#define REG_RING_STEP			16
+#define REG_RING_PHYS_LO_OFFSET		0
+#define REG_RING_PHYS_HI_OFFSET		4
+#define REG_RING_CONS_PROD_OFFSET	8	/* cons - RO, prod - RW */
+#define REG_RING_CONS_SHIFT		0
+#define REG_RING_CONS_MASK		GENMASK(15, REG_RING_CONS_SHIFT)
+#define REG_RING_PROD_SHIFT		16
+#define REG_RING_PROD_MASK		GENMASK(31, REG_RING_PROD_SHIFT)
+#define REG_RING_SIZE_OFFSET		12
+#define REG_RING_SIZE_SHIFT		0
+#define REG_RING_SIZE_MASK		GENMASK(15, REG_RING_SIZE_SHIFT)
+#define REG_RING_BUF_SIZE_SHIFT		16
+#define REG_RING_BUF_SIZE_MASK		GENMASK(27, REG_RING_BUF_SIZE_SHIFT)
+
+#define TBT_RING_CONS_PROD_REG(iobase, ringbase, ringnumber) \
+			      ((iobase) + (ringbase) + \
+			      ((ringnumber) * REG_RING_STEP) + \
+			      REG_RING_CONS_PROD_OFFSET)
+
+#define TBT_REG_RING_PROD_EXTRACT(val) (((val) & REG_RING_PROD_MASK) >> \
+				       REG_RING_PROD_SHIFT)
+
+#define TBT_REG_RING_CONS_EXTRACT(val) (((val) & REG_RING_CONS_MASK) >> \
+				       REG_RING_CONS_SHIFT)
 /*
  * 32 bytes per entry, one entry for every hop (REG_HOP_COUNT)
  * 00: enum_ring_flags
@@ -77,6 +133,19 @@ struct ring_desc {
  * ..: unknown
  */
 #define REG_RX_OPTIONS_BASE	0x29800
+#define REG_RX_OPTS_TX_E2E_HOP_ID_SHIFT	12
+#define REG_RX_OPTS_TX_E2E_HOP_ID_MASK	\
+				GENMASK(22, REG_RX_OPTS_TX_E2E_HOP_ID_SHIFT)
+#define REG_RX_OPTS_MASK_OFFSET		4
+#define REG_RX_OPTS_MASK_EOF_SHIFT	0
+#define REG_RX_OPTS_MASK_EOF_MASK	GENMASK(15, REG_RX_OPTS_MASK_EOF_SHIFT)
+#define REG_RX_OPTS_MASK_SOF_SHIFT	16
+#define REG_RX_OPTS_MASK_SOF_MASK	GENMASK(31, REG_RX_OPTS_MASK_SOF_SHIFT)
+
+#define REG_OPTS_STEP			32
+#define REG_OPTS_E2E_EN			BIT(28)
+#define REG_OPTS_RAW			BIT(30)
+#define REG_OPTS_VALID			BIT(31)
 
 /*
  * three bitfields: tx, rx, rx overflow
@@ -86,6 +155,7 @@ struct ring_desc {
  */
 #define REG_RING_NOTIFY_BASE	0x37800
 #define RING_NOTIFY_REG_COUNT(nhi) ((31 + 3 * nhi->hop_count) / 32)
+#define REG_RING_NOTIFY_STEP	4
 
 /*
  * two bitfields: rx, tx
@@ -94,8 +164,47 @@ struct ring_desc {
  */
 #define REG_RING_INTERRUPT_BASE	0x38200
 #define RING_INTERRUPT_REG_COUNT(nhi) ((31 + 2 * nhi->hop_count) / 32)
+#define REG_RING_INT_TX_PROCESSED(ring_num)		BIT(ring_num)
+#define REG_RING_INT_RX_PROCESSED(ring_num, num_paths)	BIT((ring_num) + \
+							    (num_paths))
+#define RING_INT_DISABLE(base, val) iowrite32( \
+			ioread32((base) + REG_RING_INTERRUPT_BASE) & ~(val), \
+			(base) + REG_RING_INTERRUPT_BASE)
+#define RING_INT_ENABLE(base, val) iowrite32( \
+			ioread32((base) + REG_RING_INTERRUPT_BASE) | (val), \
+			(base) + REG_RING_INTERRUPT_BASE)
+#define RING_INT_DISABLE_TX(base, ring_num) \
+	RING_INT_DISABLE(base, REG_RING_INT_TX_PROCESSED(ring_num))
+#define RING_INT_DISABLE_RX(base, ring_num, num_paths) \
+	RING_INT_DISABLE(base, REG_RING_INT_RX_PROCESSED(ring_num, num_paths))
+#define RING_INT_ENABLE_TX(base, ring_num) \
+	RING_INT_ENABLE(base, REG_RING_INT_TX_PROCESSED(ring_num))
+#define RING_INT_ENABLE_RX(base, ring_num, num_paths) \
+	RING_INT_ENABLE(base, REG_RING_INT_RX_PROCESSED(ring_num, num_paths))
+#define RING_INT_DISABLE_TX_RX(base, ring_num, num_paths) \
+	RING_INT_DISABLE(base, REG_RING_INT_TX_PROCESSED(ring_num) | \
+			       REG_RING_INT_RX_PROCESSED(ring_num, num_paths))
+
+#define REG_RING_INTERRUPT_STEP	4
+
+#define REG_INT_THROTTLING_RATE	0x38c00
+#define REG_INT_THROTTLING_RATE_STEP	4
+#define NUM_INT_VECTORS			16
+
+#define REG_INT_VEC_ALLOC_BASE	0x38c40
+#define REG_INT_VEC_ALLOC_STEP		4
+#define REG_INT_VEC_ALLOC_FIELD_BITS	4
+#define REG_INT_VEC_ALLOC_FIELD_MASK	(BIT(REG_INT_VEC_ALLOC_FIELD_BITS) - 1)
+#define REG_INT_VEC_ALLOC_PER_REG	((BITS_PER_BYTE * sizeof(u32)) / \
+					 REG_INT_VEC_ALLOC_FIELD_BITS)
 
 /* The last 11 bits contain the number of hops supported by the NHI port. */
 #define REG_HOP_COUNT		0x39640
+#define REG_HOP_COUNT_TOTAL_PATHS_MASK	GENMASK(10, 0)
+
+#define REG_HOST_INTERFACE_RST	0x39858
+
+#define REG_DMA_MISC		0x39864
+#define REG_DMA_MISC_INT_AUTO_CLEAR	BIT(2)
 
 #endif
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1451792 — [PATCH v5 8/8] thunderbolt: Adding maintainer entry

Add Amir Levy as maintainer for Thunderbolt(TM) ICM driver

Signed-off-by: Amir Levy <amir.jer.levy@intel.com>
---
 MAINTAINERS | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 771c31c..5f24eb2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10141,7 +10141,13 @@ F:	include/uapi/linux/stm.h
 THUNDERBOLT DRIVER
 M:	Andreas Noever <andreas.noever@gmail.com>
 S:	Maintained
-F:	drivers/thunderbolt/
+F:	drivers/thunderbolt/*
+
+THUNDERBOLT ICM DRIVER
+M:	Amir Levy <amir.jer.levy@intel.com>
+S:	Maintained
+F:	drivers/thunderbolt/icm/
+F:	Documentation/thunderbolt-networking.txt
 
 TI BQ27XXX POWER SUPPLY DRIVER
 R:	Andrew F. Davis <afd@ti.com>
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1451793 — [PATCH v5 7/8] thunderbolt: Networking doc

Adding Thunderbolt(TM) networking documentation.

Signed-off-by: Amir Levy <amir.jer.levy@intel.com>
---
 Documentation/00-INDEX                   |   2 +
 Documentation/thunderbolt-networking.txt | 135 +++++++++++++++++++++++++++++++
 2 files changed, 137 insertions(+)
 create mode 100644 Documentation/thunderbolt-networking.txt

diff --git a/Documentation/00-INDEX b/Documentation/00-INDEX
index cb9a6c6..80a6706 100644
--- a/Documentation/00-INDEX
+++ b/Documentation/00-INDEX
@@ -439,6 +439,8 @@ this_cpu_ops.txt
 	- List rationale behind and the way to use this_cpu operations.
 thermal/
 	- directory with information on managing thermal issues (CPU/temp)
+thunderbolt-networking.txt
+	- Thunderbolt(TM) Networking driver description.
 trace/
 	- directory with info on tracing technologies within linux
 unaligned-memory-access.txt
diff --git a/Documentation/thunderbolt-networking.txt b/Documentation/thunderbolt-networking.txt
new file mode 100644
index 0000000..b7714cf
--- /dev/null
+++ b/Documentation/thunderbolt-networking.txt
@@ -0,0 +1,135 @@
+Intel Thunderbolt(TM) Linux driver
+==================================
+
+Copyright(c) 2013 - 2016 Intel Corporation.
+
+Contact Information:
+Intel Thunderbolt mailing list <thunderbolt-software@lists.01.org>
+Edited by Michael Jamet <michael.jamet@intel.com>
+
+Overview
+========
+
+Thunderbolt(TM) Networking mode is introduced with this driver.
+This kernel code creates an ethernet device utilized in computer to computer
+communication over a Thunderbolt cable.
+This driver has been added on the top of the existing thunderbolt driver
+for systems with firwmare (FW) based Thunderbolt controllers supporting
+Thunderbolt Networking.
+
+Files
+=====
+
+- icm_nhi.c/h:	These files allow communication with the FW (a.k.a ICM) based controller.
+		In addition, they create an interface for netlink communication with
+		a user space daemon.
+
+- net.c/net.h:	These files implement the 'eth' interface for the Thunderbolt(TM)
+		networking.
+
+Interface to user space
+=======================
+
+The interface to the user space module is implemented through a Generic Netlink.
+In order to be accessed by the user space module, both kernel and user space
+modules have to register with the same GENL_NAME. In our case, this is
+simply "thunderbolt".
+The registration is done at driver initialization time for all instances of
+the Thunderbolt controllers.
+The communication is then carried through pre-defined Thunderbolt messages.
+Each specific message has a callback function that is called when
+the related message is received.
+
+The messages are defined as follows:
+* NHI_CMD_UNSPEC: Not used.
+* NHI_CMD_SUBSCRIBE: Subscription request from daemon to driver to open the
+  communication channel.
+* NHI_CMD_UNSUBSCRIBE: Request from daemon to driver to unsubscribe
+  to close communication channel.
+* NHI_CMD_QUERY_INFORMATION: Request information from the driver such as
+  driver version, FW version offset, number of ports in the controller
+  and DMA port.
+* NHI_CMD_MSG_TO_ICM: Message from user space module to FW.
+* NHI_CMD_MSG_FROM_ICM: Response from FW to user space module.
+* NHI_CMD_MAILBOX: Message that uses mailbox mechanism such as FW policy
+  changes or disconnect path.
+* NHI_CMD_APPROVE_TBT_NETWORKING: Request from user space
+  module to FW to establish path.
+* NHI_CMD_ICM_IN_SAFE_MODE: Indication that the FW has entered safe mode.
+
+Communication with ICM (Firmware)
+=================================
+
+The communication with ICM is principally achieved through
+a DMA mechanism on Ring 0.
+The driver allocates a shared memory that is physically mapped onto
+the DMA physical space at Ring 0.
+
+Interrupts
+==========
+
+Thunderbolt relies on MSI-X interrupts.
+The MSI-X vector is allocated as follows:
+ICM
+     - Tx: MSI-X vector index 0
+     - Rx: MSI-X vector index 1
+
+Port 0
+     - Tx: MSI-X vector index 2
+     - Rx: MSI-X vector index 3
+
+Port 1
+     - Tx: MSI-X vector index 4
+     - Rx: MSI-X vector index 5
+
+ICM interrupts are used for communication with ICM only.
+Port 0 and Port 1 interrupts are used for Thunderbolt Networking
+communications.
+In case MSI-X is not available, the driver requests to enable MSI only.
+
+Mutexes, semaphores and spinlocks
+=================================
+
+The driver should be able to operate in an environment where hardware
+is asynchronously accessed by multiple entities such as netlink,
+multiple controllers etc.
+
+* send_sem: This semaphore enforces unique sender (one sender at a time)
+  to avoid wrong impairing with responses. FW may process one message
+  at the time.
+* d0_exit_send_mutex: This mutex protects D0 exit (D3) situation
+  to avoid continuing to send messages to FW.
+* d0_exit_mailbox_mutex: This mutex protects D0 exit (D3) situation to
+  avoid continuing to send commands to mailbox.
+* mailbox_mutex: This mutex enforces unique sender (one sender at a time)
+  for the mailbox command.
+  A mutex is sufficient since the mailbox mechanism uses a polling mechanism
+  to get the command response.
+* lock: This spinlock protects from simultaneous changes during
+  disable/enable interrupts.
+* state_lock: This mutex comes to protect changes during net device state
+  changes and net_device operations.
+* controllers_list_rwsem: This read/write sempahore syncs the access to
+  the controllers list when there are multiple controllers in the system.
+
+FW path enablement
+==================
+
+In order to SW to communicate with the FW, the driver needs to send to FW
+the PDF driver ready command and receive response from FW.
+The driver ready command PDF value is 12 and the response is 13.
+Once the exchange is completed, the user space module should be able to send
+messages through the driver to the FW and FW starts to send notifications
+about HW/FW events.
+
+Information
+===========
+
+Mailing list:
+	thunderbolt-software@lists.01.org
+	Register at: https://lists.01.org/mailman/listinfo/thunderbolt-software
+	Archives at: https://lists.01.org/pipermail/thunderbolt-software/
+
+For additional information about Thunderbolt technology visit:
+	https://01.org/thunderbolt-sw
+	https://thunderbolttechnology.net/
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1451889 — Re: [PATCH v5 5/8] thunderbolt: Networking state machine

On Thu, Jul 28, 2016 at 11:15:18AM +0300, Amir Levy wrote:
> +static void nhi_handle_notification_msg(struct tbt_nhi_ctxt *nhi_ctxt,
> +					const u8 *msg)
> +{
> +	struct port_net_dev *port;
> +	u8 port_num;
> +
> +#define INTER_DOMAIN_LINK_SHIFT 0
> +#define INTER_DOMAIN_LINK_MASK	GENMASK(2, INTER_DOMAIN_LINK_SHIFT)
> +	switch (msg[3]) {
> +
> +	case NC_INTER_DOMAIN_CONNECTED:
> +		port_num = PORT_NUM_FROM_MSG(msg[5]);
> +#define INTER_DOMAIN_APPROVED BIT(3)
> +		if (likely(port_num < nhi_ctxt->num_ports)) {
> +			if (!(msg[5] & INTER_DOMAIN_APPROVED))

I find these interspersed #defines make the code hard to read,
but maybe that's just me.


> +				nhi_ctxt->net_devices[
> +					port_num].medium_sts =

Looks like a carriage return slipped in here.

In patch [4/8], I've found it a bit puzzling that FW->SW responses and
FW->SW notifications are defined in icm_nhi.c, whereas SW->FW commands
are defined in net.h. It would perhaps be more logical to have them
all in the header file. The FW->SW responses and SW->FW commands are
almost identical, there are odd spelling differences (CONNEXION vs.
CONNECTION).

It would probably be good to explain the PDF acronym somewhere.

I've skimmed over all patches in the series, too superficial to provide
a Reviewed-by, it's just too much code to review thoroughly and I also
lack the hardware to test it, but broadly this LGTM.

Thanks,

Lukas

[toc] | [prev] | [next] | [standalone]

#1452815 — RE: [PATCH v5 5/8] thunderbolt: Networking state machine

On Thu, Jul 28 2016, 02:35 PM, Lukas Wunner wrote:
> On Thu, Jul 28, 2016 at 11:15:18AM +0300, Amir Levy wrote:
> > +				nhi_ctxt->net_devices[
> > +					port_num].medium_sts =
> 
> Looks like a carriage return slipped in here.

Will be fixed.

> 
> In patch [4/8], I've found it a bit puzzling that FW->SW responses and
> FW->SW notifications are defined in icm_nhi.c, whereas SW->FW commands
> are defined in net.h. It would perhaps be more logical to have them all in the
> header file. The FW->SW responses and SW->FW commands are almost
> identical, there are odd spelling differences (CONNEXION vs.
> CONNECTION).

Will move them to the header and will change to CONNECTION.

> 
> It would probably be good to explain the PDF acronym somewhere.

Will explain in the enum.

> 
> I've skimmed over all patches in the series, too superficial to provide a
> Reviewed-by, it's just too much code to review thoroughly and I also lack the
> hardware to test it, but broadly this LGTM.

Thank you for the review.

[toc] | [prev] | [next] | [standalone]

#1452568 — Re: [PATCH v5 4/8] thunderbolt: Communication with the ICM (firmware)

On Thu, 28 Jul 2016 11:15:17 +0300
Amir Levy <amir.jer.levy@intel.com> wrote:

> +static LIST_HEAD(controllers_list);
> +static DECLARE_RWSEM(controllers_list_rwsem);

Why use a semaphore when simple spinlock or mutex would be better?

[toc] | [prev] | [next] | [standalone]

#1452588 — Re: [PATCH v5 4/8] thunderbolt: Communication with the ICM (firmware)

On Fri, Jul 29, 2016 at 02:02:24PM -0700, Stephen Hemminger wrote:
> On Thu, 28 Jul 2016 11:15:17 +0300
> Amir Levy <amir.jer.levy@intel.com> wrote:
> 
> > +static LIST_HEAD(controllers_list);
> > +static DECLARE_RWSEM(controllers_list_rwsem);
> 
> Why use a semaphore when simple spinlock or mutex would be better?

And never use a RW semaphore unless you can benchmark the difference
from a normal lock.  If you can't benchmark it, then don't use it...

[toc] | [prev] | [next] | [standalone]

#1452804 — RE: [PATCH v5 4/8] thunderbolt: Communication with the ICM (firmware)

On Sat, Jul 30 2016, 12:48 AM, Greg KH wrote:
> On Fri, Jul 29, 2016 at 02:02:24PM -0700, Stephen Hemminger wrote:
> > On Thu, 28 Jul 2016 11:15:17 +0300
> > Amir Levy <amir.jer.levy@intel.com> wrote:
> >
> > > +static LIST_HEAD(controllers_list); static
> > > +DECLARE_RWSEM(controllers_list_rwsem);
> >
> > Why use a semaphore when simple spinlock or mutex would be better?
> 
> And never use a RW semaphore unless you can benchmark the difference
> from a normal lock.  If you can't benchmark it, then don't use it...

I used RW semaphore since most of the time the list will be accessed for read.
Since it is used in non-time sensitive flows, I'll change it to mutex.

[toc] | [prev] | [next] | [standalone]

#1452570 — Re: [PATCH v5 4/8] thunderbolt: Communication with the ICM (firmware)

On Thu, 28 Jul 2016 11:15:17 +0300
Amir Levy <amir.jer.levy@intel.com> wrote:

> +int nhi_send_message(struct tbt_nhi_ctxt *nhi_ctxt, enum pdf_value pdf,
> +		     u32 msg_len, const u8 *msg, bool ignore_icm_resp)
> +{

Why not make msg a void * and not have to do so many casts?

[toc] | [prev] | [next] | [standalone]

#1452816 — RE: [PATCH v5 4/8] thunderbolt: Communication with the ICM (firmware)

On Sat, Jul 30 2016, 12:03 AM, Stephen Hemminger wrote:
> On Thu, 28 Jul 2016 11:15:17 +0300
> Amir Levy <amir.jer.levy@intel.com> wrote:
> 
> > +int nhi_send_message(struct tbt_nhi_ctxt *nhi_ctxt, enum pdf_value
> pdf,
> > +		     u32 msg_len, const u8 *msg, bool ignore_icm_resp) {
> 
> Why not make msg a void * and not have to do so many casts?

I couldn't agree more.

[toc] | [prev] | [next] | [standalone]

#1452575 — Re: [PATCH v5 6/8] thunderbolt: Networking transmit and receive

On Thu, 28 Jul 2016 11:15:19 +0300
Amir Levy <amir.jer.levy@intel.com> wrote:

> +		/* pad short packets */
> +		if (unlikely(skb->len < ETH_ZLEN)) {
> +			int pad_len = ETH_ZLEN - skb->len;
> +
> +			/* The skb is freed on error */
> +			if (unlikely(skb_pad(skb, pad_len))) {
> +				cleaned_count += frame_count;
> +				continue;
> +			}
> +			__skb_put(skb, pad_len);
> +		}

Packets should be padded on transmit, not on receive??

[toc] | [prev] | [next] | [standalone]

#1452802 — RE: [PATCH v5 6/8] thunderbolt: Networking transmit and receive

On Sat, Jul 30 2016, 12:07 AM, Stephen Hemminger wrote:
> On Thu, 28 Jul 2016 11:15:19 +0300
> Amir Levy <amir.jer.levy@intel.com> wrote:
> 
> > +		/* pad short packets */
> > +		if (unlikely(skb->len < ETH_ZLEN)) {
> > +			int pad_len = ETH_ZLEN - skb->len;
> > +
> > +			/* The skb is freed on error */
> > +			if (unlikely(skb_pad(skb, pad_len))) {
> > +				cleaned_count += frame_count;
> > +				continue;
> > +			}
> > +			__skb_put(skb, pad_len);
> > +		}
> 
> Packets should be padded on transmit, not on receive??

This driver emulates an Ethernet adapter on top of Thunderbolt technology.
The Thunderbolt medium hasn't any restriction on minimum frame size and doesn't have the Ethernet collision detection limitation.
So moving this code from transmit is actually an optimization - sending the minimum on the wire.
The network stack thinks it is Ethernet, it might not accept Runt frames, so the driver pads the frame in receive.

Looks like it deserves a comment in the code. Will add it.

[toc] | [prev] | [next] | [standalone]

#1452862 — Re: [PATCH v5 6/8] thunderbolt: Networking transmit and receive

From: "Levy, Amir (Jer)" <amir.jer.levy@intel.com>
Date: Sun, 31 Jul 2016 10:15:52 +0000

> The network stack thinks it is Ethernet, it might not accept Runt
> frames, so the driver pads the frame in receive.

The network stack doesn't care about this at all.  It's wasted effort
on your part.

[toc] | [prev] | [next] | [standalone]

#1452981 — RE: [PATCH v5 6/8] thunderbolt: Networking transmit and receive

On Sun, Jul 31 2016, 09:45 PM, David Miller wrote:
> From: "Levy, Amir (Jer)" <amir.jer.levy@intel.com>
> Date: Sun, 31 Jul 2016 10:15:52 +0000
> 
> > The network stack thinks it is Ethernet, it might not accept Runt
> > frames, so the driver pads the frame in receive.
> 
> The network stack doesn't care about this at all.  It's wasted effort on your
> part.

Great, I'll remove this code.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web