On Thu, Jul 02, 2026 at 10:32:49AM -0700, Stanislav Kinsburskii wrote:
> + int driver_populate_range_unlockable(...)
> + {
> + struct hmm_range range;
> + int locked;
> + ...
> +
> + range.notifier = &interval_sub;
> + range.start = ...;
> + range.end = ...;
> + range.hmm_pfns = ...;
> +
> + if (!mmget_not_zero(interval_sub.mm))
> + return -EFAULT;
> +
> + again:
> + range.notifier_seq = mmu_interval_read_begin(&interval_sub);
> + locked = 1;
> + mmap_read_lock(mm);
> + ret = hmm_range_fault_unlockable(&range, &locked);
> + if (locked)
> + mmap_read_unlock(mm);
> + if (ret) {
> + if (ret == -EBUSY)
> + goto again;
> + return ret;
> + }
> + if (!locked)
> + goto again;
Why does this sequence make any sense? I think the caller holding the
mmap_read_lock() is some historical artifact, shouldn't it be pushed
down into the hmm function itself? Looking at the callers it seems
like almost all of them want that.
I can't guess why drm_gpusvm_range_find_or_insert() is mucking around
with VMAs, a hmm user should never do that. I think I gave this
feedback many times.
Also, why the goto again? If it needs to loop again it should return
EBUSY.
> @@ -715,7 +804,7 @@ int hmm_range_fault(struct hmm_range *range)
> } while (ret == -EBUSY);
> return ret;
> }
> -EXPORT_SYMBOL(hmm_range_fault);
> +EXPORT_SYMBOL(hmm_range_fault_unlockable);
Don't export new symbols without adding users please.
I suggest you add a hmm_range_fault_unlocked() instead and convert the
places that can obviously use it to use it.
Then maybe come with the unlockable version with an intree user and a
strong justification why something needs to hold the mmap lock
externally.
Jason
