Re: [PATCH] esp: replace cli()/sti() with spin_lock_irqsave()/spin_unlock_irqrestore()

[Jim Nelson]

Jan Dittmer wrote:
> James Nelson wrote:
>
> > This is an attempt to make the esp driver SMP-correct.
>
>
> You're trying to protect the info data, correct? Could you please
> add a comment to the locks which data structures need to be
> protected by them?!
> Just blindly replacing cli/sti with spin_(un)lock doesn't make it
> SMP save.

*sigh* Thought it wouldn't be that easy.



> > diff -urN --exclude='*~' linux-2.6.10-rc3-mm1-original/drivers/char/esp.c linux-2.6.10-rc3-mm1/drivers/char/esp.c
> > --- linux-2.6.10-rc3-mm1-original/drivers/char/esp.c	2004-12-03 16:52:13.000000000 -0500
> > +++ linux-2.6.10-rc3-mm1/drivers/char/esp.c	2004-12-17 18:11:31.275037730 -0500
> > @@ -212,14 +214,14 @@
> > 	if (serial_paranoia_check(info, tty->name, "rs_stop"))
> > 		return;
> > 	
> > -	save_flags(flags); cli();
> > +	spin_lock_irqsave(&esp_lock, flags);
>
>
> info can change between return and lock

Okay.  I see what you are talking about.  I'll dig into it tomorrow (after some 
sleep).

OTOH, this would have been SMP-incorrect even before cli()/sti() was dropped in 2.5 .

> > 	if (info->IER & UART_IER_THRI) {
> > 		info->IER &= ~UART_IER_THRI;
> > 		serial_out(info, UART_ESI_CMD1, ESI_SET_SRV_MASK);
> > 		serial_out(info, UART_ESI_CMD2, info->IER);
> > 	}
> >
> > -	restore_flags(flags);
> > +	spin_unlock_irqrestore(&esp_lock, flags);
> > }
> >
> > static void rs_start(struct tty_struct *tty)
> > @@ -230,13 +232,13 @@
> > 	if (serial_paranoia_check(info, tty->name, "rs_start"))
> > 		return;
> > 	
> > -	save_flags(flags); cli();
> > +	spin_lock_irqsave(&esp_lock, flags);
>
>
> same here.
>
>
> > 	if (info->xmit_cnt && info->xmit_buf && !(info->IER & UART_IER_THRI)) {
> > 		info->IER |= UART_IER_THRI;
> > 		serial_out(info, UART_ESI_CMD1, ESI_SET_SRV_MASK);
> > 		serial_out(info, UART_ESI_CMD2, info->IER);
> > 	}
> > -	restore_flags(flags);
> > +	spin_unlock_irqrestore(&esp_lock, flags);
> > }
> >
> > /*
>
>
> Didn't read the rest. Are you sure you didn't introduce a deadlock in
> the interrupt handler? {transmit,receiver}_chars_{pio,dma} also try to take
> the lock.

Hm.  I made the assumption (silly me) that the code was inherently SMP-correct, 
and just needed the function calls replaced.

Live and learn.

Jim

> Jan
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/