Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 15 Jun 2023 16:51:35 +0200
From:   Stephan Gerhold <stephan@gerhold.net>
To:     Bjorn Andersson <andersson@kernel.org>,
        Caleb Connolly <caleb.connolly@linaro.org>
Cc:     Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] remoteproc: qcom: Handle reserved-memory
 allocation issues
Message-ID: <ZIsld-MAdkKvdzTx@gerhold.net>
References: <20230529-rproc-of-rmem-v2-0-95e39b959585@gerhold.net>
 <20230529-rproc-of-rmem-v2-1-95e39b959585@gerhold.net>
 <c3f59fb4-4dd8-f27a-d3f5-b1870006a75c@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c3f59fb4-4dd8-f27a-d3f5-b1870006a75c@linaro.org>
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, Jun 15, 2023 at 11:44:06AM +0100, Caleb Connolly wrote:
> On 6/14/23 17:31, Stephan Gerhold wrote:
> > If Linux fails to allocate the dynamic reserved memory specified in the
> > device tree, the size of the reserved_mem will be 0. Add a check for
> > this to avoid using an invalid reservation.
> > 
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> 
> Other uses of of_reserved_mem_lookup() also have unchecked uses of rmem [1],
> or check different things [2].
> 
> Does it make sense to put this check in the function itself?
> 
> I can't think of any obvious scenarios where it makes sense to differentiate
> between rmem being NULL vs having a size of zero at the time where a driver
> is fetching it.
> 
> As Bjorn described in the rmtfs patch, the memory allocation is essentially
> ignored, wouldn't it be better to print an error and invalidate the rmem in
> [3]?
> 

"Invalidating" isn't that easy because the reserved_mem is currently
stored in a simple array. Removing an entry would require shifting all
following values. But I suppose it would be easy to add the rmem->size
!= 0 check in of_reserved_mem_lookup() so it doesn't have to be checked
on all usages.

Given that no one seems to check for this at the moment I'm inclined to
agree with you that it would be better to handle this directly in
of_reserved_mem. Bjorn, what do you think?

Thanks,
Stephan

> [1]: https://elixir.bootlin.com/linux/v6.4-rc6/source/drivers/net/ethernet/mediatek/mtk_wed.c#L818
> [2]: https://elixir.bootlin.com/linux/v6.4-rc6/source/drivers/remoteproc/rcar_rproc.c#L71
> [3]: https://elixir.bootlin.com/linux/v6.4-rc6/source/drivers/of/of_reserved_mem.c#L276
> 
> // Caleb (they/them)
> > ---
> > New patch in v2, I wasn't aware of this until Bjorn posted a similar
> > patch for rmtfs:
> > https://lore.kernel.org/linux-arm-msm/20230530233643.4044823-4-quic_bjorande@quicinc.com/
> > ---
> >   drivers/remoteproc/qcom_q6v5_mss.c  | 2 +-
> >   drivers/remoteproc/qcom_q6v5_wcss.c | 2 +-
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> > index 70bffc9f33f6..a35ab6e860f3 100644
> > --- a/drivers/remoteproc/qcom_q6v5_mss.c
> > +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> > @@ -1932,7 +1932,7 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
> >   		return 0;
> >   	rmem = of_reserved_mem_lookup(node);
> > -	if (!rmem) {
> > +	if (!rmem || !rmem->size) {
> >   		dev_err(qproc->dev, "unable to resolve metadata region\n");
> >   		return -EINVAL;
> >   	}
> > diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c b/drivers/remoteproc/qcom_q6v5_wcss.c
> > index b437044aa126..9edab9d60c21 100644
> > --- a/drivers/remoteproc/qcom_q6v5_wcss.c
> > +++ b/drivers/remoteproc/qcom_q6v5_wcss.c
> > @@ -882,7 +882,7 @@ static int q6v5_alloc_memory_region(struct q6v5_wcss *wcss)
> >   		rmem = of_reserved_mem_lookup(node);
> >   	of_node_put(node);
> > -	if (!rmem) {
> > +	if (!rmem || !rmem->size) {
> >   		dev_err(dev, "unable to acquire memory-region\n");
> >   		return -EINVAL;
> >   	}
> >