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:59, Jiri Slaby wrote:
> 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?

the pos gets updated every time a half-page gets processed.  so if you
call write() and tell it to write 128 bytes, but you get an error half
way through, the pos points right at the place where the error
occurred.  i dont get what you're asking.

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

i dont see a problem here.  there is no loss of data, hardware
failure, software crashes, etc...  in other words, there is no
misbehavior.
-mike