A general question on SMP-safe driver code.

A general question on SMP-safe driver code.

[Jim Nelson]

Looking at a few older drivers, I'm trying to figure out the best ways to handle 
some locking.  Using drivers/char/esp.c as an example (since it's the one I'm 
trying to grok right now), here is one example:

static void rs_wait_until_sent(struct tty_struct *tty, int timeout)
{
	struct esp_struct *info = (struct esp_struct *)tty->driver_data;
	unsigned long orig_jiffies, char_time;
	unsigned long flags;

	if (serial_paranoia_check(info, tty->name, "rs_wait_until_sent"))
		return;

	orig_jiffies = jiffies;
	char_time = ((info->timeout - HZ / 50) / 1024) / 5;

	if (!char_time)
		char_time = 1;

	save_flags(flags); cli();
	serial_out(info, UART_ESI_CMD1, ESI_NO_COMMAND);
	serial_out(info, UART_ESI_CMD1, ESI_GET_TX_AVAIL);

	while ((serial_in(info, UART_ESI_STAT1) != 0x03) ||
		(serial_in(info, UART_ESI_STAT2) != 0xff)) {
		msleep_interruptible(jiffies_to_msecs(char_time));

		if (signal_pending(current))
			break;

		if (timeout && time_after(jiffies, orig_jiffies + timeout))
			break;

		serial_out(info, UART_ESI_CMD1, ESI_NO_COMMAND);
		serial_out(info, UART_ESI_CMD1, ESI_GET_TX_AVAIL);
	}
	
	restore_flags(flags);
	set_current_state(TASK_RUNNING);
}

Now, it seems like the cli()/sti() pair is to prevent other code from interrupting 
the whole sequence.  It looks like the only things actually need interrupts 
disabled (from a command sequencing perspective) is the serial_out pairs, but you 
want to keep other parts of the driver from sending other commands to the board.

So, would:

	down_interruptible(&info->sem);
	spin_lock_irq(&info->esp_lock);
	serial_out(info, UART_ESI_CMD1, ESI_NO_COMMAND);
	serial_out(info, UART_ESI_CMD1, ESI_GET_TX_AVAIL);
	spin_unlock_irq(&info->esp_lock);

	while ((serial_in(info, UART_ESI_STAT1) != 0x03) ||
		(serial_in(info, UART_ESI_STAT2) != 0xff)) {
		msleep_interruptible(jiffies_to_msecs(char_time));

		if (signal_pending(current))
			break;

		if (timeout && time_after(jiffies, orig_jiffies + timeout))
			break;

		spin_lock_irq(&info->esp_lock);
		serial_out(info, UART_ESI_CMD1, ESI_NO_COMMAND);
		serial_out(info, UART_ESI_CMD1, ESI_GET_TX_AVAIL);
		spin_unlock_irq(&info->esp_lock);
	up(&info->esp_sem);

work if all other areas of the driver that send commands to the board also try for 
the semaphore?

Is there an easier way of doing this?

Jim

Re: A general question on SMP-safe driver code.

[Linus Torvalds]

On Fri, 24 Dec 2004, Jim Nelson wrote:
> 
> work if all other areas of the driver that send commands to the board also try for 
> the semaphore?

The most common reason _not_ to use a semaphore, but a single simple 
spinlock is:
 - spinlocks are generally faster. 
 - you can't use semaphores to protect against interrupts, as interrupts 
   cannot take semaphores.

> Is there an easier way of doing this?

The simplest approach tends to be to just have a single spinlock per
driver (or, if the driver can drive multiple independent ports, one per
port).

The only advantage of semaphores is that you can do user accesses and you 
can sleep during them, but if you're looking at converting a driver that 
used to just depend on the global interrupt lock, that shouldn't be an 
issue anyway. Generally, the semaphores are more useful at a higher level 
(ie there is almost never any reason to protect actual _IO_ accesses with 
a semaphore).

The biggest problem with converting old-style irq locks into spinlocks
tends to be that the irq locking allowed nesting (though the use of
save_flags/restore_flags), and normal spinlocks don't.

You can make your own nesting spinlocks, of course, but the reason there
aren't any standard nesting locks in the kernel is that in pretty much all
cases you can trivially avoid the nesting by just moving the lock
sufficiently far out, or just re-organizing the source a bit.

			Linus

Re: A general question on SMP-safe driver code.

[Alan Cox]

On Gwe, 2004-12-24 at 22:58, Jim Nelson wrote:
> Looking at a few older drivers, I'm trying to figure out the best ways to handle 
> some locking.  Using drivers/char/esp.c as an example (since it's the one I'm 
> trying to grok right now), here is one example:

> ;
> 	serial_out(info, UART_ESI_CMD1, ESI_GET_TX_AVAIL);
> 	spin_unlock_irq(&info->esp_lock);
> 
> 	while ((serial_in(info, UART_ESI_STAT1) != 0x03) ||
> 		(serial_in(info, UART_ESI_STAT2) != 0xff)) {

You need to guard these as well in the locks. It might actually look a
lot cleaner to have functions esp_send_command(info, a, b) and the like
which do the locking internally ?