On Fri, Feb 28, 2020 at 3:38 AM Maulik Shah <mkshah@xxxxxxxxxxxxxx> wrote:
>
> Currently rpmh ctrlr dirty flag is set for all cases regardless of data
> is really changed or not. Add changes to update dirty flag when data is
> changed to newer values.
>
> Also move dirty flag updates to happen from within cache_lock and remove
> unnecessary INIT_LIST_HEAD() call and a default case from switch.
>
> Fixes: 600513dfeef3 ("drivers: qcom: rpmh: cache sleep/wake state requests")
> Signed-off-by: Maulik Shah <mkshah@xxxxxxxxxxxxxx>
> Reviewed-by: Srinivas Rao L <lsrao@xxxxxxxxxxxxxx>
> ---
> drivers/soc/qcom/rpmh.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
> index eb0ded0..f28afe4 100644
> --- a/drivers/soc/qcom/rpmh.c
> +++ b/drivers/soc/qcom/rpmh.c
> @@ -133,26 +133,30 @@ static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr,
>
> req->addr = cmd->addr;
> req->sleep_val = req->wake_val = UINT_MAX;
> - INIT_LIST_HEAD(&req->list);
> list_add_tail(&req->list, &ctrlr->cache);
>
> existing:
> switch (state) {
> case RPMH_ACTIVE_ONLY_STATE:
> - if (req->sleep_val != UINT_MAX)
> + if (req->sleep_val != UINT_MAX) {
> req->wake_val = cmd->data;
> + ctrlr->dirty = true;
> + }
> break;
> case RPMH_WAKE_ONLY_STATE:
> - req->wake_val = cmd->data;
> + if (req->wake_val != cmd->data) {
> + req->wake_val = cmd->data;
> + ctrlr->dirty = true;
> + }
> break;
> case RPMH_SLEEP_STATE:
> - req->sleep_val = cmd->data;
> - break;
> - default:
> + if (req->sleep_val != cmd->data) {
> + req->sleep_val = cmd->data;
> + ctrlr->dirty = true;
> + }
> break;
> }
>
> - ctrlr->dirty = true;
> unlock:
> spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
>
> @@ -287,6 +291,7 @@ static void cache_batch(struct rpmh_ctrlr *ctrlr, struct batch_cache_req *req)
>
> spin_lock_irqsave(&ctrlr->cache_lock, flags);
> list_add_tail(&req->list, &ctrlr->batch_cache);
> + ctrlr->dirty = true;
Is this fixing a case where we were not previously marking the
controller dirty but should have? I notice there's a fixes tag, but it
would be helpful to add something to the commit text indicating that
you're fixing a missing case where the controller should have been
marked dirty. With that fixed, you can add my tag:
Reviewed-by: Evan Green <evgreen@xxxxxxxxxxxx>
