| Message ID | 20200311161104.RFT.v2.8.I07c1f70e0e8f2dc0004bd38970b4e258acdc773e@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drivers: qcom: rpmh-rsc: Cleanup / add lots of comments | expand |
Hi, On Wed, Mar 11, 2020 at 4:14 PM Douglas Anderson <dianders@chromium.org> wrote: > > Auditing tcs_invalidate() made me worried. Specifically I saw that it > used spin_lock(), not spin_lock_irqsave(). That always worries me. > > As I understand it, using spin_lock() is only valid in these > situations: > > a) You know you are running in the interrupt handler (and all other > users of the lock use the "irqsave" variant). > b) You know that nobody using the lock is ever running in the > interrupt handler. > c) You know that someone else has always disabled interrupts before > your code runs and thus the "irqsave" variant is pointless. > > From auditing the driver we look OK. ...except that there is one > further corner case. If sometimes your code is called with IRQs > disabled and sometimes it's not you will get in trouble if someone > ever boots your board with "nosmp" (AKA in uniprocessor mode). In > such a case if someone else has the lock (without disabling > interrupts) and they get swapped out then your code (with interrupts > disabled) might loop forever waiting for the spinlock. > > It's just safer to use the irqsave version, so let's do that. In > future patches I believe tcs_invalidate() will always be called with > interrupts off anyway. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > Changes in v2: None > > drivers/soc/qcom/rpmh-rsc.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c > index ba489d18c20e..c82c734788b1 100644 > --- a/drivers/soc/qcom/rpmh-rsc.c > +++ b/drivers/soc/qcom/rpmh-rsc.c > @@ -218,9 +218,10 @@ static bool tcs_is_free(struct rsc_drv *drv, int tcs_id) > static int tcs_invalidate(struct rsc_drv *drv, int type) > { > int m; > + unsigned long flags; > struct tcs_group *tcs = &drv->tcs[type]; > > - spin_lock(&tcs->lock); > + spin_lock_irqsave(&tcs->lock, flags); > if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) { > spin_unlock(&tcs->lock); Noticed a bug while doing a code review of: https://lkml.kernel.org/r/1585244270-637-7-git-send-email-mkshah@codeaurora.org ...specifically my patch forgets to change the error case to spin_unlock_irqrestore(). ...but perhaps if that other patch lands when we can just remove the spinlocks from this function... I'll post more in my reply to that other patch. -Doug
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c index ba489d18c20e..c82c734788b1 100644 --- a/drivers/soc/qcom/rpmh-rsc.c +++ b/drivers/soc/qcom/rpmh-rsc.c @@ -218,9 +218,10 @@ static bool tcs_is_free(struct rsc_drv *drv, int tcs_id) static int tcs_invalidate(struct rsc_drv *drv, int type) { int m; + unsigned long flags; struct tcs_group *tcs = &drv->tcs[type]; - spin_lock(&tcs->lock); + spin_lock_irqsave(&tcs->lock, flags); if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) { spin_unlock(&tcs->lock); return 0; @@ -235,7 +236,7 @@ static int tcs_invalidate(struct rsc_drv *drv, int type) write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0); } bitmap_zero(tcs->slots, MAX_TCS_SLOTS); - spin_unlock(&tcs->lock); + spin_unlock_irqrestore(&tcs->lock, flags); return 0; }
Auditing tcs_invalidate() made me worried. Specifically I saw that it used spin_lock(), not spin_lock_irqsave(). That always worries me. As I understand it, using spin_lock() is only valid in these situations: a) You know you are running in the interrupt handler (and all other users of the lock use the "irqsave" variant). b) You know that nobody using the lock is ever running in the interrupt handler. c) You know that someone else has always disabled interrupts before your code runs and thus the "irqsave" variant is pointless. From auditing the driver we look OK. ...except that there is one further corner case. If sometimes your code is called with IRQs disabled and sometimes it's not you will get in trouble if someone ever boots your board with "nosmp" (AKA in uniprocessor mode). In such a case if someone else has the lock (without disabling interrupts) and they get swapped out then your code (with interrupts disabled) might loop forever waiting for the spinlock. It's just safer to use the irqsave version, so let's do that. In future patches I believe tcs_invalidate() will always be called with interrupts off anyway. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- Changes in v2: None drivers/soc/qcom/rpmh-rsc.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)