From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nathan_Lynch@mentor.com (Nathan Lynch) Date: Wed, 26 Aug 2015 12:08:48 -0500 Subject: [PATCH 4/4] remoteproc: debugfs: Add ability to boot remote processor using debugfs In-Reply-To: <1440594483-29601-5-git-send-email-lee.jones@linaro.org> References: <1440594483-29601-1-git-send-email-lee.jones@linaro.org> <1440594483-29601-5-git-send-email-lee.jones@linaro.org> Message-ID: <55DDF2A0.5010404@mentor.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 08/26/2015 08:08 AM, Lee Jones wrote: > Signed-off-by: Lee Jones > --- > drivers/remoteproc/remoteproc_debugfs.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) The commit message should describe why this is needed... > diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c > index 9d30809..9620962 100644 > --- a/drivers/remoteproc/remoteproc_debugfs.c > +++ b/drivers/remoteproc/remoteproc_debugfs.c > @@ -88,8 +88,33 @@ static ssize_t rproc_state_read(struct file *filp, char __user *userbuf, > return simple_read_from_buffer(userbuf, count, ppos, buf, i); > } > > +static ssize_t rproc_state_write(struct file *filp, const char __user *userbuf, > + size_t count, loff_t *ppos) > +{ > + struct rproc *rproc = filp->private_data; > + char buf[2]; > + int ret; > + > + ret = copy_from_user(buf, userbuf, 1); > + if (ret) > + return -EFAULT; > + > + switch (buf[0]) { > + case '1': > + ret = rproc_boot(rproc); > + if (ret) > + dev_warn(&rproc->dev, "Boot failed: %d\n", ret); > + break; > + default: > + rproc_shutdown(rproc); > + } > + > + return count; > +} ... and I suggest that the user interface be reconsidered. If '1' means "boot" and literally anything else means "shut down" then you can't add operations in the future without potentially breaking things. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nathan Lynch Subject: Re: [PATCH 4/4] remoteproc: debugfs: Add ability to boot remote processor using debugfs Date: Wed, 26 Aug 2015 12:08:48 -0500 Message-ID: <55DDF2A0.5010404@mentor.com> References: <1440594483-29601-1-git-send-email-lee.jones@linaro.org> <1440594483-29601-5-git-send-email-lee.jones@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1440594483-29601-5-git-send-email-lee.jones@linaro.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Lee Jones Cc: ohad@wizery.com, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kernel@stlinux.com List-Id: devicetree@vger.kernel.org On 08/26/2015 08:08 AM, Lee Jones wrote: > Signed-off-by: Lee Jones > --- > drivers/remoteproc/remoteproc_debugfs.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) The commit message should describe why this is needed... > diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c > index 9d30809..9620962 100644 > --- a/drivers/remoteproc/remoteproc_debugfs.c > +++ b/drivers/remoteproc/remoteproc_debugfs.c > @@ -88,8 +88,33 @@ static ssize_t rproc_state_read(struct file *filp, char __user *userbuf, > return simple_read_from_buffer(userbuf, count, ppos, buf, i); > } > > +static ssize_t rproc_state_write(struct file *filp, const char __user *userbuf, > + size_t count, loff_t *ppos) > +{ > + struct rproc *rproc = filp->private_data; > + char buf[2]; > + int ret; > + > + ret = copy_from_user(buf, userbuf, 1); > + if (ret) > + return -EFAULT; > + > + switch (buf[0]) { > + case '1': > + ret = rproc_boot(rproc); > + if (ret) > + dev_warn(&rproc->dev, "Boot failed: %d\n", ret); > + break; > + default: > + rproc_shutdown(rproc); > + } > + > + return count; > +} ... and I suggest that the user interface be reconsidered. If '1' means "boot" and literally anything else means "shut down" then you can't add operations in the future without potentially breaking things. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755977AbbHZRKO (ORCPT ); Wed, 26 Aug 2015 13:10:14 -0400 Received: from relay1.mentorg.com ([192.94.38.131]:62700 "EHLO relay1.mentorg.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752525AbbHZRKM (ORCPT ); Wed, 26 Aug 2015 13:10:12 -0400 Subject: Re: [PATCH 4/4] remoteproc: debugfs: Add ability to boot remote processor using debugfs To: Lee Jones References: <1440594483-29601-1-git-send-email-lee.jones@linaro.org> <1440594483-29601-5-git-send-email-lee.jones@linaro.org> CC: , , , , From: Nathan Lynch Message-ID: <55DDF2A0.5010404@mentor.com> Date: Wed, 26 Aug 2015 12:08:48 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <1440594483-29601-5-git-send-email-lee.jones@linaro.org> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/26/2015 08:08 AM, Lee Jones wrote: > Signed-off-by: Lee Jones > --- > drivers/remoteproc/remoteproc_debugfs.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) The commit message should describe why this is needed... > diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c > index 9d30809..9620962 100644 > --- a/drivers/remoteproc/remoteproc_debugfs.c > +++ b/drivers/remoteproc/remoteproc_debugfs.c > @@ -88,8 +88,33 @@ static ssize_t rproc_state_read(struct file *filp, char __user *userbuf, > return simple_read_from_buffer(userbuf, count, ppos, buf, i); > } > > +static ssize_t rproc_state_write(struct file *filp, const char __user *userbuf, > + size_t count, loff_t *ppos) > +{ > + struct rproc *rproc = filp->private_data; > + char buf[2]; > + int ret; > + > + ret = copy_from_user(buf, userbuf, 1); > + if (ret) > + return -EFAULT; > + > + switch (buf[0]) { > + case '1': > + ret = rproc_boot(rproc); > + if (ret) > + dev_warn(&rproc->dev, "Boot failed: %d\n", ret); > + break; > + default: > + rproc_shutdown(rproc); > + } > + > + return count; > +} ... and I suggest that the user interface be reconsidered. If '1' means "boot" and literally anything else means "shut down" then you can't add operations in the future without potentially breaking things.