Quoting Doug Anderson (2020-07-24 12:49:56) > Hi, > > On Fri, Jul 24, 2020 at 12:44 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote: > > > > > > - if (ret) > > > > - goto unlock; > > > > > > > > - ret = find_free_tcs(tcs); > > > > - if (ret < 0) > > > > - goto unlock; > > > > - tcs_id = ret; > > > > + wait_event_lock_irq(drv->tcs_wait, > > > > + (tcs_id = claim_tcs_for_req(drv, tcs, msg)) >= 0, > > > > > > Even though claim_tcs_for_req() only returns 0 or -EBUSY today (IOW it > > > never returns error codes other than -EBUSY), should we handle it? If > > > we don't, claim_tcs_for_req() should be very clear that it shouldn't > > > return any errors other than -EBUSY. > > > > Do you mean you want to change it to be > > > > (tcs_id = claim_tcs_for_req(drv, tcs, msg)) != -EBUSY > > > > instead of >= 0? It should return the tcs_id that was claimed, not just > > 0 or -EBUSY. > > Ah, right. Yes, you got it right. Of course then we have to add a > "if (tcd_id < 0) goto unlock", too. If you think it's not worth > adding this then we just need to make sure it's super obvious in > claim_tcs_for_req() that it's not allowed to return other errors. > Hmm right now the code will wait forever for the condition to become true, so it will only ever continue past this point if tcs_id >= 0. We could add a timeout here in another patch, but I didn't want to change the behavior of what is there in this patch. I don't really care if >= 0 or != -EBUSY is used here so I can change it to -EBUSY to provide clarity. If we add a timeout here, it would be better to change this driver to use a pull model instead of the push model it is using today so that the timeout isn't necessary. That would entail making a proper kthread that pulls requests off a queue of messages and then this asnyc call would append messages to the end of the queue and return immediately. That would be necessary if we want the async calls to work from non-sleeping contexts for example. I think Lina was alluding to this earlier in this thread.
