lkml.org 
[lkml]   [2021]   [Jan]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v8 2/2] soc: mediatek: add mt6779 devapc driver
    Hi Neal,
    Please see inline comments below.

    On Thu, Oct 15, 2020 at 11:21 AM Neal Liu <neal.liu@mediatek.com> wrote:
    >
    > 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>
    > ---
    > drivers/soc/mediatek/Kconfig | 9 ++
    > drivers/soc/mediatek/Makefile | 1 +
    > drivers/soc/mediatek/mtk-devapc.c | 308 +++++++++++++++++++++++++++++++++++++
    > 3 files changed, 318 insertions(+)
    > create mode 100644 drivers/soc/mediatek/mtk-devapc.c
    >
    > ...
    > diff --git a/drivers/soc/mediatek/mtk-devapc.c b/drivers/soc/mediatek/mtk-devapc.c
    > new file mode 100644
    > index 0000000..f1cea04
    > --- /dev/null
    > +++ b/drivers/soc/mediatek/mtk-devapc.c
    >
    > ...
    >
    > +
    > +static void clear_vio_status(struct mtk_devapc_context *ctx)
    > +{
    > + void __iomem *reg;
    > + int i;
    > +
    > + reg = ctx->infra_base + ctx->data->vio_sta_offset;
    > +
    > + for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num) - 1; i++)
    > + writel(GENMASK(31, 0), reg + 4 * i);
    > +
    > + writel(GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num) - 1, 0),
    > + reg + 4 * i);

    Is it guaranteed that vio_idx_num will always not a multiple of 32, or
    should we check that VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num) is not
    zero?

    > +}
    > +
    > +static void mask_module_irq(struct mtk_devapc_context *ctx, bool mask)
    > +{
    > + void __iomem *reg;
    > + u32 val;
    > + int i;
    > +
    > + reg = ctx->infra_base + ctx->data->vio_mask_offset;
    > +
    > + if (mask)
    > + val = GENMASK(31, 0);
    > + else
    > + val = 0;
    > +
    > + for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num) - 1; i++)
    > + writel(val, reg + 4 * i);
    > +
    > + val = readl(reg + 4 * i);
    > + if (mask)
    > + val |= GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num) - 1,
    > + 0);
    > + else
    > + val &= ~GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num) - 1,
    > + 0);

    Same here.

    > +
    > + writel(val, reg + 4 * i);
    > +}
    > +
    > +#define PHY_DEVAPC_TIMEOUT 0x10000
    > +
    > +/*
    > + * devapc_sync_vio_dbg - do "shift" mechansim" to get full violation information.

    nit: One extra double quote in the comment.

    > + * 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)
    >
    > ...
    >
    > +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))

    Does this ever happens? I feel the check should be `if (node == NULL)`.

    > + 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;

    Missing `iounmap(ctx->infra_base)` in the following error paths and
    mtk_devapc_remove. Use devm_of_iomap instead?

    > +
    > + 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, 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);
    > +
    > + clk_disable_unprepare(ctx->infra_clk);
    > +
    > + return 0;
    > +}
    > +
    > +static struct platform_driver mtk_devapc_driver = {
    > + .probe = mtk_devapc_probe,
    > + .remove = mtk_devapc_remove,
    > + .driver = {
    > + .name = "mtk-devapc",
    > + .of_match_table = mtk_devapc_dt_match,
    > + },
    > +};
    > +
    > +module_platform_driver(mtk_devapc_driver);
    > +
    > +MODULE_DESCRIPTION("Mediatek Device APC Driver");
    > +MODULE_AUTHOR("Neal Liu <neal.liu@mediatek.com>");
    > +MODULE_LICENSE("GPL");
    > --
    > 1.7.9.5

    \
     
     \ /
      Last update: 2021-01-14 10:42    [W:2.694 / U:0.184 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site