Date:	Thu, 4 Mar 2010 10:03:52 -0800
From:	Randy Dunlap <rdunlap@...otime.net>
To:	Hank Janssen <hjanssen@...rosoft.com>
Cc:	"'linux-kernel@...r.kernel.org'" <linux-kernel@...r.kernel.org>,
	"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
	"virtualization@...ts.osdl.org" <virtualization@...ts.osdl.org>,
	Greg KH <greg@...ah.com>,
	Haiyang Zhang <haiyangz@...rosoft.com>,
	Hashir Abdi <habdi@...rosoft.com>
Subject: Re: [PATCH 1/1] Stage: hv: Corrected all header comments to follow
 kernel-doc format

On Thu, 4 Mar 2010 17:48:18 +0000 Hank Janssen wrote:

> 
> From: Hank Janssen <hjanssen@...rosoft.com>
> 
> Removed kerneldoc /** from functions that should not have them.
> Added proper kerneldoc headers to functions that should have them.

Hi,

Most of the patch looks good.  I found a few nits to pick.
Please see below.

> Cc: Greg Kroah-Hartman <gregkh@...e.de>
> Signed-off-by: Hank Janssen <hjanssen@...rosoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@...rosoft.com>
> 
> ---
>  drivers/staging/hv/Channel.c     |   49 ++++++++++++++-----------
>  drivers/staging/hv/ChannelMgmt.c |   33 +++++++++--------
>  drivers/staging/hv/Connection.c  |   14 ++++----
>  drivers/staging/hv/Hv.c          |   18 +++++-----
>  drivers/staging/hv/NetVsc.c      |    8 ++--
>  drivers/staging/hv/StorVsc.c     |   10 +++---
>  drivers/staging/hv/TODO          |    1 -
>  drivers/staging/hv/Vmbus.c       |   26 +++++++-------
>  drivers/staging/hv/VmbusApi.h    |   19 ++++++++++
>  drivers/staging/hv/blkvsc_drv.c  |    6 ++--
>  drivers/staging/hv/netvsc_drv.c  |    4 +-
>  drivers/staging/hv/osd.c         |   70 +++++++++++++++++++++++++++++++++++
>  drivers/staging/hv/storvsc_drv.c |   14 ++++----
>  drivers/staging/hv/vmbus_drv.c   |   74 +++++++++++++++++++++++++------------
>  14 files changed, 233 insertions(+), 113 deletions(-)



diff --git a/drivers/staging/hv/VmbusApi.h b/drivers/staging/hv/VmbusApi.h index d089bb1..2e3a3b8 100644
> --- a/drivers/staging/hv/VmbusApi.h
> +++ b/drivers/staging/hv/VmbusApi.h
> @@ -84,6 +84,25 @@ struct hv_device_info {
>         struct hv_dev_port_info Outbound;
>  };
> 
> +/**
> + * struct vmbus_channel_interface - Contains member functions for vmbus channel
> + * @Open:      Open the channel
> + * @Close:     Close the channel
> + * @SendPacket:        Send a packet over the channel
> + * @SendPacketPageBuffer:      Send a single page buffer over the channel
> + * @SendPacketMultiPageBuffer: Send a multiple page buffers
> + * @RecvPacket:        Receive packet
> + * @RecvPacketRaw:     Receive Raw packet
> + * @EstablishGpadl:    Set up GPADL for ringbuffer
> + * @TeardownGpadl:     Teardown GPADL for ringbuffer
> + * @GetInfo:   Get info about the channel
> + *
> + * This structure contains function pointer to control vmbus channel
> + * behavior.
> + * None of these functions is externally callable, but they are used
> +for normal

There appears to be some unwanted line breaking ^here^.

> + * vmbus channel internal behavior.
> + * Only used by Hyper-V drivers.
> + */
>  struct vmbus_channel_interface {
>         int (*Open)(struct hv_device *Device, u32 SendBufferSize,
>                     u32 RecvRingBufferSize, void *UserData, u32 UserDataLen, diff --git a/drivers/staging/hv/blkvsc_drv.c b/drivers/staging/hv/blkvsc_drv.c index abeac12..c90a6aa 100644


and why are those 2 lines above joined/merged?
I suspect that it will be a bit difficult for Greg to apply this patch.


diff --git a/drivers/staging/hv/osd.c b/drivers/staging/hv/osd.c index 3a4793a..5afa94e 100644
> --- a/drivers/staging/hv/osd.c
> +++ b/drivers/staging/hv/osd.c
> @@ -77,6 +86,14 @@ void *osd_PageAlloc(unsigned int count)  }  EXPORT_SYMBOL_GPL(osd_PageAlloc);
> 
> +/**
> + * osd_PageFree() - Free pages
> + * @page:       Pointer to the first page to be freed
> + * @count:      Total number of Kernel pages you free
> + *
> + * Frees the pages allocated by osd_PageAlloc()
> + * Mainly used by Hyper-V drivers.
> + */
>  void osd_PageFree(void *page, unsigned int count)  {
>         free_pages((unsigned long)page, get_order(count * PAGE_SIZE)); @@ -85,6 +102,17 @@ void osd_PageFree(void *page, unsigned int count)  }  EXPORT_SYMBOL_GPL(osd_PageFree);
> 
> +/**
> + * osd_WaitEventCreate() - Create the event queue
> + *
> + * Allocates memory for a &struct osd_waitevent. And than calls

aha.  Correct struct reference, using '&'.  :)

> + * init_waitqueue_head to set up the wait queue for the event.
> + * This structure is usually part of a another structure that contains
> + * the actual Hyper-V device driver structure.
> + *
> + * Returns pointer to &struct osd_waitevent
> + * Mainly used by Hyper-V drivers.
> + */
>  struct osd_waitevent *osd_WaitEventCreate(void)  {
>         struct osd_waitevent *wait = kmalloc(sizeof(struct osd_waitevent), @@ -98,6 +126,19 @@ struct osd_waitevent *osd_WaitEventCreate(void)  }  EXPORT_SYMBOL_GPL(osd_WaitEventCreate);
> 
> +
> +/**
> + * osd_WaitEventSet() - Wake up the process
> + * @waitEvent: Structure to event to be woken up
> + *
> + * @waitevent is of type @struct osd_waitevent

Use '&' here also.

> + *
> + * Wake up the sleeping process so it can do some work.
> + * And set condition indicator in struct osd_waitevent to indicate
> + * the process is in a woken state.
> + *
> + * Only used by Network and Storage Hyper-V drivers.
> + */
>  void osd_WaitEventSet(struct osd_waitevent *waitEvent)  {
>         waitEvent->condition = 1;
> @@ -105,6 +146,20 @@ void osd_WaitEventSet(struct osd_waitevent *waitEvent)  }  EXPORT_SYMBOL_GPL(osd_WaitEventSet);
> 
> +/**
> + * osd_WaitEventWait() - Wait for event till condition is true
> + * @waitEvent: Structure to event to be put to sleep
> + *
> + * @waitevent is of type @struct osd_waitevent

Use '&' here also.

> + *
> + * Set up the process to sleep until waitEvent->condition get true.
> + * And set condition indicator in struct osd_waitevent to indicate
> + * the process is in a sleeping state.
> + *
> + * Returns the status of 'wait_event_interruptible()' system call
> + *
> + * Mainly used by Hyper-V drivers.
> + */
>  int osd_WaitEventWait(struct osd_waitevent *waitEvent)  {
>         int ret = 0;
> @@ -116,6 +171,21 @@ int osd_WaitEventWait(struct osd_waitevent *waitEvent)  }  EXPORT_SYMBOL_GPL(osd_WaitEventWait);
> 
> +/**
> + * osd_WaitEventWaitEx() - Wait for event or timeout for process wakeup
> + * @waitEvent: Structure to event to be put to sleep
> + * @TimeoutInMs:       Total number of Milliseconds to wait before waking up
> + *
> + * @waitevent is of type @struct osd_waitevent

Use '&' here also.

> + * Set up the process to sleep until @waitEvent->condition get true or
> + * @TimeoutInMs (Time out in Milliseconds) has been reached.
> + * And set condition indicator in struct osd_waitevent to indicate
> + * the process is in a sleeping state.
> + *
> + * Returns the status of 'wait_event_interruptible_timeout()' system
> +call
> + *
> + * Mainly used by Hyper-V drivers.
> + */
>  int osd_WaitEventWaitEx(struct osd_waitevent *waitEvent, u32 TimeoutInMs)  {
>         int ret = 0;



---
~Randy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists