From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 137DDC433EF for ; Fri, 25 Mar 2022 17:35:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229763AbiCYRg5 (ORCPT ); Fri, 25 Mar 2022 13:36:57 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41516 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230476AbiCYRg5 (ORCPT ); Fri, 25 Mar 2022 13:36:57 -0400 Received: from mail-pg1-x535.google.com (mail-pg1-x535.google.com [IPv6:2607:f8b0:4864:20::535]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4E76FD5EB7 for ; Fri, 25 Mar 2022 10:35:15 -0700 (PDT) Received: by mail-pg1-x535.google.com with SMTP id k14so7010025pga.0 for ; Fri, 25 Mar 2022 10:35:15 -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=WDLVL2CBtTtmZ03hvae7Gda3UiodG88S9fzgPpRXpvw=; b=OiWmJSqsd56fJnsMw7vy14nT7y4RqvdUDtzT0hRVl8eJM8j+kwYhzyKSXtH1X0btj+ gF6iVne9zsezOcHNXAWwuNmE+7+4oXfeQ3zzCnBO1EMiKA0V4JaqdB1fUHTpBZyfP16f mv5riwoU/Uzt2nQrsxTceBnO5Q8peVyJiIhNvjpN/r8ZKS/IRxBoZAY4eQgLXM1yU1wf RuyTp3+5TNjAhKRzpedRkXInlLXr+vIIQN8Ju/9g6WcTI/Q1Gx2gMlDHTgUo7q67SlIA XglOP+lMKdSyY9n3ZWaZDOe94rZRoU4QCqE3q5wD7lW1aIZ4E8iuhVq7n2Jf9qIqBUb6 mz9g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=WDLVL2CBtTtmZ03hvae7Gda3UiodG88S9fzgPpRXpvw=; b=HdMH7pUg9T3h3HBUGP69zXwjF5M2i7rcuMZKHHCvLZgneKYzzFLdmD5D5xmz0iMpep DNvySLrCEUwxVyp3gnakBHNRdZbmUVgIlLPPtOOuvxN/s98XOq4lFnPVANMjEpO2uePd 6iwZoiTDYnBfu6TbK5CD2TpVxxFPZenvtSUA0zP0qAt8MpjO7k2WhZC4MuuAVUT5gf4I HnZTZv5WcNjYGzslJTfVlL7vuuAezv/xVx8F18F3FxF0WIO7zqCzkHfR3H8z6ZZ1G8tL hanwkWCedkMfuhyj3NAn8TC3sw+/k/Wy8a0RRJicJA95HIzowYXqYTMuSC5/Kpd2PVll cYuw== X-Gm-Message-State: AOAM5301uNhaVR946z9MzU7k/zLjUKm+4Rbks7Rb56qdy3mF/G3wstAZ l8ZNMQppFpnLyfIL5O4ikrksNFnMbwJq9w== X-Google-Smtp-Source: ABdhPJx4UuZpPPNTjdIKS58siaD7XKd04M0BmM0IvWb3s03mLNTfP7MWSWNilRuPWBWICiRC4ete5Q== X-Received: by 2002:a05:6a00:98e:b0:4fb:1162:b2a5 with SMTP id u14-20020a056a00098e00b004fb1162b2a5mr4155395pfg.12.1648228956842; Fri, 25 Mar 2022 10:22:36 -0700 (PDT) Received: from p14s (S0106889e681aac74.cg.shawcable.net. [68.147.0.187]) by smtp.gmail.com with ESMTPSA id 16-20020a17090a005000b001c7511dc31esm6674946pjb.41.2022.03.25.10.22.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 25 Mar 2022 10:22:35 -0700 (PDT) Date: Fri, 25 Mar 2022 11:22:33 -0600 From: Mathieu Poirier To: "S.J. Wang" Cc: "bjorn.andersson@linaro.org" , "linux-remoteproc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "shengjiu.wang@gmail.com" Subject: Re: [PATCH] remoteproc: core: Remove state checking before changing state Message-ID: <20220325172233.GB3576184@p14s> References: 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-remoteproc@vger.kernel.org On Fri, Mar 25, 2022 at 03:44:20AM +0000, S.J. Wang wrote: > Hi > > > > > > > There is no mutex protecting of these state checking, which can't > > > garantee there is no another instance is trying to do same operation. > > > > > > The reference counter rproc->power is used to manage state changing > > > and there is mutex protection in each operation function for multi > > > instance case. > > > > > > So remove this state checking in rproc_cdev_write() and state_store(), > > > just let reference counter rproc->power to manage the behaviors. > > > > > > Signed-off-by: Shengjiu Wang > > > --- > > > drivers/remoteproc/remoteproc_cdev.c | 11 ----------- > > > drivers/remoteproc/remoteproc_sysfs.c | 11 ----------- > > > 2 files changed, 22 deletions(-) > > > > > > diff --git a/drivers/remoteproc/remoteproc_cdev.c > > > b/drivers/remoteproc/remoteproc_cdev.c > > > index 906ff3c4dfdd..687f205fd70a 100644 > > > --- a/drivers/remoteproc/remoteproc_cdev.c > > > +++ b/drivers/remoteproc/remoteproc_cdev.c > > > @@ -32,21 +32,10 @@ static ssize_t rproc_cdev_write(struct file *filp, > > const char __user *buf, size_ > > > return -EFAULT; > > > > > > if (!strncmp(cmd, "start", len)) { > > > - if (rproc->state == RPROC_RUNNING || > > > - rproc->state == RPROC_ATTACHED) > > > - return -EBUSY; > > > - > > > ret = rproc_boot(rproc); > > > } else if (!strncmp(cmd, "stop", len)) { > > > - if (rproc->state != RPROC_RUNNING && > > > - rproc->state != RPROC_ATTACHED) > > > - return -EINVAL; > > > - > > > ret = rproc_shutdown(rproc); > > > } else if (!strncmp(cmd, "detach", len)) { > > > - if (rproc->state != RPROC_ATTACHED) > > > - return -EINVAL; > > > - > > > ret = rproc_detach(rproc); > > > } else { > > > dev_err(&rproc->dev, "Unrecognized option\n"); diff > > > --git a/drivers/remoteproc/remoteproc_sysfs.c > > > b/drivers/remoteproc/remoteproc_sysfs.c > > > index 51a04bc6ba7a..8c7ea8922638 100644 > > > --- a/drivers/remoteproc/remoteproc_sysfs.c > > > +++ b/drivers/remoteproc/remoteproc_sysfs.c > > > @@ -194,23 +194,12 @@ static ssize_t state_store(struct device *dev, > > > int ret = 0; > > > > > > if (sysfs_streq(buf, "start")) { > > > - if (rproc->state == RPROC_RUNNING || > > > - rproc->state == RPROC_ATTACHED) > > > - return -EBUSY; > > > - > > > > As per my previous comment the above conditions need to be moved into > > rproc_boot(). > > > > > ret = rproc_boot(rproc); > > > if (ret) > > > dev_err(&rproc->dev, "Boot failed: %d\n", ret); > > > } else if (sysfs_streq(buf, "stop")) { > > > - if (rproc->state != RPROC_RUNNING && > > > - rproc->state != RPROC_ATTACHED) > > > - return -EINVAL; > > > - > > > ret = rproc_shutdown(rproc); > > > } else if (sysfs_streq(buf, "detach")) { > > > - if (rproc->state != RPROC_ATTACHED) > > > - return -EINVAL; > > > - > > > > This patch should have been part of a patch series with your other work sent > > on March 18th[1]. > > > > Thanks, > > Mathieu > > > > [1]. [PATCH] remoteproc: core: check rproc->power value before decreasing > > it > > > > Thanks for the comments. > I still have one question, if there are two instances independently to 'start' > 'stop' remoteproc, for example: > > Instance1: echo start > /sys/class/remoteproc/remoteproc0/state > Instance2: echo start > /sys/class/remoteproc/remoteproc0/state > > ... > > Instance2: echo stop > /sys/class/remoteproc/remoteproc0/state > ... > Instance1: echo stop > /sys/class/remoteproc/remoteproc0/state > > When instance2 'stop' the remoteproc, then instance1 will be impacted for > It still needs the service from remoteproc. > > That's why I just removed of the checking state, didn't move them to > rproc_boot()/rproc_shutdown()/rproc_detach(). And in order to utilize > the reference counter (rproc->power) to handle the multi-instance case. Thanks for the exta information, now I understand the problem. The above should be part of the changelog. There are two problems here: 1) Dealing with race conditions when checking the state of a remote processor. 2) Properly dealing with the remote processor's reference count. For the first one, state checks control the remote processor state machine and can't simply be removed. They have to be brought inside the mutex lock in order to avoid race conditions when multiple users interact with the remote processor. For the second one, moving the rproc->state checks inside the mutex lock in rproc_shutdown() and rproc_detach() will work with the rproc->power check. The problem is with rproc_boot(). For (at least) two years now we have lost the capability to increase the rproc->power reference count from sysfs and the cdev interface due to the rproc-state checks in state_store() and rproc_cdev_write(). I think that should be corrected but it will introduce a user space visible change, which needs to be treated carefully. With the above in mind, please send a patchset that includes one patch that removes the rproc->state checks from the "start" command in state_store() and rproc_cdev_write(). The other patch should move the rproc->state checks for the "stop" and "detach" command inside rproc_shutdown() and rproc_detach(). With that we can start a discussion on the best way to proceed. Thanks, Mathieu > > > Best regards > Wang Shengjiu