Messages in this thread | | | Date | Tue, 1 May 2018 10:10:10 -0600 | From | Lina Iyer <> | Subject | Re: [PATCH v6 05/10] drivers: qcom: rpmh-rsc: write sleep/wake requests to TCS |
| |
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@chromium.org> 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
| |