Hi Thor,
On 21.01.2016 19:34, ttha...@opensource.altera.com wrote:
> From: Thor Thayer <ttha...@opensource.altera.com>
>
> 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 <ttha...@opensource.altera.com>
> Signed-off-by: Dinh Nguyen <dingu...@opensource.altera.com>
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
