Toshiba Laptop Support and IRQ Locks
Subsystems (2)
PATCH 1/1 Toshiba Laptop Support and IRQ Locks
Hi,
Toshiba laptop support is broken. Here's my rookie attempt at fixing it.
drivers/char/toshiba.c +2 -1
@@ -82,6 +82,8 @@static int tosh_fn =0 ; +extern rwlock_t tosh_lock; +MODULE_PARM (tosh_fn,"i" );MODULE_LICENSE ("GPL" );
@@ -114,11 +116,10 @@if (tosh_fn!=0 ) { scan =inb (tosh_fn); }else { -
save_flags(flags);
-
cli();
+
write_lock_irq(&tosh_lock);
outb(0x8e, 0xe4);
scan = inb(0xe5);
-
restore_flags(flags);
+
write_unlock_irq(&tosh_lock);}
return (int) scan;
@@ -141,35 +142,32 @@
if (tosh_id==0xfccb) {
if (eax==0xfe00) {
/* fan status */ - save_flags(flags); - cli(); + write_lock_irq(&tosh_lock); outb(0xbe, 0xe4); al = inb(0xe5); - restore_flags(flags); + write_unlock_irq(&tosh_lock); regs->eax = 0x00; regs->ecx = (unsigned int) (al & 0x01);
}
if ((eax==0xff00) && (ecx==0x0000)) {
/* fan off */ - save_flags(flags); - cli(); + write_lock_irq(&tosh_lock); outb(0xbe, 0xe4); al = inb(0xe5); outb(0xbe, 0xe4); outb (al | 0x01, 0xe5); - restore_flags(flags); + write_unlock_irq(&tosh_lock); regs->eax = 0x00; regs->ecx = 0x00;
}
if ((eax==0xff00) && (ecx==0x0001)) {
/* fan on */ - save_flags(flags); - cli(); + write_lock_irq(&tosh_lock); outb(0xbe, 0xe4); al = inb(0xe5); outb(0xbe, 0xe4); outb(al & 0xfe, 0xe5); - restore_flags(flags); + write_unlock_irq(&tosh_lock); regs->eax = 0x00; regs->ecx = 0x01;
}
@@ -180,33 +178,30 @@
if (tosh_id==0xfccc) {
if (eax==0xfe00) {
/* fan status */ - save_flags(flags); - cli(); + write_lock_irq(&tosh_lock); outb(0xe0, 0xe4); al = inb(0xe5); - restore_flags(flags); + write_unlock_irq(&tosh_lock); regs->eax = 0x00; regs->ecx = al & 0x01;
}
if ((eax==0xff00) && (ecx==0x0000)) {
/* fan off */ - save_flags(flags); - cli(); + write_lock_irq(&tosh_lock); outb(0xe0, 0xe4); al = inb(0xe5); outw(0xe0 | ((al & 0xfe) << 8), 0xe4); - restore_flags(flags); + write_unlock_irq(&tosh_lock); regs->eax = 0x00; regs->ecx = 0x00;
}
if ((eax==0xff00) && (ecx==0x0001)) {
/* fan on */ - save_flags(flags); - cli(); + write_lock_irq(&tosh_lock); outb(0xe0, 0xe4); al = inb(0xe5); outw(0xe0 | ((al | 0x01) << 8), 0xe4); - restore_flags(flags); + write_unlock_irq(&tosh_lock); regs->eax = 0x00; regs->ecx = 0x01;
}
Quoted 3 lines · John Weber
Hi,
Toshiba laptop support is broken. Here's my rookie attempt at fixing it.
Looks basically sound. You probably want to use spinlock_irqsave - the spin locks are less overhead than the reader/writer locks and you don't really seem to be using it for anything else. I'm assuming we want the irqsave to block interrupts because the I/O cycles might have to happen one after another - if not they could be relaxed - perhaps Jonathan knows ?
Jonathan Buzzard
maintainerQuoted 4 lines · [email protected] said:
Hi,
Toshiba laptop support is broken. Here's my rookie attempt at fixing it.
Whats broken? I have not seen the patch, though I don't track the latest 2.5 kernels either.
Quoted 6 lines
Looks basically sound. You probably want to use
spinlock_irqsave- the spin locks are less overhead than the reader/writer locks and you don't really seem to be using it for anything else. I'm assuming we want the irqsave to block interrupts because the I/O cycles might have to happen one after another - if not they could be relaxed - perhaps Jonathan knows ?
Someone show me the patch and I can say for sure.
Two things to bare in mind, Toshiba have yet to do any sort of multi processor laptop, are extremely unlikely to ever manufacture one, and to the best of my knowledge the module only loads on Toshiba laptops. If it loads on anything else it is broken and needs fixing so it does not.
On this ground I always felt that use of cli() was more than justified. Though I understand that you might want to banish this stuff from the kernel, and use something multiprocessor safe.
Secondly bare in mind that the Toshiba laptop module for the most part puts the processor into System Management Mode. This does its own locking, once you are in SMM thats *everything* else on hold till it finishes.
While I am at it, I have been recently pestered with patches to toshiba.c to add random proc interfaces for various HCI calls. I have personally rejected the patches as I don't believe that exposing HCI calls in the kernel is the right thing to do. Firstly there are over 30 calls to expose, and it makes not sense to just expose one or two that happen to be the favourite of some random person. Secondly everything the patches do can be achieved with a userspace program that use the ioctl on /dev/toshiba that the module provides. Thus avoiding any bloat to the kernel, and avoiding the need to constantly add new proc based interfaces each time I discover a new HCI call.
If someone does propose such a patch I recommend rejecting it.
JAB.
Signature · 2 lines
Jonathan A. Buzzard Email: [email protected] Northumberland, United Kingdom. Tel: +44(0)1661-832195
Quoted 20 lines · Jonathan Buzzard
Quoted 4 lines · [email protected] said:
Hi,
Toshiba laptop support is broken. Here's my rookie attempt at fixing it.
Whats broken? I have not seen the patch, though I don't track the latest 2.5 kernels either.
Quoted 6 lines
Looks basically sound. You probably want to use
spinlock_irqsave- the spin locks are less overhead than the reader/writer locks and you don't really seem to be using it for anything else. I'm assuming we want the irqsave to block interrupts because the I/O cycles might have to happen one after another - if not they could be relaxed - perhaps Jonathan knows ?
Quoted 7 lines
Someone show me the patch and I can say for sure.
Two things to bare in mind, Toshiba have yet to do any sort of multi processor laptop, are extremely unlikely to ever manufacture one, and to the best of my knowledge the module only loads on Toshiba laptops. If it loads on anything else it is broken and needs fixing so it does not.
What about P4 Hyperthreading?
--
Brian Gerst
Jonathan Buzzard
maintainerQuoted 7 lines · [email protected] said:
Quoted 5 lines
Two things to bare in mind, Toshiba have yet to do any sort of multi processor laptop, are extremely unlikely to ever manufacture one, and to the best of my knowledge the module only loads on Toshiba laptops. If it loads on anything else it is broken and needs fixing so it does not.
What about P4 Hyperthreading?
The cli() and associated code in the toshiba SMM driver is for laptops manufactured prior to 1999. It is not relevant and never will be on any laptop with a P4 processor in it. For that matter it is not even relevant on a PIII or PII processor. It is strictly Pentiums and 486's.
JAB.
Signature · 2 lines
Jonathan A. Buzzard Email: [email protected] Northumberland, United Kingdom. Tel: +44(0)1661-832195
Quoted 7 lines · Alan Cox
Quoted 3 lines · John Weber
Hi,
Toshiba laptop support is broken. Here's my rookie attempt at fixing it.
Quoted 6 lines
Looks basically sound. You probably want to use
spinlock_irqsave- the spin locks are less overhead than the reader/writer locks and you don't really seem to be using it for anything else. I'm assuming we want the irqsave to block interrupts because the I/O cycles might have to happen one after another - if not they could be relaxed - perhaps Jonathan knows ?
Alrighty then, the patch below uses spinlocks instead of cli() and friends -- to conform to the new irq locking mechanism -- and some minor module changes while we're at it.
drivers/char/toshiba.c +6 -1
@@ -82,7 +82,13 @@static int tosh_fn =0 ; +extern spinlock_t tosh_lock; +MODULE_PARM (tosh_fn,"i" ); +MODULE_PARM_DESC (tosh_fn,"User specified Fn key detection port" ); +MODULE_AUTHOR ("Jonathan Buzzard <[email protected]>" ); +MODULE_DESCRIPTION ("Toshiba laptop SMM driver" ); +MODULE_SUPPORTED_DEVICE ("toshiba" );MODULE_LICENSE ("GPL" );
@@ -114,11 +120,10 @@if (tosh_fn!=0 ) { scan =inb (tosh_fn); }else { -
save_flags(flags);
-
cli();
+
spin_lock_irqsave(&tosh_lock,flags);
outb(0x8e, 0xe4);
scan = inb(0xe5);
-
restore_flags(flags);
+
spin_unlock_irqrestore(&tosh_lock,flags);}
return (int) scan;
@@ -141,35 +146,32 @@
if (tosh_id==0xfccb) {
if (eax==0xfe00) {
/* fan status */ - save_flags(flags); - cli(); + spin_lock_irqsave(&tosh_lock,flags); outb(0xbe, 0xe4); al = inb(0xe5); - restore_flags(flags); + spin_unlock_irqrestore(&tosh_lock,flags); regs->eax = 0x00; regs->ecx = (unsigned int) (al & 0x01);
}
if ((eax==0xff00) && (ecx==0x0000)) {
/* fan off */ - save_flags(flags); - cli(); + spin_lock_irqsave(&tosh_lock,flags); outb(0xbe, 0xe4); al = inb(0xe5); outb(0xbe, 0xe4); outb (al | 0x01, 0xe5); - restore_flags(flags); + spin_unlock_irqrestore(&tosh_lock,flags); regs->eax = 0x00; regs->ecx = 0x00;
}
if ((eax==0xff00) && (ecx==0x0001)) {
/* fan on */ - save_flags(flags); - cli(); + spin_lock_irqsave(&tosh_lock,flags); outb(0xbe, 0xe4); al = inb(0xe5); outb(0xbe, 0xe4); outb(al & 0xfe, 0xe5); - restore_flags(flags); + spin_unlock_irqrestore(&tosh_lock,flags); regs->eax = 0x00; regs->ecx = 0x01;
}
@@ -180,33 +182,30 @@
if (tosh_id==0xfccc) {
if (eax==0xfe00) {
/* fan status */ - save_flags(flags); - cli(); + spin_lock_irqsave(&tosh_lock,flags); outb(0xe0, 0xe4); al = inb(0xe5); - restore_flags(flags); + spin_unlock_irqrestore(&tosh_lock,flags); regs->eax = 0x00; regs->ecx = al & 0x01;
}
if ((eax==0xff00) && (ecx==0x0000)) {
/* fan off */ - save_flags(flags); - cli(); + spin_lock_irqsave(&tosh_lock,flags); outb(0xe0, 0xe4); al = inb(0xe5); outw(0xe0 | ((al & 0xfe) << 8), 0xe4); - restore_flags(flags); + spin_unlock_irqrestore(&tosh_lock,flags); regs->eax = 0x00; regs->ecx = 0x00;
}
if ((eax==0xff00) && (ecx==0x0001)) {
/* fan on */ - save_flags(flags); - cli(); + spin_lock_irqsave(&tosh_lock,flags); outb(0xe0, 0xe4); al = inb(0xe5); outw(0xe0 | ((al | 0x01) << 8), 0xe4); - restore_flags(flags); + spin_unlock_irqrestore(&tosh_lock,flags); regs->eax = 0x00; regs->ecx = 0x01;
}