Hi, Neal:
Neal Liu <[email protected]> 於 2020年6月19日 週五 下午6:01寫道：
>
> 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 <[email protected]>
> ---
[snip]
> +
> +/*
> + * mtk_devapc_pd_get - get devapc pd_types of register address.
> + *
> + * Returns the value of reg addr
> + */
> +static void __iomem *mtk_devapc_pd_get(struct mtk_devapc_context *devapc_ctx,
> + int slave_type,
> + enum DEVAPC_PD_REG_TYPE pd_reg_type,
> + u32 index)
> +{
> + struct mtk_devapc_vio_info *vio_info = devapc_ctx->soc->vio_info;
> + u32 slave_type_num = devapc_ctx->soc->slave_type_num;
> + const u32 *devapc_pds = devapc_ctx->soc->devapc_pds;
devapc_pds = mt6873_devapc_pds;
> + void __iomem *reg;
> +
> + if (!devapc_pds)
Never happen.
> + return NULL;
> +
> + if ((slave_type < slave_type_num &&
> + index < vio_info->vio_mask_sta_num[slave_type]) &&
> + pd_reg_type < PD_REG_TYPE_NUM) {
Always true.
> + reg = devapc_ctx->devapc_pd_base[slave_type] +
> + devapc_pds[pd_reg_type];
> +
> + if (pd_reg_type == VIO_MASK || pd_reg_type == VIO_STA)
> + reg += 0x4 * index;
> +
> + } else {
> + pr_err(PFX "Out Of Boundary, slave_type:0x%x/pd_reg_type:0x%x/index:0x%x\n",
> + slave_type, pd_reg_type, index);
> + return NULL;
> + }
> +
> + return reg;
> +}
> +
[snip]
> +
> +/*
> + * start_devapc - initialize devapc status and start receiving interrupt
> + * while devapc violation is triggered.
> + */
> +static void start_devapc(struct mtk_devapc_context *devapc_ctx)
> +{
> + u32 slave_type_num = devapc_ctx->soc->slave_type_num;
> + const struct mtk_device_info **device_info;
> + const struct mtk_device_num *ndevices;
> + void __iomem *pd_vio_shift_sta_reg;
> + void __iomem *pd_apc_con_reg;
> + int slave_type, i, vio_idx, index;
> + u32 vio_shift_sta;
> +
> + ndevices = devapc_ctx->soc->ndevices;
ndevices = mtk6873_devices_num;
> +
> + device_info = devapc_ctx->soc->device_info;
> +
> + for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> + pd_apc_con_reg = mtk_devapc_pd_get(devapc_ctx, slave_type,
> + APC_CON, 0);
> + pd_vio_shift_sta_reg = mtk_devapc_pd_get(devapc_ctx, slave_type,
> + VIO_SHIFT_STA, 0);
> +
> + if (!pd_apc_con_reg || !pd_vio_shift_sta_reg || !device_info)
> + return;
> +
> + /* Clear DEVAPC violation status */
> + writel(BIT(31), pd_apc_con_reg);
> +
> + /* Clear violation shift status */
> + vio_shift_sta = readl(pd_vio_shift_sta_reg);
> + if (vio_shift_sta)
> + writel(vio_shift_sta, pd_vio_shift_sta_reg);
> +
> + /* Clear type 2 violation status */
> + check_type2_vio_status(devapc_ctx, slave_type, &vio_idx, &i);
> +
> + /* Clear violation status */
> + for (i = 0; i < ndevices[slave_type].vio_slave_num; i++) {
> + vio_idx = device_info[slave_type][i].vio_index;
> + if ((check_vio_status(devapc_ctx, slave_type, vio_idx)
> + == VIOLATION_TRIGGERED) &&
> + clear_vio_status(devapc_ctx, slave_type,
> + vio_idx)) {
> + pr_warn(PFX "Clear vio status failed, slave_type:0x%x, vio_index:0x%x\n",
> + slave_type, vio_idx);
> +
> + index = i;
> + mtk_devapc_dump_vio_dbg(devapc_ctx, slave_type,
> + &vio_idx, &index);
> + i = index - 1;
> + }
> +
> + mask_module_irq(devapc_ctx, slave_type, vio_idx, false);
> + }
> + }
> +}
> +
> +static DEFINE_SPINLOCK(devapc_lock);
Useless, so remove it.
> +
> +/*
> + * 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)
> +{
> + u32 slave_type_num = devapc_ctx->soc->slave_type_num;
> + 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;
> + u8 perm;
> +
> + spin_lock_irqsave(&devapc_lock, flags);
> +
> + device_info = devapc_ctx->soc->device_info;
> + vio_info = devapc_ctx->soc->vio_info;
> + 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(devapc_ctx, slave_type, &vio_idx,
> + &index))
> + if (!mtk_devapc_dump_vio_dbg(devapc_ctx, slave_type,
> + &vio_idx, &index))
> + continue;
> +
> + /* Ensure that violation info are written before
> + * further operations
> + */
> + smp_mb();
> +
> + mask_module_irq(devapc_ctx, slave_type, vio_idx, true);
> +
> + clear_vio_status(devapc_ctx, slave_type, vio_idx);
> +
> + perm = get_permission(devapc_ctx, slave_type, index,
> + vio_info->domain_id);
> +
> + vio_master = devapc_ctx->soc->master_get
> + (vio_info->master_id,
> + vio_info->vio_addr,
> + slave_type,
> + vio_info->shift_sta_bit,
> + vio_info->domain_id);
Call mt6873_bus_id_to_master() directly.
> +
> + if (!vio_master)
> + vio_master = "UNKNOWN_MASTER";
> +
> + pr_info(PFX "Violation - slave_type:0x%x, sys_index:0x%x, ctrl_index:0x%x, vio_index:0x%x\n",
> + slave_type,
> + device_info[slave_type][index].sys_index,
> + device_info[slave_type][index].ctrl_index,
> + device_info[slave_type][index].vio_index);
> +
> + pr_info(PFX "Violation Master: %s\n", vio_master);
> +
> + devapc_vio_reason(perm);
> +
> + mask_module_irq(devapc_ctx, slave_type, vio_idx, false);
> + }
> +
> + spin_unlock_irqrestore(&devapc_lock, flags);
> + return IRQ_HANDLED;
> +}
> +
[snip]
> +
> +/******************************************************************************
> + * VARIABLE DEFINATION
> + ******************************************************************************/
> +#define MOD_NO_IN_1_DEVAPC 16
> +#define VIOLATION_TRIGGERED 1
> +#define VIOLATION_MASKED 1
> +#define DEAD 0xdeadbeaf
> +#define PFX "[DEVAPC]: "
> +#define SLAVE_TYPE_NUM_MAX 5
SLAVE_TYPE_NUM is 4, why SLAVE_TYPE_NUM_MAX is 5?
> +
> +#define devapc_log(p, s, fmt, args...) \
> + do { \
> + typeof(p) (_p) = (p); \
> + ((_p) += scnprintf(_p, sizeof(s) - strlen(s), fmt, ##args)); \
> + } while (0)
Useless, so remove it.
> +
> +#define UNUSED(x) (void)(x)
Useless, so remove it.
> +
> +/******************************************************************************
> + * DATA STRUCTURE & FUNCTION DEFINATION
> + ******************************************************************************/
> +enum DEVAPC_PD_REG_TYPE {
> + VIO_MASK = 0,
> + VIO_STA,
> + VIO_DBG0,
> + VIO_DBG1,
> + VIO_DBG2,
> + APC_CON,
> + VIO_SHIFT_STA,
> + VIO_SHIFT_SEL,
> + VIO_SHIFT_CON,
> + PD_REG_TYPE_NUM,
> +};
> +
> +enum DEVAPC_UT_CMD {
> + DEVAPC_UT_DAPC_VIO = 1,
> + DEVAPC_UT_SRAM_VIO,
> +};
Useless, so remove it.
> +
> +enum DEVAPC_DOM_ID {
> + DOMAIN_0 = 0,
> + DOMAIN_1,
> + DOMAIN_2,
> + DOMAIN_3,
> + DOMAIN_4,
> + DOMAIN_5,
> + DOMAIN_6,
> + DOMAIN_7,
> + DOMAIN_8,
> + DOMAIN_9,
> + DOMAIN_10,
> + DOMAIN_11,
> + DOMAIN_12,
> + DOMAIN_13,
> + DOMAIN_14,
> + DOMAIN_15,
> + DOMAIN_OTHERS,
> +};
Useless, so remove it.
> +
> +enum SRAMROM_VIO {
> + ROM_VIOLATION = 0,
> + SRAM_VIOLATION,
> +};
> +
> +enum DEVAPC_PERM_TYPE {
> + NO_PROTECTION = 0,
> + SEC_RW_ONLY,
> + SEC_RW_NS_R,
> + FORBIDDEN,
> + PERM_TYPE_NUM,
> +};
> +
> +struct mtk_devapc_dbg_status {
> + bool enable_ut;
> + bool enable_dapc; /* dump APC */
> +};
Useless, so remove it.
Regards,
Chun-Kuang.
Hi Chun-Kuang,
Thanks for your quick feedback.
On Sat, 2020-06-20 at 00:25 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu <[email protected]> 於 2020年6月19日 週五 下午6:01寫道：
> >
> > 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 <[email protected]>
> > ---
>
> [snip]
>
> > +
> > +/*
> > + * mtk_devapc_pd_get - get devapc pd_types of register address.
> > + *
> > + * Returns the value of reg addr
> > + */
> > +static void __iomem *mtk_devapc_pd_get(struct mtk_devapc_context *devapc_ctx,
> > + int slave_type,
> > + enum DEVAPC_PD_REG_TYPE pd_reg_type,
> > + u32 index)
> > +{
> > + struct mtk_devapc_vio_info *vio_info = devapc_ctx->soc->vio_info;
> > + u32 slave_type_num = devapc_ctx->soc->slave_type_num;
> > + const u32 *devapc_pds = devapc_ctx->soc->devapc_pds;
>
> devapc_pds = mt6873_devapc_pds;
Are you saying all platform related variables & functions should assign
& call it directly in this common flow?
I don't think it's a good idea to go backwards since we already extract
the common out of it.
>
>
> > + void __iomem *reg;
> > +
> > + if (!devapc_pds)
>
> Never happen.
>
> > + return NULL;
> > +
> > + if ((slave_type < slave_type_num &&
> > + index < vio_info->vio_mask_sta_num[slave_type]) &&
> > + pd_reg_type < PD_REG_TYPE_NUM) {
>
> Always true.
>
> > + reg = devapc_ctx->devapc_pd_base[slave_type] +
> > + devapc_pds[pd_reg_type];
> > +
> > + if (pd_reg_type == VIO_MASK || pd_reg_type == VIO_STA)
> > + reg += 0x4 * index;
> > +
> > + } else {
> > + pr_err(PFX "Out Of Boundary, slave_type:0x%x/pd_reg_type:0x%x/index:0x%x\n",
> > + slave_type, pd_reg_type, index);
> > + return NULL;
> > + }
> > +
> > + return reg;
> > +}
> > +
>
> [snip]
>
> > +
> > +/*
> > + * start_devapc - initialize devapc status and start receiving interrupt
> > + * while devapc violation is triggered.
> > + */
> > +static void start_devapc(struct mtk_devapc_context *devapc_ctx)
> > +{
> > + u32 slave_type_num = devapc_ctx->soc->slave_type_num;
> > + const struct mtk_device_info **device_info;
> > + const struct mtk_device_num *ndevices;
> > + void __iomem *pd_vio_shift_sta_reg;
> > + void __iomem *pd_apc_con_reg;
> > + int slave_type, i, vio_idx, index;
> > + u32 vio_shift_sta;
> > +
> > + ndevices = devapc_ctx->soc->ndevices;
>
> ndevices = mtk6873_devices_num;
>
>
> > +
> > + device_info = devapc_ctx->soc->device_info;
> > +
> > + for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> > + pd_apc_con_reg = mtk_devapc_pd_get(devapc_ctx, slave_type,
> > + APC_CON, 0);
> > + pd_vio_shift_sta_reg = mtk_devapc_pd_get(devapc_ctx, slave_type,
> > + VIO_SHIFT_STA, 0);
> > +
> > + if (!pd_apc_con_reg || !pd_vio_shift_sta_reg || !device_info)
> > + return;
> > +
> > + /* Clear DEVAPC violation status */
> > + writel(BIT(31), pd_apc_con_reg);
> > +
> > + /* Clear violation shift status */
> > + vio_shift_sta = readl(pd_vio_shift_sta_reg);
> > + if (vio_shift_sta)
> > + writel(vio_shift_sta, pd_vio_shift_sta_reg);
> > +
> > + /* Clear type 2 violation status */
> > + check_type2_vio_status(devapc_ctx, slave_type, &vio_idx, &i);
> > +
> > + /* Clear violation status */
> > + for (i = 0; i < ndevices[slave_type].vio_slave_num; i++) {
> > + vio_idx = device_info[slave_type][i].vio_index;
> > + if ((check_vio_status(devapc_ctx, slave_type, vio_idx)
> > + == VIOLATION_TRIGGERED) &&
> > + clear_vio_status(devapc_ctx, slave_type,
> > + vio_idx)) {
> > + pr_warn(PFX "Clear vio status failed, slave_type:0x%x, vio_index:0x%x\n",
> > + slave_type, vio_idx);
> > +
> > + index = i;
> > + mtk_devapc_dump_vio_dbg(devapc_ctx, slave_type,
> > + &vio_idx, &index);
> > + i = index - 1;
> > + }
> > +
> > + mask_module_irq(devapc_ctx, slave_type, vio_idx, false);
> > + }
> > + }
> > +}
> > +
> > +static DEFINE_SPINLOCK(devapc_lock);
>
> Useless, so remove it.
We use devapc_lock in below isr, what do you mean useless?
>
> > +
> > +/*
> > + * 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)
> > +{
> > + u32 slave_type_num = devapc_ctx->soc->slave_type_num;
> > + 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;
> > + u8 perm;
> > +
> > + spin_lock_irqsave(&devapc_lock, flags);
> > +
> > + device_info = devapc_ctx->soc->device_info;
> > + vio_info = devapc_ctx->soc->vio_info;
> > + 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(devapc_ctx, slave_type, &vio_idx,
> > + &index))
> > + if (!mtk_devapc_dump_vio_dbg(devapc_ctx, slave_type,
> > + &vio_idx, &index))
> > + continue;
> > +
> > + /* Ensure that violation info are written before
> > + * further operations
> > + */
> > + smp_mb();
> > +
> > + mask_module_irq(devapc_ctx, slave_type, vio_idx, true);
> > +
> > + clear_vio_status(devapc_ctx, slave_type, vio_idx);
> > +
> > + perm = get_permission(devapc_ctx, slave_type, index,
> > + vio_info->domain_id);
> > +
> > + vio_master = devapc_ctx->soc->master_get
> > + (vio_info->master_id,
> > + vio_info->vio_addr,
> > + slave_type,
> > + vio_info->shift_sta_bit,
> > + vio_info->domain_id);
>
> Call mt6873_bus_id_to_master() directly.
>
> > +
> > + if (!vio_master)
> > + vio_master = "UNKNOWN_MASTER";
> > +
> > + pr_info(PFX "Violation - slave_type:0x%x, sys_index:0x%x, ctrl_index:0x%x, vio_index:0x%x\n",
> > + slave_type,
> > + device_info[slave_type][index].sys_index,
> > + device_info[slave_type][index].ctrl_index,
> > + device_info[slave_type][index].vio_index);
> > +
> > + pr_info(PFX "Violation Master: %s\n", vio_master);
> > +
> > + devapc_vio_reason(perm);
> > +
> > + mask_module_irq(devapc_ctx, slave_type, vio_idx, false);
> > + }
> > +
> > + spin_unlock_irqrestore(&devapc_lock, flags);
> > + return IRQ_HANDLED;
> > +}
> > +
[snip]
Hi, Neal:
Neal Liu <[email protected]> 於 2020年6月20日 週六 上午11:18寫道：
>
> Hi Chun-Kuang,
>
> Thanks for your quick feedback.
>
> On Sat, 2020-06-20 at 00:25 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <[email protected]> 於 2020年6月19日 週五 下午6:01寫道：
> > >
> > > 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 <[email protected]>
> > > ---
> >
> > [snip]
> >
> > > +
> > > +/*
> > > + * mtk_devapc_pd_get - get devapc pd_types of register address.
> > > + *
> > > + * Returns the value of reg addr
> > > + */
> > > +static void __iomem *mtk_devapc_pd_get(struct mtk_devapc_context *devapc_ctx,
> > > + int slave_type,
> > > + enum DEVAPC_PD_REG_TYPE pd_reg_type,
> > > + u32 index)
> > > +{
> > > + struct mtk_devapc_vio_info *vio_info = devapc_ctx->soc->vio_info;
> > > + u32 slave_type_num = devapc_ctx->soc->slave_type_num;
> > > + const u32 *devapc_pds = devapc_ctx->soc->devapc_pds;
> >
> > devapc_pds = mt6873_devapc_pds;
>
> Are you saying all platform related variables & functions should assign
> & call it directly in this common flow?
> I don't think it's a good idea to go backwards since we already extract
> the common out of it.
I think we should "do one thing in one patch". When you mix two things
into one patch, how does reviewer know each modification belong to
first thing or second thing? For supporting multiple SoC, the patches
sequence look like this:
Patch 1: Add support SoC 1.
Patch 2: Abstract function and variable for SoC 2.
Patch 3: Add support SoC 2.
Patch 4: Abstract function and variable for SoC 3.
Patch 5: Add support SoC 3.
Patch 6: Abstract function and variable for SoC 4.
Patch 7: Add support SoC 4.
In patch 1, you should not do any thing about abstraction, but you
want to merge patch 2, 4, 6 into this patch, so this patch's title
should be "Add support SoC 1 and abstract function and varible for SoC
2, SoC 3, and SoC 4"
>
> >
> >
> > > + void __iomem *reg;
> > > +
> > > + if (!devapc_pds)
> >
> > Never happen.
> >
> > > + return NULL;
> > > +
> > > + if ((slave_type < slave_type_num &&
> > > + index < vio_info->vio_mask_sta_num[slave_type]) &&
> > > + pd_reg_type < PD_REG_TYPE_NUM) {
> >
> > Always true.
> >
> > > + reg = devapc_ctx->devapc_pd_base[slave_type] +
> > > + devapc_pds[pd_reg_type];
> > > +
> > > + if (pd_reg_type == VIO_MASK || pd_reg_type == VIO_STA)
> > > + reg += 0x4 * index;
> > > +
> > > + } else {
> > > + pr_err(PFX "Out Of Boundary, slave_type:0x%x/pd_reg_type:0x%x/index:0x%x\n",
> > > + slave_type, pd_reg_type, index);
> > > + return NULL;
> > > + }
> > > +
> > > + return reg;
> > > +}
> > > +
> >
> > [snip]
> >
> > > +
> > > +/*
> > > + * start_devapc - initialize devapc status and start receiving interrupt
> > > + * while devapc violation is triggered.
> > > + */
> > > +static void start_devapc(struct mtk_devapc_context *devapc_ctx)
> > > +{
> > > + u32 slave_type_num = devapc_ctx->soc->slave_type_num;
> > > + const struct mtk_device_info **device_info;
> > > + const struct mtk_device_num *ndevices;
> > > + void __iomem *pd_vio_shift_sta_reg;
> > > + void __iomem *pd_apc_con_reg;
> > > + int slave_type, i, vio_idx, index;
> > > + u32 vio_shift_sta;
> > > +
> > > + ndevices = devapc_ctx->soc->ndevices;
> >
> > ndevices = mtk6873_devices_num;
> >
> >
> > > +
> > > + device_info = devapc_ctx->soc->device_info;
> > > +
> > > + for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> > > + pd_apc_con_reg = mtk_devapc_pd_get(devapc_ctx, slave_type,
> > > + APC_CON, 0);
> > > + pd_vio_shift_sta_reg = mtk_devapc_pd_get(devapc_ctx, slave_type,
> > > + VIO_SHIFT_STA, 0);
> > > +
> > > + if (!pd_apc_con_reg || !pd_vio_shift_sta_reg || !device_info)
> > > + return;
> > > +
> > > + /* Clear DEVAPC violation status */
> > > + writel(BIT(31), pd_apc_con_reg);
> > > +
> > > + /* Clear violation shift status */
> > > + vio_shift_sta = readl(pd_vio_shift_sta_reg);
> > > + if (vio_shift_sta)
> > > + writel(vio_shift_sta, pd_vio_shift_sta_reg);
> > > +
> > > + /* Clear type 2 violation status */
> > > + check_type2_vio_status(devapc_ctx, slave_type, &vio_idx, &i);
> > > +
> > > + /* Clear violation status */
> > > + for (i = 0; i < ndevices[slave_type].vio_slave_num; i++) {
> > > + vio_idx = device_info[slave_type][i].vio_index;
> > > + if ((check_vio_status(devapc_ctx, slave_type, vio_idx)
> > > + == VIOLATION_TRIGGERED) &&
> > > + clear_vio_status(devapc_ctx, slave_type,
> > > + vio_idx)) {
> > > + pr_warn(PFX "Clear vio status failed, slave_type:0x%x, vio_index:0x%x\n",
> > > + slave_type, vio_idx);
> > > +
> > > + index = i;
> > > + mtk_devapc_dump_vio_dbg(devapc_ctx, slave_type,
> > > + &vio_idx, &index);
> > > + i = index - 1;
> > > + }
> > > +
> > > + mask_module_irq(devapc_ctx, slave_type, vio_idx, false);
> > > + }
> > > + }
> > > +}
> > > +
> > > +static DEFINE_SPINLOCK(devapc_lock);
> >
> > Useless, so remove it.
>
> We use devapc_lock in below isr, what do you mean useless?
We use spinlock because a thread context and irq context would access
the same resource, but where is the thread context? If the thread
context exist in another patch, move this spinlock to that patch.
Regards,
Chun-Kuang.
>
> >
> > > +
> > > +/*
> > > + * 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)
> > > +{
> > > + u32 slave_type_num = devapc_ctx->soc->slave_type_num;
> > > + 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;
> > > + u8 perm;
> > > +
> > > + spin_lock_irqsave(&devapc_lock, flags);
> > > +
> > > + device_info = devapc_ctx->soc->device_info;
> > > + vio_info = devapc_ctx->soc->vio_info;
> > > + 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(devapc_ctx, slave_type, &vio_idx,
> > > + &index))
> > > + if (!mtk_devapc_dump_vio_dbg(devapc_ctx, slave_type,
> > > + &vio_idx, &index))
> > > + continue;
> > > +
> > > + /* Ensure that violation info are written before
> > > + * further operations
> > > + */
> > > + smp_mb();
> > > +
> > > + mask_module_irq(devapc_ctx, slave_type, vio_idx, true);
> > > +
> > > + clear_vio_status(devapc_ctx, slave_type, vio_idx);
> > > +
> > > + perm = get_permission(devapc_ctx, slave_type, index,
> > > + vio_info->domain_id);
> > > +
> > > + vio_master = devapc_ctx->soc->master_get
> > > + (vio_info->master_id,
> > > + vio_info->vio_addr,
> > > + slave_type,
> > > + vio_info->shift_sta_bit,
> > > + vio_info->domain_id);
> >
> > Call mt6873_bus_id_to_master() directly.
> >
> > > +
> > > + if (!vio_master)
> > > + vio_master = "UNKNOWN_MASTER";
> > > +
> > > + pr_info(PFX "Violation - slave_type:0x%x, sys_index:0x%x, ctrl_index:0x%x, vio_index:0x%x\n",
> > > + slave_type,
> > > + device_info[slave_type][index].sys_index,
> > > + device_info[slave_type][index].ctrl_index,
> > > + device_info[slave_type][index].vio_index);
> > > +
> > > + pr_info(PFX "Violation Master: %s\n", vio_master);
> > > +
> > > + devapc_vio_reason(perm);
> > > +
> > > + mask_module_irq(devapc_ctx, slave_type, vio_idx, false);
> > > + }
> > > +
> > > + spin_unlock_irqrestore(&devapc_lock, flags);
> > > + return IRQ_HANDLED;
> > > +}
> > > +
>
> [snip]
>
Hi Chun-Kuang,
On Sun, 2020-06-21 at 07:36 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu <[email protected]> 於 2020年6月20日 週六 上午11:18寫道：
> >
> > Hi Chun-Kuang,
> >
> > Thanks for your quick feedback.
> >
> > On Sat, 2020-06-20 at 00:25 +0800, Chun-Kuang Hu wrote:
> > > Hi, Neal:
> > >
> > > Neal Liu <[email protected]> 於 2020年6月19日 週五 下午6:01寫道：
> > > >
> > > > 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 <[email protected]>
> > > > ---
> > >
> > > [snip]
> > >
> > > > +
> > > > +/*
> > > > + * mtk_devapc_pd_get - get devapc pd_types of register address.
> > > > + *
> > > > + * Returns the value of reg addr
> > > > + */
> > > > +static void __iomem *mtk_devapc_pd_get(struct mtk_devapc_context *devapc_ctx,
> > > > + int slave_type,
> > > > + enum DEVAPC_PD_REG_TYPE pd_reg_type,
> > > > + u32 index)
> > > > +{
> > > > + struct mtk_devapc_vio_info *vio_info = devapc_ctx->soc->vio_info;
> > > > + u32 slave_type_num = devapc_ctx->soc->slave_type_num;
> > > > + const u32 *devapc_pds = devapc_ctx->soc->devapc_pds;
> > >
> > > devapc_pds = mt6873_devapc_pds;
> >
> > Are you saying all platform related variables & functions should assign
> > & call it directly in this common flow?
> > I don't think it's a good idea to go backwards since we already extract
> > the common out of it.
>
> I think we should "do one thing in one patch". When you mix two things
> into one patch, how does reviewer know each modification belong to
> first thing or second thing? For supporting multiple SoC, the patches
> sequence look like this:
>
> Patch 1: Add support SoC 1.
> Patch 2: Abstract function and variable for SoC 2.
> Patch 3: Add support SoC 2.
> Patch 4: Abstract function and variable for SoC 3.
> Patch 5: Add support SoC 3.
> Patch 6: Abstract function and variable for SoC 4.
> Patch 7: Add support SoC 4.
>
> In patch 1, you should not do any thing about abstraction, but you
> want to merge patch 2, 4, 6 into this patch, so this patch's title
> should be "Add support SoC 1 and abstract function and varible for SoC
> 2, SoC 3, and SoC 4"
>
Okay, I'll try to split driver to multiple patches for different
functionality. Thanks for suggestion.
> >
> > >
> > >
> > > > + void __iomem *reg;
> > > > +
> > > > + if (!devapc_pds)
> > >
> > > Never happen.
> > >
> > > > + return NULL;
> > > > +
> > > > + if ((slave_type < slave_type_num &&
> > > > + index < vio_info->vio_mask_sta_num[slave_type]) &&
> > > > + pd_reg_type < PD_REG_TYPE_NUM) {
> > >
> > > Always true.
> > >
> > > > + reg = devapc_ctx->devapc_pd_base[slave_type] +
> > > > + devapc_pds[pd_reg_type];
> > > > +
> > > > + if (pd_reg_type == VIO_MASK || pd_reg_type == VIO_STA)
> > > > + reg += 0x4 * index;
> > > > +
> > > > + } else {
> > > > + pr_err(PFX "Out Of Boundary, slave_type:0x%x/pd_reg_type:0x%x/index:0x%x\n",
> > > > + slave_type, pd_reg_type, index);
> > > > + return NULL;
> > > > + }
> > > > +
> > > > + return reg;
> > > > +}
> > > > +
> > >
> > > [snip]
> > >
> > > > +
> > > > +/*
> > > > + * start_devapc - initialize devapc status and start receiving interrupt
> > > > + * while devapc violation is triggered.
> > > > + */
> > > > +static void start_devapc(struct mtk_devapc_context *devapc_ctx)
> > > > +{
> > > > + u32 slave_type_num = devapc_ctx->soc->slave_type_num;
> > > > + const struct mtk_device_info **device_info;
> > > > + const struct mtk_device_num *ndevices;
> > > > + void __iomem *pd_vio_shift_sta_reg;
> > > > + void __iomem *pd_apc_con_reg;
> > > > + int slave_type, i, vio_idx, index;
> > > > + u32 vio_shift_sta;
> > > > +
> > > > + ndevices = devapc_ctx->soc->ndevices;
> > >
> > > ndevices = mtk6873_devices_num;
> > >
> > >
> > > > +
> > > > + device_info = devapc_ctx->soc->device_info;
> > > > +
> > > > + for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> > > > + pd_apc_con_reg = mtk_devapc_pd_get(devapc_ctx, slave_type,
> > > > + APC_CON, 0);
> > > > + pd_vio_shift_sta_reg = mtk_devapc_pd_get(devapc_ctx, slave_type,
> > > > + VIO_SHIFT_STA, 0);
> > > > +
> > > > + if (!pd_apc_con_reg || !pd_vio_shift_sta_reg || !device_info)
> > > > + return;
> > > > +
> > > > + /* Clear DEVAPC violation status */
> > > > + writel(BIT(31), pd_apc_con_reg);
> > > > +
> > > > + /* Clear violation shift status */
> > > > + vio_shift_sta = readl(pd_vio_shift_sta_reg);
> > > > + if (vio_shift_sta)
> > > > + writel(vio_shift_sta, pd_vio_shift_sta_reg);
> > > > +
> > > > + /* Clear type 2 violation status */
> > > > + check_type2_vio_status(devapc_ctx, slave_type, &vio_idx, &i);
> > > > +
> > > > + /* Clear violation status */
> > > > + for (i = 0; i < ndevices[slave_type].vio_slave_num; i++) {
> > > > + vio_idx = device_info[slave_type][i].vio_index;
> > > > + if ((check_vio_status(devapc_ctx, slave_type, vio_idx)
> > > > + == VIOLATION_TRIGGERED) &&
> > > > + clear_vio_status(devapc_ctx, slave_type,
> > > > + vio_idx)) {
> > > > + pr_warn(PFX "Clear vio status failed, slave_type:0x%x, vio_index:0x%x\n",
> > > > + slave_type, vio_idx);
> > > > +
> > > > + index = i;
> > > > + mtk_devapc_dump_vio_dbg(devapc_ctx, slave_type,
> > > > + &vio_idx, &index);
> > > > + i = index - 1;
> > > > + }
> > > > +
> > > > + mask_module_irq(devapc_ctx, slave_type, vio_idx, false);
> > > > + }
> > > > + }
> > > > +}
> > > > +
> > > > +static DEFINE_SPINLOCK(devapc_lock);
> > >
> > > Useless, so remove it.
> >
> > We use devapc_lock in below isr, what do you mean useless?
>
> We use spinlock because a thread context and irq context would access
> the same resource, but where is the thread context? If the thread
> context exist in another patch, move this spinlock to that patch.
>
> Regards,
> Chun-Kuang.
>
> >
> > >
> > > > +
> > > > +/*
> > > > + * 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)
> > > > +{
> > > > + u32 slave_type_num = devapc_ctx->soc->slave_type_num;
> > > > + 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;
> > > > + u8 perm;
> > > > +
> > > > + spin_lock_irqsave(&devapc_lock, flags);
> > > > +
> > > > + device_info = devapc_ctx->soc->device_info;
> > > > + vio_info = devapc_ctx->soc->vio_info;
> > > > + 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(devapc_ctx, slave_type, &vio_idx,
> > > > + &index))
> > > > + if (!mtk_devapc_dump_vio_dbg(devapc_ctx, slave_type,
> > > > + &vio_idx, &index))
> > > > + continue;
> > > > +
> > > > + /* Ensure that violation info are written before
> > > > + * further operations
> > > > + */
> > > > + smp_mb();
> > > > +
> > > > + mask_module_irq(devapc_ctx, slave_type, vio_idx, true);
> > > > +
> > > > + clear_vio_status(devapc_ctx, slave_type, vio_idx);
> > > > +
> > > > + perm = get_permission(devapc_ctx, slave_type, index,
> > > > + vio_info->domain_id);
> > > > +
> > > > + vio_master = devapc_ctx->soc->master_get
> > > > + (vio_info->master_id,
> > > > + vio_info->vio_addr,
> > > > + slave_type,
> > > > + vio_info->shift_sta_bit,
> > > > + vio_info->domain_id);
> > >
> > > Call mt6873_bus_id_to_master() directly.
> > >
> > > > +
> > > > + if (!vio_master)
> > > > + vio_master = "UNKNOWN_MASTER";
> > > > +
> > > > + pr_info(PFX "Violation - slave_type:0x%x, sys_index:0x%x, ctrl_index:0x%x, vio_index:0x%x\n",
> > > > + slave_type,
> > > > + device_info[slave_type][index].sys_index,
> > > > + device_info[slave_type][index].ctrl_index,
> > > > + device_info[slave_type][index].vio_index);
> > > > +
> > > > + pr_info(PFX "Violation Master: %s\n", vio_master);
> > > > +
> > > > + devapc_vio_reason(perm);
> > > > +
> > > > + mask_module_irq(devapc_ctx, slave_type, vio_idx, false);
> > > > + }
> > > > +
> > > > + spin_unlock_irqrestore(&devapc_lock, flags);
> > > > + return IRQ_HANDLED;
> > > > +}
> > > > +
> >
> > [snip]
> >