Message-Id: <20200306155707.RFT.9.I6d3d0a3ec810dc72ff1df3cbf97deefdcdeb8eef@changeid>
Date:   Fri,  6 Mar 2020 15:59:51 -0800
From:   Douglas Anderson <dianders@...omium.org>
To:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Maulik Shah <mkshah@...eaurora.org>
Cc:     Rajendra Nayak <rnayak@...eaurora.org>, mka@...omium.org,
        evgreen@...omium.org, swboyd@...omium.org,
        Lina Iyer <ilina@...eaurora.org>,
        Douglas Anderson <dianders@...omium.org>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [RFT PATCH 9/9] drivers: qcom: rpmh-rsc: Kill cmd_cache and find_match() with fire

As talked about in the comments introduced by the patch ("drivers:
qcom: rpmh-rsc: A lot of comments"), the find_match() function()
didn't seem very sensible.  Remove it and the cmd_cache array that it
needed.

Nothing should stop us from exploring some fancy ways to avoid fully
invalidating the whole sleep/wake TCSs all the time in the future, but
find_match() doesn't seem well enough thought out to keep while we
wait for something better to arrive.

This patch isn't quite a no-op.  Specifically:

* It should be a slight performance boost of not searching through so
  many arrays.
* There is slightly less self-checking of commands written to the
  sleep/wake sets.  If it truly is an error to write to the same
  address more than once in a tcs_group then some cases (but not all)
  would have been caught before.

[1] https://lore.kernel.org/r/1583428023-19559-1-git-send-email-mkshah@codeaurora.org

Signed-off-by: Douglas Anderson <dianders@...omium.org>
---
It's possible that this might need the latest version of Maulik's
rpmh.c patches to work properly so we can really be sure that we
always invalidate before we flush.

 drivers/soc/qcom/rpmh-internal.h |   2 -
 drivers/soc/qcom/rpmh-rsc.c      | 104 +------------------------------
 2 files changed, 1 insertion(+), 105 deletions(-)

diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index 49df01af7701..7206a1ac97e8 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -34,7 +34,6 @@ struct rsc_drv;
  *                    trigger
  *             End: get irq, access req,
  *                  grab drv->lock, clear tcs_in_use, drop drv->lock
- * @cmd_cache: Flattened cache of cmds in sleep/wake TCS; num_tcs * ncpt big.
  * @slots:     Indicates which of @cmd_addr are occupied; only used for
  *             SLEEP / WAKE TCSs.  Things are tightly packed in the
  *             case that (ncpt < MAX_CMDS_PER_TCS).  That is if ncpt = 2 and
@@ -49,7 +48,6 @@ struct tcs_group {
 	int ncpt;
 	spinlock_t lock;
 	const struct tcs_request *req[MAX_TCS_PER_TYPE];
-	u32 *cmd_cache;
 	DECLARE_BITMAP(slots, MAX_TCS_SLOTS);
 };
 
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 0297da5ceeaa..733373ed56bd 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -630,93 +630,6 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
 	return ret;
 }
 
-/**
- * find_match() - Find if the cmd sequence is in the tcs_group
- * @tcs: The tcs_group to search.  Either sleep or wake.
- * @cmd: The command sequence to search for; only addr is looked at.
- * @len: The number of commands in the sequence.
- *
- * Searches through the given tcs_group to see if a given command sequence
- * is in there.
- *
- * Two sequences are matches if they modify the same set of addresses in
- * the same order.  The value of the data is not considered when deciding if
- * two things are matches.
- *
- * How this function works is best understood by example.  For our example,
- * we'll imagine our tcs group contains these (cmd, data) tuples:
- *   [(a, A), (b, B), (c, C), (d, D), (e, E), (f, F), (g, G), (h, H)]
- * ...in other words it has an element where (addr=a, data=A), etc.
- * ...we'll assume that there is one TCS in the group that can store 8 commands.
- *
- * - find_match([(a, X)]) => 0
- * - find_match([(c, X), (d, X)]) => 2
- * - find_match([(c, X), (d, X), (e, X)]) => 2
- * - find_match([(z, X)]) => -ENODATA
- * - find_match([(a, X), (y, X)]) => -EINVAL (and warning printed)
- * - find_match([(g, X), (h, X), (i, X)]) => -EINVAL (and warning printed)
- * - find_match([(y, X), (a, X)]) => -ENODATA
- *
- * NOTE: This function overall seems like it has questionable value.
- * - It can be used to update a message in the TCS with new data, but I
- *   don't believe we actually do that--we always fully invalidate and
- *   re-write everything.  Specifically it would be too limiting to force
- *   someone not to change the set of addresses written to each time.
- * - This function could be attempting to avoid writing different data to
- *   the same address twice in a tcs_group.  If that's the goal, it doesn't
- *   do a great job since find_match([(y, X), (a, X)]) return -ENODATA in my
- *   above example.
- * - If you originally wrote [(a, A), (b, B), (c, C)] and later tried to
- *   write [(a, A), (b, B)] it'd look like a match and we wouldn't consider
- *   it an error that the size got shorter.
- * - If two clients wrote sequences that happened to be placed in slots next
- *   to each other then a later check could match a sequence that was the
- *   size of both together.
- *
- * TODO: in light of the above, prehaps we can just remove this function?
- * If we later come up with fancy algorithms for updating everything without
- * full invalidations we can come up with something then.
- *
- * Only for use on sleep/wake TCSs since those are the only ones we maintain
- * tcs->slots and tcs->cmd_cache for.
- *
- * Must be called with the tcs_lock for the group held.
- *
- * Return: If the given command sequence wasn't in the tcs_group: -ENODATA.
- *         If the given command sequence was in the tcs_group: the index of
- *         the slot in the tcs_group where the first command is.
- *         In some error cases (see above), -EINVAL.
- */
-static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
-		      int len)
-{
-	int i, j;
-
-	/* Check for already cached commands */
-	for_each_set_bit(i, tcs->slots, MAX_TCS_SLOTS) {
-		if (tcs->cmd_cache[i] != cmd[0].addr)
-			continue;
-		if (i + len >= tcs->num_tcs * tcs->ncpt)
-			goto seq_err;
-		for (j = 0; j < len; j++) {
-			/*
-			 * TODO: it's actually not valid to look at
-			 * "cmd_cache[x]" if "slots[x]" doesn't have a bit
-			 * set.  Should add a check.
-			 */
-			if (tcs->cmd_cache[i + j] != cmd[j].addr)
-				goto seq_err;
-		}
-		return i;
-	}
-
-	return -ENODATA;
-
-seq_err:
-	WARN(1, "Message does not match previous sequence.\n");
-	return -EINVAL;
-}
-
 /**
  * find_slots() - Find a place to write the given message.
  * @tcs:    The controller.
@@ -728,7 +641,7 @@ static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
  *          start writing the message.
  *
  * Only for use on sleep/wake TCSs since those are the only ones we maintain
- * tcs->slots and tcs->cmd_cache for.
+ * tcs->slots for.
  *
  * Must be called with the tcs_lock for the group held.
  *
@@ -740,11 +653,6 @@ static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
 	int slot, offset;
 	int i = 0;
 
-	/* Find if we already have the msg in our TCS */
-	slot = find_match(tcs, msg->cmds, msg->num_cmds);
-	if (slot >= 0)
-		goto copy_data;
-
 	/* Do over, until we can fit the full payload in a single TCS */
 	do {
 		slot = bitmap_find_next_zero_area(tcs->slots, MAX_TCS_SLOTS,
@@ -754,11 +662,7 @@ static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
 		i += tcs->ncpt;
 	} while (slot + msg->num_cmds - 1 >= i);
 
-copy_data:
 	bitmap_set(tcs->slots, slot, msg->num_cmds);
-	/* Copy the addresses of the resources over to the slots */
-	for (i = 0; i < msg->num_cmds; i++)
-		tcs->cmd_cache[slot + i] = msg->cmds[i].addr;
 
 	offset = slot / tcs->ncpt;
 	*tcs_id = offset + tcs->offset;
@@ -889,12 +793,6 @@ static int rpmh_probe_tcs_config(struct platform_device *pdev,
 		 */
 		if (tcs->type == ACTIVE_TCS)
 			continue;
-
-		tcs->cmd_cache = devm_kcalloc(&pdev->dev,
-					      tcs->num_tcs * ncpt, sizeof(u32),
-					      GFP_KERNEL);
-		if (!tcs->cmd_cache)
-			return -ENOMEM;
 	}
 
 	drv->num_tcs = st;
-- 
2.25.1.481.gfbce0eb801-goog

Powered by blists - more mailing lists