by Lina Iyer

[permalink] [raw]

Subject: Re: [PATCH v2 01/10] drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs

Thanks Evan for your review.

On Fri, Feb 16 2018 at 21:30 +0000, Evan Green wrote:
>Hi Lina,
>
>On Thu, Feb 15, 2018 at 9:34 AM, Lina Iyer <[email protected]> wrote:

>> +
>> +/**
>> + * tcs_response: Response object for a request
>
>Can you embed the acronym definition, ie: tcs_response: Responses for
>a Trigger Command Set.
>
Sure.

>> + *
>> + * @drv: the controller
>> + * @msg: the request for this response
>> + * @m: the tcs identifier
>> + * @err: error reported in the response
>> + * @list: link list object.
>> + */
>> +struct tcs_response {
>> + struct rsc_drv *drv;
>> + struct tcs_request *msg;
>> + u32 m;
>> + int err;
>> + struct list_head list;
>> +};
>> +
>> +/**
>> + * tcs_group: group of TCSes for a request state
>> + *
>
>Document @drv.
>
OK

>> + * @type: type of the TCS in this group - active, sleep, wake
>> + * @tcs_mask: mask of the TCSes relative to all the TCSes in the RSC
>> + * @tcs_offset: start of the TCS group relative to the TCSes in the RSC
>> + * @num_tcs: number of TCSes in this type
>> + * @ncpt: number of commands in each TCS
>> + * @tcs_lock: lock for synchronizing this TCS writes
>> + * @responses: response objects for requests sent from each TCS
>> + */
>> +struct tcs_group {
>> + struct rsc_drv *drv;
>> + int type;
>> + u32 tcs_mask;
>> + u32 tcs_offset;
>> + int num_tcs;
>> + int ncpt;
>> + spinlock_t tcs_lock;
>> + struct tcs_response *responses[MAX_TCS_PER_TYPE];
>> +};
>> +
>> +/**
>> + * rsc_drv: the RSC controller
>
>Would be more helpfully described as Resource State Coordinator controller.
>
OK

>> +static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int m, int n,
>> + u32 data)
>> +{
>> + write_tcs_reg(drv, reg, m, n, data);
>> + for (;;) {
>> + if (data == read_tcs_reg(drv, reg, m, n))
>> + break;
>> + udelay(1);
>
>Should this time out and return a failure?
>
There is no reason for this fail. We just need to ensure that it is
written. Sometimes writes going through the bus, takes time to complete
the write. When we exit this function, we are assured that it is
written.

>> + }
>> +}
>> +
>> +static bool tcs_is_free(struct rsc_drv *drv, int m)
>> +{
>> + return !atomic_read(&drv->tcs_in_use[m]) &&
>> + read_tcs_reg(drv, RSC_DRV_STATUS, m, 0);
>> +}
>> +
>> +static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
>> +{
>> + int i;
>> + struct tcs_group *tcs;
>> +
>> + for (i = 0; i < TCS_TYPE_NR; i++) {
>> + if (type == drv->tcs[i].type)
>> + break;
>> + }
>> +
>> + if (i == TCS_TYPE_NR)
>> + return ERR_PTR(-EINVAL);
>> +
>> + tcs = &drv->tcs[i];
>> + if (!tcs->num_tcs)
>> + return ERR_PTR(-EINVAL);
>> +
>> + return tcs;
>> +}
>> +
>> +static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
>> + struct tcs_request *msg)
>> +{
>> + int type;
>> +
>> + switch (msg->state) {
>> + case RPMH_ACTIVE_ONLY_STATE:
>> + type = ACTIVE_TCS;
>> + break;
>> + default:
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + return get_tcs_of_type(drv, type);
>> +}
>> +
>> +static void send_tcs_response(struct tcs_response *resp)
>> +{
>> + struct rsc_drv *drv = resp->drv;
>> + unsigned long flags;
>> +
>> + if (!resp)
>> + return;
>
>Does this ever happen? Ah, I see that it might in the irq handler. But
>get_response already assumes that there is a response, and reaches
>through it. So I don't think you need this here nor the check+label in
>the irq handler.
>
>Is requesting an index out of range purely a developer error, or could
>it happen in some sort of runtime scarcity situation? If it's a
>developer error, I'd get rid of all the null checking. If it's
>something that might really happen under the right circumstances, then
>my comment above doesn't stand and you'd want to fix the null
>dereference in get_response instead.
>
I added the check so that I dont have confusing goto in the IRQ handler.

>> +
>> + spin_lock_irqsave(&drv->drv_lock, flags);
>> + INIT_LIST_HEAD(&resp->list);
>> + list_add_tail(&resp->list, &drv->response_pending);
>> + spin_unlock_irqrestore(&drv->drv_lock, flags);
>> +
>> + tasklet_schedule(&drv->tasklet);
>> +}
>> +
>> +/**
>> + * tcs_irq_handler: TX Done interrupt handler
>> + */
>> +static irqreturn_t tcs_irq_handler(int irq, void *p)
>> +{
>> + struct rsc_drv *drv = p;
>> + int m, i;
>> + u32 irq_status, sts;
>> + struct tcs_response *resp;
>> + struct tcs_cmd *cmd;
>> + int err;
>> +
>> + irq_status = read_tcs_reg(drv, RSC_DRV_IRQ_STATUS, 0, 0);
>> +
>> + for (m = 0; m < drv->num_tcs; m++) {
>> + if (!(irq_status & (u32)BIT(m)))
>> + continue;
>> +
>> + err = 0;
>> + resp = get_response(drv, m);
>> + if (!resp) {
>
>I mention this above, but I don't think get_response can gracefully
>return null, so is this needed?
>
>> + WARN_ON(1);
>> + goto skip_resp;
>> + }
>> +
>> + for (i = 0; i < resp->msg->num_payload; i++) {
>> + cmd = &resp->msg->payload[i];
>> + sts = read_tcs_reg(drv, RSC_DRV_CMD_STATUS, m, i);
>> + if ((!(sts & CMD_STATUS_ISSUED)) ||
>> + ((resp->msg->is_complete || cmd->complete) &&
>> + (!(sts & CMD_STATUS_COMPL)))) {
>> + resp->err = -EIO;
>
>You're trying to set this to -EIO, but then you clobber it to err
>(which is zero) below
>
Good catch. Will fix.

>> + break;
>> + }
>> + }
>> +
>> + resp->err = err;
>> +skip_resp:
>> + /* Reclaim the TCS */
>> + write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, m, 0, 0);
>> + write_tcs_reg(drv, RSC_DRV_IRQ_CLEAR, 0, 0, BIT(m));
>> + atomic_set(&drv->tcs_in_use[m], 0);
>> + send_tcs_response(resp);
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>[...]
>> +
>> +static void __tcs_trigger(struct rsc_drv *drv, int m)
>> +{
>> + u32 enable;
>> +
>> + /*
>> + * HW req: Clear the DRV_CONTROL and enable TCS again
>> + * While clearing ensure that the AMC mode trigger is cleared
>> + * and then the mode enable is cleared.
>> + */
>> + enable = read_tcs_reg(drv, RSC_DRV_CONTROL, m, 0);
>> + enable &= ~TCS_AMC_MODE_TRIGGER;
>> + write_tcs_reg_sync(drv, RSC_DRV_CONTROL, m, 0, enable);
>
>If write_tcs_reg_sync can fail as I suggest, you'd have to fail out of here too.
>
Explained above.

>[...]
>> diff --git a/include/soc/qcom/tcs.h b/include/soc/qcom/tcs.h
>> new file mode 100644
>> index 000000000000..9465f0560f7a
>> --- /dev/null
>> +++ b/include/soc/qcom/tcs.h
>> @@ -0,0 +1,56 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#ifndef __SOC_QCOM_TCS_H__
>> +#define __SOC_QCOM_TCS_H__
>> +
>> +#define MAX_RPMH_PAYLOAD 16
>> +
>> +/**
>> + * rpmh_state: state for the request
>> + *
>> + * RPMH_WAKE_ONLY_STATE: Resume resource state to the value previously
>> + * requested before the processor was powered down.
>> + * RPMH_SLEEP_STATE: State of the resource when the processor subsystem
>> + * is powered down. There is no client using the
>> + * resource actively.
>> + * RPMH_ACTIVE_ONLY_STATE: Active or AMC mode requests. Resource state
>> + * is aggregated immediately.
>> + */
>> +enum rpmh_state {
>> + RPMH_SLEEP_STATE,
>> + RPMH_WAKE_ONLY_STATE,
>> + RPMH_ACTIVE_ONLY_STATE,
>> +};
>> +
>> +/**
>> + * tcs_cmd: an individual request to RPMH.
>> + *
>> + * @addr: the address of the resource slv_id:18:16 | offset:0:15
>> + * @data: the resource state request
>> + * @complete: wait for this request to be complete before sending the next
>> + */
>> +struct tcs_cmd {
>> + u32 addr;
>> + u32 data;
>> + bool complete;
>> +};
>> +
>> +/**
>> + * tcs_request: A set of tcs_cmds sent togther in a TCS
>> + *
>> + * @state: state for the request.
>> + * @is_complete: expect a response from the h/w accelerator
>> + * @num_payload: the number of tcs_cmds in thsi payload
>> + * @payload: an array of tcs_cmds
>> + */
>> +struct tcs_request {
>> + enum rpmh_state state;
>> + bool is_complete;
>
>is_complete is kind of a misleading name. I would expect this to
>indicate "the request is complete", not "expecting a completion
>interrupt". Maybe everybody knows this and I'm just the confused
>newbie.
>
It is used to distinguish between a request that is fire and forget or a
completion request where you are waiting for a response from the
accelerator. It might a be a bit of a misnomer though.

Thanks,
Lina

2018-02-18 11:29:47

Hi Lina,

On Thu, Feb 22, 2018 at 9:04 AM, Lina Iyer <[email protected]> wrote:
> On Wed, Feb 21 2018 at 23:25 +0000, Evan Green wrote:
>>
>> Hello Lina,
>>
>> On Thu, Feb 15, 2018 at 9:35 AM, Lina Iyer <[email protected]> wrote:
>>>
>>> Platform drivers need make a lot of resource state requests at the same
>>> time, say, at the start or end of an usecase. It can be quite
>>> inefficient to send each request separately. Instead they can give the
>>> RPMH library a batch of requests to be sent and wait on the whole
>>> transaction to be complete.
>>>
>>> rpmh_write_batch() is a blocking call that can be used to send multiple
>>> RPMH command sets. Each RPMH command set is set asynchronously and the
>>> API blocks until all the command sets are complete and receive their
>>> tx_done callbacks.
>>>
>>> Signed-off-by: Lina Iyer <[email protected]>
>>> ---
>>> drivers/soc/qcom/rpmh.c | 150
>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>> include/soc/qcom/rpmh.h | 8 +++
>>> 2 files changed, 158 insertions(+)
>>>
>>> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
>>> index dff4c46be3af..6f60bb9a4dfa 100644
>>> --- a/drivers/soc/qcom/rpmh.c
>>> +++ b/drivers/soc/qcom/rpmh.c
>>
>> [...]
>>>
>>> @@ -394,6 +537,11 @@ int rpmh_flush(struct rpmh_client *rc)
>>> }
>>> spin_unlock_irqrestore(&rpm->lock, flags);
>>>
>>> + /* First flush the cached batch requests */
>>> + ret = flush_batch(rc);
>>> + if (ret)
>>> + return ret;
>>> +
>>> /*
>>> * Nobody else should be calling this function other than system
>>> PM,,
>>> * hence we can run without locks.
>
> This comment and the comment in the header of this function.
>
>>> @@ -438,6 +586,8 @@ int rpmh_invalidate(struct rpmh_client *rc)
>>> if (IS_ERR_OR_NULL(rc))
>>> return -EINVAL;
>>>
>>> + invalidate_batch(rc);
>>> +
>>
>>
>> Similarly to my comments in patch 7, aren't there races here with
>> adding new elements? After flush_batch, but before invalidate_batch,
>> somebody could call cache_batch, which would add new things on the end
>> of the array. These new items would be clobbered by invalidate_batch,
>> without having been flushed first.
>>
> Flush is a system PM function and is not called by drivers and is not
> expected to run conncurrently with other threads using the RPMH library.

My comment above was a little off because I was reading those two
hunks (flush_batch and invalidate_batch) as being in the same
function. They're not.

I'm okay with the locking here, though you could remove the locking
from flush_batch, since that's only run in single-threaded PM
contexts.

>
> Thanks,
> Lina
>