Re: [PATCH 1/1] Blackfin OTP Char Driver: add writing support of OTP

[Jiri Slaby <jirislaby@gmail.com>]

On 10/13/2008 11:13 AM, Bryan Wu wrote:
> From: Mike Frysinger <vapier.adi@gmail.com>

Some changelog please.

> Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>
> ---
[...]
> @@ -123,18 +132,95 @@ static ssize_t bfin_otp_write(struct file *filp, const char __user *buff, size_t
>  	if (mutex_lock_interruptible(&bfin_otp_lock))
>  		return -ERESTARTSYS;
>  
> -	/* need otp_init() documentation before this can be implemented */
> +	stampit();
> +
> +	timing = bfin_otp_init_timing();
> +	if (timing == 0) {
> +		mutex_unlock(&bfin_otp_lock);
> +		return -EIO;
> +	}
> +
> +	base_flags = OTP_CHECK_FOR_PREV_WRITE;
> +
> +	bytes_done = 0;
> +	page = *pos / (sizeof(u64) * 2);
> +	while (bytes_done < count) {
> +		flags = base_flags | (*pos % (sizeof(u64) * 2) ? OTP_UPPER_HALF : OTP_LOWER_HALF);
> +		stamp("processing page %i (0x%x:%s) from %p", page, flags,
> +			(flags & OTP_UPPER_HALF ? "upper" : "lower"), buff + bytes_done);
> +		if (copy_from_user(&content, buff + bytes_done, sizeof(content))) {
> +			bytes_done = -EFAULT;
> +			break;
> +		}
> +		ret = bfrom_OtpWrite(page, flags, &content);
> +		if (ret & OTP_MASTER_ERROR) {
> +			stamp("error from otp: 0x%x", ret);
> +			bytes_done = -EIO;
> +			break;
> +		}
> +		if (flags & OTP_UPPER_HALF)
> +			++page;
> +		bytes_done += sizeof(content);
> +		*pos += sizeof(content);

What happens to pos if it fails later?

> +	}
> +
> +	bfin_otp_deinit_timing(timing);
>  
>  	mutex_unlock(&bfin_otp_lock);
>  
> +	return bytes_done;
> +}
> +
> +static int bfin_otp_ioctl(struct inode *inode, struct file *filp,
> +                          unsigned cmd, unsigned long arg)
> +{
> +	stampit();
> +
> +	switch (cmd) {
> +	case OTPLOCK: {
> +		u32 timing;
> +		int ret = -EIO;
> +
> +		if (!allow_writes)
> +			return -EACCES;
> +
> +		if (mutex_lock_interruptible(&bfin_otp_lock))
> +			return -ERESTARTSYS;
> +
> +		timing = bfin_otp_init_timing();
> +		if (timing) {
> +			u32 otp_result = bfrom_OtpWrite(arg, OTP_LOCK, NULL);
> +			stamp("locking page %i resulted in 0x%x", arg, otp_result);
> +			if (!(otp_result & OTP_MASTER_ERROR))
> +				ret = 0;
> +
> +			bfin_otp_deinit_timing(timing);
> +		}
> +
> +		mutex_unlock(&bfin_otp_lock);
> +
> +		return ret;
> +	}
> +
> +	case MEMLOCK:
> +		allow_writes = false;
> +		return 0;
> +
> +	case MEMUNLOCK:
> +		allow_writes = true;
> +		return 0;

Please switch to unlocked_ioctl. It should be pretty easy. You should change
(and check) allow_writes under the mutex anyway.

> +	}
> +
>  	return -EINVAL;