Re: [PATCH v3 1/4] firmware: qcom_scm-64: Add atomic version of qcom_scm_call
On Tue, Aug 6, 2019 at 3:58 AM Bjorn Andersson
wrote:
>
> On Wed 19 Jun 04:34 PDT 2019, Vivek Gautam wrote:
>
> > On Tue, Jun 18, 2019 at 11:25 PM Will Deacon wrote:
> > >
> > > On Wed, Jun 12, 2019 at 12:45:51PM +0530, Vivek Gautam wrote:
> > > > There are scnenarios where drivers are required to make a
> > > > scm call in atomic context, such as in one of the qcom's
> > > > arm-smmu-500 errata [1].
> > > >
> > > > [1] ("https://source.codeaurora.org/quic/la/kernel/msm-4.9/commit/
> > > > drivers/iommu/arm-smmu.c?h=CogSystems-msm-49/
> > > > msm-4.9=da765c6c75266b38191b38ef086274943f353ea7")
> > > >
> > > > Signed-off-by: Vivek Gautam
> > > > Reviewed-by: Bjorn Andersson
> > > > ---
> > > > drivers/firmware/qcom_scm-64.c | 136
> > > > -
> > > > 1 file changed, 92 insertions(+), 44 deletions(-)
> > > >
> > > > diff --git a/drivers/firmware/qcom_scm-64.c
> > > > b/drivers/firmware/qcom_scm-64.c
> > > > index 91d5ad7cf58b..b6dca32c5ac4 100644
> > > > --- a/drivers/firmware/qcom_scm-64.c
> > > > +++ b/drivers/firmware/qcom_scm-64.c
> >
> > [snip]
> >
> > > > +
> > > > +static void qcom_scm_call_do(const struct qcom_scm_desc *desc,
> > > > + struct arm_smccc_res *res, u32 fn_id,
> > > > + u64 x5, bool atomic)
> > > > +{
> > >
> > > Maybe pass in the call type (ARM_SMCCC_FAST_CALL vs ARM_SMCCC_STD_CALL)
> > > instead of "bool atomic"? Would certainly make the callsites easier to
> > > understand.
> >
> > Sure, will do that.
> >
> > >
> > > > + int retry_count = 0;
> > > > +
> > > > + if (!atomic) {
> > > > + do {
> > > > + mutex_lock(_scm_lock);
> > > > +
> > > > + __qcom_scm_call_do(desc, res, fn_id, x5,
> > > > +ARM_SMCCC_STD_CALL);
> > > > +
> > > > + mutex_unlock(_scm_lock);
> > > > +
> > > > + if (res->a0 == QCOM_SCM_V2_EBUSY) {
> > > > + if (retry_count++ >
> > > > QCOM_SCM_EBUSY_MAX_RETRY)
> > > > + break;
> > > > + msleep(QCOM_SCM_EBUSY_WAIT_MS);
> > > > + }
> > > > + } while (res->a0 == QCOM_SCM_V2_EBUSY);
> > > > + } else {
> > > > + __qcom_scm_call_do(desc, res, fn_id, x5,
> > > > ARM_SMCCC_FAST_CALL);
> > > > + }
> > >
> > > Is it safe to make concurrent FAST calls?
> >
> > I better add a spinlock here.
> >
>
> Hi Vivek,
>
> Would you be able to respin this patch, so that we could unblock the
> introduction of the display nodes in the various device?
Will pointed [1] to the restructuring of arm-smmu to support
implementation specific details.
That hasn't been posted yet, and I haven't yet been able to work on that either.
I will be happy to respin this series with the comments addressed if
Will is okay to pull changes to unblock sdm845 devices. :)
[1] https://lore.kernel.org/patchwork/patch/1087457/
Thanks & Regards
Vivek
>
> Regards,
> Bjorn
> ___
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v3 1/4] firmware: qcom_scm-64: Add atomic version of qcom_scm_call
On Wed 19 Jun 04:34 PDT 2019, Vivek Gautam wrote:
> On Tue, Jun 18, 2019 at 11:25 PM Will Deacon wrote:
> >
> > On Wed, Jun 12, 2019 at 12:45:51PM +0530, Vivek Gautam wrote:
> > > There are scnenarios where drivers are required to make a
> > > scm call in atomic context, such as in one of the qcom's
> > > arm-smmu-500 errata [1].
> > >
> > > [1] ("https://source.codeaurora.org/quic/la/kernel/msm-4.9/commit/
> > > drivers/iommu/arm-smmu.c?h=CogSystems-msm-49/
> > > msm-4.9=da765c6c75266b38191b38ef086274943f353ea7")
> > >
> > > Signed-off-by: Vivek Gautam
> > > Reviewed-by: Bjorn Andersson
> > > ---
> > > drivers/firmware/qcom_scm-64.c | 136
> > > -
> > > 1 file changed, 92 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/drivers/firmware/qcom_scm-64.c
> > > b/drivers/firmware/qcom_scm-64.c
> > > index 91d5ad7cf58b..b6dca32c5ac4 100644
> > > --- a/drivers/firmware/qcom_scm-64.c
> > > +++ b/drivers/firmware/qcom_scm-64.c
>
> [snip]
>
> > > +
> > > +static void qcom_scm_call_do(const struct qcom_scm_desc *desc,
> > > + struct arm_smccc_res *res, u32 fn_id,
> > > + u64 x5, bool atomic)
> > > +{
> >
> > Maybe pass in the call type (ARM_SMCCC_FAST_CALL vs ARM_SMCCC_STD_CALL)
> > instead of "bool atomic"? Would certainly make the callsites easier to
> > understand.
>
> Sure, will do that.
>
> >
> > > + int retry_count = 0;
> > > +
> > > + if (!atomic) {
> > > + do {
> > > + mutex_lock(_scm_lock);
> > > +
> > > + __qcom_scm_call_do(desc, res, fn_id, x5,
> > > +ARM_SMCCC_STD_CALL);
> > > +
> > > + mutex_unlock(_scm_lock);
> > > +
> > > + if (res->a0 == QCOM_SCM_V2_EBUSY) {
> > > + if (retry_count++ >
> > > QCOM_SCM_EBUSY_MAX_RETRY)
> > > + break;
> > > + msleep(QCOM_SCM_EBUSY_WAIT_MS);
> > > + }
> > > + } while (res->a0 == QCOM_SCM_V2_EBUSY);
> > > + } else {
> > > + __qcom_scm_call_do(desc, res, fn_id, x5,
> > > ARM_SMCCC_FAST_CALL);
> > > + }
> >
> > Is it safe to make concurrent FAST calls?
>
> I better add a spinlock here.
>
Hi Vivek,
Would you be able to respin this patch, so that we could unblock the
introduction of the display nodes in the various device?
Regards,
Bjorn
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/4] firmware: qcom_scm-64: Add atomic version of qcom_scm_call
On Tue, Jun 18, 2019 at 11:25 PM Will Deacon wrote:
>
> On Wed, Jun 12, 2019 at 12:45:51PM +0530, Vivek Gautam wrote:
> > There are scnenarios where drivers are required to make a
> > scm call in atomic context, such as in one of the qcom's
> > arm-smmu-500 errata [1].
> >
> > [1] ("https://source.codeaurora.org/quic/la/kernel/msm-4.9/commit/
> > drivers/iommu/arm-smmu.c?h=CogSystems-msm-49/
> > msm-4.9=da765c6c75266b38191b38ef086274943f353ea7")
> >
> > Signed-off-by: Vivek Gautam
> > Reviewed-by: Bjorn Andersson
> > ---
> > drivers/firmware/qcom_scm-64.c | 136
> > -
> > 1 file changed, 92 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
> > index 91d5ad7cf58b..b6dca32c5ac4 100644
> > --- a/drivers/firmware/qcom_scm-64.c
> > +++ b/drivers/firmware/qcom_scm-64.c
[snip]
> > +
> > +static void qcom_scm_call_do(const struct qcom_scm_desc *desc,
> > + struct arm_smccc_res *res, u32 fn_id,
> > + u64 x5, bool atomic)
> > +{
>
> Maybe pass in the call type (ARM_SMCCC_FAST_CALL vs ARM_SMCCC_STD_CALL)
> instead of "bool atomic"? Would certainly make the callsites easier to
> understand.
Sure, will do that.
>
> > + int retry_count = 0;
> > +
> > + if (!atomic) {
> > + do {
> > + mutex_lock(_scm_lock);
> > +
> > + __qcom_scm_call_do(desc, res, fn_id, x5,
> > +ARM_SMCCC_STD_CALL);
> > +
> > + mutex_unlock(_scm_lock);
> > +
> > + if (res->a0 == QCOM_SCM_V2_EBUSY) {
> > + if (retry_count++ > QCOM_SCM_EBUSY_MAX_RETRY)
> > + break;
> > + msleep(QCOM_SCM_EBUSY_WAIT_MS);
> > + }
> > + } while (res->a0 == QCOM_SCM_V2_EBUSY);
> > + } else {
> > + __qcom_scm_call_do(desc, res, fn_id, x5, ARM_SMCCC_FAST_CALL);
> > + }
>
> Is it safe to make concurrent FAST calls?
I better add a spinlock here.
Thanks & regards
Vivek
>
> Will
> ___
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/4] firmware: qcom_scm-64: Add atomic version of qcom_scm_call
On Wed, Jun 12, 2019 at 12:45:51PM +0530, Vivek Gautam wrote:
> There are scnenarios where drivers are required to make a
> scm call in atomic context, such as in one of the qcom's
> arm-smmu-500 errata [1].
>
> [1] ("https://source.codeaurora.org/quic/la/kernel/msm-4.9/commit/
> drivers/iommu/arm-smmu.c?h=CogSystems-msm-49/
> msm-4.9=da765c6c75266b38191b38ef086274943f353ea7")
>
> Signed-off-by: Vivek Gautam
> Reviewed-by: Bjorn Andersson
> ---
> drivers/firmware/qcom_scm-64.c | 136
> -
> 1 file changed, 92 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
> index 91d5ad7cf58b..b6dca32c5ac4 100644
> --- a/drivers/firmware/qcom_scm-64.c
> +++ b/drivers/firmware/qcom_scm-64.c
> @@ -62,32 +62,71 @@ static DEFINE_MUTEX(qcom_scm_lock);
> #define FIRST_EXT_ARG_IDX 3
> #define N_REGISTER_ARGS (MAX_QCOM_SCM_ARGS - N_EXT_QCOM_SCM_ARGS + 1)
>
> -/**
> - * qcom_scm_call() - Invoke a syscall in the secure world
> - * @dev: device
> - * @svc_id: service identifier
> - * @cmd_id: command identifier
> - * @desc:Descriptor structure containing arguments and return values
> - *
> - * Sends a command to the SCM and waits for the command to finish processing.
> - * This should *only* be called in pre-emptible context.
> -*/
> -static int qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
> - const struct qcom_scm_desc *desc,
> - struct arm_smccc_res *res)
> +static void __qcom_scm_call_do(const struct qcom_scm_desc *desc,
> +struct arm_smccc_res *res, u32 fn_id,
> +u64 x5, u32 type)
> +{
> + u64 cmd;
> + struct arm_smccc_quirk quirk = {.id = ARM_SMCCC_QUIRK_QCOM_A6};
> +
> + cmd = ARM_SMCCC_CALL_VAL(type, qcom_smccc_convention,
> + ARM_SMCCC_OWNER_SIP, fn_id);
> +
> + quirk.state.a6 = 0;
> +
> + do {
> + arm_smccc_smc_quirk(cmd, desc->arginfo, desc->args[0],
> + desc->args[1], desc->args[2], x5,
> + quirk.state.a6, 0, res, );
> +
> + if (res->a0 == QCOM_SCM_INTERRUPTED)
> + cmd = res->a0;
> +
> + } while (res->a0 == QCOM_SCM_INTERRUPTED);
> +}
> +
> +static void qcom_scm_call_do(const struct qcom_scm_desc *desc,
> + struct arm_smccc_res *res, u32 fn_id,
> + u64 x5, bool atomic)
> +{
Maybe pass in the call type (ARM_SMCCC_FAST_CALL vs ARM_SMCCC_STD_CALL)
instead of "bool atomic"? Would certainly make the callsites easier to
understand.
> + int retry_count = 0;
> +
> + if (!atomic) {
> + do {
> + mutex_lock(_scm_lock);
> +
> + __qcom_scm_call_do(desc, res, fn_id, x5,
> +ARM_SMCCC_STD_CALL);
> +
> + mutex_unlock(_scm_lock);
> +
> + if (res->a0 == QCOM_SCM_V2_EBUSY) {
> + if (retry_count++ > QCOM_SCM_EBUSY_MAX_RETRY)
> + break;
> + msleep(QCOM_SCM_EBUSY_WAIT_MS);
> + }
> + } while (res->a0 == QCOM_SCM_V2_EBUSY);
> + } else {
> + __qcom_scm_call_do(desc, res, fn_id, x5, ARM_SMCCC_FAST_CALL);
> + }
Is it safe to make concurrent FAST calls?
Will
[PATCH v3 1/4] firmware: qcom_scm-64: Add atomic version of qcom_scm_call
There are scnenarios where drivers are required to make a
scm call in atomic context, such as in one of the qcom's
arm-smmu-500 errata [1].
[1] ("https://source.codeaurora.org/quic/la/kernel/msm-4.9/commit/
drivers/iommu/arm-smmu.c?h=CogSystems-msm-49/
msm-4.9=da765c6c75266b38191b38ef086274943f353ea7")
Signed-off-by: Vivek Gautam
Reviewed-by: Bjorn Andersson
---
drivers/firmware/qcom_scm-64.c | 136 -
1 file changed, 92 insertions(+), 44 deletions(-)
diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 91d5ad7cf58b..b6dca32c5ac4 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -62,32 +62,71 @@ static DEFINE_MUTEX(qcom_scm_lock);
#define FIRST_EXT_ARG_IDX 3
#define N_REGISTER_ARGS (MAX_QCOM_SCM_ARGS - N_EXT_QCOM_SCM_ARGS + 1)
-/**
- * qcom_scm_call() - Invoke a syscall in the secure world
- * @dev: device
- * @svc_id:service identifier
- * @cmd_id:command identifier
- * @desc: Descriptor structure containing arguments and return values
- *
- * Sends a command to the SCM and waits for the command to finish processing.
- * This should *only* be called in pre-emptible context.
-*/
-static int qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
-const struct qcom_scm_desc *desc,
-struct arm_smccc_res *res)
+static void __qcom_scm_call_do(const struct qcom_scm_desc *desc,
+ struct arm_smccc_res *res, u32 fn_id,
+ u64 x5, u32 type)
+{
+ u64 cmd;
+ struct arm_smccc_quirk quirk = {.id = ARM_SMCCC_QUIRK_QCOM_A6};
+
+ cmd = ARM_SMCCC_CALL_VAL(type, qcom_smccc_convention,
+ARM_SMCCC_OWNER_SIP, fn_id);
+
+ quirk.state.a6 = 0;
+
+ do {
+ arm_smccc_smc_quirk(cmd, desc->arginfo, desc->args[0],
+ desc->args[1], desc->args[2], x5,
+ quirk.state.a6, 0, res, );
+
+ if (res->a0 == QCOM_SCM_INTERRUPTED)
+ cmd = res->a0;
+
+ } while (res->a0 == QCOM_SCM_INTERRUPTED);
+}
+
+static void qcom_scm_call_do(const struct qcom_scm_desc *desc,
+struct arm_smccc_res *res, u32 fn_id,
+u64 x5, bool atomic)
+{
+ int retry_count = 0;
+
+ if (!atomic) {
+ do {
+ mutex_lock(_scm_lock);
+
+ __qcom_scm_call_do(desc, res, fn_id, x5,
+ ARM_SMCCC_STD_CALL);
+
+ mutex_unlock(_scm_lock);
+
+ if (res->a0 == QCOM_SCM_V2_EBUSY) {
+ if (retry_count++ > QCOM_SCM_EBUSY_MAX_RETRY)
+ break;
+ msleep(QCOM_SCM_EBUSY_WAIT_MS);
+ }
+ } while (res->a0 == QCOM_SCM_V2_EBUSY);
+ } else {
+ __qcom_scm_call_do(desc, res, fn_id, x5, ARM_SMCCC_FAST_CALL);
+ }
+}
+
+static int ___qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
+ const struct qcom_scm_desc *desc,
+ struct arm_smccc_res *res, bool atomic)
{
int arglen = desc->arginfo & 0xf;
- int retry_count = 0, i;
+ int i;
u32 fn_id = QCOM_SCM_FNID(svc_id, cmd_id);
- u64 cmd, x5 = desc->args[FIRST_EXT_ARG_IDX];
+ u64 x5 = desc->args[FIRST_EXT_ARG_IDX];
dma_addr_t args_phys = 0;
void *args_virt = NULL;
size_t alloc_len;
- struct arm_smccc_quirk quirk = {.id = ARM_SMCCC_QUIRK_QCOM_A6};
+ gfp_t flag = atomic ? GFP_ATOMIC : GFP_KERNEL;
if (unlikely(arglen > N_REGISTER_ARGS)) {
alloc_len = N_EXT_QCOM_SCM_ARGS * sizeof(u64);
- args_virt = kzalloc(PAGE_ALIGN(alloc_len), GFP_KERNEL);
+ args_virt = kzalloc(PAGE_ALIGN(alloc_len), flag);
if (!args_virt)
return -ENOMEM;
@@ -117,33 +156,7 @@ static int qcom_scm_call(struct device *dev, u32 svc_id,
u32 cmd_id,
x5 = args_phys;
}
- do {
- mutex_lock(_scm_lock);
-
- cmd = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
-qcom_smccc_convention,
-ARM_SMCCC_OWNER_SIP, fn_id);
-
- quirk.state.a6 = 0;
-
- do {
- arm_smccc_smc_quirk(cmd, desc->arginfo, desc->args[0],
- desc->args[1], desc->args[2], x5,
- quirk.state.a6, 0, res, );
-
- if (res->a0 == QCOM_SCM_INTERRUPTED)
- cmd = res->a0;
-
- } while (res->a0 == QCOM_SCM_INTERRUPTED);
-
-
