Hi Lina, On Thu, Feb 22, 2018 at 9:04 AM, Lina Iyer <ilina@xxxxxxxxxxxxxx> wrote: > On Wed, Feb 21 2018 at 23:25 +0000, Evan Green wrote: >> >> Hello Lina, >> >> On Thu, Feb 15, 2018 at 9:35 AM, Lina Iyer <ilina@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> >>> --- >>> drivers/soc/qcom/rpmh.c | 150 >>> ++++++++++++++++++++++++++++++++++++++++++++++++ >>> include/soc/qcom/rpmh.h | 8 +++ >>> 2 files changed, 158 insertions(+) >>> >>> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c >>> index dff4c46be3af..6f60bb9a4dfa 100644 >>> --- a/drivers/soc/qcom/rpmh.c >>> +++ b/drivers/soc/qcom/rpmh.c >> >> [...] >>> >>> @@ -394,6 +537,11 @@ int rpmh_flush(struct rpmh_client *rc) >>> } >>> spin_unlock_irqrestore(&rpm->lock, flags); >>> >>> + /* First flush the cached batch requests */ >>> + ret = flush_batch(rc); >>> + if (ret) >>> + return ret; >>> + >>> /* >>> * Nobody else should be calling this function other than system >>> PM,, >>> * hence we can run without locks. > > This comment and the comment in the header of this function. > >>> @@ -438,6 +586,8 @@ int rpmh_invalidate(struct rpmh_client *rc) >>> if (IS_ERR_OR_NULL(rc)) >>> return -EINVAL; >>> >>> + invalidate_batch(rc); >>> + >> >> >> Similarly to my comments in patch 7, aren't there races here with >> adding new elements? After flush_batch, but before invalidate_batch, >> somebody could call cache_batch, which would add new things on the end >> of the array. These new items would be clobbered by invalidate_batch, >> without having been flushed first. >> > Flush is a system PM function and is not called by drivers and is not > expected to run conncurrently with other threads using the RPMH library. My comment above was a little off because I was reading those two hunks (flush_batch and invalidate_batch) as being in the same function. They're not. I'm okay with the locking here, though you could remove the locking from flush_batch, since that's only run in single-threaded PM contexts. > > 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
