From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vladimir Zapolskiy Subject: Re: [PATCHv8 1/4] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support Date: Fri, 22 Jan 2016 08:02:06 +0200 Message-ID: <56A1C5DE.1080501@mentor.com> References: <1453397668-32094-1-git-send-email-tthayer@opensource.altera.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1453397668-32094-1-git-send-email-tthayer@opensource.altera.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: tthayer@opensource.altera.com, bp@alien8.de, dougthompson@xmission.com, m.chehab@samsung.com, robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, linux@arm.linux.org.uk, dinguyen@opensource.altera.com, grant.likely@linaro.org Cc: devicetree@vger.kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, tthayer.linux@gmail.com, linux-arm-kernel@lists.infradead.org, linux-edac@vger.kernel.org List-Id: devicetree@vger.kernel.org Hi Thor, On 21.01.2016 19:34, tthayer@opensource.altera.com wrote: > From: Thor Thayer > > Adding L2 Cache and On-Chip RAM EDAC support for the > Altera SoCs using the EDAC device model. The SDRAM > controller is using the Memory Controller model. > > Each type of ECC is individually configurable. > > Signed-off-by: Thor Thayer > Signed-off-by: Dinh Nguyen You are sending a change authored by yourself for review, but you add Dinh's SoB, what's his role here? See Documentation/SubmittingPatches "Sign your work". [snip] > +/* > + * altr_edac_device_probe() > + * This is a generic EDAC device driver that will support > + * various Altera memory devices such as the L2 cache ECC and > + * OCRAM ECC as well as the memories for other peripherals. > + * Module specific initialization is done by passing the > + * function index in the device tree. > + */ > +static int altr_edac_device_probe(struct platform_device *pdev) > +{ > + struct edac_device_ctl_info *dci; > + struct altr_edac_device_dev *drvdata; > + struct resource *r; > + int res = 0; > + struct device_node *np = pdev->dev.of_node; > + char *ecc_name = (char *)np->name; > + static int dev_instance; > + struct dentry *debugfs; > + > + if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) { > + edac_printk(KERN_ERR, EDAC_DEVICE, > + "Unable to open devm\n"); > + return -ENOMEM; > + } > + > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!r) { > + edac_printk(KERN_ERR, EDAC_DEVICE, > + "Unable to get mem resource\n"); Missing devres_release_group(&pdev->dev, NULL) on error path. > + return -ENODEV; > + } > + > + if (!devm_request_mem_region(&pdev->dev, r->start, resource_size(r), > + dev_name(&pdev->dev))) { > + edac_printk(KERN_ERR, EDAC_DEVICE, > + "%s:Error requesting mem region\n", ecc_name); See above. > + return -EBUSY; > + } > + > + dci = edac_device_alloc_ctl_info(sizeof(*drvdata), ecc_name, > + 1, ecc_name, 1, 0, NULL, 0, > + dev_instance++); > + > + if (!dci) { > + edac_printk(KERN_ERR, EDAC_DEVICE, > + "%s: Unable to allocate EDAC device\n", ecc_name); See above. > + return -ENOMEM; > + } > + > + drvdata = dci->pvt_info; > + dci->dev = &pdev->dev; > + platform_set_drvdata(pdev, dci); > + drvdata->edac_dev_name = ecc_name; > + > + drvdata->base = devm_ioremap(&pdev->dev, r->start, resource_size(r)); > + if (!drvdata->base) > + goto err; > + > + /* Get driver specific data for this EDAC device */ > + drvdata->data = of_match_node(altr_edac_device_of_match, np)->data; > + > + /* Check specific dependencies for the module */ > + if (drvdata->data->setup) { > + res = drvdata->data->setup(pdev, drvdata->base); > + if (res < 0) > + goto err; > + } > + > + drvdata->sb_irq = platform_get_irq(pdev, 0); > + res = devm_request_irq(&pdev->dev, drvdata->sb_irq, > + altr_edac_device_handler, > + 0, dev_name(&pdev->dev), dci); > + if (res < 0) > + goto err; > + > + drvdata->db_irq = platform_get_irq(pdev, 1); > + res = devm_request_irq(&pdev->dev, drvdata->db_irq, > + altr_edac_device_handler, > + 0, dev_name(&pdev->dev), dci); > + if (res < 0) > + goto err; > + > + dci->mod_name = "Altera ECC Manager"; > + dci->dev_name = drvdata->edac_dev_name; > + > + debugfs = edac_debugfs_create_dir(ecc_name); > + if (debugfs) > + altr_create_edacdev_dbgfs(dci, drvdata->data, debugfs); > + > + if (edac_device_add_device(dci)) > + goto err; > + > + devres_close_group(&pdev->dev, NULL); > + > + return 0; > +err: > + edac_printk(KERN_ERR, EDAC_DEVICE, > + "%s:Error setting up EDAC device: %d\n", ecc_name, res); > + devres_release_group(&pdev->dev, NULL); > + edac_device_free_ctl_info(dci); > + > + return res; > +} > + > +static int altr_edac_device_remove(struct platform_device *pdev) > +{ > + struct edac_device_ctl_info *dci = platform_get_drvdata(pdev); > + > + edac_device_del_device(&pdev->dev); > + edac_device_free_ctl_info(dci); > + > + return 0; > +} > + > +static struct platform_driver altr_edac_device_driver = { > + .probe = altr_edac_device_probe, > + .remove = altr_edac_device_remove, > + .driver = { > + .name = "altr_edac_device", > + .of_match_table = altr_edac_device_of_match, > + }, > +}; > +module_platform_driver(altr_edac_device_driver); > + > +/*********************** OCRAM EDAC Device Functions *********************/ > + > +#ifdef CONFIG_EDAC_ALTERA_OCRAM > + > +static void *ocram_alloc_mem(size_t size, void **other) > +{ > + struct device_node *np; > + struct gen_pool *gp; > + void *sram_addr; > + > + np = of_find_compatible_node(NULL, NULL, "altr,socfpga-ocram-ecc"); > + if (!np) > + return NULL; > + > + gp = of_gen_pool_get(np, "iram", 0); > + if (!gp) { > + of_node_put(np); > + return NULL; > + } > + of_node_put(np); gp = of_gen_pool_get(np, "iram", 0); of_node_put(np); if (!gp) return NULL; version is better. > + > + sram_addr = (void *)gen_pool_alloc(gp, size / sizeof(size_t)); > + if (!sram_addr) > + return NULL; > + > + memset(sram_addr, 0, size); Potential memory corruption, you allocate (size / sizeof(size_t)) bytes and then write size bytes. > + wmb(); /* Ensure data is written out */ > + > + *other = gp; /* Remember this handle for freeing later */ > + > + return sram_addr; > +} > + > +static void ocram_free_mem(void *p, size_t size, void *other) > +{ > + gen_pool_free((struct gen_pool *)other, (u32)p, size / sizeof(size_t)); See a comment above. > +} > + -- With best wishes, Vladimir