Re: [PATCH v2 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
Hi, Neal:
Neal Liu 於 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
> ---
[snip]
> +
> +/*
> + * sramrom_vio_handler - clean sramrom violation & print violation
> information
> + * for debugging.
> + */
> +static void sramrom_vio_handler(struct mtk_devapc_context *devapc_ctx)
> +{
> + 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 = devapc_ctx->soc->sramrom_sec_vios;
> + vio_info = devapc_ctx->soc->vio_info;
> +
> + arm_smccc_smc(MTK_SIP_KERNEL_CLR_SRAMROM_VIO,
> + 0, 0, 0, 0, 0, 0, 0, );
> +
This irq handler call arm_smccc_smc() to get into TrustZone, why not
let the whole irq handler in TrustZone?
Regards,
Chun-Kuang.
> + sramrom_vio = res.a0;
> + sramrom_vio_sta = res.a1;
> + vio_info->vio_addr = res.a2;
> +
> + if (sramrom_vio == SRAM_VIOLATION)
> + pr_info(PFX "SRAM violation is triggered\n");
> + else if (sramrom_vio == ROM_VIOLATION)
> + pr_info(PFX "ROM violation is triggered\n");
> + 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;
> +
> + if (rw)
> + vio_info->write = 1;
> + else
> + vio_info->read = 1;
> +
> + pr_info(PFX "%s: master_id:0x%x, domain_id:0x%x, rw:%s,
> vio_addr:0x%x\n",
> + __func__, vio_info->master_id, vio_info->domain_id,
> + rw ? "Write" : "Read", vio_info->vio_addr);
> +}
> +
Re: [PATCH v2 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
On Mon, 2020-06-22 at 12:05 +0200, Matthias Brugger wrote: > > On 19/06/2020 11:42, Neal Liu wrote: > > 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. > > Why don't we handle this in the TrustZone directly? There is no reason that it should be handled in TrustZone only. And it has no security issue to handle it in Normal World. > > > > > Signed-off-by: Neal Liu > > This is a really big patch and it will be difficult to review. > > Please do review the code and delete all the data structures that are not used > (I already found two). > > Please also check for code duplication (e.g. check_vio_mask and > check_vio_status > are using 99% the same code). > > Please review your data structures and try to group the information in logical > structs. For example I don't understand why we need mtk_devapc_context. It > seems > to me that all the values in there are SoC specific. > > Think how you could split the driver up in several commits adding more > functionality by each commit. With a good commit message it will be easier to > understand what the code does. > Thanks for your all suggestion, it's really helpful. I'll review again and try to make patches more readable. > On which SoC is the device used? I don't see any support for mt6873 in the > kernel or the mailing list. Why do we want to support this driver upstream if > it's not usable at all? > Our mt6873 upstream tasks is on going, we'll send basic platform drivers ASAP. > Try to understand how the upstream kernel device model works. It looks to me > as > if all the information is hardcoded. Would it make sense to pass some of the > information via DT? Sorry for the generic question, but I hadn't had the time > to > got through the whole driver to understand what it does. > > Regards, > Matthias > > > --- > > drivers/soc/mediatek/Kconfig |6 + > > drivers/soc/mediatek/Makefile |1 + > > drivers/soc/mediatek/devapc/Kconfig | 25 + > > drivers/soc/mediatek/devapc/Makefile | 13 + > > drivers/soc/mediatek/devapc/devapc-mt6873.c | 1652 > > + > > drivers/soc/mediatek/devapc/devapc-mt6873.h | 111 ++ > > drivers/soc/mediatek/devapc/devapc-mtk-multi-ao.c | 756 ++ > > drivers/soc/mediatek/devapc/devapc-mtk-multi-ao.h | 182 +++ > > 8 files changed, 2746 insertions(+) > > create mode 100644 drivers/soc/mediatek/devapc/Kconfig > > create mode 100644 drivers/soc/mediatek/devapc/Makefile > > create mode 100644 drivers/soc/mediatek/devapc/devapc-mt6873.c > > create mode 100644 drivers/soc/mediatek/devapc/devapc-mt6873.h > > create mode 100644 drivers/soc/mediatek/devapc/devapc-mtk-multi-ao.c > > create mode 100644 drivers/soc/mediatek/devapc/devapc-mtk-multi-ao.h > > > > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig > > index 59a56cd..2c9ad1f 100644 > > --- a/drivers/soc/mediatek/Kconfig > > +++ b/drivers/soc/mediatek/Kconfig > > @@ -51,4 +51,10 @@ config MTK_MMSYS > > Say yes here to add support for the MediaTek Multimedia > > Subsystem (MMSYS). > > > > +menu "Security" > > + > > +source "drivers/soc/mediatek/devapc/Kconfig" > > + > > +endmenu # Security > > + > > endmenu > > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile > > index 01f9f87..d6717a81 100644 > > --- a/drivers/soc/mediatek/Makefile > > +++ b/drivers/soc/mediatek/Makefile > > @@ -1,5 +1,6 @@ > > # SPDX-License-Identifier: GPL-2.0-only > > obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o > > +obj-$(CONFIG_MTK_DEVAPC) += devapc/ > > obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o > > obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o > > obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o > > diff --git a/drivers/soc/mediatek/devapc/Kconfig > > b/drivers/soc/mediatek/devapc/Kconfig > > new file mode 100644 > > index 000..9428360 > > --- /dev/null > > +++ b/drivers/soc/mediatek/devapc/Kconfig > > @@ -0,0 +1,25 @@ > > +config MTK_DEVAPC > > + tristate "Mediatek Device APC Support" > > + help > > + Device APC is a kernel driver controlling internal device security. > > + If someone tries to access a device, which is not allowed by the > > + device, it cannot access the device and will get a violation > > + interrupt. Device APC prevents malicious access to internal devices. > > + > > +config DEVAPC_ARCH_MULTI > > + tristate "Mediatek Device APC driver architecture multi" > > + help > > + Say yes here to enable support Mediatek > > + Device APC driver which is based on
Re: [PATCH v2 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
Hi Chun-Kuang,
On Sun, 2020-06-21 at 07:36 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu 於 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 於 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
> > > > ---
> > >
> > > [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,
> > > > +
Re: [PATCH v2 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
On 19/06/2020 11:42, Neal Liu wrote: > 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. Why don't we handle this in the TrustZone directly? > > Signed-off-by: Neal Liu This is a really big patch and it will be difficult to review. Please do review the code and delete all the data structures that are not used (I already found two). Please also check for code duplication (e.g. check_vio_mask and check_vio_status are using 99% the same code). Please review your data structures and try to group the information in logical structs. For example I don't understand why we need mtk_devapc_context. It seems to me that all the values in there are SoC specific. Think how you could split the driver up in several commits adding more functionality by each commit. With a good commit message it will be easier to understand what the code does. On which SoC is the device used? I don't see any support for mt6873 in the kernel or the mailing list. Why do we want to support this driver upstream if it's not usable at all? Try to understand how the upstream kernel device model works. It looks to me as if all the information is hardcoded. Would it make sense to pass some of the information via DT? Sorry for the generic question, but I hadn't had the time to got through the whole driver to understand what it does. Regards, Matthias > --- > drivers/soc/mediatek/Kconfig |6 + > drivers/soc/mediatek/Makefile |1 + > drivers/soc/mediatek/devapc/Kconfig | 25 + > drivers/soc/mediatek/devapc/Makefile | 13 + > drivers/soc/mediatek/devapc/devapc-mt6873.c | 1652 > + > drivers/soc/mediatek/devapc/devapc-mt6873.h | 111 ++ > drivers/soc/mediatek/devapc/devapc-mtk-multi-ao.c | 756 ++ > drivers/soc/mediatek/devapc/devapc-mtk-multi-ao.h | 182 +++ > 8 files changed, 2746 insertions(+) > create mode 100644 drivers/soc/mediatek/devapc/Kconfig > create mode 100644 drivers/soc/mediatek/devapc/Makefile > create mode 100644 drivers/soc/mediatek/devapc/devapc-mt6873.c > create mode 100644 drivers/soc/mediatek/devapc/devapc-mt6873.h > create mode 100644 drivers/soc/mediatek/devapc/devapc-mtk-multi-ao.c > create mode 100644 drivers/soc/mediatek/devapc/devapc-mtk-multi-ao.h > > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig > index 59a56cd..2c9ad1f 100644 > --- a/drivers/soc/mediatek/Kconfig > +++ b/drivers/soc/mediatek/Kconfig > @@ -51,4 +51,10 @@ config MTK_MMSYS > Say yes here to add support for the MediaTek Multimedia > Subsystem (MMSYS). > > +menu "Security" > + > +source "drivers/soc/mediatek/devapc/Kconfig" > + > +endmenu # Security > + > endmenu > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile > index 01f9f87..d6717a81 100644 > --- a/drivers/soc/mediatek/Makefile > +++ b/drivers/soc/mediatek/Makefile > @@ -1,5 +1,6 @@ > # SPDX-License-Identifier: GPL-2.0-only > obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o > +obj-$(CONFIG_MTK_DEVAPC) += devapc/ > obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o > obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o > obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o > diff --git a/drivers/soc/mediatek/devapc/Kconfig > b/drivers/soc/mediatek/devapc/Kconfig > new file mode 100644 > index 000..9428360 > --- /dev/null > +++ b/drivers/soc/mediatek/devapc/Kconfig > @@ -0,0 +1,25 @@ > +config MTK_DEVAPC > + tristate "Mediatek Device APC Support" > + help > + Device APC is a kernel driver controlling internal device security. > + If someone tries to access a device, which is not allowed by the > + device, it cannot access the device and will get a violation > + interrupt. Device APC prevents malicious access to internal devices. > + > +config DEVAPC_ARCH_MULTI > + tristate "Mediatek Device APC driver architecture multi" > + help > + Say yes here to enable support Mediatek > + Device APC driver which is based on Infra > + architecture. > + This architecture supports multiple Infra AO. > + > +config DEVAPC_MT6873 > + tristate "Mediatek MT6873 Device APC driver" > + select MTK_DEVAPC > + select DEVAPC_ARCH_MULTI > + help > + Say yes here to enable support Mediatek MT6873 > + Device APC driver. > + This driver is combined with DEVAPC_ARCH_MULTI for > + common handle flow. > diff --git a/drivers/soc/mediatek/devapc/Makefile > b/drivers/soc/mediatek/devapc/Makefile > new file mode 100644 > index 000..bd471f2 > --- /dev/null > +++
Re: [PATCH v2 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
Hi, Neal:
Neal Liu 於 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 於 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
> > > ---
> >
> > [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)
> > > +
Re: [PATCH v2 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
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 於 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
> > ---
>
> [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, _idx,
> > );
> > +
> > + /* 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,
> >
Re: [PATCH v2 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
Hi, Neal:
Neal Liu 於 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
> ---
[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, _idx, );
> +
> + /* 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,
> + _idx, );
> + 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.
> + */
>
