Messages in this thread | | | Date | Mon, 13 Oct 2008 11:59:21 +0200 | From | Jiri Slaby <> | Subject | Re: [PATCH 1/1] Blackfin OTP Char Driver: add writing support of OTP |
| |
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...
| |