diff mbox series

[v5,11/16] rpmsg: virtio: register the rpmsg_ctrl device

Message ID 20210219111501.14261-12-arnaud.pouliquen@foss.st.com (mailing list archive)
State New, archived
Headers show
Series introduce a generic IOCTL interface for RPMsg channels management | expand

Commit Message

Arnaud POULIQUEN Feb. 19, 2021, 11:14 a.m. UTC 
Instantiate the rpmsg_ioctl device on virtio RPMsg bus creation.
This provides the possibility to expose the RPMSG_CREATE_EPT_IOCTL
to create RPMsg chdev endpoints.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>

---
V5:
Fix compilation issue
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 57 +++++++++++++++++++++++++++++---
 1 file changed, 52 insertions(+), 5 deletions(-)

Comments

Mathieu Poirier March 3, 2021, 6:43 p.m. UTC | #1 
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.

> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> 
> ---
> V5:
> Fix compilation issue
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  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.

>  	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
>
Arnaud POULIQUEN March 4, 2021, 9:20 a.m. UTC | #2 
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@foss.st.com>
>>
>> ---
>> V5:
>> Fix compilation issue
>> Reported-by: kernel test robot <lkp@intel.com>
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> ---
>>  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
>>
diff mbox series

Patch

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