Re: [PATCHv8 1/4] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support
From: Thor Thayer
Date: Fri Jan 22 2016 - 10:31:20 EST
Hi Vladimir,
On 01/22/2016 12:02 AM, Vladimir Zapolskiy wrote:
Hi Thor,
On 21.01.2016 19:34, tthayer@xxxxxxxxxxxxxxxxxxxxx wrote:
From: Thor Thayer <tthayer@xxxxxxxxxxxxxxxxxxxxx>
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 <tthayer@xxxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Dinh Nguyen <dinguyen@xxxxxxxxxxxxxxxxxxxxx>
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]
While I was working in a different group at Altera last year, Dinh
submitted the previous patch revision so I kept his signed off by for
continuity. I'll just use myself in the future. Thank you for clarifying.
+/*
+ * 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.
Yes. Thank you.
+ 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.
I'm sorry, can you elaborate? Are you saying I should return something
other than NULL?
+
+ 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.
Yes, good catch. I thought I'd fixed this but missed it when I came back.
+ 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
Thank you for reviewing.