diff mbox series

[v2,14/16] rpmsg: glink: add create and release rpmsg channel ops

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

Commit Message

Arnaud POULIQUEN Dec. 22, 2020, 10:57 a.m. UTC 
Add the new ops introduced by the rpmsg_ns series and used
by the rpmsg_ctrl driver to instantiate a new rpmsg channel.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/qcom_glink_native.c | 94 ++++++++++++++++++++++++-------
 1 file changed, 75 insertions(+), 19 deletions(-)

Comments

Bjorn Andersson Jan. 5, 2021, 1:08 a.m. UTC | #1 
On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

> Add the new ops introduced by the rpmsg_ns series and used
> by the rpmsg_ctrl driver to instantiate a new rpmsg channel.
> 

This is nice for completeness sake, but I don't think it makes sense for
transports that has the nameserver "builtin" to the transport itself.

I.e. if we have the NS sitting on top of GLINK and the remote firmware
sends a "create channel" message, then this code would cause Linux to
send a in-transport "create channel" message back to the remote side in
hope that it would be willing to talk on that channel - but that would
imply that the remote side needs to have registered a rpmsg device
related to that service name - in which case it already sent a
in-transport "create channel" message.

Regards,
Bjorn

> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/qcom_glink_native.c | 94 ++++++++++++++++++++++++-------
>  1 file changed, 75 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 27a05167c18c..d74c338de077 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -205,6 +205,9 @@ static const struct rpmsg_endpoint_ops glink_endpoint_ops;
>  #define GLINK_FEATURE_INTENTLESS	BIT(1)
>  
>  static void qcom_glink_rx_done_work(struct work_struct *work);
> +static struct rpmsg_device *
> +qcom_glink_create_rpdev(struct qcom_glink *glink,
> +			struct rpmsg_channel_info *chinfo);
>  
>  static struct glink_channel *qcom_glink_alloc_channel(struct qcom_glink *glink,
>  						      const char *name)
> @@ -1203,6 +1206,37 @@ static int qcom_glink_announce_create(struct rpmsg_device *rpdev)
>  	return 0;
>  }
>  
> +static struct rpmsg_device *
> +qcom_glink_create_channel(struct rpmsg_device *rp_parent,
> +			  struct rpmsg_channel_info *chinfo)
> +{
> +	struct glink_channel *channel = to_glink_channel(rp_parent->ept);
> +	struct qcom_glink *glink = channel->glink;
> +	struct rpmsg_device *rpdev;
> +	const char *name = chinfo->name;
> +
> +	channel = qcom_glink_alloc_channel(glink, name);
> +	if (IS_ERR(channel))
> +		return ERR_PTR(PTR_ERR(channel));
> +
> +	rpdev = qcom_glink_create_rpdev(glink, chinfo);
> +	if (!IS_ERR(rpdev)) {
> +		rpdev->ept = &channel->ept;
> +		channel->rpdev = rpdev;
> +	}
> +
> +	return rpdev;
> +}
> +
> +static int qcom_glink_release_channel(struct rpmsg_device *rpdev,
> +				      struct rpmsg_channel_info *chinfo)
> +{
> +	struct glink_channel *channel = to_glink_channel(rpdev->ept);
> +	struct qcom_glink *glink = channel->glink;
> +
> +	return rpmsg_unregister_device(glink->dev, chinfo);
> +}
> +
>  static void qcom_glink_destroy_ept(struct rpmsg_endpoint *ept)
>  {
>  	struct glink_channel *channel = to_glink_channel(ept);
> @@ -1359,6 +1393,8 @@ static struct device_node *qcom_glink_match_channel(struct device_node *node,
>  static const struct rpmsg_device_ops glink_device_ops = {
>  	.create_ept = qcom_glink_create_ept,
>  	.announce_create = qcom_glink_announce_create,
> +	.create_channel = qcom_glink_create_channel,
> +	.release_channel = qcom_glink_release_channel,
>  };
>  
>  static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
> @@ -1376,13 +1412,45 @@ static void qcom_glink_rpdev_release(struct device *dev)
>  	kfree(rpdev);
>  }
>  
> +static struct rpmsg_device *
> +qcom_glink_create_rpdev(struct qcom_glink *glink,
> +			struct rpmsg_channel_info *chinfo)
> +{
> +	struct rpmsg_device *rpdev;
> +	struct device_node *node;
> +	int ret;
> +
> +	rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
> +	if (!rpdev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	strncpy(rpdev->id.name, chinfo->name, RPMSG_NAME_SIZE);
> +	rpdev->src = chinfo->src;
> +	rpdev->dst = chinfo->dst;
> +	rpdev->ops = &glink_device_ops;
> +
> +	node = qcom_glink_match_channel(glink->dev->of_node, chinfo->name);
> +	rpdev->dev.of_node = node;
> +	rpdev->dev.parent = glink->dev;
> +	rpdev->dev.release = qcom_glink_rpdev_release;
> +	rpdev->driver_override = (char *)chinfo->driver_override;
> +
> +	ret = rpmsg_register_device(rpdev);
> +	if (ret) {
> +		kfree(rpdev);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return rpdev;
> +}
> +
>  static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
>  			      char *name)
>  {
>  	struct glink_channel *channel;
>  	struct rpmsg_device *rpdev;
>  	bool create_device = false;
> -	struct device_node *node;
> +	struct rpmsg_channel_info chinfo;
>  	int lcid;
>  	int ret;
>  	unsigned long flags;
> @@ -1416,27 +1484,15 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
>  	complete_all(&channel->open_req);
>  
>  	if (create_device) {
> -		rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
> -		if (!rpdev) {
> -			ret = -ENOMEM;
> +		strncpy(chinfo.name, channel->name, sizeof(chinfo.name));
> +		chinfo.src = RPMSG_ADDR_ANY;
> +		chinfo.dst = RPMSG_ADDR_ANY;
> +		rpdev = qcom_glink_create_rpdev(glink, &chinfo);
> +		if (IS_ERR(rpdev)) {
> +			ret = PTR_ERR(rpdev);
>  			goto rcid_remove;
>  		}
> -
>  		rpdev->ept = &channel->ept;
> -		strncpy(rpdev->id.name, name, RPMSG_NAME_SIZE);
> -		rpdev->src = RPMSG_ADDR_ANY;
> -		rpdev->dst = RPMSG_ADDR_ANY;
> -		rpdev->ops = &glink_device_ops;
> -
> -		node = qcom_glink_match_channel(glink->dev->of_node, name);
> -		rpdev->dev.of_node = node;
> -		rpdev->dev.parent = glink->dev;
> -		rpdev->dev.release = qcom_glink_rpdev_release;
> -
> -		ret = rpmsg_register_device(rpdev);
> -		if (ret)
> -			goto rcid_remove;
> -
>  		channel->rpdev = rpdev;
>  	}
>  
> -- 
> 2.17.1
>
Arnaud POULIQUEN Jan. 5, 2021, 5:29 p.m. UTC | #2 
On 1/5/21 2:08 AM, Bjorn Andersson wrote:
> On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:
> 
>> Add the new ops introduced by the rpmsg_ns series and used
>> by the rpmsg_ctrl driver to instantiate a new rpmsg channel.
>>
> 
> This is nice for completeness sake, but I don't think it makes sense for
> transports that has the nameserver "builtin" to the transport itself.
> 
> I.e. if we have the NS sitting on top of GLINK and the remote firmware
> sends a "create channel" message, then this code would cause Linux to
> send a in-transport "create channel" message back to the remote side in
> hope that it would be willing to talk on that channel - but that would
> imply that the remote side needs to have registered a rpmsg device
> related to that service name - in which case it already sent a
> in-transport "create channel" message.

That was one of my main concerns about this series. I'm not familiar enough with
these drivers to make sure my proposal was 100% compatible...
How is the mapping between between the local and remote endpoints when the DST
address is not specified by the Linux application?

Regards,
Arnaud

> 
> Regards,
> Bjorn
> 
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/qcom_glink_native.c | 94 ++++++++++++++++++++++++-------
>>  1 file changed, 75 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
>> index 27a05167c18c..d74c338de077 100644
>> --- a/drivers/rpmsg/qcom_glink_native.c
>> +++ b/drivers/rpmsg/qcom_glink_native.c
>> @@ -205,6 +205,9 @@ static const struct rpmsg_endpoint_ops glink_endpoint_ops;
>>  #define GLINK_FEATURE_INTENTLESS	BIT(1)
>>  
>>  static void qcom_glink_rx_done_work(struct work_struct *work);
>> +static struct rpmsg_device *
>> +qcom_glink_create_rpdev(struct qcom_glink *glink,
>> +			struct rpmsg_channel_info *chinfo);
>>  
>>  static struct glink_channel *qcom_glink_alloc_channel(struct qcom_glink *glink,
>>  						      const char *name)
>> @@ -1203,6 +1206,37 @@ static int qcom_glink_announce_create(struct rpmsg_device *rpdev)
>>  	return 0;
>>  }
>>  
>> +static struct rpmsg_device *
>> +qcom_glink_create_channel(struct rpmsg_device *rp_parent,
>> +			  struct rpmsg_channel_info *chinfo)
>> +{
>> +	struct glink_channel *channel = to_glink_channel(rp_parent->ept);
>> +	struct qcom_glink *glink = channel->glink;
>> +	struct rpmsg_device *rpdev;
>> +	const char *name = chinfo->name;
>> +
>> +	channel = qcom_glink_alloc_channel(glink, name);
>> +	if (IS_ERR(channel))
>> +		return ERR_PTR(PTR_ERR(channel));
>> +
>> +	rpdev = qcom_glink_create_rpdev(glink, chinfo);
>> +	if (!IS_ERR(rpdev)) {
>> +		rpdev->ept = &channel->ept;
>> +		channel->rpdev = rpdev;
>> +	}
>> +
>> +	return rpdev;
>> +}
>> +
>> +static int qcom_glink_release_channel(struct rpmsg_device *rpdev,
>> +				      struct rpmsg_channel_info *chinfo)
>> +{
>> +	struct glink_channel *channel = to_glink_channel(rpdev->ept);
>> +	struct qcom_glink *glink = channel->glink;
>> +
>> +	return rpmsg_unregister_device(glink->dev, chinfo);
>> +}
>> +
>>  static void qcom_glink_destroy_ept(struct rpmsg_endpoint *ept)
>>  {
>>  	struct glink_channel *channel = to_glink_channel(ept);
>> @@ -1359,6 +1393,8 @@ static struct device_node *qcom_glink_match_channel(struct device_node *node,
>>  static const struct rpmsg_device_ops glink_device_ops = {
>>  	.create_ept = qcom_glink_create_ept,
>>  	.announce_create = qcom_glink_announce_create,
>> +	.create_channel = qcom_glink_create_channel,
>> +	.release_channel = qcom_glink_release_channel,
>>  };
>>  
>>  static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
>> @@ -1376,13 +1412,45 @@ static void qcom_glink_rpdev_release(struct device *dev)
>>  	kfree(rpdev);
>>  }
>>  
>> +static struct rpmsg_device *
>> +qcom_glink_create_rpdev(struct qcom_glink *glink,
>> +			struct rpmsg_channel_info *chinfo)
>> +{
>> +	struct rpmsg_device *rpdev;
>> +	struct device_node *node;
>> +	int ret;
>> +
>> +	rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
>> +	if (!rpdev)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	strncpy(rpdev->id.name, chinfo->name, RPMSG_NAME_SIZE);
>> +	rpdev->src = chinfo->src;
>> +	rpdev->dst = chinfo->dst;
>> +	rpdev->ops = &glink_device_ops;
>> +
>> +	node = qcom_glink_match_channel(glink->dev->of_node, chinfo->name);
>> +	rpdev->dev.of_node = node;
>> +	rpdev->dev.parent = glink->dev;
>> +	rpdev->dev.release = qcom_glink_rpdev_release;
>> +	rpdev->driver_override = (char *)chinfo->driver_override;
>> +
>> +	ret = rpmsg_register_device(rpdev);
>> +	if (ret) {
>> +		kfree(rpdev);
>> +		return ERR_PTR(ret);
>> +	}
>> +
>> +	return rpdev;
>> +}
>> +
>>  static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
>>  			      char *name)
>>  {
>>  	struct glink_channel *channel;
>>  	struct rpmsg_device *rpdev;
>>  	bool create_device = false;
>> -	struct device_node *node;
>> +	struct rpmsg_channel_info chinfo;
>>  	int lcid;
>>  	int ret;
>>  	unsigned long flags;
>> @@ -1416,27 +1484,15 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
>>  	complete_all(&channel->open_req);
>>  
>>  	if (create_device) {
>> -		rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
>> -		if (!rpdev) {
>> -			ret = -ENOMEM;
>> +		strncpy(chinfo.name, channel->name, sizeof(chinfo.name));
>> +		chinfo.src = RPMSG_ADDR_ANY;
>> +		chinfo.dst = RPMSG_ADDR_ANY;
>> +		rpdev = qcom_glink_create_rpdev(glink, &chinfo);
>> +		if (IS_ERR(rpdev)) {
>> +			ret = PTR_ERR(rpdev);
>>  			goto rcid_remove;
>>  		}
>> -
>>  		rpdev->ept = &channel->ept;
>> -		strncpy(rpdev->id.name, name, RPMSG_NAME_SIZE);
>> -		rpdev->src = RPMSG_ADDR_ANY;
>> -		rpdev->dst = RPMSG_ADDR_ANY;
>> -		rpdev->ops = &glink_device_ops;
>> -
>> -		node = qcom_glink_match_channel(glink->dev->of_node, name);
>> -		rpdev->dev.of_node = node;
>> -		rpdev->dev.parent = glink->dev;
>> -		rpdev->dev.release = qcom_glink_rpdev_release;
>> -
>> -		ret = rpmsg_register_device(rpdev);
>> -		if (ret)
>> -			goto rcid_remove;
>> -
>>  		channel->rpdev = rpdev;
>>  	}
>>  
>> -- 
>> 2.17.1
>>
diff mbox series

Patch

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 27a05167c18c..d74c338de077 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -205,6 +205,9 @@  static const struct rpmsg_endpoint_ops glink_endpoint_ops;
 #define GLINK_FEATURE_INTENTLESS	BIT(1)
 
 static void qcom_glink_rx_done_work(struct work_struct *work);
+static struct rpmsg_device *
+qcom_glink_create_rpdev(struct qcom_glink *glink,
+			struct rpmsg_channel_info *chinfo);
 
 static struct glink_channel *qcom_glink_alloc_channel(struct qcom_glink *glink,
 						      const char *name)
@@ -1203,6 +1206,37 @@  static int qcom_glink_announce_create(struct rpmsg_device *rpdev)
 	return 0;
 }
 
+static struct rpmsg_device *
+qcom_glink_create_channel(struct rpmsg_device *rp_parent,
+			  struct rpmsg_channel_info *chinfo)
+{
+	struct glink_channel *channel = to_glink_channel(rp_parent->ept);
+	struct qcom_glink *glink = channel->glink;
+	struct rpmsg_device *rpdev;
+	const char *name = chinfo->name;
+
+	channel = qcom_glink_alloc_channel(glink, name);
+	if (IS_ERR(channel))
+		return ERR_PTR(PTR_ERR(channel));
+
+	rpdev = qcom_glink_create_rpdev(glink, chinfo);
+	if (!IS_ERR(rpdev)) {
+		rpdev->ept = &channel->ept;
+		channel->rpdev = rpdev;
+	}
+
+	return rpdev;
+}
+
+static int qcom_glink_release_channel(struct rpmsg_device *rpdev,
+				      struct rpmsg_channel_info *chinfo)
+{
+	struct glink_channel *channel = to_glink_channel(rpdev->ept);
+	struct qcom_glink *glink = channel->glink;
+
+	return rpmsg_unregister_device(glink->dev, chinfo);
+}
+
 static void qcom_glink_destroy_ept(struct rpmsg_endpoint *ept)
 {
 	struct glink_channel *channel = to_glink_channel(ept);
@@ -1359,6 +1393,8 @@  static struct device_node *qcom_glink_match_channel(struct device_node *node,
 static const struct rpmsg_device_ops glink_device_ops = {
 	.create_ept = qcom_glink_create_ept,
 	.announce_create = qcom_glink_announce_create,
+	.create_channel = qcom_glink_create_channel,
+	.release_channel = qcom_glink_release_channel,
 };
 
 static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
@@ -1376,13 +1412,45 @@  static void qcom_glink_rpdev_release(struct device *dev)
 	kfree(rpdev);
 }
 
+static struct rpmsg_device *
+qcom_glink_create_rpdev(struct qcom_glink *glink,
+			struct rpmsg_channel_info *chinfo)
+{
+	struct rpmsg_device *rpdev;
+	struct device_node *node;
+	int ret;
+
+	rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
+	if (!rpdev)
+		return ERR_PTR(-ENOMEM);
+
+	strncpy(rpdev->id.name, chinfo->name, RPMSG_NAME_SIZE);
+	rpdev->src = chinfo->src;
+	rpdev->dst = chinfo->dst;
+	rpdev->ops = &glink_device_ops;
+
+	node = qcom_glink_match_channel(glink->dev->of_node, chinfo->name);
+	rpdev->dev.of_node = node;
+	rpdev->dev.parent = glink->dev;
+	rpdev->dev.release = qcom_glink_rpdev_release;
+	rpdev->driver_override = (char *)chinfo->driver_override;
+
+	ret = rpmsg_register_device(rpdev);
+	if (ret) {
+		kfree(rpdev);
+		return ERR_PTR(ret);
+	}
+
+	return rpdev;
+}
+
 static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
 			      char *name)
 {
 	struct glink_channel *channel;
 	struct rpmsg_device *rpdev;
 	bool create_device = false;
-	struct device_node *node;
+	struct rpmsg_channel_info chinfo;
 	int lcid;
 	int ret;
 	unsigned long flags;
@@ -1416,27 +1484,15 @@  static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
 	complete_all(&channel->open_req);
 
 	if (create_device) {
-		rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
-		if (!rpdev) {
-			ret = -ENOMEM;
+		strncpy(chinfo.name, channel->name, sizeof(chinfo.name));
+		chinfo.src = RPMSG_ADDR_ANY;
+		chinfo.dst = RPMSG_ADDR_ANY;
+		rpdev = qcom_glink_create_rpdev(glink, &chinfo);
+		if (IS_ERR(rpdev)) {
+			ret = PTR_ERR(rpdev);
 			goto rcid_remove;
 		}
-
 		rpdev->ept = &channel->ept;
-		strncpy(rpdev->id.name, name, RPMSG_NAME_SIZE);
-		rpdev->src = RPMSG_ADDR_ANY;
-		rpdev->dst = RPMSG_ADDR_ANY;
-		rpdev->ops = &glink_device_ops;
-
-		node = qcom_glink_match_channel(glink->dev->of_node, name);
-		rpdev->dev.of_node = node;
-		rpdev->dev.parent = glink->dev;
-		rpdev->dev.release = qcom_glink_rpdev_release;
-
-		ret = rpmsg_register_device(rpdev);
-		if (ret)
-			goto rcid_remove;
-
 		channel->rpdev = rpdev;
 	}