linux-kernel - Re: [PATCH v3 22/25] bus: mhi: ep: Add support for processing transfer ring

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f91de912-977b-4e78-2e8c-12f4c09134bd@linaro.org>
Date:   Tue, 15 Feb 2022 16:40:18 -0600
From:   Alex Elder <elder@...aro.org>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        mhi@...ts.linux.dev
Cc:     quic_hemantk@...cinc.com, quic_bbhatt@...cinc.com,
        quic_jhugo@...cinc.com, vinod.koul@...aro.org,
        bjorn.andersson@...aro.org, dmitry.baryshkov@...aro.org,
        quic_vbadigan@...cinc.com, quic_cang@...cinc.com,
        quic_skananth@...cinc.com, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 22/25] bus: mhi: ep: Add support for processing
 transfer ring

On 2/12/22 12:21 PM, Manivannan Sadhasivam wrote:
> Add support for processing the transfer ring from host. For the transfer
> ring associated with DL channel, the xfer callback will simply invoked.
> For the case of UL channel, the ring elements will be read in a buffer
> till the write pointer and later passed to the client driver using the
> xfer callback.
> 
> The client drivers should provide the callbacks for both UL and DL
> channels during registration.

I think you already checked and guaranteed that.

I have a question and suggestion below.  But it could
be considered an optimization that could be implemented
in the future, so:

Reviewed-by: Alex Elder <elder@...aro.org>

> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> ---
>   drivers/bus/mhi/ep/main.c | 49 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
> index b937c6cda9ba..baf383a4857b 100644
> --- a/drivers/bus/mhi/ep/main.c
> +++ b/drivers/bus/mhi/ep/main.c
> @@ -439,6 +439,55 @@ static int mhi_ep_read_channel(struct mhi_ep_cntrl *mhi_cntrl,
>   	return 0;
>   }
>   
> +int mhi_ep_process_tre_ring(struct mhi_ep_ring *ring, struct mhi_ep_ring_element *el)
> +{
> +	struct mhi_ep_cntrl *mhi_cntrl = ring->mhi_cntrl;
> +	struct mhi_result result = {};
> +	u32 len = MHI_EP_DEFAULT_MTU;
> +	struct mhi_ep_chan *mhi_chan;
> +	int ret;
> +
> +	mhi_chan = &mhi_cntrl->mhi_chan[ring->ch_id];
> +
> +	/*
> +	 * Bail out if transfer callback is not registered for the channel.
> +	 * This is most likely due to the client driver not loaded at this point.
> +	 */
> +	if (!mhi_chan->xfer_cb) {
> +		dev_err(&mhi_chan->mhi_dev->dev, "Client driver not available\n");
> +		return -ENODEV;
> +	}
> +
> +	if (ring->ch_id % 2) {
> +		/* DL channel */
> +		result.dir = mhi_chan->dir;
> +		mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result);
> +	} else {
> +		/* UL channel */
> +		do {
> +			result.buf_addr = kzalloc(len, GFP_KERNEL);

So you allocate an 8KB buffer into which you copy
received data, then pass that to the ->xfer_cb()
function.  Then you free that buffer.  Repeatedly.

Two questions about this:
- This suggests that after copying the data in, the
   ->xfer_cb() function will copy it again, is that
   correct?
- If that is correct, why not just reuse the same 8KB
   buffer, allocated once outside the loop?

It might also be nice to consider whether you could
allocate the buffer here and have the ->xfer_cb()
function be responsible for freeing it (and ideally,
pass it along rather than copying it again).

> +			if (!result.buf_addr)
> +				return -ENOMEM;
> +
> +			ret = mhi_ep_read_channel(mhi_cntrl, ring, &result, len);
> +			if (ret < 0) {
> +				dev_err(&mhi_chan->mhi_dev->dev, "Failed to read channel\n");
> +				kfree(result.buf_addr);
> +				return ret;
> +			}
> +
> +			result.dir = mhi_chan->dir;
> +			mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result);
> +			kfree(result.buf_addr);
> +			result.bytes_xferd = 0;
> +
> +			/* Read until the ring becomes empty */
> +		} while (!mhi_ep_queue_is_empty(mhi_chan->mhi_dev, DMA_TO_DEVICE));
> +	}
> +
> +	return 0;
> +}
> +
>   static int mhi_ep_cache_host_cfg(struct mhi_ep_cntrl *mhi_cntrl)
>   {
>   	struct device *dev = &mhi_cntrl->mhi_dev->dev;