Re: [PATCH v5 01/29] asm-generic: add generic futex for !CONFIG_SMP

[Thomas Gleixner]

B1;2802;0cOn Fri, 24 Oct 2014, Ley Foon Tan wrote:
> +#ifndef CONFIG_SMP
> +static inline int
> +futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)

If we add this to the generic code we want to have a proper docbook
comment describing the function.

> +{
> +	int op = (encoded_op >> 28) & 7;
> +	int cmp = (encoded_op >> 24) & 15;
> +	int oparg = (encoded_op << 8) >> 20;
> +	int cmparg = (encoded_op << 20) >> 20;
> +	int oldval, ret;
> +	u32 tmp;
> +
> +	if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
> +		oparg = 1 << oparg;
> +
> +	pagefault_disable();	/* implies preempt_disable() */

Now this mindlessly copied comment does not have any real value.

1) That pagefault_disable() implies preempt_disable() is an
   implementation detail which is not guaranteed to be true forever.

2) It's not telling at all, that this code really relies on both
   pagefault AND preemption being disabled.

> +
> +static inline int
> +futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
> +			      u32 oldval, u32 newval)
> +{

Docbook comment missing as well. But what's worse is, that it does not
explain what the calling convention for this function is.

As above it requires both pagefault AND preemption being disabled. The
fact that both are the same are again just an implementation detail of
todays code ....

> +	u32 val;
> +
> +	if (unlikely(get_user(val, uaddr) != 0))
> +		return -EFAULT;
> +
> +	if (val == oldval && unlikely(put_user(newval, uaddr) != 0))
> +		return -EFAULT;
> +
> +	*uval = val;
> +
> +	return 0;
> +}

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/