Messages in this thread | | | Date | Fri, 27 Apr 2018 10:24:08 -0600 | From | Lina Iyer <> | Subject | Re: [PATCH v6 09/10] drivers: qcom: rpmh: add support for batch RPMH request |
| |
On Wed, Apr 25 2018 at 17:41 -0600, Matthias Kaehlcke wrote: >On Thu, Apr 19, 2018 at 04:16:34PM -0600, Lina Iyer wrote: >> Platform drivers need make a lot of resource state requests at the same >> time, say, at the start or end of an usecase. It can be quite >> inefficient to send each request separately. Instead they can give the >> RPMH library a batch of requests to be sent and wait on the whole >> transaction to be complete. >> >> rpmh_write_batch() is a blocking call that can be used to send multiple >> RPMH command sets. Each RPMH command set is set asynchronously and the >> API blocks until all the command sets are complete and receive their >> tx_done callbacks. >> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org> >> --- >> >> Changes in v6: >> - replace rpmh_client with device * >> Changes in v4: >> - reorganize rpmh_write_batch() >> - introduce wait_count here, instead of patch#4 >> --- >> drivers/soc/qcom/rpmh.c | 155 +++++++++++++++++++++++++++++++++++++++- >> include/soc/qcom/rpmh.h | 8 +++ >> 2 files changed, 161 insertions(+), 2 deletions(-) >>
><snip> > >> +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 (ctrlr->batch_cache[index]) >> + index++; > >This will access memory beyond 'batch_cache' when the cache is full. > Ok. Will check for index.
><snip> > >> +static void invalidate_batch(struct rpmh_ctrlr *ctrlr) >> +{ >> + unsigned long flags; >> + int index = 0; >> + int i; >> + >> + spin_lock_irqsave(&ctrlr->lock, flags); >> + while (ctrlr->batch_cache[index]) >> + index++; > >Same as above. Also, why loop twice? > Good idea. Will fix.
>> + for (i = 0; i < index; i++) { >> + kfree(ctrlr->batch_cache[i]->free); >> + ctrlr->batch_cache[i] = NULL; >> + } > ><snip> > >> +/** >> + * 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. > >nit: in this driver 'n' is usually associated with the command offset >within a TCS. Since it isn't an overly descriptive name it may already >cost the reader a while to commit that to his/her memory, and now >we are overloading 'n' with a different meaning (I also noticed this in >another patch of this series, but didn't comment). > Sure, will change the variable name here.
>> /* >> * Nobody else should be calling this function other than system PM,, > ~ >Remove extra comma. OK.
Thanks, Lina
| |