2005-11-15 09:31:12

by Markus Lidel

[permalink] [raw]
Subject: [PATCH 2/5] I2O: SPARC fixes

Changes:
--------
- Fixed lot of BE <-> LE bugs which prevent it from working on SPARC.

Signed-off-by: Markus Lidel <[email protected]>


Attachments:
i2o-sparc.patch (12.68 kB)

2005-11-15 21:28:37

by David Miller

[permalink] [raw]
Subject: Re: [PATCH 2/5] I2O: SPARC fixes

From: Markus Lidel <[email protected]>
Date: Tue, 15 Nov 2005 10:31:09 +0100

> +config I2O_LCT_NOTIFY_ON_CHANGES
> + bool "Enable LCT notification"
> + depends on I2O
> + default y
> + ---help---
> + Only say N here if you have a I2O controller from SUN. The SUN
> + firmware doesn't support LCT notification on changes. If this option
> + is enabled on such a controller the driver will hang up in a endless
> + loop. On all other controllers say Y.
> +
> + If unsure, say Y.
> +

This should be detected at runtime, and that is easily done.

You can use the PCI device to get at the firmware device
node, and use that to look for a firmware device node property
that identifies it as a card from Sun.

Usually the "name" property has some identifying string in it.
Sometimes there is a property with the string "fcode" in it and you
could look for that as well.

2005-11-16 12:25:56

by Markus Lidel

[permalink] [raw]
Subject: Re: [PATCH 2/5] I2O: SPARC fixes

Hello,

David S. Miller wrote:
> From: Markus Lidel <[email protected]>
>>+config I2O_LCT_NOTIFY_ON_CHANGES
>>+ bool "Enable LCT notification"
>>+ depends on I2O
>>+ default y
>>+ ---help---
>>+ Only say N here if you have a I2O controller from SUN. The SUN
>>+ firmware doesn't support LCT notification on changes. If this option
>>+ is enabled on such a controller the driver will hang up in a endless
>>+ loop. On all other controllers say Y.
>>+
>>+ If unsure, say Y.
>>+
>
> This should be detected at runtime, and that is easily done.
> You can use the PCI device to get at the firmware device
> node, and use that to look for a firmware device node property
> that identifies it as a card from Sun.
> Usually the "name" property has some identifying string in it.
> Sometimes there is a property with the string "fcode" in it and you
> could look for that as well.

OK, i'll look at it... Thanks for the hint!



Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone: +49 82 82/99 51-0
Fax: +49 82 82/99 51-11

E-Mail: [email protected]
URL: http://www.shadowconnect.com

2005-11-16 19:18:44

by David Miller

[permalink] [raw]
Subject: Re: [PATCH 2/5] I2O: SPARC fixes

From: Markus Lidel <[email protected]>
Date: Wed, 16 Nov 2005 13:25:50 +0100

> > This should be detected at runtime, and that is easily done.
> > You can use the PCI device to get at the firmware device
> > node, and use that to look for a firmware device node property
> > that identifies it as a card from Sun.
> > Usually the "name" property has some identifying string in it.
> > Sometimes there is a property with the string "fcode" in it and you
> > could look for that as well.
>
> OK, i'll look at it... Thanks for the hint!

Actually, my idea won't work if the card is used in a non-Sparc
system. Do these cards have PCI subsystem vendor or device ID's that
identify it as a Sun card?

2005-11-17 08:12:43

by Markus Lidel

[permalink] [raw]
Subject: Re: [PATCH 2/5] I2O: SPARC fixes

Hello,

David S. Miller wrote:
>>>This should be detected at runtime, and that is easily done.
>>>You can use the PCI device to get at the firmware device
>>>node, and use that to look for a firmware device node property
>>>that identifies it as a card from Sun.
>>>Usually the "name" property has some identifying string in it.
>>>Sometimes there is a property with the string "fcode" in it and you
>>>could look for that as well.
>>OK, i'll look at it... Thanks for the hint!
> Actually, my idea won't work if the card is used in a non-Sparc
> system. Do these cards have PCI subsystem vendor or device ID's that
> identify it as a Sun card?

I don't know, because currently i only look for the PCI class. I'll check
it out and report back...

Thank you very much for your help!



Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone: +49 82 82/99 51-0
Fax: +49 82 82/99 51-11

E-Mail: [email protected]
URL: http://www.shadowconnect.com

2005-11-19 01:07:49

by Markus Lidel

[permalink] [raw]
Subject: Re: [PATCH 2/5] I2O: SPARC fixes

Hello,

David S. Miller wrote:
> From: Markus Lidel <[email protected]>
> Date: Wed, 16 Nov 2005 13:25:50 +0100
>>>This should be detected at runtime, and that is easily done.
>>>You can use the PCI device to get at the firmware device
>>>node, and use that to look for a firmware device node property
>>>that identifies it as a card from Sun.
>>>Usually the "name" property has some identifying string in it.
>>>Sometimes there is a property with the string "fcode" in it and you
>>>could look for that as well.
>>OK, i'll look at it... Thanks for the hint!
> Actually, my idea won't work if the card is used in a non-Sparc
> system. Do these cards have PCI subsystem vendor or device ID's that
> identify it as a Sun card?

Here's the output of lspci:

0003:01:03.0 Memory controller: Adaptec (formerly DPT) Domino RAID Engine
(rev 02)
Subsystem: Adaptec (formerly DPT) Domino RAID Engine
Flags: bus master, medium devsel, latency 32, IRQ 0082efe0
BIST result: 00
Memory at 000001c980100000 (32-bit, non-prefetchable) [size=64K]
Memory at 000001c988000000 (32-bit, prefetchable) [size=128M]

0003:01:04.0 I2O: Adaptec (formerly DPT) SmartRAID V Controller (rev 03)
Subsystem: Adaptec (formerly DPT) SmartRAID V Controller
Flags: bus master, medium devsel, latency 1, IRQ 0082ef80
BIST result: 00
Memory at 000001c990000000 (32-bit, non-prefetchable) [size=2M]
I/O ports at 0000000002010400 [size=256]
Expansion ROM at 0000000080000000 [disabled] [size=32K]

As it looks it's equal to the "Intel" based cards...

Don't think it will work then, right?



Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone: +49 82 82/99 51-0
Fax: +49 82 82/99 51-11

E-Mail: [email protected]
URL: http://www.shadowconnect.com

2005-11-19 01:22:52

by David Miller

[permalink] [raw]
Subject: Re: [PATCH 2/5] I2O: SPARC fixes

From: Markus Lidel <[email protected]>
Date: Sat, 19 Nov 2005 02:07:39 +0100

> Here's the output of lspci:
>
> 0003:01:03.0 Memory controller: Adaptec (formerly DPT) Domino RAID Engine
> (rev 02)
> Subsystem: Adaptec (formerly DPT) Domino RAID Engine
> Flags: bus master, medium devsel, latency 32, IRQ 0082efe0
> BIST result: 00
> Memory at 000001c980100000 (32-bit, non-prefetchable) [size=64K]
> Memory at 000001c988000000 (32-bit, prefetchable) [size=128M]
>
> 0003:01:04.0 I2O: Adaptec (formerly DPT) SmartRAID V Controller (rev 03)
> Subsystem: Adaptec (formerly DPT) SmartRAID V Controller
> Flags: bus master, medium devsel, latency 1, IRQ 0082ef80
> BIST result: 00
> Memory at 000001c990000000 (32-bit, non-prefetchable) [size=2M]
> I/O ports at 0000000002010400 [size=256]
> Expansion ROM at 0000000080000000 [disabled] [size=32K]
>
> As it looks it's equal to the "Intel" based cards...
>
> Don't think it will work then, right?

No, it won't.

Ho hum, I guess keep it a config option for now until we find a
way to auto-detect this reliably.

2005-11-19 02:59:23

by Alan

[permalink] [raw]
Subject: Re: [PATCH 2/5] I2O: SPARC fixes

On Gwe, 2005-11-18 at 17:22 -0800, David S. Miller wrote:
> Ho hum, I guess keep it a config option for now until we find a
> way to auto-detect this reliably.

The notify functionality is mandatory. You are seeing the same cards
fail on sparc but work on x86. This sounds to me a lot more like an
unfound endian bug that needs fixing than a real lack of support

2005-11-19 04:37:42

by David Miller

[permalink] [raw]
Subject: Re: [PATCH 2/5] I2O: SPARC fixes

From: Alan Cox <[email protected]>
Date: Sat, 19 Nov 2005 03:30:39 +0000

> On Gwe, 2005-11-18 at 17:22 -0800, David S. Miller wrote:
> > Ho hum, I guess keep it a config option for now until we find a
> > way to auto-detect this reliably.
>
> The notify functionality is mandatory. You are seeing the same cards
> fail on sparc but work on x86. This sounds to me a lot more like an
> unfound endian bug that needs fixing than a real lack of support

That's very possible, but it also could be that the cards
that fail only on Sparc have Sun forth firmware on them,
which would thus only load firmware on Sparc boxes.

I still think the endianness theory is more likely, however.

Perhaps the I2O datastructures should be endian annotated
and then pushed through sparse. :-)

2005-11-19 12:46:59

by Alan

[permalink] [raw]
Subject: Re: [PATCH 2/5] I2O: SPARC fixes

On Gwe, 2005-11-18 at 20:37 -0800, David S. Miller wrote:
> That's very possible, but it also could be that the cards
> that fail only on Sparc have Sun forth firmware on them,
> which would thus only load firmware on Sparc boxes.

The firmware on the DPT I2O card processor is kept in flash on the
processor. The BIOS setup code might be different but nothing at the
business end of things

2005-11-20 21:39:04

by Markus Lidel

[permalink] [raw]
Subject: Re: [PATCH 2/5] I2O: SPARC fixes

Hi...

Alan Cox wrote:
>> On Gwe, 2005-11-18 at 17:22 -0800, David S. Miller wrote:
>>>Ho hum, I guess keep it a config option for now until we find a
>>>way to auto-detect this reliably.
>> The notify functionality is mandatory. You are seeing the same cards
>> fail on sparc but work on x86. This sounds to me a lot more like an
>> unfound endian bug that needs fixing than a real lack of support
>>That's very possible, but it also could be that the cards
>>that fail only on Sparc have Sun forth firmware on them,
>>which would thus only load firmware on Sparc boxes.
> The firmware on the DPT I2O card processor is kept in flash on the
> processor. The BIOS setup code might be different but nothing at the
> business end of things

That's right there are two different firmwares. One for "Intel"-cards and
one for SUN-cards. The LCT-Notify function works as follow.

The LCT on the I2O controller has a change indicator which is incremented
by the I2O controller each time something changes (e. g. a disk is
removed or added or so). The i2o_exec_lct_notify() function only send a
number to the I2O controller. If this number is <= then the current
change indicator of the LCT, then the controller send out the new LCT to
the OS. On startup the change indicator is normally 1. So if there is
some BE<->LE issue, then the function wouldn't send in 0x00000002 but
0x02000000. What should happen then is that you won't be notified for a
long time. But that didn't happened, and the controller immediately send
out the LCT, although if you send in 0xffffffff. And as i'm told, this is
wanted for some reason for the "SUN"-firmware.

It should probably be "fixed" if you upload a "Intel"-firmware onto the
I2O controller, but because i don't own a SPARC machine with a PCI slot
myself, i don't want to try it out on someones else machine and probably
break it :-)

If someone has a SPARC machine with an I2O controller, and he want to try
it out, please let me know...

Bye...
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone: +49 82 82/99 51-0
Fax: +49 82 82/99 51-11

E-Mail: [email protected]
URL: http://www.shadowconnect.com

2005-11-20 21:42:13

by Markus Lidel

[permalink] [raw]
Subject: Re: [PATCH 2/5] I2O: SPARC fixes

Hi...

David S. Miller wrote:
> From: Alan Cox <[email protected]>
> Date: Sat, 19 Nov 2005 03:30:39 +0000
>>On Gwe, 2005-11-18 at 17:22 -0800, David S. Miller wrote:
>>>Ho hum, I guess keep it a config option for now until we find a
>>>way to auto-detect this reliably.
>>The notify functionality is mandatory. You are seeing the same cards
>>fail on sparc but work on x86. This sounds to me a lot more like an
>>unfound endian bug that needs fixing than a real lack of support
> That's very possible, but it also could be that the cards
> that fail only on Sparc have Sun forth firmware on them,
> which would thus only load firmware on Sparc boxes.
> I still think the endianness theory is more likely, however.
> Perhaps the I2O datastructures should be endian annotated
> and then pushed through sparse. :-)

Sounds good to me. Could i then find out if some BE<=>LE issues are still
left? If so, is there a document which describes how it is done, or at
least has a driver added it already?

Bye...
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone: +49 82 82/99 51-0
Fax: +49 82 82/99 51-11

E-Mail: [email protected]
URL: http://www.shadowconnect.com

2005-11-20 22:53:04

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH 2/5] I2O: SPARC fixes

On Sun, Nov 20, 2005 at 10:42:09PM +0100, Markus Lidel wrote:
> Hi...
> Sounds good to me. Could i then find out if some BE<=>LE issues are still
> left? If so, is there a document which describes how it is done, or at
> least has a driver added it already?

Hmm...

struct i2o_message {
union {
struct {
u8 version_offset;
u8 flags;
u16 size;
u32 target_tid:12;
u32 init_tid:12;
u32 function:8;
u32 icntxt; /* initiator context */
u32 tcntxt; /* transaction context */
} s;
u32 head[4];
} u;
/* List follows */
u32 body[0];
};

is one hell of an endianness issue right fscking there.

2005-11-20 23:07:19

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH 2/5] I2O: SPARC fixes

And here's another fun one:
evt->size = size;
evt->tcntxt = le32_to_cpu(msg->u.s.tcntxt);
evt->event_indicator = le32_to_cpu(msg->body[0]);
memcpy(&evt->tcntxt, &msg->u.s.tcntxt, size * 4);
in i2o_driver_dispatch().

We have
struct i2o_event {
struct work_struct work;
struct i2o_device *i2o_dev; /* I2O device pointer from which the
event reply was initiated */
u16 size; /* Size of data in 32-bit words */
u32 tcntxt; /* Transaction context used at
registration */
u32 event_indicator; /* Event indicator from reply */
u32 data[0]; /* Event data from reply */
};

and
in msg tcntxt goes right before body[0]. So we copy two 32bit values
converting to host order and then immediately overwrite them with
unconverted ones.

Looks like these assignments were meant to go *after* memcpy() and
serve as a fixup...

2005-11-20 23:22:03

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH 2/5] I2O: SPARC fixes

On Sun, Nov 20, 2005 at 11:07:14PM +0000, Al Viro wrote:
> And here's another fun one:
> evt->size = size;
> evt->tcntxt = le32_to_cpu(msg->u.s.tcntxt);
> evt->event_indicator = le32_to_cpu(msg->body[0]);
> memcpy(&evt->tcntxt, &msg->u.s.tcntxt, size * 4);
> in i2o_driver_dispatch().

Gaaack... Old code used to be
evt = kmalloc(size * 4 + sizeof(*evt), GFP_ATOMIC);
if (!evt)
return -ENOMEM;
memset(evt, 0, size * 4 + sizeof(*evt));

evt->size = size;
memcpy_fromio(&evt->tcntxt, &msg->u.s.tcntxt,
(size + 2) * 4);

Then it became
evt->size = size;
evt->tcntxt = readl(&msg->u.s.tcntxt);
evt->event_indicator = readl(&msg->body[0]);
memcpy_fromio(&evt->tcntxt, &msg->u.s.tcntxt, size * 4);

See the problem with it? The last copy should be from &msg->body[1] to
evt->data. As it is, we do not copy the last 8 bytes (which might or
might not be a problem) *AND* we overwrite tcntxt and event_indicator
with bus-endian values right after having host-endian ones carefully
assigned to them.

Breakage happened in
diff-tree 61fbfa8129c1771061a0e9f47747854293081c5b (from 34d6e07570ef74b96513145
Author: Markus Lidel <[email protected]>
Date: Thu Jun 23 22:02:11 2005 -0700

[PATCH] I2O: bugfixes and compability enhancements

2005-11-21 00:48:16

by Markus Lidel

[permalink] [raw]
Subject: Re: [PATCH 2/5] I2O: SPARC fixes

Hello,

Al Viro wrote:
> On Sun, Nov 20, 2005 at 11:07:14PM +0000, Al Viro wrote:
>> And here's another fun one:
>> evt->size = size;
>> evt->tcntxt = le32_to_cpu(msg->u.s.tcntxt);
>> evt->event_indicator = le32_to_cpu(msg->body[0]);
>> memcpy(&evt->tcntxt, &msg->u.s.tcntxt, size * 4);
>>in i2o_driver_dispatch().
> Gaaack... Old code used to be
> evt = kmalloc(size * 4 + sizeof(*evt), GFP_ATOMIC);
> if (!evt)
> return -ENOMEM;
> memset(evt, 0, size * 4 + sizeof(*evt));
>
> evt->size = size;
> memcpy_fromio(&evt->tcntxt, &msg->u.s.tcntxt,
> (size + 2) * 4);
> Then it became
> evt->size = size;
> evt->tcntxt = readl(&msg->u.s.tcntxt);
> evt->event_indicator = readl(&msg->body[0]);
> memcpy_fromio(&evt->tcntxt, &msg->u.s.tcntxt, size * 4);
> See the problem with it? The last copy should be from &msg->body[1] to
> evt->data. As it is, we do not copy the last 8 bytes (which might or
> might not be a problem) *AND* we overwrite tcntxt and event_indicator

At the moment it's not a problem, because the event system is not used...

> with bus-endian values right after having host-endian ones carefully
> assigned to them.

Yep, you're right... the memcpy_fromio is wrong... It should be:

memcpy_fromio(&evt->body[1], &msg->body[1], size * 4);

as you already mentioned...

Thanks for your help.


Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone: +49 82 82/99 51-0
Fax: +49 82 82/99 51-11

E-Mail: [email protected]
URL: http://www.shadowconnect.com

2005-11-21 01:20:11

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH 2/5] I2O: SPARC fixes

On Mon, Nov 21, 2005 at 01:48:10AM +0100, Markus Lidel wrote:
> Yep, you're right... the memcpy_fromio is wrong... It should be:
>
> memcpy_fromio(&evt->body[1], &msg->body[1], size * 4);
>
> as you already mentioned...

BTW, should that be memcpy() or memcpy_fromio()? IOW, what memory
does msg point to?

2005-11-21 03:38:54

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH 2/5] I2O: SPARC fixes

On Mon, Nov 21, 2005 at 01:48:10AM +0100, Markus Lidel wrote:
> memcpy_fromio(&evt->body[1], &msg->body[1], size * 4);

evt->data, surely?

2005-11-21 08:46:51

by Markus Lidel

[permalink] [raw]
Subject: Re: [PATCH 2/5] I2O: SPARC fixes

Hello,

Al Viro wrote:
> On Mon, Nov 21, 2005 at 01:48:10AM +0100, Markus Lidel wrote:
>>memcpy_fromio(&evt->body[1], &msg->body[1], size * 4);
> evt->data, surely?

Hehehe, probably i should first try to compile it before replying :-) But
better late then never :-)

Here the proper line:

memcpy(&evt->data, &msg->body[1], size * 4);

Thanks for you help!



Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone: +49 82 82/99 51-0
Fax: +49 82 82/99 51-11

E-Mail: [email protected]
URL: http://www.shadowconnect.com