Re: [PATCH V1] accel/amdxdna: Split mailbox channel create function

["Mario Limonciello (AMD) (kernel.org)" <superm1@kernel.org>]

On 3/5/2026 12:20 AM, Lizhi Hou wrote:
> The management channel used for firmware control command submission is
> currently created after the firmware is started. If channel creation
> fails (for example, due to memory allocation failure or workqueue
> creation interruption), the firmware remains in a pending state and is
> unable to receive any control commands.
> 
> To avoid leaving the firmware in this inconsistent state, split
> xdna_mailbox_create_channel() into two separate functions so that
> resource allocation can be completed before interacting with the
> hardware.
>    xdna_mailbox_alloc_channel()
>      Allocates memory and initializes the workqueue. This can be called
>      earlier, before interacting with the hardware.
>    xdna_mailbox_start_channel()
>      Performs the hardware interaction required to start the channel.
> 
> Rename xdna_mailbox_destroy_channel() to xdna_mailbox_free_channel().
> Ensure that xdna_mailbox_stop_channel() and xdna_mailbox_free_channel()
> properly unwind the corresponding start and allocation steps, respectively.
> 
> Fixes: b87f920b9344 ("accel/amdxdna: Support hardware mailbox")
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
> ---
>   drivers/accel/amdxdna/aie2_message.c    | 17 +++--
>   drivers/accel/amdxdna/aie2_pci.c        | 63 ++++++++++-------
>   drivers/accel/amdxdna/amdxdna_mailbox.c | 91 ++++++++++++-------------
>   drivers/accel/amdxdna/amdxdna_mailbox.h | 31 +++++----
>   4 files changed, 112 insertions(+), 90 deletions(-)
> 
> diff --git a/drivers/accel/amdxdna/aie2_message.c b/drivers/accel/amdxdna/aie2_message.c
> index 22e1a85a7ae0..ffcf3be79e23 100644
> --- a/drivers/accel/amdxdna/aie2_message.c
> +++ b/drivers/accel/amdxdna/aie2_message.c
> @@ -293,13 +293,20 @@ int aie2_create_context(struct amdxdna_dev_hdl *ndev, struct amdxdna_hwctx *hwct
>   	}
>   
>   	intr_reg = i2x.mb_head_ptr_reg + 4;
> -	hwctx->priv->mbox_chann = xdna_mailbox_create_channel(ndev->mbox, &x2i, &i2x,
> -							      intr_reg, ret);
> +	hwctx->priv->mbox_chann = xdna_mailbox_alloc_channel(ndev->mbox);
>   	if (!hwctx->priv->mbox_chann) {
>   		XDNA_ERR(xdna, "Not able to create channel");
>   		ret = -EINVAL;
>   		goto del_ctx_req;
>   	}
> +
> +	ret = xdna_mailbox_start_channel(hwctx->priv->mbox_chann, &x2i, &i2x,
> +					 intr_reg, ret);
> +	if (ret) {
> +		XDNA_ERR(xdna, "Not able to create channel");
> +		ret = -EINVAL;
> +		goto free_channel;
> +	}
>   	ndev->hwctx_num++;
>   
>   	XDNA_DBG(xdna, "Mailbox channel irq: %d, msix_id: %d", ret, resp.msix_id);
> @@ -307,6 +314,8 @@ int aie2_create_context(struct amdxdna_dev_hdl *ndev, struct amdxdna_hwctx *hwct
>   
>   	return 0;
>   
> +free_channel:
> +	xdna_mailbox_free_channel(hwctx->priv->mbox_chann);
>   del_ctx_req:
>   	aie2_destroy_context_req(ndev, hwctx->fw_ctx_id);
>   	return ret;
> @@ -322,7 +331,7 @@ int aie2_destroy_context(struct amdxdna_dev_hdl *ndev, struct amdxdna_hwctx *hwc
>   
>   	xdna_mailbox_stop_channel(hwctx->priv->mbox_chann);
>   	ret = aie2_destroy_context_req(ndev, hwctx->fw_ctx_id);
> -	xdna_mailbox_destroy_channel(hwctx->priv->mbox_chann);
> +	xdna_mailbox_free_channel(hwctx->priv->mbox_chann);
>   	XDNA_DBG(xdna, "Destroyed fw ctx %d", hwctx->fw_ctx_id);
>   	hwctx->priv->mbox_chann = NULL;
>   	hwctx->fw_ctx_id = -1;
> @@ -921,7 +930,7 @@ void aie2_destroy_mgmt_chann(struct amdxdna_dev_hdl *ndev)
>   		return;
>   
>   	xdna_mailbox_stop_channel(ndev->mgmt_chann);
> -	xdna_mailbox_destroy_channel(ndev->mgmt_chann);
> +	xdna_mailbox_free_channel(ndev->mgmt_chann);
>   	ndev->mgmt_chann = NULL;
>   }
>   
> diff --git a/drivers/accel/amdxdna/aie2_pci.c b/drivers/accel/amdxdna/aie2_pci.c
> index 977ce21eaf9f..4924a9da55b6 100644
> --- a/drivers/accel/amdxdna/aie2_pci.c
> +++ b/drivers/accel/amdxdna/aie2_pci.c
> @@ -361,10 +361,29 @@ static int aie2_hw_start(struct amdxdna_dev *xdna)
>   	}
>   	pci_set_master(pdev);
>   
> +	mbox_res.ringbuf_base = ndev->sram_base;
> +	mbox_res.ringbuf_size = pci_resource_len(pdev, xdna->dev_info->sram_bar);
> +	mbox_res.mbox_base = ndev->mbox_base;
> +	mbox_res.mbox_size = MBOX_SIZE(ndev);
> +	mbox_res.name = "xdna_mailbox";
> +	ndev->mbox = xdnam_mailbox_create(&xdna->ddev, &mbox_res);
> +	if (!ndev->mbox) {
> +		XDNA_ERR(xdna, "failed to create mailbox device");
> +		ret = -ENODEV;
> +		goto disable_dev;
> +	}
> +
> +	ndev->mgmt_chann = xdna_mailbox_alloc_channel(ndev->mbox);
> +	if (!ndev->mgmt_chann) {
> +		XDNA_ERR(xdna, "failed to alloc channel");
> +		ret = -ENODEV;
> +		goto disable_dev;
> +	}
> +
>   	ret = aie2_smu_init(ndev);
>   	if (ret) {
>   		XDNA_ERR(xdna, "failed to init smu, ret %d", ret);
> -		goto disable_dev;
> +		goto free_channel;
>   	}
>   
>   	ret = aie2_psp_start(ndev->psp_hdl);
> @@ -379,18 +398,6 @@ static int aie2_hw_start(struct amdxdna_dev *xdna)
>   		goto stop_psp;
>   	}
>   
> -	mbox_res.ringbuf_base = ndev->sram_base;
> -	mbox_res.ringbuf_size = pci_resource_len(pdev, xdna->dev_info->sram_bar);
> -	mbox_res.mbox_base = ndev->mbox_base;
> -	mbox_res.mbox_size = MBOX_SIZE(ndev);
> -	mbox_res.name = "xdna_mailbox";
> -	ndev->mbox = xdnam_mailbox_create(&xdna->ddev, &mbox_res);
> -	if (!ndev->mbox) {
> -		XDNA_ERR(xdna, "failed to create mailbox device");
> -		ret = -ENODEV;
> -		goto stop_psp;
> -	}
> -
>   	mgmt_mb_irq = pci_irq_vector(pdev, ndev->mgmt_chan_idx);
>   	if (mgmt_mb_irq < 0) {
>   		ret = mgmt_mb_irq;
> @@ -399,13 +406,13 @@ static int aie2_hw_start(struct amdxdna_dev *xdna)
>   	}
>   
>   	xdna_mailbox_intr_reg = ndev->mgmt_i2x.mb_head_ptr_reg + 4;
> -	ndev->mgmt_chann = xdna_mailbox_create_channel(ndev->mbox,
> -						       &ndev->mgmt_x2i,
> -						       &ndev->mgmt_i2x,
> -						       xdna_mailbox_intr_reg,
> -						       mgmt_mb_irq);
> -	if (!ndev->mgmt_chann) {
> -		XDNA_ERR(xdna, "failed to create management mailbox channel");
> +	ret = xdna_mailbox_start_channel(ndev->mgmt_chann,
> +					 &ndev->mgmt_x2i,
> +					 &ndev->mgmt_i2x,
> +					 xdna_mailbox_intr_reg,
> +					 mgmt_mb_irq);
> +	if (ret) {
> +		XDNA_ERR(xdna, "failed to start management mailbox channel");
>   		ret = -EINVAL;
>   		goto stop_psp;
>   	}
> @@ -413,37 +420,41 @@ static int aie2_hw_start(struct amdxdna_dev *xdna)
>   	ret = aie2_mgmt_fw_init(ndev);
>   	if (ret) {
>   		XDNA_ERR(xdna, "initial mgmt firmware failed, ret %d", ret);
> -		goto destroy_mgmt_chann;
> +		goto stop_fw;
>   	}
>   
>   	ret = aie2_pm_init(ndev);
>   	if (ret) {
>   		XDNA_ERR(xdna, "failed to init pm, ret %d", ret);
> -		goto destroy_mgmt_chann;
> +		goto stop_fw;
>   	}
>   
>   	ret = aie2_mgmt_fw_query(ndev);
>   	if (ret) {
>   		XDNA_ERR(xdna, "failed to query fw, ret %d", ret);
> -		goto destroy_mgmt_chann;
> +		goto stop_fw;
>   	}
>   
>   	ret = aie2_error_async_events_alloc(ndev);
>   	if (ret) {
>   		XDNA_ERR(xdna, "Allocate async events failed, ret %d", ret);
> -		goto destroy_mgmt_chann;
> +		goto stop_fw;
>   	}
>   
>   	ndev->dev_status = AIE2_DEV_START;
>   
>   	return 0;
>   
> -destroy_mgmt_chann:
> -	aie2_destroy_mgmt_chann(ndev);
> +stop_fw:
> +	aie2_suspend_fw(ndev);
> +	xdna_mailbox_stop_channel(ndev->mgmt_chann);
>   stop_psp:
>   	aie2_psp_stop(ndev->psp_hdl);
>   fini_smu:
>   	aie2_smu_fini(ndev);
> +free_channel:
> +	xdna_mailbox_free_channel(ndev->mgmt_chann);
> +	ndev->mgmt_chann = NULL;
>   disable_dev:
>   	pci_disable_device(pdev);
>   
> diff --git a/drivers/accel/amdxdna/amdxdna_mailbox.c b/drivers/accel/amdxdna/amdxdna_mailbox.c
> index 235a94047530..46d844a73a94 100644
> --- a/drivers/accel/amdxdna/amdxdna_mailbox.c
> +++ b/drivers/accel/amdxdna/amdxdna_mailbox.c
> @@ -460,26 +460,49 @@ int xdna_mailbox_send_msg(struct mailbox_channel *mb_chann,
>   	return ret;
>   }
>   
> -struct mailbox_channel *
> -xdna_mailbox_create_channel(struct mailbox *mb,
> -			    const struct xdna_mailbox_chann_res *x2i,
> -			    const struct xdna_mailbox_chann_res *i2x,
> -			    u32 iohub_int_addr,
> -			    int mb_irq)
> +struct mailbox_channel *xdna_mailbox_alloc_channel(struct mailbox *mb)
>   {
>   	struct mailbox_channel *mb_chann;
> -	int ret;
> -
> -	if (!is_power_of_2(x2i->rb_size) || !is_power_of_2(i2x->rb_size)) {
> -		pr_err("Ring buf size must be power of 2");
> -		return NULL;
> -	}
>   
>   	mb_chann = kzalloc_obj(*mb_chann);
>   	if (!mb_chann)
>   		return NULL;
>   
> +	INIT_WORK(&mb_chann->rx_work, mailbox_rx_worker);
> +	mb_chann->work_q = create_singlethread_workqueue(MAILBOX_NAME);
> +	if (!mb_chann->work_q) {
> +		MB_ERR(mb_chann, "Create workqueue failed");
> +		goto free_chann;
> +	}
>   	mb_chann->mb = mb;
> +
> +	return mb_chann;
> +
> +free_chann:
> +	kfree(mb_chann);
> +	return NULL;
> +}
> +
> +void xdna_mailbox_free_channel(struct mailbox_channel *mb_chann)
> +{
> +	destroy_workqueue(mb_chann->work_q);
> +	kfree(mb_chann);
> +}
> +
> +int
> +xdna_mailbox_start_channel(struct mailbox_channel *mb_chann,
> +			   const struct xdna_mailbox_chann_res *x2i,
> +			   const struct xdna_mailbox_chann_res *i2x,
> +			   u32 iohub_int_addr,
> +			   int mb_irq)
> +{
> +	int ret;
> +
> +	if (!is_power_of_2(x2i->rb_size) || !is_power_of_2(i2x->rb_size)) {
> +		pr_err("Ring buf size must be power of 2");
> +		return -EINVAL;
> +	}
> +
>   	mb_chann->msix_irq = mb_irq;
>   	mb_chann->iohub_int_addr = iohub_int_addr;
>   	memcpy(&mb_chann->res[CHAN_RES_X2I], x2i, sizeof(*x2i));
> @@ -489,61 +512,37 @@ xdna_mailbox_create_channel(struct mailbox *mb,
>   	mb_chann->x2i_tail = mailbox_get_tailptr(mb_chann, CHAN_RES_X2I);
>   	mb_chann->i2x_head = mailbox_get_headptr(mb_chann, CHAN_RES_I2X);
>   
> -	INIT_WORK(&mb_chann->rx_work, mailbox_rx_worker);
> -	mb_chann->work_q = create_singlethread_workqueue(MAILBOX_NAME);
> -	if (!mb_chann->work_q) {
> -		MB_ERR(mb_chann, "Create workqueue failed");
> -		goto free_and_out;
> -	}
> -
>   	/* Everything look good. Time to enable irq handler */
>   	ret = request_irq(mb_irq, mailbox_irq_handler, 0, MAILBOX_NAME, mb_chann);
>   	if (ret) {
>   		MB_ERR(mb_chann, "Failed to request irq %d ret %d", mb_irq, ret);
> -		goto destroy_wq;
> +		return ret;
>   	}
>   
>   	mb_chann->bad_state = false;
>   	mailbox_reg_write(mb_chann, mb_chann->iohub_int_addr, 0);
>   
> -	MB_DBG(mb_chann, "Mailbox channel created (irq: %d)", mb_chann->msix_irq);
> -	return mb_chann;
> -
> -destroy_wq:
> -	destroy_workqueue(mb_chann->work_q);
> -free_and_out:
> -	kfree(mb_chann);
> -	return NULL;
> +	MB_DBG(mb_chann, "Mailbox channel started (irq: %d)", mb_chann->msix_irq);
> +	return 0;
>   }
>   
> -int xdna_mailbox_destroy_channel(struct mailbox_channel *mb_chann)
> +void xdna_mailbox_stop_channel(struct mailbox_channel *mb_chann)
>   {
>   	struct mailbox_msg *mb_msg;
>   	unsigned long msg_id;
>   
> -	MB_DBG(mb_chann, "IRQ disabled and RX work cancelled");
> +	/* Disable an irq and wait. This might sleep. */
>   	free_irq(mb_chann->msix_irq, mb_chann);
> -	destroy_workqueue(mb_chann->work_q);
> -	/* We can clean up and release resources */
>   
> +	/* Cancel RX work and wait for it to finish */
> +	drain_workqueue(mb_chann->work_q);
> +
> +	/* We can clean up and release resources */
>   	xa_for_each(&mb_chann->chan_xa, msg_id, mb_msg)
>   		mailbox_release_msg(mb_chann, mb_msg);
> -
>   	xa_destroy(&mb_chann->chan_xa);
>   
> -	MB_DBG(mb_chann, "Mailbox channel destroyed, irq: %d", mb_chann->msix_irq);
> -	kfree(mb_chann);
> -	return 0;
> -}
> -
> -void xdna_mailbox_stop_channel(struct mailbox_channel *mb_chann)
> -{
> -	/* Disable an irq and wait. This might sleep. */
> -	disable_irq(mb_chann->msix_irq);
> -
> -	/* Cancel RX work and wait for it to finish */
> -	cancel_work_sync(&mb_chann->rx_work);
> -	MB_DBG(mb_chann, "IRQ disabled and RX work cancelled");
> +	MB_DBG(mb_chann, "Mailbox channel stopped, irq: %d", mb_chann->msix_irq);
>   }
>   
>   struct mailbox *xdnam_mailbox_create(struct drm_device *ddev,
> diff --git a/drivers/accel/amdxdna/amdxdna_mailbox.h b/drivers/accel/amdxdna/amdxdna_mailbox.h
> index ea367f2fb738..8b1e00945da4 100644
> --- a/drivers/accel/amdxdna/amdxdna_mailbox.h
> +++ b/drivers/accel/amdxdna/amdxdna_mailbox.h
> @@ -74,9 +74,16 @@ struct mailbox *xdnam_mailbox_create(struct drm_device *ddev,
>   				     const struct xdna_mailbox_res *res);
>   
>   /*
> - * xdna_mailbox_create_channel() -- Create a mailbox channel instance
> + * xdna_mailbox_alloc_channel() -- alloc a mailbox channel
>    *
> - * @mailbox: the handle return from xdna_mailbox_create()
> + * @mb: mailbox handle
> + */
> +struct mailbox_channel *xdna_mailbox_alloc_channel(struct mailbox *mb);
> +
> +/*
> + * xdna_mailbox_start_channel() -- start a mailbox channel instance
> + *
> + * @mb_chann: the handle return from xdna_mailbox_alloc_channel()
>    * @x2i: host to firmware mailbox resources
>    * @i2x: firmware to host mailbox resources
>    * @xdna_mailbox_intr_reg: register addr of MSI-X interrupt
> @@ -84,28 +91,24 @@ struct mailbox *xdnam_mailbox_create(struct drm_device *ddev,
>    *
>    * Return: If success, return a handle of mailbox channel. Otherwise, return NULL.
>    */
> -struct mailbox_channel *
> -xdna_mailbox_create_channel(struct mailbox *mailbox,
> -			    const struct xdna_mailbox_chann_res *x2i,
> -			    const struct xdna_mailbox_chann_res *i2x,
> -			    u32 xdna_mailbox_intr_reg,
> -			    int mb_irq);
> +int
> +xdna_mailbox_start_channel(struct mailbox_channel *mb_chann,
> +			   const struct xdna_mailbox_chann_res *x2i,
> +			   const struct xdna_mailbox_chann_res *i2x,
> +			   u32 xdna_mailbox_intr_reg,
> +			   int mb_irq);
>   
>   /*
> - * xdna_mailbox_destroy_channel() -- destroy mailbox channel
> + * xdna_mailbox_free_channel() -- free mailbox channel
>    *
>    * @mailbox_chann: the handle return from xdna_mailbox_create_channel()
> - *
> - * Return: if success, return 0. otherwise return error code
>    */
> -int xdna_mailbox_destroy_channel(struct mailbox_channel *mailbox_chann);
> +void xdna_mailbox_free_channel(struct mailbox_channel *mailbox_chann);
>   
>   /*
>    * xdna_mailbox_stop_channel() -- stop mailbox channel
>    *
>    * @mailbox_chann: the handle return from xdna_mailbox_create_channel()
> - *
> - * Return: if success, return 0. otherwise return error code
>    */
>   void xdna_mailbox_stop_channel(struct mailbox_channel *mailbox_chann);
>