Quoting Doug Anderson (2020-07-24 13:31:39) > Hi, > > On Fri, Jul 24, 2020 at 1:27 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote: > > > > Quoting Doug Anderson (2020-07-24 13:11:59) > > > > > > I wasn't suggesting adding a timeout. I was just saying that if > > > claim_tcs_for_req() were to ever return an error code other than > > > -EBUSY that we'd need a check for it because otherwise we'd interpret > > > the result as a tcs_id. > > > > > > > Ok that sounds like you don't want a check for -EBUSY so I'll leave this > > as >= 0. > > To clarify, I'd be OK with either of these (slight preference towards > #2, but not a strong one): > > 1. Your current code and a REALLY OBVIOUS comment in > claim_tcs_for_req() saying that we'd better not return any error codes > other than -EBUSY (because we'll just blindly retry on all of them). > > - or - > > 2. Handling error codes other than -EBUSY, like this: > > wait_event_lock_irq(drv->tcs_wait, > (tcs_id = claim_tcs_for_req(drv, tcs, msg)) != -EBUSY, > drv->lock); > if (tcs_id < 0) > goto unlock; > Ah I think I understand. You're thinking that claim_tcs_for_req() may return an error value that isn't -EBUSY some day and then this wait_event_lock_irq() will keep spinning forever when it isn't busy but invalid or some such? I'd rather not do #2 because it is dead code until claim_tcs_for_req() changes. I'll add a comment indicating that it must return something that is claimed or the caller will keep trying. When the code changes the call sites should be evaluated by the author to make sure that it keeps working. I'm afraid a really big comment won't do much to help with that in the future.
