[v3,22/25] bus: mhi: ep: Add support for processing transfer ring

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.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/bus/mhi/ep/main.c | 49 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

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@linaro.org>

> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.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;

On Tue, Feb 15, 2022 at 04:40:18PM -0600, Alex Elder wrote:
> 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@linaro.org>
> 
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.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?
> 

The allocation was moved into the loop so that the TRE length buffer could be
allocated but I somehow decided to allocate the Max length buffer. So this could
be moved outside of the loop.

Thanks,
Mani

> 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;
>