| Message ID | 20230110063745.16739-3-quic_sibis@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SCM: Add support for wait-queue aware firmware | expand |
On Jan 10 2023 12:07, Sibi Sankar wrote: ... > +static int __scm_smc_do_quirk_handle_waitq(struct device *dev, struct arm_smccc_args *waitq, > + struct arm_smccc_res *res) > +{ > + int ret; > + struct arm_smccc_args resume; > + u32 wq_ctx, smc_call_ctx, flags; > + struct arm_smccc_args *smc = waitq; > + > + do { > + __scm_smc_do_quirk(smc, res); > + > + if (res->a0 == QCOM_SCM_WAITQ_SLEEP) { > + wq_ctx = res->a1; > + smc_call_ctx = res->a2; > + flags = res->a3; > + > + if (!dev) > + return -EPROBE_DEFER; > + > + ret = qcom_scm_lookup_completion(wq_ctx); I see that this function has been created in response to Bjorn's comment [1] about avoiding the dev_get_drvdata() call, but I would prefer to not use this function as it hides the fact that the wait_for_completion() is occurring here. Knowing where the waiting is happening is useful not just for understanding code flow but also for debugging issues in the future. ... > +static struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx) > +{ This function is called qcom_scm_lookup_wq() but there is no looking up occurring here. Could this comment be added for context? /* FW currently only supports a single wq_ctx (zero). * TODO: Update this logic to include dynamic allocation and lookup of * completion structs when FW supports more wq_ctx values. */ > + /* assert wq_ctx is zero */ > + if (wq_ctx != 0) { > + dev_err(scm->dev, "No waitqueue found for wq_ctx %d\n", wq_ctx); > + return ERR_PTR(-EINVAL); > + } > + > + return &scm->waitq_comp; > +} > + ... [1] https://lore.kernel.org/lkml/20221208221125.bflo7unhcrgfsgbr@builder.lan/
Hi Sibi, Few minor comments below, On 10/01/2023 06:37, Sibi Sankar wrote: > From: Guru Das Srinagesh <quic_gurus@quicinc.com> > > When the firmware (FW) supports multiple requests per VM, multiple requests > from the same/different VM can reach the firmware at the same time. Since > the firmware currently being used has limited resources, it guards them > with a resource lock and puts requests on a wait-queue internally and > signals to HLOS that it is doing so. It does this by returning a new return > value in addition to success or error: SCM_WAITQ_SLEEP. A sleeping SCM call > can be woken up by an interrupt that the FW raises. > ... > drivers/firmware/qcom_scm-smc.c | 90 ++++++++++++++++++++++++++++++--- > drivers/firmware/qcom_scm.c | 89 +++++++++++++++++++++++++++++++- > drivers/firmware/qcom_scm.h | 8 +++ > 3 files changed, 179 insertions(+), 8 deletions(-) > > diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c > index d111833364ba..30999f04749c 100644 > --- a/drivers/firmware/qcom_scm-smc.c > +++ b/drivers/firmware/qcom_scm-smc.c ... > +static int __scm_smc_do_quirk_handle_waitq(struct device *dev, struct arm_smccc_args *waitq, > + struct arm_smccc_res *res) > +{ > + int ret; > + struct arm_smccc_args resume; > + u32 wq_ctx, smc_call_ctx, flags; > + struct arm_smccc_args *smc = waitq; > + > + do { > + __scm_smc_do_quirk(smc, res); > + > + if (res->a0 == QCOM_SCM_WAITQ_SLEEP) { > + wq_ctx = res->a1; > + smc_call_ctx = res->a2; > + flags = res->a3; > + > + if (!dev) > + return -EPROBE_DEFER; why are we checking dev pointer in the middle of the call? A comment here would really help readers. > + > + ret = qcom_scm_lookup_completion(wq_ctx); > + if (ret) > + return ret; > + > + fill_wq_resume_args(&resume, smc_call_ctx); > + smc = &resume; > + } > + } while (res->a0 == QCOM_SCM_WAITQ_SLEEP); > + > + return 0; > +} > + ... > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > index cdbfe54c8146..19ac506a9b1f 100644 > --- a/drivers/firmware/qcom_scm.c > +++ b/drivers/firmware/qcom_scm.c > @@ -4,6 +4,7 @@ > */ > #include <linux/platform_device.h> > #include <linux/init.h> > +#include <linux/interrupt.h> > #include <linux/cpumask.h> > #include <linux/export.h> > #include <linux/dma-mapping.h> > @@ -13,6 +14,7 @@ > #include <linux/qcom_scm.h> > #include <linux/of.h> > #include <linux/of_address.h> > +#include <linux/of_irq.h> > #include <linux/of_platform.h> > #include <linux/clk.h> > #include <linux/reset-controller.h> include <linux/completion.h> ?? > @@ -33,6 +35,7 @@ struct qcom_scm { > struct clk *iface_clk; > struct clk *bus_clk; > struct icc_path *path; > + struct completion waitq_comp; > struct reset_controller_dev reset; > > /* control access to the interconnect path */ > @@ -63,6 +66,9 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = { > BIT(2), BIT(1), BIT(4), BIT(6) > }; > > +#define QCOM_SMC_WAITQ_FLAG_WAKE_ONE BIT(0) > +#define QCOM_SMC_WAITQ_FLAG_WAKE_ALL BIT(1) > + > static const char * const qcom_scm_convention_names[] = { > [SMC_CONVENTION_UNKNOWN] = "unknown", > [SMC_CONVENTION_ARM_32] = "smc arm 32", > @@ -1325,11 +1331,79 @@ bool qcom_scm_is_available(void) > } > EXPORT_SYMBOL(qcom_scm_is_available); > > +static struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx) > +{ > + /* assert wq_ctx is zero */ > + if (wq_ctx != 0) { Is this correct? looks like zero is the only valid one. I thought wq_ctx was a unique number (UID). > + dev_err(scm->dev, "No waitqueue found for wq_ctx %d\n", wq_ctx); > + return ERR_PTR(-EINVAL); > + } > + > + return &scm->waitq_comp; > +} > + > +int qcom_scm_lookup_completion(u32 wq_ctx) > +{ > + struct completion *wq = NULL; > + > + wq = qcom_scm_lookup_wq(__scm, wq_ctx); > + if (IS_ERR(wq)) > + return PTR_ERR(wq); > + > + wait_for_completion(wq); We can potentially block here forever without a timeout. As you are reusing completion, I have not seen any reinitialization of completion, this could potentially return above line without waiting at all. > + > + return 0; > +} > + > +static int qcom_scm_waitq_wakeup(struct qcom_scm *scm, unsigned int wq_ctx, bool wake_all) > +{ > + struct completion *wq_to_wake; > + > + wq_to_wake = qcom_scm_lookup_wq(scm, wq_ctx); > + if (IS_ERR(wq_to_wake)) > + return PTR_ERR(wq_to_wake); > + > + if (wake_all) > + complete_all(wq_to_wake); > + else > + complete(wq_to_wake); > + > + return 0; > +} > + > +static irqreturn_t qcom_scm_irq_handler(int irq, void *data) > +{ > + int ret; > + struct qcom_scm *scm = data; > + u32 wq_ctx, flags, more_pending = 0; > + > + do { > + ret = scm_get_wq_ctx(&wq_ctx, &flags, &more_pending); > + if (ret) { > + dev_err(scm->dev, "GET_WQ_CTX SMC call failed: %d\n", ret); > + goto out; > + } > + > + if (flags != QCOM_SMC_WAITQ_FLAG_WAKE_ONE && > + flags != QCOM_SMC_WAITQ_FLAG_WAKE_ALL) { > + dev_err(scm->dev, "Invalid flags found for wq_ctx: %u\n", flags); > + goto out; > + } > + > + ret = qcom_scm_waitq_wakeup(scm, wq_ctx, !!(flags & QCOM_SMC_WAITQ_FLAG_WAKE_ALL)); > + if (ret) > + goto out; > + } while (more_pending); > + > +out: > + return IRQ_HANDLED; > +} > + > static int qcom_scm_probe(struct platform_device *pdev) > { > struct qcom_scm *scm; > unsigned long clks; > - int ret; > + int irq, ret; > > scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL); > if (!scm) > @@ -1402,6 +1476,19 @@ static int qcom_scm_probe(struct platform_device *pdev) > __scm = scm; > __scm->dev = &pdev->dev; > > + init_completion(&__scm->waitq_comp); > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + if (irq != -ENXIO) > + return irq; > + } else { > + ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler, > + IRQF_ONESHOT, "qcom-scm", __scm); > + if (ret < 0) > + return dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n"); > + } > + > __get_convention(); > > /* --srini
On Tue, Jan 10, 2023 at 12:14:11AM -0800, Guru Das Srinagesh wrote: > On Jan 10 2023 12:07, Sibi Sankar wrote: > > ... > > > +static int __scm_smc_do_quirk_handle_waitq(struct device *dev, struct arm_smccc_args *waitq, > > + struct arm_smccc_res *res) > > +{ > > + int ret; > > + struct arm_smccc_args resume; > > + u32 wq_ctx, smc_call_ctx, flags; > > + struct arm_smccc_args *smc = waitq; > > + > > + do { > > + __scm_smc_do_quirk(smc, res); > > + > > + if (res->a0 == QCOM_SCM_WAITQ_SLEEP) { > > + wq_ctx = res->a1; > > + smc_call_ctx = res->a2; > > + flags = res->a3; > > + > > + if (!dev) > > + return -EPROBE_DEFER; > > + > > + ret = qcom_scm_lookup_completion(wq_ctx); > > I see that this function has been created in response to Bjorn's comment [1] > about avoiding the dev_get_drvdata() call, but I would prefer to not use this > function as it hides the fact that the wait_for_completion() is occurring here. > My reasoning here is that I don't want the waiting for the completion that happen in one part of the driver and the completion happening in a completely different one. > Knowing where the waiting is happening is useful not just for understanding > code flow but also for debugging issues in the future. > Absolutely agree, this should be named to make that obvious to the reader. > ... > > > +static struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx) > > +{ > > This function is called qcom_scm_lookup_wq() but there is no looking up > occurring here. Could this comment be added for context? > > /* FW currently only supports a single wq_ctx (zero). > * TODO: Update this logic to include dynamic allocation and lookup of > * completion structs when FW supports more wq_ctx values. > */ > Agree. Regards, Bjorn > > + /* assert wq_ctx is zero */ > > + if (wq_ctx != 0) { > > + dev_err(scm->dev, "No waitqueue found for wq_ctx %d\n", wq_ctx); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + return &scm->waitq_comp; > > +} > > + > ... > > [1] https://lore.kernel.org/lkml/20221208221125.bflo7unhcrgfsgbr@builder.lan/
On Tue, Jan 10, 2023 at 12:07:45PM +0530, Sibi Sankar wrote: > From: Guru Das Srinagesh <quic_gurus@quicinc.com> > > When the firmware (FW) supports multiple requests per VM, multiple requests > from the same/different VM can reach the firmware at the same time. Since > the firmware currently being used has limited resources, it guards them > with a resource lock and puts requests on a wait-queue internally and > signals to HLOS that it is doing so. It does this by returning a new return > value in addition to success or error: SCM_WAITQ_SLEEP. A sleeping SCM call > can be woken up by an interrupt that the FW raises. > > 1) SCM_WAITQ_SLEEP: > > When an SCM call receives this return value instead of success > or error, FW has placed this call on a wait-queue and has signalled > HLOS to put it to non-interruptible sleep. > > Along with this return value, FW also passes to HLOS `wq_ctx` - > a unique number (UID) identifying the wait-queue that it has put > the call on, internally. This is to help HLOS with its own > bookkeeping to wake this sleeping call later. > > Additionally, FW also passes to HLOS `smc_call_ctx` - a UID > identifying the SCM call thus being put to sleep. This is also > for HLOS' bookkeeping to wake this call up later. > > These two additional values are passed via the a1 and a2 > registers. > > N.B.: The "ctx" in the above UID names = "context". > > The handshake mechanism that HLOS uses to talk to FW about wait-queue > operations involves two new SMC calls. > > 1) get_wq_ctx(): > > Arguments: None > Returns: wq_ctx, flags, more_pending > > Get the wait-queue context, and wake up either one or all of the > sleeping SCM calls associated with that wait-queue. > > Additionally, repeat this if there are more wait-queues that are > ready to have their requests woken up (`more_pending`). > > 2) wq_resume(smc_call_ctx): > > Arguments: smc_call_ctx > > HLOS needs to issue this in response to receiving an > IRQ, passing to FW the same smc_call_ctx that FW > receives from HLOS via the get_wq_ctx() call. > > (The mechanism to wake a SMC call back up is described in detail below) > > VM_1 VM_2 Firmware > │ │ │ > │ │ │ > │ │ │ > │ │ │ > │ REQUEST_1 │ │ > ├────────────────────────┼─────────────────────────────────┤ > │ │ │ > │ │ ┌──┼──┐ > │ │ │ │ │ > │ │ REQUEST_2 │ │ │ > │ ├──────────────────────────────┼──┤ │ > │ │ │ │ │Resource > │ │ │ │ │is busy > │ │ {WQ_SLEEP} │ │ │ > │ │◄─────────────────────────────┼──┤ │ > │ │ wq_ctx, smc_call_ctx │ │ │ > │ │ └──┼──┘ > │ REQUEST_1 COMPLETE │ │ > │◄───────────────────────┼─────────────────────────────────┤ > │ │ │ > │ │ IRQ │ > │ │◄─-------------------------------│ > │ │ │ > │ │ get_wq_ctx() │ > │ ├────────────────────────────────►│ > │ │ │ > │ │ │ > │ │◄────────────────────────────────┤ > │ │ wq_ctx, flags, and │ > │ │ more_pending │ > │ │ │ > │ │ │ > │ │ wq_resume(smc_call_ctx) │ > │ ├────────────────────────────────►│ > │ │ │ > │ │ │ > │ │ REQUEST_2 COMPLETE │ > │ │◄────────────────────────────────┤ > │ │ │ > │ │ │ > > With the exception of get_wq_ctx(), the other SMC call wq_resume() can > return WQ_SLEEP (these nested rounds of WQ_SLEEP are not shown in the > above diagram for the sake of simplicity). Therefore, introduce a new > do-while loop to handle multiple WQ_SLEEP return values for the same > parent SCM call. > > Request Completion in the above diagram refers to either a success > return value (zero) or error (and not SMC_WAITQ_SLEEP) > > Also add the interrupt handler that wakes up a sleeping SCM call. > > Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com> > Co-developed-by: Sibi Sankar <quic_sibis@quicinc.com> > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > --- > > v7: > - Move lookup + wait_for_completion into a single function in qcom_scm [Bjorn] > - Simplify completion retrieval [Bjorn] > > v6: > - Fix misc. nits in the scm driver [Krzysztof] > > v5: > - Handle the wake_one/wake_all flags [Guru] > - Rename flag handler to qcom_scm_waitq_wakeup [Bjorn] > - Resume scm call can return ebusy as well handle that scenario by retrying > the original smc call and not the resume call > > v4: > - platform_set_drvdata will be used by __scm_smc_do_quirk_handle_waitq to > get access to scm struct from device so retain it > - Use a single completion as it satisfies all of the current usecases [Bjorn] > - Inline scm_get_wq_ctx [Bjorn] > - Convert all pr_err to dev_err [Bjorn] > - Handle idr_destroy in a thread safe manner [Bjorn] > - Misc. Style fixes [Bjorn] > > v3: > - Fixup irq handling so as not to affect SoCs without the interrupt. > - Fix warnings reported by kernel test-bot. > > drivers/firmware/qcom_scm-smc.c | 90 ++++++++++++++++++++++++++++++--- > drivers/firmware/qcom_scm.c | 89 +++++++++++++++++++++++++++++++- > drivers/firmware/qcom_scm.h | 8 +++ > 3 files changed, 179 insertions(+), 8 deletions(-) > > diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c > index d111833364ba..30999f04749c 100644 > --- a/drivers/firmware/qcom_scm-smc.c > +++ b/drivers/firmware/qcom_scm-smc.c > @@ -52,29 +52,101 @@ static void __scm_smc_do_quirk(const struct arm_smccc_args *smc, > } while (res->a0 == QCOM_SCM_INTERRUPTED); > } > > -static void __scm_smc_do(const struct arm_smccc_args *smc, > - struct arm_smccc_res *res, bool atomic) > +static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx) > { > - int retry_count = 0; > + memset(resume->args, 0, sizeof(resume->args[0]) * ARRAY_SIZE(resume->args)); > + > + resume->args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL, > + ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP, > + SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_RESUME)); > + > + resume->args[1] = QCOM_SCM_ARGS(1); > + > + resume->args[2] = smc_call_ctx; > +} > + > +int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending) > +{ > + int ret; > + struct arm_smccc_args get_wq_ctx = {0}; > + struct arm_smccc_res get_wq_res; > + > + get_wq_ctx.args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL, > + ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP, > + SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_GET_WQ_CTX)); > + > + /* Guaranteed to return only success or error, no WAITQ_* */ > + __scm_smc_do_quirk(&get_wq_ctx, &get_wq_res); > + ret = get_wq_res.a0; > + if (ret) > + return ret; > + > + *wq_ctx = get_wq_res.a1; > + *flags = get_wq_res.a2; > + *more_pending = get_wq_res.a3; > + > + return 0; > +} > + > +static int __scm_smc_do_quirk_handle_waitq(struct device *dev, struct arm_smccc_args *waitq, > + struct arm_smccc_res *res) > +{ > + int ret; > + struct arm_smccc_args resume; > + u32 wq_ctx, smc_call_ctx, flags; > + struct arm_smccc_args *smc = waitq; > + > + do { > + __scm_smc_do_quirk(smc, res); > + > + if (res->a0 == QCOM_SCM_WAITQ_SLEEP) { > + wq_ctx = res->a1; > + smc_call_ctx = res->a2; > + flags = res->a3; > + > + if (!dev) > + return -EPROBE_DEFER; > + > + ret = qcom_scm_lookup_completion(wq_ctx); > + if (ret) > + return ret; > + > + fill_wq_resume_args(&resume, smc_call_ctx); > + smc = &resume; > + } > + } while (res->a0 == QCOM_SCM_WAITQ_SLEEP); > + > + return 0; > +} > + > +static int __scm_smc_do(struct device *dev, struct arm_smccc_args *smc, > + struct arm_smccc_res *res, bool atomic) > +{ > + int ret, retry_count = 0; > > if (atomic) { > __scm_smc_do_quirk(smc, res); > - return; > + return 0; > } > > do { > mutex_lock(&qcom_scm_lock); > > - __scm_smc_do_quirk(smc, res); > + ret = __scm_smc_do_quirk_handle_waitq(dev, smc, res); > > mutex_unlock(&qcom_scm_lock); > > + if (ret) > + return ret; > + > 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); > + > + return 0; > } > > > @@ -83,7 +155,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc, > struct qcom_scm_res *res, bool atomic) > { > int arglen = desc->arginfo & 0xf; > - int i; > + int i, ret; > dma_addr_t args_phys = 0; > void *args_virt = NULL; > size_t alloc_len; > @@ -135,13 +207,17 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc, > smc.args[SCM_SMC_LAST_REG_IDX] = args_phys; > } > > - __scm_smc_do(&smc, &smc_res, atomic); > + /* ret error check follows after args_virt cleanup*/ > + ret = __scm_smc_do(dev, &smc, &smc_res, atomic); > > if (args_virt) { > dma_unmap_single(dev, args_phys, alloc_len, DMA_TO_DEVICE); > kfree(args_virt); > } > > + if (ret) > + return ret; > + > if (res) { > res->result[0] = smc_res.a1; > res->result[1] = smc_res.a2; > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > index cdbfe54c8146..19ac506a9b1f 100644 > --- a/drivers/firmware/qcom_scm.c > +++ b/drivers/firmware/qcom_scm.c > @@ -4,6 +4,7 @@ > */ > #include <linux/platform_device.h> > #include <linux/init.h> > +#include <linux/interrupt.h> > #include <linux/cpumask.h> > #include <linux/export.h> > #include <linux/dma-mapping.h> > @@ -13,6 +14,7 @@ > #include <linux/qcom_scm.h> > #include <linux/of.h> > #include <linux/of_address.h> > +#include <linux/of_irq.h> > #include <linux/of_platform.h> > #include <linux/clk.h> > #include <linux/reset-controller.h> > @@ -33,6 +35,7 @@ struct qcom_scm { > struct clk *iface_clk; > struct clk *bus_clk; > struct icc_path *path; > + struct completion waitq_comp; > struct reset_controller_dev reset; > > /* control access to the interconnect path */ > @@ -63,6 +66,9 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = { > BIT(2), BIT(1), BIT(4), BIT(6) > }; > > +#define QCOM_SMC_WAITQ_FLAG_WAKE_ONE BIT(0) > +#define QCOM_SMC_WAITQ_FLAG_WAKE_ALL BIT(1) > + > static const char * const qcom_scm_convention_names[] = { > [SMC_CONVENTION_UNKNOWN] = "unknown", > [SMC_CONVENTION_ARM_32] = "smc arm 32", > @@ -1325,11 +1331,79 @@ bool qcom_scm_is_available(void) > } > EXPORT_SYMBOL(qcom_scm_is_available); > > +static struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx) This could either be seen as a remnant of the older versions of this patch, or a signalling of "place implementation here". I dislike both... Please rename it qcom_scm_assert_valid_wq_ctx() and just reference &scm->waitq_comp directly in the two places below. > +{ > + /* assert wq_ctx is zero */ > + if (wq_ctx != 0) { > + dev_err(scm->dev, "No waitqueue found for wq_ctx %d\n", wq_ctx); I think you should be more specific here, perhaps: "Firmware unexpectedly passed non-zero wq_ctx\n" And then as suggested a comment stating what needs to be done when that happens. > + return ERR_PTR(-EINVAL); > + } > + > + return &scm->waitq_comp; > +} > + > +int qcom_scm_lookup_completion(u32 wq_ctx) > +{ > + struct completion *wq = NULL; You assign it on the very next line, no need to do so here. > + > + wq = qcom_scm_lookup_wq(__scm, wq_ctx); > + if (IS_ERR(wq)) > + return PTR_ERR(wq); > + > + wait_for_completion(wq); > + > + return 0; > +} > + > +static int qcom_scm_waitq_wakeup(struct qcom_scm *scm, unsigned int wq_ctx, bool wake_all) > +{ > + struct completion *wq_to_wake; "wq" would be sufficient. Thanks, Bjorn > + > + wq_to_wake = qcom_scm_lookup_wq(scm, wq_ctx); > + if (IS_ERR(wq_to_wake)) > + return PTR_ERR(wq_to_wake); > + > + if (wake_all) > + complete_all(wq_to_wake); > + else > + complete(wq_to_wake); > + > + return 0; > +} > + > +static irqreturn_t qcom_scm_irq_handler(int irq, void *data) > +{ > + int ret; > + struct qcom_scm *scm = data; > + u32 wq_ctx, flags, more_pending = 0; > + > + do { > + ret = scm_get_wq_ctx(&wq_ctx, &flags, &more_pending); > + if (ret) { > + dev_err(scm->dev, "GET_WQ_CTX SMC call failed: %d\n", ret); > + goto out; > + } > + > + if (flags != QCOM_SMC_WAITQ_FLAG_WAKE_ONE && > + flags != QCOM_SMC_WAITQ_FLAG_WAKE_ALL) { > + dev_err(scm->dev, "Invalid flags found for wq_ctx: %u\n", flags); > + goto out; > + } > + > + ret = qcom_scm_waitq_wakeup(scm, wq_ctx, !!(flags & QCOM_SMC_WAITQ_FLAG_WAKE_ALL)); > + if (ret) > + goto out; > + } while (more_pending); > + > +out: > + return IRQ_HANDLED; > +} > + > static int qcom_scm_probe(struct platform_device *pdev) > { > struct qcom_scm *scm; > unsigned long clks; > - int ret; > + int irq, ret; > > scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL); > if (!scm) > @@ -1402,6 +1476,19 @@ static int qcom_scm_probe(struct platform_device *pdev) > __scm = scm; > __scm->dev = &pdev->dev; > > + init_completion(&__scm->waitq_comp); > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + if (irq != -ENXIO) > + return irq; > + } else { > + ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler, > + IRQF_ONESHOT, "qcom-scm", __scm); > + if (ret < 0) > + return dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n"); > + } > + > __get_convention(); > > /* > diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h > index db3d08a01209..018e9867d55a 100644 > --- a/drivers/firmware/qcom_scm.h > +++ b/drivers/firmware/qcom_scm.h > @@ -60,6 +60,9 @@ struct qcom_scm_res { > u64 result[MAX_QCOM_SCM_RETS]; > }; > > +int qcom_scm_lookup_completion(u32 wq_ctx); > +int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending); > + > #define SCM_SMC_FNID(s, c) ((((s) & 0xFF) << 8) | ((c) & 0xFF)) > extern int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc, > enum qcom_scm_convention qcom_convention, > @@ -129,6 +132,10 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc, > #define QCOM_SCM_SMMU_CONFIG_ERRATA1 0x03 > #define QCOM_SCM_SMMU_CONFIG_ERRATA1_CLIENT_ALL 0x02 > > +#define QCOM_SCM_SVC_WAITQ 0x24 > +#define QCOM_SCM_WAITQ_RESUME 0x02 > +#define QCOM_SCM_WAITQ_GET_WQ_CTX 0x03 > + > /* common error codes */ > #define QCOM_SCM_V2_EBUSY -12 > #define QCOM_SCM_ENOMEM -5 > @@ -137,6 +144,7 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc, > #define QCOM_SCM_EINVAL_ARG -2 > #define QCOM_SCM_ERROR -1 > #define QCOM_SCM_INTERRUPTED 1 > +#define QCOM_SCM_WAITQ_SLEEP 2 > > static inline int qcom_scm_remap_error(int err) > { > -- > 2.17.1 >
On Jan 10 2023 11:54, Bjorn Andersson wrote: > On Tue, Jan 10, 2023 at 12:14:11AM -0800, Guru Das Srinagesh wrote: > > On Jan 10 2023 12:07, Sibi Sankar wrote: > > > > ... > > > > > +static int __scm_smc_do_quirk_handle_waitq(struct device *dev, struct arm_smccc_args *waitq, > > > + struct arm_smccc_res *res) > > > +{ > > > + int ret; > > > + struct arm_smccc_args resume; > > > + u32 wq_ctx, smc_call_ctx, flags; > > > + struct arm_smccc_args *smc = waitq; > > > + > > > + do { > > > + __scm_smc_do_quirk(smc, res); > > > + > > > + if (res->a0 == QCOM_SCM_WAITQ_SLEEP) { > > > + wq_ctx = res->a1; > > > + smc_call_ctx = res->a2; > > > + flags = res->a3; > > > + > > > + if (!dev) > > > + return -EPROBE_DEFER; > > > + > > > + ret = qcom_scm_lookup_completion(wq_ctx); > > > > I see that this function has been created in response to Bjorn's comment [1] > > about avoiding the dev_get_drvdata() call, but I would prefer to not use this > > function as it hides the fact that the wait_for_completion() is occurring here. > > > > My reasoning here is that I don't want the waiting for the completion > that happen in one part of the driver and the completion happening in a > completely different one. ACK. Thank you. Guru Das.
Hey Srini, Thanks for taking time to review the series. On 1/10/23 17:44, Srinivas Kandagatla wrote: > Hi Sibi, > > Few minor comments below, > > On 10/01/2023 06:37, Sibi Sankar wrote: >> From: Guru Das Srinagesh <quic_gurus@quicinc.com> >> >> When the firmware (FW) supports multiple requests per VM, multiple >> requests >> from the same/different VM can reach the firmware at the same time. Since >> the firmware currently being used has limited resources, it guards them >> with a resource lock and puts requests on a wait-queue internally and >> signals to HLOS that it is doing so. It does this by returning a new >> return >> value in addition to success or error: SCM_WAITQ_SLEEP. A sleeping SCM >> call >> can be woken up by an interrupt that the FW raises. >> > ... > >> drivers/firmware/qcom_scm-smc.c | 90 ++++++++++++++++++++++++++++++--- >> drivers/firmware/qcom_scm.c | 89 +++++++++++++++++++++++++++++++- >> drivers/firmware/qcom_scm.h | 8 +++ >> 3 files changed, 179 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/firmware/qcom_scm-smc.c >> b/drivers/firmware/qcom_scm-smc.c >> index d111833364ba..30999f04749c 100644 >> --- a/drivers/firmware/qcom_scm-smc.c >> +++ b/drivers/firmware/qcom_scm-smc.c > ... >> +static int __scm_smc_do_quirk_handle_waitq(struct device *dev, struct >> arm_smccc_args *waitq, >> + struct arm_smccc_res *res) >> +{ >> + int ret; >> + struct arm_smccc_args resume; >> + u32 wq_ctx, smc_call_ctx, flags; >> + struct arm_smccc_args *smc = waitq; >> + >> + do { >> + __scm_smc_do_quirk(smc, res); >> + >> + if (res->a0 == QCOM_SCM_WAITQ_SLEEP) { >> + wq_ctx = res->a1; >> + smc_call_ctx = res->a2; >> + flags = res->a3; >> + >> + if (!dev) >> + return -EPROBE_DEFER; > > why are we checking dev pointer in the middle of the call? > A comment here would really help readers. Given that we no longer use drv_data to pass around scm struct, the check is no longer required. I'll drop it in the next re-spin. > >> + >> + ret = qcom_scm_lookup_completion(wq_ctx); >> + if (ret) >> + return ret; >> + >> + fill_wq_resume_args(&resume, smc_call_ctx); >> + smc = &resume; >> + } >> + } while (res->a0 == QCOM_SCM_WAITQ_SLEEP); >> + >> + return 0; >> +} >> + > ... >> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >> index cdbfe54c8146..19ac506a9b1f 100644 >> --- a/drivers/firmware/qcom_scm.c >> +++ b/drivers/firmware/qcom_scm.c >> @@ -4,6 +4,7 @@ >> */ >> #include <linux/platform_device.h> >> #include <linux/init.h> >> +#include <linux/interrupt.h> >> #include <linux/cpumask.h> >> #include <linux/export.h> >> #include <linux/dma-mapping.h> >> @@ -13,6 +14,7 @@ >> #include <linux/qcom_scm.h> >> #include <linux/of.h> >> #include <linux/of_address.h> >> +#include <linux/of_irq.h> >> #include <linux/of_platform.h> >> #include <linux/clk.h> >> #include <linux/reset-controller.h> > > include <linux/completion.h> ?? > ack > >> @@ -33,6 +35,7 @@ struct qcom_scm { >> struct clk *iface_clk; >> struct clk *bus_clk; >> struct icc_path *path; >> + struct completion waitq_comp; >> struct reset_controller_dev reset; >> /* control access to the interconnect path */ >> @@ -63,6 +66,9 @@ static const u8 >> qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = { >> BIT(2), BIT(1), BIT(4), BIT(6) >> }; >> +#define QCOM_SMC_WAITQ_FLAG_WAKE_ONE BIT(0) >> +#define QCOM_SMC_WAITQ_FLAG_WAKE_ALL BIT(1) >> + >> static const char * const qcom_scm_convention_names[] = { >> [SMC_CONVENTION_UNKNOWN] = "unknown", >> [SMC_CONVENTION_ARM_32] = "smc arm 32", >> @@ -1325,11 +1331,79 @@ bool qcom_scm_is_available(void) >> } >> EXPORT_SYMBOL(qcom_scm_is_available); >> +static struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, >> u32 wq_ctx) >> +{ >> + /* assert wq_ctx is zero */ > + if (wq_ctx != 0) { > > Is this correct? looks like zero is the only valid one. > > I thought wq_ctx was a unique number (UID). Currently the SMC calls from the kernel scm driver are still serialized and firmware only supports a single wq_ctx. This is expected to change in the future, will document it the comments. > >> + dev_err(scm->dev, "No waitqueue found for wq_ctx %d\n", wq_ctx); >> + return ERR_PTR(-EINVAL); >> + } >> + >> + return &scm->waitq_comp; >> +} >> + >> +int qcom_scm_lookup_completion(u32 wq_ctx) >> +{ >> + struct completion *wq = NULL; >> + >> + wq = qcom_scm_lookup_wq(__scm, wq_ctx); >> + if (IS_ERR(wq)) >> + return PTR_ERR(wq); >> + >> + wait_for_completion(wq); > > We can potentially block here forever without a timeout. > yeah potentially until a hung task timeout. This is what we want since we can't make additional scm calls anyway. > As you are reusing completion, I have not seen any reinitialization of > completion, this could potentially return above line without waiting at > all. A complete would paired with a single waiter, so additional completes would be neccessary for it to go through without waiting. > >> + >> + return 0; >> +} >> + >> +static int qcom_scm_waitq_wakeup(struct qcom_scm *scm, unsigned int >> wq_ctx, bool wake_all) >> +{ >> + struct completion *wq_to_wake; >> + >> + wq_to_wake = qcom_scm_lookup_wq(scm, wq_ctx); >> + if (IS_ERR(wq_to_wake)) >> + return PTR_ERR(wq_to_wake); >> + >> + if (wake_all) >> + complete_all(wq_to_wake); >> + else >> + complete(wq_to_wake); > >> + >> + return 0; >> +} >> + >> +static irqreturn_t qcom_scm_irq_handler(int irq, void *data) >> +{ >> + int ret; >> + struct qcom_scm *scm = data; >> + u32 wq_ctx, flags, more_pending = 0; >> + >> + do { >> + ret = scm_get_wq_ctx(&wq_ctx, &flags, &more_pending); >> + if (ret) { >> + dev_err(scm->dev, "GET_WQ_CTX SMC call failed: %d\n", ret); >> + goto out; >> + } >> + >> + if (flags != QCOM_SMC_WAITQ_FLAG_WAKE_ONE && >> + flags != QCOM_SMC_WAITQ_FLAG_WAKE_ALL) { >> + dev_err(scm->dev, "Invalid flags found for wq_ctx: %u\n", >> flags); >> + goto out; >> + } >> + >> + ret = qcom_scm_waitq_wakeup(scm, wq_ctx, !!(flags & >> QCOM_SMC_WAITQ_FLAG_WAKE_ALL)); >> + if (ret) >> + goto out; >> + } while (more_pending); >> + >> +out: >> + return IRQ_HANDLED; >> +} >> + >> static int qcom_scm_probe(struct platform_device *pdev) >> { >> struct qcom_scm *scm; >> unsigned long clks; >> - int ret; >> + int irq, ret; >> scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL); >> if (!scm) >> @@ -1402,6 +1476,19 @@ static int qcom_scm_probe(struct >> platform_device *pdev) >> __scm = scm; >> __scm->dev = &pdev->dev; >> + init_completion(&__scm->waitq_comp); >> + >> + irq = platform_get_irq(pdev, 0); >> + if (irq < 0) { >> + if (irq != -ENXIO) >> + return irq; >> + } else { >> + ret = devm_request_threaded_irq(__scm->dev, irq, NULL, >> qcom_scm_irq_handler, >> + IRQF_ONESHOT, "qcom-scm", __scm); >> + if (ret < 0) >> + return dev_err_probe(scm->dev, ret, "Failed to request >> qcom-scm irq\n"); >> + } >> + >> __get_convention(); >> /* > > --srini
Hey Bjorn, Thanks for taking time to review the series. On 1/10/23 23:37, Bjorn Andersson wrote: > On Tue, Jan 10, 2023 at 12:07:45PM +0530, Sibi Sankar wrote: >> From: Guru Das Srinagesh <quic_gurus@quicinc.com> >> >> When the firmware (FW) supports multiple requests per VM, multiple requests >> from the same/different VM can reach the firmware at the same time. Since >> the firmware currently being used has limited resources, it guards them >> with a resource lock and puts requests on a wait-queue internally and >> signals to HLOS that it is doing so. It does this by returning a new return >> value in addition to success or error: SCM_WAITQ_SLEEP. A sleeping SCM call >> can be woken up by an interrupt that the FW raises. >> >> 1) SCM_WAITQ_SLEEP: >> >> When an SCM call receives this return value instead of success >> or error, FW has placed this call on a wait-queue and has signalled >> HLOS to put it to non-interruptible sleep. >> >> Along with this return value, FW also passes to HLOS `wq_ctx` - >> a unique number (UID) identifying the wait-queue that it has put >> the call on, internally. This is to help HLOS with its own >> bookkeeping to wake this sleeping call later. >> >> Additionally, FW also passes to HLOS `smc_call_ctx` - a UID >> identifying the SCM call thus being put to sleep. This is also >> for HLOS' bookkeeping to wake this call up later. >> >> These two additional values are passed via the a1 and a2 >> registers. >> >> N.B.: The "ctx" in the above UID names = "context". >> >> The handshake mechanism that HLOS uses to talk to FW about wait-queue >> operations involves two new SMC calls. >> >> 1) get_wq_ctx(): >> >> Arguments: None >> Returns: wq_ctx, flags, more_pending >> >> Get the wait-queue context, and wake up either one or all of the >> sleeping SCM calls associated with that wait-queue. >> >> Additionally, repeat this if there are more wait-queues that are >> ready to have their requests woken up (`more_pending`). >> >> 2) wq_resume(smc_call_ctx): >> >> Arguments: smc_call_ctx >> >> HLOS needs to issue this in response to receiving an >> IRQ, passing to FW the same smc_call_ctx that FW >> receives from HLOS via the get_wq_ctx() call. >> >> (The mechanism to wake a SMC call back up is described in detail below) >> >> VM_1 VM_2 Firmware >> │ │ │ >> │ │ │ >> │ │ │ >> │ │ │ >> │ REQUEST_1 │ │ >> ├────────────────────────┼─────────────────────────────────┤ >> │ │ │ >> │ │ ┌──┼──┐ >> │ │ │ │ │ >> │ │ REQUEST_2 │ │ │ >> │ ├──────────────────────────────┼──┤ │ >> │ │ │ │ │Resource >> │ │ │ │ │is busy >> │ │ {WQ_SLEEP} │ │ │ >> │ │◄─────────────────────────────┼──┤ │ >> │ │ wq_ctx, smc_call_ctx │ │ │ >> │ │ └──┼──┘ >> │ REQUEST_1 COMPLETE │ │ >> │◄───────────────────────┼─────────────────────────────────┤ >> │ │ │ >> │ │ IRQ │ >> │ │◄─-------------------------------│ >> │ │ │ >> │ │ get_wq_ctx() │ >> │ ├────────────────────────────────►│ >> │ │ │ >> │ │ │ >> │ │◄────────────────────────────────┤ >> │ │ wq_ctx, flags, and │ >> │ │ more_pending │ >> │ │ │ >> │ │ │ >> │ │ wq_resume(smc_call_ctx) │ >> │ ├────────────────────────────────►│ >> │ │ │ >> │ │ │ >> │ │ REQUEST_2 COMPLETE │ >> │ │◄────────────────────────────────┤ >> │ │ │ >> │ │ │ >> >> With the exception of get_wq_ctx(), the other SMC call wq_resume() can >> return WQ_SLEEP (these nested rounds of WQ_SLEEP are not shown in the >> above diagram for the sake of simplicity). Therefore, introduce a new >> do-while loop to handle multiple WQ_SLEEP return values for the same >> parent SCM call. >> >> Request Completion in the above diagram refers to either a success >> return value (zero) or error (and not SMC_WAITQ_SLEEP) >> >> Also add the interrupt handler that wakes up a sleeping SCM call. >> >> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com> >> Co-developed-by: Sibi Sankar <quic_sibis@quicinc.com> >> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >> --- >> >> v7: >> - Move lookup + wait_for_completion into a single function in qcom_scm [Bjorn] >> - Simplify completion retrieval [Bjorn] >> >> v6: >> - Fix misc. nits in the scm driver [Krzysztof] >> >> v5: >> - Handle the wake_one/wake_all flags [Guru] >> - Rename flag handler to qcom_scm_waitq_wakeup [Bjorn] >> - Resume scm call can return ebusy as well handle that scenario by retrying >> the original smc call and not the resume call >> >> v4: >> - platform_set_drvdata will be used by __scm_smc_do_quirk_handle_waitq to >> get access to scm struct from device so retain it >> - Use a single completion as it satisfies all of the current usecases [Bjorn] >> - Inline scm_get_wq_ctx [Bjorn] >> - Convert all pr_err to dev_err [Bjorn] >> - Handle idr_destroy in a thread safe manner [Bjorn] >> - Misc. Style fixes [Bjorn] >> >> v3: >> - Fixup irq handling so as not to affect SoCs without the interrupt. >> - Fix warnings reported by kernel test-bot. >> >> drivers/firmware/qcom_scm-smc.c | 90 ++++++++++++++++++++++++++++++--- >> drivers/firmware/qcom_scm.c | 89 +++++++++++++++++++++++++++++++- >> drivers/firmware/qcom_scm.h | 8 +++ >> 3 files changed, 179 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c >> index d111833364ba..30999f04749c 100644 >> --- a/drivers/firmware/qcom_scm-smc.c >> +++ b/drivers/firmware/qcom_scm-smc.c >> @@ -52,29 +52,101 @@ static void __scm_smc_do_quirk(const struct arm_smccc_args *smc, >> } while (res->a0 == QCOM_SCM_INTERRUPTED); >> } >> >> -static void __scm_smc_do(const struct arm_smccc_args *smc, >> - struct arm_smccc_res *res, bool atomic) >> +static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx) >> { >> - int retry_count = 0; >> + memset(resume->args, 0, sizeof(resume->args[0]) * ARRAY_SIZE(resume->args)); >> + >> + resume->args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL, >> + ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP, >> + SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_RESUME)); >> + >> + resume->args[1] = QCOM_SCM_ARGS(1); >> + >> + resume->args[2] = smc_call_ctx; >> +} >> + >> +int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending) >> +{ >> + int ret; >> + struct arm_smccc_args get_wq_ctx = {0}; >> + struct arm_smccc_res get_wq_res; >> + >> + get_wq_ctx.args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL, >> + ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP, >> + SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_GET_WQ_CTX)); >> + >> + /* Guaranteed to return only success or error, no WAITQ_* */ >> + __scm_smc_do_quirk(&get_wq_ctx, &get_wq_res); >> + ret = get_wq_res.a0; >> + if (ret) >> + return ret; >> + >> + *wq_ctx = get_wq_res.a1; >> + *flags = get_wq_res.a2; >> + *more_pending = get_wq_res.a3; >> + >> + return 0; >> +} >> + >> +static int __scm_smc_do_quirk_handle_waitq(struct device *dev, struct arm_smccc_args *waitq, >> + struct arm_smccc_res *res) >> +{ >> + int ret; >> + struct arm_smccc_args resume; >> + u32 wq_ctx, smc_call_ctx, flags; >> + struct arm_smccc_args *smc = waitq; >> + >> + do { >> + __scm_smc_do_quirk(smc, res); >> + >> + if (res->a0 == QCOM_SCM_WAITQ_SLEEP) { >> + wq_ctx = res->a1; >> + smc_call_ctx = res->a2; >> + flags = res->a3; >> + >> + if (!dev) >> + return -EPROBE_DEFER; >> + >> + ret = qcom_scm_lookup_completion(wq_ctx); >> + if (ret) >> + return ret; >> + >> + fill_wq_resume_args(&resume, smc_call_ctx); >> + smc = &resume; >> + } >> + } while (res->a0 == QCOM_SCM_WAITQ_SLEEP); >> + >> + return 0; >> +} >> + >> +static int __scm_smc_do(struct device *dev, struct arm_smccc_args *smc, >> + struct arm_smccc_res *res, bool atomic) >> +{ >> + int ret, retry_count = 0; >> >> if (atomic) { >> __scm_smc_do_quirk(smc, res); >> - return; >> + return 0; >> } >> >> do { >> mutex_lock(&qcom_scm_lock); >> >> - __scm_smc_do_quirk(smc, res); >> + ret = __scm_smc_do_quirk_handle_waitq(dev, smc, res); >> >> mutex_unlock(&qcom_scm_lock); >> >> + if (ret) >> + return ret; >> + >> 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); >> + >> + return 0; >> } >> >> >> @@ -83,7 +155,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc, >> struct qcom_scm_res *res, bool atomic) >> { >> int arglen = desc->arginfo & 0xf; >> - int i; >> + int i, ret; >> dma_addr_t args_phys = 0; >> void *args_virt = NULL; >> size_t alloc_len; >> @@ -135,13 +207,17 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc, >> smc.args[SCM_SMC_LAST_REG_IDX] = args_phys; >> } >> >> - __scm_smc_do(&smc, &smc_res, atomic); >> + /* ret error check follows after args_virt cleanup*/ >> + ret = __scm_smc_do(dev, &smc, &smc_res, atomic); >> >> if (args_virt) { >> dma_unmap_single(dev, args_phys, alloc_len, DMA_TO_DEVICE); >> kfree(args_virt); >> } >> >> + if (ret) >> + return ret; >> + >> if (res) { >> res->result[0] = smc_res.a1; >> res->result[1] = smc_res.a2; >> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >> index cdbfe54c8146..19ac506a9b1f 100644 >> --- a/drivers/firmware/qcom_scm.c >> +++ b/drivers/firmware/qcom_scm.c >> @@ -4,6 +4,7 @@ >> */ >> #include <linux/platform_device.h> >> #include <linux/init.h> >> +#include <linux/interrupt.h> >> #include <linux/cpumask.h> >> #include <linux/export.h> >> #include <linux/dma-mapping.h> >> @@ -13,6 +14,7 @@ >> #include <linux/qcom_scm.h> >> #include <linux/of.h> >> #include <linux/of_address.h> >> +#include <linux/of_irq.h> >> #include <linux/of_platform.h> >> #include <linux/clk.h> >> #include <linux/reset-controller.h> >> @@ -33,6 +35,7 @@ struct qcom_scm { >> struct clk *iface_clk; >> struct clk *bus_clk; >> struct icc_path *path; >> + struct completion waitq_comp; >> struct reset_controller_dev reset; >> >> /* control access to the interconnect path */ >> @@ -63,6 +66,9 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = { >> BIT(2), BIT(1), BIT(4), BIT(6) >> }; >> >> +#define QCOM_SMC_WAITQ_FLAG_WAKE_ONE BIT(0) >> +#define QCOM_SMC_WAITQ_FLAG_WAKE_ALL BIT(1) >> + >> static const char * const qcom_scm_convention_names[] = { >> [SMC_CONVENTION_UNKNOWN] = "unknown", >> [SMC_CONVENTION_ARM_32] = "smc arm 32", >> @@ -1325,11 +1331,79 @@ bool qcom_scm_is_available(void) >> } >> EXPORT_SYMBOL(qcom_scm_is_available); >> >> +static struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx) > > This could either be seen as a remnant of the older versions of this > patch, or a signalling of "place implementation here". I dislike both... > > Please rename it qcom_scm_assert_valid_wq_ctx() and just reference > &scm->waitq_comp directly in the two places below. ack > >> +{ >> + /* assert wq_ctx is zero */ >> + if (wq_ctx != 0) { >> + dev_err(scm->dev, "No waitqueue found for wq_ctx %d\n", wq_ctx); > > I think you should be more specific here, perhaps: > > "Firmware unexpectedly passed non-zero wq_ctx\n" > > And then as suggested a comment stating what needs to be done when that > happens. ack > >> + return ERR_PTR(-EINVAL); >> + } >> + >> + return &scm->waitq_comp; >> +} >> + >> +int qcom_scm_lookup_completion(u32 wq_ctx) >> +{ >> + struct completion *wq = NULL; > > You assign it on the very next line, no need to do so here. I'll just reference it directly from the scm struct. > >> + >> + wq = qcom_scm_lookup_wq(__scm, wq_ctx); >> + if (IS_ERR(wq)) >> + return PTR_ERR(wq); >> + >> + wait_for_completion(wq); >> + >> + return 0; >> +} >> + >> +static int qcom_scm_waitq_wakeup(struct qcom_scm *scm, unsigned int wq_ctx, bool wake_all) >> +{ >> + struct completion *wq_to_wake; > > "wq" would be sufficient. I'll just reference it directly from the scm struct. > > Thanks, > Bjorn > >> + >> + wq_to_wake = qcom_scm_lookup_wq(scm, wq_ctx); >> + if (IS_ERR(wq_to_wake)) >> + return PTR_ERR(wq_to_wake); >> + >> + if (wake_all) >> + complete_all(wq_to_wake); >> + else >> + complete(wq_to_wake); >> + >> + return 0; >> +} >> + >> +static irqreturn_t qcom_scm_irq_handler(int irq, void *data) >> +{ >> + int ret; >> + struct qcom_scm *scm = data; >> + u32 wq_ctx, flags, more_pending = 0; >> + >> + do { >> + ret = scm_get_wq_ctx(&wq_ctx, &flags, &more_pending); >> + if (ret) { >> + dev_err(scm->dev, "GET_WQ_CTX SMC call failed: %d\n", ret); >> + goto out; >> + } >> + >> + if (flags != QCOM_SMC_WAITQ_FLAG_WAKE_ONE && >> + flags != QCOM_SMC_WAITQ_FLAG_WAKE_ALL) { >> + dev_err(scm->dev, "Invalid flags found for wq_ctx: %u\n", flags); >> + goto out; >> + } >> + >> + ret = qcom_scm_waitq_wakeup(scm, wq_ctx, !!(flags & QCOM_SMC_WAITQ_FLAG_WAKE_ALL)); >> + if (ret) >> + goto out; >> + } while (more_pending); >> + >> +out: >> + return IRQ_HANDLED; >> +} >> + >> static int qcom_scm_probe(struct platform_device *pdev) >> { >> struct qcom_scm *scm; >> unsigned long clks; >> - int ret; >> + int irq, ret; >> >> scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL); >> if (!scm) >> @@ -1402,6 +1476,19 @@ static int qcom_scm_probe(struct platform_device *pdev) >> __scm = scm; >> __scm->dev = &pdev->dev; >> >> + init_completion(&__scm->waitq_comp); >> + >> + irq = platform_get_irq(pdev, 0); >> + if (irq < 0) { >> + if (irq != -ENXIO) >> + return irq; >> + } else { >> + ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler, >> + IRQF_ONESHOT, "qcom-scm", __scm); >> + if (ret < 0) >> + return dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n"); >> + } >> + >> __get_convention(); >> >> /* >> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h >> index db3d08a01209..018e9867d55a 100644 >> --- a/drivers/firmware/qcom_scm.h >> +++ b/drivers/firmware/qcom_scm.h >> @@ -60,6 +60,9 @@ struct qcom_scm_res { >> u64 result[MAX_QCOM_SCM_RETS]; >> }; >> >> +int qcom_scm_lookup_completion(u32 wq_ctx); I'll rename ^^ into qcom_scm_wait_for_wq_completion instead. >> +int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending); >> + >> #define SCM_SMC_FNID(s, c) ((((s) & 0xFF) << 8) | ((c) & 0xFF)) >> extern int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc, >> enum qcom_scm_convention qcom_convention, >> @@ -129,6 +132,10 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc, >> #define QCOM_SCM_SMMU_CONFIG_ERRATA1 0x03 >> #define QCOM_SCM_SMMU_CONFIG_ERRATA1_CLIENT_ALL 0x02 >> >> +#define QCOM_SCM_SVC_WAITQ 0x24 >> +#define QCOM_SCM_WAITQ_RESUME 0x02 >> +#define QCOM_SCM_WAITQ_GET_WQ_CTX 0x03 >> + >> /* common error codes */ >> #define QCOM_SCM_V2_EBUSY -12 >> #define QCOM_SCM_ENOMEM -5 >> @@ -137,6 +144,7 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc, >> #define QCOM_SCM_EINVAL_ARG -2 >> #define QCOM_SCM_ERROR -1 >> #define QCOM_SCM_INTERRUPTED 1 >> +#define QCOM_SCM_WAITQ_SLEEP 2 >> >> static inline int qcom_scm_remap_error(int err) >> { >> -- >> 2.17.1 >>
diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c index d111833364ba..30999f04749c 100644 --- a/drivers/firmware/qcom_scm-smc.c +++ b/drivers/firmware/qcom_scm-smc.c @@ -52,29 +52,101 @@ static void __scm_smc_do_quirk(const struct arm_smccc_args *smc, } while (res->a0 == QCOM_SCM_INTERRUPTED); } -static void __scm_smc_do(const struct arm_smccc_args *smc, - struct arm_smccc_res *res, bool atomic) +static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx) { - int retry_count = 0; + memset(resume->args, 0, sizeof(resume->args[0]) * ARRAY_SIZE(resume->args)); + + resume->args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL, + ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP, + SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_RESUME)); + + resume->args[1] = QCOM_SCM_ARGS(1); + + resume->args[2] = smc_call_ctx; +} + +int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending) +{ + int ret; + struct arm_smccc_args get_wq_ctx = {0}; + struct arm_smccc_res get_wq_res; + + get_wq_ctx.args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL, + ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP, + SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_GET_WQ_CTX)); + + /* Guaranteed to return only success or error, no WAITQ_* */ + __scm_smc_do_quirk(&get_wq_ctx, &get_wq_res); + ret = get_wq_res.a0; + if (ret) + return ret; + + *wq_ctx = get_wq_res.a1; + *flags = get_wq_res.a2; + *more_pending = get_wq_res.a3; + + return 0; +} + +static int __scm_smc_do_quirk_handle_waitq(struct device *dev, struct arm_smccc_args *waitq, + struct arm_smccc_res *res) +{ + int ret; + struct arm_smccc_args resume; + u32 wq_ctx, smc_call_ctx, flags; + struct arm_smccc_args *smc = waitq; + + do { + __scm_smc_do_quirk(smc, res); + + if (res->a0 == QCOM_SCM_WAITQ_SLEEP) { + wq_ctx = res->a1; + smc_call_ctx = res->a2; + flags = res->a3; + + if (!dev) + return -EPROBE_DEFER; + + ret = qcom_scm_lookup_completion(wq_ctx); + if (ret) + return ret; + + fill_wq_resume_args(&resume, smc_call_ctx); + smc = &resume; + } + } while (res->a0 == QCOM_SCM_WAITQ_SLEEP); + + return 0; +} + +static int __scm_smc_do(struct device *dev, struct arm_smccc_args *smc, + struct arm_smccc_res *res, bool atomic) +{ + int ret, retry_count = 0; if (atomic) { __scm_smc_do_quirk(smc, res); - return; + return 0; } do { mutex_lock(&qcom_scm_lock); - __scm_smc_do_quirk(smc, res); + ret = __scm_smc_do_quirk_handle_waitq(dev, smc, res); mutex_unlock(&qcom_scm_lock); + if (ret) + return ret; + 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); + + return 0; } @@ -83,7 +155,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc, struct qcom_scm_res *res, bool atomic) { int arglen = desc->arginfo & 0xf; - int i; + int i, ret; dma_addr_t args_phys = 0; void *args_virt = NULL; size_t alloc_len; @@ -135,13 +207,17 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc, smc.args[SCM_SMC_LAST_REG_IDX] = args_phys; } - __scm_smc_do(&smc, &smc_res, atomic); + /* ret error check follows after args_virt cleanup*/ + ret = __scm_smc_do(dev, &smc, &smc_res, atomic); if (args_virt) { dma_unmap_single(dev, args_phys, alloc_len, DMA_TO_DEVICE); kfree(args_virt); } + if (ret) + return ret; + if (res) { res->result[0] = smc_res.a1; res->result[1] = smc_res.a2; diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c index cdbfe54c8146..19ac506a9b1f 100644 --- a/drivers/firmware/qcom_scm.c +++ b/drivers/firmware/qcom_scm.c @@ -4,6 +4,7 @@ */ #include <linux/platform_device.h> #include <linux/init.h> +#include <linux/interrupt.h> #include <linux/cpumask.h> #include <linux/export.h> #include <linux/dma-mapping.h> @@ -13,6 +14,7 @@ #include <linux/qcom_scm.h> #include <linux/of.h> #include <linux/of_address.h> +#include <linux/of_irq.h> #include <linux/of_platform.h> #include <linux/clk.h> #include <linux/reset-controller.h> @@ -33,6 +35,7 @@ struct qcom_scm { struct clk *iface_clk; struct clk *bus_clk; struct icc_path *path; + struct completion waitq_comp; struct reset_controller_dev reset; /* control access to the interconnect path */ @@ -63,6 +66,9 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = { BIT(2), BIT(1), BIT(4), BIT(6) }; +#define QCOM_SMC_WAITQ_FLAG_WAKE_ONE BIT(0) +#define QCOM_SMC_WAITQ_FLAG_WAKE_ALL BIT(1) + static const char * const qcom_scm_convention_names[] = { [SMC_CONVENTION_UNKNOWN] = "unknown", [SMC_CONVENTION_ARM_32] = "smc arm 32", @@ -1325,11 +1331,79 @@ bool qcom_scm_is_available(void) } EXPORT_SYMBOL(qcom_scm_is_available); +static struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx) +{ + /* assert wq_ctx is zero */ + if (wq_ctx != 0) { + dev_err(scm->dev, "No waitqueue found for wq_ctx %d\n", wq_ctx); + return ERR_PTR(-EINVAL); + } + + return &scm->waitq_comp; +} + +int qcom_scm_lookup_completion(u32 wq_ctx) +{ + struct completion *wq = NULL; + + wq = qcom_scm_lookup_wq(__scm, wq_ctx); + if (IS_ERR(wq)) + return PTR_ERR(wq); + + wait_for_completion(wq); + + return 0; +} + +static int qcom_scm_waitq_wakeup(struct qcom_scm *scm, unsigned int wq_ctx, bool wake_all) +{ + struct completion *wq_to_wake; + + wq_to_wake = qcom_scm_lookup_wq(scm, wq_ctx); + if (IS_ERR(wq_to_wake)) + return PTR_ERR(wq_to_wake); + + if (wake_all) + complete_all(wq_to_wake); + else + complete(wq_to_wake); + + return 0; +} + +static irqreturn_t qcom_scm_irq_handler(int irq, void *data) +{ + int ret; + struct qcom_scm *scm = data; + u32 wq_ctx, flags, more_pending = 0; + + do { + ret = scm_get_wq_ctx(&wq_ctx, &flags, &more_pending); + if (ret) { + dev_err(scm->dev, "GET_WQ_CTX SMC call failed: %d\n", ret); + goto out; + } + + if (flags != QCOM_SMC_WAITQ_FLAG_WAKE_ONE && + flags != QCOM_SMC_WAITQ_FLAG_WAKE_ALL) { + dev_err(scm->dev, "Invalid flags found for wq_ctx: %u\n", flags); + goto out; + } + + ret = qcom_scm_waitq_wakeup(scm, wq_ctx, !!(flags & QCOM_SMC_WAITQ_FLAG_WAKE_ALL)); + if (ret) + goto out; + } while (more_pending); + +out: + return IRQ_HANDLED; +} + static int qcom_scm_probe(struct platform_device *pdev) { struct qcom_scm *scm; unsigned long clks; - int ret; + int irq, ret; scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL); if (!scm) @@ -1402,6 +1476,19 @@ static int qcom_scm_probe(struct platform_device *pdev) __scm = scm; __scm->dev = &pdev->dev; + init_completion(&__scm->waitq_comp); + + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + if (irq != -ENXIO) + return irq; + } else { + ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler, + IRQF_ONESHOT, "qcom-scm", __scm); + if (ret < 0) + return dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n"); + } + __get_convention(); /* diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h index db3d08a01209..018e9867d55a 100644 --- a/drivers/firmware/qcom_scm.h +++ b/drivers/firmware/qcom_scm.h @@ -60,6 +60,9 @@ struct qcom_scm_res { u64 result[MAX_QCOM_SCM_RETS]; }; +int qcom_scm_lookup_completion(u32 wq_ctx); +int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending); + #define SCM_SMC_FNID(s, c) ((((s) & 0xFF) << 8) | ((c) & 0xFF)) extern int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc, enum qcom_scm_convention qcom_convention, @@ -129,6 +132,10 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc, #define QCOM_SCM_SMMU_CONFIG_ERRATA1 0x03 #define QCOM_SCM_SMMU_CONFIG_ERRATA1_CLIENT_ALL 0x02 +#define QCOM_SCM_SVC_WAITQ 0x24 +#define QCOM_SCM_WAITQ_RESUME 0x02 +#define QCOM_SCM_WAITQ_GET_WQ_CTX 0x03 + /* common error codes */ #define QCOM_SCM_V2_EBUSY -12 #define QCOM_SCM_ENOMEM -5 @@ -137,6 +144,7 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc, #define QCOM_SCM_EINVAL_ARG -2 #define QCOM_SCM_ERROR -1 #define QCOM_SCM_INTERRUPTED 1 +#define QCOM_SCM_WAITQ_SLEEP 2 static inline int qcom_scm_remap_error(int err) {