[v3,5/8] thunderbolt: Communication with the ICM (firmware)

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 <amir.jer.levy@intel.com>
---
 drivers/thunderbolt/Makefile      |    1 +
 drivers/thunderbolt/icm/Makefile  |   28 +
 drivers/thunderbolt/icm/icm_nhi.c | 1332 +++++++++++++++++++++++++++++++++++++
 drivers/thunderbolt/icm/icm_nhi.h |   84 +++
 drivers/thunderbolt/icm/net.h     |  200 ++++++
 5 files changed, 1645 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

> 
> 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 <amir.jer.levy@intel.com>
> ---
>  drivers/thunderbolt/Makefile      |    1 +
>  drivers/thunderbolt/icm/Makefile  |   28 +
>  drivers/thunderbolt/icm/icm_nhi.c | 1332
> +++++++++++++++++++++++++++++++++++++
>  drivers/thunderbolt/icm/icm_nhi.h |   84 +++
>  drivers/thunderbolt/icm/net.h     |  200 ++++++
>  5 files changed, 1645 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 0000000..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 <http://www.gnu.org/licenses/>.
> +#
> +# The full GNU General Public License is included in this distribution in
> +# the file called "COPYING".
> +#
> +# Contact Information:
> +# Intel Thunderbolt Mailing List <thunderbolt-software@lists.01.org>
> +# 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

It would be cleaner if you add the  Makefile + Kconfig   in a separate patch 

> diff --git a/drivers/thunderbolt/icm/icm_nhi.c
> b/drivers/thunderbolt/icm/icm_nhi.c
> new file mode 100644
> index 0000000..9d178a5
> --- /dev/null
> +++ b/drivers/thunderbolt/icm/icm_nhi.c
> @@ -0,0 +1,1332 @@
> +/*********************************************************
> **********************
> + *
> + * 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 <http://www.gnu.org/licenses/>.
> + *
> + * The full GNU General Public License is included in this distribution in
> + * the file called "COPYING".
> + *
> + * Contact Information:
> + * Intel Thunderbolt Mailing List <thunderbolt-software@lists.01.org>
> + * Intel Corporation, 5200 N.E. Elam Young Parkway, Hillsboro, OR 97124-
> 6497
> + *
> +
> **********************************************************
> ********************/
> +
> +#include <linux/printk.h>
> +#include <linux/crc32.h>
> +#include <linux/delay.h>
> +#include <linux/dmi.h>
> +#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)
> +
> +/*
> + * FW->SW responses
> + * RC = response code
More verbose will be better 
> + */
> +enum {
> +	RC_GET_TBT_TOPOLOGY = 1,
> +	RC_GET_VIDEO_RESOURCES_DATA,
> +	RC_DRV_READY,
> +	RC_APPROVE_PCI_CONNEXION,
> +	RC_CHALLENGE_PCI_CONNEXION,
> +	RC_ADD_DEVICE_AND_KEY,
> +	RC_INTER_DOMAIN_PKT_SENT = 8,
> +	RC_APPROVE_INTER_DOMAIN_CONNEXION = 0x10
> +};
> +
> +/*
> + * FW->SW notifications
> + * NC = notification code
Same here
> + */
> +enum {
> +	NC_DEVICE_CONNECTED = 3,
> +	NC_DEVICE_DISCONNECTED,
> +	NC_DP_DEVICE_CONNECTED_NOT_TUNNELED,
> +	NC_INTER_DOMAIN_CONNECTED,
> +	NC_INTER_DOMAIN_DISCONNECTED
> +};
> +
> +/* 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,
> +	NHI_CMD_APPROVE_TBT_NETWORKING,
> +	NHI_CMD_ICM_IN_SAFE_MODE,
> +	__NHI_CMD_MAX,
> +};
> +#define NHI_CMD_MAX (__NHI_CMD_MAX - 1)
NHI_CMD_MAX  = NHI_CMD_ICM_IN_SAFE_MODE ? 

> +/* NHI genetlink policy */
> +static const struct nla_policy nhi_genl_policy[NHI_ATTR_MAX + 1] = {
> +	[NHI_ATTR_DRV_VERSION]		= { .type = NLA_NUL_STRING,
> },
> +	[NHI_ATTR_NVM_VER_OFFSET]	= { .type = NLA_U16, },
> +	[NHI_ATTR_NUM_PORTS]		= { .type = NLA_U8, },
> +	[NHI_ATTR_DMA_PORT]		= { .type = NLA_U8, },
> +	[NHI_ATTR_SUPPORT_FULL_E2E]	= { .type = NLA_FLAG, },
> +	[NHI_ATTR_MAILBOX_CMD]		= { .type = NLA_U32, },
> +	[NHI_ATTR_PDF]			= { .type = NLA_U32, },
> +	[NHI_ATTR_MSG_TO_ICM]		= { .type = NLA_BINARY,
> +					.len =
> TBT_ICM_RING_MAX_FRAME_SIZE },
> +	[NHI_ATTR_MSG_FROM_ICM]		= { .type = NLA_BINARY,
> +					.len =
> TBT_ICM_RING_MAX_FRAME_SIZE },
> +};
> +
> +/* NHI genetlink family */
> +static struct genl_family nhi_genl_family = {
> +	.id		= GENL_ID_GENERATE,
> +	.hdrsize	= FIELD_SIZEOF(struct tbt_nhi_ctxt, id),
> +	.name		= NHI_GENL_NAME,
> +	.version	= NHI_GENL_VERSION,
> +	.maxattr	= NHI_ATTR_MAX,
> +};
> +
> +static LIST_HEAD(controllers_list);
> +static DECLARE_RWSEM(controllers_list_rwsem);
> +static atomic_t subscribers = ATOMIC_INIT(0);
> +static u32 portid;
> +
> +static bool nhi_nvm_authenticated(struct tbt_nhi_ctxt *nhi_ctxt)
> +{
> +	enum icm_operation_mode op_mode;
> +	u32 *msg_head, port_id, reg;
> +	struct sk_buff *skb;
> +	int i;
> +
> +	if (!nhi_ctxt->nvm_auth_on_boot)
> +		return true;
> +
> +	for (i = 0; i < 5; i++) {
> +		u32 status;
> +
> +		status = ioread32(nhi_ctxt->iobase + REG_FW_STS);
> +
> +		if (status & REG_FW_STS_NVM_AUTH_DONE)
> +			break;
> +		msleep(30);

30 is  big number, for polling, what is behind this?

> +	}
> +	/*
> +	 * The check for authentication is done after checking if iCM
> +	 * is present so it shouldn't reach the max tries (=5).
> +	 * Anyway, the check for full functionality below covers the error
> case.
> +	 */
> +	reg = ioread32(nhi_ctxt->iobase + REG_OUTMAIL_CMD);
> +	op_mode = (reg & REG_OUTMAIL_CMD_OP_MODE_MASK) >>
> +		  REG_OUTMAIL_CMD_OP_MODE_SHIFT;
> +	if (op_mode == FULL_FUNCTIONALITY)
> +		return true;
> +
> +	dev_warn(&nhi_ctxt->pdev->dev, "controller id %#x is in operation
> mode %#x status %#lx\n",
> +		 nhi_ctxt->id, op_mode,
> +		 (reg &
> REG_OUTMAIL_CMD_STS_MASK)>>REG_OUTMAIL_CMD_STS_SHIFT);
> +
> +	skb = genlmsg_new(NLMSG_ALIGN(nhi_genl_family.hdrsize),
> GFP_KERNEL);
> +	if (!skb) {
> +		dev_err(&nhi_ctxt->pdev->dev, "genlmsg_new failed: not
> enough memory to send controller operational mode\n");
> +		return false;
> +	}
> +
> +	/* keeping port_id into a local variable for next use */
> +	port_id = portid;
> +	msg_head = genlmsg_put(skb, port_id, 0, &nhi_genl_family, 0,
> +			       NHI_CMD_ICM_IN_SAFE_MODE);
> +	if (!msg_head) {
> +		nlmsg_free(skb);
> +		dev_err(&nhi_ctxt->pdev->dev, "genlmsg_put failed: not
> enough memory to send controller operational mode\n");
> +		return false;
> +	}
> +
> +	*msg_head = nhi_ctxt->id;
> +
> +	genlmsg_end(skb, msg_head);
> +
> +	genlmsg_unicast(&init_net, skb, port_id);
> +
> +	return false;
> +}
> +
> +int nhi_send_message(struct tbt_nhi_ctxt *nhi_ctxt, enum pdf_value pdf,
> +		     u32 msg_len, const u8 *msg, bool ignore_icm_resp)
> +{
> +	u32 prod_cons, prod, cons, attr;
> +	struct tbt_icm_ring_shared_memory *shared_mem;
> +	void __iomem *reg = TBT_RING_CONS_PROD_REG(nhi_ctxt-
> >iobase,
> +						   REG_TX_RING_BASE,
> +						   TBT_ICM_RING_NUM);
> +
> +	dev_dbg(&nhi_ctxt->pdev->dev,
> +		"send msg: controller id %#x pdf %u cmd %hhu msg len
> %u\n",
> +		nhi_ctxt->id, pdf, msg[3], msg_len);
> +
> +	if (nhi_ctxt->d0_exit) {
> +		dev_notice(&nhi_ctxt->pdev->dev,
> +			   "controller id %#x is exiting D0\n",
> +			   nhi_ctxt->id);
> +		return -ENODEV;
> +	}
> +
> +	prod_cons = ioread32(reg);
> +	prod = TBT_REG_RING_PROD_EXTRACT(prod_cons);
> +	cons = TBT_REG_RING_CONS_EXTRACT(prod_cons);
> +	if (prod >= TBT_ICM_RING_NUM_TX_BUFS) {
> +		dev_warn(&nhi_ctxt->pdev->dev,
> +			 "controller id %#x producer %u out of range\n",
> +			 nhi_ctxt->id, prod);
> +		return -ENODEV;
> +	}
> +	if (TBT_TX_RING_FULL(prod, cons,
> TBT_ICM_RING_NUM_TX_BUFS)) {
> +		dev_err(&nhi_ctxt->pdev->dev,
> +			"controller id %#x TX ring full\n",
> +			nhi_ctxt->id);
> +		return -ENOSPC;
> +	}
> +
> +	attr = (msg_len << DESC_ATTR_LEN_SHIFT) &
> DESC_ATTR_LEN_MASK;
> +	attr |= (pdf << DESC_ATTR_EOF_SHIFT) & DESC_ATTR_EOF_MASK;
> +
> +	shared_mem = nhi_ctxt->icm_ring_shared_mem;
> +	shared_mem->tx_buf_desc[prod].attributes = cpu_to_le32(attr);
> +
> +	memcpy(shared_mem->tx_buf[prod], msg, msg_len);
> +
> +	prod_cons &= ~REG_RING_PROD_MASK;
> +	prod_cons |= (((prod + 1) % TBT_ICM_RING_NUM_TX_BUFS) <<
> +		      REG_RING_PROD_SHIFT) & REG_RING_PROD_MASK;
> +
> +	if (likely(!nhi_ctxt->wait_for_icm_resp))
> +		nhi_ctxt->wait_for_icm_resp = true;
> +	else
> +		dev_dbg(&nhi_ctxt->pdev->dev,
> +			"controller id %#x wait_for_icm_resp should have
> been cleared\n",
> +			nhi_ctxt->id);
> +
> +	nhi_ctxt->ignore_icm_resp = ignore_icm_resp;
> +
> +	iowrite32(prod_cons, reg);
> +
> +	return 0;
> +}
> +
> +static int nhi_send_driver_ready_command(struct tbt_nhi_ctxt *nhi_ctxt)
> +{
> +	struct driver_ready_command {
> +		__be32 req_code;
> +		__be32 crc;
> +	} drv_rdy_cmd = {
> +		.req_code = cpu_to_be32(CC_DRV_READY),
> +	};
> +	u32 crc32;
> +
> +	crc32 = __crc32c_le(~0, (unsigned char const *)&drv_rdy_cmd,
> +			    offsetof(struct driver_ready_command, crc));
> +
> +	drv_rdy_cmd.crc = cpu_to_be32(~crc32);
> +
> +	return nhi_send_message(nhi_ctxt, PDF_SW_TO_FW_COMMAND,
> +				sizeof(drv_rdy_cmd), (u8 *)&drv_rdy_cmd,
> +				false);
> +}
> +
> +static struct tbt_nhi_ctxt *nhi_search_ctxt(u32 id)
> +{
> +	struct tbt_nhi_ctxt *nhi_ctxt;
> +
> +	list_for_each_entry(nhi_ctxt, &controllers_list, node)
> +		if (nhi_ctxt->id == id)
> +			return nhi_ctxt;

Don't you need to lock this list with the controllers_list_rwsem ?

> +
> +	return NULL;
> +}
> +
> +static int nhi_genl_subscribe(__always_unused struct sk_buff *u_skb,
> +			      struct genl_info *info)
> +			      __acquires(&nhi_ctxt->send_sem)
> +{
> +	struct tbt_nhi_ctxt *nhi_ctxt;
> +
> +	/*
> +	 * To send driver ready command to iCM, need at least one
> subscriber
> +	 * that will handle the response.
> +	 * Currently the assumption is one user mode daemon as subscriber
> +	 * so one portid global variable (without locking).
> +	 */
> +	if (atomic_inc_return(&subscribers) >= 1) {
> +		portid = info->snd_portid;
> +		down_read(&controllers_list_rwsem);
> +		list_for_each_entry(nhi_ctxt, &controllers_list, node) {
> +			int res;
> +
> +			if (nhi_ctxt->d0_exit ||
> +			    !nhi_nvm_authenticated(nhi_ctxt))
> +				continue;
> +
> +			res = down_timeout(&nhi_ctxt->send_sem,
> +
> msecs_to_jiffies(10*MSEC_PER_SEC));
> +			if (res) {
> +				dev_err(&nhi_ctxt->pdev->dev,
> +					"%s: controller id %#x timeout on
> send semaphore\n",
> +					__func__, nhi_ctxt->id);
> +				continue;
> +			}
> +
> +			if (!mutex_trylock(&nhi_ctxt-
> >d0_exit_send_mutex)) {
> +				up(&nhi_ctxt->send_sem);
> +				continue;
> +			}
> +
> +			res = nhi_send_driver_ready_command(nhi_ctxt);
> +
> +			mutex_unlock(&nhi_ctxt->d0_exit_send_mutex);
> +			if (res)
> +				up(&nhi_ctxt->send_sem);
> +		}
> +		up_read(&controllers_list_rwsem);
> +	}
> +
> +	return 0;
> +}
> +
> +static int nhi_genl_unsubscribe(__always_unused struct sk_buff *u_skb,
> +				__always_unused struct genl_info *info)
> +{
> +	atomic_dec_if_positive(&subscribers);
> +
> +	return 0;
> +}
> +
> +static int nhi_genl_query_information(__always_unused struct sk_buff
> *u_skb,
> +				      struct genl_info *info)
> +{
> +	struct tbt_nhi_ctxt *nhi_ctxt;
> +	struct sk_buff *skb;
> +	bool msg_too_long;
> +	int res = -ENODEV;
> +	u32 *msg_head;
> +
> +	if (!info || !info->userhdr)
> +		return -EINVAL;
> +
> +	skb = genlmsg_new(NLMSG_ALIGN(nhi_genl_family.hdrsize) +
> +			  nla_total_size(sizeof(DRV_VERSION)) +
> +			  nla_total_size(sizeof(nhi_ctxt->nvm_ver_offset)) +
> +			  nla_total_size(sizeof(nhi_ctxt->num_ports)) +
> +			  nla_total_size(sizeof(nhi_ctxt->dma_port)) +
> +			  nla_total_size(0),	/* nhi_ctxt-
> >support_full_e2e */
> +			  GFP_KERNEL);
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	msg_head = genlmsg_put_reply(skb, info, &nhi_genl_family, 0,
> +				     NHI_CMD_QUERY_INFORMATION);
> +	if (!msg_head) {
> +		res = -ENOMEM;
> +		goto genl_put_reply_failure;
> +	}
> +
> +	down_read(&controllers_list_rwsem);
> +
> +	nhi_ctxt = nhi_search_ctxt(*(u32 *)info->userhdr);
> +	if (nhi_ctxt && !nhi_ctxt->d0_exit) {
> +		*msg_head = nhi_ctxt->id;
> +
> +		msg_too_long = !!nla_put_string(skb,
> NHI_ATTR_DRV_VERSION,
> +						DRV_VERSION);
> +
> +		msg_too_long = msg_too_long ||
> +			       nla_put_u16(skb, NHI_ATTR_NVM_VER_OFFSET,
> +					   nhi_ctxt->nvm_ver_offset);
> +
> +		msg_too_long = msg_too_long ||
> +			       nla_put_u8(skb, NHI_ATTR_NUM_PORTS,
> +					  nhi_ctxt->num_ports);
> +
> +		msg_too_long = msg_too_long ||
> +			       nla_put_u8(skb, NHI_ATTR_DMA_PORT,
> +					  nhi_ctxt->dma_port);
> +
> +		if (msg_too_long) {
> +			res = -EMSGSIZE;
> +			goto release_ctl_list_lock;
> +		}
> +
> +		if (nhi_ctxt->support_full_e2e &&
> +		    nla_put_flag(skb, NHI_ATTR_SUPPORT_FULL_E2E)) {
> +			res = -EMSGSIZE;
> +			goto release_ctl_list_lock;
> +		}
> +		up_read(&controllers_list_rwsem);
> +
> +		genlmsg_end(skb, msg_head);
> +
> +		return genlmsg_reply(skb, info);
> +	}
> +
> +release_ctl_list_lock:
> +	up_read(&controllers_list_rwsem);
> +	genlmsg_cancel(skb, msg_head);
> +
> +genl_put_reply_failure:
> +	nlmsg_free(skb);
> +
> +	return res;
> +}
> +
> +static int nhi_genl_msg_to_icm(__always_unused struct sk_buff *u_skb,
> +			       struct genl_info *info)
> +			       __acquires(&nhi_ctxt->send_sem)
> +{
> +	struct tbt_nhi_ctxt *nhi_ctxt;
> +	int res = -ENODEV;
> +	int msg_len;
> +	void *msg;
> +
> +	if (!info || !info->userhdr || !info->attrs ||
> +	    !info->attrs[NHI_ATTR_PDF] || !info-
> >attrs[NHI_ATTR_MSG_TO_ICM])
> +		return -EINVAL;
> +
> +	msg_len = nla_len(info->attrs[NHI_ATTR_MSG_TO_ICM]);
> +	if (msg_len > TBT_ICM_RING_MAX_FRAME_SIZE)
> +		return -ENOBUFS;
> +
> +	msg = nla_data(info->attrs[NHI_ATTR_MSG_TO_ICM]);
> +
> +	down_read(&controllers_list_rwsem);
> +	nhi_ctxt = nhi_search_ctxt(*(u32 *)info->userhdr);
> +	if (nhi_ctxt && !nhi_ctxt->d0_exit) {
> +		/*
> +		 * waiting 10 seconds to receive a FW response
> +		 * if not, just give up and pop up an error
> +		 */
> +		res = down_timeout(&nhi_ctxt->send_sem,
> +				   msecs_to_jiffies(10 * MSEC_PER_SEC));
> +		if (res) {
> +			void __iomem *rx_prod_cons =
> TBT_RING_CONS_PROD_REG(
> +							nhi_ctxt->iobase,
> +							REG_RX_RING_BASE,
> +
> 	TBT_ICM_RING_NUM);
> +			void __iomem *tx_prod_cons =
> TBT_RING_CONS_PROD_REG(
> +							nhi_ctxt->iobase,
> +							REG_TX_RING_BASE,
> +
> 	TBT_ICM_RING_NUM);
> +			dev_err(&nhi_ctxt->pdev->dev,
> +				"controller id %#x timeout on send
> semaphore\n",
> +				nhi_ctxt->id);
> +			dev_dbg(&nhi_ctxt->pdev->dev,
> +				"controller id %#x, tx prod&cons=%#x, rx
> prod&cons=%#x\n",
> +				nhi_ctxt->id,
> +				ioread32(tx_prod_cons),
> +				ioread32(rx_prod_cons));
> +			goto release_ctl_list_lock;
> +		}
> +
> +		if (!mutex_trylock(&nhi_ctxt->d0_exit_send_mutex)) {
> +			up(&nhi_ctxt->send_sem);
> +			dev_notice(&nhi_ctxt->pdev->dev,
> +				   "controller id %#x is exiting D0\n",
> +				   nhi_ctxt->id);
> +			goto release_ctl_list_lock;
> +		}
> +
> +		up_read(&controllers_list_rwsem);
> +
> +		res = nhi_send_message(nhi_ctxt,
> +				       nla_get_u32(info-
> >attrs[NHI_ATTR_PDF]),
> +				       msg_len, msg, false);
> +
> +		mutex_unlock(&nhi_ctxt->d0_exit_send_mutex);
> +		if (res)
> +			up(&nhi_ctxt->send_sem);
> +
> +		return res;
> +	}
> +
> +release_ctl_list_lock:
> +	up_read(&controllers_list_rwsem);
> +	return res;
> +}
> +
> +int nhi_mailbox(struct tbt_nhi_ctxt *nhi_ctxt, u32 cmd, u32 data, bool
> deinit)
> +{
> +	u32 delay = deinit ? U32_C(20) : U32_C(100);
> +	int i;
> +
> +	dev_dbg(&nhi_ctxt->pdev->dev, "controller id %#x mbox command
> %#x\n",
> +		nhi_ctxt->id, cmd);
> +	iowrite32(data, nhi_ctxt->iobase + REG_INMAIL_DATA);
> +	iowrite32(cmd, nhi_ctxt->iobase + REG_INMAIL_CMD);
> +
> +#define NHI_INMAIL_CMD_RETRIES 50
> +	/*
> +	 * READ_ONCE fetches the value of nhi_ctxt->d0_exit every time
> +	 * and avoid optimization.
> +	 * deinit = true to continue the loop even if D3 process has been
> +	 * carried out.
> +	 */
> +	for (i = 0; (i < NHI_INMAIL_CMD_RETRIES) &&
> +		    (deinit || !READ_ONCE(nhi_ctxt->d0_exit)); i++) {
> +		cmd = ioread32(nhi_ctxt->iobase + REG_INMAIL_CMD);
> +
> +		if (cmd & REG_INMAIL_CMD_ERROR) {
> +			/*
> +			 * when deinit this just informative as it may
> +			 * due to unplug of the cable
> +			 */
> +			if (!deinit)
> +				dev_err(&nhi_ctxt->pdev->dev,
> +					"inmail error after %d msecs\n",
> +					i * delay);
> +			else
> +				dev_info(&nhi_ctxt->pdev->dev,
> +					 "inmail error after %d msecs\n",
> +					 i * delay);
> +
> +			return -EIO;
> +		}
> +
> +		if (!(cmd & REG_INMAIL_CMD_REQUEST))
> +			break;
> +
> +		msleep(delay);
> +	}
> +
> +	if (i == NHI_INMAIL_CMD_RETRIES) {
> +		if (!deinit)
> +			dev_err(&nhi_ctxt->pdev->dev, "inmail timeout\n");
> +		else
> +			dev_info(&nhi_ctxt->pdev->dev, "inmail
> timeout\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
> +
> +static int nhi_mailbox_generic(struct tbt_nhi_ctxt *nhi_ctxt, u32 mb_cmd)
> +	__releases(&controllers_list_rwsem)
> +{
> +	int res = -ENODEV;
> +
> +	if (mutex_lock_interruptible(&nhi_ctxt->mailbox_mutex)) {
> +		res = -ERESTART;
> +		goto release_ctl_list_lock;
> +	}
> +
> +	if (!mutex_trylock(&nhi_ctxt->d0_exit_mailbox_mutex)) {
> +		mutex_unlock(&nhi_ctxt->mailbox_mutex);
> +		dev_notice(&nhi_ctxt->pdev->dev,
> +			   "controller id %#x is exiting D0\n",
> +			   nhi_ctxt->id);
> +		goto release_ctl_list_lock;
> +	}
> +
> +	up_read(&controllers_list_rwsem);
> +
> +	res = nhi_mailbox(nhi_ctxt, mb_cmd, 0, false);
> +	mutex_unlock(&nhi_ctxt->d0_exit_mailbox_mutex);
> +	mutex_unlock(&nhi_ctxt->mailbox_mutex);
> +
> +	return res;
> +
> +release_ctl_list_lock:
> +	up_read(&controllers_list_rwsem);
> +	return res;
> +}
> +
> +static int nhi_genl_mailbox(__always_unused struct sk_buff *u_skb,
> +			    struct genl_info *info)
> +{
> +	struct tbt_nhi_ctxt *nhi_ctxt;
> +	u32 cmd, mb_cmd;
> +
> +	if (!info || !info->userhdr || !info->attrs ||
> +	    !info->attrs[NHI_ATTR_MAILBOX_CMD])
> +		return -EINVAL;
> +
> +	cmd = nla_get_u32(info->attrs[NHI_ATTR_MAILBOX_CMD]);
> +	mb_cmd = ((cmd << REG_INMAIL_CMD_CMD_SHIFT) &
> +		  REG_INMAIL_CMD_CMD_MASK) |
> REG_INMAIL_CMD_REQUEST;
> +
> +	down_read(&controllers_list_rwsem);
> +
> +	nhi_ctxt = nhi_search_ctxt(*(u32 *)info->userhdr);
> +	if (nhi_ctxt && !nhi_ctxt->d0_exit)
> +		return nhi_mailbox_generic(nhi_ctxt, mb_cmd);
> +
> +	up_read(&controllers_list_rwsem);
> +	return -ENODEV;
> +}
> +
> +
> +static int nhi_genl_send_msg(struct tbt_nhi_ctxt *nhi_ctxt, enum
> pdf_value pdf,
> +			     const u8 *msg, u32 msg_len)
> +{
> +	u32 *msg_head, port_id;
> +	struct sk_buff *skb;
> +	int res;
> +
> +	if (atomic_read(&subscribers) < 1) {
> +		dev_notice(&nhi_ctxt->pdev->dev, "no subscribers for
> controller id %#x, dropping message - pdf %u cmd %hhu msg len %u\n",
> +			   nhi_ctxt->id, pdf, msg[3], msg_len);
> +		return -ENOTCONN;
> +	}
> +
> +	skb = genlmsg_new(NLMSG_ALIGN(nhi_genl_family.hdrsize) +
> +			  nla_total_size(msg_len) +
> +			  nla_total_size(sizeof(pdf)),
> +			  GFP_KERNEL);
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	port_id = portid;
> +	msg_head = genlmsg_put(skb, port_id, 0, &nhi_genl_family, 0,
> +			       NHI_CMD_MSG_FROM_ICM);
> +	if (!msg_head) {
> +		res = -ENOMEM;
> +		goto genl_put_reply_failure;
> +	}
> +
> +	*msg_head = nhi_ctxt->id;
> +
> +	if (nla_put_u32(skb, NHI_ATTR_PDF, pdf) ||
> +	    nla_put(skb, NHI_ATTR_MSG_FROM_ICM, msg_len, msg)) {
> +		res = -EMSGSIZE;
> +		goto nla_put_failure;
> +	}
> +
> +	genlmsg_end(skb, msg_head);
> +
> +	return genlmsg_unicast(&init_net, skb, port_id);
> +
> +nla_put_failure:
> +	genlmsg_cancel(skb, msg_head);
> +genl_put_reply_failure:
> +	nlmsg_free(skb);
> +
> +	return res;
> +}
> +
> +static bool nhi_msg_from_icm_analysis(struct tbt_nhi_ctxt *nhi_ctxt,
> +					enum pdf_value pdf,
> +					const u8 *msg, u32 msg_len)
> +{
> +	/*
> +	 * preparation for messages that won't be sent,
> +	 * currently unused in this patch.
> +	 */
> +	bool send_event = true;
> +
> +	switch (pdf) {
> +	case PDF_ERROR_NOTIFICATION:
> +		dev_err(&nhi_ctxt->pdev->dev,
> +			"controller id %#x PDF_ERROR_NOTIFICATION %hhu
> msg len %u\n",
> +			nhi_ctxt->id, msg[11], msg_len);
> +		/* fallthrough */
> +	case PDF_WRITE_CONFIGURATION_REGISTERS:
> +		/* fallthrough */
> +	case PDF_READ_CONFIGURATION_REGISTERS:
> +		if (nhi_ctxt->wait_for_icm_resp) {
> +			nhi_ctxt->wait_for_icm_resp = false;
> +			up(&nhi_ctxt->send_sem);
> +		}
> +		break;
> +
> +	case PDF_FW_TO_SW_RESPONSE:
> +		if (nhi_ctxt->wait_for_icm_resp) {
> +			nhi_ctxt->wait_for_icm_resp = false;
> +			up(&nhi_ctxt->send_sem);
> +		}
> +		break;
> +
> +	default:
> +		dev_warn(&nhi_ctxt->pdev->dev,
> +			 "controller id %#x pdf %u isn't handled/expected\n",
> +			 nhi_ctxt->id, pdf);
> +		break;
> +	}
> +
> +	return send_event;
> +}
> +
> +static void nhi_msgs_from_icm(struct work_struct *work)
> +			      __releases(&nhi_ctxt->send_sem)
> +{
> +	struct tbt_nhi_ctxt *nhi_ctxt = container_of(work,
> typeof(*nhi_ctxt),
> +						     icm_msgs_work);
> +	void __iomem *reg = TBT_RING_CONS_PROD_REG(nhi_ctxt-
> >iobase,
> +						   REG_RX_RING_BASE,
> +						   TBT_ICM_RING_NUM);
> +	u32 prod_cons, prod, cons;
> +
> +	prod_cons = ioread32(reg);
> +	prod = TBT_REG_RING_PROD_EXTRACT(prod_cons);
> +	cons = TBT_REG_RING_CONS_EXTRACT(prod_cons);
> +	if (prod >= TBT_ICM_RING_NUM_RX_BUFS) {
> +		dev_warn(&nhi_ctxt->pdev->dev,
> +			 "controller id %#x producer %u out of range\n",
> +			 nhi_ctxt->id, prod);
> +		return;
> +	}
> +	if (cons >= TBT_ICM_RING_NUM_RX_BUFS) {
> +		dev_warn(&nhi_ctxt->pdev->dev,
> +			 "controller id %#x consumer %u out of range\n",
> +			 nhi_ctxt->id, cons);
> +		return;
> +	}
> +
> +	while (!TBT_RX_RING_EMPTY(prod, cons,
> TBT_ICM_RING_NUM_RX_BUFS) &&
> +	       !nhi_ctxt->d0_exit) {
> +		struct tbt_buf_desc *rx_desc;
> +		u8 *msg;
> +		u32 msg_len;
> +		enum pdf_value pdf;
> +		bool send_event;
> +
> +		cons = (cons + 1) % TBT_ICM_RING_NUM_RX_BUFS;
> +		rx_desc = &(nhi_ctxt->icm_ring_shared_mem-
> >rx_buf_desc[cons]);
> +		if (!(le32_to_cpu(rx_desc->attributes) &
> +		      DESC_ATTR_DESC_DONE)) {
> +			usleep_range(10, 20);
> +			if (unlikely(!(le32_to_cpu(rx_desc->attributes) &
> +				       DESC_ATTR_DESC_DONE)))
> +				dev_err(&nhi_ctxt->pdev->dev,
> +					"controller id %#x buffer %u might
> not completely processed\n",
> +					nhi_ctxt->id, cons);
> +		}
> +
> +		rmb(); /* read the descriptor and the buffer after DD check
> */
> +		pdf = (le32_to_cpu(rx_desc->attributes) &
> DESC_ATTR_EOF_MASK)
> +		      >> DESC_ATTR_EOF_SHIFT;
> +		msg = nhi_ctxt->icm_ring_shared_mem->rx_buf[cons];
> +		msg_len = (le32_to_cpu(rx_desc-
> >attributes)&DESC_ATTR_LEN_MASK)
> +			  >> DESC_ATTR_LEN_SHIFT;
> +
> +		dev_dbg(&nhi_ctxt->pdev->dev,
> +			"%s: controller id %#x pdf %u cmd %hhu msg len
> %u\n",
> +			__func__, nhi_ctxt->id, pdf, msg[3], msg_len);
> +
> +		send_event = nhi_msg_from_icm_analysis(nhi_ctxt, pdf,
> msg,
> +						       msg_len);
> +
> +		if (send_event)
> +			nhi_genl_send_msg(nhi_ctxt, pdf, msg, msg_len);
> +
> +		/* set the descriptor for another receive */
> +		rx_desc->attributes = cpu_to_le32(DESC_ATTR_REQ_STS |
> +						  DESC_ATTR_INT_EN);
> +		rx_desc->time = 0;
> +	}
> +
> +	/* free the descriptors for more receive */
> +	prod_cons &= ~REG_RING_CONS_MASK;
> +	prod_cons |= (cons << REG_RING_CONS_SHIFT) &
> REG_RING_CONS_MASK;
> +	iowrite32(prod_cons, reg);
> +
> +	if (!nhi_ctxt->d0_exit) {
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&nhi_ctxt->lock, flags);
> +		/* enable RX interrupt */
> +		RING_INT_ENABLE_RX(nhi_ctxt->iobase,
> TBT_ICM_RING_NUM,
> +				   nhi_ctxt->num_paths);
> +
> +		spin_unlock_irqrestore(&nhi_ctxt->lock, flags);
> +	}
> +}
> +
> +static irqreturn_t nhi_icm_ring_rx_msix(int __always_unused irq, void
> *data)
> +{
> +	struct tbt_nhi_ctxt *nhi_ctxt = data;
> +
> +	spin_lock(&nhi_ctxt->lock);
> +	/*
> +	 * disable RX interrupt
> +	 * We like to allow interrupt mitigation until the work item
> +	 * will be completed.
> +	 */
> +	RING_INT_DISABLE_RX(nhi_ctxt->iobase, TBT_ICM_RING_NUM,
> +			    nhi_ctxt->num_paths);
> +
> +	spin_unlock(&nhi_ctxt->lock);
> +
> +	schedule_work(&nhi_ctxt->icm_msgs_work);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t nhi_msi(int __always_unused irq, void *data)
> +{
> +	struct tbt_nhi_ctxt *nhi_ctxt = data;
> +	u32 isr0, isr1, imr0, imr1;
> +
> +	/* clear on read */
> +	isr0 = ioread32(nhi_ctxt->iobase + REG_RING_NOTIFY_BASE);
> +	isr1 = ioread32(nhi_ctxt->iobase + REG_RING_NOTIFY_BASE +
> +
> 	REG_RING_NOTIFY_STEP);
> +	if (unlikely(!isr0 && !isr1))
> +		return IRQ_NONE;
> +
> +	spin_lock(&nhi_ctxt->lock);
> +
> +	imr0 = ioread32(nhi_ctxt->iobase + REG_RING_INTERRUPT_BASE);
> +	imr1 = ioread32(nhi_ctxt->iobase + REG_RING_INTERRUPT_BASE +
> +			REG_RING_INTERRUPT_STEP);
> +	/* disable the arrived interrupts */
> +	iowrite32(imr0 & ~isr0,
> +		  nhi_ctxt->iobase + REG_RING_INTERRUPT_BASE);
> +	iowrite32(imr1 & ~isr1,
> +		  nhi_ctxt->iobase + REG_RING_INTERRUPT_BASE +
> +		  REG_RING_INTERRUPT_STEP);
> +
> +	spin_unlock(&nhi_ctxt->lock);
> +
> +	if (isr0 & REG_RING_INT_RX_PROCESSED(TBT_ICM_RING_NUM,
> +					     nhi_ctxt->num_paths))
> +		schedule_work(&nhi_ctxt->icm_msgs_work);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/**
> + * nhi_set_int_vec - Mapping of the MSIX vector entry to the ring
> + * @nhi_ctxt: contains data on NHI controller
> + * @path: ring to be mapped
> + * @msix_msg_id: msix entry to be mapped
> + */
> +static inline void nhi_set_int_vec(struct tbt_nhi_ctxt *nhi_ctxt, u32 path,
> +				   u8 msix_msg_id)
> +{
> +	void __iomem *reg;
> +	u32 step, shift, ivr;
> +
> +	if (msix_msg_id % 2)
> +		path += nhi_ctxt->num_paths;
> +
> +	step = path / REG_INT_VEC_ALLOC_PER_REG;
> +	shift = (path % REG_INT_VEC_ALLOC_PER_REG) *
> +		REG_INT_VEC_ALLOC_FIELD_BITS;
> +	reg = nhi_ctxt->iobase + REG_INT_VEC_ALLOC_BASE +
> +					(step * REG_INT_VEC_ALLOC_STEP);
> +	ivr = ioread32(reg) & ~(REG_INT_VEC_ALLOC_FIELD_MASK << shift);
> +	iowrite32(ivr | (msix_msg_id << shift), reg);
> +}
> +
> +/* NHI genetlink operations array */
> +static const struct genl_ops nhi_ops[] = {
> +	{
> +		.cmd = NHI_CMD_SUBSCRIBE,
> +		.policy = nhi_genl_policy,
> +		.doit = nhi_genl_subscribe,
> +	},
> +	{
> +		.cmd = NHI_CMD_UNSUBSCRIBE,
> +		.policy = nhi_genl_policy,
> +		.doit = nhi_genl_unsubscribe,
> +	},
> +	{
> +		.cmd = NHI_CMD_QUERY_INFORMATION,
> +		.policy = nhi_genl_policy,
> +		.doit = nhi_genl_query_information,
> +	},
> +	{
> +		.cmd = NHI_CMD_MSG_TO_ICM,
> +		.policy = nhi_genl_policy,
> +		.doit = nhi_genl_msg_to_icm,
> +		.flags = GENL_ADMIN_PERM,
> +	},
> +	{
> +		.cmd = NHI_CMD_MAILBOX,
> +		.policy = nhi_genl_policy,
> +		.doit = nhi_genl_mailbox,
> +		.flags = GENL_ADMIN_PERM,
> +	},
> +};
> +
> +static int nhi_suspend(struct device *dev) __releases(&nhi_ctxt-
> >send_sem)
> +{
> +	struct tbt_nhi_ctxt *nhi_ctxt = pci_get_drvdata(to_pci_dev(dev));
> +	void __iomem *rx_reg, *tx_reg;
> +	u32 rx_reg_val, tx_reg_val;
> +
> +	/* must be after negotiation_events, since messages might be sent
> */
> +	nhi_ctxt->d0_exit = true;
> +
> +	rx_reg = nhi_ctxt->iobase + REG_RX_OPTIONS_BASE +
> +		 (TBT_ICM_RING_NUM * REG_OPTS_STEP);
> +	rx_reg_val = ioread32(rx_reg) & ~REG_OPTS_E2E_EN;
> +	tx_reg = nhi_ctxt->iobase + REG_TX_OPTIONS_BASE +
> +		 (TBT_ICM_RING_NUM * REG_OPTS_STEP);
> +	tx_reg_val = ioread32(tx_reg) & ~REG_OPTS_E2E_EN;
> +	/* disable RX flow control  */
> +	iowrite32(rx_reg_val, rx_reg);
> +	/* disable TX flow control  */
> +	iowrite32(tx_reg_val, tx_reg);
> +	/* disable RX ring  */
> +	iowrite32(rx_reg_val & ~REG_OPTS_VALID, rx_reg);
> +
> +	mutex_lock(&nhi_ctxt->d0_exit_mailbox_mutex);
> +	mutex_lock(&nhi_ctxt->d0_exit_send_mutex);
> +
> +	cancel_work_sync(&nhi_ctxt->icm_msgs_work);
> +
> +	if (nhi_ctxt->wait_for_icm_resp) {
> +		nhi_ctxt->wait_for_icm_resp = false;
> +		nhi_ctxt->ignore_icm_resp = false;
> +		/*
> +		 * if there is response, it is lost, so unlock the send
> +		 * for the next resume.
> +		 */
> +		up(&nhi_ctxt->send_sem);
> +	}
> +
> +	mutex_unlock(&nhi_ctxt->d0_exit_send_mutex);
> +	mutex_unlock(&nhi_ctxt->d0_exit_mailbox_mutex);
> +
> +	/* wait for all TX to finish  */
> +	usleep_range(5 * USEC_PER_MSEC, 7 * USEC_PER_MSEC);
> +
> +	/* disable all interrupts */
> +	iowrite32(0, nhi_ctxt->iobase + REG_RING_INTERRUPT_BASE);
> +	/* disable TX ring  */
> +	iowrite32(tx_reg_val & ~REG_OPTS_VALID, tx_reg);
> +
> +	return 0;
> +}
> +
> +static int nhi_resume(struct device *dev) __acquires(&nhi_ctxt-
> >send_sem)
> +{
> +	dma_addr_t phys;
> +	struct tbt_nhi_ctxt *nhi_ctxt = pci_get_drvdata(to_pci_dev(dev));
> +	struct tbt_buf_desc *desc;
> +	void __iomem *iobase = nhi_ctxt->iobase;
> +	void __iomem *reg;
> +	int i;
> +
> +	if (nhi_ctxt->msix_entries) {
> +		iowrite32(ioread32(iobase + REG_DMA_MISC) |
> +
> 	REG_DMA_MISC_INT_AUTO_CLEAR,
> +			  iobase + REG_DMA_MISC);
> +		/*
> +		 * Vector #0, which is TX complete to ICM,
> +		 * isn't been used currently.
> +		 */
> +		nhi_set_int_vec(nhi_ctxt, 0, 1);
> +
> +		for (i = 2; i < nhi_ctxt->num_vectors; i++)
> +			nhi_set_int_vec(nhi_ctxt, nhi_ctxt->num_paths -
> (i/2),
> +					i);
> +	}
> +
> +	/* configure TX descriptors */
> +	for (i = 0, phys = nhi_ctxt->icm_ring_shared_mem_dma_addr;
> +	     i < TBT_ICM_RING_NUM_TX_BUFS;
> +	     i++, phys += TBT_ICM_RING_MAX_FRAME_SIZE) {
> +		desc = &nhi_ctxt->icm_ring_shared_mem->tx_buf_desc[i];
> +		desc->phys = cpu_to_le64(phys);
> +		desc->attributes = cpu_to_le32(DESC_ATTR_REQ_STS);
> +	}
> +	/* configure RX descriptors */
> +	for (i = 0;
> +	     i < TBT_ICM_RING_NUM_RX_BUFS;
> +	     i++, phys += TBT_ICM_RING_MAX_FRAME_SIZE) {
> +		desc = &nhi_ctxt->icm_ring_shared_mem->rx_buf_desc[i];
> +		desc->phys = cpu_to_le64(phys);
> +		desc->attributes = cpu_to_le32(DESC_ATTR_REQ_STS |
> +					       DESC_ATTR_INT_EN);
> +	}
> +
> +	/* configure throttling rate for interrupts */
> +	for (i = 0, reg = iobase + REG_INT_THROTTLING_RATE;
> +	     i < NUM_INT_VECTORS;
> +	     i++, reg += REG_INT_THROTTLING_RATE_STEP) {
> +		iowrite32(USEC_TO_256_NSECS(128), reg);
> +	}
> +
> +	/* configure TX for ICM ring */
> +	reg = iobase + REG_TX_RING_BASE + (TBT_ICM_RING_NUM *
> REG_RING_STEP);
> +	phys = nhi_ctxt->icm_ring_shared_mem_dma_addr +
> +		offsetof(struct tbt_icm_ring_shared_memory, tx_buf_desc);
> +	iowrite32(lower_32_bits(phys), reg + REG_RING_PHYS_LO_OFFSET);
> +	iowrite32(upper_32_bits(phys), reg + REG_RING_PHYS_HI_OFFSET);
> +	iowrite32((TBT_ICM_RING_NUM_TX_BUFS <<
> REG_RING_SIZE_SHIFT) &
> +			REG_RING_SIZE_MASK,
> +		  reg + REG_RING_SIZE_OFFSET);
> +
> +	reg = iobase + REG_TX_OPTIONS_BASE +
> (TBT_ICM_RING_NUM*REG_OPTS_STEP);
> +	iowrite32(REG_OPTS_RAW | REG_OPTS_VALID, reg);
> +
> +	/* configure RX for ICM ring */
> +	reg = iobase + REG_RX_RING_BASE + (TBT_ICM_RING_NUM *
> REG_RING_STEP);
> +	phys = nhi_ctxt->icm_ring_shared_mem_dma_addr +
> +		offsetof(struct tbt_icm_ring_shared_memory, rx_buf_desc);
> +	iowrite32(lower_32_bits(phys), reg + REG_RING_PHYS_LO_OFFSET);
> +	iowrite32(upper_32_bits(phys), reg + REG_RING_PHYS_HI_OFFSET);
> +	iowrite32(((TBT_ICM_RING_NUM_RX_BUFS <<
> REG_RING_SIZE_SHIFT) &
> +			REG_RING_SIZE_MASK) |
> +		  ((TBT_ICM_RING_MAX_FRAME_SIZE <<
> REG_RING_BUF_SIZE_SHIFT) &
> +			REG_RING_BUF_SIZE_MASK),
> +		  reg + REG_RING_SIZE_OFFSET);
> +	iowrite32(((TBT_ICM_RING_NUM_RX_BUFS - 1) <<
> REG_RING_CONS_SHIFT) &
> +			REG_RING_CONS_MASK,
> +		  reg + REG_RING_CONS_PROD_OFFSET);
> +
> +	reg = iobase + REG_RX_OPTIONS_BASE +
> (TBT_ICM_RING_NUM*REG_OPTS_STEP);
> +	iowrite32(REG_OPTS_RAW | REG_OPTS_VALID, reg);
> +
> +	/* enable RX interrupt */
> +	RING_INT_ENABLE_RX(iobase, TBT_ICM_RING_NUM, nhi_ctxt-
> >num_paths);
> +
> +	if (likely((atomic_read(&subscribers) > 0) &&
> +		   nhi_nvm_authenticated(nhi_ctxt))) {
> +		down(&nhi_ctxt->send_sem);
> +		nhi_ctxt->d0_exit = false;
> +		mutex_lock(&nhi_ctxt->d0_exit_send_mutex);
> +		/*
> +		 * interrupts are enabled here before send due to
> +		 * implicit barrier in mutex
> +		 */
> +		nhi_send_driver_ready_command(nhi_ctxt);
> +		mutex_unlock(&nhi_ctxt->d0_exit_send_mutex);
> +	} else {
> +		nhi_ctxt->d0_exit = false;
> +	}
> +
> +	return 0;
> +}
> +
> +static void icm_nhi_shutdown(struct pci_dev *pdev)
> +{
> +	nhi_suspend(&pdev->dev);
> +}
> +
> +static void icm_nhi_remove(struct pci_dev *pdev)
> +{
> +	struct tbt_nhi_ctxt *nhi_ctxt = pci_get_drvdata(pdev);
> +	int i;
> +
> +	nhi_suspend(&pdev->dev);
> +
> +	if (nhi_ctxt->net_workqueue)
> +		destroy_workqueue(nhi_ctxt->net_workqueue);
> +
> +	/*
> +	 * disable irq for msix or msi
> +	 */
> +	if (likely(nhi_ctxt->msix_entries)) {
> +		/* Vector #0 isn't been used currently */
> +		devm_free_irq(&pdev->dev, nhi_ctxt-
> >msix_entries[1].vector,
> +			      nhi_ctxt);
> +		pci_disable_msix(pdev);
> +	} else {
> +		devm_free_irq(&pdev->dev, pdev->irq, nhi_ctxt);
> +		pci_disable_msi(pdev);
> +	}
> +
> +	/*
> +	 * remove controller from the controllers list
> +	 */
> +	down_write(&controllers_list_rwsem);
> +	list_del(&nhi_ctxt->node);
> +	up_write(&controllers_list_rwsem);
> +
> +	nhi_mailbox(
> +		nhi_ctxt,
> +
> 	((CC_DRV_UNLOADS_AND_DISCONNECT_INTER_DOMAIN_PATHS
> +		  << REG_INMAIL_CMD_CMD_SHIFT) &
> +		 REG_INMAIL_CMD_CMD_MASK) |
> +		REG_INMAIL_CMD_REQUEST,
> +		0, true);
> +
> +	usleep_range(1 * USEC_PER_MSEC, 5 * USEC_PER_MSEC);
> +	iowrite32(1, nhi_ctxt->iobase + REG_HOST_INTERFACE_RST);
> +
> +	mutex_destroy(&nhi_ctxt->d0_exit_send_mutex);
> +	mutex_destroy(&nhi_ctxt->d0_exit_mailbox_mutex);
> +	mutex_destroy(&nhi_ctxt->mailbox_mutex);
> +	for (i = 0; i < nhi_ctxt->num_ports; i++)
> +		mutex_destroy(&(nhi_ctxt->net_devices[i].state_mutex));
> +}
> +
> +static int icm_nhi_probe(struct pci_dev *pdev, const struct pci_device_id
> *id)
> +{
> +	struct tbt_nhi_ctxt *nhi_ctxt;
> +	void __iomem *iobase;
> +	int i, res;
> +	bool enable_msi = false;
> +
> +	res = pcim_enable_device(pdev);
> +	if (res) {
> +		dev_err(&pdev->dev, "cannot enable PCI device,
> aborting\n");
> +		return res;
> +	}
> +
> +	res = pcim_iomap_regions(pdev, 1 << NHI_MMIO_BAR,
> pci_name(pdev));
> +	if (res) {
> +		dev_err(&pdev->dev, "cannot obtain PCI resources,
> aborting\n");
> +		return res;
> +	}
> +
> +	/* cannot fail - table is allocated in pcim_iomap_regions */
> +	iobase = pcim_iomap_table(pdev)[NHI_MMIO_BAR];
> +
> +	/* check if ICM is running */
> +	if (!(ioread32(iobase + REG_FW_STS) & REG_FW_STS_ICM_EN)) {
> +		dev_err(&pdev->dev, "ICM isn't present, aborting\n");
> +		return -ENODEV;
> +	}
> +
> +	nhi_ctxt = devm_kzalloc(&pdev->dev, sizeof(*nhi_ctxt),
> GFP_KERNEL);
> +	if (!nhi_ctxt)
> +		return -ENOMEM;
> +
> +	nhi_ctxt->pdev = pdev;
> +	nhi_ctxt->iobase = iobase;
> +	nhi_ctxt->id = (PCI_DEVID(pdev->bus->number, pdev->devfn) <<
> 16) |
> +								id->device;
> +	/*
> +	 * Number of paths represents the number of rings available for
> +	 * the controller.
> +	 */
> +	nhi_ctxt->num_paths = ioread32(iobase + REG_HOP_COUNT) &
> +
> 	REG_HOP_COUNT_TOTAL_PATHS_MASK;
> +
> +	nhi_ctxt->nvm_auth_on_boot =
> DEVICE_DATA_NVM_AUTH_ON_BOOT(
> +							id->driver_data);
> +	nhi_ctxt->support_full_e2e = DEVICE_DATA_SUPPORT_FULL_E2E(
> +							id->driver_data);
> +
> +	nhi_ctxt->dma_port = DEVICE_DATA_DMA_PORT(id->driver_data);
> +	/*
> +	 * Number of ports in the controller
> +	 */
> +	nhi_ctxt->num_ports = DEVICE_DATA_NUM_PORTS(id-
> >driver_data);
> +	nhi_ctxt->nvm_ver_offset = DEVICE_DATA_NVM_VER_OFFSET(id-
> >driver_data);
> +
> +	mutex_init(&nhi_ctxt->d0_exit_send_mutex);
> +	mutex_init(&nhi_ctxt->d0_exit_mailbox_mutex);
> +	mutex_init(&nhi_ctxt->mailbox_mutex);
> +
> +	sema_init(&nhi_ctxt->send_sem, 1);
> +
> +	INIT_WORK(&nhi_ctxt->icm_msgs_work, nhi_msgs_from_icm);
> +
> +	spin_lock_init(&nhi_ctxt->lock);
> +
> +	nhi_ctxt->net_devices = devm_kcalloc(&pdev->dev,
> +					     nhi_ctxt->num_ports,
> +					     sizeof(struct port_net_dev),
> +					     GFP_KERNEL);
> +	if (!nhi_ctxt->net_devices)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < nhi_ctxt->num_ports; i++)
> +		mutex_init(&(nhi_ctxt->net_devices[i].state_mutex));
> +
> +	/*
> +	 * allocating RX and TX vectors for ICM and per port
> +	 * for thunderbolt networking.
> +	 * The mapping of the vector is carried out by
> +	 * nhi_set_int_vec and looks like:
> +	 * 0=tx icm, 1=rx icm, 2=tx data port 0,
> +	 * 3=rx data port 0...
> +	 */
> +	nhi_ctxt->num_vectors = (1 + nhi_ctxt->num_ports) * 2;
> +	nhi_ctxt->msix_entries = devm_kcalloc(&pdev->dev,
> +					      nhi_ctxt->num_vectors,
> +					      sizeof(struct msix_entry),
> +					      GFP_KERNEL);
> +	if (likely(nhi_ctxt->msix_entries)) {
> +		for (i = 0; i < nhi_ctxt->num_vectors; i++)
> +			nhi_ctxt->msix_entries[i].entry = i;
> +		res = pci_enable_msix_exact(pdev,
> +					    nhi_ctxt->msix_entries,
> +					    nhi_ctxt->num_vectors);
> +
> +		if (res ||
> +		    /*
> +		     * Allocating ICM RX only.
> +		     * vector #0, which is TX complete to ICM,
> +		     * isn't been used currently
> +		     */
> +		    devm_request_irq(&pdev->dev,
> +				     nhi_ctxt->msix_entries[1].vector,
> +				     nhi_icm_ring_rx_msix, 0, pci_name(pdev),
> +				     nhi_ctxt)) {
> +			devm_kfree(&pdev->dev, nhi_ctxt->msix_entries);
> +			nhi_ctxt->msix_entries = NULL;
> +			enable_msi = true;
> +		}
> +	} else {
> +		enable_msi = true;
> +	}
> +	/*
> +	 * In case allocation didn't succeed, use msi instead of msix
> +	 */
> +	if (enable_msi) {
> +		res = pci_enable_msi(pdev);
> +		if (res) {
> +			dev_err(&pdev->dev, "cannot enable MSI,
> aborting\n");
> +			return res;
> +		}
> +		res = devm_request_irq(&pdev->dev, pdev->irq, nhi_msi, 0,
> +				       pci_name(pdev), nhi_ctxt);
> +		if (res) {
> +			dev_err(&pdev->dev,
> +				"request_irq failed %d, aborting\n", res);
> +			return res;
> +		}
> +	}
> +	/*
> +	 * try to work with address space of 64 bits.
> +	 * In case this doesn't work, work with 32 bits.
> +	 */
> +	if (!dma_set_mask_and_coherent(&pdev->dev,
> DMA_BIT_MASK(64))) {
> +		nhi_ctxt->pci_using_dac = true;
> +	} else {
> +		res = dma_set_mask_and_coherent(&pdev->dev,
> DMA_BIT_MASK(32));
> +		if (res) {
> +			dev_err(&pdev->dev,
> +				"No suitable DMA available, aborting\n");
> +			return res;
> +		}
> +	}
> +
> +	BUILD_BUG_ON(sizeof(struct tbt_buf_desc) != 16);
> +	BUILD_BUG_ON(sizeof(struct tbt_icm_ring_shared_memory) >
> PAGE_SIZE);
> +	nhi_ctxt->icm_ring_shared_mem = dmam_alloc_coherent(
> +			&pdev->dev, sizeof(*nhi_ctxt-
> >icm_ring_shared_mem),
> +			&nhi_ctxt->icm_ring_shared_mem_dma_addr,
> +			GFP_KERNEL | __GFP_ZERO);
> +	if (nhi_ctxt->icm_ring_shared_mem == NULL) {
> +		dev_err(&pdev->dev, "dmam_alloc_coherent failed,
> aborting\n");
> +		return -ENOMEM;
> +	}
> +
> +	nhi_ctxt->net_workqueue =
> create_singlethread_workqueue(DRV_NAME);
> +	if (!nhi_ctxt->net_workqueue) {
> +		dev_err(&pdev->dev, "create_singlethread_workqueue
> failed, aborting\n");
> +		return -ENOMEM;
> +	}
> +
> +	pci_set_master(pdev);
> +	pci_set_drvdata(pdev, nhi_ctxt);
> +
> +	nhi_resume(&pdev->dev);
> +	/*
> +	 * Add the new controller at the end of the list
> +	 */
> +	down_write(&controllers_list_rwsem);
> +	list_add_tail(&nhi_ctxt->node, &controllers_list);
> +	up_write(&controllers_list_rwsem);
> +
> +	return res;
> +}
> +
> +/*
> + * The tunneled pci bridges are siblings of us. Use resume_noirq to reenable
> + * the tunnels asap. A corresponding pci quirk blocks the downstream
> bridges
> + * resume_noirq until we are done.
> + */
> +static const struct dev_pm_ops icm_nhi_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(nhi_suspend, nhi_resume)
> +};
> +
> +static const struct pci_device_id nhi_pci_device_ids[] = {
> +	{ PCI_VDEVICE(INTEL,
> PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_2C_NHI),
> +					DEVICE_DATA(1, 5, 0xa, false, false) },
> +	{ PCI_VDEVICE(INTEL,
> PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_4C_NHI),
> +					DEVICE_DATA(2, 5, 0xa, false, false) },
> +	{ PCI_VDEVICE(INTEL,
> PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI),
> +					DEVICE_DATA(1, 5, 0xa, false, false) },
> +	{ PCI_VDEVICE(INTEL,
> PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI),
> +					DEVICE_DATA(2, 5, 0xa, false, false) },
> +	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_WIN_RIDGE_2C_NHI),
> +					DEVICE_DATA(1, 3, 0xa, false, false) },
> +	{ PCI_VDEVICE(INTEL,
> PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_NHI),
> +					DEVICE_DATA(1, 5, 0xa, true, true) },
> +	{ PCI_VDEVICE(INTEL,
> PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_NHI),
> +					DEVICE_DATA(2, 5, 0xa, true, true) },
> +	{ PCI_VDEVICE(INTEL,
> PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_NHI),
> +					DEVICE_DATA(1, 3, 0xa, true, true) },
> +	{ PCI_VDEVICE(INTEL,
> PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_NHI),
> +					DEVICE_DATA(1, 5, 0xa, true, true) },
> +	{ PCI_VDEVICE(INTEL,
> PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_NHI),
> +					DEVICE_DATA(2, 5, 0xa, true, true) },
> +	{ 0, }
> +};
> +
> +MODULE_DEVICE_TABLE(pci, nhi_pci_device_ids);
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(DRV_VERSION);
> +
> +static struct pci_driver icm_nhi_driver = {
> +	.name = DRV_NAME,
> +	.id_table = nhi_pci_device_ids,
> +	.probe = icm_nhi_probe,
> +	.remove = icm_nhi_remove,
> +	.shutdown = icm_nhi_shutdown,
> +	.driver.pm = &icm_nhi_pm_ops,
> +};
> +
> +static int __init icm_nhi_init(void)
> +{
> +	int rc;
> +
> +	if (dmi_match(DMI_BOARD_VENDOR, "Apple Inc."))
> +		return -ENODEV;
> +
> +	rc = genl_register_family_with_ops(&nhi_genl_family, nhi_ops);
> +	if (rc)
> +		goto failure;
> +
> +	rc = pci_register_driver(&icm_nhi_driver);
> +	if (rc)
> +		goto failure_genl;
> +
> +	return 0;
> +
> +failure_genl:
> +	genl_unregister_family(&nhi_genl_family);
> +
> +failure:
> +	pr_debug("nhi: error %d occurred in %s\n", rc, __func__);
> +	return rc;
> +}
> +
> +static void __exit icm_nhi_unload(void)
> +{
> +	genl_unregister_family(&nhi_genl_family);
> +	pci_unregister_driver(&icm_nhi_driver);
> +}
> +
> +module_init(icm_nhi_init);
> +module_exit(icm_nhi_unload);
> diff --git a/drivers/thunderbolt/icm/icm_nhi.h
> b/drivers/thunderbolt/icm/icm_nhi.h
> new file mode 100644
> index 0000000..3d9424d
> --- /dev/null
> +++ b/drivers/thunderbolt/icm/icm_nhi.h
> @@ -0,0 +1,84 @@
> +/*********************************************************
> **********************
> + *
> + * 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 <http://www.gnu.org/licenses/>.
> + *
> + * The full GNU General Public License is included in this distribution in
> + * the file called "COPYING".
> + *
> + * Contact Information:
> + * Intel Thunderbolt Mailing List <thunderbolt-software@lists.01.org>
> + * Intel Corporation, 5200 N.E. Elam Young Parkway, Hillsboro, OR 97124-
> 6497
> + *
> +
> **********************************************************
> ********************/
> +
> +#ifndef ICM_NHI_H_
> +#define ICM_NHI_H_
> +
> +#include <linux/pci.h>
> +#include "../nhi_regs.h"
> +
> +#define DRV_VERSION "16.1.51.1"	

Will  you really keep up with versioning of the driver ?

> +#define DRV_NAME "thunderbolt"
> +
> +#define TBT_ICM_RING_MAX_FRAME_SIZE	256
> +#define TBT_ICM_RING_NUM		0
> +#define TBT_RING_MAX_FRM_DATA_SZ
> 	(TBT_RING_MAX_FRAME_SIZE - \
> +					 sizeof(struct tbt_frame_header))
> +
> +enum icm_operation_mode {
> +	SAFE_MODE,
> +	AUTHENTICATION_MODE_FUNCTIONALITY,
> +	ENDPOINT_OPERATION_MODE,
> +	FULL_FUNCTIONALITY,
I would use some OP_MODE_ prefix 
> +};
> +
> +#define TBT_ICM_RING_NUM_TX_BUFS TBT_RING_MIN_NUM_BUFFERS
> +#define TBT_ICM_RING_NUM_RX_BUFS ((PAGE_SIZE -
> (TBT_ICM_RING_NUM_TX_BUFS * \
> +	(sizeof(struct tbt_buf_desc) + TBT_ICM_RING_MAX_FRAME_SIZE)))
> / \
> +	(sizeof(struct tbt_buf_desc) + TBT_ICM_RING_MAX_FRAME_SIZE))
> +
> +/* struct tbt_icm_ring_shared_memory - memory area for DMA */
> +struct tbt_icm_ring_shared_memory {
> +	u8
> tx_buf[TBT_ICM_RING_NUM_TX_BUFS][TBT_ICM_RING_MAX_FRAME_SIZ
> E];
> +	u8
> rx_buf[TBT_ICM_RING_NUM_RX_BUFS][TBT_ICM_RING_MAX_FRAME_SIZ
> E];
> +	struct tbt_buf_desc tx_buf_desc[TBT_ICM_RING_NUM_TX_BUFS];
> +	struct tbt_buf_desc rx_buf_desc[TBT_ICM_RING_NUM_RX_BUFS];
> +} __aligned(TBT_ICM_RING_MAX_FRAME_SIZE);
> +
> +/* mailbox data from SW */
> +#define REG_INMAIL_DATA		0x39900
> +
> +/* mailbox command from SW */
> +#define REG_INMAIL_CMD		0x39904
> +#define REG_INMAIL_CMD_CMD_SHIFT	0
> +#define REG_INMAIL_CMD_CMD_MASK		GENMASK(7,
> REG_INMAIL_CMD_CMD_SHIFT)
> +#define REG_INMAIL_CMD_ERROR		BIT(30)
> +#define REG_INMAIL_CMD_REQUEST		BIT(31)
> +
> +/* mailbox command from FW */
> +#define REG_OUTMAIL_CMD		0x3990C
> +#define REG_OUTMAIL_CMD_STS_SHIFT	0
> +#define REG_OUTMAIL_CMD_STS_MASK	GENMASK(7,
> REG_OUTMAIL_CMD_STS_SHIFT)
> +#define REG_OUTMAIL_CMD_OP_MODE_SHIFT	8
> +#define REG_OUTMAIL_CMD_OP_MODE_MASK	\
> +				GENMASK(11,
> REG_OUTMAIL_CMD_OP_MODE_SHIFT)
> +#define REG_OUTMAIL_CMD_REQUEST		BIT(31)
> +
> +#define REG_FW_STS		0x39944
> +#define REG_FW_STS_ICM_EN		GENMASK(1, 0)
> +#define REG_FW_STS_NVM_AUTH_DONE	BIT(31)
> +
> +#endif
> diff --git a/drivers/thunderbolt/icm/net.h b/drivers/thunderbolt/icm/net.h
> new file mode 100644
> index 0000000..55693ea
> --- /dev/null
> +++ b/drivers/thunderbolt/icm/net.h
> @@ -0,0 +1,200 @@
> +/*********************************************************
> **********************
> + *
> + * 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 <http://www.gnu.org/licenses/>.
> + *
> + * The full GNU General Public License is included in this distribution in
> + * the file called "COPYING".
> + *
> + * Contact Information:
> + * Intel Thunderbolt Mailing List <thunderbolt-software@lists.01.org>
> + * Intel Corporation, 5200 N.E. Elam Young Parkway, Hillsboro, OR 97124-
> 6497
> + *
> +
> **********************************************************
> ********************/
> +
> +#ifndef NET_H_
> +#define NET_H_
> +
> +#include <linux/pci.h>
> +#include <linux/netdevice.h>
> +#include <linux/mutex.h>
> +#include <linux/semaphore.h>
> +#include <net/genetlink.h>
> +
> +/*
> + * Each physical port contains 2 channels.
> + * Devices are exposed to user based on physical ports.
> + */
> +#define CHANNELS_PER_PORT_NUM 2
> +/*
> + * Calculate host physical port number (Zero-based numbering) from
> + * host channel/link which starts from 1.
> + */
> +#define PORT_NUM_FROM_LINK(link) (((link) - 1) /
> CHANNELS_PER_PORT_NUM)
> +
> +#define TBT_TX_RING_FULL(prod, cons, size) ((((prod) + 1) % (size)) ==
> (cons))
> +#define TBT_TX_RING_EMPTY(prod, cons) ((prod) == (cons))
> +#define TBT_RX_RING_FULL(prod, cons) ((prod) == (cons))
> +#define TBT_RX_RING_EMPTY(prod, cons, size) ((((cons) + 1) % (size)) ==
> (prod))
> +
> +#define PATH_FROM_PORT(num_paths, port_num) (((num_paths) - 1) -
> (port_num))
> +
> +/* PDF values for SW<->FW communication in raw mode */
> +enum pdf_value {
> +	PDF_READ_CONFIGURATION_REGISTERS = 1,
> +	PDF_WRITE_CONFIGURATION_REGISTERS,
> +	PDF_ERROR_NOTIFICATION,
> +	PDF_ERROR_ACKNOWLEDGMENT,
> +	PDF_PLUG_EVENT_NOTIFICATION,
> +	PDF_INTER_DOMAIN_REQUEST,
> +	PDF_INTER_DOMAIN_RESPONSE,
> +	PDF_CM_OVERRIDE,
> +	PDF_RESET_CIO_SWITCH,
> +	PDF_FW_TO_SW_NOTIFICATION,
> +	PDF_SW_TO_FW_COMMAND,
> +	PDF_FW_TO_SW_RESPONSE
> +};
> +
> +/*
> + * SW->FW commands
> + * CC = Command Code
> + */
> +enum {
> +	CC_GET_THUNDERBOLT_TOPOLOGY = 1,
> +	CC_GET_VIDEO_RESOURCES_DATA,
> +	CC_DRV_READY,
> +	CC_APPROVE_PCI_CONNECTION,
> +	CC_CHALLENGE_PCI_CONNECTION,
> +	CC_ADD_DEVICE_AND_KEY,
> +	CC_APPROVE_INTER_DOMAIN_CONNECTION = 0x10
> +};
> +
> +/*
> + * SW -> FW mailbox commands
> + * CC = Command Code
> + */
> +enum {
> +	CC_STOP_CM_ACTIVITY,
> +	CC_ENTER_PASS_THROUGH_MODE,
> +	CC_ENTER_CM_OWNERSHIP_MODE,
> +	CC_DRV_LOADED,
> +	CC_DRV_UNLOADED,
> +	CC_SAVE_CURRENT_CONNECTED_DEVICES,
> +	CC_DISCONNECT_PCIE_PATHS,
> +	CC_DRV_UNLOADS_AND_DISCONNECT_INTER_DOMAIN_PATHS,
> +	DISCONNECT_PORT_A_INTER_DOMAIN_PATH = 0x10,
> +	DISCONNECT_PORT_B_INTER_DOMAIN_PATH,
> +	DP_TUNNEL_MODE_IN_ORDER_PER_CAPABILITIES = 0x1E,
> +	DP_TUNNEL_MODE_MAXIMIZE_SNK_SRC_TUNNELS,
> +	CC_SET_FW_MODE_FD1_D1_CERT = 0x20,
> +	CC_SET_FW_MODE_FD1_D1_ALL,
> +	CC_SET_FW_MODE_FD1_DA_CERT,
> +	CC_SET_FW_MODE_FD1_DA_ALL,
> +	CC_SET_FW_MODE_FDA_D1_CERT,
> +	CC_SET_FW_MODE_FDA_D1_ALL,
> +	CC_SET_FW_MODE_FDA_DA_CERT,
> +	CC_SET_FW_MODE_FDA_DA_ALL
> +};
> +
> +
> +/* NHI genetlink attributes */
> +enum {
> +	NHI_ATTR_UNSPEC,
> +	NHI_ATTR_DRV_VERSION,
> +	NHI_ATTR_NVM_VER_OFFSET,
> +	NHI_ATTR_NUM_PORTS,
> +	NHI_ATTR_DMA_PORT,
> +	NHI_ATTR_SUPPORT_FULL_E2E,
> +	NHI_ATTR_MAILBOX_CMD,
> +	NHI_ATTR_PDF,
> +	NHI_ATTR_MSG_TO_ICM,
> +	NHI_ATTR_MSG_FROM_ICM,
> +	__NHI_ATTR_MAX,
> +};
> +#define NHI_ATTR_MAX (__NHI_ATTR_MAX - 1)
> +
> +struct port_net_dev {
> +	struct net_device *net_dev;
> +	struct mutex state_mutex;
> +};
> +
> +/**
> + *  struct tbt_nhi_ctxt - thunderbolt native host interface context
> + *  @node:				node in the controllers list.
> + *  @pdev:				pci device information.
> + *  @iobase:				address of I/O.
> + *  @msix_entries:			MSI-X vectors.
> + *  @icm_ring_shared_mem:		virtual address of iCM ring.
> + *  @icm_ring_shared_mem_dma_addr:	DMA addr of iCM ring.
> + *  @send_sem:				semaphore for sending
> messages to iCM
> + *					one at a time.
> + *  @mailbox_mutex:			mutex for sending mailbox
> commands to
> + *					iCM one at a time.
> + *  @d0_exit_send_mutex:		synchronizing the d0 exit with
> messages.
> + *  @d0_exit_mailbox_mutex:		synchronizing the d0 exit with
> mailbox.
> + *  @lock:				synchronizing the interrupt registers
> + *					access.
> + *  @icm_msgs_work:			work queue for handling messages
> + *					from iCM.
> + *  @net_devices:			net devices per port.
> + *  @net_workqueue:			work queue to send net messages.
> + *  @id:				id of the controller.
> + *  @num_paths:				number of paths supported
> by controller.
> + *  @nvm_ver_offset:			offset of NVM version in NVM.
> + *  @num_vectors:			number of MSI-X vectors.
> + *  @num_ports:				number of ports in the
> controller.
> + *  @dma_port:				DMA port.
> + *  @d0_exit:				whether controller exit D0 state.
> + *  @nvm_auth_on_boot:			whether iCM authenticates
> the NVM
> + *					during boot.
> + *  @wait_for_icm_resp:			whether to wait for iCM
> response.
> + *  @ignore_icm_resp:			whether to ignore iCM
> response.
> + *  @pci_using_dac:			whether using DAC.
> + *  @support_full_e2e:			whether controller support
> full E2E.
> + */
> +struct tbt_nhi_ctxt {
> +	struct list_head node;
> +	struct pci_dev *pdev;
> +	void __iomem *iobase;
> +	struct msix_entry *msix_entries;
> +	struct tbt_icm_ring_shared_memory *icm_ring_shared_mem;
> +	dma_addr_t icm_ring_shared_mem_dma_addr;
> +	struct semaphore send_sem;
> +	struct mutex mailbox_mutex;
> +	struct mutex d0_exit_send_mutex;
> +	struct mutex d0_exit_mailbox_mutex;
> +	spinlock_t lock;
> +	struct work_struct icm_msgs_work;
> +	struct port_net_dev *net_devices;
> +	struct workqueue_struct *net_workqueue;
> +	u32 id;
> +	u32 num_paths;
> +	u16 nvm_ver_offset;
> +	u8 num_vectors;
> +	u8 num_ports;
> +	u8 dma_port;
> +	bool d0_exit;
> +	bool nvm_auth_on_boot : 1;
Don't use bool with bit fields use u32 
> +	bool wait_for_icm_resp : 1;
> +	bool ignore_icm_resp : 1;
> +	bool pci_using_dac : 1;
> +	bool support_full_e2e : 1;
> +};
> +
> +int nhi_send_message(struct tbt_nhi_ctxt *nhi_ctxt, enum pdf_value pdf,
> +		      u32 msg_len, const u8 *msg, bool ignore_icm_resp);
> +int nhi_mailbox(struct tbt_nhi_ctxt *nhi_ctxt, u32 cmd, u32 data, bool
> deinit);
> +
> +#endif
> --
> 2.7.4

Hi Tomas,
Thanks for your comments.

On Thu, Jul 14 2016, 03:44 PM, Winkler, Tomas wrote:
> > +/* 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,
> > +	NHI_CMD_APPROVE_TBT_NETWORKING,
> > +	NHI_CMD_ICM_IN_SAFE_MODE,
> > +	__NHI_CMD_MAX,
> > +};
> > +#define NHI_CMD_MAX (__NHI_CMD_MAX - 1)
> NHI_CMD_MAX  = NHI_CMD_ICM_IN_SAFE_MODE ?
> 

This template is used a lot with (generic) netlink. 
Few examples:
http://lxr.free-electrons.com/source/drivers/acpi/event.c#L62
http://lxr.free-electrons.com/source/include/uapi/linux/irda.h#L224
It is easier to maintain - adding entry in the list will automatically update MAX.

[...]

> > +		u32 status;
> > +
> > +		status = ioread32(nhi_ctxt->iobase + REG_FW_STS);
> > +
> > +		if (status & REG_FW_STS_NVM_AUTH_DONE)
> > +			break;
> > +		msleep(30);
> 
> 30 is  big number, for polling, what is behind this?
> 

The NVM authentication can take time for ICM.
This number comes from experiments.

[...]

> > +static struct tbt_nhi_ctxt *nhi_search_ctxt(u32 id)
> > +{
> > +	struct tbt_nhi_ctxt *nhi_ctxt;
> > +
> > +	list_for_each_entry(nhi_ctxt, &controllers_list, node)
> > +		if (nhi_ctxt->id == id)
> > +			return nhi_ctxt;
> 
> Don't you need to lock this list with the controllers_list_rwsem ?
> 

This is a helper function for searching the list.
The callers take and release the lock.

[...]

> > +	bool nvm_auth_on_boot : 1;
> Don't use bool with bit fields use u32 

What is the concern here?
If it is size of bool, it doesn't matter for this struct.
It is more readable that the expected value is bool,
and it is used a lot in kernel, for example:
http://lxr.free-electrons.com/source/include/linux/pm.h#L558

Hi Amir,
Here are my 2 cents:

This method always returns true, should be void (unless you will change PDF_ERROR_NOTIFICATION  or other pdf values to return false), and  likewise its invocation should not check return value.

> +static bool nhi_msg_from_icm_analysis(struct tbt_nhi_ctxt *nhi_ctxt,
> +					enum pdf_value pdf,
> +					const u8 *msg, u32 msg_len)
> +{
> +	/*
> +	 * preparation for messages that won't be sent,
> +	 * currently unused in this patch.
> +	 */
> +	bool send_event = true;
> +
> +	switch (pdf) {
> +	case PDF_ERROR_NOTIFICATION:
> +		dev_err(&nhi_ctxt->pdev->dev,
> +			"controller id %#x PDF_ERROR_NOTIFICATION %hhu
> msg len %u\n",
> +			nhi_ctxt->id, msg[11], msg_len);
> +		/* fallthrough */
> +	case PDF_WRITE_CONFIGURATION_REGISTERS:
> +		/* fallthrough */
> +	case PDF_READ_CONFIGURATION_REGISTERS:
> +		if (nhi_ctxt->wait_for_icm_resp) {
> +			nhi_ctxt->wait_for_icm_resp = false;
> +			up(&nhi_ctxt->send_sem);
> +		}
> +		break;
> +
> +	case PDF_FW_TO_SW_RESPONSE:
> +		if (nhi_ctxt->wait_for_icm_resp) {
> +			nhi_ctxt->wait_for_icm_resp = false;
> +			up(&nhi_ctxt->send_sem);
> +		}
> +		break;
> +
> +	default:
> +		dev_warn(&nhi_ctxt->pdev->dev,
> +			 "controller id %#x pdf %u isn't handled/expected\n",
> +			 nhi_ctxt->id, pdf);
> +		break;
> +	}
> +
> +	return send_event;
> +}
> +

This methods always returns 0, should be void.

> +static int nhi_suspend(struct device *dev) __releases(&nhi_ctxt-
> >send_sem)
> +{
> +	struct tbt_nhi_ctxt *nhi_ctxt = pci_get_drvdata(to_pci_dev(dev));
> +	void __iomem *rx_reg, *tx_reg;
> +	u32 rx_reg_val, tx_reg_val;
> +
> +	/* must be after negotiation_events, since messages might be sent
> */
> +	nhi_ctxt->d0_exit = true;
> +
> +	rx_reg = nhi_ctxt->iobase + REG_RX_OPTIONS_BASE +
> +		 (TBT_ICM_RING_NUM * REG_OPTS_STEP);
> +	rx_reg_val = ioread32(rx_reg) & ~REG_OPTS_E2E_EN;
> +	tx_reg = nhi_ctxt->iobase + REG_TX_OPTIONS_BASE +
> +		 (TBT_ICM_RING_NUM * REG_OPTS_STEP);
> +	tx_reg_val = ioread32(tx_reg) & ~REG_OPTS_E2E_EN;
> +	/* disable RX flow control  */
> +	iowrite32(rx_reg_val, rx_reg);
> +	/* disable TX flow control  */
> +	iowrite32(tx_reg_val, tx_reg);
> +	/* disable RX ring  */
> +	iowrite32(rx_reg_val & ~REG_OPTS_VALID, rx_reg);
> +
> +	mutex_lock(&nhi_ctxt->d0_exit_mailbox_mutex);
> +	mutex_lock(&nhi_ctxt->d0_exit_send_mutex);
> +
> +	cancel_work_sync(&nhi_ctxt->icm_msgs_work);
> +
> +	if (nhi_ctxt->wait_for_icm_resp) {
> +		nhi_ctxt->wait_for_icm_resp = false;
> +		nhi_ctxt->ignore_icm_resp = false;
> +		/*
> +		 * if there is response, it is lost, so unlock the send
> +		 * for the next resume.
> +		 */
> +		up(&nhi_ctxt->send_sem);
> +	}
> +
> +	mutex_unlock(&nhi_ctxt->d0_exit_send_mutex);
> +	mutex_unlock(&nhi_ctxt->d0_exit_mailbox_mutex);
> +
> +	/* wait for all TX to finish  */
> +	usleep_range(5 * USEC_PER_MSEC, 7 * USEC_PER_MSEC);
> +
> +	/* disable all interrupts */
> +	iowrite32(0, nhi_ctxt->iobase + REG_RING_INTERRUPT_BASE);
> +	/* disable TX ring  */
> +	iowrite32(tx_reg_val & ~REG_OPTS_VALID, tx_reg);
> +
> +	return 0;
> +}
> +

This methods also always returns 0, should be void.

> +static int nhi_resume(struct device *dev) __acquires(&nhi_ctxt-
> >send_sem)
> +{
> +	dma_addr_t phys;
> +	struct tbt_nhi_ctxt *nhi_ctxt = pci_get_drvdata(to_pci_dev(dev));
> +	struct tbt_buf_desc *desc;
> +	void __iomem *iobase = nhi_ctxt->iobase;
> +	void __iomem *reg;
> +	int i;
> +
> +	if (nhi_ctxt->msix_entries) {
> +		iowrite32(ioread32(iobase + REG_DMA_MISC) |
> +
> 	REG_DMA_MISC_INT_AUTO_CLEAR,
> +			  iobase + REG_DMA_MISC);
> +		/*
> +		 * Vector #0, which is TX complete to ICM,
> +		 * isn't been used currently.
> +		 */
> +		nhi_set_int_vec(nhi_ctxt, 0, 1);
> +
> +		for (i = 2; i < nhi_ctxt->num_vectors; i++)
> +			nhi_set_int_vec(nhi_ctxt, nhi_ctxt->num_paths -
> (i/2),
> +					i);
> +	}
> +
> +	/* configure TX descriptors */
> +	for (i = 0, phys = nhi_ctxt->icm_ring_shared_mem_dma_addr;
> +	     i < TBT_ICM_RING_NUM_TX_BUFS;
> +	     i++, phys += TBT_ICM_RING_MAX_FRAME_SIZE) {
> +		desc = &nhi_ctxt->icm_ring_shared_mem->tx_buf_desc[i];
> +		desc->phys = cpu_to_le64(phys);
> +		desc->attributes = cpu_to_le32(DESC_ATTR_REQ_STS);
> +	}
> +	/* configure RX descriptors */
> +	for (i = 0;
> +	     i < TBT_ICM_RING_NUM_RX_BUFS;
> +	     i++, phys += TBT_ICM_RING_MAX_FRAME_SIZE) {
> +		desc = &nhi_ctxt->icm_ring_shared_mem->rx_buf_desc[i];
> +		desc->phys = cpu_to_le64(phys);
> +		desc->attributes = cpu_to_le32(DESC_ATTR_REQ_STS |
> +					       DESC_ATTR_INT_EN);
> +	}
> +
> +	/* configure throttling rate for interrupts */
> +	for (i = 0, reg = iobase + REG_INT_THROTTLING_RATE;
> +	     i < NUM_INT_VECTORS;
> +	     i++, reg += REG_INT_THROTTLING_RATE_STEP) {
> +		iowrite32(USEC_TO_256_NSECS(128), reg);
> +	}
> +
> +	/* configure TX for ICM ring */
> +	reg = iobase + REG_TX_RING_BASE + (TBT_ICM_RING_NUM *
> REG_RING_STEP);
> +	phys = nhi_ctxt->icm_ring_shared_mem_dma_addr +
> +		offsetof(struct tbt_icm_ring_shared_memory, tx_buf_desc);
> +	iowrite32(lower_32_bits(phys), reg + REG_RING_PHYS_LO_OFFSET);
> +	iowrite32(upper_32_bits(phys), reg + REG_RING_PHYS_HI_OFFSET);
> +	iowrite32((TBT_ICM_RING_NUM_TX_BUFS <<
> REG_RING_SIZE_SHIFT) &
> +			REG_RING_SIZE_MASK,
> +		  reg + REG_RING_SIZE_OFFSET);
> +
> +	reg = iobase + REG_TX_OPTIONS_BASE +
> (TBT_ICM_RING_NUM*REG_OPTS_STEP);
> +	iowrite32(REG_OPTS_RAW | REG_OPTS_VALID, reg);
> +
> +	/* configure RX for ICM ring */
> +	reg = iobase + REG_RX_RING_BASE + (TBT_ICM_RING_NUM *
> REG_RING_STEP);
> +	phys = nhi_ctxt->icm_ring_shared_mem_dma_addr +
> +		offsetof(struct tbt_icm_ring_shared_memory, rx_buf_desc);
> +	iowrite32(lower_32_bits(phys), reg + REG_RING_PHYS_LO_OFFSET);
> +	iowrite32(upper_32_bits(phys), reg + REG_RING_PHYS_HI_OFFSET);
> +	iowrite32(((TBT_ICM_RING_NUM_RX_BUFS <<
> REG_RING_SIZE_SHIFT) &
> +			REG_RING_SIZE_MASK) |
> +		  ((TBT_ICM_RING_MAX_FRAME_SIZE <<
> REG_RING_BUF_SIZE_SHIFT) &
> +			REG_RING_BUF_SIZE_MASK),
> +		  reg + REG_RING_SIZE_OFFSET);
> +	iowrite32(((TBT_ICM_RING_NUM_RX_BUFS - 1) <<
> REG_RING_CONS_SHIFT) &
> +			REG_RING_CONS_MASK,
> +		  reg + REG_RING_CONS_PROD_OFFSET);
> +
> +	reg = iobase + REG_RX_OPTIONS_BASE +
> (TBT_ICM_RING_NUM*REG_OPTS_STEP);
> +	iowrite32(REG_OPTS_RAW | REG_OPTS_VALID, reg);
> +
> +	/* enable RX interrupt */
> +	RING_INT_ENABLE_RX(iobase, TBT_ICM_RING_NUM, nhi_ctxt-
> >num_paths);
> +
> +	if (likely((atomic_read(&subscribers) > 0) &&
> +		   nhi_nvm_authenticated(nhi_ctxt))) {
> +		down(&nhi_ctxt->send_sem);
> +		nhi_ctxt->d0_exit = false;
> +		mutex_lock(&nhi_ctxt->d0_exit_send_mutex);
> +		/*
> +		 * interrupts are enabled here before send due to
> +		 * implicit barrier in mutex
> +		 */
> +		nhi_send_driver_ready_command(nhi_ctxt);
> +		mutex_unlock(&nhi_ctxt->d0_exit_send_mutex);
> +	} else {
> +		nhi_ctxt->d0_exit = false;
> +	}
> +
> +	return 0;
> +}


Regards,
Rami Rosen
Intel Corporation

On Thu, Jul 14 2016, 06:08 PM, Rosen, Rami wrote:
> Hi Amir,

Hi Rami,

> Here are my 2 cents:
> 
> This method always returns true, should be void (unless you will change
> PDF_ERROR_NOTIFICATION  or other pdf values to return false), and
> likewise its invocation should not check return value.
> 

This patch is the communication with the FW.
The network functionality is added in the next patches in the series 
and with it, more messages from FW.
Indeed this function always returns true in this patch, 
but while writing it, I predicted that the network functionality will 
use the send_event flag differently.
You can see the documentation of send_event - currently unused in this patch.
I don't see any harm that in this patch, this function will always return true,
while applying the rest of the series will change it.

> > +static bool nhi_msg_from_icm_analysis(struct tbt_nhi_ctxt *nhi_ctxt,
> > +					enum pdf_value pdf,
> > +					const u8 *msg, u32 msg_len)
> > +{
> > +	/*
> > +	 * preparation for messages that won't be sent,
> > +	 * currently unused in this patch.
> > +	 */
> > +	bool send_event = true;
> > +
> > +	switch (pdf) {
> > +	case PDF_ERROR_NOTIFICATION:
> > +		dev_err(&nhi_ctxt->pdev->dev,
> > +			"controller id %#x PDF_ERROR_NOTIFICATION %hhu
> > msg len %u\n",
> > +			nhi_ctxt->id, msg[11], msg_len);
> > +		/* fallthrough */
> > +	case PDF_WRITE_CONFIGURATION_REGISTERS:
> > +		/* fallthrough */
> > +	case PDF_READ_CONFIGURATION_REGISTERS:
> > +		if (nhi_ctxt->wait_for_icm_resp) {
> > +			nhi_ctxt->wait_for_icm_resp = false;
> > +			up(&nhi_ctxt->send_sem);
> > +		}
> > +		break;
> > +
> > +	case PDF_FW_TO_SW_RESPONSE:
> > +		if (nhi_ctxt->wait_for_icm_resp) {
> > +			nhi_ctxt->wait_for_icm_resp = false;
> > +			up(&nhi_ctxt->send_sem);
> > +		}
> > +		break;
> > +
> > +	default:
> > +		dev_warn(&nhi_ctxt->pdev->dev,
> > +			 "controller id %#x pdf %u isn't handled/expected\n",
> > +			 nhi_ctxt->id, pdf);
> > +		break;
> > +	}
> > +
> > +	return send_event;
> > +}
> > +
> 
> This methods always returns 0, should be void.
> 

The prototype of suspend is return int:
http://lxr.free-electrons.com/source/include/linux/pm.h#L295

> > +static int nhi_suspend(struct device *dev) __releases(&nhi_ctxt-
> > >send_sem)
> > +{
> > +	struct tbt_nhi_ctxt *nhi_ctxt = pci_get_drvdata(to_pci_dev(dev));
> > +	void __iomem *rx_reg, *tx_reg;
> > +	u32 rx_reg_val, tx_reg_val;
> > +
> > +	/* must be after negotiation_events, since messages might be sent
> > */
> > +	nhi_ctxt->d0_exit = true;
> > +
> > +	rx_reg = nhi_ctxt->iobase + REG_RX_OPTIONS_BASE +
> > +		 (TBT_ICM_RING_NUM * REG_OPTS_STEP);
> > +	rx_reg_val = ioread32(rx_reg) & ~REG_OPTS_E2E_EN;
> > +	tx_reg = nhi_ctxt->iobase + REG_TX_OPTIONS_BASE +
> > +		 (TBT_ICM_RING_NUM * REG_OPTS_STEP);
> > +	tx_reg_val = ioread32(tx_reg) & ~REG_OPTS_E2E_EN;
> > +	/* disable RX flow control  */
> > +	iowrite32(rx_reg_val, rx_reg);
> > +	/* disable TX flow control  */
> > +	iowrite32(tx_reg_val, tx_reg);
> > +	/* disable RX ring  */
> > +	iowrite32(rx_reg_val & ~REG_OPTS_VALID, rx_reg);
> > +
> > +	mutex_lock(&nhi_ctxt->d0_exit_mailbox_mutex);
> > +	mutex_lock(&nhi_ctxt->d0_exit_send_mutex);
> > +
> > +	cancel_work_sync(&nhi_ctxt->icm_msgs_work);
> > +
> > +	if (nhi_ctxt->wait_for_icm_resp) {
> > +		nhi_ctxt->wait_for_icm_resp = false;
> > +		nhi_ctxt->ignore_icm_resp = false;
> > +		/*
> > +		 * if there is response, it is lost, so unlock the send
> > +		 * for the next resume.
> > +		 */
> > +		up(&nhi_ctxt->send_sem);
> > +	}
> > +
> > +	mutex_unlock(&nhi_ctxt->d0_exit_send_mutex);
> > +	mutex_unlock(&nhi_ctxt->d0_exit_mailbox_mutex);
> > +
> > +	/* wait for all TX to finish  */
> > +	usleep_range(5 * USEC_PER_MSEC, 7 * USEC_PER_MSEC);
> > +
> > +	/* disable all interrupts */
> > +	iowrite32(0, nhi_ctxt->iobase + REG_RING_INTERRUPT_BASE);
> > +	/* disable TX ring  */
> > +	iowrite32(tx_reg_val & ~REG_OPTS_VALID, tx_reg);
> > +
> > +	return 0;
> > +}
> > +
> 
> This methods also always returns 0, should be void.
> 

The prototype of resume is return int:
http://lxr.free-electrons.com/source/include/linux/pm.h#L295

> > +static int nhi_resume(struct device *dev) __acquires(&nhi_ctxt-
> > >send_sem)
> > +{
> > +	dma_addr_t phys;
> > +	struct tbt_nhi_ctxt *nhi_ctxt = pci_get_drvdata(to_pci_dev(dev));
> > +	struct tbt_buf_desc *desc;
> > +	void __iomem *iobase = nhi_ctxt->iobase;
> > +	void __iomem *reg;
> > +	int i;
> > +
> > +	if (nhi_ctxt->msix_entries) {
> > +		iowrite32(ioread32(iobase + REG_DMA_MISC) |
> > +
> > 	REG_DMA_MISC_INT_AUTO_CLEAR,
> > +			  iobase + REG_DMA_MISC);
> > +		/*
> > +		 * Vector #0, which is TX complete to ICM,
> > +		 * isn't been used currently.
> > +		 */
> > +		nhi_set_int_vec(nhi_ctxt, 0, 1);
> > +
> > +		for (i = 2; i < nhi_ctxt->num_vectors; i++)
> > +			nhi_set_int_vec(nhi_ctxt, nhi_ctxt->num_paths -
> > (i/2),
> > +					i);
> > +	}
> > +
> > +	/* configure TX descriptors */
> > +	for (i = 0, phys = nhi_ctxt->icm_ring_shared_mem_dma_addr;
> > +	     i < TBT_ICM_RING_NUM_TX_BUFS;
> > +	     i++, phys += TBT_ICM_RING_MAX_FRAME_SIZE) {
> > +		desc = &nhi_ctxt->icm_ring_shared_mem->tx_buf_desc[i];
> > +		desc->phys = cpu_to_le64(phys);
> > +		desc->attributes = cpu_to_le32(DESC_ATTR_REQ_STS);
> > +	}
> > +	/* configure RX descriptors */
> > +	for (i = 0;
> > +	     i < TBT_ICM_RING_NUM_RX_BUFS;
> > +	     i++, phys += TBT_ICM_RING_MAX_FRAME_SIZE) {
> > +		desc = &nhi_ctxt->icm_ring_shared_mem->rx_buf_desc[i];
> > +		desc->phys = cpu_to_le64(phys);
> > +		desc->attributes = cpu_to_le32(DESC_ATTR_REQ_STS |
> > +					       DESC_ATTR_INT_EN);
> > +	}
> > +
> > +	/* configure throttling rate for interrupts */
> > +	for (i = 0, reg = iobase + REG_INT_THROTTLING_RATE;
> > +	     i < NUM_INT_VECTORS;
> > +	     i++, reg += REG_INT_THROTTLING_RATE_STEP) {
> > +		iowrite32(USEC_TO_256_NSECS(128), reg);
> > +	}
> > +
> > +	/* configure TX for ICM ring */
> > +	reg = iobase + REG_TX_RING_BASE + (TBT_ICM_RING_NUM *
> > REG_RING_STEP);
> > +	phys = nhi_ctxt->icm_ring_shared_mem_dma_addr +
> > +		offsetof(struct tbt_icm_ring_shared_memory, tx_buf_desc);
> > +	iowrite32(lower_32_bits(phys), reg + REG_RING_PHYS_LO_OFFSET);
> > +	iowrite32(upper_32_bits(phys), reg + REG_RING_PHYS_HI_OFFSET);
> > +	iowrite32((TBT_ICM_RING_NUM_TX_BUFS <<
> > REG_RING_SIZE_SHIFT) &
> > +			REG_RING_SIZE_MASK,
> > +		  reg + REG_RING_SIZE_OFFSET);
> > +
> > +	reg = iobase + REG_TX_OPTIONS_BASE +
> > (TBT_ICM_RING_NUM*REG_OPTS_STEP);
> > +	iowrite32(REG_OPTS_RAW | REG_OPTS_VALID, reg);
> > +
> > +	/* configure RX for ICM ring */
> > +	reg = iobase + REG_RX_RING_BASE + (TBT_ICM_RING_NUM *
> > REG_RING_STEP);
> > +	phys = nhi_ctxt->icm_ring_shared_mem_dma_addr +
> > +		offsetof(struct tbt_icm_ring_shared_memory, rx_buf_desc);
> > +	iowrite32(lower_32_bits(phys), reg + REG_RING_PHYS_LO_OFFSET);
> > +	iowrite32(upper_32_bits(phys), reg + REG_RING_PHYS_HI_OFFSET);
> > +	iowrite32(((TBT_ICM_RING_NUM_RX_BUFS <<
> > REG_RING_SIZE_SHIFT) &
> > +			REG_RING_SIZE_MASK) |
> > +		  ((TBT_ICM_RING_MAX_FRAME_SIZE <<
> > REG_RING_BUF_SIZE_SHIFT) &
> > +			REG_RING_BUF_SIZE_MASK),
> > +		  reg + REG_RING_SIZE_OFFSET);
> > +	iowrite32(((TBT_ICM_RING_NUM_RX_BUFS - 1) <<
> > REG_RING_CONS_SHIFT) &
> > +			REG_RING_CONS_MASK,
> > +		  reg + REG_RING_CONS_PROD_OFFSET);
> > +
> > +	reg = iobase + REG_RX_OPTIONS_BASE +
> > (TBT_ICM_RING_NUM*REG_OPTS_STEP);
> > +	iowrite32(REG_OPTS_RAW | REG_OPTS_VALID, reg);
> > +
> > +	/* enable RX interrupt */
> > +	RING_INT_ENABLE_RX(iobase, TBT_ICM_RING_NUM, nhi_ctxt-
> > >num_paths);
> > +
> > +	if (likely((atomic_read(&subscribers) > 0) &&
> > +		   nhi_nvm_authenticated(nhi_ctxt))) {
> > +		down(&nhi_ctxt->send_sem);
> > +		nhi_ctxt->d0_exit = false;
> > +		mutex_lock(&nhi_ctxt->d0_exit_send_mutex);
> > +		/*
> > +		 * interrupts are enabled here before send due to
> > +		 * implicit barrier in mutex
> > +		 */
> > +		nhi_send_driver_ready_command(nhi_ctxt);
> > +		mutex_unlock(&nhi_ctxt->d0_exit_send_mutex);
> > +	} else {
> > +		nhi_ctxt->d0_exit = false;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> 
> Regards,
> Rami Rosen
> Intel Corporation

Hi, Amir,

>This patch is the communication with the FW.
>The network functionality is added in the next patches in the series 
>and with it, more messages from FW.
>Indeed this function always returns true in this patch, 
>but while writing it, I predicted that the network functionality will 
>use the send_event flag differently.
>You can see the documentation of send_event - currently unused in this patch.
>I don't see any harm that in this patch, this function will always return true,
>while applying the rest of the series will change it.

Ok, seems reasonable.

>The prototype of suspend is return int:
>http://lxr.free-electrons.com/source/include/linux/pm.h#L295

>The prototype of resume is return int:
>http://lxr.free-electrons.com/source/include/linux/pm.h#L295

You are right about this, I missed the macro invocation in your code, SET_SYSTEM_SLEEP_PM_OPS(nhi_suspend, nhi_resume),
apologies.

 Regards,
 Rami Rosen
 Intel Corporation

> -----Original Message-----
> From: Levy, Amir (Jer)
> Sent: Thursday, July 14, 2016 17:50
> To: Winkler, Tomas <tomas.winkler@intel.com>;
> andreas.noever@gmail.com; gregkh@linuxfoundation.org;
> bhelgaas@google.com
> Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org;
> netdev@vger.kernel.org; thunderbolt-linux <thunderbolt-linux@intel.com>;
> Westerberg, Mika <mika.westerberg@intel.com>
> Subject: RE: [PATCH v3 5/8] thunderbolt: Communication with the ICM
> (firmware)
> 
> Hi Tomas,
> Thanks for your comments.
> 
> On Thu, Jul 14 2016, 03:44 PM, Winkler, Tomas wrote:
> > > +/* 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,
> > > +	NHI_CMD_APPROVE_TBT_NETWORKING,
> > > +	NHI_CMD_ICM_IN_SAFE_MODE,
> > > +	__NHI_CMD_MAX,
> > > +};
> > > +#define NHI_CMD_MAX (__NHI_CMD_MAX - 1)
> > NHI_CMD_MAX  = NHI_CMD_ICM_IN_SAFE_MODE ?
> >
> 
> This template is used a lot with (generic) netlink.
> Few examples:
> http://lxr.free-electrons.com/source/drivers/acpi/event.c#L62
> http://lxr.free-electrons.com/source/include/uapi/linux/irda.h#L224
> It is easier to maintain - adding entry in the list will automatically update
> MAX.

Fair enough. 

> [...]
> 
> > > +		u32 status;
> > > +
> > > +		status = ioread32(nhi_ctxt->iobase + REG_FW_STS);
> > > +
> > > +		if (status & REG_FW_STS_NVM_AUTH_DONE)
> > > +			break;
> > > +		msleep(30);
> >
> > 30 is  big number, for polling, what is behind this?
> >
> 
> The NVM authentication can take time for ICM.
> This number comes from experiments.

This deserve some comment, this look very random. 
 
> [...]
> 
> > > +static struct tbt_nhi_ctxt *nhi_search_ctxt(u32 id) {
> > > +	struct tbt_nhi_ctxt *nhi_ctxt;
> > > +
> > > +	list_for_each_entry(nhi_ctxt, &controllers_list, node)
> > > +		if (nhi_ctxt->id == id)
> > > +			return nhi_ctxt;
> >
> > Don't you need to lock this list with the controllers_list_rwsem ?
> >
> 
> This is a helper function for searching the list.
> The callers take and release the lock.

Since this doesn't fit the patterns in your code, the function deserves a comment, that it should be used under lock. 

> 
> [...]
> 
> > > +	bool nvm_auth_on_boot : 1;
> > Don't use bool with bit fields use u32
> 
> What is the concern here?
> If it is size of bool, it doesn't matter for this struct.
> It is more readable that the expected value is bool, and it is used a lot in
> kernel, for example:
> http://lxr.free-electrons.com/source/include/linux/pm.h#L558

Hmm, actually a nice feature I wasn't aware of, save some space and got bool type checking. 

Thanks