Hi Maciej,
On 1/25/2024 3:10 AM, Maciej Wieczor-Retman wrote:
> The CAT non-contiguous selftests have to read the file responsible for
> reporting support of non-contiguous CBMs in kernel (resctrl). Then the
> test compares if that information matches what is reported by CPUID
> output.
>
> Add a generic helper function to read an unsigned number from a file in
> /sys/fs/resctrl/info/<RESOURCE>/<FILE>.
>
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@xxxxxxxxx>
> ---
> Changelog v3:
> - Rewrite patch message.
> - Add documentation and rewrote the function. (Reinette)
>
> Changelog v2:
> - Add this patch.
>
> tools/testing/selftests/resctrl/resctrl.h | 1 +
> tools/testing/selftests/resctrl/resctrlfs.c | 39 +++++++++++++++++++++
> 2 files changed, 40 insertions(+)
>
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index a1462029998e..5116ea082d03 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -162,6 +162,7 @@ unsigned int count_contiguous_bits(unsigned long val, unsigned int *start);
> int get_full_cbm(const char *cache_type, unsigned long *mask);
> int get_mask_no_shareable(const char *cache_type, unsigned long *mask);
> int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size);
> +int resource_info_unsigned_get(const char *resource, const char *filename, unsigned int *val);
> void ctrlc_handler(int signum, siginfo_t *info, void *ptr);
> int signal_handler_register(void);
> void signal_handler_unregister(void);
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index 5750662cce57..cb5147c5f9a9 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -249,6 +249,45 @@ static int get_bit_mask(const char *filename, unsigned long *mask)
> return 0;
> }
>
> +/*
By not starting with /** the following will not be interpreted as kernel-doc
but the formatting does appear to follow the syntax, but not consistently so.
I think it would be more readable if the kernel-doc syntax is followed consistently.
> + * resource_info_unsigned_get - Read an unsigned value from a file in
> + * /sys/fs/resctrl/info/RESOURCE/FILENAME
"Read an unsigned value from /sys/fs/resctrl/info/RESOURCE/FILENAME"?
> + * @resource: Resource name that matches directory names in
names? (plural)
> + * /sys/fs/resctrl/info
> + * @filename: Filename of a file located in a directory specified with the
> + * 'resource' variable.
I think this can be shortened to "File in /sys/fs/resctrl/info/@resource"
> + * @val: Variable where the read value is saved on success.
"Contains read value on success."
(no need to refer to it as a variable/parameter, it is implied by syntax).
> + *
> + * Return: = 0 on success, < 0 on failure. On success the read value is saved into the 'val'
> + * variable.
"saved into the 'val' variable" -> "saved into @val" (since syntax indicates it is the parameter
there is no need to elaborate).
Also please let lines in comments be of consistent length.
> + */
> +int resource_info_unsigned_get(const char *resource, const char *filename,
> + unsigned int *val)
> +{
> + char reason[128], file_path[PATH_MAX];
> + FILE *fp;
> +
> + snprintf(file_path, sizeof(file_path), "%s/%s/%s", INFO_PATH, resource,
> + filename);
> +
> + fp = fopen(file_path, "r");
> + if (!fp) {
> + snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);
(apart from other discussions). "file" in message seems redundant. It can just be "Error
opening %s". It may also be useful to print file_path instead of filename to be specific
of what the code tried to open.
> + ksft_perror(reason);
> + return -1;
> + }
> +
> + if (fscanf(fp, "%u", val) <= 0) {
> + snprintf(reason, sizeof(reason), "Could not get %s's contents\n", filename);
> + ksft_perror(reason);
filename -> file_path ?
> + fclose(fp);
> + return -1;
> + }
> +
> + fclose(fp);
> + return 0;
> +}
> +
Reinette
