Messages in this thread | | | Date | Tue, 15 Feb 2022 16:40:29 -0600 | Subject | Re: [PATCH v3 23/25] bus: mhi: ep: Add support for queueing SKBs to the host | From | Alex Elder <> |
| |
On 2/12/22 12:21 PM, Manivannan Sadhasivam wrote: > Add support for queueing SKBs to the host over the transfer ring of the > relevant channel. The mhi_ep_queue_skb() API will be used by the client > networking drivers to queue the SKBs to the host over MHI bus. > > The host will add ring elements to the transfer ring periodically for > the device and the device will write SKBs to the ring elements. If a > single SKB doesn't fit in a ring element (TRE), it will be placed in > multiple ring elements and the overflow event will be sent for all ring > elements except the last one. For the last ring element, the EOT event > will be sent indicating the packet boundary. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
I'm a little confused by this, so maybe you can provide a better explanation somehow.
-Alex
> --- > drivers/bus/mhi/ep/main.c | 102 ++++++++++++++++++++++++++++++++++++++ > include/linux/mhi_ep.h | 13 +++++ > 2 files changed, 115 insertions(+) > > diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c > index baf383a4857b..e4186b012257 100644 > --- a/drivers/bus/mhi/ep/main.c > +++ b/drivers/bus/mhi/ep/main.c > @@ -488,6 +488,108 @@ int mhi_ep_process_tre_ring(struct mhi_ep_ring *ring, struct mhi_ep_ring_element > return 0; > } > > +int mhi_ep_queue_skb(struct mhi_ep_device *mhi_dev, enum dma_data_direction dir, > + struct sk_buff *skb, size_t len, enum mhi_flags mflags)
Why are both skb and len supplied? Will an skb be supplied without wanting to send all of it? Must len be less than skb->len? I'm a little confused about the interface.
Also, the data direction is *out*, right? You'll never be queueing a "receive" SKB?
> +{ > + struct mhi_ep_chan *mhi_chan = (dir == DMA_FROM_DEVICE) ? mhi_dev->dl_chan : > + mhi_dev->ul_chan; > + struct mhi_ep_cntrl *mhi_cntrl = mhi_dev->mhi_cntrl; > + struct device *dev = &mhi_chan->mhi_dev->dev; > + struct mhi_ep_ring_element *el; > + struct mhi_ep_ring *ring; > + size_t bytes_to_write; > + enum mhi_ev_ccs code; > + void *read_from_loc; > + u32 buf_remaining; > + u64 write_to_loc; > + u32 tre_len; > + int ret = 0; > + > + if (dir == DMA_TO_DEVICE) > + return -EINVAL;
Can't you just preclude this from happening, or know it won't happen by inspection?
> + > + buf_remaining = len; > + ring = &mhi_cntrl->mhi_chan[mhi_chan->chan].ring; > + > + mutex_lock(&mhi_chan->lock); > + > + do { > + /* Don't process the transfer ring if the channel is not in RUNNING state */ > + if (mhi_chan->state != MHI_CH_STATE_RUNNING) { > + dev_err(dev, "Channel not available\n"); > + ret = -ENODEV; > + goto err_exit; > + } > +
It would be nice if the caller could know whether there was enough room *before* you start transferring things. It's probably a lot of work to get to that point though.
> + if (mhi_ep_queue_is_empty(mhi_dev, dir)) { > + dev_err(dev, "TRE not available!\n"); > + ret = -EINVAL; > + goto err_exit; > + } > + > + el = &ring->ring_cache[ring->rd_offset]; > + tre_len = MHI_EP_TRE_GET_LEN(el); > + if (skb->len > tre_len) { > + dev_err(dev, "Buffer size (%d) is too large for TRE (%d)!\n", > + skb->len, tre_len);
This means the receive buffer must be big enough to hold any incoming SKB. This is *without* checking for the CHAIN flag in the TRE, so what you describe in the patch description seems not to be true. I.e., multiple TREs in a TRD will *not* be consumed if the SKB data requires more than what's left in the current TRE.
But you have some other code below, so it's likely I'm just misunderstanding this.
> + ret = -ENOMEM; > + goto err_exit; > + } > + > + bytes_to_write = min(buf_remaining, tre_len); > + read_from_loc = skb->data; > + write_to_loc = MHI_EP_TRE_GET_PTR(el); > + > + ret = mhi_cntrl->write_to_host(mhi_cntrl, read_from_loc, write_to_loc, > + bytes_to_write); > + if (ret < 0) > + goto err_exit; > + > + buf_remaining -= bytes_to_write; > + /* > + * For all TREs queued by the host for DL channel, only the EOT flag will be set. > + * If the packet doesn't fit into a single TRE, send the OVERFLOW event to > + * the host so that the host can adjust the packet boundary to next TREs. Else send > + * the EOT event to the host indicating the packet boundary. > + */ > + if (buf_remaining) > + code = MHI_EV_CC_OVERFLOW; > + else > + code = MHI_EV_CC_EOT; > + > + ret = mhi_ep_send_completion_event(mhi_cntrl, ring, bytes_to_write, code); > + if (ret) { > + dev_err(dev, "Error sending completion event\n"); > + goto err_exit; > + } > + > + mhi_ep_ring_inc_index(ring); > + } while (buf_remaining); > + > + /* > + * During high network traffic, sometimes the DL doorbell interrupt from the host is missed > + * by the endpoint. So manually check for the write pointer update here so that we don't run > + * out of buffer due to missing interrupts. > + */ > + if (ring->rd_offset + 1 == ring->wr_offset) { > + ret = mhi_ep_update_wr_offset(ring); > + if (ret) { > + dev_err(dev, "Error updating write pointer\n"); > + goto err_exit; > + } > + } > + > + mutex_unlock(&mhi_chan->lock); > + > + return 0; > + > +err_exit: > + mutex_unlock(&mhi_chan->lock); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(mhi_ep_queue_skb); > + > static int mhi_ep_cache_host_cfg(struct mhi_ep_cntrl *mhi_cntrl) > { > struct device *dev = &mhi_cntrl->mhi_dev->dev; > diff --git a/include/linux/mhi_ep.h b/include/linux/mhi_ep.h > index aaf4b6942037..75cfbf0c6fb0 100644 > --- a/include/linux/mhi_ep.h > +++ b/include/linux/mhi_ep.h > @@ -277,4 +277,17 @@ void mhi_ep_power_down(struct mhi_ep_cntrl *mhi_cntrl); > */ > bool mhi_ep_queue_is_empty(struct mhi_ep_device *mhi_dev, enum dma_data_direction dir); > > +/** > + * mhi_ep_queue_skb - Send SKBs to host over MHI Endpoint > + * @mhi_dev: Device associated with the channels > + * @dir: DMA direction for the channel > + * @skb: Buffer for holding SKBs > + * @len: Buffer length > + * @mflags: MHI Endpoint transfer flags used for the transfer > + * > + * Return: 0 if the SKBs has been sent successfully, a negative error code otherwise. > + */ > +int mhi_ep_queue_skb(struct mhi_ep_device *mhi_dev, enum dma_data_direction dir, > + struct sk_buff *skb, size_t len, enum mhi_flags mflags); > + > #endif
| |