Messages in this thread |  | | Date | Fri, 13 Apr 2018 10:16:52 -0600 | From | Lina Iyer <> | Subject | Re: [PATCH v5 01/10] drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs |
| |
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
|  |