This patch implements the documentation of event_enable_write and
event_enable_read in the form of testable function's expectations.

Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
---
 kernel/trace/trace_events.c | 72 +++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 5e84ef01d0c8..eb3c5e6e557d 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -1771,6 +1771,46 @@ static void p_stop(struct seq_file *m, void *p)
 	mutex_unlock(&event_mutex);
 }
 
+/**
+ * event_enable_read - read from a trace event file to retrieve its status.
+ * @filp: file pointer associated with the target trace event;
+ * @ubuf: user space buffer where the event status is copied to;
+ * @cnt: number of bytes to be copied to the user space buffer;
+ * @ppos: the current position in the buffer.
+ *
+ * This is a way for user space executables to retrieve the status of a
+ * specific event
+ *
+ * Function's expectations:
+ * - This function shall lock the global event_mutex before performing any
+ *   operation on the target event file and unlock it after all operations on
+ *   the target event file have completed;
+ * - This function shall retrieve the status flags from the file associated
+ *   with the target event;
+ * - This function shall format the string to report the event status to user
+ *   space:
+ *   - The first character of the string to be copied to user space shall be
+ *     set to "1" if the enabled flag is set AND the soft_disabled flag is not
+ *     set, else it shall be set to "0";
+ *   - The second character of the string to be copied to user space shall be
+ *     set to "*" if either the soft_disabled flag or the soft_mode flag is
+ *     set, else it shall be set to "\n";
+ *   - The third character of the string to be copied to user space shall b
+ *     set to "\n" if either the soft_disabled flag or the soft_mode flag is
+ *     set, else it shall be set to "0";
+ *   - Any other character in the string to be copied to user space shall be
+ *     set to "0";
+ * - This function shall check if the requested cnt bytes are equal or greater
+ *   than the length of the string to be copied to user space (else a
+ *   corrupted event status could be reported);
+ * - This function shall invoke simple_read_from_buffer() to perform the copy
+ *   of the kernel space string to ubuf.
+ *
+ * Returns the number of copied bytes on success, -ENODEV if the event file
+ * cannot be retrieved from the input filp, -EINVAL if cnt is less than the
+ * length of the string to be copied to ubuf, any error returned by
+ * simple_read_from_buffer
+ */
 static ssize_t
 event_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
 		  loff_t *ppos)
@@ -1808,6 +1848,38 @@ event_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
 	return simple_read_from_buffer(ubuf, cnt, ppos, buf, strlen(buf));
 }
 
+/**
+ * event_enable_write - write to a trace event file to enable/disable it.
+ * @filp: file pointer associated with the target trace event;
+ * @ubuf: user space buffer where the enable/disable value is copied from;
+ * @cnt: number of bytes to be copied from the user space buffer;
+ * @ppos: the current position in the buffer.
+ *
+ * This is a way for user space executables to enable or disable event
+ * recording.
+ *
+ * Function's expectations:
+ * - This function shall copy cnt bytes from the input ubuf buffer to a kernel
+ *   space buffer (or the whole input ubuf if cnt is larger than ubuf size)
+ *   and shall convert the string within the kernel space buffer into a decimal
+ *   base format number;
+ * - This function shall lock the global event_mutex before performing any
+ *   operation on the target event file and unlock it after all operations on
+ *   the target event file have completed;
+ * - This function shall check the size of the per-cpu ring-buffers used for
+ *   the event trace data and, if smaller than TRACE_BUF_SIZE_DEFAULT, expand
+ *   them to TRACE_BUF_SIZE_DEFAULT bytes (sizes larger than
+ *   TRACE_BUF_SIZE_DEFAULT are not allowed);
+ * - This function shall invoke ftrace_event_enable_disable to enable or
+ *   disable the target trace event according to the value read from user space
+ *   (0 - disable, 1 - enable);
+ *
+ * Returns 0 on success, any error returned by kstrtoul_from_user, -ENODEV if
+ * the event file cannot be retrieved from the input filp, any error returned by
+ * tracing_update_buffers, any error returned by ftrace_event_enable_disable,
+ * -EINVAL if the value copied from the user space ubuf is different from 0 or
+ *  1.
+ */
 static ssize_t
 event_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
 		   loff_t *ppos)
-- 
2.48.1

On Thu, 12 Jun 2025 12:43:49 +0200
Gabriele Paoloni <gpaoloni@redhat.com> wrote:

> This patch implements the documentation of event_enable_write and
> event_enable_read in the form of testable function's expectations.
> 
> Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
> ---
>  kernel/trace/trace_events.c | 72 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 5e84ef01d0c8..eb3c5e6e557d 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -1771,6 +1771,46 @@ static void p_stop(struct seq_file *m, void *p)
>  	mutex_unlock(&event_mutex);
>  }
>  
> +/**
> + * event_enable_read - read from a trace event file to retrieve its status.
> + * @filp: file pointer associated with the target trace event;
> + * @ubuf: user space buffer where the event status is copied to;
> + * @cnt: number of bytes to be copied to the user space buffer;

Why is the above ending with semicolons?

> + * @ppos: the current position in the buffer.
> + *
> + * This is a way for user space executables to retrieve the status of a
> + * specific event
> + *
> + * Function's expectations:
> + * - This function shall lock the global event_mutex before performing any
> + *   operation on the target event file and unlock it after all operations on
> + *   the target event file have completed;
> + * - This function shall retrieve the status flags from the file associated
> + *   with the target event;
> + * - This function shall format the string to report the event status to user
> + *   space:
> + *   - The first character of the string to be copied to user space shall be
> + *     set to "1" if the enabled flag is set AND the soft_disabled flag is not
> + *     set, else it shall be set to "0";
> + *   - The second character of the string to be copied to user space shall be
> + *     set to "*" if either the soft_disabled flag or the soft_mode flag is
> + *     set, else it shall be set to "\n";
> + *   - The third character of the string to be copied to user space shall b
> + *     set to "\n" if either the soft_disabled flag or the soft_mode flag is
> + *     set, else it shall be set to "0";
> + *   - Any other character in the string to be copied to user space shall be
> + *     set to "0";

The above could use some new lines. Like between each bullet. My
eyesight isn't as good anymore and I find reading gobs of text more
difficult. Newlines give my eyes a break.

I know this is being written so that a tool could parse it, but also
needs to be parsed by aging developers. ;-)

> + * - This function shall check if the requested cnt bytes are equal or greater
> + *   than the length of the string to be copied to user space (else a
> + *   corrupted event status could be reported);
> + * - This function shall invoke simple_read_from_buffer() to perform the copy
> + *   of the kernel space string to ubuf.

Hmm, and it is also verbose. This is a relatively simple function, and
it caused quite a bit of requirements. I wonder if it could be
shortened a bit. Otherwise more complex and larger functions are going
to be overwhelming and likely not acceptable. And those are the
functions that I think this will benefit the most!

> + *
> + * Returns the number of copied bytes on success, -ENODEV if the event file
> + * cannot be retrieved from the input filp, -EINVAL if cnt is less than the
> + * length of the string to be copied to ubuf, any error returned by
> + * simple_read_from_buffer

Returns should be formatted better where each return value is on a
separate line.

-- Steve

> + */
>  static ssize_t
>  event_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
>  		  loff_t *ppos)
> @@ -1808,6 +1848,38 @@ event_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
>  	return simple_read_from_buffer(ubuf, cnt, ppos, buf, strlen(buf));
>  }
>  
> +/**
> + * event_enable_write - write to a trace event file to enable/disable it.
> + * @filp: file pointer associated with the target trace event;
> + * @ubuf: user space buffer where the enable/disable value is copied from;
> + * @cnt: number of bytes to be copied from the user space buffer;
> + * @ppos: the current position in the buffer.
> + *
> + * This is a way for user space executables to enable or disable event
> + * recording.
> + *
> + * Function's expectations:
> + * - This function shall copy cnt bytes from the input ubuf buffer to a kernel
> + *   space buffer (or the whole input ubuf if cnt is larger than ubuf size)
> + *   and shall convert the string within the kernel space buffer into a decimal
> + *   base format number;
> + * - This function shall lock the global event_mutex before performing any
> + *   operation on the target event file and unlock it after all operations on
> + *   the target event file have completed;
> + * - This function shall check the size of the per-cpu ring-buffers used for
> + *   the event trace data and, if smaller than TRACE_BUF_SIZE_DEFAULT, expand
> + *   them to TRACE_BUF_SIZE_DEFAULT bytes (sizes larger than
> + *   TRACE_BUF_SIZE_DEFAULT are not allowed);
> + * - This function shall invoke ftrace_event_enable_disable to enable or
> + *   disable the target trace event according to the value read from user space
> + *   (0 - disable, 1 - enable);
> + *
> + * Returns 0 on success, any error returned by kstrtoul_from_user, -ENODEV if
> + * the event file cannot be retrieved from the input filp, any error returned by
> + * tracing_update_buffers, any error returned by ftrace_event_enable_disable,
> + * -EINVAL if the value copied from the user space ubuf is different from 0 or
> + *  1.
> + */
>  static ssize_t
>  event_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
>  		   loff_t *ppos)

On Wed, Jul 2, 2025 at 12:11 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu, 12 Jun 2025 12:43:49 +0200
> Gabriele Paoloni <gpaoloni@redhat.com> wrote:
>
> > This patch implements the documentation of event_enable_write and
> > event_enable_read in the form of testable function's expectations.
> >
> > Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
> > ---
> >  kernel/trace/trace_events.c | 72 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 72 insertions(+)
> >
> > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> > index 5e84ef01d0c8..eb3c5e6e557d 100644
> > --- a/kernel/trace/trace_events.c
> > +++ b/kernel/trace/trace_events.c
> > @@ -1771,6 +1771,46 @@ static void p_stop(struct seq_file *m, void *p)
> >       mutex_unlock(&event_mutex);
> >  }
> >
> > +/**
> > + * event_enable_read - read from a trace event file to retrieve its status.
> > + * @filp: file pointer associated with the target trace event;
> > + * @ubuf: user space buffer where the event status is copied to;
> > + * @cnt: number of bytes to be copied to the user space buffer;
>
> Why is the above ending with semicolons?

Sorry it should be a full stop, I'll fix it in v4.

>
> > + * @ppos: the current position in the buffer.
> > + *
> > + * This is a way for user space executables to retrieve the status of a
> > + * specific event
> > + *
> > + * Function's expectations:
> > + * - This function shall lock the global event_mutex before performing any
> > + *   operation on the target event file and unlock it after all operations on
> > + *   the target event file have completed;
> > + * - This function shall retrieve the status flags from the file associated
> > + *   with the target event;
> > + * - This function shall format the string to report the event status to user
> > + *   space:
> > + *   - The first character of the string to be copied to user space shall be
> > + *     set to "1" if the enabled flag is set AND the soft_disabled flag is not
> > + *     set, else it shall be set to "0";
> > + *   - The second character of the string to be copied to user space shall be
> > + *     set to "*" if either the soft_disabled flag or the soft_mode flag is
> > + *     set, else it shall be set to "\n";
> > + *   - The third character of the string to be copied to user space shall b
> > + *     set to "\n" if either the soft_disabled flag or the soft_mode flag is
> > + *     set, else it shall be set to "0";
> > + *   - Any other character in the string to be copied to user space shall be
> > + *     set to "0";
>
> The above could use some new lines. Like between each bullet. My
> eyesight isn't as good anymore and I find reading gobs of text more
> difficult. Newlines give my eyes a break.
>
> I know this is being written so that a tool could parse it, but also
> needs to be parsed by aging developers. ;-)

Agreed, I'll fix it in v4.

>
> > + * - This function shall check if the requested cnt bytes are equal or greater
> > + *   than the length of the string to be copied to user space (else a
> > + *   corrupted event status could be reported);
> > + * - This function shall invoke simple_read_from_buffer() to perform the copy
> > + *   of the kernel space string to ubuf.
>
> Hmm, and it is also verbose. This is a relatively simple function, and
> it caused quite a bit of requirements. I wonder if it could be
> shortened a bit. Otherwise more complex and larger functions are going
> to be overwhelming and likely not acceptable. And those are the
> functions that I think this will benefit the most!

Mmm got it. What about

* Function's expectations:
* - This function shall lock the global event_mutex before performing any
*   operation on the target event file and unlock it after all operations on
*   the target event file have completed;
*
* - This function shall format the string copied to userspace according to
*   the status flags retrieved from the target event file:
*    - The first character shall be set to "1" if the enabled flag is
set AND the
*      soft_disabled flag is not set, else it shall be set to "0";
*    - The second character is optional and shall be set to "*" if either the
*      soft_disabled flag or the soft_mode flag is set;
*   - The string shall be terminated by a newline ("\n") and any remaining
*      character shall be set to "0";
*
* - This function shall invoke simple_read_from_buffer() to perform the copy
*   of the kernel space string to ubuf.

(pls note that the check on cnt has been removed in v3 that is out already)

>
> > + *
> > + * Returns the number of copied bytes on success, -ENODEV if the event file
> > + * cannot be retrieved from the input filp, -EINVAL if cnt is less than the
> > + * length of the string to be copied to ubuf, any error returned by
> > + * simple_read_from_buffer
>
> Returns should be formatted better where each return value is on a
> separate line.

Understood, I'll fix this in v4

Many Thanks!
Gab


>
> -- Steve
>
> > + */
> >  static ssize_t
> >  event_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
> >                 loff_t *ppos)
> > @@ -1808,6 +1848,38 @@ event_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
> >       return simple_read_from_buffer(ubuf, cnt, ppos, buf, strlen(buf));
> >  }
> >
> > +/**
> > + * event_enable_write - write to a trace event file to enable/disable it.
> > + * @filp: file pointer associated with the target trace event;
> > + * @ubuf: user space buffer where the enable/disable value is copied from;
> > + * @cnt: number of bytes to be copied from the user space buffer;
> > + * @ppos: the current position in the buffer.
> > + *
> > + * This is a way for user space executables to enable or disable event
> > + * recording.
> > + *
> > + * Function's expectations:
> > + * - This function shall copy cnt bytes from the input ubuf buffer to a kernel
> > + *   space buffer (or the whole input ubuf if cnt is larger than ubuf size)
> > + *   and shall convert the string within the kernel space buffer into a decimal
> > + *   base format number;
> > + * - This function shall lock the global event_mutex before performing any
> > + *   operation on the target event file and unlock it after all operations on
> > + *   the target event file have completed;
> > + * - This function shall check the size of the per-cpu ring-buffers used for
> > + *   the event trace data and, if smaller than TRACE_BUF_SIZE_DEFAULT, expand
> > + *   them to TRACE_BUF_SIZE_DEFAULT bytes (sizes larger than
> > + *   TRACE_BUF_SIZE_DEFAULT are not allowed);
> > + * - This function shall invoke ftrace_event_enable_disable to enable or
> > + *   disable the target trace event according to the value read from user space
> > + *   (0 - disable, 1 - enable);
> > + *
> > + * Returns 0 on success, any error returned by kstrtoul_from_user, -ENODEV if
> > + * the event file cannot be retrieved from the input filp, any error returned by
> > + * tracing_update_buffers, any error returned by ftrace_event_enable_disable,
> > + * -EINVAL if the value copied from the user space ubuf is different from 0 or
> > + *  1.
> > + */
> >  static ssize_t
> >  event_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
> >                  loff_t *ppos)
>

On Wed, 2 Jul 2025 16:59:29 +0200
Gabriele Paoloni <gpaoloni@redhat.com> wrote:

> Mmm got it. What about
> 
> * Function's expectations:
> * - This function shall lock the global event_mutex before performing any
> *   operation on the target event file and unlock it after all operations on
> *   the target event file have completed;

Since 99% of the time that a lock is taken in a function it is
released, I think that should be the default assumption here, and only
when a lock is taken and not release, that should be explicitly called
out.

And also we should remove "This function" we know that these
requirements are for this function.

  - The global event_mutex shall be taken before performing any
    operation on the target event.

Should be good enough.

If the lock can be released and taken again, that too should be
explicit in the requirements otherwise it is assumed it is taken once
and not released until the operation is completed.

> *
> * - This function shall format the string copied to userspace according to
> *   the status flags retrieved from the target event file:
> *    - The first character shall be set to "1" if the enabled flag is
> set AND the
> *      soft_disabled flag is not set, else it shall be set to "0";
> *    - The second character is optional and shall be set to "*" if either the
> *      soft_disabled flag or the soft_mode flag is set;
> *   - The string shall be terminated by a newline ("\n") and any remaining
> *      character shall be set to "0";

 - The string copied to user space shall be formatted according to the
   status flags from the target event file:

   - If the enable flag is set AND the soft_disable flag is not set then
     the first character shall be set to "1" ELSE it shall be set to "0"

   - If either the soft_disable fag or the soft_mode flag is set then the
     second character shall be set to "*" ELSE it is skipped.

I think the above is easier to read and is a bit more consolidated.
Stating the status then the effect is also easier to read.

-- Steve


> *
> * - This function shall invoke simple_read_from_buffer() to perform the copy
> *   of the kernel space string to ubuf.
> 
> (pls note that the check on cnt has been removed in v3 that is out already)

On Wed, Jul 2, 2025 at 5:12 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 2 Jul 2025 16:59:29 +0200
> Gabriele Paoloni <gpaoloni@redhat.com> wrote:
>
> > Mmm got it. What about
> >
> > * Function's expectations:
> > * - This function shall lock the global event_mutex before performing any
> > *   operation on the target event file and unlock it after all operations on
> > *   the target event file have completed;
>
> Since 99% of the time that a lock is taken in a function it is
> released, I think that should be the default assumption here, and only
> when a lock is taken and not release, that should be explicitly called
> out.
>
> And also we should remove "This function" we know that these
> requirements are for this function.
>
>   - The global event_mutex shall be taken before performing any
>     operation on the target event.
>
> Should be good enough.
>
> If the lock can be released and taken again, that too should be
> explicit in the requirements otherwise it is assumed it is taken once
> and not released until the operation is completed.



>
> > *
> > * - This function shall format the string copied to userspace according to
> > *   the status flags retrieved from the target event file:
> > *    - The first character shall be set to "1" if the enabled flag is
> > set AND the
> > *      soft_disabled flag is not set, else it shall be set to "0";
> > *    - The second character is optional and shall be set to "*" if either the
> > *      soft_disabled flag or the soft_mode flag is set;
> > *   - The string shall be terminated by a newline ("\n") and any remaining
> > *      character shall be set to "0";
>
>  - The string copied to user space shall be formatted according to the
>    status flags from the target event file:
>
>    - If the enable flag is set AND the soft_disable flag is not set then
>      the first character shall be set to "1" ELSE it shall be set to "0"
>
>    - If either the soft_disable fag or the soft_mode flag is set then the
>      second character shall be set to "*" ELSE it is skipped.
>
> I think the above is easier to read and is a bit more consolidated.
> Stating the status then the effect is also easier to read.

I will add all your suggestions in v4.
Many thanks for your review!
Gab

>
> -- Steve
>
>
> > *
> > * - This function shall invoke simple_read_from_buffer() to perform the copy
> > *   of the kernel space string to ubuf.
> >
> > (pls note that the check on cnt has been removed in v3 that is out already)
>