Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752652AbcJJXzu (ORCPT ); Mon, 10 Oct 2016 19:55:50 -0400 Received: from mail-lf0-f52.google.com ([209.85.215.52]:33492 "EHLO mail-lf0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752354AbcJJXzt (ORCPT ); Mon, 10 Oct 2016 19:55:49 -0400 MIME-Version: 1.0 In-Reply-To: <1475904515-24970-5-git-send-email-joelaf@google.com> References: <1475904515-24970-1-git-send-email-joelaf@google.com> <1475904515-24970-5-git-send-email-joelaf@google.com> From: Kees Cook Date: Mon, 10 Oct 2016 16:55:46 -0700 X-Google-Sender-Auth: 4goLHXksrqoSaqZSTDud_L3HFgQ Message-ID: Subject: Re: [PATCH 4/7] pstore: Make ramoops_init_przs generic for other prz arrays To: Joel Fernandes Cc: LKML , Steven Rostedt , Anton Vorontsov , Colin Cross , Tony Luck Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5248 Lines: 144 On Fri, Oct 7, 2016 at 10:28 PM, Joel Fernandes wrote: > Currently ramoops_init_przs is hard wired only for panic dump zone array. In > preparation for the ftrace zone array (one zone per-cpu), make the function > more generic to be able to handle this case. > > Add a new ramoops_init_dump_przs function and use the generic function for the > dump prz array. Something similar was suggested in the recent "multiple pmsg" series that was posted too. Making this generic is good, though see my nit below... > > Signed-off-by: Joel Fernandes > --- > fs/pstore/ram.c | 72 ++++++++++++++++++++++++++++++++++++--------------------- > 1 file changed, 46 insertions(+), 26 deletions(-) > > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c > index 519404c..a796f49 100644 > --- a/fs/pstore/ram.c > +++ b/fs/pstore/ram.c > @@ -402,54 +402,74 @@ static void ramoops_free_przs(struct ramoops_context *cxt) > } > > static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt, > - phys_addr_t *paddr, size_t dump_mem_sz) > + struct persistent_ram_zone ***przs, > + phys_addr_t *paddr, size_t mem_sz, > + unsigned int cnt, u32 sig) > { > int err = -ENOMEM; > int i; > + size_t zone_sz; > + struct persistent_ram_zone **prz_ar; > > - if (!cxt->record_size) > - return 0; > - > - if (*paddr + dump_mem_sz - cxt->phys_addr > cxt->size) { > - dev_err(dev, "no room for dumps\n"); > + if (*paddr + mem_sz - cxt->phys_addr > cxt->size) { > + dev_err(dev, "no room for mem region (0x%zx@0x%llx) in (0x%lx@0x%llx)\n", > + mem_sz, (unsigned long long)*paddr, > + cxt->size, (unsigned long long)cxt->phys_addr); > return -ENOMEM; > } > > - cxt->max_dump_cnt = dump_mem_sz / cxt->record_size; > - if (!cxt->max_dump_cnt) > + zone_sz = mem_sz / cnt; > + if (!zone_sz) > return -ENOMEM; > > - cxt->przs = kzalloc(sizeof(*cxt->przs) * cxt->max_dump_cnt, > - GFP_KERNEL); > - if (!cxt->przs) { > - dev_err(dev, "failed to initialize a prz array for dumps\n"); > - goto fail_mem; > + prz_ar = kzalloc(sizeof(**przs) * cnt, GFP_KERNEL); While I realize this is mainly a refactoring, as long as we're in here, can you make this kcalloc instead? > + if (!prz_ar) { > + dev_err(dev, "Failed to initialize prz array\n"); > + return -ENOMEM; > } > > - for (i = 0; i < cxt->max_dump_cnt; i++) { > - cxt->przs[i] = persistent_ram_new(*paddr, cxt->record_size, 0, > + for (i = 0; i < cnt; i++) { > + prz_ar[i] = persistent_ram_new(*paddr, zone_sz, sig, > &cxt->ecc_info, > cxt->memtype); > - if (IS_ERR(cxt->przs[i])) { > - err = PTR_ERR(cxt->przs[i]); > + if (IS_ERR(prz_ar[i])) { > + err = PTR_ERR(prz_ar[i]); > dev_err(dev, "failed to request mem region (0x%zx@0x%llx): %d\n", > cxt->record_size, (unsigned long long)*paddr, err); > > while (i > 0) { > i--; > - persistent_ram_free(cxt->przs[i]); > + persistent_ram_free(prz_ar[i]); > } > - goto fail_prz; > + kfree(prz_ar); > + return err; > } > - *paddr += cxt->record_size; > + *paddr += zone_sz; > } > > + *przs = prz_ar; > return 0; > -fail_prz: > - kfree(cxt->przs); > -fail_mem: > - cxt->max_dump_cnt = 0; > - return err; > +} > + > +static int ramoops_init_dump_przs(struct device *dev, > + struct ramoops_context *cxt, > + phys_addr_t *paddr, size_t mem_sz) > +{ > + int ret; > + > + if (!cxt->record_size) > + return 0; > + > + cxt->max_dump_cnt = mem_sz / cxt->record_size; > + if (!cxt->max_dump_cnt) > + return -ENOMEM; > + > + ret = ramoops_init_przs(dev, cxt, &cxt->przs, paddr, mem_sz, > + cxt->max_dump_cnt, 0); > + if (ret) > + cxt->max_dump_cnt = 0; > + > + return ret; > } > > static int ramoops_init_prz(struct device *dev, struct ramoops_context *cxt, > @@ -601,7 +621,7 @@ static int ramoops_probe(struct platform_device *pdev) > > dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size > - cxt->pmsg_size; > - err = ramoops_init_przs(dev, cxt, &paddr, dump_mem_sz); > + err = ramoops_init_dump_przs(dev, cxt, &paddr, dump_mem_sz); > if (err) > goto fail_out; > > -- > 2.7.4 > -Kees -- Kees Cook Nexus Security