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 X-Spam-Level: X-Spam-Status: No, score=-3.9 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D45F5C2D0DB for ; Wed, 22 Jan 2020 19:20:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id AC50C21835 for ; Wed, 22 Jan 2020 19:20:21 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="TwoKubR7" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726442AbgAVTUV (ORCPT ); Wed, 22 Jan 2020 14:20:21 -0500 Received: from mail-pl1-f195.google.com ([209.85.214.195]:41683 "EHLO mail-pl1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725933AbgAVTUU (ORCPT ); Wed, 22 Jan 2020 14:20:20 -0500 Received: by mail-pl1-f195.google.com with SMTP id t14so187031plr.8 for ; Wed, 22 Jan 2020 11:20:20 -0800 (PST) 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=mpnp2dBUwgiSXJqBkhUdFPkzPC82dAX2Dn/bzDab9OI=; b=TwoKubR7jNDC4FclY67lUleRt7BkY0+OB6iyNEzor/uNZSPiIrXIXbVINxc/qzjB2/ orFLFxkbQBij/x+q2bSzp8dFpk6zF/y2rOm3kjZKMPKMe9foOSjjaogA/6DtOoLLoPDo CfJcIuhiXScG3eQdaxDxRwz8XaAJgBAUkgMuvw3WKcPAzFEzATmkX+2hI57agzEyDhYe Zl7X2nlFkxbyzB3rhlRCBTg24HqN6LGz740918w1ctTE460zXSILedH6AE2gmqgvj0md 42Ihe9mcJaSvEd0ZCW01Xs7bOgTgfuvM03QU36WVyywF3QvGmk6wgRAZsqc98UC63LXM d2iA== 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=mpnp2dBUwgiSXJqBkhUdFPkzPC82dAX2Dn/bzDab9OI=; b=dmotcOVMVv00Ak52VsrfI3UoMjz8MQNuCDjk+/zXChP3ugI0pW1IubNHPvzZ+5L3xE 99uLJ2CF3Ynb0IJYqgXF1Lzt5hrKnozy5HG3JVCoRuDDOM1SFsMIcuU6CYV/67IU7hfm sTbCxV233gkkngmn2E9Ebw3204Uwm22B3irEEEFk/Qtl42iUaWUlLBX5wnE+qTLZNV1E pHAHnCKwGZdzwD1qSIkfE/e/llyrI5NJZXn9UCIeocdrvfUrkRcTjkuRYuHfww3dTBab ikVIAA/B3VX6mdHDRos7Mar0BauqEq2LuKvRHN/Ih8G3brjlxw6Kn1y5759GTJxYQs1/ eQPA== X-Gm-Message-State: APjAAAVIsnobQEpqbcl/04xZSjS8jmsaXD/i4gwFhvkBB6ixLms1Rnry Ay0YxczQEWZI9aW0UI10YtJBkg== X-Google-Smtp-Source: APXvYqxspwypdYRSl7cOxJoNSmsNgl+draPEhXEg+oHAk4xjoVtPB+jwjiWlRB4uz0kbtwFrb1BI9Q== X-Received: by 2002:a17:902:7291:: with SMTP id d17mr12134051pll.227.1579720820090; Wed, 22 Jan 2020 11:20:20 -0800 (PST) Received: from ripper (104-188-17-28.lightspeed.sndgca.sbcglobal.net. [104.188.17.28]) by smtp.gmail.com with ESMTPSA id a15sm49191872pfh.169.2020.01.22.11.20.18 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 22 Jan 2020 11:20:19 -0800 (PST) Date: Wed, 22 Jan 2020 11:19:46 -0800 From: Bjorn Andersson To: Mathieu Poirier Cc: Rob Herring , Mark Rutland , Ohad Ben-Cohen , linux-arm-msm , devicetree@vger.kernel.org, Linux Kernel Mailing List , linux-remoteproc , Sibi Sankar , Rishabh Bhatnagar Subject: Re: [PATCH v2 2/8] remoteproc: qcom: Introduce driver to store pil info in IMEM Message-ID: <20200122191946.GA3261042@ripper> References: <20191227053215.423811-1-bjorn.andersson@linaro.org> <20191227053215.423811-3-bjorn.andersson@linaro.org> <20200110211846.GA11555@xps15> <20200122020234.GT1511@yoga> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: devicetree-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On Wed 22 Jan 11:04 PST 2020, Mathieu Poirier wrote: > On Tue, 21 Jan 2020 at 19:02, Bjorn Andersson > wrote: > > > > On Fri 10 Jan 13:18 PST 2020, Mathieu Poirier wrote: > > > On Thu, Dec 26, 2019 at 09:32:09PM -0800, Bjorn Andersson wrote: > > [..] > > > > diff --git a/drivers/remoteproc/qcom_pil_info.c b/drivers/remoteproc/qcom_pil_info.c > > > > new file mode 100644 > > > > index 000000000000..b0897ae9eae5 > > > > --- /dev/null > > > > +++ b/drivers/remoteproc/qcom_pil_info.c > > > > @@ -0,0 +1,150 @@ > > > > +// SPDX-License-Identifier: GPL-2.0-only > > > > +/* > > > > + * Copyright (c) 2019 Linaro Ltd. > > > > + */ > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > > > These should be in alphabetical order if there is no depencencies > > > between them, something checkpatch complains about. > > > > > > > Of course. > > > > > > + > > > > +struct pil_reloc_entry { > > > > + char name[8]; > > > > > > Please add a #define for the name length and reuse it in qcom_pil_info_store() > > > > > > > Ok > > > > [..] > > > > +void qcom_pil_info_store(const char *image, phys_addr_t base, size_t size) > > > > +{ > > > > + struct pil_reloc_entry *entry; > > > > + int idx = -1; > > > > + int i; > > > > + > > > > + mutex_lock(&reloc_mutex); > > > > + if (!_reloc) > > > > > > Since it is available, I would use function qcom_pil_info_available(). Also > > > checkpatch complains about indentation problems related to the 'if' condition > > > but I can't see what makes it angry. > > > > > > > Sure thing, and I'll double check the indentation. > > > > > > + goto unlock; > > > > + > > > > + for (i = 0; i < PIL_INFO_ENTRIES; i++) { > > > > + if (!_reloc->entries[i].name[0]) { > > > > + if (idx == -1) > > > > + idx = i; > > > > + continue; > > > > + } > > > > + > > > > + if (!strncmp(_reloc->entries[i].name, image, 8)) { > > > > + idx = i; > > > > + goto found; > > > > + } > > > > + } > > > > + > > > > + if (idx == -1) { > > > > + dev_warn(_reloc->dev, "insufficient PIL info slots\n"); > > > > + goto unlock; > > > > > > Given how this function is used in the next patch I think an error should be > > > reported to the caller. > > > > > > > Just to clarify, certain global errors will cause the entire device to > > be reset and allow memory contents to be extracted for analysis in post > > mortem tools. This patch ensures that this information contains > > (structured) information about where each remote processor is loaded. > > Afaict the purpose of propagating errors from this function would be for > > the caller to abort the launching of a remote processor. > > > > I think it's better to take the risk of having insufficient data for the > > post mortem tools than to fail booting a remote processor for a reason > > that won't affect normal operation. > > I understand the reasoning. In that case it is probably best to let > the caller decide what to do with the returned error than deal with it > locally, especially since this is an exported function. When using > qcom_pil_info_store(), I would write a comment that justifies the > reason for ignoring the return value (what you have above is quite > good). Otherwise it is just a matter of time before automated tools > pickup on the anomaly and send patches to fix it. > You're right, moving the decision to the remoteproc drivers will result in the decision being implemented in the right place. I will respin it accordingly. Thanks! Bjorn