> 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@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
