On 3/3/21 7:43 PM, Mathieu Poirier wrote:
> On Fri, Feb 19, 2021 at 12:14:56PM +0100, Arnaud Pouliquen wrote:
>> Instantiate the rpmsg_ioctl device on virtio RPMsg bus creation.
>
> s/rpmsg_ioctl/rpmsg_ctrl
>
> Now I understand what you meant in patch 05.
>
>> This provides the possibility to expose the RPMSG_CREATE_EPT_IOCTL
>> to create RPMsg chdev endpoints.
>
> You mean RPMSG device endpoints, i.e rpmsg_eptdev? If so I think it should be
> added to the changelog. Otherwiser someone could be tempted to look for "chdev"
> and find anything but a rpmsg_eptdev.
In fact it is RPMsg char device endpoints, I will add more explicit details in
the changelog.
>
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxxxxxxx>
>>
>> ---
>> V5:
>> Fix compilation issue
>> Reported-by: kernel test robot <lkp@xxxxxxxxx>
>> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>> ---
>> drivers/rpmsg/virtio_rpmsg_bus.c | 57 +++++++++++++++++++++++++++++---
>> 1 file changed, 52 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>> index e87d4cf926eb..2e6b34084012 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -813,14 +813,52 @@ static void rpmsg_xmit_done(struct virtqueue *svq)
>> wake_up_interruptible(&vrp->sendq);
>> }
>>
>> +static struct rpmsg_device *rpmsg_virtio_add_ctrl_dev(struct virtio_device *vdev)
>> +{
>> + struct virtproc_info *vrp = vdev->priv;
>> + struct virtio_rpmsg_channel *vch;
>> + struct rpmsg_device *rpdev_ctrl;
>> + int err = 0;
>> +
>> + vch = kzalloc(sizeof(*vch), GFP_KERNEL);
>> + if (!vch)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + /* Link the channel to the vrp */
>> + vch->vrp = vrp;
>> +
>> + /* Assign public information to the rpmsg_device */
>> + rpdev_ctrl = &vch->rpdev;
>> + rpdev_ctrl->ops = &virtio_rpmsg_ops;
>> +
>> + rpdev_ctrl->dev.parent = &vrp->vdev->dev;
>> + rpdev_ctrl->dev.release = virtio_rpmsg_release_device;
>> + rpdev_ctrl->little_endian = virtio_is_little_endian(vrp->vdev);
>> +
>> + err = rpmsg_ctrl_register_device(rpdev_ctrl);
>> + if (err) {
>> + kfree(vch);
>> + return ERR_PTR(err);
>> + }
>> +
>> + return rpdev_ctrl;
>> +}
>> +
>> +static void rpmsg_virtio_del_ctrl_dev(struct rpmsg_device *rpdev_ctrl)
>> +{
>> + if (!rpdev_ctrl)
>> + return;
>> + kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
>> +}
>> +
>> static int rpmsg_probe(struct virtio_device *vdev)
>> {
>> vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
>> static const char * const names[] = { "input", "output" };
>> struct virtqueue *vqs[2];
>> struct virtproc_info *vrp;
>> - struct virtio_rpmsg_channel *vch;
>> - struct rpmsg_device *rpdev_ns;
>> + struct virtio_rpmsg_channel *vch = NULL;
>> + struct rpmsg_device *rpdev_ns = NULL, *rpdev_ctrl;
>
> As far as I can tell @rpdev_ns doesn't have to be initialized.
You are right,no more needed in V5 with the error cases restructuring.
Thanks,
Arnaud
>
>> void *bufs_va;
>> int err = 0, i;
>> size_t total_buf_space;
>> @@ -894,12 +932,18 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>
>> vdev->priv = vrp;
>>
>> + rpdev_ctrl = rpmsg_virtio_add_ctrl_dev(vdev);
>> + if (IS_ERR(rpdev_ctrl)) {
>> + err = PTR_ERR(rpdev_ctrl);
>> + goto free_coherent;
>> + }
>> +
>> /* if supported by the remote processor, enable the name service */
>> if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
>> vch = kzalloc(sizeof(*vch), GFP_KERNEL);
>> if (!vch) {
>> err = -ENOMEM;
>> - goto free_coherent;
>> + goto free_ctrldev;
>> }
>>
>> /* Link the channel to our vrp */
>> @@ -915,7 +959,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>
>> err = rpmsg_ns_register_device(rpdev_ns);
>> if (err)
>> - goto free_coherent;
>> + goto free_vch;
>> }
>>
>> /*
>> @@ -939,8 +983,11 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>
>> return 0;
>>
>> -free_coherent:
>> +free_vch:
>> kfree(vch);
>> +free_ctrldev:
>> + rpmsg_virtio_del_ctrl_dev(rpdev_ctrl);
>> +free_coherent:
>> dma_free_coherent(vdev->dev.parent, total_buf_space,
>> bufs_va, vrp->bufs_dma);
>> vqs_del:
>> --
>> 2.17.1
>>
