| Message ID | 1515590217-24256-1-git-send-email-anup@brainfault.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 10, 2018 at 6:46 PM, Anup Patel <anup@brainfault.org> wrote: > When virtio-rpmsg device is provided via virtio-mmio transport, the > dma_alloc_coherent() (called by rpmsg_probe()) fails on ARM/ARM64 > systems because "vdev->dev.parent->parent" device is used as parameter > to dma_alloc_coherent(). > > The "vdev->dev.parent->parent" device represents underlying remoteproc > platform device when virtio-rpmsg device is provided via virtio-remoteproc > transport. When virtio-rpmsg device is provided via virtio-mmio transport, > the "vdev->dev.parent->parent" device represents the parent device of > virtio-mmio platform device and dma_alloc_coherent() fails for this device > because generally there is no corresponding platform device and dma_ops > are not setup for "vdev->dev.parent->parent". > > This patch fixes dma_alloc_coherent() usage in rpmsg_probe() by trying > dma_alloc_coherent() with "vdev->dev.parent" device when it fails with > "vdev->dev.parent->parent" device. > > Fixes: b5ab5e24e960 ("remoteproc: maintain a generic child device for > each rproc") > > Signed-off-by: Anup Patel <anup@brainfault.org> > Cc: stable@vger.kernel.org > --- > drivers/rpmsg/virtio_rpmsg_bus.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c > index 82b8300..7f8710a 100644 > --- a/drivers/rpmsg/virtio_rpmsg_bus.c > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c > @@ -64,6 +64,7 @@ > struct virtproc_info { > struct virtio_device *vdev; > struct virtqueue *rvq, *svq; > + struct device *bufs_dev; > void *rbufs, *sbufs; > unsigned int num_bufs; > unsigned int buf_size; > @@ -924,9 +925,16 @@ static int rpmsg_probe(struct virtio_device *vdev) > total_buf_space, &vrp->bufs_dma, > GFP_KERNEL); > if (!bufs_va) { > - err = -ENOMEM; > - goto vqs_del; > - } > + bufs_va = dma_alloc_coherent(vdev->dev.parent, > + total_buf_space, &vrp->bufs_dma, > + GFP_KERNEL); > + if (!bufs_va) { > + err = -ENOMEM; > + goto vqs_del; > + } else > + vrp->bufs_dev = vdev->dev.parent; > + } else > + vrp->bufs_dev = vdev->dev.parent->parent; > > dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n", > bufs_va, &vrp->bufs_dma); > @@ -988,7 +996,7 @@ static int rpmsg_probe(struct virtio_device *vdev) > return 0; > > free_coherent: > - dma_free_coherent(vdev->dev.parent->parent, total_buf_space, > + dma_free_coherent(vrp->bufs_dev, total_buf_space, > bufs_va, vrp->bufs_dma); > vqs_del: > vdev->config->del_vqs(vrp->vdev); > @@ -1023,7 +1031,7 @@ static void rpmsg_remove(struct virtio_device *vdev) > > vdev->config->del_vqs(vrp->vdev); > > - dma_free_coherent(vdev->dev.parent->parent, total_buf_space, > + dma_free_coherent(vrp->bufs_dev, total_buf_space, > vrp->rbufs, vrp->bufs_dma); > > kfree(vrp); > -- > 2.7.4 > Hi All, Any comments on this?? Regards, Anup -- 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
Hi Bjorn, Can you please have a look at this patch? Regards, Anup -- 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
On Wed, Jan 10, 2018 at 6:46 PM, Anup Patel <anup@brainfault.org> wrote: > When virtio-rpmsg device is provided via virtio-mmio transport, the > dma_alloc_coherent() (called by rpmsg_probe()) fails on ARM/ARM64 > systems because "vdev->dev.parent->parent" device is used as parameter > to dma_alloc_coherent(). > > The "vdev->dev.parent->parent" device represents underlying remoteproc > platform device when virtio-rpmsg device is provided via virtio-remoteproc > transport. When virtio-rpmsg device is provided via virtio-mmio transport, > the "vdev->dev.parent->parent" device represents the parent device of > virtio-mmio platform device and dma_alloc_coherent() fails for this device > because generally there is no corresponding platform device and dma_ops > are not setup for "vdev->dev.parent->parent". > > This patch fixes dma_alloc_coherent() usage in rpmsg_probe() by trying > dma_alloc_coherent() with "vdev->dev.parent" device when it fails with > "vdev->dev.parent->parent" device. > > Fixes: b5ab5e24e960 ("remoteproc: maintain a generic child device for > each rproc") > > Signed-off-by: Anup Patel <anup@brainfault.org> > Cc: stable@vger.kernel.org > --- > drivers/rpmsg/virtio_rpmsg_bus.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c > index 82b8300..7f8710a 100644 > --- a/drivers/rpmsg/virtio_rpmsg_bus.c > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c > @@ -64,6 +64,7 @@ > struct virtproc_info { > struct virtio_device *vdev; > struct virtqueue *rvq, *svq; > + struct device *bufs_dev; > void *rbufs, *sbufs; > unsigned int num_bufs; > unsigned int buf_size; > @@ -924,9 +925,16 @@ static int rpmsg_probe(struct virtio_device *vdev) > total_buf_space, &vrp->bufs_dma, > GFP_KERNEL); > if (!bufs_va) { > - err = -ENOMEM; > - goto vqs_del; > - } > + bufs_va = dma_alloc_coherent(vdev->dev.parent, > + total_buf_space, &vrp->bufs_dma, > + GFP_KERNEL); > + if (!bufs_va) { > + err = -ENOMEM; > + goto vqs_del; > + } else > + vrp->bufs_dev = vdev->dev.parent; > + } else > + vrp->bufs_dev = vdev->dev.parent->parent; > > dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n", > bufs_va, &vrp->bufs_dma); > @@ -988,7 +996,7 @@ static int rpmsg_probe(struct virtio_device *vdev) > return 0; > > free_coherent: > - dma_free_coherent(vdev->dev.parent->parent, total_buf_space, > + dma_free_coherent(vrp->bufs_dev, total_buf_space, > bufs_va, vrp->bufs_dma); > vqs_del: > vdev->config->del_vqs(vrp->vdev); > @@ -1023,7 +1031,7 @@ static void rpmsg_remove(struct virtio_device *vdev) > > vdev->config->del_vqs(vrp->vdev); > > - dma_free_coherent(vdev->dev.parent->parent, total_buf_space, > + dma_free_coherent(vrp->bufs_dev, total_buf_space, > vrp->rbufs, vrp->bufs_dma); > > kfree(vrp); > -- > 2.7.4 > Hi Bjorn, Can you please review this patch? Regards, Anup -- 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
On Wed 10 Jan 05:16 PST 2018, Anup Patel wrote: > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c [..] > @@ -924,9 +925,16 @@ static int rpmsg_probe(struct virtio_device *vdev) > total_buf_space, &vrp->bufs_dma, > GFP_KERNEL); > if (!bufs_va) { > - err = -ENOMEM; > - goto vqs_del; > - } > + bufs_va = dma_alloc_coherent(vdev->dev.parent, > + total_buf_space, &vrp->bufs_dma, > + GFP_KERNEL); > + if (!bufs_va) { > + err = -ENOMEM; > + goto vqs_del; > + } else > + vrp->bufs_dev = vdev->dev.parent; > + } else > + vrp->bufs_dev = vdev->dev.parent->parent; I really don't fancy the idea of us allocating on behalf of our grandparent here, as you show it's not certain that our grandparent is what someone originally expected it to be. With the purpose of being able to control these allocations there is an ongoing discussion related to this, which I believe will result in this being changed to at least vdev->dev.parent.. I do expect that this discussion will be brought up during Linaro Connect the coming week. Regards, Bjorn -- 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
On Mon, Mar 19, 2018 at 4:17 AM, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Wed 10 Jan 05:16 PST 2018, Anup Patel wrote: >> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c > [..] >> @@ -924,9 +925,16 @@ static int rpmsg_probe(struct virtio_device *vdev) >> total_buf_space, &vrp->bufs_dma, >> GFP_KERNEL); >> if (!bufs_va) { >> - err = -ENOMEM; >> - goto vqs_del; >> - } >> + bufs_va = dma_alloc_coherent(vdev->dev.parent, >> + total_buf_space, &vrp->bufs_dma, >> + GFP_KERNEL); >> + if (!bufs_va) { >> + err = -ENOMEM; >> + goto vqs_del; >> + } else >> + vrp->bufs_dev = vdev->dev.parent; >> + } else >> + vrp->bufs_dev = vdev->dev.parent->parent; > > I really don't fancy the idea of us allocating on behalf of our > grandparent here, as you show it's not certain that our grandparent is > what someone originally expected it to be. > > With the purpose of being able to control these allocations there is an > ongoing discussion related to this, which I believe will result in this > being changed to at least vdev->dev.parent.. > > > I do expect that this discussion will be brought up during Linaro > Connect the coming week. > Currently, rpmsg_probe() is broken for virtio-mmio transport hence I send this patch as a stable fix. In general, I am fine if we are eventually going towards vdev->dev.parent usage. Regards, Anup -- 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 82b8300..7f8710a 100644 --- a/drivers/rpmsg/virtio_rpmsg_bus.c +++ b/drivers/rpmsg/virtio_rpmsg_bus.c @@ -64,6 +64,7 @@ struct virtproc_info { struct virtio_device *vdev; struct virtqueue *rvq, *svq; + struct device *bufs_dev; void *rbufs, *sbufs; unsigned int num_bufs; unsigned int buf_size; @@ -924,9 +925,16 @@ static int rpmsg_probe(struct virtio_device *vdev) total_buf_space, &vrp->bufs_dma, GFP_KERNEL); if (!bufs_va) { - err = -ENOMEM; - goto vqs_del; - } + bufs_va = dma_alloc_coherent(vdev->dev.parent, + total_buf_space, &vrp->bufs_dma, + GFP_KERNEL); + if (!bufs_va) { + err = -ENOMEM; + goto vqs_del; + } else + vrp->bufs_dev = vdev->dev.parent; + } else + vrp->bufs_dev = vdev->dev.parent->parent; dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n", bufs_va, &vrp->bufs_dma); @@ -988,7 +996,7 @@ static int rpmsg_probe(struct virtio_device *vdev) return 0; free_coherent: - dma_free_coherent(vdev->dev.parent->parent, total_buf_space, + dma_free_coherent(vrp->bufs_dev, total_buf_space, bufs_va, vrp->bufs_dma); vqs_del: vdev->config->del_vqs(vrp->vdev); @@ -1023,7 +1031,7 @@ static void rpmsg_remove(struct virtio_device *vdev) vdev->config->del_vqs(vrp->vdev); - dma_free_coherent(vdev->dev.parent->parent, total_buf_space, + dma_free_coherent(vrp->bufs_dev, total_buf_space, vrp->rbufs, vrp->bufs_dma); kfree(vrp);
When virtio-rpmsg device is provided via virtio-mmio transport, the dma_alloc_coherent() (called by rpmsg_probe()) fails on ARM/ARM64 systems because "vdev->dev.parent->parent" device is used as parameter to dma_alloc_coherent(). The "vdev->dev.parent->parent" device represents underlying remoteproc platform device when virtio-rpmsg device is provided via virtio-remoteproc transport. When virtio-rpmsg device is provided via virtio-mmio transport, the "vdev->dev.parent->parent" device represents the parent device of virtio-mmio platform device and dma_alloc_coherent() fails for this device because generally there is no corresponding platform device and dma_ops are not setup for "vdev->dev.parent->parent". This patch fixes dma_alloc_coherent() usage in rpmsg_probe() by trying dma_alloc_coherent() with "vdev->dev.parent" device when it fails with "vdev->dev.parent->parent" device. Fixes: b5ab5e24e960 ("remoteproc: maintain a generic child device for each rproc") Signed-off-by: Anup Patel <anup@brainfault.org> Cc: stable@vger.kernel.org --- drivers/rpmsg/virtio_rpmsg_bus.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)