* [bug report] soc: qcom: rpmh-rsc: Sleep waiting for tcs slots to be free
@ 2024-05-08 14:49 Dan Carpenter
2024-05-08 21:01 ` Stephen Boyd
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2024-05-08 14:49 UTC (permalink / raw
To: swboyd; +Cc: linux-arm-msm
Hello Stephen Boyd,
Commit 2bc20f3c8487 ("soc: qcom: rpmh-rsc: Sleep waiting for tcs
slots to be free") from Jul 24, 2020 (linux-next), leads to the
following Smatch static checker warning:
drivers/soc/qcom/rpmh-rsc.c:658 rpmh_rsc_send_data()
warn: mixing irqsave and irq
drivers/soc/qcom/rpmh-rsc.c
645 int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
646 {
647 struct tcs_group *tcs;
648 int tcs_id;
649 unsigned long flags;
650
651 tcs = get_tcs_for_msg(drv, msg);
652 if (IS_ERR(tcs))
653 return PTR_ERR(tcs);
654
655 spin_lock_irqsave(&drv->lock, flags);
flags saves if this is called with IRQs disabled. I don't think it is.
656
657 /* Wait forever for a free tcs. It better be there eventually! */
--> 658 wait_event_lock_irq(drv->tcs_wait,
659 (tcs_id = claim_tcs_for_req(drv, tcs, msg)) >= 0,
660 drv->lock);
This will enable IRQs and then disable them again. If this were called
with IRQs disabled then this would probably be bad. (But again, I don't
think it is).
661
662 tcs->req[tcs_id - tcs->offset] = msg;
663 set_bit(tcs_id, drv->tcs_in_use);
664 if (msg->state == RPMH_ACTIVE_ONLY_STATE && tcs->type != ACTIVE_TCS) {
665 /*
666 * Clear previously programmed WAKE commands in selected
667 * repurposed TCS to avoid triggering them. tcs->slots will be
668 * cleaned from rpmh_flush() by invoking rpmh_rsc_invalidate()
669 */
670 write_tcs_reg_sync(drv, drv->regs[RSC_DRV_CMD_ENABLE], tcs_id, 0);
671 enable_tcs_irq(drv, tcs_id, true);
672 }
673 spin_unlock_irqrestore(&drv->lock, flags);
And then it sets it back to whatever it was when it was called. So
that's fine.
674
675 /*
676 * These two can be done after the lock is released because:
677 * - We marked "tcs_in_use" under lock.
678 * - Once "tcs_in_use" has been marked nobody else could be writing
679 * to these registers until the interrupt goes off.
680 * - The interrupt can't go off until we trigger w/ the last line
681 * of __tcs_set_trigger() below.
682 */
683 __tcs_buffer_write(drv, tcs_id, 0, msg);
684 __tcs_set_trigger(drv, tcs_id, true);
685
686 return 0;
687 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] soc: qcom: rpmh-rsc: Sleep waiting for tcs slots to be free
2024-05-08 14:49 [bug report] soc: qcom: rpmh-rsc: Sleep waiting for tcs slots to be free Dan Carpenter
@ 2024-05-08 21:01 ` Stephen Boyd
2024-05-09 6:41 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Stephen Boyd @ 2024-05-08 21:01 UTC (permalink / raw
To: Dan Carpenter; +Cc: linux-arm-msm
Quoting Dan Carpenter (2024-05-08 07:49:34)
> Hello Stephen Boyd,
>
> Commit 2bc20f3c8487 ("soc: qcom: rpmh-rsc: Sleep waiting for tcs
> slots to be free") from Jul 24, 2020 (linux-next), leads to the
> following Smatch static checker warning:
>
> drivers/soc/qcom/rpmh-rsc.c:658 rpmh_rsc_send_data()
> warn: mixing irqsave and irq
>
> drivers/soc/qcom/rpmh-rsc.c
> 645 int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
> 646 {
> 647 struct tcs_group *tcs;
> 648 int tcs_id;
> 649 unsigned long flags;
> 650
> 651 tcs = get_tcs_for_msg(drv, msg);
> 652 if (IS_ERR(tcs))
> 653 return PTR_ERR(tcs);
> 654
> 655 spin_lock_irqsave(&drv->lock, flags);
>
> flags saves if this is called with IRQs disabled. I don't think it is.
>
> 656
> 657 /* Wait forever for a free tcs. It better be there eventually! */
> --> 658 wait_event_lock_irq(drv->tcs_wait,
> 659 (tcs_id = claim_tcs_for_req(drv, tcs, msg)) >= 0,
> 660 drv->lock);
>
> This will enable IRQs and then disable them again. If this were called
> with IRQs disabled then this would probably be bad. (But again, I don't
> think it is).
>
> 661
> 662 tcs->req[tcs_id - tcs->offset] = msg;
> 663 set_bit(tcs_id, drv->tcs_in_use);
> 664 if (msg->state == RPMH_ACTIVE_ONLY_STATE && tcs->type != ACTIVE_TCS) {
> 665 /*
> 666 * Clear previously programmed WAKE commands in selected
> 667 * repurposed TCS to avoid triggering them. tcs->slots will be
> 668 * cleaned from rpmh_flush() by invoking rpmh_rsc_invalidate()
> 669 */
> 670 write_tcs_reg_sync(drv, drv->regs[RSC_DRV_CMD_ENABLE], tcs_id, 0);
> 671 enable_tcs_irq(drv, tcs_id, true);
> 672 }
> 673 spin_unlock_irqrestore(&drv->lock, flags);
>
> And then it sets it back to whatever it was when it was called. So
> that's fine.
>
I see. I think you want this sort of patch so that it is clearer that
this can't be called with interrupts disabled? Would Smatch be happier?
---8<----
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index a021dc71807b..568d0b8c52d6 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -645,13 +645,14 @@ int rpmh_rsc_send_data(struct rsc_drv *drv,
const struct tcs_request *msg)
{
struct tcs_group *tcs;
int tcs_id;
- unsigned long flags;
+
+ might_sleep();
tcs = get_tcs_for_msg(drv, msg);
if (IS_ERR(tcs))
return PTR_ERR(tcs);
- spin_lock_irqsave(&drv->lock, flags);
+ spin_lock_irq(&drv->lock);
/* Wait forever for a free tcs. It better be there eventually! */
wait_event_lock_irq(drv->tcs_wait,
@@ -669,7 +670,7 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, const
struct tcs_request *msg)
write_tcs_reg_sync(drv, drv->regs[RSC_DRV_CMD_ENABLE], tcs_id, 0);
enable_tcs_irq(drv, tcs_id, true);
}
- spin_unlock_irqrestore(&drv->lock, flags);
+ spin_unlock_irq(&drv->lock);
/*
* These two can be done after the lock is released because:
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [bug report] soc: qcom: rpmh-rsc: Sleep waiting for tcs slots to be free
2024-05-08 21:01 ` Stephen Boyd
@ 2024-05-09 6:41 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2024-05-09 6:41 UTC (permalink / raw
To: Stephen Boyd; +Cc: linux-arm-msm
On Wed, May 08, 2024 at 05:01:26PM -0400, Stephen Boyd wrote:
> Quoting Dan Carpenter (2024-05-08 07:49:34)
> > Hello Stephen Boyd,
> >
> > Commit 2bc20f3c8487 ("soc: qcom: rpmh-rsc: Sleep waiting for tcs
> > slots to be free") from Jul 24, 2020 (linux-next), leads to the
> > following Smatch static checker warning:
> >
> > drivers/soc/qcom/rpmh-rsc.c:658 rpmh_rsc_send_data()
> > warn: mixing irqsave and irq
> >
> > drivers/soc/qcom/rpmh-rsc.c
> > 645 int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
> > 646 {
> > 647 struct tcs_group *tcs;
> > 648 int tcs_id;
> > 649 unsigned long flags;
> > 650
> > 651 tcs = get_tcs_for_msg(drv, msg);
> > 652 if (IS_ERR(tcs))
> > 653 return PTR_ERR(tcs);
> > 654
> > 655 spin_lock_irqsave(&drv->lock, flags);
> >
> > flags saves if this is called with IRQs disabled. I don't think it is.
> >
> > 656
> > 657 /* Wait forever for a free tcs. It better be there eventually! */
> > --> 658 wait_event_lock_irq(drv->tcs_wait,
> > 659 (tcs_id = claim_tcs_for_req(drv, tcs, msg)) >= 0,
> > 660 drv->lock);
> >
> > This will enable IRQs and then disable them again. If this were called
> > with IRQs disabled then this would probably be bad. (But again, I don't
> > think it is).
> >
> > 661
> > 662 tcs->req[tcs_id - tcs->offset] = msg;
> > 663 set_bit(tcs_id, drv->tcs_in_use);
> > 664 if (msg->state == RPMH_ACTIVE_ONLY_STATE && tcs->type != ACTIVE_TCS) {
> > 665 /*
> > 666 * Clear previously programmed WAKE commands in selected
> > 667 * repurposed TCS to avoid triggering them. tcs->slots will be
> > 668 * cleaned from rpmh_flush() by invoking rpmh_rsc_invalidate()
> > 669 */
> > 670 write_tcs_reg_sync(drv, drv->regs[RSC_DRV_CMD_ENABLE], tcs_id, 0);
> > 671 enable_tcs_irq(drv, tcs_id, true);
> > 672 }
> > 673 spin_unlock_irqrestore(&drv->lock, flags);
> >
> > And then it sets it back to whatever it was when it was called. So
> > that's fine.
> >
>
> I see. I think you want this sort of patch so that it is clearer that
> this can't be called with interrupts disabled? Would Smatch be happier?
>
Ah... Actually, yeah, wait_event_lock_irq() has a schedule() it it
doesn't it? I've been meaing to make Smatch track when IRQs are
enabled/disabled so this makes me want to do that more.
Anyway, thanks. Looks good.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-05-09 6:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-08 14:49 [bug report] soc: qcom: rpmh-rsc: Sleep waiting for tcs slots to be free Dan Carpenter
2024-05-08 21:01 ` Stephen Boyd
2024-05-09 6:41 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.