On Fri, Apr 27 2018 at 17:24 -0600, Doug Anderson wrote:
Hi,
On Fri, Apr 27, 2018 at 2:54 PM, Matthias Kaehlcke <mka@xxxxxxxxxxxx> wrote:
> Am I getting something wrong here?
The for_each_set_bit() should increment the 'i' and we would attempt to
compare the first address in the request with the next command in the
TCS cache. If they don't match we repeat the process again. If it does,
then we loop through 'j' to find if the sequence matches.
Did I miss something?
One of us is clearly in need of more caffeine or ready for the
weekend, it might be me :) Maybe another pair of eyeballs could help
I need them both. Sorry about the back and forth. I understand what the
problem is. The code doesnt look right. I seem to have messed it up.
Thanks Matthias for being patient and going through this.
to resolve this deadlock ...
My single stepping above assumes that tcs->cmd_cache[i] matches
cmd[0].addr, i.e. we either found the start of the sequence we are
looking for or another sequence that starts with the same address. My
claim is that the code returns i in either case, whether the
subsequent addresses match or not.
I haven't reviewed this patch in detail, but I attempted to be another
pair of eyes here. Something is definitely wrong with the "for (j =
0; j < len; j++)" loop. I believe the code that's written right now
is equivalent to this much shorter function:
+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)
+ return i;
+ }
+
+ return -ENODATA;
+}
Specifically the test "if (tcs->cmd_cache[i] != cmd[0].addr)" does not
take "j" into account. Thus if it was false when "j == 0" it will
continue to be false for "j == 1", "j == 2", etc. Eventually you'll
hit the "else if (j == len - 1)" and return.
I believe that's what Matthias has been saying. I personally haven't
looked at the rest of the patch to see how things out to be fixed, but
I'm quite convinced that the function either has a bug or should be
written as the shorter version I've written above.
Yes, this is incorrect in its current form. This is what it should be -
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;
for (j = 0; j < len; j++) {
WARN(tcs->cmd_cache[i + j] != cmd[j].addr,
"Message does not match previous sequence.\n");
return -EINVAL;
}
if (j == len - 1)
return i;
}
return -ENODATA;
}
Thanks,
Lina
--
To unsubscribe from this list: send the line "unsubscribe linux-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
