Received: by 2002:a05:6a10:206:0:0:0:0 with SMTP id 6csp994737pxj; Thu, 17 Jun 2021 19:37:44 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzl76bJTTna5y+IHhgOgmZpDHPzkpYWm5CU5a886cJzmDqFUqf9iq7ooLoDzmyYA2uPGwuO X-Received: by 2002:a05:6e02:1207:: with SMTP id a7mr5798289ilq.91.1623983864493; Thu, 17 Jun 2021 19:37:44 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1623983864; cv=none; d=google.com; s=arc-20160816; b=HMozE+UiUpMNV+77MdvgWpjUWf3uK8kNO8XzUx9/1UqpcckLrfcBWcSQTzGdV03G/m T/72XrFDvp798/HHG9bAYM5OX5MYER52Wt26apnnWOxZVR5ravASTAvB5ByfeNzqpIlg kGHueAgGksnjSCj1tJJZ/0xzuQ9yRHDA1Hd6Vjj/nk5L+xOmrDiHlk+1eur2EN8xtMGX y33dY9Dp1jxbWcYu2MMbd9aOljvikLQyPH8DUTYmYujgknMuvKNuxohXwzBTVju11u8e FaXtuRTCzodCUuAD+Bq33ctNmmrSXAmt0+jozn0u64qDRGoURWnIMEWS1HSy/Lf07NJY h7Aw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=1ccgujzKU0wd1rTQD6F7407N4rqCkqhTNutbrnYFWtg=; b=guFp/bdvErBO6L2ASwf78xfuaQ19ETxYInS5j46F3d8JAdLSy4w2aPIsO0QTIjLdeV 56phcSvpDwU0Hpy8o4wqxiGA4vCMX9eBbEzzZeF9/OdNdge2Gn7qeZraKT1J33KBy6YG ZMyDWTlss6/jfCwWbBHuzYrPOdo8KsEbfvjFtKWx7GZAHNpAo5AOPNSPtotCtn/RAETx niFLuO6+NttU1ISh5pZ0m15MzqIIgoBestefGK2fCUw9VMkskzc9n8qgn+biGwMLaNuT MsKhKvszeZkDMWlPXiFBd5yyxL6A3rrYetGuNERMhpNfDinErGvrX+VdQS7YvgQ0xCbo H+9w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=WCNyRPMV; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id g17si4967041jan.22.2021.06.17.19.37.32; Thu, 17 Jun 2021 19:37:44 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=WCNyRPMV; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232680AbhFQVeI (ORCPT + 99 others); Thu, 17 Jun 2021 17:34:08 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45702 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231707AbhFQVeH (ORCPT ); Thu, 17 Jun 2021 17:34:07 -0400 Received: from mail-pf1-x42f.google.com (mail-pf1-x42f.google.com [IPv6:2607:f8b0:4864:20::42f]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 461EEC061574 for ; Thu, 17 Jun 2021 14:31:58 -0700 (PDT) Received: by mail-pf1-x42f.google.com with SMTP id x73so6040112pfc.8 for ; Thu, 17 Jun 2021 14:31:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=1ccgujzKU0wd1rTQD6F7407N4rqCkqhTNutbrnYFWtg=; b=WCNyRPMVg2wLmtyRu4Q86ZkFVzZRNALeN5pyVCm8G4dypcQJ3TGnleGQmkEiA4JeyU jesAcv+AuM4Dscak2YZck5c5jelmQbWSHYPSR0hJz49njhPn/M2RtpW9/OMu2gkCdCYJ maHwqM3cfk4m9XRhTBu9YgHwSVK7ATU6T2WV7H5SF7anvyjRD5qFfik23D1rhOVnMNt6 YvW8WIasWseTwDihmlQjbL5GyeiDIF8R3P/V2fm9Z2UxWklxNVzbrM1iuPNaqYzUgYYG M9YjNwwLGp0Uzz8c71T/Xz6EEcWaYGEKa58ynqKGUwoyfIihWRWszWsx6WizDVcgVh6w ekQw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=1ccgujzKU0wd1rTQD6F7407N4rqCkqhTNutbrnYFWtg=; b=rMPcoddCCS0bzLQt2A+38oIDTSzT2eyOkOa5octSXWGiZ15znSQT9RWF8PiC2olsjn Pw/MsMQf4eE43OH3nr8GFMRNRlfaInqHAk/jcwTKQDbkiKoikiwr79P6Cehbp9N45PbX MUlkf3hCPflWGPI7nLJUy0sd2OZga08e6sETxaj802//KLdLQM4Qm+o9JAYyjM/+IVHN LzMnfTWJDa0Ojq1bRTsuunLkem8FMaIY+ZATryrb9+H9kcNI6bdOtrz80WrEWYWckX25 F48suuN9REPWgu3uvC6T+7A+Lz5OMondCGZ5+MwiQb/6MMK5zARUSCq2LObsVLPNFtcQ ZOcg== X-Gm-Message-State: AOAM5334eOeNWDTaYrJdF6sSqgQhpO8AjRHrtL3i+JO7jFs0XNMH/C+4 ZXZQeBL85d3NSVkYQ8Q/jnKPyQ== X-Received: by 2002:a63:d452:: with SMTP id i18mr6909316pgj.0.1623965517755; Thu, 17 Jun 2021 14:31:57 -0700 (PDT) Received: from p14s (S0106889e681aac74.cg.shawcable.net. [68.147.0.187]) by smtp.gmail.com with ESMTPSA id y190sm6179714pfc.85.2021.06.17.14.31.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 17 Jun 2021 14:31:56 -0700 (PDT) Date: Thu, 17 Jun 2021 15:31:54 -0600 From: Mathieu Poirier To: Arnaud POULIQUEN Cc: Bjorn Andersson , Ohad Ben-Cohen , 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 Message-ID: <20210617213154.GA790564@p14s> References: <20210607173032.30133-1-arnaud.pouliquen@foss.st.com> <20210607173032.30133-4-arnaud.pouliquen@foss.st.com> <20210615200102.GE604521@p14s> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > >> --- > >> 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 > >>