Hi, On Wed, May 9, 2018 at 10:01 AM, Lina Iyer <il...@codeaurora.org> wrote: > /** > @@ -77,12 +82,14 @@ struct rpmh_request { > * @cache: the list of cached requests > * @lock: synchronize access to the controller data > * @dirty: was the cache updated since flush > + * @batch_cache: Cache sleep and wake requests sent as batch > */ > struct rpmh_ctrlr { > struct rsc_drv *drv; > struct list_head cache; > spinlock_t lock; > bool dirty; > + const struct rpmh_request *batch_cache[RPMH_MAX_BATCH_CACHE];
I'm pretty confused about why the "batch_cache" is separate from the normal cache. As far as I can tell the purpose of the two is the same but you have two totally separate code paths and data structures. > }; > > static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_CTRLR]; > @@ -133,6 +140,7 @@ void rpmh_tx_done(const struct tcs_request *msg, int r) > struct rpmh_request *rpm_msg = container_of(msg, struct rpmh_request, > msg); > struct completion *compl = rpm_msg->completion; > + atomic_t *wc = rpm_msg->wait_count; > > rpm_msg->err = r; > > @@ -143,8 +151,13 @@ void rpmh_tx_done(const struct tcs_request *msg, int r) > kfree(rpm_msg->free); > > /* Signal the blocking thread we are done */ > - if (compl) > - complete(compl); > + if (!compl) > + return; The comment above this "if" block no longer applies to the line next to it after your patch. ...but below I suggest you get rid of "wait_count", so maybe this part of the patch will go away. > +static int cache_batch(struct rpmh_ctrlr *ctrlr, > + struct rpmh_request **rpm_msg, int count) > +{ > + unsigned long flags; > + int ret = 0; > + int index = 0; > + int i; > + > + spin_lock_irqsave(&ctrlr->lock, flags); > + while (index < RPMH_MAX_BATCH_CACHE && ctrlr->batch_cache[index]) > + index++; > + if (index + count >= RPMH_MAX_BATCH_CACHE) { > + ret = -ENOMEM; > + goto fail; > + } > + > + for (i = 0; i < count; i++) > + ctrlr->batch_cache[index + i] = rpm_msg[i]; > +fail: Nit: this label is for both failure and normal exit, so call it "exit". > + spin_unlock_irqrestore(&ctrlr->lock, flags); > + > + return ret; > +} As part of my overall confusion about why the batch cache is different than the normal one: for the normal use case you still call rpmh_rsc_write_ctrl_data() for things you put in your cache, but you don't for the batch cache. I still haven't totally figured out what rpmh_rsc_write_ctrl_data() does, but it seems strange that you don't do it for the batch cache but you do for the other one. > +/** > + * rpmh_write_batch: Write multiple sets of RPMH commands and wait for the > + * batch to finish. > + * > + * @dev: the device making the request > + * @state: Active/sleep set > + * @cmd: The payload data > + * @n: The array of count of elements in each batch, 0 terminated. > + * > + * Write a request to the RSC controller without caching. If the request > + * state is ACTIVE, then the requests are treated as completion request > + * and sent to the controller immediately. The function waits until all the > + * commands are complete. If the request was to SLEEP or WAKE_ONLY, then the > + * request is sent as fire-n-forget and no ack is expected. > + * > + * May sleep. Do not call from atomic contexts for ACTIVE_ONLY requests. > + */ > +int rpmh_write_batch(const struct device *dev, enum rpmh_state state, > + const struct tcs_cmd *cmd, u32 *n) > +{ > + struct rpmh_request *rpm_msg[RPMH_MAX_REQ_IN_BATCH] = { NULL }; > + DECLARE_COMPLETION_ONSTACK(compl); > + atomic_t wait_count = ATOMIC_INIT(0); > + struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev); > + int count = 0; > + int ret, i; > + > + if (IS_ERR(ctrlr) || !cmd || !n) > + return -EINVAL; > + > + while (n[count++] > 0) > + ; > + count--; > + if (!count || count > RPMH_MAX_REQ_IN_BATCH) > + return -EINVAL; > + > + for (i = 0; i < count; i++) { > + rpm_msg[i] = __get_rpmh_msg_async(state, cmd, n[i]); > + if (IS_ERR_OR_NULL(rpm_msg[i])) { Just "IS_ERR". It's never NULL. ...also add a i-- somewhere in here or you're going to be kfree()ing your error value, aren't you? > + ret = PTR_ERR(rpm_msg[i]); > + for (; i >= 0; i--) > + kfree(rpm_msg[i]->free); > + return ret; > + } > + cmd += n[i]; > + } > + > + if (state != RPMH_ACTIVE_ONLY_STATE) > + return cache_batch(ctrlr, rpm_msg, count); Don't you need to free rpm_msg items in this case? > + > + atomic_set(&wait_count, count); > + > + for (i = 0; i < count; i++) { > + rpm_msg[i]->completion = &compl; > + rpm_msg[i]->wait_count = &wait_count; > + ret = rpmh_rsc_send_data(ctrlr->drv, &rpm_msg[i]->msg); > + if (ret) { > + int j; > + > + pr_err("Error(%d) sending RPMH message addr=%#x\n", > + ret, rpm_msg[i]->msg.cmds[0].addr); > + for (j = i; j < count; j++) > + rpmh_tx_done(&rpm_msg[j]->msg, ret); You're just using rpmh_tx_done() to free memory? Note that you'll probably do your error handling in this function a favor if you rename __get_rpmh_msg_async() to __fill_rpmh_msg() and remove the memory allocation from there. Then you can do one big allocation of the whole array in rpmh_write_batch() and then you'll only have one free at the end... > + break; "break" seems wrong here. You'll end up waiting for the completion, then I guess timing out, then returning -ETIMEDOUT? > + } > + } > + > + ret = wait_for_completion_timeout(&compl, RPMH_TIMEOUT_MS); The "wait_count" abstraction is confusing and I believe it's not needed. I think you can remove it and change the above to this (untested) code: time_left = RPMH_TIMEOUT_MS; for (i = 0; i < count; i++) { time_left = wait_for_completion_timeout(&compl, time_left); if (!time_left) return -ETIMEDOUT; } ...specifically completions are additive, so just wait "count" times and then the reader doesn't need to learn your new wait_count abstraction and try to reason about it. ...and, actually, I argue in other replies that this should't use a timeout, so even cleaner: for (i = 0; i < count; i++) wait_for_completion(&compl); Once you do that, you can also get rid of the need to pre-count "n", so all your loops turn into: for (i = 0; n[i]; i++) I suppose you might want to get rid of "RPMH_MAX_REQ_IN_BATCH" and dynamically allocate your array too, but that seems sane. As per above it seems like you should just dynamically allocate a whole array of "struct rpmh_request" items at once anyway. --- > + return (ret > 0) ? 0 : -ETIMEDOUT; > + > +} > +EXPORT_SYMBOL(rpmh_write_batch); Perhaps an even simpler thing than taking all my advice above: can't you just add a optional completion to rpmh_write_async()? That would just be stuffed into rpm_msg. Now your batch code would just be a bunch of calls to rpmh_write_async() with an equal number of wait_for_completion() calls at the end. Is there a reason that wouldn't work? You'd get rid of _a lot_ of code. -Doug