Re: [PATCH v6 09/10] drivers: qcom: rpmh: add support for batch RPMH request

[Lina Iyer]

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@xxxxxxxxxxxxxx>
> > ---
> >
> > 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