Messages in this thread | | | Date | Tue, 10 Jan 2023 12:14:54 +0000 | Subject | Re: [PATCH V7 2/2] firmware: qcom: scm: Add wait-queue handling logic | From | Srinivas Kandagatla <> |
| |
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
| |