Messages in this thread | | | Date | Wed, 11 Jan 2023 15:04:05 +0530 | Subject | Re: [PATCH V7 2/2] firmware: qcom: scm: Add wait-queue handling logic | From | Sibi Sankar <> |
| |
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
| |