Re: [PATCH RFC v2 07/11] remoteproc: assign fwnode during vdev registration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] remoteproc: unreleased reserved memory before register_virtio_device()
--

commit 3f46613846e39fe7244229a8a47044d9f38bc9cb
Author: Francesco Valla <francesco@valla.it>

remoteproc: assign fwnode during vdev registration

This commit looks up a child firmware node describing a virtio device
during vdev registration and assigns it to the rproc-virtio platform
device, using its memory-region when no parent carveout is provided.

> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index 1496659ddff9e..950d65480be59 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
[ ... ]
> @@ -394,6 +396,23 @@ static void rproc_virtio_dev_release(struct device *dev)
>  	put_device(&rvdev->pdev->dev);
>  }
>  
> +/*
> + * Attach the memory region declared inside the vdev's own devicetree node.
> + *
> + * A restricted-dma-pool region is attached by of_dma_configure() when the
> + * platform device is probed, but it is released together with the virtio
> + * device (see rproc_virtio_dev_release()); attach it again if needed, e.g.
> + * when the remote processor is restarted during a crash recovery.
> + * Any other kind of region (e.g., shared-dma-pool) is attached only here.
> + */
> +static int rproc_virtio_init_vdev_mem(struct device *dev)
> +{
> +	if (is_swiotlb_for_alloc(dev))
> +		return 0;
> +
> +	return of_reserved_mem_device_init_by_idx(dev, dev->of_node, 0);
> +}
> +
>  /**
>   * rproc_add_virtio_dev() - register an rproc-induced virtio device
>   * @rvdev: the remote vdev
> @@ -410,6 +429,7 @@ static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>  	struct device *dev = &rvdev->pdev->dev;
>  	struct virtio_device *vdev;
>  	struct rproc_mem_entry *mem;
> +	bool has_mem_region;
>  	int ret;
>  
>  	if (rproc->ops->kick == NULL) {
> @@ -417,11 +437,24 @@ static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>  		return -EINVAL;
>  	}
>  
> +	has_mem_region = of_property_present(dev->of_node, "memory-region");
> +
>  	/* Try to find dedicated vdev buffer carveout */
>  	mem = rproc_find_carveout_by_name(rproc, "vdev%dbuffer", rvdev->index);
>  	if (mem) {
>  		phys_addr_t pa;
>  
> +		if (has_mem_region) {
> +			/*
> +			 * The carveout takes precedence: drop the region that
> +			 * of_dma_configure() might have attached at probe time.
> +			 */
> +			dev_warn(dev,
> +				 "vdev%dbuffer carveout found, ignoring memory-region\n",
> +				 rvdev->index);
> +			of_reserved_mem_device_release(dev);
> +		}
> +
>  		if (mem->of_resm_idx != -1) {
>  			struct device_node *np = rproc->dev.parent->of_node;
>  
> @@ -451,6 +484,14 @@ static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>  				return ret;
>  			}
>  		}
> +	} else if (has_mem_region) {
> +		/* Use the memory region declared in the vdev's own node */
> +		ret = rproc_virtio_init_vdev_mem(dev);
> +		if (ret) {
> +			dev_err(dev, "Can't associate vdev memory-region: %d\n",
> +				ret);
> +			return ret;
> +		}

[Severity: High]
What happens to the reserved memory attached to &rvdev->pdev->dev if the
virtio device is never registered?

When rproc_handle_vdev() sets pdev_info.fwnode and registers the platform
device via platform_device_register_full(), a restricted-dma-pool region
in the vdev node is attached during probe by:

  really_probe()
    -> platform_dma_configure()
      -> of_dma_configure()
        -> of_dma_configure_id()
          -> of_dma_set_restricted_buffer()
            -> of_reserved_mem_device_init_by_idx()

This allocates a struct rmem_assigned_device entry pointing to &pdev->dev
and links it into the global of_rmem_assigned_device_list.

Similarly, in rproc_add_virtio_dev(), rproc_virtio_init_vdev_mem() (or the
carveout / parent fallback paths) attaches reserved or coherent memory to
&rvdev->pdev->dev before vdev = kzalloc_obj(*vdev) is called:

  rproc_add_virtio_dev() {
      ...
      } else if (has_mem_region) {
          ret = rproc_virtio_init_vdev_mem(dev);
          ...
      }
      vdev = kzalloc_obj(*vdev);
      if (!vdev)
          return -ENOMEM;
  }

However, of_reserved_mem_device_release(&rvdev->pdev->dev) and
dma_release_coherent_memory(&rvdev->pdev->dev) are only called in
rproc_virtio_dev_release(), not in rproc_virtio_remove() or in the error
paths of rproc_virtio_probe() and rproc_add_virtio_dev().

If rproc_virtio_probe() fails, if rproc_fw_boot() or rproc_attach() fails
after rproc_handle_vdev() (for example in rproc_alloc_registered_carveouts(),
rproc_load_segments(), rproc_prepare_subdevices(), or rproc->ops->start()),
or if rproc_add_virtio_dev() fails when rproc->ops->kick is NULL or when
kzalloc_obj(*vdev) returns NULL, rproc_virtio_dev_release() never runs.

When rproc_resource_cleanup() then unregisters and frees rvdev->pdev,
doesn't that leak the struct rmem_assigned_device entry on
of_rmem_assigned_device_list with a dangling rd->dev pointer to the freed
platform device (and leave the memory region attached across crash
recovery attempts so dma_assign_coherent_memory() fails with -EBUSY)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-remoteproc_virtio_map-v2-0-26b300e01c62@valla.it?part=7