Re: [PATCH v6 4/8] thunderbolt: Communication with the ICM (firmware)

[Greg KH]

On Mon, Aug 01, 2016 at 03:23:49PM +0300, Amir Levy wrote:
> Firmware-based (a.k.a ICM - Intel Connection Manager) controller is
> used for establishing and maintaining the Thunderbolt Networking
> connection. We need to be able to communicate with it.
> 
> Signed-off-by: Amir Levy 
> ---
>  drivers/thunderbolt/Makefile  |1 +
>  drivers/thunderbolt/icm/Makefile  |   28 +
>  drivers/thunderbolt/icm/icm_nhi.c | 1324 
> +
>  drivers/thunderbolt/icm/icm_nhi.h |   93 +++
>  drivers/thunderbolt/icm/net.h |  227 +++
>  5 files changed, 1673 insertions(+)
>  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.h
> 
> diff --git a/drivers/thunderbolt/Makefile b/drivers/thunderbolt/Makefile
> index 7a85bd1..b6aa6a3 100644
> --- a/drivers/thunderbolt/Makefile
> +++ b/drivers/thunderbolt/Makefile
> @@ -1,3 +1,4 @@
>  obj-${CONFIG_THUNDERBOLT_APPLE} := thunderbolt.o
>  thunderbolt-objs := nhi.o ctl.o tb.o switch.o cap.o path.o tunnel_pci.o 
> eeprom.o
>  
> +obj-${CONFIG_THUNDERBOLT_ICM} += icm/
> diff --git a/drivers/thunderbolt/icm/Makefile 
> b/drivers/thunderbolt/icm/Makefile
> new file mode 100644
> index 000..3adfc35
> --- /dev/null
> +++ b/drivers/thunderbolt/icm/Makefile
> @@ -0,0 +1,28 @@
> +
> +#
> +# Intel Thunderbolt(TM) driver
> +# Copyright(c) 2014 - 2016 Intel Corporation.
> +#
> +# This program is free software; you can redistribute it and/or modify it
> +# under the terms and conditions of the GNU General Public License,
> +# version 2, as published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope it will be useful, but WITHOUT
> +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> +# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> +# more details.
> +#
> +# You should have received a copy of the GNU General Public License along
> +# with this program.  If not, see .
> +#
> +# The full GNU General Public License is included in this distribution in
> +# the file called "COPYING".

Why are these two paragraphs needed?

> +#
> +# Contact Information:
> +# Intel Thunderbolt Mailing List 
> +# Intel Corporation, 5200 N.E. Elam Young Parkway, Hillsboro, OR 97124-6497

We try to stay away from addresses in kernel files as no one wants to
maintain the corporate movements of companies for years to come.  Are
you willing to do this for the next 40 years?

> +#
> +
> +
> +obj-${CONFIG_THUNDERBOLT_ICM} += thunderbolt-icm.o
> +thunderbolt-icm-objs := icm_nhi.o

All of that for a simple 2 line Makefile?  Please no, just have a 2 line
Makefile...

And you don't have a Kconfig file that adds this option, right?

What exactly does this driver do?  Why would I want it enabled?  I'm
lost...

> diff --git a/drivers/thunderbolt/icm/icm_nhi.c 
> b/drivers/thunderbolt/icm/icm_nhi.c
> new file mode 100644
> index 000..bcb5c1b
> --- /dev/null
> +++ b/drivers/thunderbolt/icm/icm_nhi.c
> @@ -0,0 +1,1324 @@
> +/***
> + *
> + * Intel Thunderbolt(TM) driver
> + * Copyright(c) 2014 - 2016 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program.  If not, see .
> + *
> + * The full GNU General Public License is included in this distribution in
> + * the file called "COPYING".
> + *
> + * Contact Information:
> + * Intel Thunderbolt Mailing List 
> + * Intel Corporation, 5200 N.E. Elam Young Parkway, Hillsboro, OR 97124-6497
> + *
> + 
> **/
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "icm_nhi.h"
> +#include "net.h"
> +
> +#define NHI_GENL_VERSION 1
> +#define NHI_GENL_NAME DRV_NAME

Why not just use DRV_NAME?

Why have this at all?  Why even have DRV_NAME?  Seems useless as you are
not going to change it.

thanks,

greg k-h

[PATCH v6 4/8] thunderbolt: Communication with the ICM (firmware)

[Amir Levy]

Firmware-based (a.k.a ICM - Intel Connection Manager) controller is
used for establishing and maintaining the Thunderbolt Networking
connection. We need to be able to communicate with it.

Signed-off-by: Amir Levy 
---
 drivers/thunderbolt/Makefile  |1 +
 drivers/thunderbolt/icm/Makefile  |   28 +
 drivers/thunderbolt/icm/icm_nhi.c | 1324 +
 drivers/thunderbolt/icm/icm_nhi.h |   93 +++
 drivers/thunderbolt/icm/net.h |  227 +++
 5 files changed, 1673 insertions(+)
 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.h

diff --git a/drivers/thunderbolt/Makefile b/drivers/thunderbolt/Makefile
index 7a85bd1..b6aa6a3 100644
--- a/drivers/thunderbolt/Makefile
+++ b/drivers/thunderbolt/Makefile
@@ -1,3 +1,4 @@
 obj-${CONFIG_THUNDERBOLT_APPLE} := thunderbolt.o
 thunderbolt-objs := nhi.o ctl.o tb.o switch.o cap.o path.o tunnel_pci.o 
eeprom.o
 
+obj-${CONFIG_THUNDERBOLT_ICM} += icm/
diff --git a/drivers/thunderbolt/icm/Makefile b/drivers/thunderbolt/icm/Makefile
new file mode 100644
index 000..3adfc35
--- /dev/null
+++ b/drivers/thunderbolt/icm/Makefile
@@ -0,0 +1,28 @@
+
+#
+# Intel Thunderbolt(TM) driver
+# Copyright(c) 2014 - 2016 Intel Corporation.
+#
+# This program is free software; you can redistribute it and/or modify it
+# under the terms and conditions of the GNU General Public License,
+# version 2, as published by the Free Software Foundation.
+#
+# This program is distributed in the hope it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+# more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program.  If not, see .
+#
+# The full GNU General Public License is included in this distribution in
+# the file called "COPYING".
+#
+# Contact Information:
+# Intel Thunderbolt Mailing List 
+# Intel Corporation, 5200 N.E. Elam Young Parkway, Hillsboro, OR 97124-6497
+#
+
+
+obj-${CONFIG_THUNDERBOLT_ICM} += thunderbolt-icm.o
+thunderbolt-icm-objs := icm_nhi.o
diff --git a/drivers/thunderbolt/icm/icm_nhi.c 
b/drivers/thunderbolt/icm/icm_nhi.c
new file mode 100644
index 000..bcb5c1b
--- /dev/null
+++ b/drivers/thunderbolt/icm/icm_nhi.c
@@ -0,0 +1,1324 @@
+/***
+ *
+ * Intel Thunderbolt(TM) driver
+ * Copyright(c) 2014 - 2016 Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see .
+ *
+ * The full GNU General Public License is included in this distribution in
+ * the file called "COPYING".
+ *
+ * Contact Information:
+ * Intel Thunderbolt Mailing List 
+ * Intel Corporation, 5200 N.E. Elam Young Parkway, Hillsboro, OR 97124-6497
+ *
+ 
**/
+
+#include 
+#include 
+#include 
+#include 
+#include "icm_nhi.h"
+#include "net.h"
+
+#define NHI_GENL_VERSION 1
+#define NHI_GENL_NAME DRV_NAME
+
+#define DEVICE_DATA(num_ports, dma_port, nvm_ver_offset, nvm_auth_on_boot,\
+   support_full_e2e) \
+   ((num_ports) | ((dma_port) << 4) | ((nvm_ver_offset) << 10) | \
+((nvm_auth_on_boot) << 22) | ((support_full_e2e) << 23))
+#define DEVICE_DATA_NUM_PORTS(device_data) ((device_data) & 0xf)
+#define DEVICE_DATA_DMA_PORT(device_data) (((device_data) >> 4) & 0x3f)
+#define DEVICE_DATA_NVM_VER_OFFSET(device_data) (((device_data) >> 10) & 0xfff)
+#define DEVICE_DATA_NVM_AUTH_ON_BOOT(device_data) (((device_data) >> 22) & 0x1)
+#define DEVICE_DATA_SUPPORT_FULL_E2E(device_data) (((device_data) >> 23) & 0x1)
+
+#define USEC_TO_256_NSECS(usec) DIV_ROUND_UP((usec) * NSEC_PER_USEC, 256)
+
+/* NHI genetlink commands */
+enum {
+   NHI_CMD_UNSPEC,
+   NHI_CMD_SUBSCRIBE,
+   NHI_CMD_UNSUBSCRIBE,
+   NHI_CMD_QUERY_INFORMATION,
+   NHI_CMD_MSG_TO_ICM,
+   NHI_CMD_MSG_FROM_ICM,
+   NHI_CMD_MAILBOX,
+