Re: [PATCH v4 1/2] drm/i915/selftests: Prevent userspace mapping invalidation

[Andi Shyti <andi.shyti@kernel.org>]

Hi Krzysztof,

> +		if (!u_pid_nr) {
> +			pr_warn("No current->mm and no PID provided to safely borrow userspace memory from.\n"
> +				"This may lead to switching off tests requiring that for mappings");

This line is too long (please check with checkpatch). The message
is to long. We don't need to provide a full explanation. You can
actually just write something like "No usable userspace mm".

jkk

> +			goto run_tests;
> +		}
> +
> +		u_pid = find_get_pid(u_pid_nr);
> +
> +		if (!u_pid) {
> +			pr_warn("Could not find PID: %d\n", u_pid_nr);
> +			goto run_tests;
> +		}
> +
> +		task = get_pid_task(u_pid, PIDTYPE_PID);
> +		put_pid(u_pid);
> +		if (!task) {
> +			pr_warn("Could not find userspace task for PID: %d\n", u_pid_nr);
> +			goto run_tests;
> +		}
> +
> +		mm = get_task_mm(task);
> +		put_task_struct(task);
> +		if (!mm) {
> +			pr_warn("Could not find address space of task with PID: %d\n", u_pid_nr);
> +			goto run_tests;
> +		}
> +
> +		kthread_use_mm(mm);
> +		mmput_async(mm);
> +		if (unlikely(!current->mm)) {
> +			pr_warn("Could not set mm as current->mm\n");
> +		}

Please remove the two brackets here.

On a general note, I'm not a big fan of the goto's used for
jumping around the code, unless it's an exit point.

Perhaps this can be written as:

	if (!current->mm) {
		mm = a_new_function(...);
		if (mm) {
			kthread_use_mm(mm);
			...
		}
		...
	}

Thanks,
Andi

> +	}
> +