Hi, On Fri, Jul 24, 2020 at 1:01 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote: > > 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. 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. -Doug
