Received: by 10.192.165.156 with SMTP id m28csp887910imm; Mon, 16 Apr 2018 10:16:33 -0700 (PDT) X-Google-Smtp-Source: AIpwx4/cdoN9Jbtu1y/WgpMuFb5bPbAFqOMsqE5zhC9fuc32q82t67UaNMie2k46n3bcGjlT1bbq X-Received: by 10.98.81.197 with SMTP id f188mr5760959pfb.136.1523898993396; Mon, 16 Apr 2018 10:16:33 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1523898993; cv=none; d=google.com; s=arc-20160816; b=dolNXQ9y2o7Vk/sg1A/M2XXxBwTrUJ/0EcISqWqunD+NAzApFt8PpFnL5XmiWgeKuz O4YfzEFe9bOuuv5iMOTBCkmrBe2ScVMd94//N+HpCMatBaUF2tgFJ0ji7XIVk74mqQed Ac3Q5+iTUzkNSYyuq38dN1YqL6oxWX2SWT2sNQTZ8+awsgLyDgtejREWCSzEZG4PfBhI iXbrwoa+LZ81FFdfqxBfFR3Jvkp2pS55Hst9N9E5ZYioyPF9Q66Fgvtpdt65WxEYYv4u hnfaSjfVsTQnUs9F0pE2axFIdNRfRVnFXLmnn9x0l1hngBwmLxqcNINiofvfqa6dFCer qa1A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature :arc-authentication-results; bh=hFdgRzEvKQuV9hixrzWly7Y4WiYUd9Ltq2arzSSoZtw=; b=l22VMO5GCHhBXmXU7AdP/e8IB2hDjGt30aknDMcMBn0BrfZOgX/qLWg7BhP8GtZ2kR RV3UIctnhzT3hwhf1em0wmfjCYstqEh+kQ/SxczxIGhJNwdPjwapOsNDHA9exzfQhOUL qJhkNxS21FejyX/2qrQS2xJGaJdWY0b952q5j9amshdd5cZMIWdNRxv34MMBbU6XMMbT AvGnM8SbbWnydqeecOkBptp/cd3R9Vwh1hIYQFQbv+i99QdmNzObCzCf1sMV0wTWrh+L fNsW5EyJ4+pWK3Nn2eO1qNHXVreqwRgkkuO5An9JnhBnHD9lfkpJTK9E2RVnh63UPu33 /WIw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=HY2pFBf8; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id t16si11183312pfj.10.2018.04.16.10.16.14; Mon, 16 Apr 2018 10:16:33 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=HY2pFBf8; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752802AbeDPRPA (ORCPT + 99 others); Mon, 16 Apr 2018 13:15:00 -0400 Received: from mail-oi0-f66.google.com ([209.85.218.66]:33556 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752308AbeDPRO6 (ORCPT ); Mon, 16 Apr 2018 13:14:58 -0400 Received: by mail-oi0-f66.google.com with SMTP id 126-v6so15246365oig.0 for ; Mon, 16 Apr 2018 10:14:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=hFdgRzEvKQuV9hixrzWly7Y4WiYUd9Ltq2arzSSoZtw=; b=HY2pFBf8qs4S4o9GDdFq+hhRd7P+hxxDdrgjBaAcS7bSqIYAsKWxIMu6ntscQi4Ocp RicsEq5w3ar6x22n1VeuA62r3gKZryFfwYp2W0z2WKaVdE8ZHuDNunyQVvphJLcZUYJp OBmghgwVrKgDNVS0zDoJpdxFu+hTUFb1803ro= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=hFdgRzEvKQuV9hixrzWly7Y4WiYUd9Ltq2arzSSoZtw=; b=jNnaQ5He3BA57J4KJz+n7tEvmhLJTKbchBOzZ3JXA97rqYwOuIkkckbZYxnEjaSPCa Rw6qKQnU2JYNgB+ThSymjLB4fXqHRNOA/1DV8v+GnezOLDjveXuTnkkgyikgQrVexSyA Zey32BEMWeRm62hUtfrpTusep4R3He0RzQGj5HDWcda2H/V202tDwkFiAH4iGD8BOZJb JLl9RBTeuEuMnX++I++RD7FTzKUa9mH+/mA7eeRHdrualkwi50V1kk+pbl1PxJ6UquJI J9J5WIF0oBrZgEqkO/E1ob20+CNcWqDRpha5G/ZfieW6f5xVva5ErZsuLBTF0DcE5N0+ oGEw== X-Gm-Message-State: ALQs6tAd1Bs+h/h/LXkc9b7mdMGBcn5KRiOWnXtB2FNSTifqXJsMcRi+ /4hR8Z/0mqyYsXMaYnXcwFOQqKjQknQ= X-Received: by 2002:aca:6bcd:: with SMTP id g196-v6mr16104380oic.268.1523898897430; Mon, 16 Apr 2018 10:14:57 -0700 (PDT) Received: from mail-oi0-f53.google.com (mail-oi0-f53.google.com. [209.85.218.53]) by smtp.gmail.com with ESMTPSA id s77-v6sm2730794oih.40.2018.04.16.10.14.56 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 16 Apr 2018 10:14:56 -0700 (PDT) Received: by mail-oi0-f53.google.com with SMTP id 126-v6so15246238oig.0 for ; Mon, 16 Apr 2018 10:14:56 -0700 (PDT) X-Received: by 2002:aca:4acc:: with SMTP id x195-v6mr14149531oia.11.1523898895405; Mon, 16 Apr 2018 10:14:55 -0700 (PDT) MIME-Version: 1.0 References: <1523390893-10904-1-git-send-email-rishabhb@codeaurora.org> <1523390893-10904-3-git-send-email-rishabhb@codeaurora.org> In-Reply-To: From: Evan Green Date: Mon, 16 Apr 2018 17:14:19 +0000 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v4 2/2] drivers: soc: Add LLCC driver To: rishabhb@codeaurora.org Cc: linux-arm-kernel@lists.infradead.org, linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, linux-arm@lists.infradead.org, linux-kernel@vger.kernel.org, tsoni@codeaurora.org, kyan@codeaurora.org, ckadabi@codeaurora.org, stanimir.varbanov@linaro.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Apr 13, 2018 at 4:08 PM wrote: > On 2018-04-12 15:02, Evan Green wrote: > > Hi Rishabh, > > > > On Tue, Apr 10, 2018 at 1:09 PM Rishabh Bhatnagar > > > > wrote: > > > >> LLCC (Last Level Cache Controller) provides additional cache memory > >> in the system. LLCC is partitioned into multiple slices and each > >> slice gets its own priority, size, ID and other config parameters. > >> LLCC driver programs these parameters for each slice. Clients that > >> are assigned to use LLCC need to get information such size & ID of the > >> slice they get and activate or deactivate the slice as needed. LLCC > >> driver > >> provides API for the clients to perform these operations. > > > >> Signed-off-by: Channagoud Kadabi > >> Signed-off-by: Rishabh Bhatnagar > >> --- > >> drivers/soc/qcom/Kconfig | 17 ++ > >> drivers/soc/qcom/Makefile | 2 + > >> drivers/soc/qcom/llcc-sdm845.c | 110 ++++++++++ > >> drivers/soc/qcom/llcc-slice.c | 404 > > +++++++++++++++++++++++++++++++++++++ > >> include/linux/soc/qcom/llcc-qcom.h | 168 +++++++++++++++ > >> 5 files changed, 701 insertions(+) > >> create mode 100644 drivers/soc/qcom/llcc-sdm845.c > >> create mode 100644 drivers/soc/qcom/llcc-slice.c > >> create mode 100644 include/linux/soc/qcom/llcc-qcom.h > > > > [...] > >> diff --git a/drivers/soc/qcom/llcc-sdm845.c > > b/drivers/soc/qcom/llcc-sdm845.c > >> new file mode 100644 > >> index 0000000..619b226 > >> --- /dev/null > >> +++ b/drivers/soc/qcom/llcc-sdm845.c > >> @@ -0,0 +1,110 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> +/* > >> + * Copyright (c) 2017-2018, The Linux Foundation. All rights > >> reserved. > >> + * > >> + */ > >> + > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> + > >> +/* > >> + * SCT(System Cache Table) entry contains of the following parameters > > > > contains the following members: > > > >> + * name: Name of the client's use case for which the llcc slice is > >> used > >> + * uid: Unique id for the client's use case > > > > s/uid/usecase_id/ > > > >> + * slice_id: llcc slice id for each client > >> + * max_cap: The maximum capacity of the cache slice provided in KB > >> + * priority: Priority of the client used to select victim line for > > replacement > >> + * fixed_size: Determine if the slice has a fixed capacity > > > > "Boolean indicating if the slice has a fixed capacity" might be better > > > >> diff --git a/drivers/soc/qcom/llcc-slice.c > >> b/drivers/soc/qcom/llcc-slice.c > >> new file mode 100644 > >> index 0000000..67a81b0 > >> --- /dev/null > >> +++ b/drivers/soc/qcom/llcc-slice.c > > ... > >> +static int llcc_update_act_ctrl(struct llcc_drv_data *drv, u32 sid, > >> + u32 act_ctrl_reg_val, u32 status) > >> +{ > >> + u32 act_ctrl_reg; > >> + u32 status_reg; > >> + u32 slice_status; > >> + int ret = 0; > >> + > >> + act_ctrl_reg = drv->bcast_off + LLCC_TRP_ACT_CTRLn(sid); > >> + status_reg = drv->bcast_off + LLCC_TRP_STATUSn(sid); > >> + > >> + /*Set the ACTIVE trigger*/ > > > > Add spaces around /* */ > > > >> + act_ctrl_reg_val |= ACT_CTRL_ACT_TRIG; > >> + ret = regmap_write(drv->regmap, act_ctrl_reg, > >> act_ctrl_reg_val); > >> + if (ret) > >> + return ret; > >> + > >> + /* Clear the ACTIVE trigger */ > >> + act_ctrl_reg_val &= ~ACT_CTRL_ACT_TRIG; > >> + ret = regmap_write(drv->regmap, act_ctrl_reg, > >> act_ctrl_reg_val); > >> + if (ret) > >> + return ret; > >> + > >> + ret = regmap_read_poll_timeout(drv->regmap, status_reg, > > slice_status, > >> + !(slice_status & status), 0, > > LLCC_STATUS_READ_DELAY); > >> + return ret; > >> +} > >> + > >> +/** > >> + * llcc_slice_activate - Activate the llcc slice > >> + * @desc: Pointer to llcc slice descriptor > >> + * > >> + * A value zero will be returned on success and a negative errno will > > > > a value of zero > > > >> + * be returned in error cases > >> + */ > >> +int llcc_slice_activate(struct llcc_slice_desc *desc) > >> +{ > >> + int ret; > >> + u32 act_ctrl_val; > >> + struct llcc_drv_data *drv; > >> + > >> + if (desc == NULL) > >> + return -EINVAL; > > > > I think we can remove this check, right? > > > >> + > >> + drv = dev_get_drvdata(desc->dev); > >> + if (!drv) > >> + return -EINVAL; > >> + > >> + mutex_lock(&drv->lock); > >> + if (test_bit(desc->slice_id, drv->bitmap)) { > >> + mutex_unlock(&drv->lock); > >> + return 0; > >> + } > >> + > >> + act_ctrl_val = ACT_CTRL_OPCODE_ACTIVATE << > >> ACT_CTRL_OPCODE_SHIFT; > >> + > >> + ret = llcc_update_act_ctrl(drv, desc->slice_id, act_ctrl_val, > >> + DEACTIVATE); > >> + > >> + __set_bit(desc->slice_id, drv->bitmap); > >> + mutex_unlock(&drv->lock); > >> + > >> + return ret; > >> +} > >> +EXPORT_SYMBOL_GPL(llcc_slice_activate); > >> + > >> +/** > >> + * llcc_slice_deactivate - Deactivate the llcc slice > >> + * @desc: Pointer to llcc slice descriptor > >> + * > >> + * A value zero will be returned on success and a negative errno will > >> + * be returned in error cases > >> + */ > >> +int llcc_slice_deactivate(struct llcc_slice_desc *desc) > >> +{ > >> + u32 act_ctrl_val; > >> + int ret; > >> + struct llcc_drv_data *drv; > >> + > >> + if (desc == NULL) > >> + return -EINVAL; > >> + > >> + drv = dev_get_drvdata(desc->dev); > >> + if (!drv) > >> + return -EINVAL; > >> + > >> + mutex_lock(&drv->lock); > >> + if (!test_bit(desc->slice_id, drv->bitmap)) { > >> + mutex_unlock(&drv->lock); > >> + return 0; > >> + } > >> + act_ctrl_val = ACT_CTRL_OPCODE_DEACTIVATE << > > ACT_CTRL_OPCODE_SHIFT; > >> + > >> + ret = llcc_update_act_ctrl(drv, desc->slice_id, act_ctrl_val, > >> + ACTIVATE); > >> + > >> + __clear_bit(desc->slice_id, drv->bitmap); > >> + mutex_unlock(&drv->lock); > >> + > >> + return ret; > >> +} > >> +EXPORT_SYMBOL_GPL(llcc_slice_deactivate); > >> + > >> +/** > >> + * llcc_get_slice_id - return the slice id > >> + * @desc: Pointer to llcc slice descriptor > >> + * > >> + * A positive value will be returned on success and a negative errno > >> will > >> + * be returned on error > >> + */ > >> +int llcc_get_slice_id(struct llcc_slice_desc *desc) > >> +{ > >> + if (!desc) > >> + return -EINVAL; > > > > Can we remove this check too? > > > We need this check and the following one to protect against the > null pointer access which might happen if client doesn't get > the descriptor before accessing size and slice_id. Is this just to protect against errors made during development, or is there an expected code path through which this might actually happen? If it's just to protect against developer error, then based on other feedback I've seen on these lists, the null pointer dereference is preferred, and we should remove this check. If it's an actual expected flow, then I guess clients will call llcc_slice_getd(), see the ERR_PTR(-ENOENT), transform that into NULL, and then call these other functions anyway? -Evan