| Message ID | 1481142941-15616-2-git-send-email-loic.pallardy@st.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 12/07/2016 02:35 PM, Loic Pallardy wrote: > Rpmsg buffer size is currently fixed to 512 bytes. > This patch introduces a new capability in struct virtproc_info > to tune shared buffer size between host and coprocessor > according to the needs. This patch breaks rpmsg functionality w.r.t bisectability. The new buf_size variable is not initialized (is 0) in this patch while you replace the RPMSG_BUF_SIZE with vrp->buf_size in most call sites. You introduce the initialization in Patch 4, that should be moved up. The ops to override can stay in Patch 4. regards Suman > > Signed-off-by: Loic Pallardy <loic.pallardy@st.com> > --- > drivers/rpmsg/virtio_rpmsg_bus.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c > index 3090b0d..1f6dfc6 100644 > --- a/drivers/rpmsg/virtio_rpmsg_bus.c > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c > @@ -45,6 +45,7 @@ > * @rbufs: kernel address of rx buffers > * @sbufs: kernel address of tx buffers > * @num_bufs: total number of buffers for rx and tx > + * @buf_size: size of one rx or tx buffer > * @last_sbuf: index of last tx buffer used > * @bufs_dma: dma base addr of the buffers > * @tx_lock: protects svq, sbufs and sleepers, to allow concurrent senders. > @@ -65,6 +66,7 @@ struct virtproc_info { > struct virtqueue *rvq, *svq; > void *rbufs, *sbufs; > unsigned int num_bufs; > + unsigned int buf_size; > int last_sbuf; > dma_addr_t bufs_dma; > struct mutex tx_lock; > @@ -158,7 +160,7 @@ struct virtio_rpmsg_channel { > * processor. > */ > #define MAX_RPMSG_NUM_BUFS (512) > -#define RPMSG_BUF_SIZE (512) > +#define MAX_RPMSG_BUF_SIZE (512) > > /* > * Local addresses are dynamically allocated on-demand. > @@ -429,7 +431,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp) > * (half of our buffers are used for sending messages) > */ > if (vrp->last_sbuf < vrp->num_bufs / 2) > - ret = vrp->sbufs + RPMSG_BUF_SIZE * vrp->last_sbuf++; > + ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++; > /* or recycle a used one */ > else > ret = virtqueue_get_buf(vrp->svq, &len); > @@ -555,7 +557,7 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev, > * messaging), or to improve the buffer allocator, to support > * variable-length buffer sizes. > */ > - if (len > RPMSG_BUF_SIZE - sizeof(struct rpmsg_hdr)) { > + if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) { > dev_err(dev, "message is too big (%d)\n", len); > return -EMSGSIZE; > } > @@ -696,8 +698,8 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev, > * We currently use fixed-sized buffers, so trivially sanitize > * the reported payload length. > */ > - if (len > RPMSG_BUF_SIZE || > - msg->len > (len - sizeof(struct rpmsg_hdr))) { > + if (len > vrp->buf_size || > + msg->len > (len - sizeof(struct rpmsg_hdr))) { > dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg->len); > return -EINVAL; > } > @@ -729,7 +731,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev, > dev_warn(dev, "msg received with no recipient\n"); > > /* publish the real size of the buffer */ > - sg_init_one(&sg, msg, RPMSG_BUF_SIZE); > + sg_init_one(&sg, msg, vrp->buf_size); > > /* add the buffer back to the remote processor's virtqueue */ > err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL); > @@ -886,7 +888,7 @@ static int rpmsg_probe(struct virtio_device *vdev) > else > vrp->num_bufs = MAX_RPMSG_NUM_BUFS; > > - total_buf_space = vrp->num_bufs * RPMSG_BUF_SIZE; > + total_buf_space = vrp->num_bufs * vrp->buf_size; > > /* allocate coherent memory for the buffers */ > bufs_va = dma_alloc_coherent(vdev->dev.parent->parent, > @@ -909,9 +911,9 @@ static int rpmsg_probe(struct virtio_device *vdev) > /* set up the receive buffers */ > for (i = 0; i < vrp->num_bufs / 2; i++) { > struct scatterlist sg; > - void *cpu_addr = vrp->rbufs + i * RPMSG_BUF_SIZE; > + void *cpu_addr = vrp->rbufs + i * vrp->buf_size; > > - sg_init_one(&sg, cpu_addr, RPMSG_BUF_SIZE); > + sg_init_one(&sg, cpu_addr, vrp->buf_size); > > err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr, > GFP_KERNEL); > @@ -976,7 +978,7 @@ static int rpmsg_remove_device(struct device *dev, void *data) > static void rpmsg_remove(struct virtio_device *vdev) > { > struct virtproc_info *vrp = vdev->priv; > - size_t total_buf_space = vrp->num_bufs * RPMSG_BUF_SIZE; > + size_t total_buf_space = vrp->num_bufs * vrp->buf_size; > int ret; > > vdev->config->reset(vdev); > -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Suman Anna [mailto:s-anna@ti.com] > Sent: Saturday, January 14, 2017 2:40 AM > To: Loic PALLARDY <loic.pallardy@st.com>; bjorn.andersson@linaro.org; > ohad@wizery.com; lee.jones@linaro.org; Patrice CHOTARD > <patrice.chotard@st.com> > Cc: linux-remoteproc@vger.kernel.org; kernel@stlinux.com > Subject: Re: [PATCH v1 1/6] rpmsg: virtio_rpmsg: set rpmsg_buf_size > customizable > > On 12/07/2016 02:35 PM, Loic Pallardy wrote: > > Rpmsg buffer size is currently fixed to 512 bytes. > > This patch introduces a new capability in struct virtproc_info to tune > > shared buffer size between host and coprocessor according to the > > needs. > > This patch breaks rpmsg functionality w.r.t bisectability. The new buf_size > variable is not initialized (is 0) in this patch while you replace the > RPMSG_BUF_SIZE with vrp->buf_size in most call sites. You introduce the > initialization in Patch 4, that should be moved up. The ops to override can > stay in Patch 4. Exact vrp->buf_size should be forced to MAX_RPMSG_BUF_SIZE (512) to preserve virtio_rpmsg behavior. Thanks, Loic > > regards > Suman > > > > > Signed-off-by: Loic Pallardy <loic.pallardy@st.com> > > --- > > drivers/rpmsg/virtio_rpmsg_bus.c | 22 ++++++++++++---------- > > 1 file changed, 12 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c > > b/drivers/rpmsg/virtio_rpmsg_bus.c > > index 3090b0d..1f6dfc6 100644 > > --- a/drivers/rpmsg/virtio_rpmsg_bus.c > > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c > > @@ -45,6 +45,7 @@ > > * @rbufs: kernel address of rx buffers > > * @sbufs: kernel address of tx buffers > > * @num_bufs: total number of buffers for rx and tx > > + * @buf_size: size of one rx or tx buffer > > * @last_sbuf: index of last tx buffer used > > * @bufs_dma: dma base addr of the buffers > > * @tx_lock: protects svq, sbufs and sleepers, to allow concurrent > senders. > > @@ -65,6 +66,7 @@ struct virtproc_info { > > struct virtqueue *rvq, *svq; > > void *rbufs, *sbufs; > > unsigned int num_bufs; > > + unsigned int buf_size; > > int last_sbuf; > > dma_addr_t bufs_dma; > > struct mutex tx_lock; > > @@ -158,7 +160,7 @@ struct virtio_rpmsg_channel { > > * processor. > > */ > > #define MAX_RPMSG_NUM_BUFS (512) > > -#define RPMSG_BUF_SIZE (512) > > +#define MAX_RPMSG_BUF_SIZE (512) > > > > /* > > * Local addresses are dynamically allocated on-demand. > > @@ -429,7 +431,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp) > > * (half of our buffers are used for sending messages) > > */ > > if (vrp->last_sbuf < vrp->num_bufs / 2) > > - ret = vrp->sbufs + RPMSG_BUF_SIZE * vrp->last_sbuf++; > > + ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++; > > /* or recycle a used one */ > > else > > ret = virtqueue_get_buf(vrp->svq, &len); @@ -555,7 +557,7 > @@ static > > int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev, > > * messaging), or to improve the buffer allocator, to support > > * variable-length buffer sizes. > > */ > > - if (len > RPMSG_BUF_SIZE - sizeof(struct rpmsg_hdr)) { > > + if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) { > > dev_err(dev, "message is too big (%d)\n", len); > > return -EMSGSIZE; > > } > > @@ -696,8 +698,8 @@ static int rpmsg_recv_single(struct virtproc_info > *vrp, struct device *dev, > > * We currently use fixed-sized buffers, so trivially sanitize > > * the reported payload length. > > */ > > - if (len > RPMSG_BUF_SIZE || > > - msg->len > (len - sizeof(struct rpmsg_hdr))) { > > + if (len > vrp->buf_size || > > + msg->len > (len - sizeof(struct rpmsg_hdr))) { > > dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg- > >len); > > return -EINVAL; > > } > > @@ -729,7 +731,7 @@ static int rpmsg_recv_single(struct virtproc_info > *vrp, struct device *dev, > > dev_warn(dev, "msg received with no recipient\n"); > > > > /* publish the real size of the buffer */ > > - sg_init_one(&sg, msg, RPMSG_BUF_SIZE); > > + sg_init_one(&sg, msg, vrp->buf_size); > > > > /* add the buffer back to the remote processor's virtqueue */ > > err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL); @@ > > -886,7 +888,7 @@ static int rpmsg_probe(struct virtio_device *vdev) > > else > > vrp->num_bufs = MAX_RPMSG_NUM_BUFS; > > > > - total_buf_space = vrp->num_bufs * RPMSG_BUF_SIZE; > > + total_buf_space = vrp->num_bufs * vrp->buf_size; > > > > /* allocate coherent memory for the buffers */ > > bufs_va = dma_alloc_coherent(vdev->dev.parent->parent, > > @@ -909,9 +911,9 @@ static int rpmsg_probe(struct virtio_device *vdev) > > /* set up the receive buffers */ > > for (i = 0; i < vrp->num_bufs / 2; i++) { > > struct scatterlist sg; > > - void *cpu_addr = vrp->rbufs + i * RPMSG_BUF_SIZE; > > + void *cpu_addr = vrp->rbufs + i * vrp->buf_size; > > > > - sg_init_one(&sg, cpu_addr, RPMSG_BUF_SIZE); > > + sg_init_one(&sg, cpu_addr, vrp->buf_size); > > > > err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr, > > GFP_KERNEL); > > @@ -976,7 +978,7 @@ static int rpmsg_remove_device(struct device *dev, > > void *data) static void rpmsg_remove(struct virtio_device *vdev) { > > struct virtproc_info *vrp = vdev->priv; > > - size_t total_buf_space = vrp->num_bufs * RPMSG_BUF_SIZE; > > + size_t total_buf_space = vrp->num_bufs * vrp->buf_size; > > int ret; > > > > vdev->config->reset(vdev); > > -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c index 3090b0d..1f6dfc6 100644 --- a/drivers/rpmsg/virtio_rpmsg_bus.c +++ b/drivers/rpmsg/virtio_rpmsg_bus.c @@ -45,6 +45,7 @@ * @rbufs: kernel address of rx buffers * @sbufs: kernel address of tx buffers * @num_bufs: total number of buffers for rx and tx + * @buf_size: size of one rx or tx buffer * @last_sbuf: index of last tx buffer used * @bufs_dma: dma base addr of the buffers * @tx_lock: protects svq, sbufs and sleepers, to allow concurrent senders. @@ -65,6 +66,7 @@ struct virtproc_info { struct virtqueue *rvq, *svq; void *rbufs, *sbufs; unsigned int num_bufs; + unsigned int buf_size; int last_sbuf; dma_addr_t bufs_dma; struct mutex tx_lock; @@ -158,7 +160,7 @@ struct virtio_rpmsg_channel { * processor. */ #define MAX_RPMSG_NUM_BUFS (512) -#define RPMSG_BUF_SIZE (512) +#define MAX_RPMSG_BUF_SIZE (512) /* * Local addresses are dynamically allocated on-demand. @@ -429,7 +431,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp) * (half of our buffers are used for sending messages) */ if (vrp->last_sbuf < vrp->num_bufs / 2) - ret = vrp->sbufs + RPMSG_BUF_SIZE * vrp->last_sbuf++; + ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++; /* or recycle a used one */ else ret = virtqueue_get_buf(vrp->svq, &len); @@ -555,7 +557,7 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev, * messaging), or to improve the buffer allocator, to support * variable-length buffer sizes. */ - if (len > RPMSG_BUF_SIZE - sizeof(struct rpmsg_hdr)) { + if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) { dev_err(dev, "message is too big (%d)\n", len); return -EMSGSIZE; } @@ -696,8 +698,8 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev, * We currently use fixed-sized buffers, so trivially sanitize * the reported payload length. */ - if (len > RPMSG_BUF_SIZE || - msg->len > (len - sizeof(struct rpmsg_hdr))) { + if (len > vrp->buf_size || + msg->len > (len - sizeof(struct rpmsg_hdr))) { dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg->len); return -EINVAL; } @@ -729,7 +731,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev, dev_warn(dev, "msg received with no recipient\n"); /* publish the real size of the buffer */ - sg_init_one(&sg, msg, RPMSG_BUF_SIZE); + sg_init_one(&sg, msg, vrp->buf_size); /* add the buffer back to the remote processor's virtqueue */ err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL); @@ -886,7 +888,7 @@ static int rpmsg_probe(struct virtio_device *vdev) else vrp->num_bufs = MAX_RPMSG_NUM_BUFS; - total_buf_space = vrp->num_bufs * RPMSG_BUF_SIZE; + total_buf_space = vrp->num_bufs * vrp->buf_size; /* allocate coherent memory for the buffers */ bufs_va = dma_alloc_coherent(vdev->dev.parent->parent, @@ -909,9 +911,9 @@ static int rpmsg_probe(struct virtio_device *vdev) /* set up the receive buffers */ for (i = 0; i < vrp->num_bufs / 2; i++) { struct scatterlist sg; - void *cpu_addr = vrp->rbufs + i * RPMSG_BUF_SIZE; + void *cpu_addr = vrp->rbufs + i * vrp->buf_size; - sg_init_one(&sg, cpu_addr, RPMSG_BUF_SIZE); + sg_init_one(&sg, cpu_addr, vrp->buf_size); err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr, GFP_KERNEL); @@ -976,7 +978,7 @@ static int rpmsg_remove_device(struct device *dev, void *data) static void rpmsg_remove(struct virtio_device *vdev) { struct virtproc_info *vrp = vdev->priv; - size_t total_buf_space = vrp->num_bufs * RPMSG_BUF_SIZE; + size_t total_buf_space = vrp->num_bufs * vrp->buf_size; int ret; vdev->config->reset(vdev);
Rpmsg buffer size is currently fixed to 512 bytes. This patch introduces a new capability in struct virtproc_info to tune shared buffer size between host and coprocessor according to the needs. Signed-off-by: Loic Pallardy <loic.pallardy@st.com> --- drivers/rpmsg/virtio_rpmsg_bus.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)