| Message ID | 20211108141937.13016-13-arnaud.pouliquen@foss.st.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Restructure the rpmsg_char driver and introduce rpmsg_ctrl driver | expand |
On Mon 08 Nov 08:19 CST 2021, Arnaud Pouliquen wrote: > When a channel is created by user space application with the > RPMSG_CREATE_DEV_IOCTL controls, a ns announcement has to be sent > (depending on backend) to inform the remote side that a new service > is available. > > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> > --- > drivers/rpmsg/rpmsg_core.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c > index bdcde57c22f6..63227279397d 100644 > --- a/drivers/rpmsg/rpmsg_core.c > +++ b/drivers/rpmsg/rpmsg_core.c > @@ -145,6 +145,9 @@ EXPORT_SYMBOL(rpmsg_destroy_ept); > * > * This function returns a pointer to an endpoint created and assigned as the default > * endpoint of the rpmsg device. > + * If we need to, we also announce about this channel to the remote > + * processor. This announcement is needed in case the driver is exposing an rpmsg service that has > + * been created locally. > * > * Drivers should provide their @rpdev channel (so the new endpoint would belong > * to the same remote processor their channel belongs to), an rx callback > @@ -161,6 +164,7 @@ struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev, > struct rpmsg_channel_info chinfo) > { > struct rpmsg_endpoint *ept; > + int err = 0; > > if (WARN_ON(!rpdev)) > return NULL; > @@ -183,6 +187,16 @@ struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev, > rpdev->ept = ept; > rpdev->src = ept->addr; > > + if (rpdev->ops->announce_create) > + err = rpdev->ops->announce_create(rpdev); > + if (err) { > + rpmsg_destroy_ept(ept); > + rpdev->ept = NULL; > + rpdev->src = RPMSG_ADDR_ANY; > + > + return NULL; > + } > + Unless I'm missing something I think this would be cleaner as: if (rpdev->ops->announce_create) { err = rpdev->ops->announce_create(rpdev); if (err) { ...; } } which also implies that you don't need to zero-initialize err. Other than that, this looks good and follows what would happen in rpmsg_dev_probe()... PS. In rpmsg_dev_probe(), if rpdrv->probe() succeeds but announce_create returns a failure we will exit the function with an error, which will just fail really_probe() and we won't ever clean up the device (i.e. call rpdev->remove()). Regards, Bjorn > return ept; > } > EXPORT_SYMBOL(rpmsg_create_default_ept); > -- > 2.17.1 >
On 12/3/21 2:58 AM, Bjorn Andersson wrote: > On Mon 08 Nov 08:19 CST 2021, Arnaud Pouliquen wrote: > >> When a channel is created by user space application with the >> RPMSG_CREATE_DEV_IOCTL controls, a ns announcement has to be sent >> (depending on backend) to inform the remote side that a new service >> is available. >> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> >> --- >> drivers/rpmsg/rpmsg_core.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c >> index bdcde57c22f6..63227279397d 100644 >> --- a/drivers/rpmsg/rpmsg_core.c >> +++ b/drivers/rpmsg/rpmsg_core.c >> @@ -145,6 +145,9 @@ EXPORT_SYMBOL(rpmsg_destroy_ept); >> * >> * This function returns a pointer to an endpoint created and assigned as the default >> * endpoint of the rpmsg device. >> + * If we need to, we also announce about this channel to the remote >> + * processor. This announcement is needed in case the driver is exposing an rpmsg service that has >> + * been created locally. >> * >> * Drivers should provide their @rpdev channel (so the new endpoint would belong >> * to the same remote processor their channel belongs to), an rx callback >> @@ -161,6 +164,7 @@ struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev, >> struct rpmsg_channel_info chinfo) >> { >> struct rpmsg_endpoint *ept; >> + int err = 0; >> >> if (WARN_ON(!rpdev)) >> return NULL; >> @@ -183,6 +187,16 @@ struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev, >> rpdev->ept = ept; >> rpdev->src = ept->addr; >> >> + if (rpdev->ops->announce_create) >> + err = rpdev->ops->announce_create(rpdev); >> + if (err) { >> + rpmsg_destroy_ept(ept); >> + rpdev->ept = NULL; >> + rpdev->src = RPMSG_ADDR_ANY; >> + >> + return NULL; >> + } >> + > > Unless I'm missing something I think this would be cleaner as: > > if (rpdev->ops->announce_create) { > err = rpdev->ops->announce_create(rpdev); > if (err) { > ...; > } > } > > which also implies that you don't need to zero-initialize err. Ack, i will change this. > > Other than that, this looks good and follows what would happen in > rpmsg_dev_probe()... > > > PS. In rpmsg_dev_probe(), if rpdrv->probe() succeeds but announce_create > returns a failure we will exit the function with an error, which will > just fail really_probe() and we won't ever clean up the device (i.e. > call rpdev->remove()). Right the error management in rpmsg_dev_probe needs to be improved. I will probably found time to address this when preparing the next revision of my patchset (just need before a clarification from you about patch v7 10/12] rpmsg: char: Introduce the "rpmsg-raw" channel). Thanks, Arnaud > > Regards, > Bjorn > >> return ept; >> } >> EXPORT_SYMBOL(rpmsg_create_default_ept); >> -- >> 2.17.1 >>
diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c index bdcde57c22f6..63227279397d 100644 --- a/drivers/rpmsg/rpmsg_core.c +++ b/drivers/rpmsg/rpmsg_core.c @@ -145,6 +145,9 @@ EXPORT_SYMBOL(rpmsg_destroy_ept); * * This function returns a pointer to an endpoint created and assigned as the default * endpoint of the rpmsg device. + * If we need to, we also announce about this channel to the remote + * processor. This announcement is needed in case the driver is exposing an rpmsg service that has + * been created locally. * * Drivers should provide their @rpdev channel (so the new endpoint would belong * to the same remote processor their channel belongs to), an rx callback @@ -161,6 +164,7 @@ struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev, struct rpmsg_channel_info chinfo) { struct rpmsg_endpoint *ept; + int err = 0; if (WARN_ON(!rpdev)) return NULL; @@ -183,6 +187,16 @@ struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev, rpdev->ept = ept; rpdev->src = ept->addr; + if (rpdev->ops->announce_create) + err = rpdev->ops->announce_create(rpdev); + if (err) { + rpmsg_destroy_ept(ept); + rpdev->ept = NULL; + rpdev->src = RPMSG_ADDR_ANY; + + return NULL; + } + return ept; } EXPORT_SYMBOL(rpmsg_create_default_ept);
When a channel is created by user space application with the RPMSG_CREATE_DEV_IOCTL controls, a ns announcement has to be sent (depending on backend) to inform the remote side that a new service is available. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> --- drivers/rpmsg/rpmsg_core.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)