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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Tue, 3 Nov 2015 19:47:06 -0500
From:	Sinan Kaya <okaya@...eaurora.org>
To:	Andy Shevchenko <andy.shevchenko@...il.com>
Cc:	dmaengine <dmaengine@...r.kernel.org>, timur@...eaurora.org,
	cov@...eaurora.org, jcm@...hat.com,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Vinod Koul <vinod.koul@...el.com>,
	Dan Williams <dan.j.williams@...el.com>,
	devicetree <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 1/3] dma: add Qualcomm Technologies HIDMA management
 driver



On 11/3/2015 5:22 AM, Andy Shevchenko wrote:
> On Mon, Nov 2, 2015 at 8:07 AM, Sinan Kaya <okaya@...eaurora.org> wrote:
>> The Qualcomm Technologies HIDMA device has been designed
>> to support virtualization technology. The driver has been
>> divided into two to follow the hardware design. The management
>> driver is executed in hypervisor context and is the main
>> management entity for all channels provided by the device.
>> The channel driver is executed in the hypervisor/guest OS
>> context.
>>
>> All channel devices get probed in the hypervisor
>> context during power up. They show up as DMA engine
>> channels. Then, before starting the virtualization; each
>> channel device is unbound from the hypervisor by VFIO
>> and assigned to the guest machine for control.
>>
>> This management driver will be used by the system
>> admin to monitor/reset the execution state of the DMA
>> channels. This will be the management interface.
>
>
>> +static ssize_t qcom_hidma_mgmt_evtena(struct file *file,
>> +       const char __user *user_buf, size_t count, loff_t *ppos)
>> +{
>> +       char temp_buf[16+1];
>
> 16 is a magic number. Make it defined constant.
>

removed it

>> +       struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private;
>> +       u32 event;
>> +       ssize_t ret;
>> +       unsigned long val;
>> +
>> +       temp_buf[16] = '\0';
>> +       if (copy_from_user(temp_buf, user_buf, min_t(int, count, 16)))
>> +               goto out;
>> +
>> +       ret = kstrtoul(temp_buf, 16, &val);
>
> kstrtoul_from_user?

nice, simpler.

>
>> +       if (ret) {
>> +               dev_warn(&mgmtdev->pdev->dev, "unknown event\n");
>> +               goto out;
>> +       }
>> +
>> +       event = (u32)val & HW_EVENTS_CFG_MASK;
>> +
>> +       pm_runtime_get_sync(&mgmtdev->pdev->dev);
>> +       writel(event, mgmtdev->dev_virtaddr + HW_EVENTS_CFG_OFFSET);
>> +       pm_runtime_mark_last_busy(&mgmtdev->pdev->dev);
>> +       pm_runtime_put_autosuspend(&mgmtdev->pdev->dev);
>> +out:
>> +       return count;
>> +}
>> +
>> +static const struct file_operations qcom_hidma_mgmt_evtena_fops = {
>> +       .write = qcom_hidma_mgmt_evtena,
>> +};
>> +
>> +struct fileinfo {
>> +       char *name;
>> +       int mode;
>> +       const struct file_operations *ops;
>> +};
>> +
>> +static struct fileinfo files[] = {
>> +       {"info", S_IRUGO, &qcom_hidma_mgmt_fops},
>> +       {"err", S_IRUGO,  &qcom_hidma_mgmt_err_fops},
>> +       {"mhiderrclr", S_IWUSR, &qcom_hidma_mgmt_mhiderr_clrfops},
>> +       {"evterrclr", S_IWUSR, &qcom_hidma_mgmt_evterr_clrfops},
>> +       {"ideerrclr", S_IWUSR, &qcom_hidma_mgmt_ideerr_clrfops},
>> +       {"odeerrclr", S_IWUSR, &qcom_hidma_mgmt_odeerr_clrfops},
>> +       {"msierrclr", S_IWUSR, &qcom_hidma_mgmt_msierr_clrfops},
>> +       {"treerrclr", S_IWUSR, &qcom_hidma_mgmt_treerr_clrfops},
>> +       {"evtena", S_IWUSR, &qcom_hidma_mgmt_evtena_fops},
>> +};
>> +
>> +static void qcom_hidma_mgmt_debug_uninit(struct qcom_hidma_mgmt_dev *mgmtdev)
>> +{
>> +       debugfs_remove_recursive(mgmtdev->debugfs);
>> +}
>> +
>> +static int qcom_hidma_mgmt_debug_init(struct qcom_hidma_mgmt_dev *mgmtdev)
>> +{
>> +       int rc = 0;
>> +       u32 i;
>> +       struct dentry   *fs_entry;
>> +
>> +       mgmtdev->debugfs = debugfs_create_dir(dev_name(&mgmtdev->pdev->dev),
>> +                                               NULL);
>> +       if (!mgmtdev->debugfs) {
>> +               rc = -ENODEV;
>> +               return rc;
>> +       }
>> +
>> +       for (i = 0; i < ARRAY_SIZE(files); i++) {
>> +               fs_entry = debugfs_create_file(files[i].name,
>> +                                       files[i].mode, mgmtdev->debugfs,
>> +                                       mgmtdev, files[i].ops);
>> +               if (!fs_entry) {
>> +                       rc = -ENOMEM;
>> +                       goto cleanup;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +cleanup:
>> +       qcom_hidma_mgmt_debug_uninit(mgmtdev);
>> +       return rc;
>> +}
>> +#else
>> +static void qcom_hidma_mgmt_debug_uninit(struct qcom_hidma_mgmt_dev *mgmtdev)
>> +{
>> +}
>> +static int qcom_hidma_mgmt_debug_init(struct qcom_hidma_mgmt_dev *mgmtdev)
>> +{
>> +       return 0;
>> +}
>> +#endif
>> +
>> +static int qcom_hidma_mgmt_setup(struct qcom_hidma_mgmt_dev *mgmtdev)
>> +{
>> +       u32 val;
>> +       u32 i;
>> +
>> +       val = readl(mgmtdev->dev_virtaddr + MAX_BUS_REQ_LEN_OFFSET);
>> +       val = val &
>> +               ~(MAX_BUS_REQ_LEN_MASK << MAX_BUS_WR_REQ_BIT_POS);
>> +       val = val |
>> +               (mgmtdev->max_write_request << MAX_BUS_WR_REQ_BIT_POS);
>> +       val = val & ~(MAX_BUS_REQ_LEN_MASK);
>> +       val = val | (mgmtdev->max_read_request);
>> +       writel(val, mgmtdev->dev_virtaddr + MAX_BUS_REQ_LEN_OFFSET);
>> +
>> +       val = readl(mgmtdev->dev_virtaddr + MAX_XACTIONS_OFFSET);
>> +       val = val &
>> +               ~(MAX_WR_XACTIONS_MASK << MAX_WR_XACTIONS_BIT_POS);
>> +       val = val |
>> +               (mgmtdev->max_wr_xactions << MAX_WR_XACTIONS_BIT_POS);
>> +       val = val & ~(MAX_RD_XACTIONS_MASK);
>> +       val = val | (mgmtdev->max_rd_xactions);
>> +       writel(val, mgmtdev->dev_virtaddr + MAX_XACTIONS_OFFSET);
>> +
>> +       mgmtdev->sw_version = readl(mgmtdev->dev_virtaddr + SW_VERSION_OFFSET);
>> +
>> +       for (i = 0; i < mgmtdev->dma_channels; i++) {
>> +               val = readl(mgmtdev->dev_virtaddr + QOS_N_OFFSET + (4 * i));
>> +               val = val & ~(1 << PRIORITY_BIT_POS);
>> +               val = val |
>> +                       ((mgmtdev->priority[i] & 0x1) << PRIORITY_BIT_POS);
>> +               val = val & ~(WEIGHT_MASK << WRR_BIT_POS);
>> +               val = val
>> +                       | ((mgmtdev->weight[i] & WEIGHT_MASK) << WRR_BIT_POS);
>> +               writel(val, mgmtdev->dev_virtaddr + QOS_N_OFFSET + (4 * i));
>> +       }
>> +
>> +       val = readl(mgmtdev->dev_virtaddr + CHRESET_TIMEOUT_OFFSET);
>> +       val = val & ~CHRESET_TIMEOUUT_MASK;
>> +       val = val | (mgmtdev->chreset_timeout & CHRESET_TIMEOUUT_MASK);
>> +       writel(val, mgmtdev->dev_virtaddr + CHRESET_TIMEOUT_OFFSET);
>> +
>> +       val = readl(mgmtdev->dev_virtaddr + CFG_OFFSET);
>> +       val = val | 1;
>> +       writel(val, mgmtdev->dev_virtaddr + CFG_OFFSET);
>> +
>> +       return 0;
>> +}
>> +
>> +static int qcom_hidma_mgmt_probe(struct platform_device *pdev)
>> +{
>> +       struct resource *dma_resource;
>> +       int irq;
>> +       int rc;
>> +       u32 i;
>> +       struct qcom_hidma_mgmt_dev *mgmtdev;
>
> Better move this line to the top of definition block.
>
done

>> +
>> +       pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);
>> +       pm_runtime_use_autosuspend(&pdev->dev);
>> +       pm_runtime_set_active(&pdev->dev);
>> +       pm_runtime_enable(&pdev->dev);
>> +       dma_resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       if (!dma_resource) {
>> +               dev_err(&pdev->dev, "No memory resources found\n");
>> +               rc = -ENODEV;
>> +               goto out;
>> +       }
>
> Consolidate with devm_ioremap_resource()
>

OK

>> +
>> +       irq = platform_get_irq(pdev, 0);
>> +       if (irq < 0) {
>> +               dev_err(&pdev->dev, "irq resources not found\n");
>> +               rc = -ENODEV;
>> +               goto out;
>> +       }
>> +
>> +       mgmtdev = devm_kzalloc(&pdev->dev, sizeof(*mgmtdev), GFP_KERNEL);
>> +       if (!mgmtdev) {
>> +               rc = -ENOMEM;
>> +               goto out;
>> +       }
>> +
>> +       mgmtdev->pdev = pdev;
>> +
>> +       pm_runtime_get_sync(&mgmtdev->pdev->dev);
>> +       mgmtdev->dev_addrsize = resource_size(dma_resource);
>> +       mgmtdev->dev_virtaddr = devm_ioremap_resource(&pdev->dev,
>> +                                                       dma_resource);
>> +       if (IS_ERR(mgmtdev->dev_virtaddr)) {
>> +               dev_err(&pdev->dev, "can't map i/o memory at %pa\n",
>> +                       &dma_resource->start);
>> +               rc = -ENOMEM;
>> +               goto out;
>> +       }
>> +
>> +       if (device_property_read_u32(&pdev->dev, "dma-channels",
>> +               &mgmtdev->dma_channels)) {
>> +               dev_err(&pdev->dev, "number of channels missing\n");
>> +               rc = -EINVAL;
>> +               goto out;
>> +       }
>> +
>> +       if (device_property_read_u32(&pdev->dev, "channel-reset-timeout",
>> +               &mgmtdev->chreset_timeout)) {
>> +               dev_err(&pdev->dev, "channel reset timeout missing\n");
>> +               rc = -EINVAL;
>> +               goto out;
>> +       }
>> +
>> +       if (device_property_read_u32(&pdev->dev, "max-write-burst-bytes",
>> +               &mgmtdev->max_write_request)) {
>> +               dev_err(&pdev->dev, "max-write-burst-bytes missing\n");
>> +               rc = -EINVAL;
>> +               goto out;
>> +       }
>> +       if ((mgmtdev->max_write_request != 128) &&
>> +               (mgmtdev->max_write_request != 256) &&
>> +               (mgmtdev->max_write_request != 512) &&
>> +               (mgmtdev->max_write_request != 1024)) {
>
> is_power_of_2()  && min/max ?
>
ok

>> +               dev_err(&pdev->dev, "invalid write request %d\n",
>> +                       mgmtdev->max_write_request);
>> +               rc = -EINVAL;
>> +               goto out;
>> +       }
>> +
>> +       if (device_property_read_u32(&pdev->dev, "max-read-burst-bytes",
>> +               &mgmtdev->max_read_request)) {
>> +               dev_err(&pdev->dev, "max-read-burst-bytes missing\n");
>> +               rc = -EINVAL;
>> +               goto out;
>> +       }
>> +
>> +       if ((mgmtdev->max_read_request != 128) &&
>> +               (mgmtdev->max_read_request != 256) &&
>> +               (mgmtdev->max_read_request != 512) &&
>> +               (mgmtdev->max_read_request != 1024)) {
>
> Ditto.
>
done

>> +               dev_err(&pdev->dev, "invalid read request %d\n",
>> +                       mgmtdev->max_read_request);
>> +               rc = -EINVAL;
>> +               goto out;
>> +       }
>> +
>> +       if (device_property_read_u32(&pdev->dev, "max-write-transactions",
>> +               &mgmtdev->max_wr_xactions)) {
>> +               dev_err(&pdev->dev, "max-write-transactions missing\n");
>> +               rc = -EINVAL;
>> +               goto out;
>> +       }
>> +
>> +       if (device_property_read_u32(&pdev->dev, "max-read-transactions",
>> +               &mgmtdev->max_rd_xactions)) {
>> +               dev_err(&pdev->dev, "max-read-transactions missing\n");
>> +               rc = -EINVAL;
>> +               goto out;
>> +       }
>> +
>> +       mgmtdev->priority = devm_kcalloc(&pdev->dev,
>> +               mgmtdev->dma_channels, sizeof(*mgmtdev->priority), GFP_KERNEL);
>> +       if (!mgmtdev->priority) {
>> +               rc = -ENOMEM;
>> +               goto out;
>> +       }
>> +
>> +       mgmtdev->weight = devm_kcalloc(&pdev->dev,
>> +               mgmtdev->dma_channels, sizeof(*mgmtdev->weight), GFP_KERNEL);
>> +       if (!mgmtdev->weight) {
>> +               rc = -ENOMEM;
>> +               goto out;
>> +       }
>> +
>> +       if (device_property_read_u32_array(&pdev->dev, "channel-priority",
>> +                               mgmtdev->priority, mgmtdev->dma_channels)) {
>> +               dev_err(&pdev->dev, "channel-priority missing\n");
>> +               rc = -EINVAL;
>> +               goto out;
>> +       }
>> +
>> +       if (device_property_read_u32_array(&pdev->dev, "channel-weight",
>> +                               mgmtdev->weight, mgmtdev->dma_channels)) {
>> +               dev_err(&pdev->dev, "channel-weight missing\n");
>> +               rc = -EINVAL;
>> +               goto out;
>> +       }
>> +
>> +       for (i = 0; i < mgmtdev->dma_channels; i++) {
>> +               if (mgmtdev->weight[i] > 15) {
>
> 15 is magic.
>

I created a new macro called MAX_CHANNEL_WEIGHT.

>> +                       dev_err(&pdev->dev,
>> +                               "max value of weight can be 15.\n");
>> +                       rc = -EINVAL;
>> +                       goto out;
>> +               }
>> +
>> +               /* weight needs to be at least one */
>> +               if (mgmtdev->weight[i] == 0)
>> +                       mgmtdev->weight[i] = 1;
>> +       }
>> +
>> +       rc = qcom_hidma_mgmt_setup(mgmtdev);
>> +       if (rc) {
>> +               dev_err(&pdev->dev, "setup failed\n");
>> +               goto out;
>> +       }
>> +
>> +       rc = qcom_hidma_mgmt_debug_init(mgmtdev);
>> +       if (rc) {
>> +               dev_err(&pdev->dev, "debugfs init failed\n");
>> +               goto out;
>> +       }
>> +
>> +       dev_info(&pdev->dev,
>> +               "HI-DMA engine management driver registration complete\n");
>
> You may put some useful information here, otherwise looks like a debug message.
>
ok, I did now. I copied the syntax from another driver to here.

>> +       platform_set_drvdata(pdev, mgmtdev);
>> +       pm_runtime_mark_last_busy(&mgmtdev->pdev->dev);
>> +       pm_runtime_put_autosuspend(&mgmtdev->pdev->dev);
>> +       return 0;
>> +out:
>> +       pm_runtime_disable(&pdev->dev);
>> +       pm_runtime_put_sync_suspend(&pdev->dev);
>> +       return rc;
>> +}
>> +
>> +static int qcom_hidma_mgmt_remove(struct platform_device *pdev)
>> +{
>> +       struct qcom_hidma_mgmt_dev *mgmtdev = platform_get_drvdata(pdev);
>> +
>> +       pm_runtime_get_sync(&mgmtdev->pdev->dev);
>> +       qcom_hidma_mgmt_debug_uninit(mgmtdev);
>> +       pm_runtime_put_sync_suspend(&pdev->dev);
>> +       pm_runtime_disable(&pdev->dev);
>> +
>> +       dev_info(&pdev->dev, "HI-DMA engine management driver removed\n");
>
> Useless message.

removed .

>
>> +       return 0;
>> +}
>> +
>> +#if IS_ENABLED(CONFIG_ACPI)
>> +static const struct acpi_device_id qcom_hidma_mgmt_acpi_ids[] = {
>> +       {"QCOM8060"},
>> +       {},
>> +};
>> +#endif
>> +
>> +static const struct of_device_id qcom_hidma_mgmt_match[] = {
>> +       { .compatible = "qcom,hidma-mgmt-1.0", },
>> +       {},
>> +};
>> +MODULE_DEVICE_TABLE(of, qcom_hidma_mgmt_match);
>> +
>> +static struct platform_driver qcom_hidma_mgmt_driver = {
>> +       .probe = qcom_hidma_mgmt_probe,
>> +       .remove = qcom_hidma_mgmt_remove,
>> +       .driver = {
>> +               .name = "hidma-mgmt",
>> +               .of_match_table = qcom_hidma_mgmt_match,
>> +               .acpi_match_table = ACPI_PTR(qcom_hidma_mgmt_acpi_ids),
>> +       },
>> +};
>> +module_platform_driver(qcom_hidma_mgmt_driver);
>> +MODULE_LICENSE("GPL v2");
>> --
>> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>
>
>

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists