Messages in this thread | | | Date | Tue, 10 Jan 2023 00:14:11 -0800 | From | Guru Das Srinagesh <> | Subject | Re: [PATCH V7 2/2] firmware: qcom: scm: Add wait-queue handling logic |
| |
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/
| |