[patches] Re: [PATCH v4 04/17] Add documentation for __riscv_flush_icache

  On Mon, 15 Jan 2018 08:49:05 PST (-0800), joseph@codesourcery.com wrote:
> On Sat, 13 Jan 2018, Palmer Dabbelt wrote:
>
>> +@node RISC-V
>> +@appendixsec RISC-V-specific Facilities
>> +
>> +Cache management facilities specific to RISC-V systems that implement the Linux
>> +ABI are declared in @file{sysdeps/unix/sysv/linux/riscv/sys/cachectl.h}.
>
> This manual is for *users*, which means it should reference the installed
> header sys/cachectl.h, not the location within the glibc source tree.
>
>> +
>> +@deftypefun {void} __riscv_flush_icache(void *start, void *end, unsigned long int flags)
>
> That's @var{start}, @var{end}, @var{flags} in the @deftypefun line.
>
>> +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
>> +Enforce ordering between stores and instruction cache fetches.  The flags
>
> And then again @var{flags} (an argument, not a variable) in the
> description of the function's semantics.
>
>> +variable is used to determine if this applies just to the local thread or to
>> +all threads in the system.
>> +@end deftypefun
>
> "used to determine" how?  You need to say explicitly what the semantics of
> all defined values of that argument are.  You also need to say what the
> semantics of the other arguments are; generally I'd expect documentation
> for a function to refer explicitly to each argument, @var{start},
> @var{end} and @var{flags}, in defining the function semantics in terms of
> those arguments.

Sorry about that, that documentation was terrible.  Hopefully this is a bit 
better.
  

On Mon, 22 Jan 2018, Palmer Dabbelt wrote:

> -@deftypefun {void} __riscv_flush_icache(void *start, void *end, unsigned long
> int flags)
> +@deftypefun {void} __riscv_flush_icache(void *@var{__start}, void
> *@var{__end},
> +					unsigned long int @var{__flags})

No __ on parameter names in the manual, only in the installed header.  And 
you can't split the @deftypefun line like that.  (Texinfo supports a 
syntax with @ at end of line when such an @def* line is split, but I don't 
think glibc's own scripts processing .texi files handle that.)

> @safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
> -Enforce ordering between stores and instruction cache fetches.  The flags
> -variable is used to determine if this applies just to the local thread or to
> -all threads in the system.
> +Enforce ordering between stores and instruction cache fetches.  The range of
> +addresses over which ordering is enforced is specified by @var{__start} and
> +@var{__end}.  The @var{__flags} variable controls how the ordering is
> enforced,

As I said before I think this should be called an argument not a variable.

> +with @code{SYS_RISCV_FLUSH_ICACHE_LOCAL} specifying that the ordering is
> +enforced only with respect to the local instruction stream (as opposed to all
> +instruction streams).

And what other values are valid and what do they mean?  Is 0 the only 
other alternative value, meaning ordering over all cores?  Are other 
values reserved for future use?  You seem to describe only one value, 
SYS_RISCV_FLUSH_ICACHE_LOCAL, without saying anything about any other 
values such as 0.
  

On Mon, 22 Jan 2018 13:58:00 PST (-0800), joseph@codesourcery.com wrote:
> On Mon, 22 Jan 2018, Palmer Dabbelt wrote:
>
>> -@deftypefun {void} __riscv_flush_icache(void *start, void *end, unsigned long
>> int flags)
>> +@deftypefun {void} __riscv_flush_icache(void *@var{__start}, void
>> *@var{__end},
>> +					unsigned long int @var{__flags})
>
> No __ on parameter names in the manual, only in the installed header.  And
> you can't split the @deftypefun line like that.  (Texinfo supports a
> syntax with @ at end of line when such an @def* line is split, but I don't
> think glibc's own scripts processing .texi files handle that.)

Thanks.

>> @safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
>> -Enforce ordering between stores and instruction cache fetches.  The flags
>> -variable is used to determine if this applies just to the local thread or to
>> -all threads in the system.
>> +Enforce ordering between stores and instruction cache fetches.  The range of
>> +addresses over which ordering is enforced is specified by @var{__start} and
>> +@var{__end}.  The @var{__flags} variable controls how the ordering is
>> enforced,
>
> As I said before I think this should be called an argument not a variable.

Sorry, I missed that.

>> +with @code{SYS_RISCV_FLUSH_ICACHE_LOCAL} specifying that the ordering is
>> +enforced only with respect to the local instruction stream (as opposed to all
>> +instruction streams).
>
> And what other values are valid and what do they mean?  Is 0 the only
> other alternative value, meaning ordering over all cores?  Are other
> values reserved for future use?  You seem to describe only one value,
> SYS_RISCV_FLUSH_ICACHE_LOCAL, without saying anything about any other
> values such as 0.

How does this look?

    @node RISC-V
    @appendixsec RISC-V-specific Facilities
    
    Cache management facilities specific to RISC-V systems that implement the Linux
    ABI are declared in @file{sys/cachectl.h}.
    
    @deftypefun {void} __riscv_flush_icache(void *@var{start}, void *@var{end}, unsigned long int @var{flags})
    @safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
    Enforce ordering between stores and instruction cache fetches.  The range of
    addresses over which ordering is enforced is specified by @var{start} and
    @var{end}.  The @var{flags} argument controls the extent of this ordering, with
    the default behavior (a @var{flags} value of 0) being to enforce the fence on
    all threads in the current process.  Setting the
    @code{SYS_RISCV_FLUSH_ICACHE_LOCAL} bit allows users to indicate that enforcing
    ordering on only the current thread is necessary.  All other flag bits are
    reserved.
  

On Mon, 22 Jan 2018, Palmer Dabbelt wrote:

> How does this look?

Seems reasonable.