From: Jan Dittmer <jdittmer@ppp0.net>
To: James Nelson <james4765@verizon.net>
Cc: kernel-janitors@lists.osdl.org, linux-kernel@vger.kernel.org,
akpm@osdl.org
Subject: Re: [PATCH] esp: replace cli()/sti() with spin_lock_irqsave()/spin_unlock_irqrestore()
Date: Sat, 18 Dec 2004 02:21:00 +0100 [thread overview]
Message-ID: <41C385FC.3010109@ppp0.net> (raw)
In-Reply-To: <20041217231915.14919.49991.15433@localhost.localdomain>
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.
> 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
> 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.
Jan
next prev parent reply other threads:[~2004-12-18 1:21 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2004-12-17 23:18 [PATCH] esp: replace cli()/sti() with spin_lock_irqsave()/spin_unlock_irqrestore() James Nelson
2004-12-18 1:21 ` Jan Dittmer [this message]
2004-12-18 3:46 ` Jim Nelson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=41C385FC.3010109@ppp0.net \
--to=jdittmer@ppp0.net \
--cc=akpm@osdl.org \
--cc=james4765@verizon.net \
--cc=kernel-janitors@lists.osdl.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox