Re: [PATCH v2 2/3] firmware: qcom: scm: Fix reserved memory cleanup on probe failure

[Mukesh Ojha]

On Tue, Jun 30, 2026 at 12:07:26PM +0200, Konrad Dybcio wrote:
> On 6/29/26 4:17 PM, Mukesh Ojha wrote:
> > of_reserved_mem_device_init() adds an entry to a global list with no
> > devres counterpart. If qcom_scm_probe() fails after the call the
> > assignment is never cleaned up. A probe retry would add a duplicate
> > entry, leaking the original one permanently.
> > 
> > Add an err_rmem label that calls of_reserved_mem_device_release() and
> > route all error paths after of_reserved_mem_device_init() through it.
> > of_reserved_mem_device_release() is safe to call unconditionally as it
> > simply walks an empty list when nothing was assigned.
> > 
> > Fixes: a33b2579c8d3 ("firmware: qcom: scm: add support for SHM bridge memory carveout")
> > Signed-off-by: Mukesh Ojha <mukesh.ojha@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/firmware/qcom/qcom_scm.c | 21 +++++++++++++++------
> >  1 file changed, 15 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > index 464ae3b4ca43..f0e19fc314b4 100644
> > --- a/drivers/firmware/qcom/qcom_scm.c
> > +++ b/drivers/firmware/qcom/qcom_scm.c
> > @@ -2785,9 +2785,11 @@ static int qcom_scm_probe(struct platform_device *pdev)
> >  				     "Failed to setup the reserved memory region for TZ mem\n");
> >  
> >  	ret = qcom_tzmem_enable(scm->dev);
> > -	if (ret)
> > -		return dev_err_probe(scm->dev, ret,
> > -				     "Failed to enable the TrustZone memory allocator\n");
> > +	if (ret) {
> > +		dev_err_probe(scm->dev, ret,
> > +			      "Failed to enable the TrustZone memory allocator\n");
> 
> Assign ret = dev_err_probe to preserve the usefulness of this API
>

Sure.

> Alternatively, I cobbled this together, feel free to take it forward
> (compile-tested only):


Sure, thank you along with all the clean up.

-Mukesh
> 
> From 917c849fd21c6660ba0bd55f8b6ce4cb5dfc8299 Mon Sep 17 00:00:00 2001
> From: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>
> Date: Tue, 30 Jun 2026 12:02:16 +0200
> Subject: [PATCH] of: reserved_mem: Introduce devres-managed initialization
>  functions
> 
> Introduce devres-based helpers for of_reserved_mem_device_init(_by_idx)
> to help fight dangling references and ever so slightly reduce the
> number of boilerplate deinitialization calls.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>
> ---
>  drivers/of/of_reserved_mem.c    | 37 +++++++++++++++++++++++++++++++++
>  include/linux/of_reserved_mem.h | 25 ++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 82222bd45ac6..79fa04d4cf04 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -787,6 +787,43 @@ void of_reserved_mem_device_release(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(of_reserved_mem_device_release);
>  
> +static void devm_of_reserved_mem_device_release(struct device *dev, void *res)
> +{
> +	of_reserved_mem_device_release(*(struct device **)res);
> +}
> +
> +/**
> + * devm_of_reserved_mem_device_init_by_idx - Resource managed of_reserved_mem_device_init_by_idx()
> + * @dev: Pointer to the device to configure
> + *
> + * This function assigns respective DMA-mapping operations based on the first
> + * reserved memory region specified by 'memory-region' property in device tree
> + * node of the given device.
> + *
> + * Returns: Negative errno on failure or zero on success.
> + */
> +int devm_of_reserved_mem_device_init_by_idx(struct device *dev, struct device_node *np, int idx)
> +{
> +	struct device **ptr;
> +	int ret;
> +
> +	ptr = devres_alloc(devm_of_reserved_mem_device_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return -ENOMEM;
> +
> +	ret = of_reserved_mem_device_init_by_idx(dev, np, idx);
> +	if (ret) {
> +		devres_free(ptr);
> +		return ret;
> +	}
> +
> +	*ptr = dev;
> +	devres_add(dev, ptr);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_of_reserved_mem_device_init_by_idx);
> +
>  /**
>   * of_reserved_mem_lookup() - acquire reserved_mem from a device node
>   * @np:		node pointer of the desired reserved-memory region
> diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h
> index e8b20b29fa68..28beeeb91f4e 100644
> --- a/include/linux/of_reserved_mem.h
> +++ b/include/linux/of_reserved_mem.h
> @@ -47,6 +47,8 @@ int of_reserved_mem_region_to_resource_byname(const struct device_node *np,
>  					      const char *name, struct resource *res);
>  int of_reserved_mem_region_count(const struct device_node *np);
>  
> +int devm_of_reserved_mem_device_init_by_idx(struct device *dev, struct device_node *np, int idx);
> +
>  #else
>  
>  #define RESERVEDMEM_OF_DECLARE(name, compat, ops)			\
> @@ -91,6 +93,14 @@ static inline int of_reserved_mem_region_count(const struct device_node *np)
>  {
>  	return 0;
>  }
> +
> +static inline int devm_of_reserved_mem_device_init_by_idx(struct device *dev,
> +							  struct device_node *np,
> +							  int idx)
> +{
> +	return -ENOSYS;
> +}
> +
>  #endif
>  
>  /**
> @@ -108,4 +118,19 @@ static inline int of_reserved_mem_device_init(struct device *dev)
>  	return of_reserved_mem_device_init_by_idx(dev, dev->of_node, 0);
>  }
>  
> +/**
> + * of_reserved_mem_device_init() - Devres-managed version of of_reserved_mem_device_init()
> + * @dev:	Pointer to the device to configure
> + *
> + * This function assigns respective DMA-mapping operations based on the first
> + * reserved memory region specified by 'memory-region' property in device tree
> + * node of the given device.
> + *
> + * Returns error code or zero on success.
> + */
> +static inline int devm_of_reserved_mem_device_init(struct device *dev)
> +{
> +	return devm_of_reserved_mem_device_init_by_idx(dev, dev->of_node, 0);
> +}
> +
>  #endif /* __OF_RESERVED_MEM_H */
> -- 
> 2.54.0
> 
> 
> Konrad

-- 
-Mukesh Ojha