> -----Original Message----- > From: KY Srinivasan > Sent: Monday, February 2, 2015 3:42 AM > To: Dexuan Cui; Vitaly Kuznetsov > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; driverdev- > devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx; > jasowang@xxxxxxxxxx; 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@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 > > > > > -----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. > > 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@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
