Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver

[Tomasz Figa]

Hi,

Please find next part of my comments inline.

On Fri, Mar 6, 2015 at 7:48 PM,  <yong.wu@xxxxxxxxxxxx> wrote:

[snip]

> +/*
> + * pimudev is a global var for dma_alloc_coherent.
> + * It is not accepatable, we will delete it if "domain_alloc" is enabled

It looks like we indeed need to use dma_alloc_coherent() and we don't
have a good way to pass the device pointer to domain_init callback.

If you don't expect SoCs in the nearest future to have multiple M4U
blocks, then I guess this global variable could stay, after changing
the comment into an explanation why it's correct. Also it should be
moved to the top of the file, below #include directives, as this is
where usually global variables are located.

> + */
> +static struct device *pimudev;
> +
> +static int mtk_iommu_domain_init(struct iommu_domain *domain)
> +{
> +       struct mtk_iommu_domain *priv;
> +
> +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->pgd = dma_alloc_coherent(pimudev, M4U_PGD_SIZE, &priv->pgd_pa,
> +                                      GFP_KERNEL);
> +       if (!priv->pgd) {
> +               pr_err("dma_alloc_coherent pagetable fail\n");
> +               goto err_pgtable;

ret = -ENOMEM;
goto err_free_priv;

> +       }
> +
> +       if (!IS_ALIGNED(priv->pgd_pa, M4U_PGD_SIZE)) {
> +               pr_err("pagetable not aligned pa 0x%pad-0x%p align 0x%x\n",
> +                      &priv->pgd_pa, priv->pgd, M4U_PGD_SIZE);
> +               goto err_pgtable;

ret = -ENOMEM;
goto err_dmafree;

> +       }
> +
> +       memset(priv->pgd, 0, M4U_PGD_SIZE);
> +
> +       spin_lock_init(&priv->pgtlock);
> +       spin_lock_init(&priv->portlock);
> +       domain->priv = priv;
> +
> +       domain->geometry.aperture_start = 0;
> +       domain->geometry.aperture_end   = (unsigned int)~0;

Please replace the cast with ~0U and fix multiple spaces between =.

> +       domain->geometry.force_aperture = true;
> +
> +       return 0;
> +
> +err_pgtable:

err_dma_free:

> +       if (priv->pgd)

Remove this check.

> +               dma_free_coherent(pimudev, M4U_PGD_SIZE, priv->pgd,
> +                                 priv->pgd_pa);

err_free_priv:

> +       kfree(priv);
> +       return -ENOMEM;

return ret;

> +}
> +
> +static void mtk_iommu_domain_destroy(struct iommu_domain *domain)
> +{
> +       struct mtk_iommu_domain *priv = domain->priv;
> +
> +       dma_free_coherent(priv->piommuinfo->dev, M4U_PGD_SIZE,
> +                         priv->pgd, priv->pgd_pa);
> +       kfree(domain->priv);
> +       domain->priv = NULL;
> +}
> +
> +static int mtk_iommu_attach_device(struct iommu_domain *domain,
> +                                  struct device *dev)
> +{
> +       unsigned long flags;
> +       struct mtk_iommu_domain *priv = domain->priv;
> +       struct mtk_iommu_info *piommu = priv->piommuinfo;
> +       struct of_phandle_args out_args = {0};
> +       struct device *imudev;
> +       unsigned int i = 0;
> +
> +       if (!piommu)

Could you explain when this can happen?

> +               goto imudev;

return 0;

> +       else

No else needed.

> +               imudev = piommu->dev;
> +
> +       spin_lock_irqsave(&priv->portlock, flags);

What is protected by this spinlock?

> +
> +       while (!of_parse_phandle_with_args(dev->of_node, "iommus",
> +                                          "#iommu-cells", i, &out_args)) {
> +               if (1 == out_args.args_count) {

Can we be sure that this is actually referring to our IOMMU?

Maybe this should be rewritten to

if (out_args.np != imudev->of_node)
        continue;
if (out_args.args_count != 1) {
        dev_err(imudev, "invalid #iommu-cells property for IOMMU %s\n",

}

> +                       unsigned int portid = out_args.args[0];
> +
> +                       dev_dbg(dev, "iommu add port:%d\n", portid);

imudev should be used here instead of dev.

> +
> +                       mtk_iommu_config_port(piommu, portid);
> +
> +                       if (i == 0)
> +                               dev->archdata.dma_ops =
> +                                       piommu->dev->archdata.dma_ops;

Shouldn't this be set automatically by IOMMU or DMA mapping core?

> +               }
> +               i++;
> +       }
> +
> +       spin_unlock_irqrestore(&priv->portlock, flags);
> +
> +imudev:
> +       return 0;
> +}
> +
> +static void mtk_iommu_detach_device(struct iommu_domain *domain,
> +                                   struct device *dev)
> +{

No hardware (de)configuration or clean-up necessary?

> +}
> +
> +static int mtk_iommu_map(struct iommu_domain *domain, unsigned long iova,
> +                        phys_addr_t paddr, size_t size, int prot)
> +{
> +       struct mtk_iommu_domain *priv = domain->priv;
> +       unsigned long flags;
> +       int ret;
> +
> +       spin_lock_irqsave(&priv->pgtlock, flags);
> +       ret = m4u_map(priv, (unsigned int)iova, paddr, size, prot);
> +       mtk_iommu_invalidate_tlb(priv->piommuinfo, 0,
> +                                iova, iova + size - 1);
> +       spin_unlock_irqrestore(&priv->pgtlock, flags);
> +
> +       return ret;
> +}
> +
> +static size_t mtk_iommu_unmap(struct iommu_domain *domain,
> +                             unsigned long iova, size_t size)
> +{
> +       struct mtk_iommu_domain *priv = domain->priv;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&priv->pgtlock, flags);
> +       m4u_unmap(priv, (unsigned int)iova, size);
> +       mtk_iommu_invalidate_tlb(priv->piommuinfo, 0,
> +                                iova, iova + size - 1);
> +       spin_unlock_irqrestore(&priv->pgtlock, flags);
> +
> +       return size;
> +}
> +
> +static phys_addr_t mtk_iommu_iova_to_phys(struct iommu_domain *domain,
> +                                         dma_addr_t iova)
> +{
> +       struct mtk_iommu_domain *priv = domain->priv;
> +       unsigned long flags;
> +       struct m4u_pte_info_t pte;
> +
> +       spin_lock_irqsave(&priv->pgtlock, flags);
> +       m4u_get_pte_info(priv, (unsigned int)iova, &pte);
> +       spin_unlock_irqrestore(&priv->pgtlock, flags);
> +
> +       return pte.pa;
> +}
> +
> +static struct iommu_ops mtk_iommu_ops = {
> +       .domain_init = mtk_iommu_domain_init,
> +       .domain_destroy = mtk_iommu_domain_destroy,
> +       .attach_dev = mtk_iommu_attach_device,
> +       .detach_dev = mtk_iommu_detach_device,
> +       .map = mtk_iommu_map,
> +       .unmap = mtk_iommu_unmap,
> +       .map_sg = default_iommu_map_sg,
> +       .iova_to_phys = mtk_iommu_iova_to_phys,
> +       .pgsize_bitmap = SZ_4K | SZ_64K | SZ_1M | SZ_16M,
> +};
> +
> +static const struct of_device_id mtk_iommu_of_ids[] = {
> +       { .compatible = "mediatek,mt8173-iommu",
> +         .data = &mtk_iommu_mt8173_cfg,
> +       },
> +       {}
> +};
> +
> +static int mtk_iommu_probe(struct platform_device *pdev)
> +{
> +       int ret;
> +       struct iommu_domain *domain;
> +       struct mtk_iommu_domain *mtk_domain;
> +       struct mtk_iommu_info *piommu;
> +       struct iommu_dma_domain  *dom;
> +       const struct of_device_id *of_id;
> +
> +       piommu = devm_kzalloc(&pdev->dev, sizeof(struct mtk_iommu_info),

sizeof(*piommu)

> +                             GFP_KERNEL);
> +       if (!piommu)
> +               return -ENOMEM;
> +
> +       pimudev = &pdev->dev;
> +       piommu->dev = &pdev->dev;
> +
> +       of_id = of_match_node(mtk_iommu_of_ids, pdev->dev.of_node);
> +       if (!of_id)
> +               return -ENODEV;

Please print an error message.

> +
> +       piommu->protect_va = devm_kmalloc(piommu->dev, MTK_PROTECT_PA_ALIGN*2,

style: Operators like * should have space on both sides.

> +                                         GFP_KERNEL);

Shouldn't dma_alloc_coherent() be used for this?

> +       if (!piommu->protect_va)
> +               goto protect_err;

Please return -ENOMEM here directly, as there is nothing to clean up
in this case.

> +       memset(piommu->protect_va, 0x55, MTK_PROTECT_PA_ALIGN*2);
> +
> +       piommu->imucfg = (const struct mtk_iommu_cfg *)of_id->data;

Why this cast is needed? Since of_id->data is const void * it should
be fine without a cast.

> +
> +       ret = mtk_iommu_parse_dt(pdev, piommu);
> +       if (ret) {
> +               dev_err(piommu->dev, "iommu dt parse fail\n");
> +               goto protect_err;

Still nothing to clean-up, so you can safely just return ret;

> +       }
> +
> +       /* alloc memcache for level-2 pgt */
> +       piommu->m4u_pte_kmem = kmem_cache_create("m4u_pte", IMU_BYTES_PER_PTE,
> +                                                IMU_BYTES_PER_PTE, 0, NULL);
> +
> +       if (IS_ERR_OR_NULL(piommu->m4u_pte_kmem)) {

Looks like the convention used by kmem_cache_create() is valid ptr or
NULL, no ERR_PTR()s.

> +               dev_err(piommu->dev, "pte cached create fail %p\n",
> +                       piommu->m4u_pte_kmem);
> +               goto protect_err;

Still nothing to clean-up.

> +       }
> +
> +       arch_setup_dma_ops(piommu->dev, 0, (1ULL<<32) - 1, &mtk_iommu_ops, 0);

style: Missing spaces around << operator.

> +
> +       dom = get_dma_domain(piommu->dev);
> +       domain = iommu_dma_raw_domain(dom);
> +
> +       mtk_domain = domain->priv;

domain is already dereferenced here, but NULL pointer check is two lines below.

> +       mtk_domain->piommuinfo = piommu;
> +
> +       if (!domain)
> +               goto pte_err;

Please print error message.

> +
> +       ret = mtk_iommu_hw_init(mtk_domain);
> +       if (ret < 0)
> +               goto hw_err;

Please print error message.

> +
> +       if (devm_request_irq(piommu->dev, piommu->irq,
> +                            mtk_iommu_isr, IRQF_TRIGGER_NONE,
> +                            "mtkiommu", (void *)domain)) {

Please align wrapped lines using tabs only.

Also, what do you mean by IRQF_TRIGGER_NONE? If you don't need any
special flags then 0 is enough.

Also, usually dev_name() is used for interrupt name.

Also, unnecessary cast of domain to void *.

> +               dev_err(piommu->dev, "IRQ request %d failed\n",
> +                       piommu->irq);
> +               goto hw_err;
> +       }
> +
> +       iommu_set_fault_handler(domain, mtk_iommu_fault_handler, piommu);

I don't see any other drivers doing this. Isn't this for upper layers,
so that they can set their own generic fault handlers?

> +
> +       dev_set_drvdata(piommu->dev, piommu);

This should be set before allowing the interrupt to fire. In other
words, the driver should be fully set up at the time of enabling the
IRQ.

> +
> +       return 0;

style: Missing blank line.

> +hw_err:
> +       arch_teardown_dma_ops(piommu->dev);
> +pte_err:
> +       kmem_cache_destroy(piommu->m4u_pte_kmem);
> +protect_err:
> +       dev_err(piommu->dev, "probe error\n");

Please replace this with specific messages for all errors (in case the
called function doesn't already print one like kmalloc and friends).

> +       return 0;

Returning 0, which means success, doesn't look like a good idea for
signalling a failure. Please return the correct error code as received
from function that errors out if possible.

End of part 3.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/