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

[Jiri Slaby <jirislaby@gmail.com>]

On 10/13/2008 11:43 AM, Mike Frysinger wrote:
> On Mon, Oct 13, 2008 at 05:37, Jiri Slaby wrote:
>> On 10/13/2008 11:13 AM, Bryan Wu wrote:
>>> @@ -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".

Sure there is. Next iteration of the loop. I.e. what happens if bfrom_OtpWrite
fails for the second time?

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

Yeah, the assignment/check is.

But is this OK to you:
PROCESS 1			PROCESS 2
lock
  set allow_writes
write
   check allow_writes
   be interrupted
				whatever
				unlock
				    unset allow_writes
				sleep
   mutex lock
   the processing...