Re: [igt-dev] [PATCH i-g-t 6/8] igt/params: use igt_params_set_save for igt_set_module_param*

Juha-Pekka Heikkila
2020-04-28T19:04:21Z

On 28.4.2020 15.54, Petri Latvala wrote:
> On Tue, Apr 21, 2020 at 07:17:23PM +0300, Juha-Pekka Heikkila wrote:
>> Unify parameter access.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>> ---
>> lib/igt_params.c | 133 +++++++++++++++++++++++++++++++++++------------
>> 1 file changed, 100 insertions(+), 33 deletions(-)
>>
>> diff --git a/lib/igt_params.c b/lib/igt_params.c
>> index 8de47b02..369d224c 100644
>> --- a/lib/igt_params.c
>> +++ b/lib/igt_params.c
>> @@ -27,12 +27,14 @@
>> #include <stdbool.h>
>> #include <stdio.h>
>> #include <sys/ioctl.h>
>> +#include <sys/stat.h>
>>
>> #include <i915_drm.h>
>>
>> #include "igt_core.h"
>> #include "igt_params.h"
>> #include "igt_sysfs.h"
>> +#include "igt_debugfs.h"
>>
>> struct module_param_data {
>> char *path;
>> @@ -107,15 +109,59 @@ static void igt_params_save(int dir, const char *path, const char *name)
>> module_params = data;
>> }
>>
>> -static int __igt_params_open(int device, char **outpath)
>> +static int __igt_params_open(int device, char **outpath, const char *param)
>> {
>> int dir, params = -1;
>> + struct stat buffer;
>> + char searchname[64];
>> + char searchpath[PATH_MAX];
>> + char *foundname, *ctx;
>>
>> - dir = igt_sysfs_open(device);
>> + dir = igt_debugfs_dir(device);
>> if (dir >= 0) {
>> - params = openat(dir,
>> - "device/driver/module/parameters",
>> - O_RDONLY);
>> + int devname;
>> +
>> + devname = openat(dir, "name", O_RDONLY);
>> + igt_assert(devname >= 0);
>> +
>> + read(devname, searchname, sizeof(searchname));
>> + close(devname);
>> +
>> + foundname = strtok_r(searchname, " ", &ctx);
>> + igt_assert(foundname);
>> +
>> + snprintf(searchpath, PATH_MAX, "%s_params", foundname);
>> + params = openat(dir, searchpath, O_RDONLY);
>> +
>> + if (params >= 0) {
>> + char *debugfspath = malloc(PATH_MAX);
>> +
>> + igt_debugfs_path(device, debugfspath, PATH_MAX);
>> + if (param != NULL) {
>> + char filepath[PATH_MAX];
>> +
>> + snprintf(filepath, PATH_MAX, "%s/%s",
>> + debugfspath, param);
>> +
>> + if (stat(filepath, &buffer) == 0) {
>> + if (outpath != NULL)
>> + *outpath = debugfspath;
>> + else
>> + free(debugfspath);
>> + } else {
>> + free(debugfspath);
>> + close(params);
>> + params = -1;
>> + }
>> + } else if (outpath != NULL) {
>> + /*
>> + * Caller is responsible to free this.
>> + */
>> + *outpath = debugfspath;
>> + } else {
>> + free(debugfspath);
>> + }
>> + }
>> close(dir);
>> }
>>
>> @@ -124,11 +170,51 @@ static int __igt_params_open(int device, char **outpath)
>> char name[32] = "";
>> char path[PATH_MAX];
>>
>> - memset(&version, 0, sizeof(version));
>> - version.name_len = sizeof(name);
>> - version.name = name;
>> - ioctl(device, DRM_IOCTL_VERSION, &version);
>> -
>> + if (device == -1) {
>> + /*
>> + * find default device
>> + */
>
> Oh, here's the defaulting.
>
> Like stated in the replies for one of the other patches, I'm not
> greatly fond of carrying around a concept of a "default" device. Are
> there users of -1 today?

There was handful of those, kms_busy comes first to my mind since I was
looking at it today. Agreed this defaulting doesn't sound good idea but
I'd first put plumbing in place and once there's no users scrap it.
There's for example kms_force_connector_basic@force-load-detect test
which I cannot now go test how it go if change this behavior since it
says igt_require(connector->connector_type == DRM_MODE_CONNECTOR_VGA)

>
> This is also considerably more code that one would expect from just
> "use params_set_save" as the subject states.
>
>
>
>
>> + int file, i;
>> + const char *debugfs_root = igt_debugfs_mount();
>> +
>> + igt_assert(debugfs_root);
>> +
>> + for (i = 0; i < 63; i++) {
>> + char testpath[PATH_MAX];
>> +
>> + snprintf(searchpath, PATH_MAX,
>> + "%s/dri/%d/name", debugfs_root, i);
>> +
>> + file = open(searchpath, O_RDONLY);
>> +
>> + if (file < 0)
>> + continue;
>> +
>> + read(file, searchname, sizeof(searchname));
>> + close(file);
>> +
>> + foundname = strtok_r(searchname, " ", &ctx);
>> + if (!foundname)
>> + continue;
>> +
>> + snprintf(testpath, PATH_MAX,
>> + "/sys/module/%s/parameters",
>> + foundname);
>> +
>> + if (stat(testpath, &buffer) == 0 &&
>> + S_ISDIR(buffer.st_mode)) {
>> + memcpy(name, foundname,
>> + (sizeof(name) < strlen(foundname) ?
>> + sizeof(name) : strlen(foundname)));
>
> If foundname is larger than the array, doesn't this leave 'name'
> without a nul-termination?

ah, I got caught by the famous buffer overflow thingy. I'll fix.

Thanks for the comments.