Hi,

	It was recommended that I use a completion in a driver I am working on. 
While figuring out how to use one, I noticed that there was no kernel 
doc block comments. I am trying to add them. I would rather not have to 
respin the patch for corrections.

--- include/linux/completion.h.orig	2008-08-13 00:56:52.000000000 -0700
+++ include/linux/completion.h	2008-08-19 17:47:49.000000000 -0700
@@ -10,6 +10,14 @@

  #include <linux/wait.h>

+/**
+ * struct completion - structure used to maintain state for a "completion"
+ *
+ * This is the opaque structure used to maintain the state for a 
"completion".
+ * See also:  complete(), wait_for_completion() (and friends _timeout,
+ * _interruptible, _interruptible_timeout, and _killable), 
init_completion(),
+ * and macros DECLARE_COMPLETION() and INIT_COMPLETION().
+ */
  struct completion {
  	unsigned int done;
  	wait_queue_head_t wait;
@@ -21,6 +29,14 @@
  #define COMPLETION_INITIALIZER_ONSTACK(work) \
  	({ init_completion(&work); work; })

+/**
+ * DECLARE_COMPLETION: - declare and initialize a completion structure
+ * @work:  identifier for the completion structure
+ *
+ * This macro declares and initializes a completion structure. 
Generally used
+ * for static declarations. You should use the _ONSTACK variant for 
automatic
+ * variables.
+ */
  #define DECLARE_COMPLETION(work) \
  	struct completion work = COMPLETION_INITIALIZER(work)

@@ -29,6 +45,13 @@
   * completions - so we use the _ONSTACK() variant for those that
   * are on the kernel stack:
   */
+/**
+ * DECLARE_COMPLETION_ONSTACK: - declare and initialize a completion 
structure
+ * @work:  identifier for the completion structure
+ *
+ * This macro declares and initializes a completion structure on the kernel
+ * stack.
+ */
  #ifdef CONFIG_LOCKDEP
  # define DECLARE_COMPLETION_ONSTACK(work) \
  	struct completion work = COMPLETION_INITIALIZER_ONSTACK(work)
@@ -36,6 +59,13 @@
  # define DECLARE_COMPLETION_ONSTACK(work) ...
[ message continues ]

Rather than documenting exactly how the queuing and wakeup occurs on
all functions, you should document it once. i.e. that completions
currently use FIFO queuing. It is probably best to do this at the
definition of the struct completion.

The reason is that if the implementation changes (e.g. to support
priorities and inheritence) the comments are then incorrect and
then there's lots of comments to remove^Wchange.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--

[ message continues ]

the text looks good to me - but many of the lines are line-wrapped 
(longer than 80 chars) - which should be avoided for free-flowing text.

	Ingo
--

[ message continues ]

Kevin, as a side note:  LDD3 chapter 5 pages 114 ff has some guidance.
http://lwn.net/Kernel/LDD3/
I'm only pointing to it for the unlikely case that you didn't come
across LDD3 yet.  The dead tree version is affordable too, and more
comfortable to read than the online version.
-- 
Stefan Richter
-=====-==--- =--- =-=--
http://arcgraph.de/sr/
--

[ message continues ]

Oh! I have that! That is how I figured out that complete() is used to
signal that the wait thingies don't have to wait anymore.

Here is the latest (maybe line wrap is set to 99 so it won't be goofed
anymore?):

--- include/linux/completion.h.orig	2008-08-13 00:56:52.000000000 -0700
+++ include/linux/completion.h	2008-08-20 01:47:21.000000000 -0700
@@ -10,6 +10,18 @@

  #include <linux/wait.h>

+/**
+ * struct completion - structure used to maintain state for a "completion"
+ *
+ * This is the opaque structure used to maintain the state for a "completion".
+ * Completions currently use a FIFO to queue threads that have to wait for
+ * the "completion" event.
+ *
+ * See also:  complete(), wait_for_completion() (and friends _timeout,
+ * _interruptible, _interruptible_timeout, and _killable), init_completion(),
+ * and macros DECLARE_COMPLETION(), DECLARE_COMPLETION_ONSTACK(), and
+ * INIT_COMPLETION().
+ */
  struct completion {
  	unsigned int done;
  	wait_queue_head_t wait;
@@ -21,6 +33,14 @@
  #define COMPLETION_INITIALIZER_ONSTACK(work) \
  	({ init_completion(&work); work; })

+/**
+ * DECLARE_COMPLETION: - declare and initialize a completion structure
+ * @work:  identifier for the completion structure
+ *
+ * This macro declares and initializes a completion structure. Generally used
+ * for static declarations. You should use the _ONSTACK variant for automatic
+ * variables.
+ */
  #define DECLARE_COMPLETION(work) \
  	struct completion work = COMPLETION_INITIALIZER(work)

@@ -29,6 +49,13 @@
   * completions - so we use the _ONSTACK() variant for those that
   * are on the kernel stack:
   */
+/**
+ * DECLARE_COMPLETION_ONSTACK: - declare and initialize a completion structure
+ * @work:  identifier for the completion structure
+ *
+ * This macro declares and initializes a completion structure on the kernel
+ * stack.
+ */
  #ifdef CONFIG_LOCKDEP
  # define DECLARE_COMPLETION_ONSTACK(work) \
  	struct completion work = ...
[ message continues ]

the patch wont apply as it has whitespace corruption (double spaces). 
Please check Documentation/email-clients.txt.

	Ingo
--

[ message continues ]

The double spacing between the diffs? That will prevent it from applying?

I'm still just trying to get the content correct.

Who should this patch be sent to when it is ready? Also, I think I remember
reading in submitting patches that patches should be cced to the general
linux-kernel list. This correct?

kevin
--

[ message continues ]

No. It's a per line corruption. Instead of:

"+<space>code"

your mail had:

"+<space><space>code"


me would be fine, it's scheduler code. I can take MIME attachments too 
if that's easier for you.

	Ingo
--

[ message continues ]