All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mathieu Poirier <mathieu.poirier@linaro.org>
To: Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>,
	Ohad Ben-Cohen <ohad@wizery.com>,
	linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com, julien.massot@iot.bzh
Subject: Re: [PATCH 3/4] rpmsg: char: Introduce the "rpmsg-raw" channel
Date: Thu, 17 Jun 2021 15:31:54 -0600	[thread overview]
Message-ID: <20210617213154.GA790564@p14s> (raw)
In-Reply-To: <b55cd4e5-fb9d-a0ab-03a9-3a771898db04@foss.st.com>

On Wed, Jun 16, 2021 at 02:38:26PM +0200, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> On 6/15/21 10:01 PM, Mathieu Poirier wrote:
> > On Mon, Jun 07, 2021 at 07:30:31PM +0200, Arnaud Pouliquen wrote:
> >> Allows to probe the endpoint device on a remote name service announcement,
> >> by registering a rpmsg_driverfor the "rpmsg-raw" channel.
> >>
> >> With this patch the /dev/rpmsgX interface can be instantiated by the remote
> >> firmware.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  drivers/rpmsg/rpmsg_char.c | 54 ++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 52 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> >> index 4199ac1bee10..3b850b218eb0 100644
> >> --- a/drivers/rpmsg/rpmsg_char.c
> >> +++ b/drivers/rpmsg/rpmsg_char.c
> >> @@ -25,6 +25,8 @@
> >>  
> >>  #include "rpmsg_char.h"
> >>  
> >> +#define RPMSG_CHAR_DEVNAME "rpmsg-raw"
> >> +
> >>  static dev_t rpmsg_major;
> >>  static struct class *rpmsg_class;
> >>  
> >> @@ -416,6 +418,40 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
> >>  }
> >>  EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
> >>  
> >> +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
> >> +{
> >> +	struct rpmsg_channel_info chinfo;
> >> +
> >> +	memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
> >> +	chinfo.src = rpdev->src;
> >> +	chinfo.dst = rpdev->dst;
> >> +
> >> +	return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true);
> > 
> > I am a little puzzled here as to why we need different modes... Why can't we
> > simply call rpmsg_chrdev_eptdev_create() and let the endpoint be created on
> > open() and destroyed on release() as per the current implementation?
> 
> The main reason is the support of the NS announcement
> a NS announcement is received from the remote processor:
> channel name: "rpmsg-raw"
> remote address (dst address): 0x400
> local address (scr address) : RPMSG_ADDR_ANY
> => no default endpoint, and not local address.
> 
> case 1) if we use legacy implementation ( no default endpoint)
> => create/destroy endpoint on open/stop
> - on first open: created endpoint is bound to scr address 0x406
> - a first message is sent to the remote side, the address 0x406 is stored as
> default channel dst address on remote side.
> - on close: endpoint is closed and associated address 0x406 is free.
> - another driver create an enpoint the address 0x406 is reserved for this new
> endpoint.
> - on new open:  scr address is set to next value 0x407
> => how to inform remote processor that the address has changed?
> => no reservation mechanism that ensure that you can reuse the same address
> 
> case 2) relying on use_default_ept
> => Ensure that both side have always the same addresses to communicate.

I see the problem and your solution is adequate - I think the code simply needs
to be moved around a little.  Here is what I suggest:

1) Create the endpoint in rpmsg_chrdev_probe(), just before calling
rpmsg_chrdev_eptdev_create().  That way changes to rpmsg_eptdev_open() can be
kept to a minimum.  I don't think we'll be needing
__rpmsg_chrdev_eptdev_create() anymore.

2) We can get rid of use_default_ept by taking advantage of the fact that the
rpmsg_char driver does not use rpmsg_device::ept.  If we create the endpoint in
rpmsg_chrdev_probe() we know that if rpdev->ept exists, we must not create
or destroy the endpoint in rpmsg_eptdev_open() and rpmsg_eptdev_release().

3) Function rpmsg_eptdev_open() doesn't change much.  If rpdev->ept is NULL than
an endpoint is created as the current implementation.  Otherwise we simply do:

        eptdev->ept = rpdev->ept;

4) Make sure the teardown path works as well.  From what I can see, it should.

5) Add a __lot__ of comments.

If the above all works this entire patchset should become really small.

> 
> > 
> > I'd rather keep things simple for the refactoring and introduce new features
> > later if need be.
> 
> Yes I agree with you, but here it could become a nightmare for the remote
> processor if the Linux endpoint address is not stable.
> 
> Anyway we can consider this as a workaround waiting the extension of the NS
> announcement to have a better management of the address exchange on channel
> initialization.
> 
> Thanks
> Arnaud
> 
> > 
> > As I said, it may be that I don't understand the usecase.
> > 
> > Thanks,
> > Mathieu
> > 
> >> +}
> >> +
> >> +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
> >> +	if (ret)
> >> +		dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret);
> >> +}
> >> +
> >> +static struct rpmsg_device_id rpmsg_chrdev_id_table[] = {
> >> +	{ .name	= RPMSG_CHAR_DEVNAME },
> >> +	{ },
> >> +};
> >> +
> >> +static struct rpmsg_driver rpmsg_chrdev_driver = {
> >> +	.probe = rpmsg_chrdev_probe,
> >> +	.remove = rpmsg_chrdev_remove,
> >> +	.id_table = rpmsg_chrdev_id_table,
> >> +	.drv = {
> >> +		.name = "rpmsg_chrdev",
> >> +	},
> >> +};
> >> +
> >>  static int rpmsg_chrdev_init(void)
> >>  {
> >>  	int ret;
> >> @@ -429,16 +465,30 @@ static int rpmsg_chrdev_init(void)
> >>  	rpmsg_class = class_create(THIS_MODULE, "rpmsg");
> >>  	if (IS_ERR(rpmsg_class)) {
> >>  		pr_err("failed to create rpmsg class\n");
> >> -		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> >> -		return PTR_ERR(rpmsg_class);
> >> +		ret = PTR_ERR(rpmsg_class);
> >> +		goto free_region;
> >> +	}
> >> +
> >> +	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
> >> +	if (ret < 0) {
> >> +		pr_err("rpmsg: failed to register rpmsg raw driver\n");
> >> +		goto free_class;
> >>  	}
> >>  
> >>  	return 0;
> >> +
> >> +free_class:
> >> +	class_destroy(rpmsg_class);
> >> +free_region:
> >> +	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> >> +
> >> +	return ret;
> >>  }
> >>  postcore_initcall(rpmsg_chrdev_init);
> >>  
> >>  static void rpmsg_chrdev_exit(void)
> >>  {
> >> +	unregister_rpmsg_driver(&rpmsg_chrdev_driver);
> >>  	class_destroy(rpmsg_class);
> >>  	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> >>  }
> >> -- 
> >> 2.17.1
> >>

  reply	other threads:[~2021-06-17 21:32 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-07 17:30 [PATCH 0/4] rpmsg: char: introduce the rpmsg-raw channel Arnaud Pouliquen
2021-06-07 17:30 ` [PATCH 1/4] rpmsg: Introduce rpmsg_create_default_ept function Arnaud Pouliquen
2021-06-21 22:42   ` Mathieu Poirier
2021-06-07 17:30 ` [PATCH 2/4] rpmsg: char: Add possibility to create and reuse default endpoint Arnaud Pouliquen
2021-06-07 17:30 ` [PATCH 3/4] rpmsg: char: Introduce the "rpmsg-raw" channel Arnaud Pouliquen
2021-06-15 20:01   ` Mathieu Poirier
2021-06-16 12:38     ` Arnaud POULIQUEN
2021-06-17 21:31       ` Mathieu Poirier [this message]
2021-06-18 11:35         ` Arnaud POULIQUEN
2021-06-21 22:38           ` Mathieu Poirier
2021-06-22  8:21             ` Arnaud POULIQUEN
2021-06-22 20:30               ` Mathieu Poirier
2021-06-07 17:30 ` [PATCH 4/4] rpmsg: char: Return error if user tries to destroy a default endpoint Arnaud Pouliquen
2021-06-08  9:08 ` [PATCH 0/4] rpmsg: char: introduce the rpmsg-raw channel Julien Massot
2021-06-08 14:26 ` Mathieu Poirier
2021-06-08 15:27   ` Arnaud POULIQUEN

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210617213154.GA790564@p14s \
    --to=mathieu.poirier@linaro.org \
    --cc=arnaud.pouliquen@foss.st.com \
    --cc=bjorn.andersson@linaro.org \
    --cc=julien.massot@iot.bzh \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=linux-stm32@st-md-mailman.stormreply.com \
    --cc=ohad@wizery.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.