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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----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.

> 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


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




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux