Message-ID: <20180410234042.GC6727@builder>
Date:   Tue, 10 Apr 2018 16:40:42 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Lina Iyer <ilina@...eaurora.org>
Cc:     andy.gross@...aro.org, david.brown@...aro.org,
        linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
        rnayak@...eaurora.org, linux-kernel@...r.kernel.org,
        sboyd@...nel.org, evgreen@...omium.org, dianders@...omium.org
Subject: Re: [PATCH v5 01/10] drivers: qcom: rpmh-rsc: add RPMH controller
 for QCOM SoCs

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.

> +	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.

> +	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.

> +	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?

> +}
> +
> +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];

> +{
> +	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).

> +	}
> +
> +	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.

> + */
> +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"?

> +
> +	/*
> +	 * 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.

> +	}
> +
> +	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.

> +	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.

> +	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.

> +	if (IS_ERR(resp)) {
> +		ret = PTR_ERR(resp);
> +		goto done_write;
> +	}
> +	resp->m = m;

You never read resp->m...

> +
> +	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.

> +
> +	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);

Regards,
Bjorn

Powered by blists - more mailing lists