On Tue, Sep 27, 2016 at 04:43:37PM +0300, Amir Levy wrote:

Why is this sentence needed?

+ *
+ * The full GNU General Public License is included in this distribution in
+ * the file called "COPYING".

Why is this sentence needed?

+ *
+ * Contact Information:
+ * Intel Thunderbolt Mailing List [off-list ref]

Shouldn't this just be in the MAINTAINERS file?

+ * Intel Corporation, 5200 N.E. Elam Young Parkway, Hillsboro, OR 97124-6497

Unless you are going to track the address of Intel for the next 40+
years, don't put it in a source comment.  Please just drop it.

+ *
+ ******************************************************************************/
+
+#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 "thunderbolt"
+
+#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,
+	NHI_CMD_APPROVE_TBT_NETWORKING,
+	NHI_CMD_ICM_IN_SAFE_MODE,
+	__NHI_CMD_MAX,
+};
+#define NHI_CMD_MAX (__NHI_CMD_MAX - 1)
+
+/* 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 DEFINE_MUTEX(controllers_list_mutex);
+static atomic_t subscribers = ATOMIC_INIT(0);
+static u32 portid;

This is really odd, why have a global single 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;
+
+	/*
+	 * The check for NVM authentication can take time for iCM,
+	 * especially in low power configuration.
+	 */
+	for (i = 0; i < 5; i++) {
+		u32 status = ioread32(nhi_ctxt->iobase + REG_FW_STS);
+
+		if (status & REG_FW_STS_NVM_AUTH_DONE)
+			break;
+
+		msleep(30);
+	}
+	/*
+	 * 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 void *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, ((u8 *)msg)[3], msg_len);

Don't put trace debug calls in your code, either use a real tracepoint,
or just use ftrace.  Delete these, they aren't needed once you want to
submit the code for merging as it should not be needed.

You do this in a lot of other places as well, please fix all of them.

+
+	if (nhi_ctxt->d0_exit) {
+		dev_notice(&nhi_ctxt->pdev->dev,
+			   "controller id %#x is exiting D0\n",
+			   nhi_ctxt->id);

What can a user do about this?

+		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);

What can a user do about this?

+		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);

What can a user do about this?

+		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);

No zero-copy, sad :(

+
+	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;

Can you measure the speed difference with likely() here?  If so, great,
if not, drop it as the cpu can guess this better than you or I can.

+	else
+		dev_dbg(&nhi_ctxt->pdev->dev,
+			"controller id %#x wait_for_icm_resp should have been cleared\n",
+			nhi_ctxt->id);

Huh?  So this isn't a real issue?

+
+	nhi_ctxt->ignore_icm_resp = ignore_icm_resp;
+
+	iowrite32(prod_cons, reg);
+
+	return 0;
+}

thanks,

greg k-h