Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932339AbbH0Gq1 (ORCPT ); Thu, 27 Aug 2015 02:46:27 -0400 Received: from mail-wi0-f176.google.com ([209.85.212.176]:38277 "EHLO mail-wi0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753549AbbH0Go4 (ORCPT ); Thu, 27 Aug 2015 02:44:56 -0400 Date: Thu, 27 Aug 2015 07:44:52 +0100 From: Lee Jones To: Nathan Lynch Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, ohad@wizery.com, devicetree@vger.kernel.org, kernel@stlinux.com Subject: Re: [PATCH 4/4] remoteproc: debugfs: Add ability to boot remote processor using debugfs Message-ID: <20150827064452.GC19409@x1> References: <1440594483-29601-1-git-send-email-lee.jones@linaro.org> <1440594483-29601-5-git-send-email-lee.jones@linaro.org> <55DDF2A0.5010404@mentor.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <55DDF2A0.5010404@mentor.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1920 Lines: 58 On Wed, 26 Aug 2015, Nathan Lynch wrote: > 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. Good points, will fix. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/