Re: [PATCH v4 2/4] firmware: qcom_scm: Clear scm pointer on probe failure

[Maximilian Luz]

First off, sorry again for the long delay and thanks for being patient
with me (and for the review of course). I'm finally getting back to
finding some time for Linux things again, so I think I've mostly settled
in by now.

On 6/28/23 13:20, Johan Hovold wrote:
> On Mon, May 29, 2023 at 01:03:49AM +0200, Maximilian Luz wrote:
> > When setting-up the IRQ goes wrong, the __scm pointer currently remains
> > set even though we fail to probe the driver successfully. Due to this,
> > access to __scm may go wrong since associated resources (clocks, ...)
> > have been released. Therefore, clear the __scm pointer when setting-up
> > the IRQ fails.
> >
> > Fixes: 6bf325992236 ("firmware: qcom: scm: Add wait-queue handling logic")
> > Signed-off-by: Maximilian Luz <luzmaximilian@xxxxxxxxx>
> > ---
> >
> > Patch introduced in v4
> >
> > ---
> >   drivers/firmware/qcom_scm.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > index fde33acd46b7..d0070b833889 100644
> > --- a/drivers/firmware/qcom_scm.c
> > +++ b/drivers/firmware/qcom_scm.c
> > @@ -1488,8 +1488,10 @@ static int qcom_scm_probe(struct platform_device *pdev)
> >   	} else {
> >   		ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler,
> >   						IRQF_ONESHOT, "qcom-scm", __scm);
> > -		if (ret < 0)
> > +		if (ret < 0) {
> > +			__scm = NULL;
>
> This looks fragile at best. Clients use qcom_scm_is_available() to see
> if __scm is available and do not expect it to go away once it is live.

Hmm, you're right. The whole situation is probably not ideal and that
fix is really just a bad band-aid.

> It looks like you can hold off on initialising __scm until you've
> requested the interrupt, either by using IRQ_NOAUTOEN or fixing
> qcom_scm_waitq_wakeup() so that it doesn't use __scm directly.
>
> That would also take care of the previous branch which may also leave
> __scm set after the structure itself has been released on errors.

Agreed.

> You'll have similar problems when registering qseecom which currently
> depend on __scm being set, though. Clearing the pointer in that case is
> clearly broken as you currently rely on devres for deregistering the aux
> clients on errors (i.e. the clients using __scm are still registered
> when you clear the pointer in patch 3/4).

Oh right, I hadn't thought of that. I'll have to rework that.

> >   			return dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n");
> > +		}
> >   	}
> >   
> >   	__get_convention();
>
> Johan