> -----Original Message----- > From: Vitaly Kuznetsov [mailto:vkuznets@xxxxxxxxxx] > Sent: Thursday, January 29, 2015 21:22 PM > To: Dexuan Cui > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; driverdev- > devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx; > jasowang@xxxxxxxxxx; KY Srinivasan; Haiyang Zhang > Subject: Re: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM > > Dexuan Cui <decui@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> > > Signed-off-by: Dexuan Cui <decui@xxxxxxxxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
