Message-ID: <CAAOTY__7gmvp+U+xD12KJkxuya=CkD8xV67S3pfC60gyfajQiA@mail.gmail.com>
Date:   Mon, 17 Aug 2020 23:13:56 +0800
From:   Chun-Kuang Hu <chunkuang.hu@...nel.org>
To:     Neal Liu <neal.liu@...iatek.com>
Cc:     Chun-Kuang Hu <chunkuang.hu@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        devicetree@...r.kernel.org,
        wsd_upstream <wsd_upstream@...iatek.com>,
        lkml <linux-kernel@...r.kernel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v6 2/2] soc: mediatek: add mt6779 devapc driver

Hi, Neal:

Neal Liu <neal.liu@...iatek.com> 於 2020年8月17日 週一 下午12:02寫道：
>
> Hi Chun-Kuang,
>
> On Sat, 2020-08-15 at 11:03 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <neal.liu@...iatek.com> 於 2020年8月13日 週四 上午11:33寫道：
> > >
> > > 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@...iatek.com>
> > > ---
> >
> > [snip]
> >
> > > +/*
> > > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > > + *                        violation information including which master violates
> > > + *                        access slave.
> > > + */
> > > +static irqreturn_t devapc_violation_irq(int irq_number,
> > > +                                       struct mtk_devapc_context *ctx)
> > > +{
> > > +       /*
> > > +        * Mask slave's irq before clearing vio status.
> > > +        * Must do it to avoid nested interrupt and prevent
> > > +        * unexpected behavior.
> > > +        */
> > > +       mask_module_irq(ctx, true);
> >
> > I still don't understand why nested interrupt happen. If two CPU
> > process different devapc interrupt at the same time, mask interrupt
> > could not prevent these two CPU to sync vio dbg at the same time. As I
> > know, in ARM CPU, only CPU0 process irq handler, and all devapc
> > interrupt has the same priority, so why nested interrupt happen? Could
> > you explain more detail about how nested interrupt happen?
>
> If there is another violation happened before previous violation is
> fully handled, nested interrupt would happen.
>
> Let's me take an example:
> vio A happen
> enter A ISR
> ...             vio B happen
> finish A ISR    enter B ISR
>                 ...
>                 finish B ISR
>
> We mask all module's irq to avoid nested interrupt.

This is not 'nested' interrupt. After A ISR is finished, B ISR happen.
So A ISR and B ISR are consecutive interrupt, not nested interrupt.
To compare mask irq and no mask irq, Let's consider this situation:

1. 1000 consecutive violation happen, the time period between two
violation is 0.01 ms, so the total time is 10ms. (In 10ms, 1000
violation happen)
2. One ISR handle time is 1 ms, so in one ISR handler, 100 violation happen.

For mask irq solution, 10 ISR handler is trigger. For no mask irq
solution, 11 ISR handler is trigger.
I think these two solution have similar result, and no mask irq
solution print more information (If these 1000 violation is trigger by
20 different driver, no mask solution may show one more driver than
mask solution)
So I think it's not necessary to mask irq in irq handler.

>
> >
> > > +
> > > +       while (devapc_sync_vio_dbg(ctx))
> > > +               devapc_extract_vio_dbg(ctx);
> > > +
> > > +       /*
> > > +        * Ensure that violation info are written
> > > +        * before further operations
> > > +        */
> > > +       smp_mb();
> > > +
> > > +       clear_vio_status(ctx);
> > > +       mask_module_irq(ctx, false);
> > > +
> > > +       return IRQ_HANDLED;
> > > +}
> > > +
> >
> > [snip]
> >
> > > +
> > > +static int mtk_devapc_remove(struct platform_device *pdev)
> > > +{
> > > +       struct mtk_devapc_context *ctx = platform_get_drvdata(pdev);
> > > +
> > > +       stop_devapc(ctx);
> > > +
> > > +       if (ctx->infra_clk)
> >
> > This always true.
>
> Does it mean that remove function would be called only if probe function
> is returned successfully?

Yes.

> Is there any chance this function would be called directly?

No.

Regards,
Chun-Kuang.

>
> >
> > Regards,
> > Chun-Kuang.
> >
> > > +               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 = KBUILD_MODNAME,
> > > +               .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@...iatek.com>");
> > > +MODULE_LICENSE("GPL");
> > > --
> > > 1.7.9.5
> > > _______________________________________________
> > > Linux-mediatek mailing list
> > > Linux-mediatek@...ts.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-mediatek
>

Powered by blists - more mailing lists