Quoting Douglas Anderson (2020-04-22 14:55:02)
> The rpmh-rsc code had both a driver-level lock (sometimes referred to
> in comments as drv->lock) and a lock per-TCS. The idea was supposed
> to be that there would be times where you could get by with just
> locking a TCS lock and therefor other RPMH users wouldn't be blocked.
>
> The above didn't work out so well.
>
> Looking at tcs_write() the bigger drv->lock was held for most of the
> function anyway. Only the __tcs_buffer_write() and
> __tcs_set_trigger() calls were called without it the drv->lock. It
without holding the drv->lock
> actually turns out that in tcs_write() we don't need to hold the
> drv->lock for those function calls anyway even if the per-TCS lock
> isn't there anymore.
Why?
> Thus, from a tcs_write() point of view, the
> per-TCS lock was useless.
>
> Looking at rpmh_rsc_write_ctrl_data(), only the per-TCS lock was held.
> It turns out, though, that this function already needs to be called
> with the equivalent of the drv->lock held anyway (we either need to
> hold drv->lock as we will in a future patch or we need to know no
> other CPUs could be running as happens today). Specifically
> rpmh_rsc_write_ctrl_data() might be writing to a TCS that has been
> borrowed for writing an active transation but it never checks this.
>
> Let's eliminate this extra overhead and avoid possible AB BA locking
> headaches.
>
> Suggested-by: Maulik Shah <mkshah@xxxxxxxxxxxxxx>
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---
>
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index e540e49fd61c..71cebe7fd452 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -581,24 +575,19 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
> if (IS_ERR(tcs))
> return PTR_ERR(tcs);
>
> - spin_lock_irqsave(&tcs->lock, flags);
> - spin_lock(&drv->lock);
> + spin_lock_irqsave(&drv->lock, flags);
> /*
> * The h/w does not like if we send a request to the same address,
> * when one is already in-flight or being processed.
> */
> ret = check_for_req_inflight(drv, tcs, msg);
> - if (ret) {
> - spin_unlock(&drv->lock);
> - goto done_write;
> - }
> + if (ret)
> + goto err;
Nitpick: Usually 'goto err' is used for error paths, not unlock paths.
Use 'goto unlock' for that.
>
> - tcs_id = find_free_tcs(tcs);
> - if (tcs_id < 0) {
> - ret = tcs_id;
> - spin_unlock(&drv->lock);
> - goto done_write;
> - }
> + ret = find_free_tcs(tcs);
> + if (ret < 0)
> + goto err;
> + tcs_id = ret;
>
> tcs->req[tcs_id - tcs->offset] = msg;
> set_bit(tcs_id, drv->tcs_in_use);
> @@ -612,13 +601,21 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
> write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, tcs_id, 0);
> enable_tcs_irq(drv, tcs_id, true);
> }
> - spin_unlock(&drv->lock);
> + spin_unlock_irqrestore(&drv->lock, flags);
>
> + /*
> + * These two can be done after the lock is released because:
> + * - We marked "tcs_in_use" under lock.
> + * - Once "tcs_in_use" has been marked nobody else could be writing
> + * to these registers until the interrupt goes off.
> + * - The interrupt can't go off until we trigger.
trigger via some function?
> + */
> __tcs_buffer_write(drv, tcs_id, 0, msg);
> __tcs_set_trigger(drv, tcs_id, true);
>
> -done_write:
> - spin_unlock_irqrestore(&tcs->lock, flags);
> + return 0;
> +err:
> + spin_unlock_irqrestore(&drv->lock, flags);
> return ret;
> }
>
