Re: [PATCH v2] net: qrtr: mhi: synchronize qrtr and mhi preparation

[Johan Hovold]

On Wed, Jun 04, 2025 at 02:05:42PM -0700, Chris Lew wrote:
> The call to qrtr_endpoint_register() was moved before
> mhi_prepare_for_transfer_autoqueue() to prevent a case where a dl
> callback can occur before the qrtr endpoint is registered.
> 
> Now the reverse can happen where qrtr will try to send a packet
> before the channels are prepared. The correct sequence needs to be
> prepare the mhi channel, register the qrtr endpoint, queue buffers for
> receiving dl transfers.
> 
> Since qrtr will not use mhi_prepare_for_transfer_autoqueue(), qrtr must
> do the buffer management and requeue the buffers in the dl_callback.
> Sizing of the buffers will be inherited from the mhi controller
> settings.
> 
> Fixes: 68a838b84eff ("net: qrtr: start MHI channel after endpoit creation")
> Reported-by: Johan Hovold <johan@xxxxxxxxxx>
> Closes: https://lore.kernel.org/linux-arm-msm/ZyTtVdkCCES0lkl4@xxxxxxxxxxxxxxxxxxxx/
> Signed-off-by: Chris Lew <chris.lew@xxxxxxxxxxxxxxxx>

Thanks for the update. I believe this one should have a stable tag as
well as it fixes a critical boot failure on Qualcomm platforms that we
hit frequently with the in-kernel pd-mapper.

And it indeed fixes the crash:

Tested-by: Johan Hovold <johan+linaro@xxxxxxxxxx>

>  /* From MHI to QRTR */
> @@ -24,13 +26,22 @@ static void qcom_mhi_qrtr_dl_callback(struct mhi_device *mhi_dev,
>  	struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
>  	int rc;
>  
> -	if (!qdev || mhi_res->transaction_status)
> +	if (!qdev)
> +		return;
> +
> +	if (mhi_res->transaction_status == -ENOTCONN) {
> +		devm_kfree(qdev->dev, mhi_res->buf_addr);

Why do you need to free this buffer here?

AFAICS, all buffers are allocated at probe() and freed at (after)
remove().

> +		return;
> +	} else if (mhi_res->transaction_status) {
>  		return;
> +	}
>  
>  	rc = qrtr_endpoint_post(&qdev->ep, mhi_res->buf_addr,
>  				mhi_res->bytes_xferd);
>  	if (rc == -EINVAL)
>  		dev_err(qdev->dev, "invalid ipcrouter packet\n");
> +
> +	rc = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, mhi_res->buf_addr, qdev->dl_buf_len, MHI_EOT);

Please try to stay within 80 columns except when not doing so
significantly improves readability.

Also you don't do anything with rc here. Should you log an error at
least?

>  }
 
> +static int qrtr_mhi_queue_rx(struct qrtr_mhi_dev *qdev)
> +{
> +	struct mhi_device *mhi_dev = qdev->mhi_dev;
> +	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> +	int rc = 0;
> +	int nr_el;
> +
> +	qdev->dl_buf_len = mhi_cntrl->buffer_len;
> +	nr_el = mhi_get_free_desc_count(mhi_dev, DMA_FROM_DEVICE);
> +	while (nr_el--) {
> +		void *buf;
> +
> +		buf = devm_kzalloc(qdev->dev, qdev->dl_buf_len, GFP_KERNEL);
> +		if (!buf) {
> +			rc = -ENOMEM;
> +			break;
> +		}
> +		rc = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, buf, qdev->dl_buf_len, MHI_EOT);

80 cols here too.

> +		if (rc)
> +			break;
> +	}
> +	return rc;
> +}
> +
>  static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev,
>  			       const struct mhi_device_id *id)
>  {
> @@ -87,17 +122,24 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev,
>  	qdev->ep.xmit = qcom_mhi_qrtr_send;
>  
>  	dev_set_drvdata(&mhi_dev->dev, qdev);
> -	rc = qrtr_endpoint_register(&qdev->ep, QRTR_EP_NID_AUTO);
> +
> +	/* start channels */
> +	rc = mhi_prepare_for_transfer(mhi_dev);
>  	if (rc)
>  		return rc;
>  
> -	/* start channels */
> -	rc = mhi_prepare_for_transfer_autoqueue(mhi_dev);
> +	rc = qrtr_endpoint_register(&qdev->ep, QRTR_EP_NID_AUTO);
>  	if (rc) {
> -		qrtr_endpoint_unregister(&qdev->ep);
> +		mhi_unprepare_from_transfer(mhi_dev);
>  		return rc;
>  	}
>  
> +	rc = qrtr_mhi_queue_rx(qdev);
> +	if (rc) {
> +		qrtr_endpoint_unregister(&qdev->ep);
> +		mhi_unprepare_from_transfer(mhi_dev);

Jakub already pointed out the missing return here. Perhaps you should
consider adding error labels for the unwinding.

> +	}
> +
>  	dev_dbg(qdev->dev, "Qualcomm MHI QRTR driver probed\n");
>  
>  	return 0;

Johan