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] > +/* > + * 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html