[PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM

[PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM

[Dexuan Cui]

Without this patch, the state is put to CHANNEL_OPENING_STATE, and when
the driver is loaded next time, vmbus_open() will fail immediately due to
newchannel->state != CHANNEL_OPEN_STATE.

CC: "K. Y. Srinivasan" <kys@microsoft.com>
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/hv/channel.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 2978f5e..26dcf26 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -89,9 +89,10 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
 	out = (void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO,
 		get_order(send_ringbuffer_size + recv_ringbuffer_size));
 
-	if (!out)
-		return -ENOMEM;
-
+	if (!out) {
+		err = -ENOMEM;
+		goto error0;
+	}
 
 	in = (void *)((unsigned long)out + send_ringbuffer_size);
 
@@ -199,6 +200,7 @@ error0:
 	free_pages((unsigned long)out,
 		get_order(send_ringbuffer_size + recv_ringbuffer_size));
 	kfree(open_info);
+	newchannel->state = CHANNEL_OPEN_STATE;
 	return err;
 }
 EXPORT_SYMBOL_GPL(vmbus_open);
-- 
1.9.1

[PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM

[Dexuan Cui]

Without this patch, the state is put to CHANNEL_OPENING_STATE, and when
the driver is loaded next time, vmbus_open() will fail immediately due to
newchannel->state != CHANNEL_OPEN_STATE.

CC: "K. Y. Srinivasan" <kys@microsoft.com>
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/hv/channel.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 2978f5e..26dcf26 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -89,9 +89,10 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
 	out = (void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO,
 		get_order(send_ringbuffer_size + recv_ringbuffer_size));
 
-	if (!out)
-		return -ENOMEM;
-
+	if (!out) {
+		err = -ENOMEM;
+		goto error0;
+	}
 
 	in = (void *)((unsigned long)out + send_ringbuffer_size);
 
@@ -199,6 +200,7 @@ error0:
 	free_pages((unsigned long)out,
 		get_order(send_ringbuffer_size + recv_ringbuffer_size));
 	kfree(open_info);
+	newchannel->state = CHANNEL_OPEN_STATE;
 	return err;
 }
 EXPORT_SYMBOL_GPL(vmbus_open);
-- 
1.9.1

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

Re: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM

[Vitaly Kuznetsov]

Dexuan Cui <decui@microsoft.com> writes:

> Without this patch, the state is put to CHANNEL_OPENING_STATE, and when
> the driver is loaded next time, vmbus_open() will fail immediately due to
> newchannel->state != CHANNEL_OPEN_STATE.

The patch makes sense, but I have one small doubt. We call vmbus_open
from probe functions of various devices. E.g. in hyperv-keyboard we
have:
 error = vmbus_open(...)
 if (error)
     goto err_free_mem;

and we don't call vmbus_close(...) on this path so no
CHANNELMSG_CLOSECHANNEL will be send. Who's gonna retry probe? Wouldn't
it be better to close the channel?

>
> CC: "K. Y. Srinivasan" <kys@microsoft.com>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/hv/channel.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 2978f5e..26dcf26 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -89,9 +89,10 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
>  	out = (void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO,
>  		get_order(send_ringbuffer_size + recv_ringbuffer_size));
>
> -	if (!out)
> -		return -ENOMEM;
> -
> +	if (!out) {
> +		err = -ENOMEM;
> +		goto error0;
> +	}
>
>  	in = (void *)((unsigned long)out + send_ringbuffer_size);
>
> @@ -199,6 +200,7 @@ error0:
>  	free_pages((unsigned long)out,
>  		get_order(send_ringbuffer_size + recv_ringbuffer_size));
>  	kfree(open_info);
> +	newchannel->state = CHANNEL_OPEN_STATE;
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(vmbus_open);

-- 
  Vitaly

Re: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM

[Vitaly Kuznetsov]

Dexuan Cui <decui@microsoft.com> writes:

> Without this patch, the state is put to CHANNEL_OPENING_STATE, and when
> the driver is loaded next time, vmbus_open() will fail immediately due to
> newchannel->state != CHANNEL_OPEN_STATE.

The patch makes sense, but I have one small doubt. We call vmbus_open
from probe functions of various devices. E.g. in hyperv-keyboard we
have:
 error = vmbus_open(...)
 if (error)
     goto err_free_mem;

and we don't call vmbus_close(...) on this path so no
CHANNELMSG_CLOSECHANNEL will be send. Who's gonna retry probe? Wouldn't
it be better to close the channel?

>
> CC: "K. Y. Srinivasan" <kys@microsoft.com>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/hv/channel.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 2978f5e..26dcf26 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -89,9 +89,10 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
>  	out = (void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO,
>  		get_order(send_ringbuffer_size + recv_ringbuffer_size));
>
> -	if (!out)
> -		return -ENOMEM;
> -
> +	if (!out) {
> +		err = -ENOMEM;
> +		goto error0;
> +	}
>
>  	in = (void *)((unsigned long)out + send_ringbuffer_size);
>
> @@ -199,6 +200,7 @@ error0:
>  	free_pages((unsigned long)out,
>  		get_order(send_ringbuffer_size + recv_ringbuffer_size));
>  	kfree(open_info);
> +	newchannel->state = CHANNEL_OPEN_STATE;
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(vmbus_open);

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

RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM

[Dexuan Cui]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Thursday, January 29, 2015 21:22 PM
> To: Dexuan Cui
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org; driverdev-
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com; KY Srinivasan; Haiyang Zhang
> Subject: Re: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM
> 
> Dexuan Cui <decui@microsoft.com> writes:
> 
> > Without this patch, the state is put to CHANNEL_OPENING_STATE, and when
> > the driver is loaded next time, vmbus_open() will fail immediately due to
> > newchannel->state != CHANNEL_OPEN_STATE.
> 
> The patch makes sense, but I have one small doubt. We call vmbus_open
> from probe functions of various devices. E.g. in hyperv-keyboard we
> have:
>  error = vmbus_open(...)
>  if (error)
>      goto err_free_mem;
> 
> and we don't call vmbus_close(...) on this path so no
> CHANNELMSG_CLOSECHANNEL will be send.
Exactly.

> Who's gonna retry probe?
The user can try 'rmmod' and 'modprobe' the module to re-probe.

> Wouldn't it be better to close the channel?
Good question!

In your example, due to " goto err_free_mem", we don't run
vmbus_close(), so the memory allocated for the ringbuffer is actually leaked!

Next time when we reload the module, vmbus_open() will allocate new
memory for the ringbuffer.

KY, Haiyang,
Can you please confirm this issue?

Thanks,
-- Dexuan

> 
> >
> > CC: "K. Y. Srinivasan" <kys@microsoft.com>
> > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > ---
> >  drivers/hv/channel.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> > index 2978f5e..26dcf26 100644
> > --- a/drivers/hv/channel.c
> > +++ b/drivers/hv/channel.c
> > @@ -89,9 +89,10 @@ int vmbus_open(struct vmbus_channel *newchannel,
> u32 send_ringbuffer_size,
> >  	out = (void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO,
> >  		get_order(send_ringbuffer_size + recv_ringbuffer_size));
> >
> > -	if (!out)
> > -		return -ENOMEM;
> > -
> > +	if (!out) {
> > +		err = -ENOMEM;
> > +		goto error0;
> > +	}
> >
> >  	in = (void *)((unsigned long)out + send_ringbuffer_size);
> >
> > @@ -199,6 +200,7 @@ error0:
> >  	free_pages((unsigned long)out,
> >  		get_order(send_ringbuffer_size + recv_ringbuffer_size));
> >  	kfree(open_info);
> > +	newchannel->state = CHANNEL_OPEN_STATE;
> >  	return err;
> >  }
> >  EXPORT_SYMBOL_GPL(vmbus_open);
> 
> --
>   Vitaly

RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM

[Dexuan Cui]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Thursday, January 29, 2015 21:22 PM
> To: Dexuan Cui
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org; driverdev-
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com; KY Srinivasan; Haiyang Zhang
> Subject: Re: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM
> 
> Dexuan Cui <decui@microsoft.com> writes:
> 
> > Without this patch, the state is put to CHANNEL_OPENING_STATE, and when
> > the driver is loaded next time, vmbus_open() will fail immediately due to
> > newchannel->state != CHANNEL_OPEN_STATE.
> 
> The patch makes sense, but I have one small doubt. We call vmbus_open
> from probe functions of various devices. E.g. in hyperv-keyboard we
> have:
>  error = vmbus_open(...)
>  if (error)
>      goto err_free_mem;
> 
> and we don't call vmbus_close(...) on this path so no
> CHANNELMSG_CLOSECHANNEL will be send.
Exactly.

> Who's gonna retry probe?
The user can try 'rmmod' and 'modprobe' the module to re-probe.

> Wouldn't it be better to close the channel?
Good question!

In your example, due to " goto err_free_mem", we don't run
vmbus_close(), so the memory allocated for the ringbuffer is actually leaked!

Next time when we reload the module, vmbus_open() will allocate new
memory for the ringbuffer.

KY, Haiyang,
Can you please confirm this issue?

Thanks,
-- Dexuan

> 
> >
> > CC: "K. Y. Srinivasan" <kys@microsoft.com>
> > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > ---
> >  drivers/hv/channel.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> > index 2978f5e..26dcf26 100644
> > --- a/drivers/hv/channel.c
> > +++ b/drivers/hv/channel.c
> > @@ -89,9 +89,10 @@ int vmbus_open(struct vmbus_channel *newchannel,
> u32 send_ringbuffer_size,
> >  	out = (void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO,
> >  		get_order(send_ringbuffer_size + recv_ringbuffer_size));
> >
> > -	if (!out)
> > -		return -ENOMEM;
> > -
> > +	if (!out) {
> > +		err = -ENOMEM;
> > +		goto error0;
> > +	}
> >
> >  	in = (void *)((unsigned long)out + send_ringbuffer_size);
> >
> > @@ -199,6 +200,7 @@ error0:
> >  	free_pages((unsigned long)out,
> >  		get_order(send_ringbuffer_size + recv_ringbuffer_size));
> >  	kfree(open_info);
> > +	newchannel->state = CHANNEL_OPEN_STATE;
> >  	return err;
> >  }
> >  EXPORT_SYMBOL_GPL(vmbus_open);
> 
> --
>   Vitaly
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM

[KY Srinivasan]

> -----Original Message-----
> From: Dexuan Cui
> Sent: Thursday, January 29, 2015 9:03 PM
> To: Vitaly Kuznetsov
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org; driverdev-
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com; KY Srinivasan; Haiyang Zhang
> Subject: RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on
> ENOMEM
> 
> > -----Original Message-----
> > From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> > Sent: Thursday, January 29, 2015 21:22 PM
> > To: Dexuan Cui
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > driverdev- devel@linuxdriverproject.org; olaf@aepfle.de;
> > apw@canonical.com; jasowang@redhat.com; KY Srinivasan; Haiyang Zhang
> > Subject: Re: [PATCH 3/3] hv: vmbus_open(): reset the channel state on
> > ENOMEM
> >
> > Dexuan Cui <decui@microsoft.com> writes:
> >
> > > Without this patch, the state is put to CHANNEL_OPENING_STATE, and
> > > when the driver is loaded next time, vmbus_open() will fail
> > > immediately due to
> > > newchannel->state != CHANNEL_OPEN_STATE.
> >
> > The patch makes sense, but I have one small doubt. We call vmbus_open
> > from probe functions of various devices. E.g. in hyperv-keyboard we
> > have:
> >  error = vmbus_open(...)
> >  if (error)
> >      goto err_free_mem;
> >
> > and we don't call vmbus_close(...) on this path so no
> > CHANNELMSG_CLOSECHANNEL will be send.

If vmbus_open() fails, depending on where the failure occurred, the necessary rollback is expected to have
been done in the vmbus_open() function itself. In vmbus_open function  the last thing we do is to send the request
to the host to open the channel. If this fails, then there is no need to close the channel.

> Exactly.
> 
> > Who's gonna retry probe?
> The user can try 'rmmod' and 'modprobe' the module to re-probe.
> 
> > Wouldn't it be better to close the channel?
> Good question!
We close the channel only if the open() of the channel succeeded.

> 
> In your example, due to " goto err_free_mem", we don't run vmbus_close(),
> so the memory allocated for the ringbuffer is actually leaked!
> 
> Next time when we reload the module, vmbus_open() will allocate new
> memory for the ringbuffer.

I must be missing something here. When vmbus_open() fails, we will rollback the state and free up the memory
allocated for the ring buffer.

As I look at the vmbus_open() code I do see an issue with regards cleaning up the gpadl state and I am sending a patch for this
shortly. I will base this on top of the this series from Dexuan.

Regards,

K. Y

RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM

[KY Srinivasan]

> -----Original Message-----
> From: Dexuan Cui
> Sent: Thursday, January 29, 2015 9:03 PM
> To: Vitaly Kuznetsov
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org; driverdev-
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com; KY Srinivasan; Haiyang Zhang
> Subject: RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on
> ENOMEM
> 
> > -----Original Message-----
> > From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> > Sent: Thursday, January 29, 2015 21:22 PM
> > To: Dexuan Cui
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > driverdev- devel@linuxdriverproject.org; olaf@aepfle.de;
> > apw@canonical.com; jasowang@redhat.com; KY Srinivasan; Haiyang Zhang
> > Subject: Re: [PATCH 3/3] hv: vmbus_open(): reset the channel state on
> > ENOMEM
> >
> > Dexuan Cui <decui@microsoft.com> writes:
> >
> > > Without this patch, the state is put to CHANNEL_OPENING_STATE, and
> > > when the driver is loaded next time, vmbus_open() will fail
> > > immediately due to
> > > newchannel->state != CHANNEL_OPEN_STATE.
> >
> > The patch makes sense, but I have one small doubt. We call vmbus_open
> > from probe functions of various devices. E.g. in hyperv-keyboard we
> > have:
> >  error = vmbus_open(...)
> >  if (error)
> >      goto err_free_mem;
> >
> > and we don't call vmbus_close(...) on this path so no
> > CHANNELMSG_CLOSECHANNEL will be send.

If vmbus_open() fails, depending on where the failure occurred, the necessary rollback is expected to have
been done in the vmbus_open() function itself. In vmbus_open function  the last thing we do is to send the request
to the host to open the channel. If this fails, then there is no need to close the channel.

> Exactly.
> 
> > Who's gonna retry probe?
> The user can try 'rmmod' and 'modprobe' the module to re-probe.
> 
> > Wouldn't it be better to close the channel?
> Good question!
We close the channel only if the open() of the channel succeeded.

> 
> In your example, due to " goto err_free_mem", we don't run vmbus_close(),
> so the memory allocated for the ringbuffer is actually leaked!
> 
> Next time when we reload the module, vmbus_open() will allocate new
> memory for the ringbuffer.

I must be missing something here. When vmbus_open() fails, we will rollback the state and free up the memory
allocated for the ring buffer.

As I look at the vmbus_open() code I do see an issue with regards cleaning up the gpadl state and I am sending a patch for this
shortly. I will base this on top of the this series from Dexuan.

Regards,

K. Y

RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM

[Dexuan Cui]

> -----Original Message-----
> From: KY Srinivasan
> Sent: Monday, February 2, 2015 3:42 AM
> To: Dexuan Cui; Vitaly Kuznetsov
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org; driverdev-
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com; Haiyang Zhang
> Subject: RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM
> > -----Original Message-----
> > From: Dexuan Cui
> > Sent: Thursday, January 29, 2015 9:03 PM
> > To: Vitaly Kuznetsov
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org; driverdev-
> > devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> > jasowang@redhat.com; KY Srinivasan; Haiyang Zhang
> > Subject: RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on
> > ENOMEM
> >
> > > -----Original Message-----
> > > From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> > > Sent: Thursday, January 29, 2015 21:22 PM
> > > To: Dexuan Cui
> > > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > driverdev- devel@linuxdriverproject.org; olaf@aepfle.de;
> > > apw@canonical.com; jasowang@redhat.com; KY Srinivasan; Haiyang Zhang
> > > Subject: Re: [PATCH 3/3] hv: vmbus_open(): reset the channel state on
> > > ENOMEM
> > >
> > > Dexuan Cui <decui@microsoft.com> writes:
> > >
> > > > Without this patch, the state is put to CHANNEL_OPENING_STATE, and
> > > > when the driver is loaded next time, vmbus_open() will fail
> > > > immediately due to
> > > > newchannel->state != CHANNEL_OPEN_STATE.
> > >
> > > The patch makes sense, but I have one small doubt. We call vmbus_open
> > > from probe functions of various devices. E.g. in hyperv-keyboard we
> > > have:
> > >  error = vmbus_open(...)
> > >  if (error)
> > >      goto err_free_mem;
> > >
> > > and we don't call vmbus_close(...) on this path so no
> > > CHANNELMSG_CLOSECHANNEL will be send.
> 
> If vmbus_open() fails, depending on where the failure occurred, the necessary
> rollback is expected to have
> been done in the vmbus_open() function itself. In vmbus_open function  the last
> thing we do is to send the request
> to the host to open the channel. If this fails, then there is no need to close the
> channel.
Got it.

> > Exactly.
> >
> > > Who's gonna retry probe?
> > The user can try 'rmmod' and 'modprobe' the module to re-probe.
> >
> > > Wouldn't it be better to close the channel?
> > Good question!
> We close the channel only if the open() of the channel succeeded.
Got it.

> >
> > In your example, due to " goto err_free_mem", we don't run vmbus_close(),
> > so the memory allocated for the ringbuffer is actually leaked!
> >
> > Next time when we reload the module, vmbus_open() will allocate new
> > memory for the ringbuffer.
> 
> I must be missing something here. When vmbus_open() fails, we will rollback the
> state and free up the memory
> allocated for the ring buffer.
KY,
Sorry, I didn't check the code carefully...
You're correct. There is no issue here.

> 
> As I look at the vmbus_open() code I do see an issue with regards cleaning up
> the gpadl state and I am sending a patch for this
> shortly. I will base this on top of the this series from Dexuan.
Thanks, KY!
I'm not sure you meant the below line at the end of vmbus_open():
        if (err == 0)
                newchannel->state = CHANNEL_OPENED_STATE;
        ELSE
	???

-- Dexuan

RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM

[Dexuan Cui]

> -----Original Message-----
> From: KY Srinivasan
> Sent: Monday, February 2, 2015 3:42 AM
> To: Dexuan Cui; Vitaly Kuznetsov
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org; driverdev-
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com; Haiyang Zhang
> Subject: RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM
> > -----Original Message-----
> > From: Dexuan Cui
> > Sent: Thursday, January 29, 2015 9:03 PM
> > To: Vitaly Kuznetsov
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org; driverdev-
> > devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> > jasowang@redhat.com; KY Srinivasan; Haiyang Zhang
> > Subject: RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on
> > ENOMEM
> >
> > > -----Original Message-----
> > > From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> > > Sent: Thursday, January 29, 2015 21:22 PM
> > > To: Dexuan Cui
> > > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > driverdev- devel@linuxdriverproject.org; olaf@aepfle.de;
> > > apw@canonical.com; jasowang@redhat.com; KY Srinivasan; Haiyang Zhang
> > > Subject: Re: [PATCH 3/3] hv: vmbus_open(): reset the channel state on
> > > ENOMEM
> > >
> > > Dexuan Cui <decui@microsoft.com> writes:
> > >
> > > > Without this patch, the state is put to CHANNEL_OPENING_STATE, and
> > > > when the driver is loaded next time, vmbus_open() will fail
> > > > immediately due to
> > > > newchannel->state != CHANNEL_OPEN_STATE.
> > >
> > > The patch makes sense, but I have one small doubt. We call vmbus_open
> > > from probe functions of various devices. E.g. in hyperv-keyboard we
> > > have:
> > >  error = vmbus_open(...)
> > >  if (error)
> > >      goto err_free_mem;
> > >
> > > and we don't call vmbus_close(...) on this path so no
> > > CHANNELMSG_CLOSECHANNEL will be send.
> 
> If vmbus_open() fails, depending on where the failure occurred, the necessary
> rollback is expected to have
> been done in the vmbus_open() function itself. In vmbus_open function  the last
> thing we do is to send the request
> to the host to open the channel. If this fails, then there is no need to close the
> channel.
Got it.

> > Exactly.
> >
> > > Who's gonna retry probe?
> > The user can try 'rmmod' and 'modprobe' the module to re-probe.
> >
> > > Wouldn't it be better to close the channel?
> > Good question!
> We close the channel only if the open() of the channel succeeded.
Got it.

> >
> > In your example, due to " goto err_free_mem", we don't run vmbus_close(),
> > so the memory allocated for the ringbuffer is actually leaked!
> >
> > Next time when we reload the module, vmbus_open() will allocate new
> > memory for the ringbuffer.
> 
> I must be missing something here. When vmbus_open() fails, we will rollback the
> state and free up the memory
> allocated for the ring buffer.
KY,
Sorry, I didn't check the code carefully...
You're correct. There is no issue here.

> 
> As I look at the vmbus_open() code I do see an issue with regards cleaning up
> the gpadl state and I am sending a patch for this
> shortly. I will base this on top of the this series from Dexuan.
Thanks, KY!
I'm not sure you meant the below line at the end of vmbus_open():
        if (err == 0)
                newchannel->state = CHANNEL_OPENED_STATE;
        ELSE
	???

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