Re: [Intel-xe] [PATCH 1/1] drm/xe: timeout needs to be a signed value

[Andi Shyti <andi.shyti@linux.intel.com>]

Hi Fei,

[...]

> -static unsigned long to_jiffies_timeout(struct drm_xe_wait_user_fence *args)
> +static long to_jiffies_timeout(struct xe_device *xe,
> +			       struct drm_xe_wait_user_fence *args)
>  {
> -	unsigned long timeout;
> +	unsigned long long t;
> +	long timeout;
>  
> -	if (args->flags & DRM_XE_UFENCE_WAIT_ABSTIME)
> -		return drm_timeout_abs_to_jiffies(args->timeout);
> +	/*
> +	 * For negative timeout we want to wait "forever" by setting
> +	 * MAX_SCHEDULE_TIMEOUT. But we have to assign this value also
> +	 * to args->timeout to avoid being zeroed on the signal delivery
> +	 * (see arithmetics after wait).
> +	 */
> +	if (args->timeout < 0) {
> +		args->timeout = MAX_SCHEDULE_TIMEOUT;
> +		return MAX_SCHEDULE_TIMEOUT;
> +	}
>  
> -	if (args->timeout == MAX_SCHEDULE_TIMEOUT || args->timeout == 0)
> -		return args->timeout;
> +	if (args->timeout == 0)
> +		return 0;
>  
> -	timeout = nsecs_to_jiffies(args->timeout);
> +	/*
> +	 * Save the timeout to an u64 variable because nsecs_to_jiffies
> +	 * might return a value that overflows s32 variable.
> +	 */
> +	if (args->flags & DRM_XE_UFENCE_WAIT_ABSTIME)
> +		t = drm_timeout_abs_to_jiffies(args->timeout);
> +	else
> +		t = nsecs_to_jiffies(args->timeout);
> +
> +	/*
> +	 * Anything greater then MAX_SCHEDULE_TIMEOUT is meaningless,
> +	 * also we don't want to cap it at MAX_SCHEDULE_TIMEOUT because
> +	 * apparently user doesn't mean to wait forever, otherwise the
> +	 * args->timeout should have been set to a negative value.
> +	 */
> +	if (t > MAX_SCHEDULE_TIMEOUT)
> +		timeout = MAX_SCHEDULE_TIMEOUT - 1;
> +	else
> +		timeout = t;

you found some boundary case here... thanks!

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> 

Andi