Re: [RFC PATCH v3 5/8] CAIF Protocol Stack

sjur.brandeland@stericsson.com wrote:

From: Sjur Braendeland <redacted>

what Patrick said:
patch description should be here and each email subject should be different
and describe what that patch does.
Please read Documentation/SubmittingPatches.

(more below)

Tell us what CAIF means, probably in the help text.

+	Say Y here if you need to use a phone modem that uses CAIF as transport.
+	You will also need to say yes to any CAIF physical devices that your platform
+	supports.
+	This can be either built-in or a loadable module; if you select to build it as module
+	then the rest of CAIF also needs to built as modules.
+	See Documentation/CAIF for a further explanation on how to use and configure.
+
+if CAIF
+
+config CAIF_NETDEV
+	tristate "CAIF Network device"
+	default CAIF
+	---help---
+	Say Y if you will be using the CAIF based network device.

	                           a (or any) CAIF-based

+	This can be either built-in or a loadable module.
+	If you select to build it as a built-in then the main CAIF device must also be a built-in.
+	If unsure say Y.
+
+config CAIF_SOCK
+	tristate "CAIF Sockets"
+	default CAIF
+	---help---
+	Say Y if you will be using the CAIF Sockets.

	drop "the" ----------------^^^

+	This can be either built-in or a loadable module.
+	If you select to build it as a built-in then the main CAIF device mist also be a built-in.

s/mist/must/

+	If unsure say Y.
+
+config  CAIF_DEBUG
+	bool "Enable Debug"
+	default n
+	--- help ---
+	Enable the inclusion of debug code in the CAIF stack.
+	Be aware that doing this will impact performance.
+	If unsure say N.
+
+
+# Include physical drivers
+source "drivers/net/caif/Kconfig"
+endif
+#endmenu

Use documented/expected kernel-doc notation, please.  I.e., drop "func".

+ * @data:		data to add to buffer
+ * @data_length:	length of data
+ */
+struct sk_buff *caif_create_skb(unsigned char *data, unsigned int data_length)
+{
+	/* NOTE: Make room for CAIF headers when using SKB inside CAIF. */
+	struct sk_buff *skb =
+	    alloc_skb(data_length + CAIF_SKB_HEAD_RESERVE +
+		      CAIF_SKB_TAIL_RESERVE, GFP_ATOMIC);
+	if (skb == NULL)
+		return NULL;
+	skb_reserve(skb, CAIF_SKB_HEAD_RESERVE);
+
+	memcpy(skb_put(skb, data_length), data, data_length);
+	return skb;
+}
+EXPORT_SYMBOL(caif_create_skb);
+

...

+
+module_init(caif_device_init);
+module_exit(caif_device_exit);
+
+
+
+
+
+
+
+
+

delete all of those trailing empty lines.

Do not use /** to begin comments that are not kernel-doc notation.
(more found below, but deleted for now)

+static int caif_sktrecv_cb(struct layer *layr, struct cfpkt *pkt)
+{

+/* Send a signal as a consequence of sendmsg, sendto or caif_sendmsg. */
+static int caif_sendmsg(struct kiocb *kiocb, struct socket *sock,
+			struct msghdr *msg, size_t len)
+{
+
+	struct sock *sk = sock->sk;
+	struct caifsock *cf_sk = container_of(sk, struct caifsock, sk);
+	void *payload;
+	size_t payload_size = msg->msg_iov->iov_len;
+	struct cfpkt *pkt = NULL;
+	struct transmt_info info;
+	unsigned char *txbuf;
+	int err = 0;
+	ssize_t ret = -EIO;
+	int result;
+	struct sk_buff *skb;
+	CAIFLOG_TRACE("Enter %s\n", __func__);
+	caif_assert(msg->msg_iovlen == 1);
+
+	if (cf_sk == NULL) {
+		CAIFLOG_TRACE("[%s:%d] private_data not set!\n",
+			      __func__, __LINE__);
+		ret = -EBADFD;
+		goto write_error_no_unlock;
+	}
+
+	if (unlikely(msg->msg_iov->iov_base == NULL)) {
+		CAIFLOG_WARN("Buffer is NULL.\n");
+		ret = -EINVAL;
+		goto write_error_no_unlock;
+	}
+
+	payload = msg->msg_iov->iov_base;
+	if (payload_size > CAIF_MAX_PAYLOAD_SIZE) {
+		CAIFLOG_TRACE("[%s:%d] buffer too long\n", __func__, __LINE__);
+		ret = -EINVAL;
+		goto write_error_no_unlock;
+	}
+	/* I want to be alone on cf_sk (except status and queue) */
+	lock_sock(&(cf_sk->sk));
+
+	caif_assert(cf_sk->pktq);
+
+	if (!STATE_IS_OPEN(cf_sk)) {
+		/* Socket is closed or closing */
+		if (!STATE_IS_PENDING(cf_sk)) {
+			CAIFLOG_TRACE("socket is closed (by remote end)\n");
+			ret = -EPIPE;
+		} else {
+			CAIFLOG_TRACE("socket is closing...\n");
+			ret = -EBADF;
+		}
+		goto write_error;
+	}
+
+	/* Socket is open or opening */
+	if (STATE_IS_PENDING(cf_sk)) {
+		CAIFLOG_TRACE("socket is opening...\n");
+
+		if (msg->msg_flags & MSG_DONTWAIT) {
+			/* We can't block */
+			CAIFLOG_TRACE("state pending and MSG_DONTWAIT\n");
+			ret = -EAGAIN;
+			goto write_error;
+		}
+
+		/* Let readers in */
+		release_sock(&cf_sk->sk);
+
+		/* Blocking mode; state is pending and we need to wait
+		   for its conclusion */
+		result =
+		    wait_event_interruptible(*cf_sk->sk.sk_sleep,
+					     !STATE_IS_PENDING(cf_sk));
+
+		/* I want to be alone on cf_sk (except status and queue) */
+		lock_sock(&(cf_sk->sk));
+
+		if (result == -ERESTARTSYS) {
+			CAIFLOG_TRACE
+			    (" wait_event_interruptible woken by a signal (1)");
+			ret = -ERESTARTSYS;
+			goto write_error;
+		}
+	}
+
+	if (STATE_IS_REMOTE_SHUTDOWN(cf_sk)) {
+		CAIFLOG_TRACE("received remote_shutdown indication\n");
+		ret = -ESHUTDOWN;
+		goto write_error;
+	}
+
+	if (!TX_FLOW_IS_ON(cf_sk)) {
+
+		/* Flow is off. Check non-block flag */
+		if (msg->msg_flags & MSG_DONTWAIT) {
+			CAIFLOG_TRACE
+			("caif_sendmsg: MSG_DONTWAIT and tx flow off");
+			ret = -EAGAIN;
+			goto write_error;
+		}
+
+		/* release lock before waiting */
+		release_sock(&cf_sk->sk);
+
+		/* Wait until flow is on or socket is closed */
+		if (wait_event_interruptible(*cf_sk->sk.sk_sleep,
+					TX_FLOW_IS_ON(cf_sk)
+					|| !STATE_IS_OPEN(cf_sk)
+					|| STATE_IS_REMOTE_SHUTDOWN(cf_sk)
+					) == -ERESTARTSYS) {
+			CAIFLOG_TRACE
+				("caif_sendmsg:"
+				" wait_event_interruptible woken by a signal");
+			ret = -ERESTARTSYS;
+			goto write_error_no_unlock;
+		}
+
+		/* I want to be alone on cf_sk (except status and queue) */
+		lock_sock(&(cf_sk->sk));
+
+		if (!STATE_IS_OPEN(cf_sk)) {
+			/* someone closed the link, report error */
+			CAIFLOG_TRACE("[%s:%d] remote end shutdown!\n",
+				      __func__, __LINE__);
+			ret = -EPIPE;
+			goto write_error;
+		}
+
+		if (STATE_IS_REMOTE_SHUTDOWN(cf_sk)) {
+			CAIFLOG_TRACE("received remote_shutdown indication\n");
+			ret = -ESHUTDOWN;
+			goto write_error;
+		}
+	}
+
+	/* Create packet, buf=NULL means no copying */
+	skb = caif_alloc_send_skb(sk,
+				payload_size + CAIF_NEEDED_HEADROOM +
+				CAIF_NEEDED_TAILROOM,
+				&err);
+	skb_reserve(skb, CAIF_NEEDED_HEADROOM);
+	pkt = cfpkt_fromnative(CAIF_DIR_OUT, skb);
+	caif_assert((void *)pkt == (void *)skb);
+
+	if (pkt == NULL) {
+		CAIFLOG_TRACE
+			("caif_sendmsg: cfpkt_create_pkt returned NULL\n");
+		ret = -EIO;
+		goto write_error;
+	}
+
+	if (!cfpkt_raw_append(pkt, (void **) &txbuf, payload_size)) {
+		CAIFLOG_TRACE("caif_sendmsg: cfpkt_raw_append failed\n");
+		cfpkt_destroy(pkt);
+		ret = -EINVAL;
+		goto write_error;
+	}
+
+	/* Copy data into buffer. */
+	if (copy_from_user(txbuf, payload, payload_size)) {
+		CAIFLOG_TRACE
+			("caif_sendmsg: copy_from_user returned non zero.\n");
+		cfpkt_destroy(pkt);
+		ret = -EINVAL;
+		goto write_error;
+	}
+	memset(&info, 0, sizeof(info));
+
+	/* Send the packet down the stack. */
+	caif_assert(cf_sk->layer.dn);
+	caif_assert(cf_sk->layer.dn->transmit);
+
+	do {
+			ret = cf_sk->layer.dn->transmit(cf_sk->layer.dn,
+							&info, pkt);

odd indentation?

+		if (likely((ret >= 0) || (ret != -EAGAIN)))
+			break;
+
+		/* EAGAIN - retry */
+		if (msg->msg_flags & MSG_DONTWAIT) {
+			CAIFLOG_TRACE
+			    ("NONBLOCK and transmit failed, error = %d\n",
+			     ret);
+			ret = -EAGAIN;
+			goto write_error;
+		}
+
+		/* Let readers in */
+		release_sock(&cf_sk->sk);
+
+		/* Wait until flow is on or socket is closed */
+		if (wait_event_interruptible(*cf_sk->sk.sk_sleep,
+					TX_FLOW_IS_ON(cf_sk)
+					|| !STATE_IS_OPEN(cf_sk)
+					|| STATE_IS_REMOTE_SHUTDOWN(cf_sk)
+					) == -ERESTARTSYS) {
+			CAIFLOG_TRACE
+			    ("wait_event_interruptible woken by a signal");
+			ret = -ERESTARTSYS;
+			goto write_error_no_unlock;
+		}
+
+		/* I want to be alone on cf_sk (except status and queue) */
+		lock_sock(&(cf_sk->sk));
+
+	} while (ret == -EAGAIN);
+
+	if (ret < 0) {
+		cfpkt_destroy(pkt);
+		CAIFLOG_TRACE("transmit failed, error = %d\n",
+			      ret);
+
+		goto write_error;
+	}
+
+	release_sock(&cf_sk->sk);
+	CAIFLOG_EXIT("");
+	return payload_size;
+
+write_error:
+	release_sock(&cf_sk->sk);
+write_error_no_unlock:
+	CAIFLOG_EXIT("");
+	return ret;
+}
+

+
+static int setsockopt(struct socket *sock,
+			int lvl, int opt, char __user *ov, int ol)
+{
+	struct sock *sk = sock->sk;
+	struct caifsock *cf_sk = container_of(sk, struct caifsock, sk);
+	struct caif_channel_opt confopt;
+	int res;
+
+	if (lvl != SOL_CAIF) {
+		CAIFLOG_TRACE("setsockopt bad level\n");
+			return -ENOPROTOOPT;

odd indentation.

+	}
+
+	switch (opt) {
+	case CAIF_CHANNEL_OPT:
+		if (ol < sizeof(struct caif_channel_opt)) {
+			CAIFLOG_TRACE("setsockopt"
+					" CAIF_CHANNEL_CONFIG bad size\n");
+				return -EINVAL;

odd indentation.

+		}
+		res = copy_from_user(&confopt, ov, sizeof(confopt));
+		if (res)
+			return res;
+		lock_sock(&(cf_sk->sk));
+		cf_sk->config.priority = confopt.priority;
+		cf_sk->config.phy_pref = confopt.link_selector;
+		strncpy(cf_sk->config.phy_name, confopt.link_name,
+			sizeof(cf_sk->config.phy_name));
+		CAIFLOG_TRACE("Setting sockopt pri=%d pref=%d name=%s\n",
+			      cf_sk->config.priority,
+			      cf_sk->config.phy_pref,
+			      cf_sk->config.phy_name);
+		release_sock(&cf_sk->sk);
+		return 0;
+/* TODO: Implement the remaining options:
+ *	case CAIF_REQ_PARAM_OPT:
+ *	case CAIF_RSP_PARAM_OPT:
+ *	case CAIF_UTIL_FLOW_OPT:
+ *	case CAIF_CONN_INFO_OPT:
+ *	case CAIF_CONN_ID_OPT:
+ */
+	default:
+		CAIFLOG_TRACE("setsockopt: unhandled option %d\n",
+			      opt);
+		return -EINVAL;
+	}
+
+
+}

+int caif_connect(struct socket *sock, struct sockaddr *uservaddr,
+	       int sockaddr_len, int flags)
+{
+	struct caifsock *cf_sk = NULL;
+	int result = -1;
+	int mode = 0;
+	int ret = -EIO;
+    struct sock *sk = sock->sk;

indent above line with tab, not spaces.

+
+	CAIFLOG_TRACE("Enter %s\n", __func__);
+
+	BUG_ON(sk == NULL);
+
+	cf_sk = container_of(sk, struct caifsock, sk);
+
+	CAIFLOG_TRACE("cf_sk=%p OPEN=%d, TX_FLOW=%d, RX_FLOW=%d\n", cf_sk,
+		      STATE_IS_OPEN(cf_sk),
+		      TX_FLOW_IS_ON(cf_sk), RX_FLOW_IS_ON(cf_sk));
+
+	sk->sk_state    = TCP_CLOSE;
+	sock->state     = SS_UNCONNECTED;
+
+	/* FIXME: SOCK_DGRAM should be removed? */
+	if (sock->type == SOCK_DGRAM || sock->type == SOCK_SEQPACKET) {
+		sock->state     = SS_CONNECTED;
+		sk->sk_state    = TCP_ESTABLISHED;
+	} else
+		goto out;
+
+	/* I want to be alone on cf_sk (except status and queue) */
+	lock_sock(&(cf_sk->sk));
+
+	ret = caif_channel_config(cf_sk, uservaddr, sockaddr_len,
+				&cf_sk->config);
+	if (ret) {
+		CAIFLOG_TRACE
+		    ("[%s:%d] Cannot set socket address\n",
+		     __func__, __LINE__);
+		goto open_error;
+	}
+
+
+#ifdef CONFIG_DEBUG_FS
+	atomic_inc(&cf_sk->num_open);
+#endif
+	mode = SKT_READ_FLAG | SKT_WRITE_FLAG;
+
+	/* If socket is not open, make sure socket is in fully closed state */
+	if (!STATE_IS_OPEN(cf_sk)) {
+		/* Has link close response been received
+		   (if we ever sent it)? */
+		if (STATE_IS_PENDING(cf_sk)) {
+			/* Still waiting for close response from remote.
+			   If opened non-blocking, report "would block" */
+			if (flags & MSG_DONTWAIT) {
+				CAIFLOG_TRACE("MSG_DONTWAIT"
+					" && close pending\n");
+				ret = -EAGAIN;
+				goto open_error;
+			}
+
+			CAIFLOG_TRACE
+			    ("wait for close response from remote...\n");
+
+			release_sock(&cf_sk->sk);
+
+			/* Blocking mode; close is pending and we need to wait
+			   for its conclusion */
+			result =
+			    wait_event_interruptible(*cf_sk->sk.sk_sleep,
+						     !STATE_IS_PENDING(cf_sk));
+
+			lock_sock(&(cf_sk->sk));
+			if (result == -ERESTARTSYS) {
+				CAIFLOG_TRACE
+				    ("wait_event_interruptible woken"
+				     "by a signal (1)");
+				ret = -ERESTARTSYS;
+				goto open_error;
+			}
+		}
+	}

...

+}

+
+/* This function is called as part of close. */
+static int caif_release(struct socket *sock)
+{
+	struct sock *sk = sock->sk;
+	struct caifsock *cf_sk = NULL;
+	CAIFLOG_TRACE("Enter %s\n", __func__);
+
+	caif_assert(sk != NULL);
+	cf_sk = container_of(sk, struct caifsock, sk);
+
+	caif_shutdown(sock, SHUT_RDWR);
+	lock_sock(&(cf_sk->sk));
+	/* NOTE: This is how the sock->sk structure is used all over the linux
+	 *       socket implementation. Since there is no locking protecting
+	 *       the sock->sk pointer, the linux socket implementation do not
+	 *       support multiple threads using the same socket descriptor,
+	 *       since if one of the threads close the socket, the other
+	 *       threads may end up accessing a pointer to NULL. See for
+	 *       instance sock_setsockopt in net/socket.c, it uses sock->sk
+	 *       without checking if its value is NULL. */

	/*
	 * Note: This is the Linux long comment
	 * style.  Please use it.
	 */

+	sock->sk = NULL;
+
+	/* Unlink the socket from the hunt name table. */
+
+
+	/* Trigger the pending attachs from and to the socket.  This needs to
+	 * be done before the socket starts to be released. */
+
+
+	/* Remove pending new link requests. */
+
+	/* Detach the socket from its process context by making it orphan. */
+	sock_orphan(sk);
+
+	/* Setting SHUTDOWN_MASK means that both send and receive are shutdown
+	 * for the socket. */
+	sk->sk_shutdown = SHUTDOWN_MASK;
+
+	/* Set the socket state to closed, the TCP_CLOSE macro is used when
+	 * closing any socket. */
+	sk->sk_state = TCP_CLOSE;
+
+	/* Flush out this sockets receive queue. */
+	drain_queue(cf_sk);
+
+	/* Finally release the socket. */
+	STATE_IS_PENDING_DESTROY(cf_sk);
+	release_sock(&cf_sk->sk);
+	sock_put(sk);
+
+	return 0;
+
+	/* The rest of the cleanup will be handled from the
+	 * caif_sock_destructor */
+}

+/* This function is called when a socket is finally destructed. */

                                                       destroyed. */

+static void caif_sock_destructor(struct sock *sk)
+{
+	struct caifsock *cf_sk = NULL;
+	CAIFLOG_TRACE("Enter %s\n", __func__);
+	cf_sk = container_of(sk, struct caifsock, sk);
+
+	/* Error checks. */
+	caif_assert(!atomic_read(&sk->sk_wmem_alloc));
+	caif_assert(sk_unhashed(sk));
+	caif_assert(!sk->sk_socket);
+
+	if (!sock_flag(sk, SOCK_DEAD)) {
+		CAIFLOG_TRACE("Attempt to release alive caif socket: 0x%p", sk);
+		return;
+	}
+
+	lock_sock(&(cf_sk->sk));
+
+	if (STATE_IS_OPEN(cf_sk)) {
+		CAIFLOG_TRACE
+			("[%s:%d] socket is opened (cf_sk=%p)"
+				" file_mode = 0x%x\n", __func__, __LINE__,
+				cf_sk, cf_sk->file_mode);
+		release_sock(&cf_sk->sk);
+		return;
+	}
+
+	CAIFLOG_TRACE("pktq = %p \n", cf_sk->pktq);
+	drain_queue(cf_sk);
+	CAIFLOG_TRACE("pktq = %p \n", cf_sk->pktq);
+
+	cfglu_free(cf_sk->pktq);
+
+#ifdef CONFIG_DEBUG_FS
+	if (cf_sk->debugfs_device_dir != NULL)
+		debugfs_remove_recursive(cf_sk->debugfs_device_dir);
+#endif
+
+	release_sock(&cf_sk->sk);
+	CAIFLOG_WARN("caif_sock_destructor: Removing socket %s\n",
+		cf_sk->name);
+	/* Decrease the global caif socket counter. */
+	atomic_dec(&caif_nr_socks);
+	return;
+}
+

Above doesn't need to be on 2 lines.

+		/* Find from name. */
+		if (name) {
+			if (!strncmp(tmp->name, name, sizeof(tmp->name)))
+				dev = tmp;
+		} else
+			/* Get the first element if name is not specified. */
+			dev = tmp;
+		if (dev) {
+			CAIFLOG_TRACE("[%s:%d] match %s \n",
+				__func__, __LINE__,  name);
+			if (remove_from_list)
+				list_del(list_node);
+			break;
+		}
+	}
+	spin_unlock(&list_lock);
+	return dev;
+}

-- 
~Randy