Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability
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 -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability
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 -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability
On Thu, Jul 23, 2015 at 03:05:16AM +, 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 -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability
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:898return 0; drivers/hv/channel.c:944return 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 -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability
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:898return 0; drivers/hv/channel.c:944return 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 -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability
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 -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability
On Wed, Jul 22, 2015 at 10:09:10AM +, 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 -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability
From: Dan Carpenter Sent: Wednesday, July 22, 2015 18:36 To: Dexuan Cui On Wed, Jul 22, 2015 at 10:09:10AM +, 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 -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability
This will be used by the coming net/hvsock driver. Signed-off-by: Dexuan Cui de...@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). +
Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability
Dexuan Cui de...@microsoft.com writes: This will be used by the coming net/hvsock driver. Signed-off-by: Dexuan Cui de...@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, +