| Message ID | 20200306155707.RFT.6.Icf2213131ea652087f100129359052c83601f8b0@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drivers: qcom: rpmh-rsc: Cleanup / add lots of comments | expand |
Hi, On Fri, Mar 6, 2020 at 4:00 PM Douglas Anderson <dianders@chromium.org> wrote: > > From trawling through the code (see the "A lot of comments" change) I > found that "tcs_in_use" was only kept up-to-date for ACTIVE_ONLY TCSs. > ...yet tcs_is_free() was checking the variable called from > tcs_invalidate() and tcs_invalidate() is only used for non-ACTIVE_ONLY > TCSs. > > Let's change tcs_invalidate() to just check the "RSC_DRV_STATUS" > register, which was presumably the important part. > > It also feels like for ACTIVE_ONLY TCSs that it probably wasn't > important to check the "RSC_DRV_STATUS". We'll keep doing it just in > case but we'll add a warning if it ever actually mattered. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/soc/qcom/rpmh-rsc.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) After other RPMH email threads, it's possible that this patch isn't quite right. ...but also the code wasn't quite right before. Specifically if we have 0 active TCSs then it's possible that we've used a wake TCS to send an active-only request. That would be a case where "tcs_in_use" might be set and we'd need to make sure that tcs_invalidate() checks it. However: 1. We need to add locking to tcs_invalidate() since "tcs_in_use" is protected by drv->lock and tcs_invalidate() didn't grab that lock. 2. We should add documentation explaining the situation. I'm still waiting on overall review / testing of my series, but I'll put it on my list to address this for v2. -Doug
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c index 190226151029..c63441182358 100644 --- a/drivers/soc/qcom/rpmh-rsc.c +++ b/drivers/soc/qcom/rpmh-rsc.c @@ -164,7 +164,7 @@ static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int tcs_id, } /** - * tcs_is_free() - Return if a TCS is totally free. + * tcs_is_free() - Return if an ACTIVE_ONLY TCS is totally free. * @drv: The RSC controller. * @tcs_id: The global ID of this TCS. * @@ -177,8 +177,23 @@ static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int tcs_id, */ static bool tcs_is_free(struct rsc_drv *drv, int tcs_id) { - return !test_bit(tcs_id, drv->tcs_in_use) && - read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id); + if (test_bit(tcs_id, drv->tcs_in_use)) + return false; + + if (read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id) != 0) + return true; + + /* + * If this warning never ever hits then we can change this function + * to just look at "tcs_in_use" and skip the read of the + * RSC_DRV_STATUS register. + * + * If this warning _does_ hit, we should figure out if this is just + * the way the hardware works or if there is some bug being pointed + * out. + */ + WARN(1, "Driver thought TCS was free but HW reported busy\n"); + return false; } /** @@ -204,7 +219,7 @@ static int tcs_invalidate(struct rsc_drv *drv, int type) } for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) { - if (!tcs_is_free(drv, m)) { + if (read_tcs_reg(drv, RSC_DRV_STATUS, m) == 0) { spin_unlock(&tcs->lock); return -EAGAIN; }
From trawling through the code (see the "A lot of comments" change) I found that "tcs_in_use" was only kept up-to-date for ACTIVE_ONLY TCSs. ...yet tcs_is_free() was checking the variable called from tcs_invalidate() and tcs_invalidate() is only used for non-ACTIVE_ONLY TCSs. Let's change tcs_invalidate() to just check the "RSC_DRV_STATUS" register, which was presumably the important part. It also feels like for ACTIVE_ONLY TCSs that it probably wasn't important to check the "RSC_DRV_STATUS". We'll keep doing it just in case but we'll add a warning if it ever actually mattered. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/soc/qcom/rpmh-rsc.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)