| 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
| ||
|
Date: Fri, 27 Apr 2018 10:55:36 -0600
From: Lina Iyer <ilina@...eaurora.org>
To: Matthias Kaehlcke <mka@...omium.org>
Cc: andy.gross@...aro.org, david.brown@...aro.org,
linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
rnayak@...eaurora.org, bjorn.andersson@...aro.org,
linux-kernel@...r.kernel.org, sboyd@...nel.org,
evgreen@...omium.org, dianders@...omium.org
Subject: Re: [PATCH v6 04/10] drivers: qcom: rpmh: add RPMH helper functions
On Thu, Apr 26 2018 at 12:05 -0600, Matthias Kaehlcke wrote:
>Hi Lina,
>
>On Thu, Apr 19, 2018 at 04:16:29PM -0600, Lina Iyer wrote:
>> Sending RPMH requests and waiting for response from the controller
>> through a callback is common functionality across all platform drivers.
>> To simplify drivers, add a library functions to create RPMH client and
>> send resource state requests.
>>
>> rpmh_write() is a synchronous blocking call that can be used to send
>> active state requests.
>>
>> Signed-off-by: Lina Iyer <ilina@...eaurora.org>
>> ---
>>
>> Changes in v6:
>> - replace rpmh_client with device
>> - inline wait_for_tx_done()
>>
>> Changes in v4:
>> - use const struct tcs_cmd in API
>> - remove wait count from this patch
>> - changed -EFAULT to -EINVAL
>> ---
>> drivers/soc/qcom/Makefile | 4 +-
>> drivers/soc/qcom/rpmh-internal.h | 6 ++
>> drivers/soc/qcom/rpmh-rsc.c | 8 ++
>> drivers/soc/qcom/rpmh.c | 168 +++++++++++++++++++++++++++++++
>> include/soc/qcom/rpmh.h | 25 +++++
>> 5 files changed, 210 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/soc/qcom/rpmh.c
>> create mode 100644 include/soc/qcom/rpmh.h
>>
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index cb6300f6a8e9..bb395c3202ca 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -7,7 +7,9 @@ obj-$(CONFIG_QCOM_PM) += spm.o
>> obj-$(CONFIG_QCOM_QMI_HELPERS) += qmi_helpers.o
>> qmi_helpers-y += qmi_encdec.o qmi_interface.o
>> obj-$(CONFIG_QCOM_RMTFS_MEM) += rmtfs_mem.o
>> -obj-$(CONFIG_QCOM_RPMH) += rpmh-rsc.o
>> +obj-$(CONFIG_QCOM_RPMH) += qcom_rpmh.o
>> +qcom_rpmh-y += rpmh-rsc.o
>> +qcom_rpmh-y += rpmh.o
>> obj-$(CONFIG_QCOM_SMD_RPM) += smd-rpm.o
>> obj-$(CONFIG_QCOM_SMEM) += smem.o
>> obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
>> diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
>> index cc29176f1303..d9a21726e568 100644
>> --- a/drivers/soc/qcom/rpmh-internal.h
>> +++ b/drivers/soc/qcom/rpmh-internal.h
>> @@ -14,6 +14,7 @@
>> #define MAX_CMDS_PER_TCS 16
>> #define MAX_TCS_PER_TYPE 3
>> #define MAX_TCS_NR (MAX_TCS_PER_TYPE * TCS_TYPE_NR)
>> +#define RPMH_MAX_CTRLR 2
>>
>> struct rsc_drv;
>>
>> @@ -52,6 +53,7 @@ struct tcs_group {
>> * @tcs: TCS groups
>> * @tcs_in_use: s/w state of the TCS
>> * @lock: synchronize state of the controller
>> + * @list: element in list of drv
>> */
>> struct rsc_drv {
>> const char *name;
>> @@ -61,9 +63,13 @@ struct rsc_drv {
>> struct tcs_group tcs[TCS_TYPE_NR];
>> DECLARE_BITMAP(tcs_in_use, MAX_TCS_NR);
>> spinlock_t lock;
>> + struct list_head list;
>> };
>>
>> +extern struct list_head rsc_drv_list;
>>
>> int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg);
>>
>> +void rpmh_tx_done(const struct tcs_request *msg, int r);
> ~
>
>nit: consider using a more expressive name, like status, rc, res or
>err.
>
OK. Will fix.
><snip>
>
>> +static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
>> +{
>> + int i;
>> + struct rsc_drv *p, *drv = dev_get_drvdata(dev->parent);
>> + struct rpmh_ctrlr *ctrlr = ERR_PTR(-EINVAL);
>> +
>> + if (!drv)
>> + return ctrlr;
>> +
>> + for (i = 0; i < RPMH_MAX_CTRLR; i++) {
>> + if (rpmh_rsc[i].drv == drv) {
>> + ctrlr = &rpmh_rsc[i];
>> + return ctrlr;
>> + }
>> + }
>> +
>> + list_for_each_entry(p, &rsc_drv_list, list) {
>> + if (drv == p) {
>> + for (i = 0; i < RPMH_MAX_CTRLR; i++) {
>> + if (!rpmh_rsc[i].drv)
>> + break;
>> + }
>> + rpmh_rsc[i].drv = drv;
>
>There is a race condition, get_rpmh_ctrlr() could be executed
>simulatenously in different contexts, and select the same
>controller instance for different DRVs.
>
True. This will need to be serialized.
>It's probably an unlikely case, but to be safe you'd have to do
>something like this:
>
>retry:
>for (i = 0; i < RPMH_MAX_CTRLR; i++) {
> if (!rpmh_rsc[i].drv)
> break;
>}
>
>spin_lock(&rpmh_rsc[i].lock);
>
>if (!rpmh_rsc[i].drv) {
> rpmh_rsc[i].drv = drv;
> ctrlr = &rpmh_rsc[i];
>} else {
> spin_unlock(&rpmh_rsc[i].lock);
> goto retry;
>}
>
>spin_unlock(&rpmh_rsc[i].lock);
>
>
>The above code doesn't address another potential error case, where
>#DRV > RPMH_MAX_CTRLR. In this case we'd access memory beyond
>rpmh_rsc. This would be a configuration error, not sure if it's
>strictly necessary to handle it.
>
I think I might be able to solve for this a bit simpler with another
spinlock for the rpmh_rsc.
><snip>
>
>> +/**
>> + * rpmh_write: Write a set of RPMH commands and block until response
>> + *
>> + * @rc: The RPMh handle got from rpmh_get_client
> ~~~~
>
>nit: Other than this the driver consistently uses 'RPMH'
>
OK.
Thanks for the review Matthias.
-- Lina
Powered by blists - more mailing lists