| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <56A24C34.3000303@opensource.altera.com>
Date: Fri, 22 Jan 2016 09:35:16 -0600
From: Thor Thayer <tthayer@...nsource.altera.com>
To: Vladimir Zapolskiy <vladimir_zapolskiy@...tor.com>, <bp@...en8.de>,
<dougthompson@...ssion.com>, <m.chehab@...sung.com>,
<robh+dt@...nel.org>, <pawel.moll@....com>, <mark.rutland@....com>,
<ijc+devicetree@...lion.org.uk>, <galak@...eaurora.org>,
<linux@....linux.org.uk>, <dinguyen@...nsource.altera.com>,
<grant.likely@...aro.org>
CC: <devicetree@...r.kernel.org>, <linux-doc@...r.kernel.org>,
<linux-edac@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>, <tthayer.linux@...il.com>
Subject: Re: [PATCHv8 1/4] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC
Support
Hi Vladimir,
On 01/22/2016 12:02 AM, Vladimir Zapolskiy wrote:
> Hi Thor,
>
> On 21.01.2016 19:34, tthayer@...nsource.altera.com wrote:
>> From: Thor Thayer <tthayer@...nsource.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 <tthayer@...nsource.altera.com>
>> Signed-off-by: Dinh Nguyen <dinguyen@...nsource.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]
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.
Powered by blists - more mailing lists