Before the line vmbus_open() returns, srv->util_cb can be already running
and the variables, like util_fw_version, are needed by the srv->util_cb.
So we have to make sure the variables are initialized before the vmbus_open().
CC: "K. Y. Srinivasan" <[email protected]>
Reviewed-by: Vitaly Kuznetsov <[email protected]>
Signed-off-by: Dexuan Cui <[email protected]>
---
v2:
This is a RESEND.
I just added Vitaly's Reviewed-by.
drivers/hv/hv_util.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index 3b9c9ef..c5be773 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -340,12 +340,8 @@ static int util_probe(struct hv_device *dev,
set_channel_read_state(dev->channel, false);
- ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL, 0,
- srv->util_cb, dev->channel);
- if (ret)
- goto error;
-
hv_set_drvdata(dev, srv);
+
/*
* Based on the host; initialize the framework and
* service version numbers we will negotiate.
@@ -365,6 +361,11 @@ static int util_probe(struct hv_device *dev,
hb_srv_version = HB_VERSION;
}
+ ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL, 0,
+ srv->util_cb, dev->channel);
+ if (ret)
+ goto error;
+
return 0;
error:
--
1.9.1
On Mon, Feb 2, 2015 at 12:35 PM, Dexuan Cui <[email protected]> wrote:
> Before the line vmbus_open() returns, srv->util_cb can be already
> running
> and the variables, like util_fw_version, are needed by the
> srv->util_cb.
A questions is why we do this for util only? Can callbacks of other
devices be called before vmbus_open() returns?
>
> So we have to make sure the variables are initialized before the
> vmbus_open().
>
> CC: "K. Y. Srinivasan" <[email protected]>
> Reviewed-by: Vitaly Kuznetsov <[email protected]>
> Signed-off-by: Dexuan Cui <[email protected]>
> ---
>
> v2:
> This is a RESEND.
> I just added Vitaly's Reviewed-by.
>
> drivers/hv/hv_util.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index 3b9c9ef..c5be773 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -340,12 +340,8 @@ static int util_probe(struct hv_device *dev,
>
> set_channel_read_state(dev->channel, false);
>
> - ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL,
> 0,
> - srv->util_cb, dev->channel);
> - if (ret)
> - goto error;
> -
> hv_set_drvdata(dev, srv);
> +
This seems unnecessary.
>
> /*
> * Based on the host; initialize the framework and
> * service version numbers we will negotiate.
> @@ -365,6 +361,11 @@ static int util_probe(struct hv_device *dev,
> hb_srv_version = HB_VERSION;
> }
>
> + ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL,
> 0,
> + srv->util_cb, dev->channel);
> + if (ret)
> + goto error;
> +
> return 0;
>
> error:
> --
> 1.9.1
>
> -----Original Message-----
> From: Jason Wang [mailto:[email protected]]
> Sent: Monday, February 2, 2015 17:36 PM
> To: Dexuan Cui
> Cc: [email protected]; [email protected]; driverdev-
> [email protected]; [email protected]; [email protected]; KY
> Srinivasan; [email protected]; Haiyang Zhang
> Subject: Re: [PATCH v2 1/3] hv: hv_util: move vmbus_open() to a later place
>
>
>
> On Mon, Feb 2, 2015 at 12:35 PM, Dexuan Cui <[email protected]> wrote:
> > Before the line vmbus_open() returns, srv->util_cb can be already
> > running
> > and the variables, like util_fw_version, are needed by the
> > srv->util_cb.
>
> A questions is why we do this for util only? Can callbacks of other
> devices be called before vmbus_open() returns?
The variables are used in vmbus_prep_negotiate_resp(), which is only for
the util devices.
I think the other devices should already handle the similar issue properly.
If this is not the case, we need to fix them too.
>
> >
> > So we have to make sure the variables are initialized before the
> > vmbus_open().
> >
> > CC: "K. Y. Srinivasan" <[email protected]>
> > Reviewed-by: Vitaly Kuznetsov <[email protected]>
> > Signed-off-by: Dexuan Cui <[email protected]>
> > ---
> >
> > v2:
> > This is a RESEND.
> > I just added Vitaly's Reviewed-by.
> >
> > drivers/hv/hv_util.c | 11 ++++++-----
> > 1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> > index 3b9c9ef..c5be773 100644
> > --- a/drivers/hv/hv_util.c
> > +++ b/drivers/hv/hv_util.c
> > @@ -340,12 +340,8 @@ static int util_probe(struct hv_device *dev,
> >
> > set_channel_read_state(dev->channel, false);
> >
> > - ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL,
> > 0,
> > - srv->util_cb, dev->channel);
> > - if (ret)
> > - goto error;
> > -
> > hv_set_drvdata(dev, srv);
> > +
>
> This seems unnecessary.
Yeah, it's an empty line, splitting the line and the below comment.
I'm Ok to not have it.
> > /*
> > * Based on the host; initialize the framework and
> > * service version numbers we will negotiate.
> > @@ -365,6 +361,11 @@ static int util_probe(struct hv_device *dev,
> > hb_srv_version = HB_VERSION;
> > }
> >
> > + ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL,
> > 0,
> > + srv->util_cb, dev->channel);
> > + if (ret)
> > + goto error;
> > +
> > return 0;
> >
> > error:
> > --
> > 1.9.1
> >
-- Dexuan
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Mon, Feb 2, 2015 at 6:09 PM, Dexuan Cui <[email protected]> wrote:
>> -----Original Message-----
>> From: Jason Wang [mailto:[email protected]]
>> Sent: Monday, February 2, 2015 17:36 PM
>> To: Dexuan Cui
>> Cc: [email protected]; [email protected];
>> driverdev-
>> [email protected]; [email protected]; [email protected]; KY
>> Srinivasan; [email protected]; Haiyang Zhang
>> Subject: Re: [PATCH v2 1/3] hv: hv_util: move vmbus_open() to a
>> later place
>>
>>
>>
>> On Mon, Feb 2, 2015 at 12:35 PM, Dexuan Cui <[email protected]>
>> wrote:
>> > Before the line vmbus_open() returns, srv->util_cb can be already
>> > running
>> > and the variables, like util_fw_version, are needed by the
>> > srv->util_cb.
>>
>> A questions is why we do this for util only? Can callbacks of other
>> devices be called before vmbus_open() returns?
> The variables are used in vmbus_prep_negotiate_resp(), which is only
> for
> the util devices.
>
> I think the other devices should already handle the similar issue
> properly.
> If this is not the case, we need to fix them too.
Better to check all the others, e.g in balloon_probe(), it call
hv_set_drvdata() after vmbus_open() and dose several datas setups in
the middle. If balloon_onchannelcallback() could be called before
hv_set_drvdata(), the code looks wrong.
Thanks
> -----Original Message-----
> From: Jason Wang [mailto:[email protected]]
> Sent: Monday, February 2, 2015 7:09 PM
> To: Dexuan Cui
> Cc: [email protected]; [email protected]; driverdev-
> [email protected]; [email protected]; [email protected]; KY
> Srinivasan; [email protected]; Haiyang Zhang
> Subject: RE: [PATCH v2 1/3] hv: hv_util: move vmbus_open() to a later place
>
>
>
> On Mon, Feb 2, 2015 at 6:09 PM, Dexuan Cui <[email protected]> wrote:
> >> -----Original Message-----
> >> From: Jason Wang [mailto:[email protected]]
> >> Sent: Monday, February 2, 2015 17:36 PM
> >> To: Dexuan Cui
> >> Cc: [email protected]; [email protected];
> >> driverdev-
> >> [email protected]; [email protected]; [email protected]; KY
> >> Srinivasan; [email protected]; Haiyang Zhang
> >> Subject: Re: [PATCH v2 1/3] hv: hv_util: move vmbus_open() to a
> >> later place
> >>
> >>
> >>
> >> On Mon, Feb 2, 2015 at 12:35 PM, Dexuan Cui <[email protected]>
> >> wrote:
> >> > Before the line vmbus_open() returns, srv->util_cb can be already
> >> > running > and the variables, like util_fw_version, are needed by
> >> the > srv->util_cb.
> >>
> >> A questions is why we do this for util only? Can callbacks of other
> >> devices be called before vmbus_open() returns?
> > The variables are used in vmbus_prep_negotiate_resp(), which is only
> > for the util devices.
> >
> > I think the other devices should already handle the similar issue
> > properly.
> > If this is not the case, we need to fix them too.
>
> Better to check all the others, e.g in balloon_probe(), it call
> hv_set_drvdata() after vmbus_open() and dose several datas setups in the
> middle. If balloon_onchannelcallback() could be called before
> hv_set_drvdata(), the code looks wrong.
Jason,
For all other device types, the guest initiates the communication with the host and potentially
negotiates appropriate (supported) version with the host. For the services packaged in the util
driver, the flow is a little different - the host pushes the version information into the guest. So,
the fix Dexuan made is only needed in the util driver.
Regards,
K. Y
>
> Thanks
>
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Tue, Feb 3, 2015 at 11:30 AM, KY Srinivasan <[email protected]>
wrote:
>
>
>> -----Original Message-----
>> From: Jason Wang [mailto:[email protected]]
>> Sent: Monday, February 2, 2015 7:09 PM
>> To: Dexuan Cui
>> Cc: [email protected]; [email protected];
>> driverdev-
>> [email protected]; [email protected]; [email protected]; KY
>> Srinivasan; [email protected]; Haiyang Zhang
>> Subject: RE: [PATCH v2 1/3] hv: hv_util: move vmbus_open() to a
>> later place
>>
>>
>>
>> On Mon, Feb 2, 2015 at 6:09 PM, Dexuan Cui <[email protected]>
>> wrote:
>> >> -----Original Message-----
>> >> From: Jason Wang [mailto:[email protected]]
>> >> Sent: Monday, February 2, 2015 17:36 PM
>> >> To: Dexuan Cui
>> >> Cc: [email protected]; [email protected];
>> >> driverdev-
>> >> [email protected]; [email protected];
>> [email protected]; KY
>> >> Srinivasan; [email protected]; Haiyang Zhang
>> >> Subject: Re: [PATCH v2 1/3] hv: hv_util: move vmbus_open() to a
>> >> later place
>> >>
>> >>
>> >>
>> >> On Mon, Feb 2, 2015 at 12:35 PM, Dexuan Cui
>> <[email protected]>
>> >> wrote:
>> >> > Before the line vmbus_open() returns, srv->util_cb can be
>> already
>> >> > running > and the variables, like util_fw_version, are needed
>> by
>> >> the > srv->util_cb.
>> >>
>> >> A questions is why we do this for util only? Can callbacks of
>> other
>> >> devices be called before vmbus_open() returns?
>> > The variables are used in vmbus_prep_negotiate_resp(), which is
>> only
>> > for the util devices.
>> >
>> > I think the other devices should already handle the similar issue
>> > properly.
>> > If this is not the case, we need to fix them too.
>>
>> Better to check all the others, e.g in balloon_probe(), it call
>> hv_set_drvdata() after vmbus_open() and dose several datas setups
>> in the
>> middle. If balloon_onchannelcallback() could be called before
>> hv_set_drvdata(), the code looks wrong.
>
> Jason,
>
> For all other device types, the guest initiates the communication
> with the host and potentially
> negotiates appropriate (supported) version with the host. For the
> services packaged in the util
> driver, the flow is a little different - the host pushes the version
> information into the guest. So,
> the fix Dexuan made is only needed in the util driver.
>
> Regards,
>
> K. Y
Thanks, so you mean for other device, it won't get any interrupt before
guest negotiate the version with host?
>
>>
>> Thanks
>>
>>
>
> -----Original Message-----
> From: Jason Wang [mailto:[email protected]]
> Sent: Monday, February 2, 2015 7:38 PM
> To: KY Srinivasan
> Cc: Dexuan Cui; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Haiyang Zhang
> Subject: RE: [PATCH v2 1/3] hv: hv_util: move vmbus_open() to a later place
>
>
>
> On Tue, Feb 3, 2015 at 11:30 AM, KY Srinivasan <[email protected]>
> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Jason Wang [mailto:[email protected]]
> >> Sent: Monday, February 2, 2015 7:09 PM
> >> To: Dexuan Cui
> >> Cc: [email protected]; [email protected];
> >> driverdev-
> >> [email protected]; [email protected]; [email protected]; KY
> >> Srinivasan; [email protected]; Haiyang Zhang
> >> Subject: RE: [PATCH v2 1/3] hv: hv_util: move vmbus_open() to a
> >> later place
> >>
> >>
> >>
> >> On Mon, Feb 2, 2015 at 6:09 PM, Dexuan Cui <[email protected]>
> >> wrote:
> >> >> -----Original Message-----
> >> >> From: Jason Wang [mailto:[email protected]] >> Sent: Monday,
> >> February 2, 2015 17:36 PM >> To: Dexuan Cui >> Cc:
> >> [email protected]; [email protected]; >>
> >> driverdev- >> [email protected]; [email protected];
> >> [email protected]; KY >> Srinivasan; [email protected]; Haiyang
> >> Zhang >> Subject: Re: [PATCH v2 1/3] hv: hv_util: move vmbus_open()
> >> to a >> later place >> >> >> >> On Mon, Feb 2, 2015 at 12:35
> >> PM, Dexuan Cui <[email protected]> >> wrote:
> >> >> > Before the line vmbus_open() returns, srv->util_cb can be
> >> already >> > running > and the variables, like util_fw_version, are
> >> needed by >> the > srv->util_cb.
> >> >>
> >> >> A questions is why we do this for util only? Can callbacks of
> >> other >> devices be called before vmbus_open() returns?
> >> > The variables are used in vmbus_prep_negotiate_resp(), which is
> >> only > for the util devices.
> >> >
> >> > I think the other devices should already handle the similar issue
> >> > properly.
> >> > If this is not the case, we need to fix them too.
> >>
> >> Better to check all the others, e.g in balloon_probe(), it call
> >> hv_set_drvdata() after vmbus_open() and dose several datas setups in
> >> the middle. If balloon_onchannelcallback() could be called before
> >> hv_set_drvdata(), the code looks wrong.
> >
> > Jason,
> >
> > For all other device types, the guest initiates the communication with
> > the host and potentially negotiates appropriate (supported) version
> > with the host. For the services packaged in the util driver, the flow
> > is a little different - the host pushes the version information into
> > the guest. So, the fix Dexuan made is only needed in the util driver.
> >
> > Regards,
> >
> > K. Y
>
> Thanks, so you mean for other device, it won't get any interrupt before guest
> negotiate the version with host?
Yes, the guest initiates the version negotiation. Are you seeing something different?
K. Y
> >
> >>
> >> Thanks
> >>
> >>
> >
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Tue, Feb 3, 2015 at 11:40 AM, KY Srinivasan <[email protected]>
wrote:
>
>
>> -----Original Message-----
>> From: Jason Wang [mailto:[email protected]]
>> Sent: Monday, February 2, 2015 7:38 PM
>> To: KY Srinivasan
>> Cc: Dexuan Cui; [email protected];
>> [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; Haiyang Zhang
>> Subject: RE: [PATCH v2 1/3] hv: hv_util: move vmbus_open() to a
>> later place
>>
>>
>>
>> On Tue, Feb 3, 2015 at 11:30 AM, KY Srinivasan <[email protected]>
>> wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: Jason Wang [mailto:[email protected]]
>> >> Sent: Monday, February 2, 2015 7:09 PM
>> >> To: Dexuan Cui
>> >> Cc: [email protected]; [email protected];
>> >> driverdev-
>> >> [email protected]; [email protected];
>> [email protected]; KY
>> >> Srinivasan; [email protected]; Haiyang Zhang
>> >> Subject: RE: [PATCH v2 1/3] hv: hv_util: move vmbus_open() to a
>> >> later place
>> >>
>> >>
>> >>
>> >> On Mon, Feb 2, 2015 at 6:09 PM, Dexuan Cui <[email protected]>
>> >> wrote:
>> >> >> -----Original Message-----
>> >> >> From: Jason Wang [mailto:[email protected]] >> Sent:
>> Monday,
>> >> February 2, 2015 17:36 PM >> To: Dexuan Cui >> Cc:
>> >> [email protected]; [email protected]; >>
>> >> driverdev- >> [email protected]; [email protected];
>> >> [email protected]; KY >> Srinivasan; [email protected];
>> Haiyang
>> >> Zhang >> Subject: Re: [PATCH v2 1/3] hv: hv_util: move
>> vmbus_open()
>> >> to a >> later place >> >> >> >> On Mon, Feb 2, 2015 at
>> 12:35
>> >> PM, Dexuan Cui <[email protected]> >> wrote:
>> >> >> > Before the line vmbus_open() returns, srv->util_cb can be
>> >> already >> > running > and the variables, like
>> util_fw_version, are
>> >> needed by >> the > srv->util_cb.
>> >> >>
>> >> >> A questions is why we do this for util only? Can callbacks
>> of
>> >> other >> devices be called before vmbus_open() returns?
>> >> > The variables are used in vmbus_prep_negotiate_resp(), which
>> is
>> >> only > for the util devices.
>> >> >
>> >> > I think the other devices should already handle the similar
>> issue
>> >> > properly.
>> >> > If this is not the case, we need to fix them too.
>> >>
>> >> Better to check all the others, e.g in balloon_probe(), it call
>> >> hv_set_drvdata() after vmbus_open() and dose several datas
>> setups in
>> >> the middle. If balloon_onchannelcallback() could be called
>> before
>> >> hv_set_drvdata(), the code looks wrong.
>> >
>> > Jason,
>> >
>> > For all other device types, the guest initiates the communication
>> with
>> > the host and potentially negotiates appropriate (supported)
>> version
>> > with the host. For the services packaged in the util driver, the
>> flow
>> > is a little different - the host pushes the version information
>> into
>> > the guest. So, the fix Dexuan made is only needed in the util
>> driver.
>> >
>> > Regards,
>> >
>> > K. Y
>>
>> Thanks, so you mean for other device, it won't get any interrupt
>> before guest
>> negotiate the version with host?
>
> Yes, the guest initiates the version negotiation. Are you seeing
> something different?
>
> K. Y
No, thanks.
On Mon, Feb 2, 2015 at 12:35 PM, Dexuan Cui <[email protected]> wrote:
> Before the line vmbus_open() returns, srv->util_cb can be already
> running
> and the variables, like util_fw_version, are needed by the
> srv->util_cb.
>
> So we have to make sure the variables are initialized before the
> vmbus_open().
>
> CC: "K. Y. Srinivasan" <[email protected]>
> Reviewed-by: Vitaly Kuznetsov <[email protected]>
> Signed-off-by: Dexuan Cui <[email protected]>
> ---
>
> v2:
> This is a RESEND.
> I just added Vitaly's Reviewed-by.
>
> drivers/hv/hv_util.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index 3b9c9ef..c5be773 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -340,12 +340,8 @@ static int util_probe(struct hv_device *dev,
>
> set_channel_read_state(dev->channel, false);
>
> - ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL,
> 0,
> - srv->util_cb, dev->channel);
> - if (ret)
> - goto error;
> -
> hv_set_drvdata(dev, srv);
> +
> /*
> * Based on the host; initialize the framework and
> * service version numbers we will negotiate.
> @@ -365,6 +361,11 @@ static int util_probe(struct hv_device *dev,
> hb_srv_version = HB_VERSION;
> }
>
> + ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL,
> 0,
> + srv->util_cb, dev->channel);
> + if (ret)
> + goto error;
> +
> return 0;
>
> error:
> --
> 1.9.1
Reviewed-by: Jason Wang <[email protected]>