Re: [PATCH v5 2/2] soc: mediatek: add mt6779 devapc driver

[Chun-Kuang Hu <chunkuang.hu@kernel.org>]

Hi, Neal:

Neal Liu <neal.liu@mediatek.com> 於 2020年8月7日 週五 上午10:34寫道：
>
> MediaTek bus fabric provides TrustZone security support and data
> protection to prevent slaves from being accessed by unexpected
> masters.
> The security violation is logged and sent to the processor for
> further analysis or countermeasures.
>
> Any occurrence of security violation would raise an interrupt, and
> it will be handled by mtk-devapc driver. The violation
> information is printed in order to find the murderer.
>
> Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> ---

[snip]

> +
> +#define PHY_DEVAPC_TIMEOUT     0x10000
> +
> +/*
> + * devapc_sync_vio_dbg - do "shift" mechansim" to get full violation information.
> + *                       shift mechanism is depends on devapc hardware design.
> + *                       Mediatek devapc set multiple slaves as a group.
> + *                       When violation is triggered, violation info is kept
> + *                       inside devapc hardware.
> + *                       Driver should do shift mechansim to sync full violation
> + *                       info to VIO_DBGs registers.
> + *
> + */
> +static int devapc_sync_vio_dbg(struct mtk_devapc_context *ctx)
> +{
> +       void __iomem *pd_vio_shift_sta_reg;
> +       void __iomem *pd_vio_shift_sel_reg;
> +       void __iomem *pd_vio_shift_con_reg;
> +       int min_shift_group;
> +       int ret;
> +       u32 val;
> +
> +       pd_vio_shift_sta_reg = ctx->infra_base +
> +                              ctx->data->vio_shift_sta_offset;
> +       pd_vio_shift_sel_reg = ctx->infra_base +
> +                              ctx->data->vio_shift_sel_offset;
> +       pd_vio_shift_con_reg = ctx->infra_base +
> +                              ctx->data->vio_shift_con_offset;
> +
> +       /* Find the minimum shift group which has violation */
> +       val = readl(pd_vio_shift_sta_reg);
> +       if (!val)
> +               return false;
> +
> +       min_shift_group = __ffs(val);
> +
> +       /* Assign the group to sync */
> +       writel(0x1 << min_shift_group, pd_vio_shift_sel_reg);
> +
> +       /* Start syncing */
> +       writel(0x1, pd_vio_shift_con_reg);
> +
> +       ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val == 0x3, 0,
> +                                PHY_DEVAPC_TIMEOUT);
> +       if (ret) {
> +               dev_err(ctx->dev, "%s: Shift violation info failed\n", __func__);
> +               return false;
> +       }
> +
> +       /* Stop syncing */
> +       writel(0x0, pd_vio_shift_con_reg);
> +       writel(0x0, pd_vio_shift_sel_reg);

This is redundant because you set this register before start syncing.

> +       writel(0x1 << min_shift_group, pd_vio_shift_sta_reg);

You read this register to find minimum shift group, but you write it
back into this register, so this function would get the same minimum
shift group in next time, isn't it?

> +
> +       return true;
> +}
> +
> +/*
> + * devapc_extract_vio_dbg - extract full violation information after doing
> + *                          shift mechanism.
> + */
> +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> +{
> +       struct mtk_devapc_vio_dbgs *vio_dbgs;

struct mtk_devapc_vio_dbgs vio_dbgs;

Use stack instead of allocating from heap.

> +       void __iomem *vio_dbg0_reg;
> +       void __iomem *vio_dbg1_reg;
> +
> +       vio_dbgs = devm_kzalloc(ctx->dev, sizeof(struct mtk_devapc_vio_dbgs),
> +                               GFP_KERNEL);
> +       if (!vio_dbgs)
> +               return;
> +
> +       vio_dbg0_reg = ctx->infra_base + ctx->data->vio_dbg0_offset;
> +       vio_dbg1_reg = ctx->infra_base + ctx->data->vio_dbg1_offset;
> +
> +       vio_dbgs->vio_dbg0 = readl(vio_dbg0_reg);
> +       vio_dbgs->vio_dbg1 = readl(vio_dbg1_reg);
> +
> +       /* Print violation information */
> +       if (vio_dbgs->dbg0_bits.vio_w)
> +               dev_info(ctx->dev, "Write Violation\n");
> +       else if (vio_dbgs->dbg0_bits.vio_r)
> +               dev_info(ctx->dev, "Read Violation\n");
> +
> +       dev_info(ctx->dev, "Bus ID:0x%x, Dom ID:0x%x, Vio Addr:0x%x\n",
> +                vio_dbgs->dbg0_bits.mstid, vio_dbgs->dbg0_bits.dmnid,
> +                vio_dbgs->vio_dbg1);
> +}
> +

[snip]

> +
> +/*
> + * start_devapc - unmask slave's irq to start receiving devapc violation.
> + */
> +static void start_devapc(struct mtk_devapc_context *ctx)
> +{
> +       void __iomem *pd_apc_con_reg;
> +
> +       pd_apc_con_reg = ctx->infra_base + ctx->data->apc_con_offset;
> +       writel(BIT(31), pd_apc_con_reg);

pd_apc_con_reg is used once, so

writel(BIT(31), ctx->infra_base + ctx->data->apc_con_offset);

> +
> +       mask_module_irq(ctx, false);
> +}
> +

[snip]

> +
> +static int mtk_devapc_probe(struct platform_device *pdev)
> +{
> +       struct device_node *node = pdev->dev.of_node;
> +       struct mtk_devapc_context *ctx;
> +       u32 devapc_irq;
> +       int ret;
> +
> +       if (IS_ERR(node))
> +               return -ENODEV;
> +
> +       ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
> +       if (!ctx)
> +               return -ENOMEM;
> +
> +       ctx->data = of_device_get_match_data(&pdev->dev);
> +       ctx->dev = &pdev->dev;
> +
> +       ctx->infra_base = of_iomap(node, 0);
> +       if (!ctx->infra_base)
> +               return -EINVAL;
> +
> +       devapc_irq = irq_of_parse_and_map(node, 0);
> +       if (!devapc_irq)
> +               return -EINVAL;
> +
> +       ctx->infra_clk = devm_clk_get(&pdev->dev, "devapc-infra-clock");
> +       if (IS_ERR(ctx->infra_clk))
> +               return -EINVAL;
> +
> +       if (clk_prepare_enable(ctx->infra_clk))
> +               return -EINVAL;
> +
> +       ret = devm_request_irq(&pdev->dev, devapc_irq,
> +                              (irq_handler_t)devapc_violation_irq,
> +                              IRQF_TRIGGER_NONE, "devapc", ctx);
> +       if (ret) {
> +               clk_disable_unprepare(ctx->infra_clk);
> +               return ret;
> +       }
> +
> +       platform_set_drvdata(pdev, ctx);
> +
> +       start_devapc(ctx);
> +
> +       return 0;
> +}
> +
> +static int mtk_devapc_remove(struct platform_device *pdev)
> +{
> +       struct mtk_devapc_context *ctx = platform_get_drvdata(pdev);
> +

stop_devapc(ctx);

Regards,
Chun-Kuang.

> +       if (ctx->infra_clk)
> +               clk_disable_unprepare(ctx->infra_clk);
> +
> +       return 0;
> +}
> +