Date:   Tue, 16 Jun 2020 14:45:36 +0800
From:   Neal Liu <neal.liu@...iatek.com>
To:     Chun-Kuang Hu <chunkuang.hu@...nel.org>
CC:     Neal Liu <neal.liu@...iatek.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        wsd_upstream <wsd_upstream@...iatek.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver

Hi Chun-Kuang,


On Sat, 2020-06-13 at 07:20 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <neal.liu@...iatek.com> 於 2020年6月9日 週二 下午6:25寫道：
> >
> > MT6873 bus frabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violations are 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 devapc-mt6873 driver. The violation
> > information is printed in order to find the murderer.
> >
> > Signed-off-by: Neal Liu <neal.liu@...iatek.com>
> 
> [snip]
> 
> > +
> > +/*
> > + * sramrom_vio_handler - clean sramrom violation & print violation information
> > + *                      for debugging.
> > + */
> > +static void sramrom_vio_handler(void)
> > +{
> > +       const struct mtk_sramrom_sec_vio_desc *sramrom_vios;
> > +       struct mtk_devapc_vio_info *vio_info;
> > +       struct arm_smccc_res res;
> > +       size_t sramrom_vio_sta;
> > +       int sramrom_vio;
> > +       u32 rw;
> > +
> > +       sramrom_vios = mtk_devapc_ctx->soc->sramrom_sec_vios;
> > +       vio_info = mtk_devapc_ctx->soc->vio_info;
> > +
> > +       arm_smccc_smc(MTK_SIP_KERNEL_CLR_SRAMROM_VIO,
> > +                     0, 0, 0, 0, 0, 0, 0, &res);
> > +
> > +       sramrom_vio = res.a0;
> > +       sramrom_vio_sta = res.a1;
> > +       vio_info->vio_addr = res.a2;
> > +
> > +       if (sramrom_vio == SRAM_VIOLATION)
> > +               pr_info(PFX "%s, SRAM violation is triggered\n", __func__);
> > +       else if (sramrom_vio == ROM_VIOLATION)
> > +               pr_info(PFX "%s, ROM violation is triggered\n", __func__);
> > +       else
> > +               return;
> > +
> > +       vio_info->master_id = (sramrom_vio_sta & sramrom_vios->vio_id_mask)
> > +                       >> sramrom_vios->vio_id_shift;
> > +       vio_info->domain_id = (sramrom_vio_sta & sramrom_vios->vio_domain_mask)
> > +                       >> sramrom_vios->vio_domain_shift;
> > +       rw = (sramrom_vio_sta & sramrom_vios->vio_rw_mask) >>
> > +                       sramrom_vios->vio_rw_shift;
> 
> I think some information, such as master_id, would be get in
> devapc_extract_vio_dbg(), you need not to get it here.

sramrom violation is from type2 slaves, and these kinds of slaves'
violation informations are stored in different registers than normal
slaves.
So we would check is it type2 violation or normal violation, and get
violation information from corresponding registers.

> > +
> > +       if (rw)
> > +               vio_info->write = 1;
> > +       else
> > +               vio_info->read = 1;
> > +
> > +       pr_info(PFX "%s: %s:0x%x, %s:0x%x, %s:%s, %s:0x%x\n",
> > +               __func__, "master_id", vio_info->master_id,
> > +               "domain_id", vio_info->domain_id,
> > +               "rw", rw ? "Write" : "Read",
> > +               "vio_addr", vio_info->vio_addr);
> > +}
> > +
> 
> [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, void *dev_id)
> > +{
> > +       u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> 
> Don't make  mtk_devapc_ctx a global variable. You should allocate
> instance of  mtk_devapc_ctx in probe(), and pass  mtk_devapc_ctx to
> the last parameter of devm_request_irq(), and it would be the second
> parameter of irq handler.

Okay, I'll try to revise and upstream again.

> > +       const struct mtk_device_info **device_info;
> > +       struct mtk_devapc_vio_info *vio_info;
> > +       int slave_type, vio_idx, index;
> > +       const char *vio_master;
> > +       unsigned long flags;
> > +       bool normal;
> > +       u8 perm;
> > +
> > +       spin_lock_irqsave(&devapc_lock, flags);
> > +
> > +       device_info = mtk_devapc_ctx->soc->device_info;
> > +       vio_info = mtk_devapc_ctx->soc->vio_info;
> > +       normal = false;
> > +       vio_idx = -1;
> > +       index = -1;
> > +
> > +       /* There are multiple DEVAPC_PD */
> > +       for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> > +               if (!check_type2_vio_status(slave_type, &vio_idx, &index))
> > +                       if (!mtk_devapc_dump_vio_dbg(slave_type, &vio_idx,
> > +                                                    &index))
> > +                               continue;
> > +
> > +               /* Ensure that violation info are written before
> > +                * further operations
> > +                */
> > +               smp_mb();
> > +               normal = true;
> > +
> > +               mask_module_irq(slave_type, vio_idx, true);
> > +
> > +               if (clear_vio_status(slave_type, vio_idx))
> > +                       pr_warn(PFX "%s, %s:0x%x, %s:0x%x\n",
> > +                               "clear vio status failed",
> > +                               "slave_type", slave_type,
> > +                               "vio_index", vio_idx);
> > +
> > +               perm = get_permission(slave_type, index, vio_info->domain_id);
> > +
> > +               vio_master = mtk_devapc_ctx->soc->master_get
> > +                       (vio_info->master_id,
> > +                        vio_info->vio_addr,
> > +                        slave_type,
> > +                        vio_info->shift_sta_bit,
> > +                        vio_info->domain_id);
> > +
> > +               if (!vio_master) {
> > +                       pr_warn(PFX "master_get failed\n");
> > +                       vio_master = "UNKNOWN_MASTER";
> > +               }
> > +
> > +               pr_info(PFX "%s - %s:0x%x, %s:0x%x, %s:0x%x, %s:0x%x\n",
> > +                       "Violation", "slave_type", slave_type,
> > +                       "sys_index",
> > +                       device_info[slave_type][index].sys_index,
> > +                       "ctrl_index",
> > +                       device_info[slave_type][index].ctrl_index,
> > +                       "vio_index",
> > +                       device_info[slave_type][index].vio_index);
> > +
> > +               pr_info(PFX "%s %s %s %s\n",
> > +                       "Violation - master:", vio_master,
> > +                       "access violation slave:",
> > +                       device_info[slave_type][index].device);
> > +
> > +               devapc_vio_reason(perm);
> > +
> > +               devapc_extra_handler(slave_type, vio_master, vio_idx,
> > +                                    vio_info->vio_addr);
> > +
> > +               mask_module_irq(slave_type, vio_idx, false);
> > +       }
> > +
> > +       if (normal) {
> > +               spin_unlock_irqrestore(&devapc_lock, flags);
> > +               return IRQ_HANDLED;
> > +       }
> > +
> > +       spin_unlock_irqrestore(&devapc_lock, flags);
> > +       return IRQ_HANDLED;
> > +}
> > +
> 
> [snip]
> 
> > +
> > +int mtk_devapc_probe(struct platform_device *pdev, struct mtk_devapc_soc *soc)
> > +{
> > +       struct device_node *node = pdev->dev.of_node;
> > +       u32 slave_type_num;
> > +       int slave_type;
> > +       int ret;
> > +
> > +       if (IS_ERR(node))
> > +               return -ENODEV;
> > +
> > +       mtk_devapc_ctx->soc = soc;
> > +       slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> > +
> > +       for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> > +               mtk_devapc_ctx->devapc_pd_base[slave_type] =
> > +                       of_iomap(node, slave_type);
> > +               if (!mtk_devapc_ctx->devapc_pd_base[slave_type])
> > +                       return -EINVAL;
> > +       }
> > +
> > +       mtk_devapc_ctx->infracfg_base = of_iomap(node, slave_type_num + 1);
> > +       if (!mtk_devapc_ctx->infracfg_base)
> > +               return -EINVAL;
> > +
> > +       mtk_devapc_ctx->devapc_irq = irq_of_parse_and_map(node, 0);
> > +       if (!mtk_devapc_ctx->devapc_irq)
> > +               return -EINVAL;
> > +
> > +       /* CCF (Common Clock Framework) */
> > +       mtk_devapc_ctx->devapc_infra_clk = devm_clk_get(&pdev->dev,
> > +                                                       "devapc-infra-clock");
> > +
> > +       if (IS_ERR(mtk_devapc_ctx->devapc_infra_clk))
> > +               return -EINVAL;
> > +
> > +       proc_create("devapc_dbg", 0664, NULL, &devapc_dbg_fops);
> > +
> > +       if (clk_prepare_enable(mtk_devapc_ctx->devapc_infra_clk))
> > +               return -EINVAL;
> > +
> > +       start_devapc();
> > +
> > +       ret = devm_request_irq(&pdev->dev, mtk_devapc_ctx->devapc_irq,
> > +                              (irq_handler_t)devapc_violation_irq,
> > +                              IRQF_TRIGGER_NONE, "devapc", NULL);
> > +       if (ret) {
> > +               pr_err(PFX "request devapc irq failed, ret:%d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_devapc_probe);
> 
> Why export probe function?
> 
> > +
> > +int mtk_devapc_remove(struct platform_device *dev)
> > +{
> > +       clk_disable_unprepare(mtk_devapc_ctx->devapc_infra_clk);
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_devapc_remove);
> 
> Ditto.
> 
> Regards,
> Chun-Kuang.
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Powered by blists - more mailing lists