| Message ID | 20201222105726.16906-15-arnaud.pouliquen@foss.st.com |
|---|---|
| State | New |
| Headers | show |
| Series | introduce generic IOCTL interface for RPMsg channels management | expand |
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 >
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 --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; }
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(-)