Date:   Mon, 13 Jul 2020 16:27:24 +0800
From:   Neal Liu <neal.liu@...iatek.com>
To:     Chun-Kuang Hu <chunkuang.hu@...nel.org>
CC:     Neal Liu <neal.liu@...iatek.com>, 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 v2 2/2] soc: mediatek: add mtk-devapc driver

Hi Chun-Kuang,

Thanks for your review.

On Fri, 2020-07-10 at 22:21 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <neal.liu@...iatek.com> 於 2020年7月10日 週五 上午11:23寫道：
> >
> > Hi Chun-Kuang,
> >
> > Thanks for your review.
> >
> > On Thu, 2020-07-09 at 21:01 +0800, Chun-Kuang Hu wrote:
> > > Hi, Neal:
> > >
> > > Neal Liu <neal.liu@...iatek.com> 於 2020年7月9日 週四 下午5:13寫道：
> > > >
> > > > 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]
> > >
> > > > +
> > > > +static u32 get_shift_group(struct mtk_devapc_context *devapc_ctx,
> > > > +                          int slave_type, int vio_idx)
> > >
> > > vio_idx  is useless, so remove it.
> > >
> >
> > yes, my mistake. I'll remove it on next patch.
> >
> > > > +{
> > > > +       u32 vio_shift_sta;
> > > > +       void __iomem *reg;
> > > > +       int bit;
> > > > +
> > > > +       reg = mtk_devapc_pd_get(devapc_ctx, slave_type, VIO_SHIFT_STA, 0);
> > > > +       vio_shift_sta = readl(reg);
> > > > +
> > > > +       for (bit = 0; bit < 32; bit++) {
> > > > +               if ((vio_shift_sta >> bit) & 0x1)
> > > > +                       break;
> > > > +       }
> > > > +
> > > > +       return bit;
> > > > +}
> > > > +
> > >
> > > [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 *devapc_ctx)
> > > > +{
> > > > +       const struct mtk_device_info **device_info;
> > > > +       int slave_type_num;
> > > > +       int vio_idx = -1;
> > > > +       int slave_type;
> > > > +
> > > > +       slave_type_num = devapc_ctx->slave_type_num;
> > > > +       device_info = devapc_ctx->device_info;
> > > > +
> > > > +       for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> > >
> > > If slave_type_num is 1, I think the code should be simpler.
> >
> > slave_type_num is depends on DT data, it's not always 1.
> 
> Please change commit title to "add mt6779 mtk-devapc driver". This
> patch is just for mt6779. If slave_type_num = 1 in mt6779, there is
> only one slave and we don't need a slave_type variable. Add
> slave_type_num in the patch of adding one SoC which has multiple
> slaves.

If slave_type_num value is passed from DT data, could we still assume
its value? Does it make sense to have this strong assumption?

I'm going to remove mtk_device_info struct array, and pass all SoC
specific data from DT.
Is it okay to keep slave_type_num as a variance?

> 
> >
> > >
> > > > +               if (!mtk_devapc_dump_vio_dbg(devapc_ctx, slave_type, &vio_idx))
> > > > +                       continue;
> > > > +
> > > > +               /* Ensure that violation info are written before
> > > > +                * further operations
> > > > +                */
> > > > +               smp_mb();
> > > > +
> > > > +               mask_module_irq(devapc_ctx, slave_type, vio_idx, true);
> > >
> > > Why do you mask irq?
> >
> > It has to mask slave's irq before clear violation status.
> > It's one of hardware design.
> 
> If don't do this before clear_vio_status, what would happen? The clear
> would fail?

If we don't mask slave's irq before clear vio status, It might trigger
another interrupt before current ISR finished. The nested interrupt will
have unexpected behavior and hardware state machine goes wrong.

> 
> >
> > >
> > > > +
> > > > +               clear_vio_status(devapc_ctx, slave_type, vio_idx);
> > > > +
> > > > +               mask_module_irq(devapc_ctx, slave_type, vio_idx, false);
> > > > +       }
> > > > +
> > > > +       return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > +/*
> > > > + * start_devapc - initialize devapc status and start receiving interrupt
> > > > + *               while devapc violation is triggered.
> > > > + */
> > >
> > > [snip]
> > >
> > > > +
> > > > +struct mtk_device_info {
> > > > +       int sys_index;
> > >
> > > Useless, so remove it.
> >
> > We need to print it as our debug information.
> > But I did not apply it on this patch, I'll add it on next patch.
> 
> I think vio address is enough to find out the murder, so remove it in
> this patch. If it provide another information, add it in another patch
> and describe clear about what is this and how to use this information.
> 

Okay, it make sense. I'll remove it in next patches.

> >
> > >
> > > > +       int ctrl_index;
> > >
> > > Ditto.
> > >
> > > Regards,
> > > Chun-Kuang.
> > >
> > > > +       int vio_index;
> > > > +};
> > > > +
> >

Powered by blists - more mailing lists