| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <af00da0284e32d3698db855edbf74e45@codeaurora.org>
Date: Mon, 16 Apr 2018 13:50:58 -0700
From: rishabhb@...eaurora.org
To: Evan Green <evgreen@...omium.org>
Cc: linux-arm-kernel@...ts.infradead.org,
linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
linux-arm@...ts.infradead.org, linux-kernel@...r.kernel.org,
tsoni@...eaurora.org, kyan@...eaurora.org, ckadabi@...eaurora.org,
stanimir.varbanov@...aro.org
Subject: Re: [PATCH v4 2/2] drivers: soc: Add LLCC driver
On 2018-04-16 10:14, Evan Green wrote:
> On Fri, Apr 13, 2018 at 4:08 PM <rishabhb@...eaurora.org> wrote:
>
>> On 2018-04-12 15:02, Evan Green wrote:
>> > Hi Rishabh,
>> >
>> > On Tue, Apr 10, 2018 at 1:09 PM Rishabh Bhatnagar
>> > <rishabhb@...eaurora.org>
>> > 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 <ckadabi@...eaurora.org>
>> >> Signed-off-by: Rishabh Bhatnagar <rishabhb@...eaurora.org>
>> >> ---
>> >> 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 <linux/kernel.h>
>> >> +#include <linux/module.h>
>> >> +#include <linux/of.h>
>> >> +#include <linux/of_device.h>
>> >> +#include <linux/soc/qcom/llcc-qcom.h>
>> >> +
>> >> +/*
>> >> + * 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?
>
Yes, this is just to protect against development error. I will remove
the
check here and above.
-Rishabh
Powered by blists - more mailing lists