| Message ID | 20120512001840.GJ14782@lizard |
|---|---|
| State | New |
| Headers | show |
On Fri, May 11, 2012 at 5:18 PM, Anton Vorontsov <anton.vorontsov@linaro.org> wrote: > The patch switches pstore RAM backend to use persistent_ram routines, > one step closer to the ECC support. > > Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org> As mentioned, I'm all for this consolidation. That said, some notes below... > --- > fs/pstore/ram.c | 109 ++++++++++++++++++++++++++++++------------------------- > 1 file changed, 60 insertions(+), 49 deletions(-) > > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c > index b26b58e..cf0ad92 100644 > --- a/fs/pstore/ram.c > +++ b/fs/pstore/ram.c > @@ -62,7 +62,7 @@ MODULE_PARM_DESC(dump_oops, > "set to 1 to dump oopses, 0 to only dump panics (default 1)"); > > struct ramoops_context { > - void *virt_addr; > + struct persistent_ram_zone **przs; > phys_addr_t phys_addr; > unsigned long size; > size_t record_size; > @@ -90,39 +90,56 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type, > struct pstore_info *psi) > { > ssize_t size; > - char *rambuf; > struct ramoops_context *cxt = psi->data; > + struct persistent_ram_zone *prz; > > if (cxt->read_count >= cxt->max_count) > return -EINVAL; > + > *id = cxt->read_count++; > + prz = cxt->przs[*id]; > + > /* Only supports dmesg output so far. */ > *type = PSTORE_TYPE_DMESG; > /* TODO(kees): Bogus time for the moment. */ > time->tv_sec = 0; > time->tv_nsec = 0; > > - rambuf = cxt->virt_addr + (*id * cxt->record_size); > - size = strnlen(rambuf, cxt->record_size); > + size = persistent_ram_old_size(prz); > *buf = kmalloc(size, GFP_KERNEL); > if (*buf == NULL) > return -ENOMEM; > - memcpy(*buf, rambuf, size); > + memcpy(*buf, persistent_ram_old(prz), size); > > return size; > } > > +static size_t ramoops_write_kmsg_hdr(struct persistent_ram_zone *prz) > +{ > + char *hdr; > + struct timeval timestamp; > + size_t len; > + > + do_gettimeofday(×tamp); > + hdr = kasprintf(GFP_ATOMIC, RAMOOPS_KERNMSG_HDR "%lu.%lu\n", > + (long)timestamp.tv_sec, (long)timestamp.tv_usec); > + WARN_ON_ONCE(!hdr); > + len = hdr ? strlen(hdr) : 0; > + persistent_ram_write(prz, hdr, len); > + kfree(hdr); > + > + return len; > +} > + > static int ramoops_pstore_write(enum pstore_type_id type, > enum kmsg_dump_reason reason, > u64 *id, > unsigned int part, > size_t size, struct pstore_info *psi) > { > - char *buf; > - size_t res; > - struct timeval timestamp; > struct ramoops_context *cxt = psi->data; > - size_t available = cxt->record_size; > + struct persistent_ram_zone *prz = cxt->przs[cxt->count]; > + size_t hlen; > > /* Currently ramoops is designed to only store dmesg dumps. */ > if (type != PSTORE_TYPE_DMESG) > @@ -147,22 +164,10 @@ static int ramoops_pstore_write(enum pstore_type_id type, > if (part != 1) > return -ENOSPC; > > - buf = cxt->virt_addr + (cxt->count * cxt->record_size); > - > - res = sprintf(buf, "%s", RAMOOPS_KERNMSG_HDR); > - buf += res; > - available -= res; > - > - do_gettimeofday(×tamp); > - res = sprintf(buf, "%lu.%lu\n", (long)timestamp.tv_sec, (long)timestamp.tv_usec); > - buf += res; > - available -= res; > - > - if (size > available) > - size = available; > - > - memcpy(buf, cxt->pstore.buf, size); > - memset(buf + size, '\0', available - size); > + hlen = ramoops_write_kmsg_hdr(prz); > + if (size + hlen > prz->buffer_size) > + size = prz->buffer_size - hlen; > + persistent_ram_write(prz, cxt->pstore.buf, size); > > cxt->count = (cxt->count + 1) % cxt->max_count; > > @@ -172,14 +177,12 @@ static int ramoops_pstore_write(enum pstore_type_id type, > static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, > struct pstore_info *psi) > { > - char *buf; > struct ramoops_context *cxt = psi->data; > > if (id >= cxt->max_count) > return -EINVAL; > > - buf = cxt->virt_addr + (id * cxt->record_size); > - memset(buf, '\0', cxt->record_size); > + persistent_ram_free_old(cxt->przs[id]); Hm, I don't think persistent_ram_free_old() is what's wanted here. That appears to entirely release the region? I want to make sure the memory is cleared first. And will this area come back on a write, or does it stay released? > > return 0; > } > @@ -200,6 +203,7 @@ static int __init ramoops_probe(struct platform_device *pdev) > struct ramoops_platform_data *pdata = pdev->dev.platform_data; > struct ramoops_context *cxt = &oops_cxt; > int err = -EINVAL; > + int i; > > /* Only a single ramoops area allowed at a time, so fail extra > * probes. > @@ -237,32 +241,37 @@ static int __init ramoops_probe(struct platform_device *pdev) > cxt->record_size = pdata->record_size; > cxt->dump_oops = pdata->dump_oops; > > + cxt->przs = kzalloc(sizeof(*cxt->przs) * cxt->max_count, GFP_KERNEL); > + if (!cxt->przs) { > + pr_err("failed to initialize a prz array\n"); > + goto fail_przs; This should be fail_out. > + } > + > + for (i = 0; i < cxt->max_count; i++) { > + size_t sz = cxt->record_size; > + phys_addr_t start = cxt->phys_addr + sz * i; > + > + cxt->przs[i] = persistent_ram_new(start, sz, 0); persistent_ram_new() is marked as __init, so this is unsafe to call if built as a module. I think persistent_ram_new() will need to lose the __init marking, or I'm misunderstanding something. > + if (IS_ERR(cxt->przs[i])) { > + err = PTR_ERR(cxt->przs[i]); > + pr_err("failed to initialize a prz\n"); Since neither persistent_ram_new() nor persistent_ram_buffer_map() report the location of the failure, I'd like to keep the error report (removed below "pr_err("request mem region (0x%lx@0x%llx) failed\n",...") for failures, so there is something actionable in dmesg when the platform data is mismatched for the hardware. > + goto fail_prz; This should be fail_przs. > + } > + } > + > cxt->pstore.data = cxt; > - cxt->pstore.bufsize = cxt->record_size; > - cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL); > spin_lock_init(&cxt->pstore.buf_lock); > + cxt->pstore.bufsize = cxt->przs[0]->buffer_size; > + cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL); I don't see a reason to re-order these (nothing can use buf yet because we haven't registered it with pstore yet). > if (!cxt->pstore.buf) { > pr_err("cannot allocate pstore buffer\n"); > goto fail_clear; > } > > - if (!request_mem_region(cxt->phys_addr, cxt->size, "ramoops")) { > - pr_err("request mem region (0x%lx@0x%llx) failed\n", > - cxt->size, (unsigned long long)cxt->phys_addr); > - err = -EINVAL; > - goto fail_buf; > - } > - > - cxt->virt_addr = ioremap(cxt->phys_addr, cxt->size); > - if (!cxt->virt_addr) { > - pr_err("ioremap failed\n"); > - goto fail_mem_region; > - } > - > err = pstore_register(&cxt->pstore); > if (err) { > pr_err("registering with pstore failed\n"); > - goto fail_iounmap; > + goto fail_pstore; This should be fail_buf. > } > > /* > @@ -280,15 +289,17 @@ static int __init ramoops_probe(struct platform_device *pdev) > > return 0; > > -fail_iounmap: > - iounmap(cxt->virt_addr); > -fail_mem_region: > - release_mem_region(cxt->phys_addr, cxt->size); > -fail_buf: > +fail_pstore: No reason to rename this from "fail_buf". > kfree(cxt->pstore.buf); > fail_clear: > cxt->pstore.bufsize = 0; > cxt->max_count = 0; > +fail_przs: > + for (i = 0; cxt->przs[i]; i++) > + persistent_ram_free(cxt->przs[i]); This can lead to a BUG, since persistent_ram_free() doesn't handle NULL arguments. > + kfree(cxt->przs); > +fail_prz: > + kfree(cxt->pstore.buf); This target (fail_prz) should be removed, and the kfree is redundant to fail_buf above. > fail_out: > return err; > } > -- > 1.7.9.2 > -Kees
Hello Kees, On Mon, May 14, 2012 at 03:21:17PM -0700, Kees Cook wrote: [...] > > - buf = cxt->virt_addr + (id * cxt->record_size); > > - memset(buf, '\0', cxt->record_size); > > + persistent_ram_free_old(cxt->przs[id]); > > Hm, I don't think persistent_ram_free_old() is what's wanted here. > That appears to entirely release the region? I want to make sure the > memory is cleared first. And will this area come back on a write, or > does it stay released? It just releases ECC-restored memory region (a copy). The original (persistent) region is still fully reusable after that call. (It is a pity that pstore internals can't use the restored copy directly, as pstore expects that it will release the region itself after pstore_mkfile(), so we somewhat duplicate the memory during psi->read(). We'd better fix it some day, but it's a minor issue so far.) > > > > return 0; > > } > > @@ -200,6 +203,7 @@ static int __init ramoops_probe(struct platform_device *pdev) > > struct ramoops_platform_data *pdata = pdev->dev.platform_data; > > struct ramoops_context *cxt = &oops_cxt; > > int err = -EINVAL; > > + int i; > > > > /* Only a single ramoops area allowed at a time, so fail extra > > * probes. > > @@ -237,32 +241,37 @@ static int __init ramoops_probe(struct platform_device *pdev) > > cxt->record_size = pdata->record_size; > > cxt->dump_oops = pdata->dump_oops; > > > > + cxt->przs = kzalloc(sizeof(*cxt->przs) * cxt->max_count, GFP_KERNEL); > > + if (!cxt->przs) { > > + pr_err("failed to initialize a prz array\n"); > > + goto fail_przs; > > This should be fail_out. Thanks, will fix all of these error handling negligences. > > + } > > + > > + for (i = 0; i < cxt->max_count; i++) { > > + size_t sz = cxt->record_size; > > + phys_addr_t start = cxt->phys_addr + sz * i; > > + > > + cxt->przs[i] = persistent_ram_new(start, sz, 0); > > persistent_ram_new() is marked as __init, so this is unsafe to call if > built as a module. I think persistent_ram_new() will need to lose the > __init marking, or I'm misunderstanding something. Um. ramoops' probe routine is also __init. persistent_ram_new is a part of ramoops module, so their __init functions will be discarded at the same time. ram_console can't be a module, so it is also fine. So I think it's all fine. > > + if (IS_ERR(cxt->przs[i])) { > > + err = PTR_ERR(cxt->przs[i]); > > + pr_err("failed to initialize a prz\n"); > > Since neither persistent_ram_new() nor persistent_ram_buffer_map() > report the location of the failure, I'd like to keep the error report > (removed below "pr_err("request mem region (0x%lx@0x%llx) > failed\n",...") for failures, so there is something actionable in > dmesg when the platform data is mismatched for the hardware. Sure thing, will do. I'll also start using dev_err() for new code, that way it's more clearer which module reported the error. [...] > > cxt->pstore.data = cxt; > > - cxt->pstore.bufsize = cxt->record_size; > > - cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL); > > spin_lock_init(&cxt->pstore.buf_lock); > > + cxt->pstore.bufsize = cxt->przs[0]->buffer_size; > > + cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL); > > I don't see a reason to re-order these (nothing can use buf yet > because we haven't registered it with pstore yet). Yeah, this is a left over. Thank for catching. [...] > > +fail_przs: > > + for (i = 0; cxt->przs[i]; i++) > > + persistent_ram_free(cxt->przs[i]); > > This can lead to a BUG, since persistent_ram_free() doesn't handle > NULL arguments. The for loop has 'cxt->przs[i]' condition. :-) Thanks for the review!
On Tue, May 15, 2012 at 11:14 PM, Anton Vorontsov <anton.vorontsov@linaro.org> wrote: > Hello Kees, > > On Mon, May 14, 2012 at 03:21:17PM -0700, Kees Cook wrote: > [...] >> > - buf = cxt->virt_addr + (id * cxt->record_size); >> > - memset(buf, '\0', cxt->record_size); >> > + persistent_ram_free_old(cxt->przs[id]); >> >> Hm, I don't think persistent_ram_free_old() is what's wanted here. >> That appears to entirely release the region? I want to make sure the >> memory is cleared first. And will this area come back on a write, or >> does it stay released? > > It just releases ECC-restored memory region (a copy). The original > (persistent) region is still fully reusable after that call. Ah-ha, okay. So this still needs to clear the memory in the "real" copy then. Thanks for the clarification. >> > + } >> > + >> > + for (i = 0; i < cxt->max_count; i++) { >> > + size_t sz = cxt->record_size; >> > + phys_addr_t start = cxt->phys_addr + sz * i; >> > + >> > + cxt->przs[i] = persistent_ram_new(start, sz, 0); >> >> persistent_ram_new() is marked as __init, so this is unsafe to call if >> built as a module. I think persistent_ram_new() will need to lose the >> __init marking, or I'm misunderstanding something. > > Um. ramoops' probe routine is also __init. persistent_ram_new is a > part of ramoops module, so their __init functions will be discarded > at the same time. > > ram_console can't be a module, so it is also fine. > > So I think it's all fine. This is what I get for staring at patches instead of applying them. :) Yeah, if it's all built together, it's no problem. It looked to me like they were in different modules. >> > +fail_przs: >> > + for (i = 0; cxt->przs[i]; i++) >> > + persistent_ram_free(cxt->przs[i]); >> >> This can lead to a BUG, since persistent_ram_free() doesn't handle >> NULL arguments. > > The for loop has 'cxt->przs[i]' condition. :-) Okay, fair enough. :) > Thanks for the review! Sure thing! Thanks for doing this work; I'm excited to have access in ramoops to the new interfaces. :) -Kees
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c index b26b58e..cf0ad92 100644 --- a/fs/pstore/ram.c +++ b/fs/pstore/ram.c @@ -62,7 +62,7 @@ MODULE_PARM_DESC(dump_oops, "set to 1 to dump oopses, 0 to only dump panics (default 1)"); struct ramoops_context { - void *virt_addr; + struct persistent_ram_zone **przs; phys_addr_t phys_addr; unsigned long size; size_t record_size; @@ -90,39 +90,56 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type, struct pstore_info *psi) { ssize_t size; - char *rambuf; struct ramoops_context *cxt = psi->data; + struct persistent_ram_zone *prz; if (cxt->read_count >= cxt->max_count) return -EINVAL; + *id = cxt->read_count++; + prz = cxt->przs[*id]; + /* Only supports dmesg output so far. */ *type = PSTORE_TYPE_DMESG; /* TODO(kees): Bogus time for the moment. */ time->tv_sec = 0; time->tv_nsec = 0; - rambuf = cxt->virt_addr + (*id * cxt->record_size); - size = strnlen(rambuf, cxt->record_size); + size = persistent_ram_old_size(prz); *buf = kmalloc(size, GFP_KERNEL); if (*buf == NULL) return -ENOMEM; - memcpy(*buf, rambuf, size); + memcpy(*buf, persistent_ram_old(prz), size); return size; } +static size_t ramoops_write_kmsg_hdr(struct persistent_ram_zone *prz) +{ + char *hdr; + struct timeval timestamp; + size_t len; + + do_gettimeofday(×tamp); + hdr = kasprintf(GFP_ATOMIC, RAMOOPS_KERNMSG_HDR "%lu.%lu\n", + (long)timestamp.tv_sec, (long)timestamp.tv_usec); + WARN_ON_ONCE(!hdr); + len = hdr ? strlen(hdr) : 0; + persistent_ram_write(prz, hdr, len); + kfree(hdr); + + return len; +} + static int ramoops_pstore_write(enum pstore_type_id type, enum kmsg_dump_reason reason, u64 *id, unsigned int part, size_t size, struct pstore_info *psi) { - char *buf; - size_t res; - struct timeval timestamp; struct ramoops_context *cxt = psi->data; - size_t available = cxt->record_size; + struct persistent_ram_zone *prz = cxt->przs[cxt->count]; + size_t hlen; /* Currently ramoops is designed to only store dmesg dumps. */ if (type != PSTORE_TYPE_DMESG) @@ -147,22 +164,10 @@ static int ramoops_pstore_write(enum pstore_type_id type, if (part != 1) return -ENOSPC; - buf = cxt->virt_addr + (cxt->count * cxt->record_size); - - res = sprintf(buf, "%s", RAMOOPS_KERNMSG_HDR); - buf += res; - available -= res; - - do_gettimeofday(×tamp); - res = sprintf(buf, "%lu.%lu\n", (long)timestamp.tv_sec, (long)timestamp.tv_usec); - buf += res; - available -= res; - - if (size > available) - size = available; - - memcpy(buf, cxt->pstore.buf, size); - memset(buf + size, '\0', available - size); + hlen = ramoops_write_kmsg_hdr(prz); + if (size + hlen > prz->buffer_size) + size = prz->buffer_size - hlen; + persistent_ram_write(prz, cxt->pstore.buf, size); cxt->count = (cxt->count + 1) % cxt->max_count; @@ -172,14 +177,12 @@ static int ramoops_pstore_write(enum pstore_type_id type, static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, struct pstore_info *psi) { - char *buf; struct ramoops_context *cxt = psi->data; if (id >= cxt->max_count) return -EINVAL; - buf = cxt->virt_addr + (id * cxt->record_size); - memset(buf, '\0', cxt->record_size); + persistent_ram_free_old(cxt->przs[id]); return 0; } @@ -200,6 +203,7 @@ static int __init ramoops_probe(struct platform_device *pdev) struct ramoops_platform_data *pdata = pdev->dev.platform_data; struct ramoops_context *cxt = &oops_cxt; int err = -EINVAL; + int i; /* Only a single ramoops area allowed at a time, so fail extra * probes. @@ -237,32 +241,37 @@ static int __init ramoops_probe(struct platform_device *pdev) cxt->record_size = pdata->record_size; cxt->dump_oops = pdata->dump_oops; + cxt->przs = kzalloc(sizeof(*cxt->przs) * cxt->max_count, GFP_KERNEL); + if (!cxt->przs) { + pr_err("failed to initialize a prz array\n"); + goto fail_przs; + } + + for (i = 0; i < cxt->max_count; i++) { + size_t sz = cxt->record_size; + phys_addr_t start = cxt->phys_addr + sz * i; + + cxt->przs[i] = persistent_ram_new(start, sz, 0); + if (IS_ERR(cxt->przs[i])) { + err = PTR_ERR(cxt->przs[i]); + pr_err("failed to initialize a prz\n"); + goto fail_prz; + } + } + cxt->pstore.data = cxt; - cxt->pstore.bufsize = cxt->record_size; - cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL); spin_lock_init(&cxt->pstore.buf_lock); + cxt->pstore.bufsize = cxt->przs[0]->buffer_size; + cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL); if (!cxt->pstore.buf) { pr_err("cannot allocate pstore buffer\n"); goto fail_clear; } - if (!request_mem_region(cxt->phys_addr, cxt->size, "ramoops")) { - pr_err("request mem region (0x%lx@0x%llx) failed\n", - cxt->size, (unsigned long long)cxt->phys_addr); - err = -EINVAL; - goto fail_buf; - } - - cxt->virt_addr = ioremap(cxt->phys_addr, cxt->size); - if (!cxt->virt_addr) { - pr_err("ioremap failed\n"); - goto fail_mem_region; - } - err = pstore_register(&cxt->pstore); if (err) { pr_err("registering with pstore failed\n"); - goto fail_iounmap; + goto fail_pstore; } /* @@ -280,15 +289,17 @@ static int __init ramoops_probe(struct platform_device *pdev) return 0; -fail_iounmap: - iounmap(cxt->virt_addr); -fail_mem_region: - release_mem_region(cxt->phys_addr, cxt->size); -fail_buf: +fail_pstore: kfree(cxt->pstore.buf); fail_clear: cxt->pstore.bufsize = 0; cxt->max_count = 0; +fail_przs: + for (i = 0; cxt->przs[i]; i++) + persistent_ram_free(cxt->przs[i]); + kfree(cxt->przs); +fail_prz: + kfree(cxt->pstore.buf); fail_out: return err; }
The patch switches pstore RAM backend to use persistent_ram routines, one step closer to the ECC support. Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org> --- fs/pstore/ram.c | 109 ++++++++++++++++++++++++++++++------------------------- 1 file changed, 60 insertions(+), 49 deletions(-)