[PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dexuan Cui]

This will be used by the coming net/hvsock driver.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/hv/channel.c      | 133 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/hv/hyperv_vmbus.h |   4 ++
 drivers/hv/ring_buffer.c  |  14 +++++
 include/linux/hyperv.h    |  32 +++++++++++
 4 files changed, 183 insertions(+)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index b09d1b7..ffdef03 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -758,6 +758,53 @@ int vmbus_sendpacket_pagebuffer_ctl(struct vmbus_channel *channel,
 EXPORT_SYMBOL_GPL(vmbus_sendpacket_pagebuffer_ctl);
 
 /*
+ * vmbus_sendpacket_hvsock - Send the hvsock payload 'buf' into the vmbus
+ * ringbuffer
+ */
+int vmbus_sendpacket_hvsock(struct vmbus_channel *channel, void *buf, u32 len)
+{
+	struct vmpipe_proto_header pipe_hdr;
+	struct vmpacket_descriptor desc;
+	struct kvec bufferlist[4];
+	u32 packetlen_aligned;
+	u32 packetlen;
+	u64 aligned_data = 0;
+	bool signal = false;
+	int ret;
+
+	packetlen = HVSOCK_HEADER_LEN + len;
+	packetlen_aligned = ALIGN(packetlen, sizeof(u64));
+
+	/* Setup the descriptor */
+	desc.type = VM_PKT_DATA_INBAND;
+	/* in 8-bytes granularity */
+	desc.offset8 = sizeof(struct vmpacket_descriptor) >> 3;
+	desc.len8 = (u16)(packetlen_aligned >> 3);
+	desc.flags = 0;
+	desc.trans_id = 0;
+
+	pipe_hdr.pkt_type = 1;
+	pipe_hdr.data_size = len;
+
+	bufferlist[0].iov_base = &desc;
+	bufferlist[0].iov_len  = sizeof(struct vmpacket_descriptor);
+	bufferlist[1].iov_base = &pipe_hdr;
+	bufferlist[1].iov_len  = sizeof(struct vmpipe_proto_header);
+	bufferlist[2].iov_base = buf;
+	bufferlist[2].iov_len  = len;
+	bufferlist[3].iov_base = &aligned_data;
+	bufferlist[3].iov_len  = packetlen_aligned - packetlen;
+
+	ret = hv_ringbuffer_write(&channel->outbound, bufferlist, 4, &signal);
+
+	if (ret == 0 && signal)
+		vmbus_setevent(channel);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(vmbus_sendpacket_hvsock);
+
+/*
  * vmbus_sendpacket_pagebuffer - Send a range of single-page buffer
  * packets using a GPADL Direct packet type.
  */
@@ -978,3 +1025,89 @@ int vmbus_recvpacket_raw(struct vmbus_channel *channel, void *buffer,
 	return ret;
 }
 EXPORT_SYMBOL_GPL(vmbus_recvpacket_raw);
+
+/*
+ * vmbus_recvpacket_hvsock - Receive the hvsock payload from the vmbus
+ * ringbuffer into the 'buffer'.
+ */
+int vmbus_recvpacket_hvsock(struct vmbus_channel *channel, void *buffer,
+			    u32 bufferlen, u32 *buffer_actual_len)
+{
+	struct vmpipe_proto_header *pipe_hdr;
+	struct vmpacket_descriptor *desc;
+	u32 packet_len, payload_len;
+	bool signal = false;
+	int ret;
+
+	*buffer_actual_len = 0;
+
+	if (bufferlen < HVSOCK_HEADER_LEN)
+		return -ENOBUFS;
+
+	ret = hv_ringbuffer_peek(&channel->inbound, buffer,
+				 HVSOCK_HEADER_LEN);
+	if (ret != 0)
+		return 0;
+
+	desc = (struct vmpacket_descriptor *)buffer;
+	packet_len = desc->len8 << 3;
+	if (desc->type != VM_PKT_DATA_INBAND ||
+	    desc->offset8 != (sizeof(*desc) / 8) ||
+	    packet_len < HVSOCK_HEADER_LEN)
+		return -EIO;
+
+	pipe_hdr = (struct vmpipe_proto_header *)(desc + 1);
+	payload_len = pipe_hdr->data_size;
+
+	if (pipe_hdr->pkt_type != 1 || payload_len == 0)
+		return -EIO;
+
+	if (HVSOCK_PKT_LEN(payload_len) != packet_len + PREV_INDICES_LEN)
+		return -EIO;
+
+	if (bufferlen < packet_len - HVSOCK_HEADER_LEN)
+		return -ENOBUFS;
+
+	/* Copy over the hvsock payload to the user buffer */
+	ret = hv_ringbuffer_read(&channel->inbound, buffer,
+				 packet_len - HVSOCK_HEADER_LEN,
+				 HVSOCK_HEADER_LEN, &signal);
+	if (ret != 0)
+		return ret;
+
+	*buffer_actual_len = payload_len;
+
+	if (signal)
+		vmbus_setevent(channel);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vmbus_recvpacket_hvsock);
+
+/*
+ * vmbus_get_hvsock_rw_status - can the ringbuffer be read/written?
+ */
+void vmbus_get_hvsock_rw_status(struct vmbus_channel *channel,
+				bool *can_read, bool *can_write)
+{
+	u32 avl_read_bytes, avl_write_bytes, dummy;
+
+	if (can_read != NULL) {
+		hv_get_ringbuffer_available_space(&channel->inbound,
+						  &avl_read_bytes,
+						  &dummy);
+		*can_read = avl_read_bytes >= HVSOCK_MIN_PKT_LEN;
+	}
+
+	/* We write into the ringbuffer only when we're able to write a
+	 * a payload of 4096 bytes (the actual written payload's length may be
+	 * less than 4096).
+	 */
+	if (can_write != NULL) {
+		hv_get_ringbuffer_available_space(&channel->outbound,
+						  &dummy,
+						  &avl_write_bytes);
+		*can_write = avl_write_bytes > HVSOCK_PKT_LEN(PAGE_SIZE);
+	}
+}
+EXPORT_SYMBOL_GPL(vmbus_get_hvsock_rw_status);
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index cddc0c9..16b6ac4 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -608,6 +608,10 @@ int hv_ringbuffer_write(struct hv_ring_buffer_info *ring_info,
 int hv_ringbuffer_peek(struct hv_ring_buffer_info *ring_info, void *buffer,
 		   u32 buflen);
 
+void hv_get_ringbuffer_available_space(struct hv_ring_buffer_info *inring_info,
+				       u32 *bytes_avail_toread,
+				       u32 *bytes_avail_towrite);
+
 int hv_ringbuffer_read(struct hv_ring_buffer_info *ring_info,
 		   void *buffer,
 		   u32 buflen,
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 6361d12..e493c66 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -501,6 +501,20 @@ int hv_ringbuffer_peek(struct hv_ring_buffer_info *Inring_info,
 	return 0;
 }
 
+void hv_get_ringbuffer_available_space(struct hv_ring_buffer_info *inring_info,
+				       u32 *bytes_avail_toread,
+				       u32 *bytes_avail_towrite)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&inring_info->ring_lock, flags);
+
+	hv_get_ringbuffer_availbytes(inring_info,
+				     bytes_avail_toread,
+				     bytes_avail_towrite);
+
+	spin_unlock_irqrestore(&inring_info->ring_lock, flags);
+}
 
 /*
  *
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 264093a..c8e27da 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -885,6 +885,9 @@ extern int vmbus_sendpacket_ctl(struct vmbus_channel *channel,
 				  u32 flags,
 				  bool kick_q);
 
+extern int vmbus_sendpacket_hvsock(struct vmbus_channel *channel,
+				   void *buf, u32 len);
+
 extern int vmbus_sendpacket_pagebuffer(struct vmbus_channel *channel,
 					    struct hv_page_buffer pagebuffers[],
 					    u32 pagecount,
@@ -934,6 +937,11 @@ extern int vmbus_recvpacket_raw(struct vmbus_channel *channel,
 				     u32 *buffer_actual_len,
 				     u64 *requestid);
 
+extern int vmbus_recvpacket_hvsock(struct vmbus_channel *channel, void *buffer,
+				   u32 bufferlen, u32 *buffer_actual_len);
+
+extern void vmbus_get_hvsock_rw_status(struct vmbus_channel *channel,
+				       bool *can_read, bool *can_write);
 
 extern void vmbus_ontimer(unsigned long data);
 
@@ -1261,4 +1269,28 @@ extern __u32 vmbus_proto_version;
 
 int vmbus_send_tl_connect_request(const uuid_le *shv_guest_servie_id,
 				  const uuid_le *shv_host_servie_id);
+struct vmpipe_proto_header {
+	u32 pkt_type;
+
+	union {
+		u32 data_size;
+		struct {
+			u16 data_size;
+			u16 offset;
+		} partial;
+	};
+} __packed;
+
+/* see hv_ringbuffer_read() and hv_ringbuffer_write() */
+#define PREV_INDICES_LEN	(sizeof(u64))
+
+#define HVSOCK_HEADER_LEN	(sizeof(struct vmpacket_descriptor) + \
+				 sizeof(struct vmpipe_proto_header))
+
+#define HVSOCK_PKT_LEN(payload_len)	(HVSOCK_HEADER_LEN + \
+					ALIGN((payload_len), 8) + \
+					PREV_INDICES_LEN)
+
+#define HVSOCK_MIN_PKT_LEN	HVSOCK_PKT_LEN(1)
+
 #endif /* _HYPERV_H */
-- 
2.1.0

[PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dexuan Cui]

This will be used by the coming net/hvsock driver.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/hv/channel.c      | 133 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/hv/hyperv_vmbus.h |   4 ++
 drivers/hv/ring_buffer.c  |  14 +++++
 include/linux/hyperv.h    |  32 +++++++++++
 4 files changed, 183 insertions(+)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index b09d1b7..ffdef03 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -758,6 +758,53 @@ int vmbus_sendpacket_pagebuffer_ctl(struct vmbus_channel *channel,
 EXPORT_SYMBOL_GPL(vmbus_sendpacket_pagebuffer_ctl);
 
 /*
+ * vmbus_sendpacket_hvsock - Send the hvsock payload 'buf' into the vmbus
+ * ringbuffer
+ */
+int vmbus_sendpacket_hvsock(struct vmbus_channel *channel, void *buf, u32 len)
+{
+	struct vmpipe_proto_header pipe_hdr;
+	struct vmpacket_descriptor desc;
+	struct kvec bufferlist[4];
+	u32 packetlen_aligned;
+	u32 packetlen;
+	u64 aligned_data = 0;
+	bool signal = false;
+	int ret;
+
+	packetlen = HVSOCK_HEADER_LEN + len;
+	packetlen_aligned = ALIGN(packetlen, sizeof(u64));
+
+	/* Setup the descriptor */
+	desc.type = VM_PKT_DATA_INBAND;
+	/* in 8-bytes granularity */
+	desc.offset8 = sizeof(struct vmpacket_descriptor) >> 3;
+	desc.len8 = (u16)(packetlen_aligned >> 3);
+	desc.flags = 0;
+	desc.trans_id = 0;
+
+	pipe_hdr.pkt_type = 1;
+	pipe_hdr.data_size = len;
+
+	bufferlist[0].iov_base = &desc;
+	bufferlist[0].iov_len  = sizeof(struct vmpacket_descriptor);
+	bufferlist[1].iov_base = &pipe_hdr;
+	bufferlist[1].iov_len  = sizeof(struct vmpipe_proto_header);
+	bufferlist[2].iov_base = buf;
+	bufferlist[2].iov_len  = len;
+	bufferlist[3].iov_base = &aligned_data;
+	bufferlist[3].iov_len  = packetlen_aligned - packetlen;
+
+	ret = hv_ringbuffer_write(&channel->outbound, bufferlist, 4, &signal);
+
+	if (ret == 0 && signal)
+		vmbus_setevent(channel);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(vmbus_sendpacket_hvsock);
+
+/*
  * vmbus_sendpacket_pagebuffer - Send a range of single-page buffer
  * packets using a GPADL Direct packet type.
  */
@@ -978,3 +1025,89 @@ int vmbus_recvpacket_raw(struct vmbus_channel *channel, void *buffer,
 	return ret;
 }
 EXPORT_SYMBOL_GPL(vmbus_recvpacket_raw);
+
+/*
+ * vmbus_recvpacket_hvsock - Receive the hvsock payload from the vmbus
+ * ringbuffer into the 'buffer'.
+ */
+int vmbus_recvpacket_hvsock(struct vmbus_channel *channel, void *buffer,
+			    u32 bufferlen, u32 *buffer_actual_len)
+{
+	struct vmpipe_proto_header *pipe_hdr;
+	struct vmpacket_descriptor *desc;
+	u32 packet_len, payload_len;
+	bool signal = false;
+	int ret;
+
+	*buffer_actual_len = 0;
+
+	if (bufferlen < HVSOCK_HEADER_LEN)
+		return -ENOBUFS;
+
+	ret = hv_ringbuffer_peek(&channel->inbound, buffer,
+				 HVSOCK_HEADER_LEN);
+	if (ret != 0)
+		return 0;
+
+	desc = (struct vmpacket_descriptor *)buffer;
+	packet_len = desc->len8 << 3;
+	if (desc->type != VM_PKT_DATA_INBAND ||
+	    desc->offset8 != (sizeof(*desc) / 8) ||
+	    packet_len < HVSOCK_HEADER_LEN)
+		return -EIO;
+
+	pipe_hdr = (struct vmpipe_proto_header *)(desc + 1);
+	payload_len = pipe_hdr->data_size;
+
+	if (pipe_hdr->pkt_type != 1 || payload_len == 0)
+		return -EIO;
+
+	if (HVSOCK_PKT_LEN(payload_len) != packet_len + PREV_INDICES_LEN)
+		return -EIO;
+
+	if (bufferlen < packet_len - HVSOCK_HEADER_LEN)
+		return -ENOBUFS;
+
+	/* Copy over the hvsock payload to the user buffer */
+	ret = hv_ringbuffer_read(&channel->inbound, buffer,
+				 packet_len - HVSOCK_HEADER_LEN,
+				 HVSOCK_HEADER_LEN, &signal);
+	if (ret != 0)
+		return ret;
+
+	*buffer_actual_len = payload_len;
+
+	if (signal)
+		vmbus_setevent(channel);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vmbus_recvpacket_hvsock);
+
+/*
+ * vmbus_get_hvsock_rw_status - can the ringbuffer be read/written?
+ */
+void vmbus_get_hvsock_rw_status(struct vmbus_channel *channel,
+				bool *can_read, bool *can_write)
+{
+	u32 avl_read_bytes, avl_write_bytes, dummy;
+
+	if (can_read != NULL) {
+		hv_get_ringbuffer_available_space(&channel->inbound,
+						  &avl_read_bytes,
+						  &dummy);
+		*can_read = avl_read_bytes >= HVSOCK_MIN_PKT_LEN;
+	}
+
+	/* We write into the ringbuffer only when we're able to write a
+	 * a payload of 4096 bytes (the actual written payload's length may be
+	 * less than 4096).
+	 */
+	if (can_write != NULL) {
+		hv_get_ringbuffer_available_space(&channel->outbound,
+						  &dummy,
+						  &avl_write_bytes);
+		*can_write = avl_write_bytes > HVSOCK_PKT_LEN(PAGE_SIZE);
+	}
+}
+EXPORT_SYMBOL_GPL(vmbus_get_hvsock_rw_status);
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index cddc0c9..16b6ac4 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -608,6 +608,10 @@ int hv_ringbuffer_write(struct hv_ring_buffer_info *ring_info,
 int hv_ringbuffer_peek(struct hv_ring_buffer_info *ring_info, void *buffer,
 		   u32 buflen);
 
+void hv_get_ringbuffer_available_space(struct hv_ring_buffer_info *inring_info,
+				       u32 *bytes_avail_toread,
+				       u32 *bytes_avail_towrite);
+
 int hv_ringbuffer_read(struct hv_ring_buffer_info *ring_info,
 		   void *buffer,
 		   u32 buflen,
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 6361d12..e493c66 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -501,6 +501,20 @@ int hv_ringbuffer_peek(struct hv_ring_buffer_info *Inring_info,
 	return 0;
 }
 
+void hv_get_ringbuffer_available_space(struct hv_ring_buffer_info *inring_info,
+				       u32 *bytes_avail_toread,
+				       u32 *bytes_avail_towrite)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&inring_info->ring_lock, flags);
+
+	hv_get_ringbuffer_availbytes(inring_info,
+				     bytes_avail_toread,
+				     bytes_avail_towrite);
+
+	spin_unlock_irqrestore(&inring_info->ring_lock, flags);
+}
 
 /*
  *
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 264093a..c8e27da 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -885,6 +885,9 @@ extern int vmbus_sendpacket_ctl(struct vmbus_channel *channel,
 				  u32 flags,
 				  bool kick_q);
 
+extern int vmbus_sendpacket_hvsock(struct vmbus_channel *channel,
+				   void *buf, u32 len);
+
 extern int vmbus_sendpacket_pagebuffer(struct vmbus_channel *channel,
 					    struct hv_page_buffer pagebuffers[],
 					    u32 pagecount,
@@ -934,6 +937,11 @@ extern int vmbus_recvpacket_raw(struct vmbus_channel *channel,
 				     u32 *buffer_actual_len,
 				     u64 *requestid);
 
+extern int vmbus_recvpacket_hvsock(struct vmbus_channel *channel, void *buffer,
+				   u32 bufferlen, u32 *buffer_actual_len);
+
+extern void vmbus_get_hvsock_rw_status(struct vmbus_channel *channel,
+				       bool *can_read, bool *can_write);
 
 extern void vmbus_ontimer(unsigned long data);
 
@@ -1261,4 +1269,28 @@ extern __u32 vmbus_proto_version;
 
 int vmbus_send_tl_connect_request(const uuid_le *shv_guest_servie_id,
 				  const uuid_le *shv_host_servie_id);
+struct vmpipe_proto_header {
+	u32 pkt_type;
+
+	union {
+		u32 data_size;
+		struct {
+			u16 data_size;
+			u16 offset;
+		} partial;
+	};
+} __packed;
+
+/* see hv_ringbuffer_read() and hv_ringbuffer_write() */
+#define PREV_INDICES_LEN	(sizeof(u64))
+
+#define HVSOCK_HEADER_LEN	(sizeof(struct vmpacket_descriptor) + \
+				 sizeof(struct vmpipe_proto_header))
+
+#define HVSOCK_PKT_LEN(payload_len)	(HVSOCK_HEADER_LEN + \
+					ALIGN((payload_len), 8) + \
+					PREV_INDICES_LEN)
+
+#define HVSOCK_MIN_PKT_LEN	HVSOCK_PKT_LEN(1)
+
 #endif /* _HYPERV_H */
-- 
2.1.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Vitaly Kuznetsov]

Dexuan Cui <decui@microsoft.com> writes:

> This will be used by the coming net/hvsock driver.
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/hv/channel.c      | 133 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/hv/hyperv_vmbus.h |   4 ++
>  drivers/hv/ring_buffer.c  |  14 +++++
>  include/linux/hyperv.h    |  32 +++++++++++
>  4 files changed, 183 insertions(+)
>
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index b09d1b7..ffdef03 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -758,6 +758,53 @@ int vmbus_sendpacket_pagebuffer_ctl(struct vmbus_channel *channel,
>  EXPORT_SYMBOL_GPL(vmbus_sendpacket_pagebuffer_ctl);
>
>  /*
> + * vmbus_sendpacket_hvsock - Send the hvsock payload 'buf' into the vmbus
> + * ringbuffer
> + */
> +int vmbus_sendpacket_hvsock(struct vmbus_channel *channel, void *buf, u32 len)
> +{
> +	struct vmpipe_proto_header pipe_hdr;
> +	struct vmpacket_descriptor desc;
> +	struct kvec bufferlist[4];
> +	u32 packetlen_aligned;
> +	u32 packetlen;
> +	u64 aligned_data = 0;
> +	bool signal = false;
> +	int ret;
> +
> +	packetlen = HVSOCK_HEADER_LEN + len;
> +	packetlen_aligned = ALIGN(packetlen, sizeof(u64));
> +
> +	/* Setup the descriptor */
> +	desc.type = VM_PKT_DATA_INBAND;
> +	/* in 8-bytes granularity */
> +	desc.offset8 = sizeof(struct vmpacket_descriptor) >> 3;
> +	desc.len8 = (u16)(packetlen_aligned >> 3);
> +	desc.flags = 0;
> +	desc.trans_id = 0;
> +
> +	pipe_hdr.pkt_type = 1;
> +	pipe_hdr.data_size = len;
> +
> +	bufferlist[0].iov_base = &desc;
> +	bufferlist[0].iov_len  = sizeof(struct vmpacket_descriptor);
> +	bufferlist[1].iov_base = &pipe_hdr;
> +	bufferlist[1].iov_len  = sizeof(struct vmpipe_proto_header);
> +	bufferlist[2].iov_base = buf;
> +	bufferlist[2].iov_len  = len;
> +	bufferlist[3].iov_base = &aligned_data;
> +	bufferlist[3].iov_len  = packetlen_aligned - packetlen;
> +
> +	ret = hv_ringbuffer_write(&channel->outbound, bufferlist, 4, &signal);
> +
> +	if (ret == 0 && signal)
> +		vmbus_setevent(channel);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(vmbus_sendpacket_hvsock);
> +
> +/*
>   * vmbus_sendpacket_pagebuffer - Send a range of single-page buffer
>   * packets using a GPADL Direct packet type.
>   */
> @@ -978,3 +1025,89 @@ int vmbus_recvpacket_raw(struct vmbus_channel *channel, void *buffer,
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(vmbus_recvpacket_raw);
> +
> +/*
> + * vmbus_recvpacket_hvsock - Receive the hvsock payload from the vmbus
> + * ringbuffer into the 'buffer'.
> + */
> +int vmbus_recvpacket_hvsock(struct vmbus_channel *channel, void *buffer,
> +			    u32 bufferlen, u32 *buffer_actual_len)
> +{
> +	struct vmpipe_proto_header *pipe_hdr;
> +	struct vmpacket_descriptor *desc;
> +	u32 packet_len, payload_len;
> +	bool signal = false;
> +	int ret;
> +
> +	*buffer_actual_len = 0;
> +
> +	if (bufferlen < HVSOCK_HEADER_LEN)
> +		return -ENOBUFS;
> +
> +	ret = hv_ringbuffer_peek(&channel->inbound, buffer,
> +				 HVSOCK_HEADER_LEN);
> +	if (ret != 0)
> +		return 0;

I'd suggest you do something like

    if (ret == -EAGIAIN)
        return 0;
    else if (ret)
        return ret;

to make it future-proof (e.g. when a new error is returned by
hv_ringbuffer_peek). And a comment would also be useful as it is unclear
why we silence errors here.

> +
> +	desc = (struct vmpacket_descriptor *)buffer;
> +	packet_len = desc->len8 << 3;
> +	if (desc->type != VM_PKT_DATA_INBAND ||
> +	    desc->offset8 != (sizeof(*desc) / 8) ||
> +	    packet_len < HVSOCK_HEADER_LEN)
> +		return -EIO;
> +
> +	pipe_hdr = (struct vmpipe_proto_header *)(desc + 1);
> +	payload_len = pipe_hdr->data_size;
> +
> +	if (pipe_hdr->pkt_type != 1 || payload_len == 0)
> +		return -EIO;
> +
> +	if (HVSOCK_PKT_LEN(payload_len) != packet_len + PREV_INDICES_LEN)
> +		return -EIO;
> +
> +	if (bufferlen < packet_len - HVSOCK_HEADER_LEN)
> +		return -ENOBUFS;
> +
> +	/* Copy over the hvsock payload to the user buffer */
> +	ret = hv_ringbuffer_read(&channel->inbound, buffer,
> +				 packet_len - HVSOCK_HEADER_LEN,
> +				 HVSOCK_HEADER_LEN, &signal);
> +	if (ret != 0)
> +		return ret;
> +
> +	*buffer_actual_len = payload_len;
> +
> +	if (signal)
> +		vmbus_setevent(channel);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(vmbus_recvpacket_hvsock);
> +
> +/*
> + * vmbus_get_hvsock_rw_status - can the ringbuffer be read/written?
> + */
> +void vmbus_get_hvsock_rw_status(struct vmbus_channel *channel,
> +				bool *can_read, bool *can_write)
> +{
> +	u32 avl_read_bytes, avl_write_bytes, dummy;
> +
> +	if (can_read != NULL) {
> +		hv_get_ringbuffer_available_space(&channel->inbound,
> +						  &avl_read_bytes,
> +						  &dummy);
> +		*can_read = avl_read_bytes >= HVSOCK_MIN_PKT_LEN;
> +	}
> +
> +	/* We write into the ringbuffer only when we're able to write a

Not sure why checkpatch.pl doesn't complain but according to the
CodingStyle multi-line comments outside of drivers/net and net/ are
supposed to start with and empty '/*' line.

> +	 * a payload of 4096 bytes (the actual written payload's length may be
> +	 * less than 4096).
> +	 */
> +	if (can_write != NULL) {
> +		hv_get_ringbuffer_available_space(&channel->outbound,
> +						  &dummy,
> +						  &avl_write_bytes);
> +		*can_write = avl_write_bytes > HVSOCK_PKT_LEN(PAGE_SIZE);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(vmbus_get_hvsock_rw_status);
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index cddc0c9..16b6ac4 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -608,6 +608,10 @@ int hv_ringbuffer_write(struct hv_ring_buffer_info *ring_info,
>  int hv_ringbuffer_peek(struct hv_ring_buffer_info *ring_info, void *buffer,
>  		   u32 buflen);
>
> +void hv_get_ringbuffer_available_space(struct hv_ring_buffer_info *inring_info,
> +				       u32 *bytes_avail_toread,
> +				       u32 *bytes_avail_towrite);
> +
>  int hv_ringbuffer_read(struct hv_ring_buffer_info *ring_info,
>  		   void *buffer,
>  		   u32 buflen,
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index 6361d12..e493c66 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -501,6 +501,20 @@ int hv_ringbuffer_peek(struct hv_ring_buffer_info *Inring_info,
>  	return 0;
>  }
>
> +void hv_get_ringbuffer_available_space(struct hv_ring_buffer_info *inring_info,
> +				       u32 *bytes_avail_toread,
> +				       u32 *bytes_avail_towrite)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&inring_info->ring_lock, flags);
> +
> +	hv_get_ringbuffer_availbytes(inring_info,
> +				     bytes_avail_toread,
> +				     bytes_avail_towrite);
> +
> +	spin_unlock_irqrestore(&inring_info->ring_lock, flags);
> +}
>
>  /*
>   *
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 264093a..c8e27da 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -885,6 +885,9 @@ extern int vmbus_sendpacket_ctl(struct vmbus_channel *channel,
>  				  u32 flags,
>  				  bool kick_q);
>
> +extern int vmbus_sendpacket_hvsock(struct vmbus_channel *channel,
> +				   void *buf, u32 len);
> +

I *think* if your argument list makes it to the second line it is supposed
to be alligned with the first one (e.g. 'void' should start at the same
position as 'struct' on the previous line).

>  extern int vmbus_sendpacket_pagebuffer(struct vmbus_channel *channel,
>  					    struct hv_page_buffer pagebuffers[],
>  					    u32 pagecount,
> @@ -934,6 +937,11 @@ extern int vmbus_recvpacket_raw(struct vmbus_channel *channel,
>  				     u32 *buffer_actual_len,
>  				     u64 *requestid);
>
> +extern int vmbus_recvpacket_hvsock(struct vmbus_channel *channel, void *buffer,
> +				   u32 bufferlen, u32 *buffer_actual_len);
> +

the same.

> +extern void vmbus_get_hvsock_rw_status(struct vmbus_channel *channel,
> +				       bool *can_read, bool *can_write);
>

the same.

>  extern void vmbus_ontimer(unsigned long data);
>
> @@ -1261,4 +1269,28 @@ extern __u32 vmbus_proto_version;
>
>  int vmbus_send_tl_connect_request(const uuid_le *shv_guest_servie_id,
>  				  const uuid_le *shv_host_servie_id);
> +struct vmpipe_proto_header {
> +	u32 pkt_type;
> +
> +	union {
> +		u32 data_size;
> +		struct {
> +			u16 data_size;
> +			u16 offset;
> +		} partial;
> +	};
> +} __packed;
> +
> +/* see hv_ringbuffer_read() and hv_ringbuffer_write() */
> +#define PREV_INDICES_LEN	(sizeof(u64))
> +
> +#define HVSOCK_HEADER_LEN	(sizeof(struct vmpacket_descriptor) + \
> +				 sizeof(struct vmpipe_proto_header))
> +
> +#define HVSOCK_PKT_LEN(payload_len)	(HVSOCK_HEADER_LEN + \
> +					ALIGN((payload_len), 8) + \
> +					PREV_INDICES_LEN)
> +
> +#define HVSOCK_MIN_PKT_LEN	HVSOCK_PKT_LEN(1)
> +
>  #endif /* _HYPERV_H */

-- 
  Vitaly

RE: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dexuan Cui]

> From: Vitaly Kuznetsov
> Sent: Tuesday, July 21, 2015 22:07
> > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
>> ...
> > +int vmbus_recvpacket_hvsock(struct vmbus_channel *channel, void *buffer,
> > +			    u32 bufferlen, u32 *buffer_actual_len)
> > +{
> > +	struct vmpipe_proto_header *pipe_hdr;
> > +	struct vmpacket_descriptor *desc;
> > +	u32 packet_len, payload_len;
> > +	bool signal = false;
> > +	int ret;
> > +
> > +	*buffer_actual_len = 0;
> > +
> > +	if (bufferlen < HVSOCK_HEADER_LEN)
> > +		return -ENOBUFS;
> > +
> > +	ret = hv_ringbuffer_peek(&channel->inbound, buffer,
> > +				 HVSOCK_HEADER_LEN);
> > +	if (ret != 0)
> > +		return 0;
> 
> I'd suggest you do something like
> 
>     if (ret == -EAGIAIN)
>         return 0;
>     else if (ret)
>         return ret;
> 
> to make it future-proof (e.g. when a new error is returned by
> hv_ringbuffer_peek). And a comment would also be useful as it is unclear
> why we silence errors here.
Hi Vitaly,
Thanks! 
I think I made a mistake here:
the "if (ret != 0) return 0;" should be changed
to   "if (ret != 0) return ret;"

vmbus_recvpacket_hvsock() is only invoked in hvsock_recvmsg() in the
case of can_read == true && need_refill == true, which means 
the bytes-available-to-read in the ringbuffer is bigger than
HVSOCK_HEADER_LEN, so here hv_ringbuffer_peek() can only return 0.

I'll fix this in V4.

> > +void vmbus_get_hvsock_rw_status(struct vmbus_channel *channel,
> > +				bool *can_read, bool *can_write)
> > +{
> > +	u32 avl_read_bytes, avl_write_bytes, dummy;
> > +
> > +	if (can_read != NULL) {
> > +		hv_get_ringbuffer_available_space(&channel->inbound,
> > +						  &avl_read_bytes,
> > +						  &dummy);
> > +		*can_read = avl_read_bytes >= HVSOCK_MIN_PKT_LEN;
> > +	}
> > +
> > +	/* We write into the ringbuffer only when we're able to write a
> 
> Not sure why checkpatch.pl doesn't complain but according to the
> CodingStyle multi-line comments outside of drivers/net and net/ are
> supposed to start with and empty '/*' line.
Yeah, you're correct. I'll fix this in V4.

BTW, it seems the rule is not strictly obeyed:  :-)
kernel/sched/fair.c:7290:       /* don't kick the active_load_balance_cpu_stop,
kernel/pid.c:273:                       /* When all that is left in the pid namespace
kernel/watchdog.c:293:  /* check for a hardlockup e
kernel/signal.c:2376:   /* A signal was successfully delivered, and the

> > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> > @@ -885,6 +885,9 @@ extern int vmbus_sendpacket_ctl(struct
> vmbus_channel *channel,
> >  				  u32 flags,
> >  				  bool kick_q);
> >
> > +extern int vmbus_sendpacket_hvsock(struct vmbus_channel *channel,
> > +				   void *buf, u32 len);
> > +
> 
> I *think* if your argument list makes it to the second line it is supposed
> to be alligned with the first one (e.g. 'void' should start at the same
> position as 'struct' on the previous line).

I think it's indeed aligned in the patch, i.e., here the 's' and the 'v' are at the
same column.

If I use Vim's ":set list" command, it will like like

extern int vmbus_sendpacket_hvsock(struct vmbus_channel *channel,$
^I^I^I^I   void *buf, u32 len);$

Here I use 4 Tabs and 3 white spaces to make sure the 's' and the 'v' are at
the same column.
In the patch, due to the leading '+', they doesn't look like at the same column.

Please let me know if I'm not using the correct aligning method.
To be frank, I might depend too much on scripts/checkpatch.pl, which didn't
find obvious issues in the patch. :-)

> >  extern int vmbus_sendpacket_pagebuffer(struct vmbus_channel *channel,
> >  					    struct hv_page_buffer pagebuffers[],
> >  					    u32 pagecount,
> > @@ -934,6 +937,11 @@ extern int vmbus_recvpacket_raw(struct
> vmbus_channel *channel,
> >  				     u32 *buffer_actual_len,
> >  				     u64 *requestid);
> >
> > +extern int vmbus_recvpacket_hvsock(struct vmbus_channel *channel, void
> *buffer,
> > +				   u32 bufferlen, u32 *buffer_actual_len);
> > +
> 
> the same.
ditto

> > +extern void vmbus_get_hvsock_rw_status(struct vmbus_channel *channel,
> > +				       bool *can_read, bool *can_write);
> >
> 
> the same.
ditto.

Thanks,
-- Dexuan

RE: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dexuan Cui]

> From: Vitaly Kuznetsov
> Sent: Tuesday, July 21, 2015 22:07
> > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
>> ...
> > +int vmbus_recvpacket_hvsock(struct vmbus_channel *channel, void *buffer,
> > +			    u32 bufferlen, u32 *buffer_actual_len)
> > +{
> > +	struct vmpipe_proto_header *pipe_hdr;
> > +	struct vmpacket_descriptor *desc;
> > +	u32 packet_len, payload_len;
> > +	bool signal = false;
> > +	int ret;
> > +
> > +	*buffer_actual_len = 0;
> > +
> > +	if (bufferlen < HVSOCK_HEADER_LEN)
> > +		return -ENOBUFS;
> > +
> > +	ret = hv_ringbuffer_peek(&channel->inbound, buffer,
> > +				 HVSOCK_HEADER_LEN);
> > +	if (ret != 0)
> > +		return 0;
> 
> I'd suggest you do something like
> 
>     if (ret == -EAGIAIN)
>         return 0;
>     else if (ret)
>         return ret;
> 
> to make it future-proof (e.g. when a new error is returned by
> hv_ringbuffer_peek). And a comment would also be useful as it is unclear
> why we silence errors here.
Hi Vitaly,
Thanks! 
I think I made a mistake here:
the "if (ret != 0) return 0;" should be changed
to   "if (ret != 0) return ret;"

vmbus_recvpacket_hvsock() is only invoked in hvsock_recvmsg() in the
case of can_read == true && need_refill == true, which means 
the bytes-available-to-read in the ringbuffer is bigger than
HVSOCK_HEADER_LEN, so here hv_ringbuffer_peek() can only return 0.

I'll fix this in V4.

> > +void vmbus_get_hvsock_rw_status(struct vmbus_channel *channel,
> > +				bool *can_read, bool *can_write)
> > +{
> > +	u32 avl_read_bytes, avl_write_bytes, dummy;
> > +
> > +	if (can_read != NULL) {
> > +		hv_get_ringbuffer_available_space(&channel->inbound,
> > +						  &avl_read_bytes,
> > +						  &dummy);
> > +		*can_read = avl_read_bytes >= HVSOCK_MIN_PKT_LEN;
> > +	}
> > +
> > +	/* We write into the ringbuffer only when we're able to write a
> 
> Not sure why checkpatch.pl doesn't complain but according to the
> CodingStyle multi-line comments outside of drivers/net and net/ are
> supposed to start with and empty '/*' line.
Yeah, you're correct. I'll fix this in V4.

BTW, it seems the rule is not strictly obeyed:  :-)
kernel/sched/fair.c:7290:       /* don't kick the active_load_balance_cpu_stop,
kernel/pid.c:273:                       /* When all that is left in the pid namespace
kernel/watchdog.c:293:  /* check for a hardlockup e
kernel/signal.c:2376:   /* A signal was successfully delivered, and the

> > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> > @@ -885,6 +885,9 @@ extern int vmbus_sendpacket_ctl(struct
> vmbus_channel *channel,
> >  				  u32 flags,
> >  				  bool kick_q);
> >
> > +extern int vmbus_sendpacket_hvsock(struct vmbus_channel *channel,
> > +				   void *buf, u32 len);
> > +
> 
> I *think* if your argument list makes it to the second line it is supposed
> to be alligned with the first one (e.g. 'void' should start at the same
> position as 'struct' on the previous line).

I think it's indeed aligned in the patch, i.e., here the 's' and the 'v' are at the
same column.

If I use Vim's ":set list" command, it will like like

extern int vmbus_sendpacket_hvsock(struct vmbus_channel *channel,$
^I^I^I^I   void *buf, u32 len);$

Here I use 4 Tabs and 3 white spaces to make sure the 's' and the 'v' are at
the same column.
In the patch, due to the leading '+', they doesn't look like at the same column.

Please let me know if I'm not using the correct aligning method.
To be frank, I might depend too much on scripts/checkpatch.pl, which didn't
find obvious issues in the patch. :-)

> >  extern int vmbus_sendpacket_pagebuffer(struct vmbus_channel *channel,
> >  					    struct hv_page_buffer pagebuffers[],
> >  					    u32 pagecount,
> > @@ -934,6 +937,11 @@ extern int vmbus_recvpacket_raw(struct
> vmbus_channel *channel,
> >  				     u32 *buffer_actual_len,
> >  				     u64 *requestid);
> >
> > +extern int vmbus_recvpacket_hvsock(struct vmbus_channel *channel, void
> *buffer,
> > +				   u32 bufferlen, u32 *buffer_actual_len);
> > +
> 
> the same.
ditto

> > +extern void vmbus_get_hvsock_rw_status(struct vmbus_channel *channel,
> > +				       bool *can_read, bool *can_write);
> >
> 
> the same.
ditto.

Thanks,
-- Dexuan

RE: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dexuan Cui]

> From: Vitaly Kuznetsov
> Sent: Tuesday, July 21, 2015 22:07
> > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
>> ...
> > +int vmbus_recvpacket_hvsock(struct vmbus_channel *channel, void *buffer,
> > +			    u32 bufferlen, u32 *buffer_actual_len)
> > +{
> > +	struct vmpipe_proto_header *pipe_hdr;
> > +	struct vmpacket_descriptor *desc;
> > +	u32 packet_len, payload_len;
> > +	bool signal = false;
> > +	int ret;
> > +
> > +	*buffer_actual_len = 0;
> > +
> > +	if (bufferlen < HVSOCK_HEADER_LEN)
> > +		return -ENOBUFS;
> > +
> > +	ret = hv_ringbuffer_peek(&channel->inbound, buffer,
> > +				 HVSOCK_HEADER_LEN);
> > +	if (ret != 0)
> > +		return 0;
> 
> I'd suggest you do something like
> 
>     if (ret == -EAGIAIN)
>         return 0;
>     else if (ret)
>         return ret;
> 
> to make it future-proof (e.g. when a new error is returned by
> hv_ringbuffer_peek). And a comment would also be useful as it is unclear
> why we silence errors here.
Hi Vitaly,
Thanks! 
I think I made a mistake here:
the "if (ret != 0) return 0;" should be changed
to   "if (ret != 0) return ret;"

vmbus_recvpacket_hvsock() is only invoked in hvsock_recvmsg() in the
case of can_read == true && need_refill == true, which means 
the bytes-available-to-read in the ringbuffer is bigger than
HVSOCK_HEADER_LEN, so here hv_ringbuffer_peek() can only return 0.

I'll fix this in V4.

> > +void vmbus_get_hvsock_rw_status(struct vmbus_channel *channel,
> > +				bool *can_read, bool *can_write)
> > +{
> > +	u32 avl_read_bytes, avl_write_bytes, dummy;
> > +
> > +	if (can_read != NULL) {
> > +		hv_get_ringbuffer_available_space(&channel->inbound,
> > +						  &avl_read_bytes,
> > +						  &dummy);
> > +		*can_read = avl_read_bytes >= HVSOCK_MIN_PKT_LEN;
> > +	}
> > +
> > +	/* We write into the ringbuffer only when we're able to write a
> 
> Not sure why checkpatch.pl doesn't complain but according to the
> CodingStyle multi-line comments outside of drivers/net and net/ are
> supposed to start with and empty '/*' line.
Yeah, you're correct. I'll fix this in V4.

BTW, it seems the rule is not strictly obeyed:  :-)
kernel/sched/fair.c:7290:       /* don't kick the active_load_balance_cpu_stop,
kernel/pid.c:273:                       /* When all that is left in the pid namespace
kernel/watchdog.c:293:  /* check for a hardlockup e
kernel/signal.c:2376:   /* A signal was successfully delivered, and the

> > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> > @@ -885,6 +885,9 @@ extern int vmbus_sendpacket_ctl(struct
> vmbus_channel *channel,
> >  				  u32 flags,
> >  				  bool kick_q);
> >
> > +extern int vmbus_sendpacket_hvsock(struct vmbus_channel *channel,
> > +				   void *buf, u32 len);
> > +
> 
> I *think* if your argument list makes it to the second line it is supposed
> to be alligned with the first one (e.g. 'void' should start at the same
> position as 'struct' on the previous line).

I think it's indeed aligned in the patch, i.e., here the 's' and the 'v' are at the
same column.

If I use Vim's ":set list" command, it will like like

extern int vmbus_sendpacket_hvsock(struct vmbus_channel *channel,$
^I^I^I^I   void *buf, u32 len);$

Here I use 4 Tabs and 3 white spaces to make sure the 's' and the 'v' are at
the same column.
In the patch, due to the leading '+', they doesn't look like at the same column.

Please let me know if I'm not using the correct aligning method.
To be frank, I might depend too much on scripts/checkpatch.pl, which didn't
find obvious issues in the patch. :-)

> >  extern int vmbus_sendpacket_pagebuffer(struct vmbus_channel *channel,
> >  					    struct hv_page_buffer pagebuffers[],
> >  					    u32 pagecount,
> > @@ -934,6 +937,11 @@ extern int vmbus_recvpacket_raw(struct
> vmbus_channel *channel,
> >  				     u32 *buffer_actual_len,
> >  				     u64 *requestid);
> >
> > +extern int vmbus_recvpacket_hvsock(struct vmbus_channel *channel, void
> *buffer,
> > +				   u32 bufferlen, u32 *buffer_actual_len);
> > +
> 
> the same.
ditto

> > +extern void vmbus_get_hvsock_rw_status(struct vmbus_channel *channel,
> > +				       bool *can_read, bool *can_write);
> >
> 
> the same.
ditto.

Thanks,
-- Dexuan
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dan Carpenter]

On Wed, Jul 22, 2015 at 10:09:10AM +0000, Dexuan Cui wrote:
> > I'd suggest you do something like
> > 
> >     if (ret == -EAGIAIN)
> >         return 0;
> >     else if (ret)
> >         return ret;
> > 
> > to make it future-proof (e.g. when a new error is returned by
> > hv_ringbuffer_peek). And a comment would also be useful as it is unclear
> > why we silence errors here.
> Hi Vitaly,
> Thanks! 
> I think I made a mistake here:
> the "if (ret != 0) return 0;" should be changed
> to   "if (ret != 0) return ret;"

The double negative really doesn't not make the code more complicated.
I like using a quadruple negative instead.

	if (ret != 0 != 0)
		return ret;

regards,
dan carpenter

Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dan Carpenter]

On Wed, Jul 22, 2015 at 10:09:10AM +0000, Dexuan Cui wrote:
> > I'd suggest you do something like
> > 
> >     if (ret == -EAGIAIN)
> >         return 0;
> >     else if (ret)
> >         return ret;
> > 
> > to make it future-proof (e.g. when a new error is returned by
> > hv_ringbuffer_peek). And a comment would also be useful as it is unclear
> > why we silence errors here.
> Hi Vitaly,
> Thanks! 
> I think I made a mistake here:
> the "if (ret != 0) return 0;" should be changed
> to   "if (ret != 0) return ret;"

The double negative really doesn't not make the code more complicated.
I like using a quadruple negative instead.

	if (ret != 0 != 0)
		return ret;

regards,
dan carpenter

Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dan Carpenter]

On Wed, Jul 22, 2015 at 10:09:10AM +0000, Dexuan Cui wrote:
> > I'd suggest you do something like
> > 
> >     if (ret == -EAGIAIN)
> >         return 0;
> >     else if (ret)
> >         return ret;
> > 
> > to make it future-proof (e.g. when a new error is returned by
> > hv_ringbuffer_peek). And a comment would also be useful as it is unclear
> > why we silence errors here.
> Hi Vitaly,
> Thanks! 
> I think I made a mistake here:
> the "if (ret != 0) return 0;" should be changed
> to   "if (ret != 0) return ret;"

The double negative really doesn't not make the code more complicated.
I like using a quadruple negative instead.

	if (ret != 0 != 0)
		return ret;

regards,
dan carpenter


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dexuan Cui]

> From: Dan Carpenter
> Sent: Wednesday, July 22, 2015 18:36
> To: Dexuan Cui
> On Wed, Jul 22, 2015 at 10:09:10AM +0000, Dexuan Cui wrote:
> > > I'd suggest you do something like
> > >
> > >     if (ret == -EAGIAIN)
> > >         return 0;
> > >     else if (ret)
> > >         return ret;
> > >
> > > to make it future-proof (e.g. when a new error is returned by
> > > hv_ringbuffer_peek). And a comment would also be useful as it is unclear
> > > why we silence errors here.
> > Hi Vitaly,
> > Thanks!
> > I think I made a mistake here:
> > the "if (ret != 0) return 0;" should be changed
> > to   "if (ret != 0) return ret;"
Usually 0 means success to me, so 
to me, "ret != 0" reads like "ret is not successful" and seems natural. 

The kind of usage is not rare in the kernel code:

decui@lin:~/linux-next$ grep 'if (ret != 0)' kernel/ include/ ipc/  -r  | wc -l
28
decui@lin:~/linux-next$ grep 'if (ret != 0)' drivers/   -r  | wc -l
1031
 
> The double negative really doesn't not make the code more complicated.
> I like using a quadruple negative instead.
> 
> 	if (ret != 0 != 0)
> 		return ret;
> dan carpenter
Hi Dan, I read this as a humor.  :-)

I'll take the suggestion and remember to use this in V4 and in future:
 
if (ret)
	return ret;

Thanks!

-- Dexuan

RE: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dexuan Cui]

> From: Dan Carpenter
> Sent: Wednesday, July 22, 2015 18:36
> To: Dexuan Cui
> On Wed, Jul 22, 2015 at 10:09:10AM +0000, Dexuan Cui wrote:
> > > I'd suggest you do something like
> > >
> > >     if (ret == -EAGIAIN)
> > >         return 0;
> > >     else if (ret)
> > >         return ret;
> > >
> > > to make it future-proof (e.g. when a new error is returned by
> > > hv_ringbuffer_peek). And a comment would also be useful as it is unclear
> > > why we silence errors here.
> > Hi Vitaly,
> > Thanks!
> > I think I made a mistake here:
> > the "if (ret != 0) return 0;" should be changed
> > to   "if (ret != 0) return ret;"
Usually 0 means success to me, so 
to me, "ret != 0" reads like "ret is not successful" and seems natural. 

The kind of usage is not rare in the kernel code:

decui@lin:~/linux-next$ grep 'if (ret != 0)' kernel/ include/ ipc/  -r  | wc -l
28
decui@lin:~/linux-next$ grep 'if (ret != 0)' drivers/   -r  | wc -l
1031
 
> The double negative really doesn't not make the code more complicated.
> I like using a quadruple negative instead.
> 
> 	if (ret != 0 != 0)
> 		return ret;
> dan carpenter
Hi Dan, I read this as a humor.  :-)

I'll take the suggestion and remember to use this in V4 and in future:
 
if (ret)
	return ret;

Thanks!

-- Dexuan

RE: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dexuan Cui]

> From: Dan Carpenter
> Sent: Wednesday, July 22, 2015 18:36
> To: Dexuan Cui
> On Wed, Jul 22, 2015 at 10:09:10AM +0000, Dexuan Cui wrote:
> > > I'd suggest you do something like
> > >
> > >     if (ret == -EAGIAIN)
> > >         return 0;
> > >     else if (ret)
> > >         return ret;
> > >
> > > to make it future-proof (e.g. when a new error is returned by
> > > hv_ringbuffer_peek). And a comment would also be useful as it is unclear
> > > why we silence errors here.
> > Hi Vitaly,
> > Thanks!
> > I think I made a mistake here:
> > the "if (ret != 0) return 0;" should be changed
> > to   "if (ret != 0) return ret;"
Usually 0 means success to me, so 
to me, "ret != 0" reads like "ret is not successful" and seems natural. 

The kind of usage is not rare in the kernel code:

decui@lin:~/linux-next$ grep 'if (ret != 0)' kernel/ include/ ipc/  -r  | wc -l
28
decui@lin:~/linux-next$ grep 'if (ret != 0)' drivers/   -r  | wc -l
1031
 
> The double negative really doesn't not make the code more complicated.
> I like using a quadruple negative instead.
> 
> 	if (ret != 0 != 0)
> 		return ret;
> dan carpenter
Hi Dan, I read this as a humor.  :-)

I'll take the suggestion and remember to use this in V4 and in future:
 
if (ret)
	return ret;

Thanks!

-- Dexuan

Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dan Carpenter]

On Thu, Jul 23, 2015 at 03:05:16AM +0000, Dexuan Cui wrote:
> The kind of usage is not rare in the kernel code:

Yeah.  But it's used 5% of the time.  If it's under 15% then there is a
risk that we'll write a checkpatch rule to enforce the standard way...
There are some places where != 0 is idiomatic, like when you are talking
about the number zero.  strcmp() and friends should always be != 0 or
== 0.

In this specific case, writing it as "if (ret != 0)" caused the bug.  If
we had written it as "if (ret) return ret;" then there are no zeroes so
wouldn't have been any temptation to return the zero instead of the ret.

> Hi Dan, I read this as a humor.  :-)

:)

regards,
dan carpenter

Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dan Carpenter]

On Thu, Jul 23, 2015 at 03:05:16AM +0000, Dexuan Cui wrote:
> The kind of usage is not rare in the kernel code:

Yeah.  But it's used 5% of the time.  If it's under 15% then there is a
risk that we'll write a checkpatch rule to enforce the standard way...
There are some places where != 0 is idiomatic, like when you are talking
about the number zero.  strcmp() and friends should always be != 0 or
== 0.

In this specific case, writing it as "if (ret != 0)" caused the bug.  If
we had written it as "if (ret) return ret;" then there are no zeroes so
wouldn't have been any temptation to return the zero instead of the ret.

> Hi Dan, I read this as a humor.  :-)

:)

regards,
dan carpenter

Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dan Carpenter]

On Thu, Jul 23, 2015 at 03:05:16AM +0000, Dexuan Cui wrote:
> The kind of usage is not rare in the kernel code:

Yeah.  But it's used 5% of the time.  If it's under 15% then there is a
risk that we'll write a checkpatch rule to enforce the standard way...
There are some places where != 0 is idiomatic, like when you are talking
about the number zero.  strcmp() and friends should always be != 0 or
== 0.

In this specific case, writing it as "if (ret != 0)" caused the bug.  If
we had written it as "if (ret) return ret;" then there are no zeroes so
wouldn't have been any temptation to return the zero instead of the ret.

> Hi Dan, I read this as a humor.  :-)

:)

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dan Carpenter]

On Thu, Jul 23, 2015 at 01:10:57PM +0300, Dan Carpenter wrote:
> In this specific case, writing it as "if (ret != 0)" caused the bug.  If
> we had written it as "if (ret) return ret;" then there are no zeroes so
> wouldn't have been any temptation to return the zero instead of the ret.

I did a search to see if returning the zero instead of the ret was a
common mistake and it seems like it might be.  I did:

grep 'if (ret != 0)' drivers/   -r -A1 -n | grep "return 0;" | perl -ne 's/.c-(\d+)-/.c:$1/; print'

drivers/gpu/drm/nouveau/nouveau_display.c:111                   return 0;
drivers/regulator/wm8400-regulator.c:47         return 0;
drivers/platform/x86/dell-laptop.c:859          return 0;
drivers/media/dvb-frontends/dibx000_common.c:213                                return 0;
drivers/media/dvb-frontends/dibx000_common.c:217                                return 0;
drivers/media/dvb-frontends/dibx000_common.c:235                                return 0;
drivers/media/dvb-frontends/dibx000_common.c:239                                return 0;
drivers/hv/channel.c:898                return 0;
drivers/hv/channel.c:944                return 0;

A bunch of those look suspicious but I don't know the subsystems well
enough to be sure.  Can you check the last two?

regards,
dan carpenter

Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dan Carpenter]

On Thu, Jul 23, 2015 at 01:10:57PM +0300, Dan Carpenter wrote:
> In this specific case, writing it as "if (ret != 0)" caused the bug.  If
> we had written it as "if (ret) return ret;" then there are no zeroes so
> wouldn't have been any temptation to return the zero instead of the ret.

I did a search to see if returning the zero instead of the ret was a
common mistake and it seems like it might be.  I did:

grep 'if (ret != 0)' drivers/   -r -A1 -n | grep "return 0;" | perl -ne 's/.c-(\d+)-/.c:$1/; print'

drivers/gpu/drm/nouveau/nouveau_display.c:111                   return 0;
drivers/regulator/wm8400-regulator.c:47         return 0;
drivers/platform/x86/dell-laptop.c:859          return 0;
drivers/media/dvb-frontends/dibx000_common.c:213                                return 0;
drivers/media/dvb-frontends/dibx000_common.c:217                                return 0;
drivers/media/dvb-frontends/dibx000_common.c:235                                return 0;
drivers/media/dvb-frontends/dibx000_common.c:239                                return 0;
drivers/hv/channel.c:898                return 0;
drivers/hv/channel.c:944                return 0;

A bunch of those look suspicious but I don't know the subsystems well
enough to be sure.  Can you check the last two?

regards,
dan carpenter

Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dan Carpenter]

On Thu, Jul 23, 2015 at 01:10:57PM +0300, Dan Carpenter wrote:
> In this specific case, writing it as "if (ret != 0)" caused the bug.  If
> we had written it as "if (ret) return ret;" then there are no zeroes so
> wouldn't have been any temptation to return the zero instead of the ret.

I did a search to see if returning the zero instead of the ret was a
common mistake and it seems like it might be.  I did:

grep 'if (ret != 0)' drivers/   -r -A1 -n | grep "return 0;" | perl -ne 's/.c-(\d+)-/.c:$1/; print'

drivers/gpu/drm/nouveau/nouveau_display.c:111                   return 0;
drivers/regulator/wm8400-regulator.c:47         return 0;
drivers/platform/x86/dell-laptop.c:859          return 0;
drivers/media/dvb-frontends/dibx000_common.c:213                                return 0;
drivers/media/dvb-frontends/dibx000_common.c:217                                return 0;
drivers/media/dvb-frontends/dibx000_common.c:235                                return 0;
drivers/media/dvb-frontends/dibx000_common.c:239                                return 0;
drivers/hv/channel.c:898                return 0;
drivers/hv/channel.c:944                return 0;

A bunch of those look suspicious but I don't know the subsystems well
enough to be sure.  Can you check the last two?

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dexuan Cui]

> From: Dan Carpenter
> Sent: Thursday, July 23, 2015 18:25
> On Thu, Jul 23, 2015 at 01:10:57PM +0300, Dan Carpenter wrote:
> > In this specific case, writing it as "if (ret != 0)" caused the bug.  If
> > we had written it as "if (ret) return ret;" then there are no zeroes so
> > wouldn't have been any temptation to return the zero instead of the ret.
> 
> I did a search to see if returning the zero instead of the ret was a
> common mistake and it seems like it might be.  I did:
> 
> grep 'if (ret != 0)' drivers/   -r -A1 -n | grep "return 0;" | perl -ne 's/.c-(\d+)-/.c:$1/;
> print'
> 
> drivers/gpu/drm/nouveau/nouveau_display.c:111                   return 0;
> drivers/regulator/wm8400-regulator.c:47         return 0;
> drivers/platform/x86/dell-laptop.c:859          return 0;
> drivers/media/dvb-frontends/dibx000_common.c:213                                return 0;
> drivers/media/dvb-frontends/dibx000_common.c:217                                return 0;
> drivers/media/dvb-frontends/dibx000_common.c:235                                return 0;
> drivers/media/dvb-frontends/dibx000_common.c:239                                return 0;
> drivers/hv/channel.c:898                return 0;
> drivers/hv/channel.c:944                return 0;
> 
> A bunch of those look suspicious but I don't know the subsystems well
> enough to be sure.  Can you check the last two?
> 
> dan carpenter

Thanks, Dan!

After I checked the code, I think there is no issue for the last two:
in the case of "if (ret != 0) return 0;",  the output parameter buffer_actual_len is
zero and it is explicitly checked by the callers.
This may seem not natural and I think we can improve it in future.

BTW, at the end of vmbus_recvpacket(), the "return 0;" should be "return ret;", but
since the output parameter buffer_actual_len is checked by the callers, I think it's
OK for now.

Thanks,
-- Dexuan

RE: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dexuan Cui]

> From: Dan Carpenter
> Sent: Thursday, July 23, 2015 18:25
> On Thu, Jul 23, 2015 at 01:10:57PM +0300, Dan Carpenter wrote:
> > In this specific case, writing it as "if (ret != 0)" caused the bug.  If
> > we had written it as "if (ret) return ret;" then there are no zeroes so
> > wouldn't have been any temptation to return the zero instead of the ret.
> 
> I did a search to see if returning the zero instead of the ret was a
> common mistake and it seems like it might be.  I did:
> 
> grep 'if (ret != 0)' drivers/   -r -A1 -n | grep "return 0;" | perl -ne 's/.c-(\d+)-/.c:$1/;
> print'
> 
> drivers/gpu/drm/nouveau/nouveau_display.c:111                   return 0;
> drivers/regulator/wm8400-regulator.c:47         return 0;
> drivers/platform/x86/dell-laptop.c:859          return 0;
> drivers/media/dvb-frontends/dibx000_common.c:213                                return 0;
> drivers/media/dvb-frontends/dibx000_common.c:217                                return 0;
> drivers/media/dvb-frontends/dibx000_common.c:235                                return 0;
> drivers/media/dvb-frontends/dibx000_common.c:239                                return 0;
> drivers/hv/channel.c:898                return 0;
> drivers/hv/channel.c:944                return 0;
> 
> A bunch of those look suspicious but I don't know the subsystems well
> enough to be sure.  Can you check the last two?
> 
> dan carpenter

Thanks, Dan!

After I checked the code, I think there is no issue for the last two:
in the case of "if (ret != 0) return 0;",  the output parameter buffer_actual_len is
zero and it is explicitly checked by the callers.
This may seem not natural and I think we can improve it in future.

BTW, at the end of vmbus_recvpacket(), the "return 0;" should be "return ret;", but
since the output parameter buffer_actual_len is checked by the callers, I think it's
OK for now.

Thanks,
-- Dexuan

RE: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dexuan Cui]

> From: Dan Carpenter
> Sent: Thursday, July 23, 2015 18:25
> On Thu, Jul 23, 2015 at 01:10:57PM +0300, Dan Carpenter wrote:
> > In this specific case, writing it as "if (ret != 0)" caused the bug.  If
> > we had written it as "if (ret) return ret;" then there are no zeroes so
> > wouldn't have been any temptation to return the zero instead of the ret.
> 
> I did a search to see if returning the zero instead of the ret was a
> common mistake and it seems like it might be.  I did:
> 
> grep 'if (ret != 0)' drivers/   -r -A1 -n | grep "return 0;" | perl -ne 's/.c-(\d+)-/.c:$1/;
> print'
> 
> drivers/gpu/drm/nouveau/nouveau_display.c:111                   return 0;
> drivers/regulator/wm8400-regulator.c:47         return 0;
> drivers/platform/x86/dell-laptop.c:859          return 0;
> drivers/media/dvb-frontends/dibx000_common.c:213                                return 0;
> drivers/media/dvb-frontends/dibx000_common.c:217                                return 0;
> drivers/media/dvb-frontends/dibx000_common.c:235                                return 0;
> drivers/media/dvb-frontends/dibx000_common.c:239                                return 0;
> drivers/hv/channel.c:898                return 0;
> drivers/hv/channel.c:944                return 0;
> 
> A bunch of those look suspicious but I don't know the subsystems well
> enough to be sure.  Can you check the last two?
> 
> dan carpenter

Thanks, Dan!

After I checked the code, I think there is no issue for the last two:
in the case of "if (ret != 0) return 0;",  the output parameter buffer_actual_len is
zero and it is explicitly checked by the callers.
This may seem not natural and I think we can improve it in future.

BTW, at the end of vmbus_recvpacket(), the "return 0;" should be "return ret;", but
since the output parameter buffer_actual_len is checked by the callers, I think it's
OK for now.

Thanks,
-- Dexuan
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Sudip Mukherjee]

On Thu, Jul 23, 2015 at 01:24:50PM +0300, Dan Carpenter wrote:
> On Thu, Jul 23, 2015 at 01:10:57PM +0300, Dan Carpenter wrote:
> > In this specific case, writing it as "if (ret != 0)" caused the bug.  If
> > we had written it as "if (ret) return ret;" then there are no zeroes so
> > wouldn't have been any temptation to return the zero instead of the ret.
> 
> I did a search to see if returning the zero instead of the ret was a
> common mistake and it seems like it might be.  I did:
> 
> grep 'if (ret != 0)' drivers/   -r -A1 -n | grep "return 0;" | perl -ne 's/.c-(\d+)-/.c:$1/; print'
> 
> drivers/gpu/drm/nouveau/nouveau_display.c:111                   return 0;
This is also ok, the function is supposed to return ret or-ed with the
relevant flags based on the scan position. It is considered error if 0
is returned (without any flag).

regards
sudip

Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Sudip Mukherjee]

On Thu, Jul 23, 2015 at 01:24:50PM +0300, Dan Carpenter wrote:
> On Thu, Jul 23, 2015 at 01:10:57PM +0300, Dan Carpenter wrote:
> > In this specific case, writing it as "if (ret != 0)" caused the bug.  If
> > we had written it as "if (ret) return ret;" then there are no zeroes so
> > wouldn't have been any temptation to return the zero instead of the ret.
> 
> I did a search to see if returning the zero instead of the ret was a
> common mistake and it seems like it might be.  I did:
> 
> grep 'if (ret != 0)' drivers/   -r -A1 -n | grep "return 0;" | perl -ne 's/.c-(\d+)-/.c:$1/; print'
> 
> drivers/gpu/drm/nouveau/nouveau_display.c:111                   return 0;
This is also ok, the function is supposed to return ret or-ed with the
relevant flags based on the scan position. It is considered error if 0
is returned (without any flag).

regards
sudip

Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Sudip Mukherjee]

On Thu, Jul 23, 2015 at 01:24:50PM +0300, Dan Carpenter wrote:
> On Thu, Jul 23, 2015 at 01:10:57PM +0300, Dan Carpenter wrote:
> > In this specific case, writing it as "if (ret != 0)" caused the bug.  If
> > we had written it as "if (ret) return ret;" then there are no zeroes so
> > wouldn't have been any temptation to return the zero instead of the ret.
> 
> I did a search to see if returning the zero instead of the ret was a
> common mistake and it seems like it might be.  I did:
> 
> grep 'if (ret != 0)' drivers/   -r -A1 -n | grep "return 0;" | perl -ne 's/.c-(\d+)-/.c:$1/; print'
> 
> drivers/gpu/drm/nouveau/nouveau_display.c:111                   return 0;
This is also ok, the function is supposed to return ret or-ed with the
relevant flags based on the scan position. It is considered error if 0
is returned (without any flag).

regards
sudip
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dan Carpenter]

On Fri, Jul 24, 2015 at 11:57:01AM +0530, Sudip Mukherjee wrote:
> This is also ok, the function is supposed to return ret or-ed with the
> relevant flags based on the scan position. It is considered error if 0
> is returned (without any flag).

Yeah.  You're right.  I looked through my list again this morning and
they all seem fine...

regards,
dan carpenter

Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dan Carpenter]

On Fri, Jul 24, 2015 at 11:57:01AM +0530, Sudip Mukherjee wrote:
> This is also ok, the function is supposed to return ret or-ed with the
> relevant flags based on the scan position. It is considered error if 0
> is returned (without any flag).

Yeah.  You're right.  I looked through my list again this morning and
they all seem fine...

regards,
dan carpenter

Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

[Dan Carpenter]

On Fri, Jul 24, 2015 at 11:57:01AM +0530, Sudip Mukherjee wrote:
> This is also ok, the function is supposed to return ret or-ed with the
> relevant flags based on the scan position. It is considered error if 0
> is returned (without any flag).

Yeah.  You're right.  I looked through my list again this morning and
they all seem fine...

regards,
dan carpenter