Re: [PATCH v5 01/10] drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs
From: Lina Iyer
Date: Fri Apr 13 2018 - 12:17:03 EST
On Tue, Apr 10 2018 at 17:40 -0600, Bjorn Andersson wrote:
On Thu 05 Apr 09:18 PDT 2018, Lina Iyer wrote:
[..]
diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
[..]
+/**
+ * struct tcs_response: Response object for a request
+ *
+ * @drv: the controller
+ * @msg: the request for this response
+ * @m: the tcs identifier
+ * @err: error reported in the response
+ * @list: element in list of pending response objects
+ */
+struct tcs_response {
+ struct rsc_drv *drv;
+ const struct tcs_request *msg;
+ u32 m;
m is assigned in one place but never used.
Right. Remnant from the downstream driver that uses buffers of
responses.
+ int err;
+ struct list_head list;
+};
[..]
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
[..]
+static struct tcs_group *get_tcs_from_index(struct rsc_drv *drv, int m)
+{
+ struct tcs_group *tcs;
+ int i;
+
+ for (i = 0; i < drv->num_tcs; i++) {
+ tcs = &drv->tcs[i];
+ if (tcs->mask & BIT(m))
+ return tcs;
+ }
+
+ WARN(i == drv->num_tcs, "Incorrect TCS index %d", m);
+
+ return NULL;
+}
+
+static struct tcs_response *setup_response(struct rsc_drv *drv,
+ const struct tcs_request *msg, int m)
+{
+ struct tcs_response *resp;
+ struct tcs_group *tcs;
+
+ resp = kzalloc(sizeof(*resp), GFP_ATOMIC);
I still don't like the idea that you allocate a response struct for each
request, then upon getting an ack post this on a list and schedule a
tasklet in order to optionally deliver the return value to the waiting
caller.
Why don't you just just add the "err" and a completion to the
tcs_request struct and if it's a sync operation you complete that in
your irq handler?
That would remove the response struct, the list of them, the tasklet and
the dynamic memory handling - at the "cost" of making the code possible
to follow.
Hmm.. Ok. Will try to simplify.
+ if (!resp)
+ return ERR_PTR(-ENOMEM);
+
+ resp->drv = drv;
+ resp->msg = msg;
+ resp->err = 0;
+
+ tcs = get_tcs_from_index(drv, m);
+ if (!tcs)
+ return ERR_PTR(-EINVAL);
+
+ assert_spin_locked(&tcs->lock);
I tried to boot the kernel with the rpmh-clk and rpmh-regulator drivers
and I kept hitting this assert.
Turns out that find_free_tcs() finds an empty TCS with index 'm' within
the tcs, then passes it to setup_response() which tries to use the 'm'
to figure out which tcs contains the TCS we're operating on.
But as 'm' is in tcs-local space and get_tcs_from_index() tries to
lookup the TCS in the global drv space we get hold of the wrong TCS.
You are right. I will fix it. Thanks for point out. Wonder what is in
your DT, that caused this to be triggered. Clearly it's missing my
setup.
+ tcs->responses[m - tcs->offset] = resp;
+
+ return resp;
+}
+
+static void free_response(struct tcs_response *resp)
+{
+ kfree(resp);
+}
+
+static struct tcs_response *get_response(struct rsc_drv *drv, u32 m)
+{
+ struct tcs_group *tcs = get_tcs_from_index(drv, m);
+
+ return tcs->responses[m - tcs->offset];
+}
+
+static u32 read_tcs_reg(struct rsc_drv *drv, int reg, int m, int n)
+{
+ return readl_relaxed(drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * m +
+ RSC_DRV_CMD_OFFSET * n);
+}
+
+static void write_tcs_reg(struct rsc_drv *drv, int reg, int m, int n, u32 data)
+{
+ writel_relaxed(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * m +
+ RSC_DRV_CMD_OFFSET * n);
Do you really want this relaxed? Isn't the ordering of these
significant?
The ordering isnt. I can make it not relaxed. Only ordering requirement
is that we tigger after writing everything.
+}
+
+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);
+ }
+}
+
+static bool tcs_is_free(struct rsc_drv *drv, int m)
+{
+ return !test_bit(m, drv->tcs_in_use) &&
+ read_tcs_reg(drv, RSC_DRV_STATUS, m, 0);
+}
+
+static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
According to rpmh_rsc_probe() the tcs array is indexed by "type", so you
can replace the entire function with:
return &drv->tcs[type];
Hmm. Ok.
+{
+ 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,
+ const 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;
+ unsigned long flags;
+
+ if (!resp)
+ return;
+
+ drv = resp->drv;
+ 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;
+
+ 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;
+
+ resp = get_response(drv, m);
+ if (WARN_ON(!resp))
This will only ever fail in the beginning of time, as soon as you've
utilized every TCS at least once resp will never be NULL, as you never
clear it.
+ goto skip_resp;
+
+ resp->err = 0;
+ for (i = 0; i < resp->msg->num_cmds; i++) {
+ cmd = &resp->msg->cmds[i];
+ sts = read_tcs_reg(drv, RSC_DRV_CMD_STATUS, m, i);
+ if (!(sts & CMD_STATUS_ISSUED) ||
+ ((resp->msg->wait_for_compl || cmd->wait) &&
+ !(sts & CMD_STATUS_COMPL))) {
+ resp->err = -EIO;
+ break;
+ }
+ }
+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));
+ clear_bit(m, drv->tcs_in_use);
+ send_tcs_response(resp);
As I suggested above, rather than putting resp on a list and schedule a
tasklet to free and possibly deliver the value or "err" to a client just
keep track of the current msg for the TCS for sync operations, set "err"
and fire the completion (and then untie the request from the TCS).
Ok.
+ }
+
+ return IRQ_HANDLED;
+}
+
+/**
+ * tcs_notify_tx_done: TX Done for requests that got a response
+ *
+ * @data: the tasklet argument
+ *
+ * Tasklet function to notify MBOX that we are done with the request.
+ * Handles all pending reponses whenever run.
This is accidental complexity from the downstream use of the mailbox
framework, we don't need it.
Yes. will remove this.
+ */
+static void tcs_notify_tx_done(unsigned long data)
+{
+ struct rsc_drv *drv = (struct rsc_drv *)data;
+ struct tcs_response *resp;
+ unsigned long flags;
+
+ for (;;) {
+ spin_lock_irqsave(&drv->drv_lock, flags);
+ resp = list_first_entry_or_null(&drv->response_pending,
+ struct tcs_response, list);
+ if (!resp) {
+ spin_unlock_irqrestore(&drv->drv_lock, flags);
+ break;
+ }
+ list_del(&resp->list);
+ spin_unlock_irqrestore(&drv->drv_lock, flags);
+ free_response(resp);
+ }
+}
+
+static void __tcs_buffer_write(struct rsc_drv *drv, int m, int n,
+ const struct tcs_request *msg)
+{
+ u32 msgid, cmd_msgid;
+ u32 cmd_enable = 0;
+ u32 cmd_complete;
+ struct tcs_cmd *cmd;
+ int i, j;
+
+ cmd_msgid = CMD_MSGID_LEN;
+ cmd_msgid |= msg->wait_for_compl ? CMD_MSGID_RESP_REQ : 0;
+ cmd_msgid |= CMD_MSGID_WRITE;
+
+ cmd_complete = read_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
+
+ for (i = 0, j = n; i < msg->num_cmds; i++, j++) {
+ cmd = &msg->cmds[i];
+ cmd_enable |= BIT(j);
+ cmd_complete |= cmd->wait << j;
+ msgid = cmd_msgid;
+ msgid |= cmd->wait ? CMD_MSGID_RESP_REQ : 0;
+ write_tcs_reg(drv, RSC_DRV_CMD_MSGID, m, j, msgid);
+ write_tcs_reg(drv, RSC_DRV_CMD_ADDR, m, j, cmd->addr);
+ write_tcs_reg(drv, RSC_DRV_CMD_DATA, m, j, cmd->data);
+ }
+
+ write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0, cmd_complete);
+ cmd_enable |= read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, m, 0);
+ write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, m, 0, cmd_enable);
+}
+
+static void __tcs_trigger(struct rsc_drv *drv, int m)
+{
+ u32 enable;
"enable"?
Sorry?
+
+ /*
+ * 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);
+ enable &= ~TCS_AMC_MODE_ENABLE;
+ write_tcs_reg_sync(drv, RSC_DRV_CONTROL, m, 0, enable);
+
+ /* Enable the AMC mode on the TCS and then trigger the TCS */
+ enable = TCS_AMC_MODE_ENABLE;
+ write_tcs_reg_sync(drv, RSC_DRV_CONTROL, m, 0, enable);
+ enable |= TCS_AMC_MODE_TRIGGER;
+ write_tcs_reg_sync(drv, RSC_DRV_CONTROL, m, 0, enable);
+}
+
+static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
+ const struct tcs_request *msg)
+{
+ unsigned long curr_enabled;
+ u32 addr;
+ int i, j, k;
+ int m = tcs->offset;
+
+ for (i = 0; i < tcs->num_tcs; i++, m++) {
+ if (tcs_is_free(drv, m))
+ continue;
+
+ curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, m, 0);
+
+ for_each_set_bit(j, &curr_enabled, MAX_CMDS_PER_TCS) {
+ addr = read_tcs_reg(drv, RSC_DRV_CMD_ADDR, m, j);
+ for (k = 0; k < msg->num_cmds; k++) {
+ if (addr == msg->cmds[k].addr)
+ return -EBUSY;
+ }
+ }
+ }
+
+ return 0;
+}
+
+static int find_free_tcs(struct tcs_group *tcs)
+{
+ int m;
+
+ for (m = 0; m < tcs->num_tcs; m++) {
+ if (tcs_is_free(tcs->drv, tcs->offset + m))
+ return m;
The returned index is within the tcs but is passed to setup_response()
where it's used as the index of the TCS, so this needs to return
tcs->offset + m so that setup_response() will be able to find the tcs
again.
Correct. Will fix.
+ }
+
+ return -EBUSY;
+}
+
+static int tcs_mbox_write(struct rsc_drv *drv, const struct tcs_request *msg)
+{
+ struct tcs_group *tcs;
+ int m;
+ struct tcs_response *resp = NULL;
No need to initialize resp.
Ok.
+ unsigned long flags;
+ int ret;
+
+ tcs = get_tcs_for_msg(drv, msg);
+ if (IS_ERR(tcs))
+ return PTR_ERR(tcs);
+
+ spin_lock_irqsave(&tcs->lock, flags);
+ m = find_free_tcs(tcs);
+ if (m < 0) {
+ ret = m;
+ goto done_write;
+ }
+
+ /*
+ * The h/w does not like if we send a request to the same address,
+ * when one is already in-flight or being processed.
+ */
+ ret = check_for_req_inflight(drv, tcs, msg);
This scans all TCS in the DRV for any operations on msg->cmds[*].addr,
but you're only holding a lock for tcs. Either cross-tcs operations
doesn't matter and check_for_req_inflight() can loose one of the loops
or the locking used here is too optimistic.
We only need to look at the AMC TCSes and see if any of them are in use.
We dont care about the request being present in sleep/wake TCS.
+ if (ret)
+ goto done_write;
+
+ resp = setup_response(drv, msg, m);
Alternatively we could just actually pass "tcs" to setup_response() so
that it doesn't have to search for it based on drv and m. But I think
it's cleaner if we just associate the msg with the TCS and complete that
directly in the irq handler - if it's a sync operation.
Got it.
+ if (IS_ERR(resp)) {
+ ret = PTR_ERR(resp);
+ goto done_write;
+ }
+ resp->m = m;
You never read resp->m...
Remnant. Will remove the structure itself.
+
+ set_bit(m, drv->tcs_in_use);
+ __tcs_buffer_write(drv, m, 0, msg);
+ __tcs_trigger(drv, m);
+
+done_write:
+ spin_unlock_irqrestore(&tcs->lock, flags);
+ return ret;
+}
+
+/**
+ * rpmh_rsc_send_data: Validate the incoming message and write to the
+ * appropriate TCS block.
+ *
+ * @drv: the controller
+ * @msg: the data to be sent
+ *
+ * Return: 0 on success, -EINVAL on error.
+ * Note: This call blocks until a valid data is written to the TCS.
+ */
+int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
+{
+ int ret;
+
+ if (!msg || !msg->cmds || !msg->num_cmds ||
+ msg->num_cmds > MAX_RPMH_PAYLOAD)
+ return -EINVAL;
You're the only caller of this function, which means that if this ever
evaluates to true you will return -EINVAL and your bug will be way
harder to find than if you just end up panicing because we dereferenced
any of these null pointers.
At least wrap the whole thing in a WARN_ON() to make it possible to
detect when this happen.
Ok. Will do.
+
+ do {
+ ret = tcs_mbox_write(drv, msg);
+ if (ret == -EBUSY) {
+ pr_info_ratelimited("TCS Busy, retrying RPMH message send: addr=%#x\n",
+ msg->cmds[0].addr);
+ udelay(10);
+ }
+ } while (ret == -EBUSY);
+
+ return ret;
+}
+EXPORT_SYMBOL(rpmh_rsc_send_data);
Thanks for the review.
-- Lina