Re: [PATCH V6 2/2] firmware: qcom: scm: Add wait-queue handling logic

[Bjorn Andersson]

On Thu, Dec 08, 2022 at 12:10:31PM +0530, Sibi Sankar wrote:
> diff --git 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)
> +{
> +	struct qcom_scm *scm;
> +	struct completion *wq = NULL;
> +	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;
> +
> +			scm = dev_get_drvdata(dev);
> +			wq = qcom_scm_lookup_wq(scm, wq_ctx);
> +			if (IS_ERR_OR_NULL(wq)) {
> +				dev_err(dev, "No waitqueue found for wq_ctx %d: %ld\n",
> +					wq_ctx, PTR_ERR(wq));
> +				return PTR_ERR(wq) ? : -EINVAL;
> +			}
> +
> +			wait_for_completion(wq);

I think it would be cleaner to push the lookup + wait_for_completion
into a function in qcom_scm.c. Then you don't need to pull the drvdata
and you have the wq handling grouped in one place.

> +			fill_wq_resume_args(&resume, smc_call_ctx);
> +			smc = &resume;
> +			wq = NULL;
> +		}
> +	} while (res->a0 == QCOM_SCM_WAITQ_SLEEP);
> +
> +	return 0;
> +}
[..]
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
[..]
> +struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx)
> +{
> +	int err;
> +	unsigned long flags;
> +	u32 wq_ctx_idr = wq_ctx;
> +	struct completion *wq = NULL;
> +
> +	spin_lock_irqsave(&scm->waitq.idr_lock, flags);
> +	wq = idr_find(&scm->waitq.idr, wq_ctx);
> +	if (wq)
> +		goto out;
> +
> +	wq = &scm->waitq.waitq_comp;

The idr here gives an impression of providing similar functionality as
in the previous post, but will actually always provide the same
completion. As such, if the firmware would start to use multiple wq_ctx
I believe this would should up as something fairly non-trivial to debug.

I think it's better to make this dead simple and assert that wq_ctx is 0
and just return the one and only completion.

> +
> +	err = idr_alloc_u32(&scm->waitq.idr, wq, &wq_ctx_idr,
> +			    U32_MAX, GFP_ATOMIC);

PS. Thinking about it further, imagine the firmware people deciding to
be funny and allocating the wq_ctx in a cyclic fashion. The idr will
consume all your ram after a while...

Regards,
Bjorn

> +	if (err < 0)
> +		wq = ERR_PTR(err);
> +
> +out:
> +	spin_unlock_irqrestore(&scm->waitq.idr_lock, flags);
> +	return wq;
> +}