Received: by 2002:a05:6a10:206:0:0:0:0 with SMTP id 6csp578114pxj; Fri, 28 May 2021 10:19:36 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzr/HfBAmaZ8C7Il/+poVng0kvC1/byB6PT2KpEFnm2gDtYhGoNRTiOXe8FiiAOwr3phxQM X-Received: by 2002:a92:c26f:: with SMTP id h15mr7651059ild.298.1622222376616; Fri, 28 May 2021 10:19:36 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1622222376; cv=none; d=google.com; s=arc-20160816; b=rlQ8LNjtYdoeTl5b05uKW1SLB/amrOjrsS57fMTm3MOeDovchrvOOj7a8dv+UpY6xq Pa/UPERvPPIoI7U3b1rvV6nDikDV4OdzDlctb3MjVrgMMznCArCWxbmQEyjy4w6H259D +3mj0TUB7s9R9OlujTf9EpIsf7xLQ8XiM+8NmLvtbbipo223cEMsqpt7X433xK2wracB fbSrOvhlugFnmgpuLzDtLHXLBHvMiz9CzMgGpnfSnz9CkYnS4tSlb3t5hXemxK7MJ7y2 6Lf06Tp3l+E49pz+H4/0akSPPXykOVxUqzbzTjP2CQqf+X2Y9Fafk7qpCnRApwy5TZD2 yX0Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:content-language :in-reply-to:mime-version:user-agent:date:message-id:from:references :cc:to:subject:dkim-signature; bh=8+ulhTM+gzJOOYMrxR6OJCETO3scGYEm/Q7ZRZMWEXE=; b=bjf4CC45lDkSlFiFcasLzrr7FetWtk3Tazgs32OuYo+E4NNFnIi9F/ndBNLMmrpN2t SdTtHj5Z5cGr+n545sohN3XG3TRHTFRspcA0+tVgUiRFC+1f5dsaxf9GEfygnEdvKRan Of0txPjkjM47BeD5merntgLX7GOlySt7f0pRDAQKbfRT1rO9PRpF3oWYG9JLsEPAS7XN AxRf8dkeaF2285AJB4gQepjE0ylqxbO07D2GiiZRD/hCbgG9xPwinYhHKPZ4qTbl5uol jy4JJZ3zQO81IKGOu3nVGZCUTjlKK7eendngAIj4k3jtOd3sDi8wyQ6WCTvMx8ka5RGj gpWQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@ti.com header.s=ti-com-17Q1 header.b=rh+u7S6n; 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=QUARANTINE sp=NONE dis=NONE) header.from=ti.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id w3si6216786iow.99.2021.05.28.10.19.23; Fri, 28 May 2021 10:19:36 -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=@ti.com header.s=ti-com-17Q1 header.b=rh+u7S6n; 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=QUARANTINE sp=NONE dis=NONE) header.from=ti.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236290AbhE1Qlo (ORCPT + 99 others); Fri, 28 May 2021 12:41:44 -0400 Received: from fllv0016.ext.ti.com ([198.47.19.142]:54810 "EHLO fllv0016.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235570AbhE1Qlm (ORCPT ); Fri, 28 May 2021 12:41:42 -0400 Received: from lelv0265.itg.ti.com ([10.180.67.224]) by fllv0016.ext.ti.com (8.15.2/8.15.2) with ESMTP id 14SGe3TH016682; Fri, 28 May 2021 11:40:03 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1622220003; bh=8+ulhTM+gzJOOYMrxR6OJCETO3scGYEm/Q7ZRZMWEXE=; h=Subject:To:CC:References:From:Date:In-Reply-To; b=rh+u7S6nbxD3S+XkNjBnIHRcsT26C5ZGmIZ66fNkAszR9Y9+s/jL6mJ6huEBBztY0 5/M91bDFg3okvNkavx62dVBZCLWXVllpcixTbJTUVQrRbZEqXKUWPdMN0UIN7keXev KZw75FHH/LOVO7PnIxXvZgssSINqrwWC6XU1chW4= Received: from DFLE107.ent.ti.com (dfle107.ent.ti.com [10.64.6.28]) by lelv0265.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 14SGe2iK013662 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Fri, 28 May 2021 11:40:02 -0500 Received: from DFLE114.ent.ti.com (10.64.6.35) by DFLE107.ent.ti.com (10.64.6.28) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2176.2; Fri, 28 May 2021 11:40:02 -0500 Received: from fllv0040.itg.ti.com (10.64.41.20) by DFLE114.ent.ti.com (10.64.6.35) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2176.2 via Frontend Transport; Fri, 28 May 2021 11:40:02 -0500 Received: from [10.250.35.153] (ileax41-snat.itg.ti.com [10.172.224.153]) by fllv0040.itg.ti.com (8.15.2/8.15.2) with ESMTP id 14SGe2S7051725; Fri, 28 May 2021 11:40:02 -0500 Subject: Re: [PATCH 2/6] remoteproc: Add support for detach-only during shutdown To: Bjorn Andersson CC: Mathieu Poirier , Lokesh Vutla , Tero Kristo , , , References: <20210522000309.26134-1-s-anna@ti.com> <20210522000309.26134-3-s-anna@ti.com> From: Suman Anna Message-ID: Date: Fri, 28 May 2021 11:40:02 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Bjorn, On 5/27/21 11:11 PM, Bjorn Andersson wrote: > On Fri 21 May 19:03 CDT 2021, Suman Anna wrote: > >> The remoteproc core has support for both stopping and detaching a >> remote processor that was attached to previously, through both the >> remoteproc sysfs and cdev interfaces. The rproc_shutdown() though >> unconditionally only uses the stop functionality at present. This >> may not be the default desired functionality for all the remoteproc >> platform drivers. >> >> Introduce a new rproc state flag 'detach_on_shutdown' that individual >> remoteproc drivers can set to only allow detach in rproc_shutdown() >> that would have been invoked when the driver is uninstalled, so that >> remote processor continues to run undisturbed even after the driver >> removal. >> > > I dislike the introduction of knobs for everything and would much rather > see that we define some sound defaults. Can we make shutdown just do > detach() if that's supported otherwise stop(). > I maybe missing your point, but the change in remoteproc_core below exactly does that, right? Are you saying drop the checks in remoteproc_cdev and remoteproc_sysfs? The asymmetry did bug me as well, but it is already existing even before this patch. I personally would have preferred a cleaner and symmetrical attach, start, stop, detach, but existing code has overloaded attach into start (keys off by RPROC_OFFLINE/RPROC_DETACHED) while introducing a separate detach from stop. I have retained the meaning of stop as shutdown from userspace interface perspective, but enforcing the checks for detach only remoteprocs. The logic in rproc_shutdown is for driver paths. > This still allows userspace to explicitly stop the detachable remoteproc > before shutdown, if for some reason that's what you want... This is the existing behavior and the difference between stop and detach. That behavior is maintained for remoteprocs not setting the detach_on_shutdown flag. I am only restricting the behavior for those that set it. Mathieu, Your thoughts on this? regards Suman > > Regards, > Bjorn > >> Signed-off-by: Suman Anna >> --- >> drivers/remoteproc/remoteproc_cdev.c | 7 +++++++ >> drivers/remoteproc/remoteproc_core.c | 5 ++++- >> drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++ >> include/linux/remoteproc.h | 3 +++ >> 4 files changed, 20 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c >> index 0b8a84c04f76..473467711a09 100644 >> --- a/drivers/remoteproc/remoteproc_cdev.c >> +++ b/drivers/remoteproc/remoteproc_cdev.c >> @@ -42,6 +42,13 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_ >> rproc->state != RPROC_ATTACHED) >> return -EINVAL; >> >> + if (rproc->state == RPROC_ATTACHED && >> + rproc->detach_on_shutdown) { >> + dev_err(&rproc->dev, >> + "stop not supported for this rproc, use detach\n"); >> + return -EINVAL; >> + } >> + >> rproc_shutdown(rproc); >> } else if (!strncmp(cmd, "detach", len)) { >> if (rproc->state != RPROC_ATTACHED) >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >> index 6019f46001c8..e8ab3eb41f00 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -2074,7 +2074,10 @@ void rproc_shutdown(struct rproc *rproc) >> if (!atomic_dec_and_test(&rproc->power)) >> goto out; >> >> - ret = rproc_stop(rproc, false); >> + if (rproc->detach_on_shutdown && rproc->state == RPROC_ATTACHED) >> + ret = __rproc_detach(rproc); >> + else >> + ret = rproc_stop(rproc, false); >> if (ret) { >> atomic_inc(&rproc->power); >> goto out; >> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c >> index ea8b89f97d7b..1785fbcb1075 100644 >> --- a/drivers/remoteproc/remoteproc_sysfs.c >> +++ b/drivers/remoteproc/remoteproc_sysfs.c >> @@ -206,6 +206,12 @@ static ssize_t state_store(struct device *dev, >> rproc->state != RPROC_ATTACHED) >> return -EINVAL; >> >> + if (rproc->state == RPROC_ATTACHED && >> + rproc->detach_on_shutdown) { >> + dev_err(&rproc->dev, "stop not supported for this rproc, use detach\n"); >> + return -EINVAL; >> + } >> + >> rproc_shutdown(rproc); >> } else if (sysfs_streq(buf, "detach")) { >> if (rproc->state != RPROC_ATTACHED) >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >> index 42a1f30e33a7..35ef921676a1 100644 >> --- a/include/linux/remoteproc.h >> +++ b/include/linux/remoteproc.h >> @@ -530,6 +530,8 @@ struct rproc_dump_segment { >> * @elf_machine: firmware ELF machine >> * @cdev: character device of the rproc >> * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release >> + * @detach_on_shutdown: flag to indicate if remoteproc cannot be shutdown in >> + * attached state and _only_ support detach >> */ >> struct rproc { >> struct list_head node; >> @@ -569,6 +571,7 @@ struct rproc { >> u16 elf_machine; >> struct cdev cdev; >> bool cdev_put_on_release; >> + bool detach_on_shutdown; >> }; >> >> /** >> -- >> 2.30.1 >>