Re: [PATCH] Add kerneldoc for flush_scheduled_work()

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Randy Dunlap]

From: hannes@cmpxchg.org

On Thu, Aug 13, 2009 at 09:25:14AM +0200, Ingo Molnar wrote:
> 
> * James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > On Wed, 2009-08-12 at 10:13 -0400, Alan Stern wrote:
> > > On Wed, 12 Aug 2009, Ingo Molnar wrote:
> > > 
> > > > > And here I was thinking kerneldoc doesn't actually work 
> > > > > like that, but perhaps Randy fixed it so the initial 
> > > > > description can line-wrap?
> > > 
> > > Yes, that's what I thought too.  If kerneldoc has been fixed 
> > > then the description line certainly should get wrapped.
> > 
> > I really don't think it needs to be fixed: it's a feature not a 
> > bug.  It requires people writing kernel doc actually to think of 
> > one line summaries.
> 
> As long as the argument is that it's good to have limitations just 
> because it has good effects as well (which the gist of your argument 
> seems to be), i disagree.
> 
> That's a very basic argument of freedom. Just consider the Gestapo 
> which was also a 'feature' to keep criminals in check. Did you know 
> that there were record low levels of petty criminality both in nazi 
> Germany and during communism (and under just about any totalitarian 
> regime)? Still nobody in their right mind is arguing that just due 
> to that they are the right social model ...

| Although I really like how you Godwin'd kerneldoc comments ;-), we do
| have other features that are features because of their limiting effect
| all over the place, don't we?  The 80-columns code rule e.g. or our
| limited set of allowed indenting characters.

> I think this DocBook limitation needs to be fixed, because there are 
> legitimate cases where a function name got too long (for no fault of 
> its own, but for properties of the name-space it is operating in), 
> and we do not want a nanny state beat it into a single line.

| Agreed, just as in the other rules, one should be able to bend this
| one once in a while without technical consequences, i.e. without
| kerneldoc breaking.


Any of you, please feel free to send patches.  Thanks.

~Randy

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Johannes Weiner]

Hello Randy,

On Thu, Aug 13, 2009 at 05:06:28AM -0700, Randy Dunlap wrote:
> From: hannes@cmpxchg.org
> 
> On Thu, Aug 13, 2009 at 09:25:14AM +0200, Ingo Molnar wrote:
> > 
> > * James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > 
> > > On Wed, 2009-08-12 at 10:13 -0400, Alan Stern wrote:
> > > > On Wed, 12 Aug 2009, Ingo Molnar wrote:
> > > > 
> > > > > > And here I was thinking kerneldoc doesn't actually work 
> > > > > > like that, but perhaps Randy fixed it so the initial 
> > > > > > description can line-wrap?
> > > > 
> > > > Yes, that's what I thought too.  If kerneldoc has been fixed 
> > > > then the description line certainly should get wrapped.
> > > 
> > > I really don't think it needs to be fixed: it's a feature not a 
> > > bug.  It requires people writing kernel doc actually to think of 
> > > one line summaries.
> > 
> > As long as the argument is that it's good to have limitations just 
> > because it has good effects as well (which the gist of your argument 
> > seems to be), i disagree.
> > 
> > That's a very basic argument of freedom. Just consider the Gestapo 
> > which was also a 'feature' to keep criminals in check. Did you know 
> > that there were record low levels of petty criminality both in nazi 
> > Germany and during communism (and under just about any totalitarian 
> > regime)? Still nobody in their right mind is arguing that just due 
> > to that they are the right social model ...
> 
> | Although I really like how you Godwin'd kerneldoc comments ;-), we do
> | have other features that are features because of their limiting effect
> | all over the place, don't we?  The 80-columns code rule e.g. or our
> | limited set of allowed indenting characters.
> 
> > I think this DocBook limitation needs to be fixed, because there are 
> > legitimate cases where a function name got too long (for no fault of 
> > its own, but for properties of the name-space it is operating in), 
> > and we do not want a nanny state beat it into a single line.
> 
> | Agreed, just as in the other rules, one should be able to bend this
> | one once in a while without technical consequences, i.e. without
> | kerneldoc breaking.
> 
> 
> Any of you, please feel free to send patches.  Thanks.

Okay, I came up with a syntax to allow continued lines in short
descriptions and parameter descriptons.

I can successfully parse

---
/**
 *	get_tty_driver		-	find device of a tty
 *					...and everything
 *	@device: device identifier
 *		... to identify the device with
 *		... that is to be matched
 *	@index: returns the index of the tty
 *		... for your personal pleasure
 *
 *	This routine returns a tty driver structure, given a device number
 *	and also passes back the index number.
 *
 *	Locking: caller must hold tty_mutex
 */
---

to

---
Name:

get_tty_driver - find device of a tty and everything

Synopsis:

struct tty_driver * get_tty_driver (dev_t device,
                                    int * index);

Arguments:

device
        device identifier to identify the device with that is to be matched
index
        returns the index of the tty for your personal pleasure

Description:

This routine returns a tty driver structure, given a device number
and also passes back the index number.
Locking:

caller must hold tty_mutex
---

Unfortunately, perl requires me to ignore my pathetic rest of taste,
so it may well be horribly ugly without me noticing ;) Would the
following work for you?  I will happily incorporate improvements.

	Hannes

---

diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index b52d340..e427b0a 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -279,6 +279,7 @@ my $doc_special = "\@\%\$\&";
 my $doc_start = '^/\*\*\s*$'; # Allow whitespace at end of comment start.
 my $doc_end = '\*/';
 my $doc_com = '\s*\*\s*';
+my $doc_cont = $doc_com . '\.\.\.\s*(.+)';
 my $doc_decl = $doc_com . '(\w+)';
 my $doc_sect = $doc_com . '([' . $doc_special . ']?[\w\s]+):(.*)';
 my $doc_content = $doc_com . '(.*)';
@@ -1995,6 +1996,7 @@ sub process_file($) {
     my $identifier;
     my $func;
     my $descr;
+    my $item;
     my $initial_section_counter = $section_counter;
 
     if (defined($ENV{'SRCTREE'})) {
@@ -2044,7 +2046,9 @@ sub process_file($) {
 		    $descr =~ s/\s*$//;
 		    $descr =~ s/\s+/ /;
 		    $declaration_purpose = xml_escape($descr);
+		    $item = \$declaration_purpose;
 		} else {
+		    $state = 2;
 		    $declaration_purpose = "";
 		}
 
@@ -2075,6 +2079,15 @@ sub process_file($) {
 		++$warnings;
 		$state = 0;
 	    }
+	} elsif (/$doc_cont/o) {
+	    # continued description
+	    if (defined($item)) {
+		chomp($$item);
+		$$item .= " " . $1;
+	    } else {
+		print STDERR "Warning(${file}:$.): Unexpected continuation\n";
+		++$warnings;
+	    }
 	} elsif ($state == 2) {	# look for head: lines, and include content
 	    if (/$doc_sect/o) {
 		$newsection = $1;
@@ -2098,6 +2111,7 @@ sub process_file($) {
 		    }
 		    $contents .= "\n";
 		}
+		$item = \$contents;
 		$section = $newsection;
 	    } elsif (/$doc_end/) {
 
@@ -2114,6 +2128,7 @@ sub process_file($) {
 
 		$prototype = "";
 		$state = 3;
+		$item = undef;
 		$brcount = 0;
 #		print STDERR "end of doc comment, looking for prototype\n";
 	    } elsif (/$doc_content/) {
@@ -2127,10 +2142,12 @@ sub process_file($) {
 		} else {
 		    $contents .= $1 . "\n";
 		}
+		$item = undef;
 	    } else {
 		# i dont know - bad line?  ignore.
 		print STDERR "Warning(${file}:$.): bad line: $_";
 		++$warnings;
+		$item = undef;
 	    }
 	} elsif ($state == 3) {	# scanning for function '{' (end of prototype)
 	    if ($decl_type eq 'function') {

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[James Bottomley]

On Thu, 2009-08-13 at 16:51 +0200, Johannes Weiner wrote:
> Okay, I came up with a syntax to allow continued lines in short
> descriptions and parameter descriptons.
> 
> I can successfully parse
> 
> ---
> /**
>  *	get_tty_driver		-	find device of a tty
>  *					...and everything

I'm not so keen on the ... syntax ... suggestions below

>  *	@device: device identifier
>  *		... to identify the device with
>  *		... that is to be matched
>  *	@index: returns the index of the tty
>  *		... for your personal pleasure
>  *
>  *	This routine returns a tty driver structure, given a device number
>  *	and also passes back the index number.
>  *
>  *	Locking: caller must hold tty_mutex
>  */
> ---
> 
> to
> 
> ---
> Name:
> 
> get_tty_driver - find device of a tty and everything
> 
> Synopsis:
> 
> struct tty_driver * get_tty_driver (dev_t device,
>                                     int * index);
> 
> Arguments:
> 
> device
>         device identifier to identify the device with that is to be matched
> index
>         returns the index of the tty for your personal pleasure
> 
> Description:
> 
> This routine returns a tty driver structure, given a device number
> and also passes back the index number.
> Locking:
> 
> caller must hold tty_mutex
> ---
> 
> Unfortunately, perl requires me to ignore my pathetic rest of taste,
> so it may well be horribly ugly without me noticing ;) Would the
> following work for you?  I will happily incorporate improvements.
> 
> 	Hannes
> 
> ---
> 
> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
> index b52d340..e427b0a 100755
> --- a/scripts/kernel-doc
> +++ b/scripts/kernel-doc
> @@ -279,6 +279,7 @@ my $doc_special = "\@\%\$\&";
>  my $doc_start = '^/\*\*\s*$'; # Allow whitespace at end of comment start.
>  my $doc_end = '\*/';
>  my $doc_com = '\s*\*\s*';
> +my $doc_cont = $doc_com . '\.\.\.\s*(.+)';

how about making this

$doc_cont = $doc_com.'\s*([^@].*)';

That way anything that doesn't begin with a variable declaration would
be treated as comment continuation.  Might need a \s is the brackets to
ensure blank lines are OK and not treated as continuations.

James

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Randy Dunlap]

James Bottomley wrote:
> On Thu, 2009-08-13 at 16:51 +0200, Johannes Weiner wrote:
>> Okay, I came up with a syntax to allow continued lines in short
>> descriptions and parameter descriptons.
>>
>> I can successfully parse
>>
>> ---
>> /**
>>  *	get_tty_driver		-	find device of a tty
>>  *					...and everything
> 
> I'm not so keen on the ... syntax ... suggestions below

I like this even less than James does.

>>  *	@device: device identifier
>>  *		... to identify the device with
>>  *		... that is to be matched
>>  *	@index: returns the index of the tty
>>  *		... for your personal pleasure
>>  *
>>  *	This routine returns a tty driver structure, given a device number
>>  *	and also passes back the index number.
>>  *
>>  *	Locking: caller must hold tty_mutex
>>  */
>> ---
>>
>> to
>>
>> ---
>> Name:
>>
>> get_tty_driver - find device of a tty and everything
>>
>> Synopsis:
>>
>> struct tty_driver * get_tty_driver (dev_t device,
>>                                     int * index);
>>
>> Arguments:
>>
>> device
>>         device identifier to identify the device with that is to be matched
>> index
>>         returns the index of the tty for your personal pleasure
>>
>> Description:
>>
>> This routine returns a tty driver structure, given a device number
>> and also passes back the index number.
>> Locking:
>>
>> caller must hold tty_mutex
>> ---
>>
>> Unfortunately, perl requires me to ignore my pathetic rest of taste,
>> so it may well be horribly ugly without me noticing ;) Would the
>> following work for you?  I will happily incorporate improvements.
>>
>> 	Hannes
>>
>> ---
>>
>> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
>> index b52d340..e427b0a 100755
>> --- a/scripts/kernel-doc
>> +++ b/scripts/kernel-doc
>> @@ -279,6 +279,7 @@ my $doc_special = "\@\%\$\&";
>>  my $doc_start = '^/\*\*\s*$'; # Allow whitespace at end of comment start.
>>  my $doc_end = '\*/';
>>  my $doc_com = '\s*\*\s*';
>> +my $doc_cont = $doc_com . '\.\.\.\s*(.+)';
> 
> how about making this
> 
> $doc_cont = $doc_com.'\s*([^@].*)';
> 
> That way anything that doesn't begin with a variable declaration would
> be treated as comment continuation.  Might need a \s is the brackets to
> ensure blank lines are OK and not treated as continuations.

The goal should be to accept what is currently in the kernel source tree IMO,
and this suggestion looks like it would support that.

~Randy

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Johannes Weiner]

On Thu, Aug 13, 2009 at 09:20:54AM -0700, Randy Dunlap wrote:
> James Bottomley wrote:
> > On Thu, 2009-08-13 at 16:51 +0200, Johannes Weiner wrote:
> >> Okay, I came up with a syntax to allow continued lines in short
> >> descriptions and parameter descriptons.
> >>
> >> I can successfully parse
> >>
> >> ---
> >> /**
> >>  *	get_tty_driver		-	find device of a tty
> >>  *					...and everything
> > 
> > I'm not so keen on the ... syntax ... suggestions below
> 
> I like this even less than James does.

Fair enough.

> >> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
> >> index b52d340..e427b0a 100755
> >> --- a/scripts/kernel-doc
> >> +++ b/scripts/kernel-doc
> >> @@ -279,6 +279,7 @@ my $doc_special = "\@\%\$\&";
> >>  my $doc_start = '^/\*\*\s*$'; # Allow whitespace at end of comment start.
> >>  my $doc_end = '\*/';
> >>  my $doc_com = '\s*\*\s*';
> >> +my $doc_cont = $doc_com . '\.\.\.\s*(.+)';
> > 
> > how about making this
> > 
> > $doc_cont = $doc_com.'\s*([^@].*)';
> > 
> > That way anything that doesn't begin with a variable declaration would
> > be treated as comment continuation.  Might need a \s is the brackets to
> > ensure blank lines are OK and not treated as continuations.

The \s comes from $doc_com already.  We need the [^\s] as you said to
skip empty lines but also the slash for this one:

	* @foo: yada
	*/

> The goal should be to accept what is currently in the kernel source tree IMO,
> and this suggestion looks like it would support that.

Yeah.  I also noticed that multi-line blocks starting with @foo: are
already handled, making the indirect referencing unnecessary.  The
result is a bit simpler and more straight-forward, I think.

	Hannes

--
From: Johannes Weiner <hannes@cmpxchg.org>
Subject: kernel-doc: allow multi-line declaration purpose descriptions

Allow the short description after symbol name and dash in a kernel-doc
comment to span multiple lines, like this:

	/**
	 * unmap_mapping_range - unmap the portion of all mmaps in the
	 *	specified address_space corresponding to the specified
	 *	page range in the underlying file.
	 * @mapping: the address space containing mmaps to be unmapped.
	 * ...
	 */

The short description ends with a newline, a parameter description or
the end of the comment block.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---

diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index b52d340..2921aab 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -281,6 +281,7 @@ my $doc_end = '\*/';
 my $doc_com = '\s*\*\s*';
 my $doc_decl = $doc_com . '(\w+)';
 my $doc_sect = $doc_com . '([' . $doc_special . ']?[\w\s]+):(.*)';
+my $doc_cont = $doc_com . '([^@/\s].*)';
 my $doc_content = $doc_com . '(.*)';
 my $doc_block = $doc_com . 'DOC:\s*(.*)?';
 
@@ -1995,6 +1996,7 @@ sub process_file($) {
     my $identifier;
     my $func;
     my $descr;
+    my $in_purpose = 0;
     my $initial_section_counter = $section_counter;
 
     if (defined($ENV{'SRCTREE'})) {
@@ -2044,6 +2046,7 @@ sub process_file($) {
 		    $descr =~ s/\s*$//;
 		    $descr =~ s/\s+/ /;
 		    $declaration_purpose = xml_escape($descr);
+		    $in_purpose = 1;
 		} else {
 		    $declaration_purpose = "";
 		}
@@ -2075,7 +2078,12 @@ sub process_file($) {
 		++$warnings;
 		$state = 0;
 	    }
+	} elsif ($in_purpose == 1 && /$doc_cont/o) {
+	    # continued description
+	    chomp($declaration_purpose);
+	    $declaration_purpose .= " " . $1;
 	} elsif ($state == 2) {	# look for head: lines, and include content
+	    $in_purpose = 0;
 	    if (/$doc_sect/o) {
 		$newsection = $1;
 		$newcontents = $2;

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Randy Dunlap]

Johannes Weiner wrote:
> On Thu, Aug 13, 2009 at 09:20:54AM -0700, Randy Dunlap wrote:
>> James Bottomley wrote:
>>> On Thu, 2009-08-13 at 16:51 +0200, Johannes Weiner wrote:
>>>> Okay, I came up with a syntax to allow continued lines in short
>>>> descriptions and parameter descriptons.
>>>>
>>>> I can successfully parse
>>>>
>>>> ---
>>>> /**
>>>>  *	get_tty_driver		-	find device of a tty
>>>>  *					...and everything
>>> I'm not so keen on the ... syntax ... suggestions below
>> I like this even less than James does.
> 
> Fair enough.
> 
>>>> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
>>>> index b52d340..e427b0a 100755
>>>> --- a/scripts/kernel-doc
>>>> +++ b/scripts/kernel-doc
>>>> @@ -279,6 +279,7 @@ my $doc_special = "\@\%\$\&";
>>>>  my $doc_start = '^/\*\*\s*$'; # Allow whitespace at end of comment start.
>>>>  my $doc_end = '\*/';
>>>>  my $doc_com = '\s*\*\s*';
>>>> +my $doc_cont = $doc_com . '\.\.\.\s*(.+)';
>>> how about making this
>>>
>>> $doc_cont = $doc_com.'\s*([^@].*)';
>>>
>>> That way anything that doesn't begin with a variable declaration would
>>> be treated as comment continuation.  Might need a \s is the brackets to
>>> ensure blank lines are OK and not treated as continuations.
> 
> The \s comes from $doc_com already.  We need the [^\s] as you said to
> skip empty lines but also the slash for this one:
> 
> 	* @foo: yada
> 	*/
> 
>> The goal should be to accept what is currently in the kernel source tree IMO,
>> and this suggestion looks like it would support that.
> 
> Yeah.  I also noticed that multi-line blocks starting with @foo: are
> already handled, making the indirect referencing unnecessary.  The
> result is a bit simpler and more straight-forward, I think.
> 
> 	Hannes
> 
> --
> From: Johannes Weiner <hannes@cmpxchg.org>
> Subject: kernel-doc: allow multi-line declaration purpose descriptions
> 
> Allow the short description after symbol name and dash in a kernel-doc
> comment to span multiple lines, like this:
> 
> 	/**
> 	 * unmap_mapping_range - unmap the portion of all mmaps in the
> 	 *	specified address_space corresponding to the specified
> 	 *	page range in the underlying file.
> 	 * @mapping: the address space containing mmaps to be unmapped.
> 	 * ...
> 	 */
> 
> The short description ends with a newline, a parameter description or
> the end of the comment block.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
> 
> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
> index b52d340..2921aab 100755
> --- a/scripts/kernel-doc
> +++ b/scripts/kernel-doc
> @@ -281,6 +281,7 @@ my $doc_end = '\*/';
>  my $doc_com = '\s*\*\s*';
>  my $doc_decl = $doc_com . '(\w+)';
>  my $doc_sect = $doc_com . '([' . $doc_special . ']?[\w\s]+):(.*)';
> +my $doc_cont = $doc_com . '([^@/\s].*)';
>  my $doc_content = $doc_com . '(.*)';
>  my $doc_block = $doc_com . 'DOC:\s*(.*)?';
>  
> @@ -1995,6 +1996,7 @@ sub process_file($) {
>      my $identifier;
>      my $func;
>      my $descr;
> +    my $in_purpose = 0;
>      my $initial_section_counter = $section_counter;
>  
>      if (defined($ENV{'SRCTREE'})) {
> @@ -2044,6 +2046,7 @@ sub process_file($) {
>  		    $descr =~ s/\s*$//;
>  		    $descr =~ s/\s+/ /;
>  		    $declaration_purpose = xml_escape($descr);
> +		    $in_purpose = 1;
>  		} else {
>  		    $declaration_purpose = "";
>  		}
> @@ -2075,7 +2078,12 @@ sub process_file($) {
>  		++$warnings;
>  		$state = 0;
>  	    }
> +	} elsif ($in_purpose == 1 && /$doc_cont/o) {
> +	    # continued description
> +	    chomp($declaration_purpose);
> +	    $declaration_purpose .= " " . $1;
>  	} elsif ($state == 2) {	# look for head: lines, and include content
> +	    $in_purpose = 0;
>  	    if (/$doc_sect/o) {
>  		$newsection = $1;
>  		$newcontents = $2;

Hi Hannes,

This looks good in theory, but it doesn't survive a "make htmldocs"
after the patch is applied.

It could be the ending kernel-doc comment characters:
 **/

The problem is in drivers/scsi/scsi_devinfo.c:
/**
 * scsi_dev_info_list_delete - called from scsi.c:exit_scsi to remove the scsi_dev_info_list.
 **/
void scsi_exit_devinfo(void)
{


I'm currently on vacation, but I'll look into it more if you can't
do so.

Thanks.

[PATCH] scsi: fix func names in kernel-doc

[Randy Dunlap]

Randy Dunlap wrote:
> Johannes Weiner wrote:
>> On Thu, Aug 13, 2009 at 09:20:54AM -0700, Randy Dunlap wrote:
>>> James Bottomley wrote:
>>>> On Thu, 2009-08-13 at 16:51 +0200, Johannes Weiner wrote:
>>>>> Okay, I came up with a syntax to allow continued lines in short
>>>>> descriptions and parameter descriptons.
>>>>>
>>>>> I can successfully parse
>>>>>
>>>>> ---
>>>>> /**
>>>>>  *	get_tty_driver		-	find device of a tty
>>>>>  *					...and everything
>>>> I'm not so keen on the ... syntax ... suggestions below
>>> I like this even less than James does.
>> Fair enough.
>>
>>>>> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
>>>>> index b52d340..e427b0a 100755
>>>>> --- a/scripts/kernel-doc
>>>>> +++ b/scripts/kernel-doc
>>>>> @@ -279,6 +279,7 @@ my $doc_special = "\@\%\$\&";
>>>>>  my $doc_start = '^/\*\*\s*$'; # Allow whitespace at end of comment start.
>>>>>  my $doc_end = '\*/';
>>>>>  my $doc_com = '\s*\*\s*';
>>>>> +my $doc_cont = $doc_com . '\.\.\.\s*(.+)';
>>>> how about making this
>>>>
>>>> $doc_cont = $doc_com.'\s*([^@].*)';
>>>>
>>>> That way anything that doesn't begin with a variable declaration would
>>>> be treated as comment continuation.  Might need a \s is the brackets to
>>>> ensure blank lines are OK and not treated as continuations.
>> The \s comes from $doc_com already.  We need the [^\s] as you said to
>> skip empty lines but also the slash for this one:
>>
>> 	* @foo: yada
>> 	*/
>>
>>> The goal should be to accept what is currently in the kernel source tree IMO,
>>> and this suggestion looks like it would support that.
>> Yeah.  I also noticed that multi-line blocks starting with @foo: are
>> already handled, making the indirect referencing unnecessary.  The
>> result is a bit simpler and more straight-forward, I think.
>>
>> 	Hannes
>>
>> --
>> From: Johannes Weiner <hannes@cmpxchg.org>
>> Subject: kernel-doc: allow multi-line declaration purpose descriptions
>>
>> Allow the short description after symbol name and dash in a kernel-doc
>> comment to span multiple lines, like this:
>>
>> 	/**
>> 	 * unmap_mapping_range - unmap the portion of all mmaps in the
>> 	 *	specified address_space corresponding to the specified
>> 	 *	page range in the underlying file.
>> 	 * @mapping: the address space containing mmaps to be unmapped.
>> 	 * ...
>> 	 */
>>
>> The short description ends with a newline, a parameter description or
>> the end of the comment block.
>>
>> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
>> ---
>>
>> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
>> index b52d340..2921aab 100755
>> --- a/scripts/kernel-doc
>> +++ b/scripts/kernel-doc
>> @@ -281,6 +281,7 @@ my $doc_end = '\*/';
>>  my $doc_com = '\s*\*\s*';
>>  my $doc_decl = $doc_com . '(\w+)';
>>  my $doc_sect = $doc_com . '([' . $doc_special . ']?[\w\s]+):(.*)';
>> +my $doc_cont = $doc_com . '([^@/\s].*)';
>>  my $doc_content = $doc_com . '(.*)';
>>  my $doc_block = $doc_com . 'DOC:\s*(.*)?';
>>  
>> @@ -1995,6 +1996,7 @@ sub process_file($) {
>>      my $identifier;
>>      my $func;
>>      my $descr;
>> +    my $in_purpose = 0;
>>      my $initial_section_counter = $section_counter;
>>  
>>      if (defined($ENV{'SRCTREE'})) {
>> @@ -2044,6 +2046,7 @@ sub process_file($) {
>>  		    $descr =~ s/\s*$//;
>>  		    $descr =~ s/\s+/ /;
>>  		    $declaration_purpose = xml_escape($descr);
>> +		    $in_purpose = 1;
>>  		} else {
>>  		    $declaration_purpose = "";
>>  		}
>> @@ -2075,7 +2078,12 @@ sub process_file($) {
>>  		++$warnings;
>>  		$state = 0;
>>  	    }
>> +	} elsif ($in_purpose == 1 && /$doc_cont/o) {
>> +	    # continued description
>> +	    chomp($declaration_purpose);
>> +	    $declaration_purpose .= " " . $1;
>>  	} elsif ($state == 2) {	# look for head: lines, and include content
>> +	    $in_purpose = 0;
>>  	    if (/$doc_sect/o) {
>>  		$newsection = $1;
>>  		$newcontents = $2;
> 
> Hi Hannes,
> 
> This looks good in theory, but it doesn't survive a "make htmldocs"
> after the patch is applied.
> 
> It could be the ending kernel-doc comment characters:
>  **/
> 
> The problem is in drivers/scsi/scsi_devinfo.c:
> /**
>  * scsi_dev_info_list_delete - called from scsi.c:exit_scsi to remove the scsi_dev_info_list.
>  **/
> void scsi_exit_devinfo(void)
> {
> 
> 
> I'm currently on vacation, but I'll look into it more if you can't
> do so.

Duh.  James, the kernel-doc func name and actual func name don't match.
Here's a patch for that and one other mismatched func name.


-----
From: Randy Dunlap <randy.dunlap@oracle.com>

Fix scsi_devinfo.c kernel-doc function names to match
actual function names.

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---
 drivers/scsi/scsi_devinfo.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- lnx-2631-rc6.orig/drivers/scsi/scsi_devinfo.c
+++ lnx-2631-rc6/drivers/scsi/scsi_devinfo.c
@@ -454,7 +454,7 @@ int scsi_get_device_flags(struct scsi_de
 
 
 /**
- * get_device_flags_keyed - get device specific flags from the dynamic device list.
+ * scsi_get_device_flags_keyed - get device specific flags from the dynamic device list
  * @sdev:       &scsi_device to get flags for
  * @vendor:	vendor name
  * @model:	model name
@@ -685,7 +685,7 @@ MODULE_PARM_DESC(default_dev_flags,
 		 "scsi default device flag integer value");
 
 /**
- * scsi_dev_info_list_delete - called from scsi.c:exit_scsi to remove the scsi_dev_info_list.
+ * scsi_exit_devinfo - remove /proc/scsi/device_info & the scsi_dev_info_list
  **/
 void scsi_exit_devinfo(void)
 {

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Johannes Weiner]

Hello Randy,

On Fri, Aug 14, 2009 at 11:23:55AM -0700, Randy Dunlap wrote:
> Johannes Weiner wrote:
> > diff --git a/scripts/kernel-doc b/scripts/kernel-doc
> > index b52d340..2921aab 100755
> > --- a/scripts/kernel-doc
> > +++ b/scripts/kernel-doc
> > @@ -281,6 +281,7 @@ my $doc_end = '\*/';
> >  my $doc_com = '\s*\*\s*';
> >  my $doc_decl = $doc_com . '(\w+)';
> >  my $doc_sect = $doc_com . '([' . $doc_special . ']?[\w\s]+):(.*)';
> > +my $doc_cont = $doc_com . '([^@/\s].*)';
> >  my $doc_content = $doc_com . '(.*)';
> >  my $doc_block = $doc_com . 'DOC:\s*(.*)?';
> >  
> > @@ -1995,6 +1996,7 @@ sub process_file($) {
> >      my $identifier;
> >      my $func;
> >      my $descr;
> > +    my $in_purpose = 0;
> >      my $initial_section_counter = $section_counter;
> >  
> >      if (defined($ENV{'SRCTREE'})) {
> > @@ -2044,6 +2046,7 @@ sub process_file($) {
> >  		    $descr =~ s/\s*$//;
> >  		    $descr =~ s/\s+/ /;
> >  		    $declaration_purpose = xml_escape($descr);
> > +		    $in_purpose = 1;
> >  		} else {
> >  		    $declaration_purpose = "";
> >  		}
> > @@ -2075,7 +2078,12 @@ sub process_file($) {
> >  		++$warnings;
> >  		$state = 0;
> >  	    }
> > +	} elsif ($in_purpose == 1 && /$doc_cont/o) {
> > +	    # continued description
> > +	    chomp($declaration_purpose);
> > +	    $declaration_purpose .= " " . $1;
> >  	} elsif ($state == 2) {	# look for head: lines, and include content
> > +	    $in_purpose = 0;
> >  	    if (/$doc_sect/o) {
> >  		$newsection = $1;
> >  		$newcontents = $2;
> 
> Hi Hannes,
> 
> This looks good in theory, but it doesn't survive a "make htmldocs"
> after the patch is applied.
> 
> It could be the ending kernel-doc comment characters:
>  **/
> 
> The problem is in drivers/scsi/scsi_devinfo.c:
> /**
>  * scsi_dev_info_list_delete - called from scsi.c:exit_scsi to remove the scsi_dev_info_list.
>  **/
> void scsi_exit_devinfo(void)
> {
> 
> 
> I'm currently on vacation, but I'll look into it more if you can't
> do so.

Yeah, it's the terminating **/ which matches $doc_cont.  I will try to
send an updated version this evening.

	Hannes

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Johannes Weiner]

Hello,

On Tue, Aug 18, 2009 at 11:04:19AM +0200, Johannes Weiner wrote:
> Hello Randy,
> 
> On Fri, Aug 14, 2009 at 11:23:55AM -0700, Randy Dunlap wrote:
> > Johannes Weiner wrote:
> > > diff --git a/scripts/kernel-doc b/scripts/kernel-doc
> > > index b52d340..2921aab 100755
> > > --- a/scripts/kernel-doc
> > > +++ b/scripts/kernel-doc
> > > @@ -281,6 +281,7 @@ my $doc_end = '\*/';
> > >  my $doc_com = '\s*\*\s*';
> > >  my $doc_decl = $doc_com . '(\w+)';
> > >  my $doc_sect = $doc_com . '([' . $doc_special . ']?[\w\s]+):(.*)';
> > > +my $doc_cont = $doc_com . '([^@/\s].*)';
> > >  my $doc_content = $doc_com . '(.*)';
> > >  my $doc_block = $doc_com . 'DOC:\s*(.*)?';
> > >  
> > > @@ -1995,6 +1996,7 @@ sub process_file($) {
> > >      my $identifier;
> > >      my $func;
> > >      my $descr;
> > > +    my $in_purpose = 0;
> > >      my $initial_section_counter = $section_counter;
> > >  
> > >      if (defined($ENV{'SRCTREE'})) {
> > > @@ -2044,6 +2046,7 @@ sub process_file($) {
> > >  		    $descr =~ s/\s*$//;
> > >  		    $descr =~ s/\s+/ /;
> > >  		    $declaration_purpose = xml_escape($descr);
> > > +		    $in_purpose = 1;
> > >  		} else {
> > >  		    $declaration_purpose = "";
> > >  		}
> > > @@ -2075,7 +2078,12 @@ sub process_file($) {
> > >  		++$warnings;
> > >  		$state = 0;
> > >  	    }
> > > +	} elsif ($in_purpose == 1 && /$doc_cont/o) {
> > > +	    # continued description
> > > +	    chomp($declaration_purpose);
> > > +	    $declaration_purpose .= " " . $1;
> > >  	} elsif ($state == 2) {	# look for head: lines, and include content
> > > +	    $in_purpose = 0;
> > >  	    if (/$doc_sect/o) {
> > >  		$newsection = $1;
> > >  		$newcontents = $2;
> > 
> > Hi Hannes,
> > 
> > This looks good in theory, but it doesn't survive a "make htmldocs"
> > after the patch is applied.
> > 
> > It could be the ending kernel-doc comment characters:
> >  **/
> > 
> > The problem is in drivers/scsi/scsi_devinfo.c:
> > /**
> >  * scsi_dev_info_list_delete - called from scsi.c:exit_scsi to remove the scsi_dev_info_list.
> >  **/
> > void scsi_exit_devinfo(void)
> > {
> > 
> > 
> > I'm currently on vacation, but I'll look into it more if you can't
> > do so.
> 
> Yeah, it's the terminating **/ which matches $doc_cont.  I will try to
> send an updated version this evening.

I got completely rid of the extra re.  Just parse a non-empty content
line following the declaration purpose immediately as continuation.

It survives make htmldocs, the scsi_exit_devinfo() doc looks okay and
for stuff that had continuation lines before, it does what's expected
- e.g. for the doc of kernel/sched.c::init_sd_power_savings_stats().

It behaves differently for broken docs
(mm/page_alloc.c::calculate_zone_inactive_ratio e.g.), but that
shouldn't matter.

I didn't find any other misbehaviour when checking random samples.

---
From: Johannes Weiner <hannes@cmpxchg.org>
Subject: kernel-doc: allow multi-line declaration purpose descriptions

Allow the short description after symbol name and dash in a kernel-doc
comment to span multiple lines, e.g. like this:

	/**
	 * unmap_mapping_range - unmap the portion of all mmaps in the
	 *	specified address_space corresponding to the specified
	 *	page range in the underlying file.
	 * @mapping: the address space containing mmaps to be unmapped.
	 * ...
	 */

The short description ends with a parameter description, an empty line
or the end of the comment block.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 Documentation/kernel-doc-nano-HOWTO.txt |    4 +++-
 scripts/kernel-doc                      |   19 ++++++++++++++-----
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/Documentation/kernel-doc-nano-HOWTO.txt b/Documentation/kernel-doc-nano-HOWTO.txt
index 4d04572..348b9e5 100644
--- a/Documentation/kernel-doc-nano-HOWTO.txt
+++ b/Documentation/kernel-doc-nano-HOWTO.txt
@@ -66,7 +66,9 @@ Example kernel-doc function comment:
  * The longer description can have multiple paragraphs.
  */
 
-The first line, with the short description, must be on a single line.
+The short description following the subject can span multiple lines
+and ends with an @argument description, an empty line or the end of
+the comment block.
 
 The @argument descriptions must begin on the very next line following
 this opening short function description line, with no intervening
diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index b52d340..6194ef5 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -1995,6 +1995,7 @@ sub process_file($) {
     my $identifier;
     my $func;
     my $descr;
+    my $in_purpose = 0;
     my $initial_section_counter = $section_counter;
 
     if (defined($ENV{'SRCTREE'})) {
@@ -2044,6 +2045,7 @@ sub process_file($) {
 		    $descr =~ s/\s*$//;
 		    $descr =~ s/\s+/ /;
 		    $declaration_purpose = xml_escape($descr);
+		    $in_purpose = 1;
 		} else {
 		    $declaration_purpose = "";
 		}
@@ -2090,6 +2092,7 @@ sub process_file($) {
 		}
 
 		$in_doc_sect = 1;
+		$in_purpose = 0;
 		$contents = $newcontents;
 		if ($contents ne "") {
 		    while ((substr($contents, 0, 1) eq " ") ||
@@ -2119,11 +2122,17 @@ sub process_file($) {
 	    } elsif (/$doc_content/) {
 		# miguel-style comment kludge, look for blank lines after
 		# @parameter line to signify start of description
-		if ($1 eq "" &&
-			($section =~ m/^@/ || $section eq $section_context)) {
-		    dump_section($file, $section, xml_escape($contents));
-		    $section = $section_default;
-		    $contents = "";
+		if ($1 eq "") {
+		    if ($section =~ m/^@/ || $section eq $section_context) {
+			dump_section($file, $section, xml_escape($contents));
+			$section = $section_default;
+			$contents = "";
+		    }
+		    $in_purpose = 0;
+		} elsif ($in_purpose == 1) {
+		    # Continued declaration purpose
+		    chomp($declaration_purpose);
+		    $declaration_purpose .= " " . $1;
 		} else {
 		    $contents .= $1 . "\n";
 		}

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Randy Dunlap]

Johannes Weiner wrote:
>> Yeah, it's the terminating **/ which matches $doc_cont.  I will try to
>> send an updated version this evening.
> 
> I got completely rid of the extra re.  Just parse a non-empty content
> line following the declaration purpose immediately as continuation.
> 
> It survives make htmldocs, the scsi_exit_devinfo() doc looks okay and
> for stuff that had continuation lines before, it does what's expected
> - e.g. for the doc of kernel/sched.c::init_sd_power_savings_stats().
> 
> It behaves differently for broken docs
> (mm/page_alloc.c::calculate_zone_inactive_ratio e.g.), but that
> shouldn't matter.

Right, no problem on that one (for which I sent Andrew a patch some
time ago).

> I didn't find any other misbehaviour when checking random samples.

It's very close.  I only checked/compared one docbook: mac80211.
There is some kind of paragraph end handling difference.

In processing include/net/mac80211.h, struct ieee80211_tx_info,
without the patch, it ends with:

This structure is placed in skb->cb for three uses: (1) mac80211 TX control - mac80211 tells the driver what to do (2) driver internal use (if applicable) (3) TX status information - driver tells mac80211 what happened

The TX control's sta pointer is only valid during the ->tx call, it may be NULL.

and with the patch, those 2 paragraphs are run together:

This structure is placed in skb->cb for three uses: (1) mac80211 TX control - mac80211 tells the driver what to do (2) driver internal use (if applicable) (3) TX status information - driver tells mac80211 what happened The TX control's sta pointer is only valid during the ->tx call, it may be NULL.



I don't think that this will be difficult to find/fix...

Thanks.

> ---
> From: Johannes Weiner <hannes@cmpxchg.org>
> Subject: kernel-doc: allow multi-line declaration purpose descriptions
> 
> Allow the short description after symbol name and dash in a kernel-doc
> comment to span multiple lines, e.g. like this:
> 
> 	/**
> 	 * unmap_mapping_range - unmap the portion of all mmaps in the
> 	 *	specified address_space corresponding to the specified
> 	 *	page range in the underlying file.
> 	 * @mapping: the address space containing mmaps to be unmapped.
> 	 * ...
> 	 */
> 
> The short description ends with a parameter description, an empty line
> or the end of the comment block.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  Documentation/kernel-doc-nano-HOWTO.txt |    4 +++-
>  scripts/kernel-doc                      |   19 ++++++++++++++-----
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/kernel-doc-nano-HOWTO.txt b/Documentation/kernel-doc-nano-HOWTO.txt
> index 4d04572..348b9e5 100644
> --- a/Documentation/kernel-doc-nano-HOWTO.txt
> +++ b/Documentation/kernel-doc-nano-HOWTO.txt
> @@ -66,7 +66,9 @@ Example kernel-doc function comment:
>   * The longer description can have multiple paragraphs.
>   */
>  
> -The first line, with the short description, must be on a single line.
> +The short description following the subject can span multiple lines
> +and ends with an @argument description, an empty line or the end of
> +the comment block.
>  
>  The @argument descriptions must begin on the very next line following
>  this opening short function description line, with no intervening
> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
> index b52d340..6194ef5 100755
> --- a/scripts/kernel-doc
> +++ b/scripts/kernel-doc
> @@ -1995,6 +1995,7 @@ sub process_file($) {
>      my $identifier;
>      my $func;
>      my $descr;
> +    my $in_purpose = 0;
>      my $initial_section_counter = $section_counter;
>  
>      if (defined($ENV{'SRCTREE'})) {
> @@ -2044,6 +2045,7 @@ sub process_file($) {
>  		    $descr =~ s/\s*$//;
>  		    $descr =~ s/\s+/ /;
>  		    $declaration_purpose = xml_escape($descr);
> +		    $in_purpose = 1;
>  		} else {
>  		    $declaration_purpose = "";
>  		}
> @@ -2090,6 +2092,7 @@ sub process_file($) {
>  		}
>  
>  		$in_doc_sect = 1;
> +		$in_purpose = 0;
>  		$contents = $newcontents;
>  		if ($contents ne "") {
>  		    while ((substr($contents, 0, 1) eq " ") ||
> @@ -2119,11 +2122,17 @@ sub process_file($) {
>  	    } elsif (/$doc_content/) {
>  		# miguel-style comment kludge, look for blank lines after
>  		# @parameter line to signify start of description
> -		if ($1 eq "" &&
> -			($section =~ m/^@/ || $section eq $section_context)) {
> -		    dump_section($file, $section, xml_escape($contents));
> -		    $section = $section_default;
> -		    $contents = "";
> +		if ($1 eq "") {
> +		    if ($section =~ m/^@/ || $section eq $section_context) {
> +			dump_section($file, $section, xml_escape($contents));
> +			$section = $section_default;
> +			$contents = "";
> +		    }
> +		    $in_purpose = 0;
> +		} elsif ($in_purpose == 1) {
> +		    # Continued declaration purpose
> +		    chomp($declaration_purpose);
> +		    $declaration_purpose .= " " . $1;
>  		} else {
>  		    $contents .= $1 . "\n";
>  		}

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Randy Dunlap]

Randy Dunlap wrote:
> Johannes Weiner wrote:
>>> Yeah, it's the terminating **/ which matches $doc_cont.  I will try to
>>> send an updated version this evening.
>> I got completely rid of the extra re.  Just parse a non-empty content
>> line following the declaration purpose immediately as continuation.
>>
>> It survives make htmldocs, the scsi_exit_devinfo() doc looks okay and
>> for stuff that had continuation lines before, it does what's expected
>> - e.g. for the doc of kernel/sched.c::init_sd_power_savings_stats().
>>
>> It behaves differently for broken docs
>> (mm/page_alloc.c::calculate_zone_inactive_ratio e.g.), but that
>> shouldn't matter.
> 
> Right, no problem on that one (for which I sent Andrew a patch some
> time ago).
> 
>> I didn't find any other misbehaviour when checking random samples.
> 
> It's very close.  I only checked/compared one docbook: mac80211.
> There is some kind of paragraph end handling difference.
> 
> In processing include/net/mac80211.h, struct ieee80211_tx_info,
> without the patch, it ends with:
> 
> This structure is placed in skb->cb for three uses: (1) mac80211 TX control - mac80211 tells the driver what to do (2) driver internal use (if applicable) (3) TX status information - driver tells mac80211 what happened
> 
> The TX control's sta pointer is only valid during the ->tx call, it may be NULL.
> 
> and with the patch, those 2 paragraphs are run together:
> 
> This structure is placed in skb->cb for three uses: (1) mac80211 TX control - mac80211 tells the driver what to do (2) driver internal use (if applicable) (3) TX status information - driver tells mac80211 what happened The TX control's sta pointer is only valid during the ->tx call, it may be NULL.
> 
> 
> 
> I don't think that this will be difficult to find/fix...
> 
> Thanks.

Bah.  There are  lots of kernel-doc problems in mac80211.h.
I will be sending  a patch for those...

>> ---
>> From: Johannes Weiner <hannes@cmpxchg.org>
>> Subject: kernel-doc: allow multi-line declaration purpose descriptions
>>
>> Allow the short description after symbol name and dash in a kernel-doc
>> comment to span multiple lines, e.g. like this:
>>
>> 	/**
>> 	 * unmap_mapping_range - unmap the portion of all mmaps in the
>> 	 *	specified address_space corresponding to the specified
>> 	 *	page range in the underlying file.
>> 	 * @mapping: the address space containing mmaps to be unmapped.
>> 	 * ...
>> 	 */
>>
>> The short description ends with a parameter description, an empty line
>> or the end of the comment block.
>>
>> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
>> ---
>>  Documentation/kernel-doc-nano-HOWTO.txt |    4 +++-
>>  scripts/kernel-doc                      |   19 ++++++++++++++-----
>>  2 files changed, 17 insertions(+), 6 deletions(-)

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Johannes Weiner]

On Wed, Aug 19, 2009 at 04:21:23PM -0700, Randy Dunlap wrote:
> Johannes Weiner wrote:
> >> Yeah, it's the terminating **/ which matches $doc_cont.  I will try to
> >> send an updated version this evening.
> > 
> > I got completely rid of the extra re.  Just parse a non-empty content
> > line following the declaration purpose immediately as continuation.
> > 
> > It survives make htmldocs, the scsi_exit_devinfo() doc looks okay and
> > for stuff that had continuation lines before, it does what's expected
> > - e.g. for the doc of kernel/sched.c::init_sd_power_savings_stats().
> > 
> > It behaves differently for broken docs
> > (mm/page_alloc.c::calculate_zone_inactive_ratio e.g.), but that
> > shouldn't matter.
> 
> Right, no problem on that one (for which I sent Andrew a patch some
> time ago).
> 
> > I didn't find any other misbehaviour when checking random samples.
> 
> It's very close.  I only checked/compared one docbook: mac80211.
> There is some kind of paragraph end handling difference.
> 
> In processing include/net/mac80211.h, struct ieee80211_tx_info,
> without the patch, it ends with:
> 
> This structure is placed in skb->cb for three uses: (1) mac80211 TX control - mac80211 tells the driver what to do (2) driver internal use (if applicable) (3) TX status information - driver tells mac80211 what happened
> 
> The TX control's sta pointer is only valid during the ->tx call, it may be NULL.
> 
> and with the patch, those 2 paragraphs are run together:
> 
> This structure is placed in skb->cb for three uses: (1) mac80211 TX control - mac80211 tells the driver what to do (2) driver internal use (if applicable) (3) TX status information - driver tells mac80211 what happened The TX control's sta pointer is only valid during the ->tx call, it may be NULL.

Yeah, I forgot to actually collect empty lines in the documentation
bodies when changing that conditional.  Stupid.  I fixed it in the
attached version.

---
From: Johannes Weiner <hannes@cmpxchg.org>
Subject: kernel-doc: allow multi-line declaration purpose descriptions

Allow the short description after symbol name and dash in a kernel-doc
comment to span multiple lines, e.g. like this:

	/**
	 * unmap_mapping_range - unmap the portion of all mmaps in the
	 *	specified address_space corresponding to the specified
	 *	page range in the underlying file.
	 * @mapping: the address space containing mmaps to be unmapped.
	 * ...
	 */

The short description ends with a parameter description, an empty line
or the end of the comment block.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---

diff --git a/Documentation/kernel-doc-nano-HOWTO.txt b/Documentation/kernel-doc-nano-HOWTO.txt
index 4d04572..348b9e5 100644
--- a/Documentation/kernel-doc-nano-HOWTO.txt
+++ b/Documentation/kernel-doc-nano-HOWTO.txt
@@ -66,7 +66,9 @@ Example kernel-doc function comment:
  * The longer description can have multiple paragraphs.
  */
 
-The first line, with the short description, must be on a single line.
+The short description following the subject can span multiple lines
+and ends with an @argument description, an empty line or the end of
+the comment block.
 
 The @argument descriptions must begin on the very next line following
 this opening short function description line, with no intervening
diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index b52d340..d8b3641 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -1995,6 +1995,7 @@ sub process_file($) {
     my $identifier;
     my $func;
     my $descr;
+    my $in_purpose = 0;
     my $initial_section_counter = $section_counter;
 
     if (defined($ENV{'SRCTREE'})) {
@@ -2044,6 +2045,7 @@ sub process_file($) {
 		    $descr =~ s/\s*$//;
 		    $descr =~ s/\s+/ /;
 		    $declaration_purpose = xml_escape($descr);
+		    $in_purpose = 1;
 		} else {
 		    $declaration_purpose = "";
 		}
@@ -2090,6 +2092,7 @@ sub process_file($) {
 		}
 
 		$in_doc_sect = 1;
+		$in_purpose = 0;
 		$contents = $newcontents;
 		if ($contents ne "") {
 		    while ((substr($contents, 0, 1) eq " ") ||
@@ -2119,11 +2122,19 @@ sub process_file($) {
 	    } elsif (/$doc_content/) {
 		# miguel-style comment kludge, look for blank lines after
 		# @parameter line to signify start of description
-		if ($1 eq "" &&
-			($section =~ m/^@/ || $section eq $section_context)) {
-		    dump_section($file, $section, xml_escape($contents));
-		    $section = $section_default;
-		    $contents = "";
+		if ($1 eq "") {
+		    if ($section =~ m/^@/ || $section eq $section_context) {
+			dump_section($file, $section, xml_escape($contents));
+			$section = $section_default;
+			$contents = "";
+		    } else {
+			$contents .= "\n";
+		    }
+		    $in_purpose = 0;
+		} elsif ($in_purpose == 1) {
+		    # Continued declaration purpose
+		    chomp($declaration_purpose);
+		    $declaration_purpose .= " " . $1;
 		} else {
 		    $contents .= $1 . "\n";
 		}

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Randy Dunlap]

On Mon, 24 Aug 2009 12:06:54 -0700 (PDT) Johannes Weiner wrote:

> On Wed, Aug 19, 2009 at 04:21:23PM -0700, Randy Dunlap wrote:
> > Johannes Weiner wrote:
> > >> Yeah, it's the terminating **/ which matches $doc_cont.  I will try to
> > >> send an updated version this evening.
> > > 
> > > I got completely rid of the extra re.  Just parse a non-empty content
> > > line following the declaration purpose immediately as continuation.
> > > 
> > > It survives make htmldocs, the scsi_exit_devinfo() doc looks okay and
> > > for stuff that had continuation lines before, it does what's expected
> > > - e.g. for the doc of kernel/sched.c::init_sd_power_savings_stats().
> > > 
> > > It behaves differently for broken docs
> > > (mm/page_alloc.c::calculate_zone_inactive_ratio e.g.), but that
> > > shouldn't matter.
> > 
> > Right, no problem on that one (for which I sent Andrew a patch some
> > time ago).
> > 
> > > I didn't find any other misbehaviour when checking random samples.
> > 
> > It's very close.  I only checked/compared one docbook: mac80211.
> > There is some kind of paragraph end handling difference.
> > 
> > In processing include/net/mac80211.h, struct ieee80211_tx_info,
> > without the patch, it ends with:
> > 
> > This structure is placed in skb->cb for three uses: (1) mac80211 TX control - mac80211 tells the driver what to do (2) driver internal use (if applicable) (3) TX status information - driver tells mac80211 what happened
> > 
> > The TX control's sta pointer is only valid during the ->tx call, it may be NULL.
> > 
> > and with the patch, those 2 paragraphs are run together:
> > 
> > This structure is placed in skb->cb for three uses: (1) mac80211 TX control - mac80211 tells the driver what to do (2) driver internal use (if applicable) (3) TX status information - driver tells mac80211 what happened The TX control's sta pointer is only valid during the ->tx call, it may be NULL.
> 
> Yeah, I forgot to actually collect empty lines in the documentation
> bodies when changing that conditional.  Stupid.  I fixed it in the
> attached version.

Thanks.  I had spent some time on it but I hadn't quite found the magical
incantation to preserve the exact same output as without the patch, but your
patch now does that.

I'll add this to my kernel-doc quilt patch series.

Oh, one question below...

> ---
> From: Johannes Weiner <hannes@cmpxchg.org>
> Subject: kernel-doc: allow multi-line declaration purpose descriptions
> 
> Allow the short description after symbol name and dash in a kernel-doc
> comment to span multiple lines, e.g. like this:
> 
> 	/**
> 	 * unmap_mapping_range - unmap the portion of all mmaps in the
> 	 *	specified address_space corresponding to the specified
> 	 *	page range in the underlying file.
> 	 * @mapping: the address space containing mmaps to be unmapped.
> 	 * ...
> 	 */
> 
> The short description ends with a parameter description, an empty line
> or the end of the comment block.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
> 
> diff --git a/Documentation/kernel-doc-nano-HOWTO.txt b/Documentation/kernel-doc-nano-HOWTO.txt
> index 4d04572..348b9e5 100644
> --- a/Documentation/kernel-doc-nano-HOWTO.txt
> +++ b/Documentation/kernel-doc-nano-HOWTO.txt
> @@ -66,7 +66,9 @@ Example kernel-doc function comment:
>   * The longer description can have multiple paragraphs.
>   */
>  
> -The first line, with the short description, must be on a single line.
> +The short description following the subject can span multiple lines
> +and ends with an @argument description, an empty line or the end of
> +the comment block.
>  
>  The @argument descriptions must begin on the very next line following
>  this opening short function description line, with no intervening
> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
> index b52d340..d8b3641 100755
> --- a/scripts/kernel-doc
> +++ b/scripts/kernel-doc
> @@ -1995,6 +1995,7 @@ sub process_file($) {
>      my $identifier;
>      my $func;
>      my $descr;
> +    my $in_purpose = 0;
>      my $initial_section_counter = $section_counter;
>  
>      if (defined($ENV{'SRCTREE'})) {
> @@ -2044,6 +2045,7 @@ sub process_file($) {
>  		    $descr =~ s/\s*$//;
>  		    $descr =~ s/\s+/ /;
>  		    $declaration_purpose = xml_escape($descr);
> +		    $in_purpose = 1;
>  		} else {
>  		    $declaration_purpose = "";
>  		}
> @@ -2090,6 +2092,7 @@ sub process_file($) {
>  		}
>  
>  		$in_doc_sect = 1;
> +		$in_purpose = 0;
>  		$contents = $newcontents;
>  		if ($contents ne "") {
>  		    while ((substr($contents, 0, 1) eq " ") ||
> @@ -2119,11 +2122,19 @@ sub process_file($) {
>  	    } elsif (/$doc_content/) {
>  		# miguel-style comment kludge, look for blank lines after
>  		# @parameter line to signify start of description
> -		if ($1 eq "" &&
> -			($section =~ m/^@/ || $section eq $section_context)) {
> -		    dump_section($file, $section, xml_escape($contents));
> -		    $section = $section_default;
> -		    $contents = "";
> +		if ($1 eq "") {
> +		    if ($section =~ m/^@/ || $section eq $section_context) {
> +			dump_section($file, $section, xml_escape($contents));
> +			$section = $section_default;
> +			$contents = "";
> +		    } else {
> +			$contents .= "\n";
> +		    }
> +		    $in_purpose = 0;
> +		} elsif ($in_purpose == 1) {
> +		    # Continued declaration purpose
> +		    chomp($declaration_purpose);
> +		    $declaration_purpose .= " " . $1;

Why shouldn't this be:
		    $declaration_purpose .= " " . xml_escape($1);
?


>  		} else {
>  		    $contents .= $1 . "\n";
>  		}


---
~Randy
LPC 2009, Sept. 23-25, Portland, Oregon
http://linuxplumbersconf.org/2009/

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Johannes Weiner]

Hello Randy,

On Mon, Aug 24, 2009 at 12:27:11PM -0700, Randy Dunlap wrote:
> On Mon, 24 Aug 2009 12:06:54 -0700 (PDT) Johannes Weiner wrote:
> 
> I'll add this to my kernel-doc quilt patch series.

Thanks!

> Oh, one question below...

> > @@ -2119,11 +2122,19 @@ sub process_file($) {
> >  	    } elsif (/$doc_content/) {
> >  		# miguel-style comment kludge, look for blank lines after
> >  		# @parameter line to signify start of description
> > -		if ($1 eq "" &&
> > -			($section =~ m/^@/ || $section eq $section_context)) {
> > -		    dump_section($file, $section, xml_escape($contents));
> > -		    $section = $section_default;
> > -		    $contents = "";
> > +		if ($1 eq "") {
> > +		    if ($section =~ m/^@/ || $section eq $section_context) {
> > +			dump_section($file, $section, xml_escape($contents));
> > +			$section = $section_default;
> > +			$contents = "";
> > +		    } else {
> > +			$contents .= "\n";
> > +		    }
> > +		    $in_purpose = 0;
> > +		} elsif ($in_purpose == 1) {
> > +		    # Continued declaration purpose
> > +		    chomp($declaration_purpose);
> > +		    $declaration_purpose .= " " . $1;
> 
> Why shouldn't this be:
> 		    $declaration_purpose .= " " . xml_escape($1);
> ?

Sorry, this should be escaped of course!  Could you edit the patch on
your side?

Thanks,
	Hannes

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Randy Dunlap]

On Mon, 24 Aug 2009 22:09:50 +0200 Johannes Weiner wrote:

> Hello Randy,
> 
> On Mon, Aug 24, 2009 at 12:27:11PM -0700, Randy Dunlap wrote:
> > On Mon, 24 Aug 2009 12:06:54 -0700 (PDT) Johannes Weiner wrote:
> > 
> > I'll add this to my kernel-doc quilt patch series.
> 
> Thanks!
> 
> > Oh, one question below...
> 
> > > @@ -2119,11 +2122,19 @@ sub process_file($) {
> > >  	    } elsif (/$doc_content/) {
> > >  		# miguel-style comment kludge, look for blank lines after
> > >  		# @parameter line to signify start of description
> > > -		if ($1 eq "" &&
> > > -			($section =~ m/^@/ || $section eq $section_context)) {
> > > -		    dump_section($file, $section, xml_escape($contents));
> > > -		    $section = $section_default;
> > > -		    $contents = "";
> > > +		if ($1 eq "") {
> > > +		    if ($section =~ m/^@/ || $section eq $section_context) {
> > > +			dump_section($file, $section, xml_escape($contents));
> > > +			$section = $section_default;
> > > +			$contents = "";
> > > +		    } else {
> > > +			$contents .= "\n";
> > > +		    }
> > > +		    $in_purpose = 0;
> > > +		} elsif ($in_purpose == 1) {
> > > +		    # Continued declaration purpose
> > > +		    chomp($declaration_purpose);
> > > +		    $declaration_purpose .= " " . $1;
> > 
> > Why shouldn't this be:
> > 		    $declaration_purpose .= " " . xml_escape($1);
> > ?
> 
> Sorry, this should be escaped of course!  Could you edit the patch on
> your side?

Sure, will do.


---
~Randy
LPC 2009, Sept. 23-25, Portland, Oregon
http://linuxplumbersconf.org/2009/

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Randy Dunlap]

From: peterz@infradead.org

On Wed, 2009-08-12 at 11:41 +0200, Ingo Molnar wrote:
> * Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > This patch (as1279) adds kerneldoc for flush_scheduled_work()
> > containing a stern warning that the function should be avoided.
> > 
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

> >  
> > +/**
> > + * flush_scheduled_work - ensure that all work scheduled on keventd_wq has run to completion.

> > + */
> 
> Looks good - a small nit: please use proper/consistent line length, 
> something like:
> 
> /**
>  * flush_scheduled_work - ensure that all work scheduled on 
>  *			  keventd_wq has run to completion
>  *

>  */


| And here I was thinking kerneldoc doesn't actually work like that, but
| perhaps Randy fixed it so the initial description can line-wrap?

Nope, function short description is still just a one-liner.

~Randy

[PATCH] Add kerneldoc for flush_scheduled_work()

[Alan Stern]

This patch (as1279) adds kerneldoc for flush_scheduled_work()
containing a stern warning that the function should be avoided.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

Index: usb-2.6/kernel/workqueue.c
===================================================================
--- usb-2.6.orig/kernel/workqueue.c
+++ usb-2.6/kernel/workqueue.c
@@ -739,6 +739,24 @@ int schedule_on_each_cpu(work_func_t fun
 	return 0;
 }
 
+/**
+ * flush_scheduled_work - ensure that all work scheduled on keventd_wq has run to completion.
+ *
+ * Blocks until all works on the keventd_wq global workqueue have completed.
+ * We sleep until all works present upon entry have been handled, but we
+ * are not livelocked by new incoming ones.
+ *
+ * Use of this function is discouraged, as it is highly prone to deadlock.
+ * It should never be called from within a work routine on the global
+ * queue, and it should never be called while holding a mutex required
+ * by one of the works on the global queue.  But the fact that keventd_wq
+ * _is_ global means that it can contain works requiring practically any
+ * mutex.  Hence this routine shouldn't be called while holding any mutex.
+ *
+ * Consider using cancel_work_sync() or cancel_delayed_work_sync() instead.
+ * They don't do the same thing (they cancel the work instead of waiting
+ * for it to complete), but in most cases they will suffice.
+ */
 void flush_scheduled_work(void)
 {
 	flush_workqueue(keventd_wq);

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Ingo Molnar]

* Alan Stern <stern@rowland.harvard.edu> wrote:

> This patch (as1279) adds kerneldoc for flush_scheduled_work()
> containing a stern warning that the function should be avoided.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ---
> 
> Index: usb-2.6/kernel/workqueue.c
> ===================================================================
> --- usb-2.6.orig/kernel/workqueue.c
> +++ usb-2.6/kernel/workqueue.c
> @@ -739,6 +739,24 @@ int schedule_on_each_cpu(work_func_t fun
>  	return 0;
>  }
>  
> +/**
> + * flush_scheduled_work - ensure that all work scheduled on keventd_wq has run to completion.
> + *
> + * Blocks until all works on the keventd_wq global workqueue have completed.
> + * We sleep until all works present upon entry have been handled, but we
> + * are not livelocked by new incoming ones.
> + *
> + * Use of this function is discouraged, as it is highly prone to deadlock.
> + * It should never be called from within a work routine on the global
> + * queue, and it should never be called while holding a mutex required
> + * by one of the works on the global queue.  But the fact that keventd_wq
> + * _is_ global means that it can contain works requiring practically any
> + * mutex.  Hence this routine shouldn't be called while holding any mutex.
> + *
> + * Consider using cancel_work_sync() or cancel_delayed_work_sync() instead.
> + * They don't do the same thing (they cancel the work instead of waiting
> + * for it to complete), but in most cases they will suffice.
> + */

Looks good - a small nit: please use proper/consistent line length, 
something like:

/**
 * flush_scheduled_work - ensure that all work scheduled on 
 *			  keventd_wq has run to completion
 *
 * Blocks until all works on the keventd_wq global workqueue have 
 * completed.  We sleep until all works present upon entry have been 
 * handled, but we are not livelocked by new incoming ones.
 *
 * Use of this function is discouraged, as it is highly prone to 
 * deadlock.  It should never be called from within a work routine 
 * on the global queue, and it should never be called while holding 
 * a mutex required by one of the works on the global queue.  But 
 * the fact that keventd_wq _is_ global means that it can contain 
 * works requiring practically any mutex.  Hence this routine 
 * shouldn't be called while holding any mutex.
 *
 * Consider using cancel_work_sync() or cancel_delayed_work_sync() 
 * instead.  They don't do the same thing (they cancel the work 
 * instead of waiting for it to complete), but in most cases they 
 * will suffice.
 */

Thanks,

	Ingo

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Peter Zijlstra]

On Wed, 2009-08-12 at 11:41 +0200, Ingo Molnar wrote:
> * Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > This patch (as1279) adds kerneldoc for flush_scheduled_work()
> > containing a stern warning that the function should be avoided.
> > 
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

> >  
> > +/**
> > + * flush_scheduled_work - ensure that all work scheduled on keventd_wq has run to completion.

> > + */
> 
> Looks good - a small nit: please use proper/consistent line length, 
> something like:
> 
> /**
>  * flush_scheduled_work - ensure that all work scheduled on 
>  *			  keventd_wq has run to completion
>  *

>  */


And here I was thinking kerneldoc doesn't actually work like that, but
perhaps Randy fixed it so the initial description can line-wrap?

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, 2009-08-12 at 11:41 +0200, Ingo Molnar wrote:
> > * Alan Stern <stern@rowland.harvard.edu> wrote:
> > 
> > > This patch (as1279) adds kerneldoc for flush_scheduled_work()
> > > containing a stern warning that the function should be avoided.
> > > 
> > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> > >  
> > > +/**
> > > + * flush_scheduled_work - ensure that all work scheduled on keventd_wq has run to completion.
> 
> > > + */
> > 
> > Looks good - a small nit: please use proper/consistent line length, 
> > something like:
> > 
> > /**
> >  * flush_scheduled_work - ensure that all work scheduled on 
> >  *			  keventd_wq has run to completion
> >  *
> 
> >  */
> 
> 
> And here I was thinking kerneldoc doesn't actually work 
> like that, but perhaps Randy fixed it so the initial 
> description can line-wrap?

Ah, the main changes i did were to the body, that is what 
caught my eyes. If KernelDoc doesnt handle line-wrap in the 
first line then that has to stay so. (and KernelDoc needs to 
be fixed as well - it should result in better documentation, 
not worse)

	Ingo

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Alan Stern]

On Wed, 12 Aug 2009, Ingo Molnar wrote:

> > And here I was thinking kerneldoc doesn't actually work 
> > like that, but perhaps Randy fixed it so the initial 
> > description can line-wrap?

Yes, that's what I thought too.  If kerneldoc has been fixed then the 
description line certainly should get wrapped.

On Wed, 12 Aug 2009, Ingo Molnar wrote:

> Looks good - a small nit: please use proper/consistent line length,
> something like:

> Ah, the main changes i did were to the body, that is what 
> caught my eyes.

Point taken.  Would you like me to reformat the body text and resubmit
the patch?

Alan Stern

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Ingo Molnar]

* Alan Stern <stern@rowland.harvard.edu> wrote:

> On Wed, 12 Aug 2009, Ingo Molnar wrote:
> 
> > > And here I was thinking kerneldoc doesn't actually work 
> > > like that, but perhaps Randy fixed it so the initial 
> > > description can line-wrap?
> 
> Yes, that's what I thought too.  If kerneldoc has been fixed then the 
> description line certainly should get wrapped.
> 
> On Wed, 12 Aug 2009, Ingo Molnar wrote:
> 
> > Looks good - a small nit: please use proper/consistent line length,
> > something like:
> 
> > Ah, the main changes i did were to the body, that is what 
> > caught my eyes.
> 
> Point taken.  Would you like me to reformat the body text and 
> resubmit the patch?

Yeah, please do if you've got the time.

	Ingo

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[James Bottomley]

On Wed, 2009-08-12 at 10:13 -0400, Alan Stern wrote:
> On Wed, 12 Aug 2009, Ingo Molnar wrote:
> 
> > > And here I was thinking kerneldoc doesn't actually work 
> > > like that, but perhaps Randy fixed it so the initial 
> > > description can line-wrap?
> 
> Yes, that's what I thought too.  If kerneldoc has been fixed then the 
> description line certainly should get wrapped.

I really don't think it needs to be fixed: it's a feature not a bug.  It
requires people writing kernel doc actually to think of one line
summaries.

LSI recently tried to submit a ten line wrapped summary which the
current feature makes it very easy to knock back and say this must be a
single line, so trim it and move the rest to the function description
body.

James

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Ingo Molnar]

* James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> On Wed, 2009-08-12 at 10:13 -0400, Alan Stern wrote:
> > On Wed, 12 Aug 2009, Ingo Molnar wrote:
> > 
> > > > And here I was thinking kerneldoc doesn't actually work 
> > > > like that, but perhaps Randy fixed it so the initial 
> > > > description can line-wrap?
> > 
> > Yes, that's what I thought too.  If kerneldoc has been fixed 
> > then the description line certainly should get wrapped.
> 
> I really don't think it needs to be fixed: it's a feature not a 
> bug.  It requires people writing kernel doc actually to think of 
> one line summaries.

As long as the argument is that it's good to have limitations just 
because it has good effects as well (which the gist of your argument 
seems to be), i disagree.

That's a very basic argument of freedom. Just consider the Gestapo 
which was also a 'feature' to keep criminals in check. Did you know 
that there were record low levels of petty criminality both in nazi 
Germany and during communism (and under just about any totalitarian 
regime)? Still nobody in their right mind is arguing that just due 
to that they are the right social model ...

I think this DocBook limitation needs to be fixed, because there are 
legitimate cases where a function name got too long (for no fault of 
its own, but for properties of the name-space it is operating in), 
and we do not want a nanny state beat it into a single line.

> LSI recently tried to submit a ten line wrapped summary which the 
> current feature makes it very easy to knock back and say this must 
> be a single line, so trim it and move the rest to the function 
> description body.

Uhm, the democratic solution for _that_ problem is to add a very 
simple check/warning to checkpatch.pl.

Freedom to use common sense and stuff. That principle is in the US 
Constitution as well, or something quite similar to that, right? ;-)

	Ingo

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Johannes Weiner]

On Thu, Aug 13, 2009 at 09:25:14AM +0200, Ingo Molnar wrote:
> 
> * James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > On Wed, 2009-08-12 at 10:13 -0400, Alan Stern wrote:
> > > On Wed, 12 Aug 2009, Ingo Molnar wrote:
> > > 
> > > > > And here I was thinking kerneldoc doesn't actually work 
> > > > > like that, but perhaps Randy fixed it so the initial 
> > > > > description can line-wrap?
> > > 
> > > Yes, that's what I thought too.  If kerneldoc has been fixed 
> > > then the description line certainly should get wrapped.
> > 
> > I really don't think it needs to be fixed: it's a feature not a 
> > bug.  It requires people writing kernel doc actually to think of 
> > one line summaries.
> 
> As long as the argument is that it's good to have limitations just 
> because it has good effects as well (which the gist of your argument 
> seems to be), i disagree.
> 
> That's a very basic argument of freedom. Just consider the Gestapo 
> which was also a 'feature' to keep criminals in check. Did you know 
> that there were record low levels of petty criminality both in nazi 
> Germany and during communism (and under just about any totalitarian 
> regime)? Still nobody in their right mind is arguing that just due 
> to that they are the right social model ...

Although I really like how you Godwin'd kerneldoc comments ;-), we do
have other features that are features because of their limiting effect
all over the place, don't we?  The 80-columns code rule e.g. or our
limited set of allowed indenting characters.

> I think this DocBook limitation needs to be fixed, because there are 
> legitimate cases where a function name got too long (for no fault of 
> its own, but for properties of the name-space it is operating in), 
> and we do not want a nanny state beat it into a single line.

Agreed, just as in the other rules, one should be able to bend this
one once in a while without technical consequences, i.e. without
kerneldoc breaking.

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Ingo Molnar]

* Johannes Weiner <hannes@cmpxchg.org> wrote:

> On Thu, Aug 13, 2009 at 09:25:14AM +0200, Ingo Molnar wrote:
> > 
> > * James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > 
> > > On Wed, 2009-08-12 at 10:13 -0400, Alan Stern wrote:
> > > > On Wed, 12 Aug 2009, Ingo Molnar wrote:
> > > > 
> > > > > > And here I was thinking kerneldoc doesn't actually work 
> > > > > > like that, but perhaps Randy fixed it so the initial 
> > > > > > description can line-wrap?
> > > > 
> > > > Yes, that's what I thought too.  If kerneldoc has been fixed 
> > > > then the description line certainly should get wrapped.
> > > 
> > > I really don't think it needs to be fixed: it's a feature not a 
> > > bug.  It requires people writing kernel doc actually to think of 
> > > one line summaries.
> > 
> > As long as the argument is that it's good to have limitations just 
> > because it has good effects as well (which the gist of your argument 
> > seems to be), i disagree.
> > 
> > That's a very basic argument of freedom. Just consider the Gestapo 
> > which was also a 'feature' to keep criminals in check. Did you know 
> > that there were record low levels of petty criminality both in nazi 
> > Germany and during communism (and under just about any totalitarian 
> > regime)? Still nobody in their right mind is arguing that just due 
> > to that they are the right social model ...
> 
> Although I really like how you Godwin'd kerneldoc comments ;-), 
> [...]

Yeah, i'm quite proud of that angle too! It helps keep things 
technical ;-)

> [...] we do have other features that are features because of their 
> limiting effect all over the place, don't we?  The 80-columns code 
> rule e.g. or our limited set of allowed indenting characters.

The difference is that KernelDoc violations break the KernelDoc 
build (and the KernelDoc end result) and get reported (and fixed) as 
regressions.

While 80-columns code rule is a flexible rule that can be ignored if 
it causes a worse end result.

I'd even proffer that the majority of overlong lines in KernelDoc 
are real problems. (just like the majority of line-80 problems are 
real problems on some level)

> > I think this DocBook limitation needs to be fixed, because there 
> > are legitimate cases where a function name got too long (for no 
> > fault of its own, but for properties of the name-space it is 
> > operating in), and we do not want a nanny state beat it into a 
> > single line.
> 
> Agreed, just as in the other rules, one should be able to bend 
> this one once in a while without technical consequences, i.e. 
> without kerneldoc breaking.

Yeah, exactly. Which is why i suggested KernelDoc to be fixed. In 
terms of non-intrusive warnings, scripts/checkpatch.pl is a pretty 
good place.

	Ingo

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[James Bottomley]

On Thu, 2009-08-13 at 09:25 +0200, Ingo Molnar wrote:
> * James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > On Wed, 2009-08-12 at 10:13 -0400, Alan Stern wrote:
> > > On Wed, 12 Aug 2009, Ingo Molnar wrote:
> > > 
> > > > > And here I was thinking kerneldoc doesn't actually work 
> > > > > like that, but perhaps Randy fixed it so the initial 
> > > > > description can line-wrap?
> > > 
> > > Yes, that's what I thought too.  If kerneldoc has been fixed 
> > > then the description line certainly should get wrapped.
> > 
> > I really don't think it needs to be fixed: it's a feature not a 
> > bug.  It requires people writing kernel doc actually to think of 
> > one line summaries.
> 
> As long as the argument is that it's good to have limitations just 
> because it has good effects as well (which the gist of your argument 
> seems to be), i disagree.

You're free to disagree

> That's a very basic argument of freedom. Just consider the Gestapo 
> which was also a 'feature' to keep criminals in check. Did you know 
> that there were record low levels of petty criminality both in nazi 
> Germany and during communism (and under just about any totalitarian 
> regime)? Still nobody in their right mind is arguing that just due 
> to that they are the right social model ...
> 
> I think this DocBook limitation needs to be fixed, because there are 
> legitimate cases where a function name got too long (for no fault of 
> its own, but for properties of the name-space it is operating in), 
> and we do not want a nanny state beat it into a single line.

But your argument is bogus.  We have tons of additional rules in Linux
coding and we have a variety of enforcement mechanisms from BUILD_BUG_ON
through to checkpatch.pl.  This really doesn't have anything to do with
"freedom".

There are really two questions here

     1. Is it a good rule that our oneline docbook function summaries
        should be, well, one line?
     2. Is the enforcement mechanism for this rule adequate?

I think 1. is reasonable.  I think 2. needs work because you don't see
the problem until make doc.

James

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[James Bottomley]

On Tue, 2009-08-11 at 17:06 -0400, Alan Stern wrote:
> This patch (as1279) adds kerneldoc for flush_scheduled_work()
> containing a stern warning that the function should be avoided.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ---
> 
> Index: usb-2.6/kernel/workqueue.c
> ===================================================================
> --- usb-2.6.orig/kernel/workqueue.c
> +++ usb-2.6/kernel/workqueue.c
> @@ -739,6 +739,24 @@ int schedule_on_each_cpu(work_func_t fun
>  	return 0;
>  }
>  
> +/**
> + * flush_scheduled_work - ensure that all work scheduled on keventd_wq has run to completion.
> + *
> + * Blocks until all works on the keventd_wq global workqueue have completed.
> + * We sleep until all works present upon entry have been handled, but we
> + * are not livelocked by new incoming ones.
> + *
> + * Use of this function is discouraged, as it is highly prone to deadlock.
> + * It should never be called from within a work routine on the global
> + * queue, and it should never be called while holding a mutex required
> + * by one of the works on the global queue.  But the fact that keventd_wq
> + * _is_ global means that it can contain works requiring practically any
> + * mutex.  Hence this routine shouldn't be called while holding any mutex.

I really don't see why this should be "discouraged".  It has exactly the
same entangled deadlock problems as a lot of other functions like
wait_event().

> + * Consider using cancel_work_sync() or cancel_delayed_work_sync() instead.
> + * They don't do the same thing (they cancel the work instead of waiting
> + * for it to complete), but in most cases they will suffice.
> + */

And this is wrong advice.  If you've violated the entangled deadlock
rules, the cancel functions will deadlock on you as well if the work is
still pending.  The other problem is that cancellation is a completely
different operation from flushing.  Flush is usually used in driver
shutdown routines to complete all outstanding tasks before going away.
Cancel wouldn't really have the same effect since the tasks may never
get done.

The final problem with this is that it creates a dichotomy between this
function, which is simply a wrapper for flush_workqueue() on the
keventd_wq and flush_workqueue().  flush_workqueue() has no scary
warnings, so are we trying to encourage driver writers all to create
their own workqueues for everything and declaring the global kernel
workqueue useless?  Or are we saying flushing workqueues is always bad
and we just haven't got around to updating the documentation on
flush_workqueue()?

James

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Alan Stern]

On Wed, 12 Aug 2009, James Bottomley wrote:

> > + * Use of this function is discouraged, as it is highly prone to deadlock.
> > + * It should never be called from within a work routine on the global
> > + * queue, and it should never be called while holding a mutex required
> > + * by one of the works on the global queue.  But the fact that keventd_wq
> > + * _is_ global means that it can contain works requiring practically any
> > + * mutex.  Hence this routine shouldn't be called while holding any mutex.
> 
> I really don't see why this should be "discouraged".  It has exactly the
> same entangled deadlock problems as a lot of other functions like
> wait_event().

In this case the problems tend to be worse, because all the items on
the workqueue are involved.  Any one of them could cause a deadlock.

With wait_event(), the interaction generally involves only one other
logical flow of control -- the one that ends up signalling the
waitqueue.  But with flush_scheduled_work() the interaction involves
everything on the workqueue, even items not at all related to event
being waited for.

In short, it's a lot easier to fall into this trap with
flush_scheduled_work() than with wait_event().  That's why using the
function is discouraged.


> > + * Consider using cancel_work_sync() or cancel_delayed_work_sync() instead.
> > + * They don't do the same thing (they cancel the work instead of waiting
> > + * for it to complete), but in most cases they will suffice.
> > + */
> 
> And this is wrong advice.  If you've violated the entangled deadlock
> rules, the cancel functions will deadlock on you as well if the work is
> still pending.

No they won't.  They will remove the work item from the workqueue right
away, without blocking, assuming it hasn't started yet (i.e., is still 
pending).  This is a large part of their raison d'etre.

>  The other problem is that cancellation is a completely
> different operation from flushing.  Flush is usually used in driver
> shutdown routines to complete all outstanding tasks before going away.

I disagree.  In practice, flush is usually used in driver shutdown
routines to insure that there are no outstanding tasks before going
away.  Whether the tasks run to completion or are cancelled doesn't
matter to the driver, so long as they don't remain outstanding.

> Cancel wouldn't really have the same effect since the tasks may never
> get done.

That's what I wrote in the kerneldoc.  You're agreeing with me!  It's a
miracle!  :-)


> The final problem with this is that it creates a dichotomy between this
> function, which is simply a wrapper for flush_workqueue() on the
> keventd_wq and flush_workqueue().  flush_workqueue() has no scary
> warnings, so are we trying to encourage driver writers all to create
> their own workqueues for everything and declaring the global kernel
> workqueue useless?

Sometimes creating a private workqueue is appropriate, but usually it
isn't.  The kerneldoc isn't meant to warn writers away from using the
global workqueue; it's meant only to warn them away from
flush_scheduled_work().  It's perfectly possible to use one without the
other -- especially if you can substitute cancel_work_sync().

>  Or are we saying flushing workqueues is always bad
> and we just haven't got around to updating the documentation on
> flush_workqueue()?

That's a good point.  It might be worthwhile bringing it up in the
kerneldoc.

With private workqueues, the writer has much more control over the work 
items added to the queue.  It's feasible to audit the code and verify 
that none of them will cause flush_workqueue() to deadlock.  With the 
global workqueue this isn't feasible.  Hence the dichotomy.

Alan Stern

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[James Bottomley]

On Wed, 2009-08-12 at 10:54 -0400, Alan Stern wrote:
> > > + * Consider using cancel_work_sync() or cancel_delayed_work_sync() instead.
> > > + * They don't do the same thing (they cancel the work instead of waiting
> > > + * for it to complete), but in most cases they will suffice.
> > > + */
> > 
> > And this is wrong advice.  If you've violated the entangled deadlock
> > rules, the cancel functions will deadlock on you as well if the work is
> > still pending.
> 
> No they won't.  They will remove the work item from the workqueue right
> away, without blocking, assuming it hasn't started yet (i.e., is still 
> pending).  This is a large part of their raison d'etre.

Yes, they will ... you advised the _sync function which waits if the
work is in progress and hence induces the entanglement.  You can get
away with this if you don't use _sync (but then you won't know when the
queue is safely not touching any module code before removal).

James

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Alan Stern]

On Wed, 12 Aug 2009, James Bottomley wrote:

> On Wed, 2009-08-12 at 10:54 -0400, Alan Stern wrote:
> > > > + * Consider using cancel_work_sync() or cancel_delayed_work_sync() instead.
> > > > + * They don't do the same thing (they cancel the work instead of waiting
> > > > + * for it to complete), but in most cases they will suffice.
> > > > + */
> > > 
> > > And this is wrong advice.  If you've violated the entangled deadlock
> > > rules, the cancel functions will deadlock on you as well if the work is
> > > still pending.
> > 
> > No they won't.  They will remove the work item from the workqueue right
> > away, without blocking, assuming it hasn't started yet (i.e., is still 
> > pending).  This is a large part of their raison d'etre.
> 
> Yes, they will ... you advised the _sync function which waits if the
> work is in progress and hence induces the entanglement.  You can get
> away with this if you don't use _sync (but then you won't know when the
> queue is safely not touching any module code before removal).

This is partly a simple misunderstanding.  By "pending", I thought you
meant that the work item was still on the workqueue but had not yet
been invoked (i.e., "pending" as opposed to "running").  In that
situation there will be no deadlock, as I said.

But let's consider the case you bring up, where the work item _has_
started to run.  It's true that the work routine can cause deadlock by
violating the locking rules, regardless of whether you use
cancel_work_sync() or flush_scheduled_work().

Nevertheless, even in this case cancel_work_sync() is _still_ more
reliable than flush_schedule_work().  Example: Let's suppose you're
interested in work item A, which is at the head of the workqueue and
has already started running.  Let's also suppose that A doesn't violate
the locking rules (since otherwise things are hopeless).  Finally,
let's assume the workqueue contains a second unrelated work item, B.

Here's what can happen with flush_scheduled_work():

	CPU 0			CPU 1
	-----			-----
				A:	(invoked from workqueue)
				... does some stuff ...
	lock_mutex(&m);
	flush_scheduled_work();
	... waits ...
				A finishes
				B:	(invoked from workqueue)
				lock_mutex(&m);		/* Deadlock! */

Now consider the same situation with cancel_work_sync():

	CPU 0			CPU 1
	-----			-----
				A:	(invoked from workqueue)
				... does some stuff ...
	lock_mutex(&m);
	cancel_work_sync(&A);
	... waits ...
				A finishes
	... continues running ...
				B:	(invoked from workqueue)
				lock_mutex(&m);
	unlock_mutex(&m);
				... continues running ...

You can see the difference.  There's no doubt about it,
flush_scheduled_work() is more dangerous than cancel_work_sync().

Alan Stern

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[James Bottomley]

On Wed, 2009-08-12 at 11:44 -0400, Alan Stern wrote:
> On Wed, 12 Aug 2009, James Bottomley wrote:
> 
> > On Wed, 2009-08-12 at 10:54 -0400, Alan Stern wrote:
> > > > > + * Consider using cancel_work_sync() or cancel_delayed_work_sync() instead.
> > > > > + * They don't do the same thing (they cancel the work instead of waiting
> > > > > + * for it to complete), but in most cases they will suffice.
> > > > > + */
> > > > 
> > > > And this is wrong advice.  If you've violated the entangled deadlock
> > > > rules, the cancel functions will deadlock on you as well if the work is
> > > > still pending.
> > > 
> > > No they won't.  They will remove the work item from the workqueue right
> > > away, without blocking, assuming it hasn't started yet (i.e., is still 
> > > pending).  This is a large part of their raison d'etre.
> > 
> > Yes, they will ... you advised the _sync function which waits if the
> > work is in progress and hence induces the entanglement.  You can get
> > away with this if you don't use _sync (but then you won't know when the
> > queue is safely not touching any module code before removal).
> 
> This is partly a simple misunderstanding.  By "pending", I thought you
> meant that the work item was still on the workqueue but had not yet
> been invoked (i.e., "pending" as opposed to "running").  In that
> situation there will be no deadlock, as I said.
> 
> But let's consider the case you bring up, where the work item _has_
> started to run.  It's true that the work routine can cause deadlock by
> violating the locking rules, regardless of whether you use
> cancel_work_sync() or flush_scheduled_work().
> 
> Nevertheless, even in this case cancel_work_sync() is _still_ more
> reliable than flush_schedule_work().  Example: Let's suppose you're
> interested in work item A, which is at the head of the workqueue and
> has already started running.  Let's also suppose that A doesn't violate
> the locking rules (since otherwise things are hopeless).  Finally,
> let's assume the workqueue contains a second unrelated work item, B.
> 
> Here's what can happen with flush_scheduled_work():
> 
> 	CPU 0			CPU 1
> 	-----			-----
> 				A:	(invoked from workqueue)
> 				... does some stuff ...
> 	lock_mutex(&m);
> 	flush_scheduled_work();
> 	... waits ...
> 				A finishes
> 				B:	(invoked from workqueue)
> 				lock_mutex(&m);		/* Deadlock! */
> 
> Now consider the same situation with cancel_work_sync():
> 
> 	CPU 0			CPU 1
> 	-----			-----
> 				A:	(invoked from workqueue)
> 				... does some stuff ...
> 	lock_mutex(&m);
> 	cancel_work_sync(&A);
> 	... waits ...
> 				A finishes
> 	... continues running ...
> 				B:	(invoked from workqueue)
> 				lock_mutex(&m);
> 	unlock_mutex(&m);
> 				... continues running ...
> 
> You can see the difference.  There's no doubt about it,
> flush_scheduled_work() is more dangerous than cancel_work_sync().

This all boils down to "the race window for a deadlock may be narrower".

Instead of training programmers to narrow deadlock races, we should be
training them to avoid them.

The entangled deadlock problem occurs in all of our _sync() APIs as well
as interrupt and other workqueue stuff.

The rules are something like

Never use synchronous operations if you can avoid them.  If you must use
operations that wait for another thread to complete (say because you're
about to destroy data structures that queued elements may be using):
     1. Never hold any locks or mutexes while waiting for the completion
        of synchronous operations
     2. If you have to hold a lock while waiting:
             1. If it's a global lock, make sure you're using a local
                queue and that nothing you submitted to the queue can
                take the lock
             2. If it's a local lock, you may use a global queue but
                must still make sure that nothing you submitted to the
                queue can take the lock.

James

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Alan Stern]

On Wed, 12 Aug 2009, James Bottomley wrote:

> This all boils down to "the race window for a deadlock may be narrower".
> 
> Instead of training programmers to narrow deadlock races, we should be
> training them to avoid them.
> 
> The entangled deadlock problem occurs in all of our _sync() APIs as well
> as interrupt and other workqueue stuff.
> 
> The rules are something like
> 
> Never use synchronous operations if you can avoid them.  If you must use
> operations that wait for another thread to complete (say because you're
> about to destroy data structures that queued elements may be using):
>      1. Never hold any locks or mutexes while waiting for the completion
>         of synchronous operations
>      2. If you have to hold a lock while waiting:
>              1. If it's a global lock, make sure you're using a local
>                 queue and that nothing you submitted to the queue can
>                 take the lock
>              2. If it's a local lock, you may use a global queue but
>                 must still make sure that nothing you submitted to the
>                 queue can take the lock.

I haven't seen these rules written down anywhere in the kernel 
documentation.  Presumably people are supposed to be aware of them 
already?

Anyway, 2.1 and 2.2 are wrong.  They should read: "... make sure that
nothing submitted to the queue calls any of your routines that can take
the lock."  The point being that even though _you_ don't submit
anything bad to the queue, somebody else might do so.

If you use cancel_work_sync() instead of flush_scheduled_work() then 
the rules become less onerous.  In place of your 2.1 and 2.2, we have:

	Make sure the work item you are cancelling cannot take
	the lock.

This is much easier to verify.

Alan Stern

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[James Bottomley]

On Wed, 2009-08-12 at 12:23 -0400, Alan Stern wrote:
> On Wed, 12 Aug 2009, James Bottomley wrote:
> 
> > This all boils down to "the race window for a deadlock may be narrower".
> > 
> > Instead of training programmers to narrow deadlock races, we should be
> > training them to avoid them.
> > 
> > The entangled deadlock problem occurs in all of our _sync() APIs as well
> > as interrupt and other workqueue stuff.
> > 
> > The rules are something like
> > 
> > Never use synchronous operations if you can avoid them.  If you must use
> > operations that wait for another thread to complete (say because you're
> > about to destroy data structures that queued elements may be using):
> >      1. Never hold any locks or mutexes while waiting for the completion
> >         of synchronous operations
> >      2. If you have to hold a lock while waiting:
> >              1. If it's a global lock, make sure you're using a local
> >                 queue and that nothing you submitted to the queue can
> >                 take the lock
> >              2. If it's a local lock, you may use a global queue but
> >                 must still make sure that nothing you submitted to the
> >                 queue can take the lock.
> 
> I haven't seen these rules written down anywhere in the kernel 
> documentation.  Presumably people are supposed to be aware of them 
> already?

Um, well they seemed pretty obvious to me, so I just wrote them down.

> Anyway, 2.1 and 2.2 are wrong.  They should read: "... make sure that
> nothing submitted to the queue calls any of your routines that can take
> the lock."  The point being that even though _you_ don't submit
> anything bad to the queue, somebody else might do so.

Semantically "make sure that nothing submitted to the queue can take a
lock" means exactly that ... it includes all routines called by the
queue function.

> If you use cancel_work_sync() instead of flush_scheduled_work() then 
> the rules become less onerous.  In place of your 2.1 and 2.2, we have:
> 
> 	Make sure the work item you are cancelling cannot take
> 	the lock.
> 
> This is much easier to verify.

It's the same, surely ... you must verify that everything going on to
the queue obeys the rules otherwise you get things on it which can't be
cancelled ... and thus either the advice to call cancel is wrong, or we
could call flush because the locking rules are obeyed.

James

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Alan Stern]

On Wed, 12 Aug 2009, James Bottomley wrote:

> > >      2. If you have to hold a lock while waiting:
> > >              1. If it's a global lock, make sure you're using a local
> > >                 queue and that nothing you submitted to the queue can
> > >                 take the lock
> > >              2. If it's a local lock, you may use a global queue but
> > >                 must still make sure that nothing you submitted to the
> > >                 queue can take the lock.

> > Anyway, 2.1 and 2.2 are wrong.  They should read: "... make sure that

(I overstated things; 2.1 is okay.  But 2.2. is wrong.  And 
flush_scheduled_work() definitely concerns a global queue.)

> > nothing submitted to the queue calls any of your routines that can take
> > the lock."  The point being that even though _you_ don't submit
> > anything bad to the queue, somebody else might do so.
> 
> Semantically "make sure that nothing submitted to the queue can take a
> lock" means exactly that ... it includes all routines called by the
> queue function.

James, you wrote "make sure that nothing _you_ submitted to the queue" 
(my emphasis).  That is quite different from "make sure that nothing 
submitted to the queue".  One is doable, the other isn't.

> > If you use cancel_work_sync() instead of flush_scheduled_work() then 
> > the rules become less onerous.  In place of your 2.1 and 2.2, we have:
> > 
> > 	Make sure the work item you are cancelling cannot take
> > 	the lock.
> > 
> > This is much easier to verify.
> 
> It's the same, surely ... you must verify that everything going on to
> the queue obeys the rules otherwise you get things on it which can't be
> cancelled ...

Not at all.  With cancel_work_sync() you must verify only that the
item you want to cancel obeys the rules.  There's no need to check the
other items -- and a public workqueue like keventd_wq certainly will
contain other items.

> and thus either the advice to call cancel is wrong, or we
> could call flush because the locking rules are obeyed.

No; as shown above, your logic is flawed.

Alan Stern

P.S.: Ingo, I rather expected to see you comment on this thread.  What 
is your opinion?

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[James Bottomley]

On Wed, 2009-08-12 at 13:25 -0400, Alan Stern wrote:
> On Wed, 12 Aug 2009, James Bottomley wrote:
> 
> > > >      2. If you have to hold a lock while waiting:
> > > >              1. If it's a global lock, make sure you're using a local
> > > >                 queue and that nothing you submitted to the queue can
> > > >                 take the lock
> > > >              2. If it's a local lock, you may use a global queue but
> > > >                 must still make sure that nothing you submitted to the
> > > >                 queue can take the lock.
> 
> > > Anyway, 2.1 and 2.2 are wrong.  They should read: "... make sure that
> 
> (I overstated things; 2.1 is okay.  But 2.2. is wrong.  And 
> flush_scheduled_work() definitely concerns a global queue.)
> 
> > > nothing submitted to the queue calls any of your routines that can take
> > > the lock."  The point being that even though _you_ don't submit
> > > anything bad to the queue, somebody else might do so.
> > 
> > Semantically "make sure that nothing submitted to the queue can take a
> > lock" means exactly that ... it includes all routines called by the
> > queue function.
> 
> James, you wrote "make sure that nothing _you_ submitted to the queue" 
> (my emphasis).  That is quite different from "make sure that nothing 
> submitted to the queue".  One is doable, the other isn't.

If it's a local lock, how would something someone else submitted to the
queue, which would be out of scope, take this local lock?  A local lock,
by definition is local to the code scope you control.

> > > If you use cancel_work_sync() instead of flush_scheduled_work() then 
> > > the rules become less onerous.  In place of your 2.1 and 2.2, we have:
> > > 
> > > 	Make sure the work item you are cancelling cannot take
> > > 	the lock.
> > > 
> > > This is much easier to verify.
> > 
> > It's the same, surely ... you must verify that everything going on to
> > the queue obeys the rules otherwise you get things on it which can't be
> > cancelled ...
> 
> Not at all.  With cancel_work_sync() you must verify only that the
> item you want to cancel obeys the rules.  There's no need to check the
> other items -- and a public workqueue like keventd_wq certainly will
> contain other items.

Um, so this is called on driver removal, which is an asynchronous event.
Thus, how can you assure that what's on the queue (which you are
advocating calling cancel sync for) doesn't violate the locking rules at
the time the driver is removed, unless you assure that every piece of
work submitted doesn't violate them.

Thus the rules for calling flush and cancel sync are the same.

James


> > and thus either the advice to call cancel is wrong, or we
> > could call flush because the locking rules are obeyed.
> 
> No; as shown above, your logic is flawed.
> 
> Alan Stern
> 
> P.S.: Ingo, I rather expected to see you comment on this thread.  What 
> is your opinion?

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Alan Stern]

On Wed, 12 Aug 2009, James Bottomley wrote:

> If it's a local lock, how would something someone else submitted to the
> queue, which would be out of scope, take this local lock?  A local lock,
> by definition is local to the code scope you control.

This can happen easily.  Somebody else submits a work item to the
queue, the work routine calls one of your publicly exported functions,
and that function takes the local lock.

> > Not at all.  With cancel_work_sync() you must verify only that the
> > item you want to cancel obeys the rules.  There's no need to check the
> > other items -- and a public workqueue like keventd_wq certainly will
> > contain other items.
> 
> Um, so this is called on driver removal, which is an asynchronous event.
> Thus, how can you assure that what's on the queue (which you are
> advocating calling cancel sync for) doesn't violate the locking rules at
> the time the driver is removed, unless you assure that every piece of
> work submitted doesn't violate them.

You can assure it by auditing the work routine's code.  A single work
item involves a single function, plus whatever that function calls -- 
it is easily checked.

You don't need to inspect every work item ever added to the queue, only 
the one that you want to cancel.

> Thus the rules for calling flush and cancel sync are the same.

They are not.  With cancel you need to verify that the one item you are
cancelling obeys the rules.  With flush you need to verify that
_everything_ on the queue is safe with respect to locking.

The only valid reason for a driver to call flush_scheduled_work()  
would be that it knows there's an outstanding work item, which has to
be completed or cancelled, but it doesn't have a pointer to the item.  
Then cancellation is impossible.

Alan Stern

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[James Bottomley]

On Wed, 2009-08-12 at 14:16 -0400, Alan Stern wrote:
> On Wed, 12 Aug 2009, James Bottomley wrote:
> 
> > If it's a local lock, how would something someone else submitted to the
> > queue, which would be out of scope, take this local lock?  A local lock,
> > by definition is local to the code scope you control.
> 
> This can happen easily.  Somebody else submits a work item to the
> queue, the work routine calls one of your publicly exported functions,
> and that function takes the local lock.

Fine, so local means not exported and not usable by exported functions.

> > > Not at all.  With cancel_work_sync() you must verify only that the
> > > item you want to cancel obeys the rules.  There's no need to check the
> > > other items -- and a public workqueue like keventd_wq certainly will
> > > contain other items.
> > 
> > Um, so this is called on driver removal, which is an asynchronous event.
> > Thus, how can you assure that what's on the queue (which you are
> > advocating calling cancel sync for) doesn't violate the locking rules at
> > the time the driver is removed, unless you assure that every piece of
> > work submitted doesn't violate them.
> 
> You can assure it by auditing the work routine's code.  A single work
> item involves a single function, plus whatever that function calls -- 
> it is easily checked.
> 
> You don't need to inspect every work item ever added to the queue, only 
> the one that you want to cancel.

This is true, but not relevant to the documentation you were trying to
add.  You're advocating using cancel sync as a replacement for flush ...
thus you have to cancel every pending piece of your work currently on
the queue.  My contention is that this means your rules for submission
if you do this must be the same as they would be if you'd just called
flush; making the advice moot.

James


> > Thus the rules for calling flush and cancel sync are the same.
> 
> They are not.  With cancel you need to verify that the one item you are
> cancelling obeys the rules.  With flush you need to verify that
> _everything_ on the queue is safe with respect to locking.
> 
> The only valid reason for a driver to call flush_scheduled_work()  
> would be that it knows there's an outstanding work item, which has to
> be completed or cancelled, but it doesn't have a pointer to the item.  
> Then cancellation is impossible.
> 
> Alan Stern

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Alan Stern]

On Wed, 12 Aug 2009, James Bottomley wrote:

> On Wed, 2009-08-12 at 14:16 -0400, Alan Stern wrote:
> > On Wed, 12 Aug 2009, James Bottomley wrote:
> > 
> > > If it's a local lock, how would something someone else submitted to the
> > > queue, which would be out of scope, take this local lock?  A local lock,
> > > by definition is local to the code scope you control.
> > 
> > This can happen easily.  Somebody else submits a work item to the
> > queue, the work routine calls one of your publicly exported functions,
> > and that function takes the local lock.
> 
> Fine, so local means not exported and not usable by exported functions.

I'll accept that.  However it means that the SCSI midlayer violates
your own locking rules with regard to the scan mutex.  Whereas if
flush_scheduled_work() were avoided, the locking rules would not be
violated.


> > You can assure it by auditing the work routine's code.  A single work
> > item involves a single function, plus whatever that function calls -- 
> > it is easily checked.
> > 
> > You don't need to inspect every work item ever added to the queue, only 
> > the one that you want to cancel.
> 
> This is true, but not relevant to the documentation you were trying to
> add.  You're advocating using cancel sync as a replacement for flush ...
> thus you have to cancel every pending piece of your work currently on
> the queue.

In most cases there is only one or two work items to cancel.

I'll stick my neck out even farther: In the majority of places where
flush_scheduled_work() is used in the kernel, cancel_work_sync() would
work just as well if not better, and it isn't used only because the
code was written before cancel_work_sync() existed (or before the
writer knew about it).

>  My contention is that this means your rules for submission
> if you do this must be the same as they would be if you'd just called
> flush; making the advice moot.

The rules for submission are _not_ the same.  With 
flush_scheduled_work():

	Make sure that any lock you hold while flushing is private
	and is not used (even indirectly) by any of your publicly 
	exported routines.

With cancel_work_sync():

	Make sure that any lock you hold while cancelling is not
	used (even indirectly) by the work routine being cancelled.

Alan Stern

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[James Bottomley]

On Wed, 2009-08-12 at 14:48 -0400, Alan Stern wrote:
> On Wed, 12 Aug 2009, James Bottomley wrote:
> 
> > On Wed, 2009-08-12 at 14:16 -0400, Alan Stern wrote:
> > > On Wed, 12 Aug 2009, James Bottomley wrote:
> > > 
> > > > If it's a local lock, how would something someone else submitted to the
> > > > queue, which would be out of scope, take this local lock?  A local lock,
> > > > by definition is local to the code scope you control.
> > > 
> > > This can happen easily.  Somebody else submits a work item to the
> > > queue, the work routine calls one of your publicly exported functions,
> > > and that function takes the local lock.
> > 
> > Fine, so local means not exported and not usable by exported functions.
> 
> I'll accept that.  However it means that the SCSI midlayer violates
> your own locking rules with regard to the scan mutex.  Whereas if
> flush_scheduled_work() were avoided, the locking rules would not be
> violated.

Why do you think I've been trying to get rid of it?  Global mutexes
covering large swathes of code are always a bad idea.  The async code
already picked up a deadlock entanglement with it.

> > > You can assure it by auditing the work routine's code.  A single work
> > > item involves a single function, plus whatever that function calls -- 
> > > it is easily checked.
> > > 
> > > You don't need to inspect every work item ever added to the queue, only 
> > > the one that you want to cancel.
> > 
> > This is true, but not relevant to the documentation you were trying to
> > add.  You're advocating using cancel sync as a replacement for flush ...
> > thus you have to cancel every pending piece of your work currently on
> > the queue.
> 
> In most cases there is only one or two work items to cancel.
>
> I'll stick my neck out even farther: In the majority of places where
> flush_scheduled_work() is used in the kernel, cancel_work_sync() would
> work just as well if not better, and it isn't used only because the
> code was written before cancel_work_sync() existed (or before the
> writer knew about it).
> 
> >  My contention is that this means your rules for submission
> > if you do this must be the same as they would be if you'd just called
> > flush; making the advice moot.
> 
> The rules for submission are _not_ the same.  With 
> flush_scheduled_work():
> 
> 	Make sure that any lock you hold while flushing is private
> 	and is not used (even indirectly) by any of your publicly 
> 	exported routines.
> 
> With cancel_work_sync():
> 
> 	Make sure that any lock you hold while cancelling is not
> 	used (even indirectly) by the work routine being cancelled.

The hair splitting has got to the point where I don't really care.  The
point is that flush_scheduled_work() and cancel_work_sync() have similar
problems.  Amazingly that was my original point.

James

Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[Alan Stern]

On Wed, 12 Aug 2009, James Bottomley wrote:

> > I'll accept that.  However it means that the SCSI midlayer violates
> > your own locking rules with regard to the scan mutex.  Whereas if
> > flush_scheduled_work() were avoided, the locking rules would not be
> > violated.
> 
> Why do you think I've been trying to get rid of it?  Global mutexes
> covering large swathes of code are always a bad idea.  The async code
> already picked up a deadlock entanglement with it.

And yet it does serve an important purpose.

An alternative to that mutex would be to make all scanning and removal 
activities on a host funnel through a single thread.  Do you think that 
would be preferable?  (I'm not trying to be sarcastic -- this is a 
serious question.)


> > The rules for submission are _not_ the same.  With 
> > flush_scheduled_work():
> > 
> > 	Make sure that any lock you hold while flushing is private
> > 	and is not used (even indirectly) by any of your publicly 
> > 	exported routines.
> > 
> > With cancel_work_sync():
> > 
> > 	Make sure that any lock you hold while cancelling is not
> > 	used (even indirectly) by the work routine being cancelled.
> 
> The hair splitting has got to the point where I don't really care.  The
> point is that flush_scheduled_work() and cancel_work_sync() have similar
> problems.  Amazingly that was my original point.

And my original point was that the problems, though similar in nature, 
are harder to deal with in one case than in the other.

Alan Stern