On Thu, Jun 20, 2019 at 04:30:03PM -0400, Doug Ledford wrote: > On Thu, 2019-06-20 at 17:05 -0300, Jason Gunthorpe wrote: > > On Thu, Jun 20, 2019 at 03:48:23PM -0400, Doug Ledford wrote: > > > > > It's an msleep() waiting for a hardware command to > > > complete. Waiting > > > synchronously for a command that has the purpose of stopping the > > > card's > > > operation does not sound like an incorrect locking or concurrency > > > model > > > to me. It sounds sane, albeit annoying. > > > > If it was the only sleep loop you might have a point, but it isn't, > > every other patch series lately seems to be adding more sleep > > loops. This sleep loop is already wrapping another sleep loop under > > __hns_roce_cmq_send() - which, for some reason, doesn't have an > > interrupt driven completion path. > > > > Nor is there any explanation why we need a sleep loop on top of a > > sleep loop, or why the command is allowed to fail or why retrying the > > failed command is even a good idea, or why it can't be properly > > interrupt driven! > > > > I'm frankly sick of it, maybe you should review HNS patches for a > > while.. > > Are you sure this hasn't changed over time and you didn't realize it? > I'm not seeing all the sleeps you are talking about. It is because I wasn't applying those patches :) > In fact, if I grep for "sleep" in hw/hns/ I only find 9 instances: 5 > in hns_roce_hw_v1 and 4 in hns_roce_hw_v2, so really only 5 at most Most of our drivers have 0 sleep loops (excluding the hfi/qib uglyness), so 9 is aleady absurd for a 2010 era driver. Jason
