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
