Re: [patch 0/2] semaphores: add down_interruptible_timeout() and asm-generic/semaphore.h

[Inaky Perez-Gonzalez <inaky@linux.intel.com>]

On Tuesday 27 February 2007 12:45, Ingo Oeser wrote:
> Hi Inaky,
>
> On Tuesday, 27. February 2007, Inaky Perez-Gonzalez wrote:
> > On Monday 26 February 2007 18:18, Alan wrote:
> > > > Yeah, I need semaphore. This is a hw register that says when the hw
> > > > is ready to accept a new command. Code that wants to send commands
> > > > has to down the semaphore and then send it. When hw is ready to get a
> > > > new command, it sends and IRQ and the IRQ up()s the semaphore.
> > >
> > > So you need a mutex not a semaphore
> >
> > Theoretically I could use a mutex. Practically it would trigger ugly
> > complications. Only the owner can unlock a mutex (for example), so
> > I could not unlock from an IRQ handler -- not to mention that the
> > semantic rules outlined in Documentation/mutex-design.txt explicitly
> > forbid IRQ usage.
> >
> > And then, this is what semaphores where designed for, as gates :)
> > for once that I get to use a semaphore properly...
>
> But they are not required for that :-)
>
> I would suggest to use an irq-safe spinlock for the hardware access
> and a status indicator (ready for command), if this is really just a
> command register. If the status indicator is updated (in IRQ) and read
> under spinlock, that is safe.

I have that already. The registers are shared for the two flows of
information, sending commands to the device and receiving notifications
from them, so any access to the registers is protected by a IRQ spinlock.

Now, it would be kind of pointless to do a busy poll for the command-ready
indicator if I have an IRQ that notifies me of it, ergo there the need
for a semaphore. I could use a waitqueue, as akpm suggested, but I still
need something else that says 'its only me who has the command sending
privilege now', and I'd still need to push a timeout something to detect
broken hw.

So: 
1 - to send a command I need to wait for !register.execute_command
2 - to access 'register', I need to have an spinlock
3 - when register.executing_command drops to zero, I get an IRQ
4 - other code flows access register

If I didn't have to wait for register.execute_command(), we have the usual
construct:

execute_command(hw) {
  ...
  spin_lock_irqsave(hw->lock, flags);
  prep_command_buffer;
  writel(execute_command, register)
  spin_unlock_irqrestore(hw->lock, flags);
  ...
}

checking for 'register.execute_command == 0' inside the spinlock protected
area, bailing out if busy and waiting for a while before retrying is another
(fugly) option that I am not willing to implement. 

Another option is using a wait_event() mechanism that is woken up from the 
IRQ that says register.execute_command is zero, but that's basically 
a semaphore.

A FIFO or queue is the most complex of the mechanisms, because now I have 
to create all this support infrastructure to store the queued commands.

A semaphore simplifies everything -- I guess I have brain corruption, because
I can't understand your objections.

execute_command(hw) {
  ...
  down(hw->cmd_semaphore);
  /* now it's only this thread who can send a command */
  spin_lock_irqsave(hw->lock, flags);
  prep_command_buffer;
  writel(execute_command, register)
  spin_unlock_irqrestore(hw->lock, flags);
  ...
}

irq_handler(hw) {
  ...
  spin_lock_irqsave(hw->lock, flags);
  if (readl(irq_register & execute_command_cleared))
	up(hw->cmd_sem);
  ...
  // touch command register here for other stuff
  ...
  spin_unlock_irqrestore(hw->lock, flags);
  ...
}

It's not a mutex or spinlock, is a gating or sequencing mechanism that
allows just one guy at a time.

> Timeout based locking mechanisms are flawed, because they introduce the
> hard to find timing sensitive bugs.

The only reason why the timeout is added is to detect when the hw is dead
so we can recover. This shouldn't create timing bugs because if it times
out [in this case], the hw is broken and we are going to reset it.

> Semaphores aren't good "busy/ready flags", as you might have already
> noticed.

That's why I am not using them as a flag.

-- Inaky