2006-08-02 04:31:24

by Jesse Brandeburg

[permalink] [raw]
Subject: Re: Linux v2.6.18-rc3

On 7/31/06, Alan Stern <[email protected]> wrote:
> On Mon, 31 Jul 2006, Andrew Morton wrote:
>
> > core_initcall() would suit. That's actually a bit late for this sort of
> > thing, but we can always add a new section later if it becomes a problem.
> > I'd suggest that we ensure that srcu_notifier_chain_register() performs a
> > reliable BUG() if it gets called too early.
>
> Here's a patch to test. I can't try it out on my machine because
> 2.6.18-rc2-mm1 (even without the patch) crashes partway through a
> suspend-to-disk, in a way that's extremely hard to debug. Some sort of
> spinlock-related bug occurs within ioapic_write_entry.

can't test because I also can't suspend or hibernate with rc2-mm1
(resume causes hard hang with the backlight and screen off) The issue
i reported was against linus' 2.6.18-rc3 kernel.

patch didn't apply to 2.6.18-rc3.

Thanks,
Jesse


2006-08-02 04:59:25

by Andrew Morton

[permalink] [raw]
Subject: Re: Linux v2.6.18-rc3

On Tue, 1 Aug 2006 21:31:22 -0700
"Jesse Brandeburg" <[email protected]> wrote:

> On 7/31/06, Alan Stern <[email protected]> wrote:
> > On Mon, 31 Jul 2006, Andrew Morton wrote:
> >
> > > core_initcall() would suit. That's actually a bit late for this sort of
> > > thing, but we can always add a new section later if it becomes a problem.
> > > I'd suggest that we ensure that srcu_notifier_chain_register() performs a
> > > reliable BUG() if it gets called too early.
> >
> > Here's a patch to test. I can't try it out on my machine because
> > 2.6.18-rc2-mm1 (even without the patch) crashes partway through a
> > suspend-to-disk, in a way that's extremely hard to debug. Some sort of
> > spinlock-related bug occurs within ioapic_write_entry.
>
> can't test because I also can't suspend or hibernate with rc2-mm1
> (resume causes hard hang with the backlight and screen off) The issue
> i reported was against linus' 2.6.18-rc3 kernel.
>

This might help?


author Jiri Slaby <[email protected]> Tue, 01 Aug 2006 01:16:13 +0159

--- a/arch/i386/kernel/io_apic.c
+++ b/arch/i386/kernel/io_apic.c
@@ -2360,6 +2360,7 @@ static int ioapic_resume(struct sys_devi
reg_00.bits.ID = mp_ioapics[dev->id].mpc_apicid;
io_apic_write(dev->id, 0, reg_00.raw);
}
+ spin_unlock_irqrestore(&ioapic_lock, flags);
for (i = 0; i < nr_ioapic_registers[dev->id]; i ++)
ioapic_write_entry(dev->id, i, entry[i]);

-

2006-08-02 19:57:08

by Jesse Brandeburg

[permalink] [raw]
Subject: Re: Linux v2.6.18-rc3

On 8/1/06, Andrew Morton <[email protected]> wrote:
> On Tue, 1 Aug 2006 21:31:22 -0700
> "Jesse Brandeburg" <[email protected]> wrote:
>
> > On 7/31/06, Alan Stern <[email protected]> wrote:
> > > On Mon, 31 Jul 2006, Andrew Morton wrote:
> > >
> > > > core_initcall() would suit. That's actually a bit late for this sort of
> > > > thing, but we can always add a new section later if it becomes a problem.
> > > > I'd suggest that we ensure that srcu_notifier_chain_register() performs a
> > > > reliable BUG() if it gets called too early.
> > >
> > > Here's a patch to test. I can't try it out on my machine because
> > > 2.6.18-rc2-mm1 (even without the patch) crashes partway through a
> > > suspend-to-disk, in a way that's extremely hard to debug. Some sort of
> > > spinlock-related bug occurs within ioapic_write_entry.
> >
> > can't test because I also can't suspend or hibernate with rc2-mm1
> > (resume causes hard hang with the backlight and screen off) The issue
> > i reported was against linus' 2.6.18-rc3 kernel.
> >
>
> This might help?
>
>
> author Jiri Slaby <[email protected]> Tue, 01 Aug 2006 01:16:13 +0159
>
> --- a/arch/i386/kernel/io_apic.c
> +++ b/arch/i386/kernel/io_apic.c
> @@ -2360,6 +2360,7 @@ static int ioapic_resume(struct sys_devi
> reg_00.bits.ID = mp_ioapics[dev->id].mpc_apicid;
> io_apic_write(dev->id, 0, reg_00.raw);
> }
> + spin_unlock_irqrestore(&ioapic_lock, flags);
> for (i = 0; i < nr_ioapic_registers[dev->id]; i ++)
> ioapic_write_entry(dev->id, i, entry[i]);
>
> -
>
>

after applying this patch from jiri as well as the patch from alan, I
can now suspend and resume, and the patch from alan seems to work too,
but I have no idea if it executed.

BTW, I get junk out the serial port with the first bits of printk (and
during resume from S3 too) but then something manages to init the
serial port to the right speed and text starts coming out correctly.

2006-08-02 20:17:20

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: Linux v2.6.18-rc3

On Wednesday 02 August 2006 21:57, Jesse Brandeburg wrote:
> On 8/1/06, Andrew Morton <[email protected]> wrote:
> > On Tue, 1 Aug 2006 21:31:22 -0700
> > "Jesse Brandeburg" <[email protected]> wrote:
> >
> > > On 7/31/06, Alan Stern <[email protected]> wrote:
> > > > On Mon, 31 Jul 2006, Andrew Morton wrote:
> > > >
> > > > > core_initcall() would suit. That's actually a bit late for this sort of
> > > > > thing, but we can always add a new section later if it becomes a problem.
> > > > > I'd suggest that we ensure that srcu_notifier_chain_register() performs a
> > > > > reliable BUG() if it gets called too early.
> > > >
> > > > Here's a patch to test. I can't try it out on my machine because
> > > > 2.6.18-rc2-mm1 (even without the patch) crashes partway through a
> > > > suspend-to-disk, in a way that's extremely hard to debug. Some sort of
> > > > spinlock-related bug occurs within ioapic_write_entry.
> > >
> > > can't test because I also can't suspend or hibernate with rc2-mm1
> > > (resume causes hard hang with the backlight and screen off) The issue
> > > i reported was against linus' 2.6.18-rc3 kernel.
> > >
> >
> > This might help?
> >
> >
> > author Jiri Slaby <[email protected]> Tue, 01 Aug 2006 01:16:13 +0159
> >
> > --- a/arch/i386/kernel/io_apic.c
> > +++ b/arch/i386/kernel/io_apic.c
> > @@ -2360,6 +2360,7 @@ static int ioapic_resume(struct sys_devi
> > reg_00.bits.ID = mp_ioapics[dev->id].mpc_apicid;
> > io_apic_write(dev->id, 0, reg_00.raw);
> > }
> > + spin_unlock_irqrestore(&ioapic_lock, flags);
> > for (i = 0; i < nr_ioapic_registers[dev->id]; i ++)
> > ioapic_write_entry(dev->id, i, entry[i]);
> >
> > -
> >
> >
>
> after applying this patch from jiri as well as the patch from alan, I
> can now suspend and resume, and the patch from alan seems to work too,
> but I have no idea if it executed.
>
> BTW, I get junk out the serial port with the first bits of printk (and
> during resume from S3 too) but then something manages to init the
> serial port to the right speed and text starts coming out correctly.

Please try the following patch from Russell King and see if it helps.

drivers/char/tty_io.c | 13 +++++++++++++
drivers/serial/serial_core.c | 12 ++++++------
include/linux/tty.h | 2 ++
3 files changed, 21 insertions(+), 6 deletions(-)

Index: linux-2.6.18-rc1-mm2/drivers/char/tty_io.c
===================================================================
--- linux-2.6.18-rc1-mm2.orig/drivers/char/tty_io.c
+++ linux-2.6.18-rc1-mm2/drivers/char/tty_io.c
@@ -1663,6 +1663,19 @@ release_mem_out:
}

/*
+ * Get a copy of the termios structure for the driver/index
+ */
+void tty_get_termios(struct tty_driver *driver, int idx, struct termios *tio)
+{
+ lock_kernel();
+ if (driver->termios[idx])
+ *tio = *driver->termios[idx];
+ else
+ *tio = driver->init_termios;
+ unlock_kernel();
+}
+
+/*
* Releases memory associated with a tty structure, and clears out the
* driver table slots.
*/
Index: linux-2.6.18-rc1-mm2/drivers/serial/serial_core.c
===================================================================
--- linux-2.6.18-rc1-mm2.orig/drivers/serial/serial_core.c
+++ linux-2.6.18-rc1-mm2/drivers/serial/serial_core.c
@@ -1981,16 +1981,16 @@ int uart_resume_port(struct uart_driver
struct termios termios;

/*
- * First try to use the console cflag setting.
+ * Get the termios for this line
*/
- memset(&termios, 0, sizeof(struct termios));
- termios.c_cflag = port->cons->cflag;
+ tty_get_termios(drv->tty_driver, port->line, &termios);

/*
- * If that's unset, use the tty termios setting.
+ * If the console cflag is still set, subsitute that
+ * for the termios cflag.
*/
- if (state->info && state->info->tty && termios.c_cflag == 0)
- termios = *state->info->tty->termios;
+ if (port->cons->cflag)
+ termios.c_cflag = port->cons->cflag;

port->ops->set_termios(port, &termios, NULL);
console_start(port->cons);
Index: linux-2.6.18-rc1-mm2/include/linux/tty.h
===================================================================
--- linux-2.6.18-rc1-mm2.orig/include/linux/tty.h
+++ linux-2.6.18-rc1-mm2/include/linux/tty.h
@@ -284,6 +284,8 @@ extern int tty_read_raw_data(struct tty_
int buflen);
extern void tty_write_message(struct tty_struct *tty, char *msg);

+extern void tty_get_termios(struct tty_driver *drv, int idx, struct termios *tio);
+
extern int is_orphaned_pgrp(int pgrp);
extern int is_ignored(int sig);
extern int tty_signal(int sig, struct tty_struct *tty);

2006-08-02 20:23:25

by Russell King

[permalink] [raw]
Subject: Re: Linux v2.6.18-rc3

On Wed, Aug 02, 2006 at 10:16:54PM +0200, Rafael J. Wysocki wrote:
> Please try the following patch from Russell King and see if it helps.

I'd have missed this if it weren't for that comment. It hasn't been
merged so far due to the lack of feedback on it... Thanks for trying
to get that feedback.

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core

2006-08-02 20:27:10

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: Linux v2.6.18-rc3

On Wednesday 02 August 2006 22:23, Russell King wrote:
> On Wed, Aug 02, 2006 at 10:16:54PM +0200, Rafael J. Wysocki wrote:
> > Please try the following patch from Russell King and see if it helps.
>
> I'd have missed this if it weren't for that comment. It hasn't been
> merged so far due to the lack of feedback on it... Thanks for trying
> to get that feedback.

Well, it fixes the problem for me. ;-)

2006-08-02 20:33:04

by Dave Jones

[permalink] [raw]
Subject: Re: Linux v2.6.18-rc3

On Wed, Aug 02, 2006 at 09:23:09PM +0100, Russell King wrote:
> On Wed, Aug 02, 2006 at 10:16:54PM +0200, Rafael J. Wysocki wrote:
> > Please try the following patch from Russell King and see if it helps.
>
> I'd have missed this if it weren't for that comment. It hasn't been
> merged so far due to the lack of feedback on it... Thanks for trying
> to get that feedback.

I'm fairly certain I gave feedback on this, and I'm sure I recall Pavel
did too. It did nothing for me.

Dave

--
http://www.codemonkey.org.uk

2006-08-02 20:38:36

by Alan Stern

[permalink] [raw]
Subject: [PATCH 2/2] cpufreq: make the transition_notifier chain use SRCU

This patch (as762) changes the cpufreq_transition_notifier_list from a
blocking_notifier_head to an srcu_notifier_head. This will prevent errors
caused attempting to call down_read() to access the notifier chain at a
time when interrupts must remain disabled, during system suspend.

It's not clear to me whether this is really necessary; perhaps the
chain could be made into an atomic_notifier. However a couple of the
callout routines do use blocking operations, so this approach seems safer.

The head of the notifier chain needs to be initialized before use; this is
done by an __init routine at core_initcall time. If this turns out not to
be a good choice, it can easily be changed.

Signed-off-by: Alan Stern <[email protected]>

---

Index: 2.6.18-rc2-mm1/drivers/cpufreq/cpufreq.c
===================================================================
--- 2.6.18-rc2-mm1.orig/drivers/cpufreq/cpufreq.c
+++ 2.6.18-rc2-mm1/drivers/cpufreq/cpufreq.c
@@ -52,8 +52,14 @@ static void handle_update(void *data);
* The mutex locks both lists.
*/
static BLOCKING_NOTIFIER_HEAD(cpufreq_policy_notifier_list);
-static BLOCKING_NOTIFIER_HEAD(cpufreq_transition_notifier_list);
+static struct srcu_notifier_head cpufreq_transition_notifier_list;

+static int __init init_cpufreq_transition_notifier_list(void)
+{
+ srcu_init_notifier_head(&cpufreq_transition_notifier_list);
+ return 0;
+}
+core_initcall(init_cpufreq_transition_notifier_list);

static LIST_HEAD(cpufreq_governor_list);
static DEFINE_MUTEX (cpufreq_governor_mutex);
@@ -262,14 +268,14 @@ void cpufreq_notify_transition(struct cp
freqs->old = policy->cur;
}
}
- blocking_notifier_call_chain(&cpufreq_transition_notifier_list,
+ srcu_notifier_call_chain(&cpufreq_transition_notifier_list,
CPUFREQ_PRECHANGE, freqs);
adjust_jiffies(CPUFREQ_PRECHANGE, freqs);
break;

case CPUFREQ_POSTCHANGE:
adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
- blocking_notifier_call_chain(&cpufreq_transition_notifier_list,
+ srcu_notifier_call_chain(&cpufreq_transition_notifier_list,
CPUFREQ_POSTCHANGE, freqs);
if (likely(policy) && likely(policy->cpu == freqs->cpu))
policy->cur = freqs->new;
@@ -1049,7 +1055,7 @@ static int cpufreq_suspend(struct sys_de
freqs.old = cpu_policy->cur;
freqs.new = cur_freq;

- blocking_notifier_call_chain(&cpufreq_transition_notifier_list,
+ srcu_notifier_call_chain(&cpufreq_transition_notifier_list,
CPUFREQ_SUSPENDCHANGE, &freqs);
adjust_jiffies(CPUFREQ_SUSPENDCHANGE, &freqs);

@@ -1130,7 +1136,7 @@ static int cpufreq_resume(struct sys_dev
freqs.old = cpu_policy->cur;
freqs.new = cur_freq;

- blocking_notifier_call_chain(
+ srcu_notifier_call_chain(
&cpufreq_transition_notifier_list,
CPUFREQ_RESUMECHANGE, &freqs);
adjust_jiffies(CPUFREQ_RESUMECHANGE, &freqs);
@@ -1176,7 +1182,7 @@ int cpufreq_register_notifier(struct not

switch (list) {
case CPUFREQ_TRANSITION_NOTIFIER:
- ret = blocking_notifier_chain_register(
+ ret = srcu_notifier_chain_register(
&cpufreq_transition_notifier_list, nb);
break;
case CPUFREQ_POLICY_NOTIFIER:
@@ -1208,7 +1214,7 @@ int cpufreq_unregister_notifier(struct n

switch (list) {
case CPUFREQ_TRANSITION_NOTIFIER:
- ret = blocking_notifier_chain_unregister(
+ ret = srcu_notifier_chain_unregister(
&cpufreq_transition_notifier_list, nb);
break;
case CPUFREQ_POLICY_NOTIFIER:

2006-08-02 20:38:35

by Alan Stern

[permalink] [raw]
Subject: [PATCH 1/2] SRCU: report out-of-memory errors

Currently the init_srcu_struct() routine has no way to report
out-of-memory errors. This patch (as761) makes it return -ENOMEM when the
per-cpu data allocation fails.

The patch also makes srcu_init_notifier_head() report a BUG if a notifier
head can't be initialized. Perhaps it should return -ENOMEM instead, but
in the most likely cases where this might occur I don't think any recovery
is possible. Notifier chains generally are not created dynamically.

Signed-off-by: Alan Stern <[email protected]>

---

Paul, I trust you will agree with the changes this makes to the SRCU code.

The second part of this patch series will convert the cpufreq transition
notifier chain to use SRCU, with the initialization occuring in a
core_initcall routine. Although I haven't actually tried it, it seems
very likely that an attempt to use the notifier chain before it has been
initialized will cause a memory-address fault.

Alan Stern


Index: 2.6.18-rc2-mm1/kernel/sys.c
===================================================================
--- 2.6.18-rc2-mm1.orig/kernel/sys.c
+++ 2.6.18-rc2-mm1/kernel/sys.c
@@ -516,7 +516,7 @@ EXPORT_SYMBOL_GPL(srcu_notifier_call_cha
void srcu_init_notifier_head(struct srcu_notifier_head *nh)
{
mutex_init(&nh->mutex);
- init_srcu_struct(&nh->srcu);
+ BUG_ON(init_srcu_struct(&nh->srcu) < 0);
nh->head = NULL;
}

Index: 2.6.18-rc2-mm1/kernel/srcu.c
===================================================================
--- 2.6.18-rc2-mm1.orig/kernel/srcu.c
+++ 2.6.18-rc2-mm1/kernel/srcu.c
@@ -42,11 +42,12 @@
* to any other function. Each srcu_struct represents a separate domain
* of SRCU protection.
*/
-void init_srcu_struct(struct srcu_struct *sp)
+int init_srcu_struct(struct srcu_struct *sp)
{
sp->completed = 0;
- sp->per_cpu_ref = alloc_percpu(struct srcu_struct_array);
mutex_init(&sp->mutex);
+ sp->per_cpu_ref = alloc_percpu(struct srcu_struct_array);
+ return (sp->per_cpu_ref ? 0 : -ENOMEM);
}

/*
Index: 2.6.18-rc2-mm1/include/linux/srcu.h
===================================================================
--- 2.6.18-rc2-mm1.orig/include/linux/srcu.h
+++ 2.6.18-rc2-mm1/include/linux/srcu.h
@@ -43,7 +43,7 @@ struct srcu_struct {
#define srcu_barrier()
#endif /* #else #ifndef CONFIG_PREEMPT */

-void init_srcu_struct(struct srcu_struct *sp);
+int init_srcu_struct(struct srcu_struct *sp);
void cleanup_srcu_struct(struct srcu_struct *sp);
int srcu_read_lock(struct srcu_struct *sp);
void srcu_read_unlock(struct srcu_struct *sp, int idx);

2006-08-02 20:56:52

by Paul E. McKenney

[permalink] [raw]
Subject: Re: [PATCH 1/2] SRCU: report out-of-memory errors

On Wed, Aug 02, 2006 at 04:38:28PM -0400, Alan Stern wrote:
> Currently the init_srcu_struct() routine has no way to report
> out-of-memory errors. This patch (as761) makes it return -ENOMEM when the
> per-cpu data allocation fails.
>
> The patch also makes srcu_init_notifier_head() report a BUG if a notifier
> head can't be initialized. Perhaps it should return -ENOMEM instead, but
> in the most likely cases where this might occur I don't think any recovery
> is possible. Notifier chains generally are not created dynamically.

Acked-by: Paul E. McKenney <[email protected]>

> Signed-off-by: Alan Stern <[email protected]>
>
> ---
>
> Paul, I trust you will agree with the changes this makes to the SRCU code.

Indeed I do... Good catch!!!

Thanx, Paul

> The second part of this patch series will convert the cpufreq transition
> notifier chain to use SRCU, with the initialization occuring in a
> core_initcall routine. Although I haven't actually tried it, it seems
> very likely that an attempt to use the notifier chain before it has been
> initialized will cause a memory-address fault.
>
> Alan Stern
>
>
> Index: 2.6.18-rc2-mm1/kernel/sys.c
> ===================================================================
> --- 2.6.18-rc2-mm1.orig/kernel/sys.c
> +++ 2.6.18-rc2-mm1/kernel/sys.c
> @@ -516,7 +516,7 @@ EXPORT_SYMBOL_GPL(srcu_notifier_call_cha
> void srcu_init_notifier_head(struct srcu_notifier_head *nh)
> {
> mutex_init(&nh->mutex);
> - init_srcu_struct(&nh->srcu);
> + BUG_ON(init_srcu_struct(&nh->srcu) < 0);
> nh->head = NULL;
> }
>
> Index: 2.6.18-rc2-mm1/kernel/srcu.c
> ===================================================================
> --- 2.6.18-rc2-mm1.orig/kernel/srcu.c
> +++ 2.6.18-rc2-mm1/kernel/srcu.c
> @@ -42,11 +42,12 @@
> * to any other function. Each srcu_struct represents a separate domain
> * of SRCU protection.
> */
> -void init_srcu_struct(struct srcu_struct *sp)
> +int init_srcu_struct(struct srcu_struct *sp)
> {
> sp->completed = 0;
> - sp->per_cpu_ref = alloc_percpu(struct srcu_struct_array);
> mutex_init(&sp->mutex);
> + sp->per_cpu_ref = alloc_percpu(struct srcu_struct_array);
> + return (sp->per_cpu_ref ? 0 : -ENOMEM);
> }
>
> /*
> Index: 2.6.18-rc2-mm1/include/linux/srcu.h
> ===================================================================
> --- 2.6.18-rc2-mm1.orig/include/linux/srcu.h
> +++ 2.6.18-rc2-mm1/include/linux/srcu.h
> @@ -43,7 +43,7 @@ struct srcu_struct {
> #define srcu_barrier()
> #endif /* #else #ifndef CONFIG_PREEMPT */
>
> -void init_srcu_struct(struct srcu_struct *sp);
> +int init_srcu_struct(struct srcu_struct *sp);
> void cleanup_srcu_struct(struct srcu_struct *sp);
> int srcu_read_lock(struct srcu_struct *sp);
> void srcu_read_unlock(struct srcu_struct *sp, int idx);
>

2006-08-02 20:58:38

by Russell King

[permalink] [raw]
Subject: Re: Linux v2.6.18-rc3

On Wed, Aug 02, 2006 at 04:32:36PM -0400, Dave Jones wrote:
> On Wed, Aug 02, 2006 at 09:23:09PM +0100, Russell King wrote:
> > On Wed, Aug 02, 2006 at 10:16:54PM +0200, Rafael J. Wysocki wrote:
> > > Please try the following patch from Russell King and see if it helps.
> >
> > I'd have missed this if it weren't for that comment. It hasn't been
> > merged so far due to the lack of feedback on it... Thanks for trying
> > to get that feedback.
>
> I'm fairly certain I gave feedback on this, and I'm sure I recall Pavel
> did too. It did nothing for me.

Maybe you did, but it never got here. The last two messages in the
thread were mine posting this patch and pleading for some feedback
which never came.

Rafael has reported that it fixes his problem, which is great - and is
the first bit of feedback I've received on it (thanks Rafael.)

I've no idea why it doesn't work for you though.

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core

2006-08-02 21:02:22

by Dave Jones

[permalink] [raw]
Subject: Re: Linux v2.6.18-rc3

On Wed, Aug 02, 2006 at 09:58:24PM +0100, Russell King wrote:

> Maybe you did, but it never got here. The last two messages in the
> thread were mine posting this patch and pleading for some feedback
> which never came.
>
> Rafael has reported that it fixes his problem, which is great - and is
> the first bit of feedback I've received on it (thanks Rafael.)
>
> I've no idea why it doesn't work for you though.

doesn't make it any worse at least ;-)

Dave
--
http://www.codemonkey.org.uk

2006-08-02 21:19:31

by Linus Torvalds

[permalink] [raw]
Subject: Re: Linux v2.6.18-rc3



On Wed, 2 Aug 2006, Russell King wrote:
>
> Rafael has reported that it fixes his problem, which is great - and is
> the first bit of feedback I've received on it (thanks Rafael.)
>
> I've no idea why it doesn't work for you though.

Well, more importantly, why would we do something like this in the first
place?

Wouldn't it be a _lot_ better to just use the bog-standard
"suspend/resume" callbacks, and let serial drivers just suspend/resume on
their own, instead of having upper layers generate these fake
"set_termios()" calls?

The serial layer should use set_termios() when users set the termios state
(surprise surprise), not to emulate suspend/restore.

Real hardware tends to want to do a lot more _anyway_ for suspend/restore,
so the argument that "set_termios()" already exists as an interface is
pretty bogus.

Linus

2006-08-02 21:38:49

by Russell King

[permalink] [raw]
Subject: Re: Linux v2.6.18-rc3

On Wed, Aug 02, 2006 at 02:18:55PM -0700, Linus Torvalds wrote:
> Well, more importantly, why would we do something like this in the first
> place?

The low level drivers can do that already if they so wish. We provide
a library function to allow them to do the generic parts, which is
what we're talking about here.

> The serial layer should use set_termios() when users set the termios state
> (surprise surprise), not to emulate suspend/restore.

Yes Linus, you're obviously right. Would you mind re-engineering this
while I'm away for the next few days. For _ALL_ serial drivers, not
just 8250. Thanks.

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core

2006-08-02 22:05:25

by Linus Torvalds

[permalink] [raw]
Subject: Re: Linux v2.6.18-rc3



On Wed, 2 Aug 2006, Russell King wrote:
>
> > The serial layer should use set_termios() when users set the termios state
> > (surprise surprise), not to emulate suspend/restore.
>
> Yes Linus, you're obviously right. Would you mind re-engineering this
> while I'm away for the next few days. For _ALL_ serial drivers, not
> just 8250. Thanks.

The problem is that right now, the silly set_termios() call can be
actively detrimental to sub-drivers that do this right.

I suspect it would be a lot better to just fix a few major serial drivers
(and yes, that means primarily 8250), and just force others to fix
themselves as developers get around to it and care (in many cases they
might not even do so). As it is, going to set_termios way is likely to
just make things _worse_ in the long run, by just not letting the serial
driver do what's sane.

Linus

2006-08-02 22:05:51

by Russell King

[permalink] [raw]
Subject: Re: Linux v2.6.18-rc3

On Wed, Aug 02, 2006 at 10:38:34PM +0100, Russell King wrote:
> On Wed, Aug 02, 2006 at 02:18:55PM -0700, Linus Torvalds wrote:
> > The serial layer should use set_termios() when users set the termios state
> > (surprise surprise), not to emulate suspend/restore.
>
> Yes Linus, you're obviously right. Would you mind re-engineering this
> while I'm away for the next few days. For _ALL_ serial drivers, not
> just 8250. Thanks.

BTW, you'll need to replicate some of the quirk behaviour on some 8250
compatible UARTs when restoring registers - you can't blindly write
the registers in any one particular order.

Some UARTs have side effects which reset some features if you do that
(eg, TI16750 UARTs with 64 byte FIFOs need a different register writing
order from everything else, otherwise it turns off the 64-byte FIFOs.)
Others require you to write a sequence of values to a register, etc.

Okay, consider me gone from this point forwards for the next few days.

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core