From: Olaf Hering <olaf@aepfle.de>
To: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] Drivers: hv: util: Fix a bug in version negotiation code for util services
Date: Wed, 4 Sep 2013 17:06:48 +0200 [thread overview]
Message-ID: <20130904150648.GA9162@aepfle.de> (raw)
In-Reply-To: <20130904135754.GA2010@aepfle.de>
On Wed, Sep 04, Olaf Hering wrote:
> I suggest to let callers deal with error handling. Also as a cleanup,
> vmbus_prep_negotiate_resp does not make use of the negop passed to it.
> So that arg can be removed.
Simplify vmbus_prep_negotiate_resp. If we take the optimistic approach
that negotiation will not fail for any of the callers of
vmbus_prep_negotiate_resp this patch on top of yours should fix 2008 and
2012r2.
Olaf
---
drivers/hv/channel_mgmt.c | 22 +++++++++-------------
drivers/hv/hv_kvp.c | 8 ++++----
drivers/hv/hv_snapshot.c | 3 +--
drivers/hv/hv_util.c | 7 +++----
include/linux/hyperv.h | 4 +---
5 files changed, 18 insertions(+), 26 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 12ec8c8..dcaad3e 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -41,7 +41,6 @@ struct vmbus_channel_message_table_entry {
/**
* vmbus_prep_negotiate_resp() - Create default response for Hyper-V Negotiate message
* @icmsghdrp: Pointer to msg header structure
- * @icmsg_negotiate: Pointer to negotiate message structure
* @buf: Raw buffer channel data
*
* @icmsghdrp is of type &struct icmsg_hdr.
@@ -54,10 +53,10 @@ struct vmbus_channel_message_table_entry {
*
* Mainly used by Hyper-V drivers.
*/
-bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
- struct icmsg_negotiate *negop, u8 *buf,
+bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp, u8 *buf,
int fw_version, int srv_version)
{
+ struct icmsg_negotiate *negop;
int icframe_major, icframe_minor;
int icmsg_major, icmsg_minor;
int fw_major, fw_minor;
@@ -65,7 +64,6 @@ bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
int i;
bool found_match = false;
- icmsghdrp->icmsgsize = 0x10;
fw_major = (fw_version >> 16);
fw_minor = (fw_version & 0xFFFF);
@@ -116,19 +114,17 @@ bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
* version numbers we can support.
*/
-fw_error:
- if (!found_match) {
- negop->icframe_vercnt = 0;
- negop->icmsg_vercnt = 0;
- } else {
+ if (found_match) {
+ icmsghdrp->icmsgsize = 0x10;
negop->icframe_vercnt = 1;
negop->icmsg_vercnt = 1;
+ negop->icversion_data[0].major = icframe_major;
+ negop->icversion_data[0].minor = icframe_minor;
+ negop->icversion_data[1].major = icmsg_major;
+ negop->icversion_data[1].minor = icmsg_minor;
}
- negop->icversion_data[0].major = icframe_major;
- negop->icversion_data[0].minor = icframe_minor;
- negop->icversion_data[1].major = icmsg_major;
- negop->icversion_data[1].minor = icmsg_minor;
+fw_error:
return found_match;
}
diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index 5312720..31e338a 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -584,7 +584,6 @@ void hv_kvp_onchannelcallback(void *context)
struct hv_kvp_msg *kvp_msg;
struct icmsg_hdr *icmsghdrp;
- struct icmsg_negotiate *negop = NULL;
if (kvp_transaction.active) {
/*
@@ -607,14 +606,15 @@ void hv_kvp_onchannelcallback(void *context)
* We start with win8 version and if the host cannot
* support that we use the previous version.
*/
- if (vmbus_prep_negotiate_resp(icmsghdrp, negop,
+ if (vmbus_prep_negotiate_resp(icmsghdrp,
recv_buffer, UTIL_FW_MAJOR_MINOR,
WIN8_SRV_MAJOR_MINOR))
goto done;
- vmbus_prep_negotiate_resp(icmsghdrp, negop,
+ if (vmbus_prep_negotiate_resp(icmsghdrp,
recv_buffer, UTIL_FW_MAJOR_MINOR,
- WIN7_SRV_MAJOR_MINOR);
+ WIN7_SRV_MAJOR_MINOR))
+ goto done;
} else {
kvp_msg = (struct hv_kvp_msg *)&recv_buffer[
diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
index e4572f3..51e8203 100644
--- a/drivers/hv/hv_snapshot.c
+++ b/drivers/hv/hv_snapshot.c
@@ -170,7 +170,6 @@ void hv_vss_onchannelcallback(void *context)
struct icmsg_hdr *icmsghdrp;
- struct icmsg_negotiate *negop = NULL;
if (vss_transaction.active) {
/*
@@ -189,7 +188,7 @@ void hv_vss_onchannelcallback(void *context)
sizeof(struct vmbuspipe_hdr)];
if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
- vmbus_prep_negotiate_resp(icmsghdrp, negop,
+ vmbus_prep_negotiate_resp(icmsghdrp,
recv_buffer, UTIL_FW_MAJOR_MINOR,
VSS_MAJOR_MINOR);
} else {
diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index c16164d..01d7b17 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -88,7 +88,6 @@ static void shutdown_onchannelcallback(void *context)
struct shutdown_msg_data *shutdown_msg;
struct icmsg_hdr *icmsghdrp;
- struct icmsg_negotiate *negop = NULL;
vmbus_recvpacket(channel, shut_txf_buf,
PAGE_SIZE, &recvlen, &requestid);
@@ -98,7 +97,7 @@ static void shutdown_onchannelcallback(void *context)
sizeof(struct vmbuspipe_hdr)];
if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
- vmbus_prep_negotiate_resp(icmsghdrp, negop,
+ vmbus_prep_negotiate_resp(icmsghdrp,
shut_txf_buf, UTIL_FW_MAJOR_MINOR,
SHUTDOWN_MAJOR_MINOR);
} else {
@@ -225,7 +224,7 @@ static void timesync_onchannelcallback(void *context)
sizeof(struct vmbuspipe_hdr)];
if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
- vmbus_prep_negotiate_resp(icmsghdrp, NULL, time_txf_buf,
+ vmbus_prep_negotiate_resp(icmsghdrp, time_txf_buf,
UTIL_FW_MAJOR_MINOR,
TIMESYNCH_MAJOR_MINOR);
} else {
@@ -266,7 +265,7 @@ static void heartbeat_onchannelcallback(void *context)
sizeof(struct vmbuspipe_hdr)];
if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
- vmbus_prep_negotiate_resp(icmsghdrp, NULL,
+ vmbus_prep_negotiate_resp(icmsghdrp,
hbeat_txf_buf, UTIL_FW_MAJOR_MINOR,
HEARTBEAT_MAJOR_MINOR);
} else {
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 4994907..084a858 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1502,9 +1502,7 @@ struct hyperv_service_callback {
};
#define MAX_SRV_VER 0x7ffffff
-extern bool vmbus_prep_negotiate_resp(struct icmsg_hdr *,
- struct icmsg_negotiate *, u8 *, int,
- int);
+extern bool vmbus_prep_negotiate_resp(struct icmsg_hdr *, u8 *, int, int);
int hv_kvp_init(struct hv_util_service *);
void hv_kvp_deinit(void);
next prev parent reply other threads:[~2013-09-04 15:06 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-07-02 17:31 [PATCH 1/1] Drivers: hv: util: Fix a bug in version negotiation code for util services K. Y. Srinivasan
2013-07-17 12:35 ` KY Srinivasan
2013-07-17 17:12 ` gregkh
2013-07-17 17:18 ` KY Srinivasan
2013-09-04 13:57 ` Olaf Hering
2013-09-04 15:06 ` Olaf Hering [this message]
2013-09-04 17:33 ` KY Srinivasan
2013-09-04 17:39 ` Olaf Hering
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130904150648.GA9162@aepfle.de \
--to=olaf@aepfle.de \
--cc=gregkh@linuxfoundation.org \
--cc=kys@microsoft.com \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox