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@xxxxxxxxxxx>
>> ---
>> 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
>>
