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

["Mike Frysinger" <vapier.adi@gmail.com>]

On Mon, Oct 13, 2008 at 05:37, Jiri Slaby wrote:
> On 10/13/2008 11:13 AM, Bryan Wu wrote:
>> From: Mike Frysinger <vapier.adi@gmail.com>
>
> Some changelog please.

i implemented write support.  like the subject says ... not sure what
else to say.

>> @@ -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?

there is no state maintained in the hardware.  the pos gets updated
only when a half-page actually gets processed.  so there is no
"later".

>> +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.

will do, thanks

> You should change (and check) allow_writes under the mutex anyway.

not really.  the mutex is to restrict access to the OTP hardware, not
driver state.  because there is none.  access to allow_writes is
atomic in the hardware anyways.

thanks for the review
-mike