tp_smapi conflict with IDE, hdaps

tp_smapi conflict with IDE, hdaps

[Shem Multinymous]

Hi,

I'm developing a new kernel module, tp_smapi, for providing access to
special features of ThinkPad laptops via a sysfs interface. See [1]
for details and [2] for the GPL sourcecode.

This module conflicts with two other systems, due to use of common IO resource.

Conflict with IDE system:
One of the functions provided by tp_smapi is setting the CD-ROM
speed+spindown ("hdparm -E" and "eject -x" have no effect on these
laptops). This is achieved by sending an appropriate command to the
laptop's SMAPI BIOS, whose implementation is totally opaque [3].
Evidently, the SMAPI BIOS sends some ATA command to the drive. If the
kernel is accessing the drive at the same time (e.g., an ongoing "cat
/dev/scd0"), the machine hangs. The ideal solution would be to figure
out the relevant ATA commands and add them to libata/ata_piix/ide, but
it's not clear how to do that. So tp_smapi needs to obtain some lock
guaranteeing there is no access (or ongoing transaction) to that ATA
device.

Conflict with the "hdaps" module:
Another function provided by tp_smapi is reporting extended battery
status, including some data not provided through ACPI. This conflict
with the recently added HDAPS accelerometer driver. Both drivers read
their data from the same ports (0x1604-0x161F), which implement a
query-reponse transaction interface, so both drivers talking to the
hardware simultaneously will wreak havoc. Some synchronization is
needed, and a way to address the request_region conflict.

What is standard procedure for resolving such conflicts?

  Shem

[1] http://thinkwiki.org/wiki/SMAPI_support_for_Linux
[2] Current: http://tpctl.sourceforge.net/rel/tp_smapi-0.09.tgz
    Future: http://sf.net/project/showfiles.php?group_id=1212&package_id=171579
[3] The SMAPI BIOS runs in SMM and thus cannot be debugged by mere mortals.
     See tp_smapi's README for known details:
    http://sourceforge.net/project/shownotes.php?release_id=377806&group_id=1212

Re: tp_smapi conflict with IDE, hdaps

[Alan Cox]

On Maw, 2005-12-13 at 16:35 +0200, Shem Multinymous wrote:
> Evidently, the SMAPI BIOS sends some ATA command to the drive. If the
> kernel is accessing the drive at the same time (e.g., an ongoing "cat
> /dev/scd0"), the machine hangs. The ideal solution would be to figure
> out the relevant ATA commands and add them to libata/ata_piix/ide, but
> it's not clear how to do that. So tp_smapi needs to obtain some lock
> guaranteeing there is no access (or ongoing transaction) to that ATA
> device.

You will need to find out the command. Trying to arbitrate libata access
with unknown bios behaviour isn't going to have a sane resolution. There
are standard commands for this so they ought to work. If not we need to
know why, who makes the drive used etc

> with the recently added HDAPS accelerometer driver. Both drivers read
> their data from the same ports (0x1604-0x161F), which implement a
> query-reponse transaction interface, so both drivers talking to the
> hardware simultaneously will wreak havoc. Some synchronization is
> needed, and a way to address the request_region conflict.
> 
> What is standard procedure for resolving such conflicts?

You probably want a low level driver that just arbitrates the interface
and implements the basic query/response transaction interface and
locking and then is called by both HDAPS and your driver (and no doubt
other future drivers talking to that controller). It can thene export
the interface to both drivers.

Re: tp_smapi conflict with IDE, hdaps

[Shem Multinymous]

Hi,

On 12/13/05, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Maw, 2005-12-13 at 16:35 +0200, Shem Multinymous wrote:
> > Evidently, the SMAPI BIOS sends some ATA command to the drive. If the
> > kernel is accessing the drive at the same time (e.g., an ongoing "cat
> > /dev/scd0"), the machine hangs.
>
> You will need to find out the command.

Sure, that would be ideal. But how? You can't get that from the SMAPI
BIOS - it's totally opaque. You just write a constant to port 0xB2,
which triggers an SMI; the BIOS merrily does its thing in SMM and
returns; you see the final results in the CPU registers.

> There are standard commands for this so they ought to work. If not we need to
> know why, who makes the drive used etc

ThinkPad T43 BIOS 1.24, Hitahi HTS726060M9AT00 firmware MH4OA6GA. No
idea how to proceed beyond this.

The thing is, there *is* a working interface, which is also used by
the Windows drivers...


> Trying to arbitrate libata access with unknown bios behaviour isn't going to have a
> sane resolution.

Why? BTW, isn't this similar to the queue freeze functionality needed
by the disk park part of the ThinkPad HDAPS?


> > with the recently added HDAPS accelerometer driver. Both drivers read
> > their data from the same ports (0x1604-0x161F), which implement a
> > query-reponse transaction interface, so both drivers talking to the
> > hardware simultaneously will wreak havoc. Some synchronization is
> > needed, and a way to address the request_region conflict.
> >
> > What is standard procedure for resolving such conflicts?
>
> You probably want a low level driver that just arbitrates the interface
> and implements the basic query/response transaction interface and
> locking and then is called by both HDAPS and your driver (and no doubt
> other future drivers talking to that controller). It can thene export
> the interface to both drivers.

We don't understand the controller interface sufficiently well to
fully abstract it (no specs, and the two conflicting drivers do things
somewhat differently), so for now the low-level driver may only handle
locking... Is there an easier way to just share a mutex?

Anyway, can you point out a minimal example (or two) of such low-level
drivers in the current kernel, so I can imitate the recommended
interface convention?

  Shem

Re: tp_smapi conflict with IDE, hdaps

[Alan Cox]

On Maw, 2005-12-13 at 17:29 +0200, Shem Multinymous wrote:
> Sure, that would be ideal. But how? You can't get that from the SMAPI
> BIOS - it's totally opaque. You just write a constant to port 0xB2,
> which triggers an SMI; the BIOS merrily does its thing in SMM and
> returns; you see the final results in the CPU registers.

Sounds like it needs someone with an ATA bus analyser, or of course
someone from IBM to be helpful

> The thing is, there *is* a working interface, which is also used by
> the Windows drivers...

And the BIOS and driver hackers for IBM wrote both bits and had all the
source and made them aware of each other. We don't have that luxury.

> > Trying to arbitrate libata access with unknown bios behaviour isn't going to have a
> > sane resolution.
> 
> Why? BTW, isn't this similar to the queue freeze functionality needed
> by the disk park part of the ThinkPad HDAPS?

What else does that code do, what else might it confuse, what rules and
locking are hidden in the windows driver that are unknown. Want to risk
everyones data for that ?

HDAPS doesn't need it btw.

> We don't understand the controller interface sufficiently well to
> fully abstract it (no specs, and the two conflicting drivers do things
> somewhat differently), so for now the low-level driver may only handle
> locking... Is there an easier way to just share a mutex?

Yes but that isn't neccessarily the right thing to do. You want the
abstraction for the resource ownership and expansion. Can you summarize
the two drivers interaction with the ports ?

> Anyway, can you point out a minimal example (or two) of such low-level
> drivers in the current kernel, so I can imitate the recommended
> interface convention?

One large scale example is the i2c bus code which has to deal with
multiple devices on multiple busses all being used by multiple people at
the same time.

Another is I2O where the I2O core code owns the I2O controller and the
detail for it and is used by various device drivers on top. That one is
fairly high level however and not exactly minimal.

It may well be that in your case the 'core' module can only identify the
ports, claim them, release them on unload and provide 'lock' and
'unlock' functions and the base address.

Re: tp_smapi conflict with IDE, hdaps

[Shem Multinymous]

Hi,

On 12/13/05, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> Sounds like it needs someone with an ATA bus analyser, or of course
> someone from IBM to be helpful

(I wonder which is more implausible...)


> > > Trying to arbitrate libata access with unknown bios behaviour isn't going to have a
> > > sane resolution.
> >
> > Why? BTW, isn't this similar to the queue freeze functionality needed
> > by the disk park part of the ThinkPad HDAPS?
>
> What else does that code do, what else might it confuse, what rules and
> locking are hidden in the windows driver that are unknown. Want to risk
> everyones data for that ?

We already take that risk to some degree, since the SMAPI BIOS is also
invoked by the ACPI DSDT and by external events.


> HDAPS doesn't need it btw.

It's not implemented yet, but I gather it's necessary for preventing
the disk from spinning back up as the laptop slides off the table.
Maybe I missed some subsequent discussion?


> > We don't understand the controller interface sufficiently well to
> > fully abstract it (no specs, and the two conflicting drivers do things
> > somewhat differently), so for now the low-level driver may only handle
> > locking... Is there an easier way to just share a mutex?
>
> Yes but that isn't neccessarily the right thing to do. You want the
> abstraction for the resource ownership and expansion. Can you summarize
> the two drivers interaction with the ports ?

You write "command" values into IO ports 0x1610 and 0x161F and, in
some cases, read some results from ports 0x1610-0x161F. Throughout
that, you inspect various bits (whose meaning we don't understand) in
the status port 0x1604. The details of the commands, scheduling and
status bits differ between the drivers. I don't think a full-blown
ownership and expansion infrastructure is necessary, or even possible
without better understanding.


> One large scale example is the i2c bus code which has to deal with
> multiple devices on multiple busses all being used by multiple people at
> the same time.
>
> Another is I2O where the I2O core code owns the I2O controller and the
> detail for it and is used by various device drivers on top. That one is
> fairly high level however and not exactly minimal.
>
> It may well be that in your case the 'core' module can only identify the
> ports, claim them, release them on unload and provide 'lock' and
> 'unlock' functions and the base address.

Thanks for the pointers. I guess the minimal approach is probably
ideal here; are there any such dumb drivers lying around?

  Shem

Re: tp_smapi conflict with IDE, hdaps

[Alan Cox]

On Maw, 2005-12-13 at 18:04 +0200, Shem Multinymous wrote:
> > What else does that code do, what else might it confuse, what rules and
> > locking are hidden in the windows driver that are unknown. Want to risk
> > everyones data for that ?
> 
> We already take that risk to some degree, since the SMAPI BIOS is also
> invoked by the ACPI DSDT and by external events.

But the ACPI DSDT is OS independant in theory and in practice to a fair
extent. It can't make assumptions about windows drivers.

> > HDAPS doesn't need it btw.
> 
> It's not implemented yet, but I gather it's necessary for preventing
> the disk from spinning back up as the laptop slides off the table.
> Maybe I missed some subsequent discussion?

HDAPS wants to be able to talk with the IDE/libata layer to request it
to hold off requests, thats very different to "gee I wonder what the
bios did"

> You write "command" values into IO ports 0x1610 and 0x161F and, in
> some cases, read some results from ports 0x1610-0x161F. Throughout
> that, you inspect various bits (whose meaning we don't understand) in
> the status port 0x1604. The details of the commands, scheduling and
> status bits differ between the drivers. I don't think a full-blown
> ownership and expansion infrastructure is necessary, or even possible
> without better understanding.

Ok


> Thanks for the pointers. I guess the minimal approach is probably
> ideal here; are there any such dumb drivers lying around?

Its probably easier to write than go find one


I mean you need


tp_hw_init() {
   if (not a thinkpad) return -ENODEV
   request_region(...)
   return 0
}

tp_hw_exit() {
   release_region(...)
}

EXPORT_SYMBOL_GPL(tp_hw_lock); 
EXPORT_SYMBOL_GPL(tp_hw_unlock);

tp_hw_lock() {
    down(&tp_sem);
}

tp_hw_unlock() {
    up(&tp_sem);
}

/* And perhaps if the port varies by machine */

EXPORT_SYMBOL_GPL(tp_hw_addr);

tp_hw_addr() {
    return 0x1600;
}

It can really be that simple

Re: tp_smapi conflict with IDE, hdaps

[Shem Multinymous]

Hi

On 12/13/05, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> But the ACPI DSDT is OS independant in theory and in practice to a fair
> extent. It can't make assumptions about windows drivers.

The IDE issue is moot in light of SET_STREAMING, but just for the
record: the preinstalled Windows on this ThinkPad uses Microsoft's
default IDE driver and Intel's default 82801FBM driver, so it doesn't
look like it's doing SMAPI-specific synchronization.


> Its probably easier to write than go find one
>
> I mean you need
>
> tp_hw_init() {
[code snipped]

Oh, thanks! That's far less voodoo than I feared.

I'll coordinate things with Robert Love (hdaps maintainer).

  Shem

Re: tp_smapi conflict with IDE, hdaps

[Shem Multinymous]

On 12/13/05, Shem Multinymous <multinymous@gmail.com> wrote:
> ThinkPad T43 BIOS 1.24, Hitahi HTS726060M9AT00 firmware MH4OA6GA.

Oops, sorry, that's the hard disk. The drive is a Matshita UJ-822S
firmware 1.61 (branded by IBM as "UltraBay Slim DVD Multi-Burner
Plus").

Meanwhile, I found out that with this drive, "hdparm -E" does affect
CD-R discs, but not DVD-R discs. The SMAPI BIOS command affects both.

  Shem

Re: tp_smapi conflict with IDE, hdaps

[Alan Cox]

On Maw, 2005-12-13 at 20:41 +0200, Shem Multinymous wrote:
> Meanwhile, I found out that with this drive, "hdparm -E" does affect
> CD-R discs

That is expected behaviour. DVD speed is controlled by different
interfaces

Re: tp_smapi conflict with IDE, hdaps

[Shem Multinymous]

On 12/13/05, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Maw, 2005-12-13 at 20:41 +0200, Shem Multinymous wrote:
> > Meanwhile, I found out that with this drive, "hdparm -E" does affect
> > CD-R discs
> That is expected behaviour. DVD speed is controlled by different
> interfaces

Duh! It's set via SET_STREAMING instead of SELECT_SPEED. There are
even a couple of (rejected?) kernel patches [1][2] and a userspace
tool [3], though neither hdparm nor eject know about it.

Good, so CD/DVD speed is none of tp_smapi's business. Thanks for the info!

  Shem

[1] http://lkml.org/lkml/2005/8/21/55
[2] http://seclists.org/lists/linux-kernel/2005/Aug/7393.html
[3] http://safari.iki.fi/speedcontrol.c

Re: tp_smapi conflict with IDE, hdaps

[Mark Lord]

Shem Multinymous wrote:
..
> Duh! It's set via SET_STREAMING instead of SELECT_SPEED.

Now added onto my TO-DO list for hdparm.

Thanks!

Re: tp_smapi conflict with IDE, hdaps

[Robert Love]

On Tue, 2005-12-13 at 16:35 +0200, Shem Multinymous wrote:

> Conflict with the "hdaps" module:
> Another function provided by tp_smapi is reporting extended battery
> status, including some data not provided through ACPI. This conflict
> with the recently added HDAPS accelerometer driver. Both drivers read
> their data from the same ports (0x1604-0x161F), which implement a
> query-reponse transaction interface, so both drivers talking to the
> hardware simultaneously will wreak havoc. Some synchronization is
> needed, and a way to address the request_region conflict.

Alan's response is the correct course of action here, but I have a
question: What other data in 0x1604-0x161F is there?

	Robert Love

Re: tp_smapi conflict with IDE, hdaps

[Shem Multinymous]

On 12/13/05, Robert Love <rlove@rlove.org> wrote:
> On Tue, 2005-12-13 at 16:35 +0200, Shem Multinymous wrote:
> Alan's response is the correct course of action here,

Ideally, but I'm not sure we understand the interface sufficiently
well to abstract it beyond a mere mutex. Compare your hdaps.c code to
the following relevant code from tp_smapi.c (stripped down a bit for
simplicty):

----------------------------------------
#define APS_ROW_LEN 16
static int read_aps_row(u8 arg1610, u8 arg161F, u8* buf) {
	int retries, i;
	int ret = -EIO;

	for (retries=APS_MAX_RETRIES; retries>0; --retries) {
		if (inb(0x1604)&0x40) { /* readout pending? */
			inb(0x161F); /* discard it */
			udelay(10);
		} else {
			outb(arg1610, 0x1610);
			if (inb(0x1604)&0x20)
				goto wrote1610;
		}
	}
	goto out;
wrote1610:
	outb(arg161F, 0x161F);
	for (retries=APS_MAX_RETRIES; retries>0; --retries) {
		if (inb(0x1604)&0x40) /* readout pending? */
			goto gotdata;
			udelay(10);
	}
	goto out;
gotdata:
	for (i=0; i<APS_ROW_LEN; ++i) {
		buf[i] = inb(0x1610+i);
	}
	ret = 0;
out:
	return ret;
}
----------------------------------------
(Yes, the loop/goto structure should be cleanup up a bit.)

Not to mention the HDAPS init code, which is voodoo.


> question: What other data in 0x1604-0x161F is there?

I looked only at the readout "rows" given by arg1610=1,..,18 and
arg161F=0,1. In that range, I didn't see anything obviously
interesting in the "rows" not already used by the battery readout and
HDAPS. I don't think any of the Windows driver reads other "rows". But
for all we know, this could be a window into the embedded controller's
memory or something of the sorts. Also, some of these "rows" are
actually commands given during HDAPS init, so the "read_row"
abstraction is obviously not accurate.

  Shem