Hi Harvey.

Can I ask you to look into the worst offenders so we
can make -D__CHECK_ENDIAN__ enabled per default
in the kernel.
Or maybe we should do it anyway?

I made a quick test-run with a x86 64 bit defconfig.
My first thought was that this was just really bad
because the amount of warnings roughly doubled.

But then inspecting it a little closer I could see
that 8 files had an increase of > 100 additional
warnings when we enabled __CHECK_ENDIAN__ and
that smells like easy targets to bring down the noise.

I did not dare do it for an allyesconfig run - I 
am not that brave.

Comments?

	Sam
--

lots more sparse warnings, the places that are not so bad....are not
so bad afterwards either.

There's still so many of the trivial warnings that maybe it's not the
right time for this yet.  Ask me again when we get to the 2.6.26
timeframe, we'll see how many I've gotten rid of by then.

Harvey


--

OK - thanks for looking into it.

	Sam
--

Well, I've got the worst of fs and drivers/ata done so far, still
weeping over the 5500 warnings in drivers. (X86_32 allyesconfig).
People ignore the existing warnings anyway, why not toss a few more
on the pile?


Well, I'm just going through the _trivial_ ones first to try and cut
the noise down a bit.  I think with all the patches I have out there
I've cut a little over 1000 lines off an allyesconfig build on X86_32
so far.


Harvey

--

I looked into drivers/firewire and drivers/ieee1394.  As expected, there
are quite a lot endianess related warnings in the latter because this is
code from way before sparse was regularly used.

There are also a few warnings in the former, even though sparse checks
were run before submission of the whole drivers/firewire stack.  I will
follow up with two patches:
	1/2 firewire: endianess fix
	2/2 firewire: endianess annotations
Whether the "fix" is really a fix remains to be seen; I don't have a big
endian Linux box myself.
-- 
Stefan Richter
-=====-==--- --=- =-===
http://arcgraph.de/sr/

--

Doesn't hurt anything on my end, spb2 devices work, they survive bus resets 
and so on. The funky uninorth controller definitely doesn't work, but it 
didn't before the patch either, and the fix is being chased in another patch 
series.

Signed-off-by: Jarod Wilson <jwilson@redhat.com>


-- 
Jarod Wilson
jwilson@redhat.com
--

Kills warnings from 'make C=1 CHECKFLAGS="-D__CHECK_ENDIAN__" modules':

drivers/firewire/fw-transaction.c:771:10: warning: incorrect type in assignment (different base types)
drivers/firewire/fw-transaction.c:771:10:    expected unsigned int [unsigned] [usertype] <noident>
drivers/firewire/fw-transaction.c:771:10:    got restricted unsigned int [usertype] <noident>
drivers/firewire/fw-transaction.h:93:10: warning: incorrect type in assignment (different base types)
drivers/firewire/fw-transaction.h:93:10:    expected unsigned int [unsigned] [usertype] <noident>
drivers/firewire/fw-transaction.h:93:10:    got restricted unsigned int [usertype] <noident>
drivers/firewire/fw-ohci.c:1490:8: warning: restricted degrades to integer
drivers/firewire/fw-ohci.c:1490:35: warning: restricted degrades to integer
drivers/firewire/fw-ohci.c:1516:5: warning: cast to restricted type

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: sparclinux@vger.kernel.org
Cc: linuxppc-dev@ozlabs.org
---
 drivers/firewire/fw-ohci.c        |    4 ++--
 drivers/firewire/fw-transaction.c |    2 +-
 drivers/firewire/fw-transaction.h |    6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

Index: linux/drivers/firewire/fw-ohci.c
===================================================================
--- linux.orig/drivers/firewire/fw-ohci.c
+++ linux/drivers/firewire/fw-ohci.c
@@ -1487,7 +1487,7 @@ static int handle_ir_dualbuffer_packet(s
 	void *p, *end;
 	int i;
 
-	if (db->first_res_count > 0 && db->second_res_count > 0) {
+	if (db->first_res_count != 0 && db->second_res_count != 0) {
 		if (ctx->excess_bytes <= le16_to_cpu(db->second_req_count)) {
 			/* This descriptor isn't done yet, stop iteration. */
 			return 0;
@@ -1513,7 +1513,7 @@ static int handle_ir_dualbuffer_packet(s
 		memcpy(ctx->header + i + 4, p + 8, ctx->base.header_size - 4);
 		i += ctx->base.header_size;
 		ctx->excess_bytes +=
-			(le...
[ message continues ]

The generation of incoming requests was filled in in wrong byte order on
machines with big endian CPU.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: sparclinux@vger.kernel.org
Cc: linuxppc-dev@ozlabs.org
---

This patch is a shot in the dark, based on a warning when building with
C=1 CHECKFLAGS="-D__CHECK_ENDIAN__".  Is it really a fix, or was the
previous code accidentally correct?

This needs to be tested on different big endian PCs, if possible with
the Apple Uninorth FireWire controller and other types of controllers.
One test which involves ohci->request_generation is simply with an SBP-2
device (harddisk, CD-ROM...).  Does SBP-2 login etc. work?

If possible, also test whether the device remains accessible after
forcing a bus reset, e.g. by "echo br short > firecontrol".  You need
the easy to build utility firecontrol and a libraw1394 with "juju"
backend.  See wiki.linux1394.org for directions.


 drivers/firewire/fw-ohci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/drivers/firewire/fw-ohci.c
===================================================================
--- linux.orig/drivers/firewire/fw-ohci.c
+++ linux/drivers/firewire/fw-ohci.c
@@ -375,7 +375,7 @@ static __le32 *handle_ar_packet(struct a
 	 */
 
 	if (p.ack + 16 == 0x09)
-		ohci->request_generation = (buffer[2] >> 16) & 0xff;
+		ohci->request_generation = (p.header[2] >> 16) & 0xff;
 	else if (ctx == &ohci->ar_request_ctx)
 		fw_core_handle_request(&ohci->card, &p);
 	else

-- 
Stefan Richter
-=====-==--- --=- =-===
http://arcgraph.de/sr/

--

Do we have the workaround for the old Apple UniNorth in the new FW OHCI
driver (for selfID swapping iirc ?)

There are several variants of Apple OHCI's here, but afaik only one with
a "problem".

Cheers,
Ben.


--

According to ohci1394.c, it selfIDs and headers of incoming packets are 
not byte-swapped by the old Apple Uninorth FireWire part.  And no, 
firewire-ohci doesn't have the workaround yet.

It should be trivial to copy'n'paste ohci1394's workaround into fw-ohci, 
but it would be good if someone could test before and after.

BTW, since that code is touched everytime a packet is received, we 
should enclose such a workaround in #ifdef CONFIG_PPC_PMAC && 
CONFIG_PPC32, shouldn't we?  (As a second step after adding the workaround.)
-- 
Stefan Richter
-=====-==--- --=- ===--
http://arcgraph.de/sr/
--

Works just fine with the Apple UniNorth controller in my powerbook in cursory 
testing. Tested with multiple sbp2 hard disks, plugging and unplugging, 

Forgot to check that it survived bus resets. Will try to double-check that 
tonight. I don't have any other ppc systems w/firewire controllers handy at 
the moment to try other controllers, but can probably manage to get one set 
up soon...

-- 
Jarod Wilson
jwilson@redhat.com
--

Survives bus resets just fine, including with ongoing I/O from an sbp2 disk.


-- 
Jarod Wilson
jwilson@redhat.com
--

Which specific rev/version of the uninorth controller ?

There is the "interesting" one has vendorID Apple and deviceID 0x0018,
the normal ones have different deviceIDs (and are just lucent
controllers afaik).

Ben.


--

lspci says Apple Computer Inc. UniNorth 2 FireWire (rev 81), pci id 106b:0031, 
subsys id 106b:5811.


Under Mac OS X, system.log says "FireWire (OHCI) Apple ID 31 built-in now 
active". Could still be lucent though, judging by the subsys device ID of 
5811, which matches up w/the Lucent/Agere FW323. But no, apparently I don't 
have the interesting one.


-- 
Jarod Wilson
jwilson@redhat.com
--

Well, it's interesting in the sense that it's a "normal" OHCI then on a
BE machine :-) My Pismo, which had the weirdo one, unfortunately died a
while ago. I'll see if I can find another machine with that one in.

Cheers,
Ben.


--

Ah, the pismo has it, eh? I think I may actually know of someone in the office 
that still has one of those that I might be able to borrow and poke at...


-- 
Jarod Wilson
jwilson@redhat.com
--

I -think- it has it... Pismo definitely has one of the first variant of
UniNorth with "working" FW afaik.

The first UniNorth was used in the first "toilet-seat" ibook, but I
think this one didn't have firewire, or a non-working one... and in the
first Sawtooth G4 for which FW and Ethernet even were separate PCI chips
because the ones in UniNorth were too broken.

It's possible that early G4 titanium powerbooks or other model of FW
iBooks have that UniNorth FW variant too.

Ben.


--

Still no luck finding one here. The person I was thinking of has a 
Lombard, which has no firewire. I did get ahold of a 667MHz Titanium, 
but its got an Agere FW323. Pretty sure my old man actually has a Pismo, 
but its about a 3000 mile drive over to my folks house. The search 
continues... I wonder how many people still actually 1) have a machine 
with this controller, 2) are running Linux on it and 3) use firewire 
devices with it. Both of you, please speak up, we're trying to help you! 
(if only out of morbid curiosity to see this mythical goofy controller).

-- 
Jarod Wilson
jwilson@redhat.com
--

Definitely yes to 1) and 2), I have a Pismo which I use on a virtually
daily basis (and about to remove the last remnants of MacOS on it). 
However I have disabled Firewire because it would not sleep and wake 
up properly. 

I can test it on Wednesday with a 5GB fireflly disk from 2001.

Please tell me which configuration options I need to set for
Firewire (which stack, etc...).

	Regards,
	Gabriel
--

Config options of the new stack:
FIREWIRE=m
FIREWIRE_OHCI=m
FIREWIRE_SBP2=m

Config options of the old stack:
IEEE1394=m
IEEE1394_OHCI1394=m
IEEE1394_SBP2=m
and if desired also the other drivers for raw userspace access,
isochronous I/O (alias video1394 even though it can also be used for
other purposes), DV I/O, and IPv4 over 1394.

The two SBP2 drivers also need SCSI and BLK_DEV_SD a.k.a. SCSI disk
support or/and BLK_DEV_SR a.k.a. SCSI CDROM support.

You can also set the options to Y but I find loadable and hence
unloadable modules more practical... 'cause I unload and reload them all
the time.  :-)

Regarding which of the two stacks to build:  If you are going to test
FireWire with a storage device only, then you can choose either stack.
Caveats:
  - You could build and install both stacks but should then blacklist
    at least one of ohci1394 or firewire-ohci.  Better keep it simple
    and install only one of the stacks at a time.
  - We still have a serious use-after-free bug in the new stack.  This
    may lead to kernel panic if the kernel was build with (slab? or
    page allocation?) debugging enabled.
Users of IP over 1394 and pro/semipro audio still need the old stack.
Users of video should stick with the stack which their distribution has
enabled because our support in the lowlevel libraries libraw1394 and
libdc1394 to switch between the stacks is not quite comfortable yet.

Suspend (to RAM) and resume worked for me [TM] when I last tested them
with each stack.  I tested i586/APM, x86-64/ACPI, and last weekend ppc32
on 1st generation PowerBook G4.  I haven't tested hibernate (to disk)
and restore yet.

For successful resume on the Pismo with the new stack, you will most
certainly need the brand new patches which add PPC_PMAC platform support
to firewire-ohci.  From what I saw with the PowerBook, you will also
need the UniNorth rev1 related patch.  I wasn't able to fully test that
patch though because the electrical interface of my PowerBook's onboard
Fir...
[ message continues ]

Indeed, although this machine typically had non-modular kernels, I
compiled one for these tests.

For now I have only tested the new stack with a 6 year old 1.8" disk
and everything works, including suspend to RAM. The kernel is 2.6.25-rc4
plus additional pull from linux1394-2.6.git: 2.6.25-rc4-00032-g8d36ba4.

Thanks a lot.

	Regards,
	Gabriel

P.S: it seems that something broke in the APM emulation around rc2

I have never used suspend to disk on this machine. Suspend to RAM
failed when ieee1394 was loaded (or built-in) since 2.6.22 or so. 

For now I have only tested the new stack with a 6 year old 1.8" disk
and everything works, including suspend to RAM. The kernel is 2.6.25-rc4
plus additional pull from linux1394-2.6.git: 2.6.25-rc4-00032-g8d36ba4.

Thanks a lot.

	Regards,
	Gabriel

P.S: it seems that something broke in the APM emulation around rc2
on this machine, battery level reads at -1% from /proc/apm.
--

That's great.  Thanks for testing.
-- 
Stefan Richter
-=====-==--- --== --==-
http://arcgraph.de/sr/
--

The old stack also works. I forgot to mention that with the new
stack (not with the old) there is a message when going to sleep:

firewire_ohci: pci_set_power_state failed with -5

but it is harmless, and if I understand correctly due to the 
fact that the power management on this chip is not implemented 
through capability fields in the PCI configuration space.

	Gabriel
--

I have a first-generation titanium powerbook that has this controller
(assuming we're talking about vendor/device id = 0x106b / 0x18), and
yes I run Linux (only) on it and use firewire disks. :)

Paul.
--

Yup, seems that's the one. Sounds like we had another one hiding in plain site 
in Stefan's hands too, the thing just was meeting criterion #2. ;)


-- 
Jarod Wilson
jwilson@redhat.com
--

I actually have a TiBook 400 myself, but so far without Linux, and its 
FireWire PHY is dead.  But I can use CardBus FireWire cards on it to do 
basic testing on a big endian PC, and I can test the selfID 
byte-swapping by the PHY-less onboard controller.

I now started a Fedora 8 live CD (self-test says the medium is 
corrupt... need to burn another one) and dmesg says:
firewire_ohci: Added fw-ohci device 0002:24:0e.0, OHCI version 1.0
firewire_ohci: recursive bus reset detected, discarding self ids
[...]

The second line looks like this is indeed one of those which needs the 
header byte-swap workaround which ohci1394 has but firewire-ohci hasn't yet.

On the weekend I'm going to attempt to put Linux on this PowerBook, at last.
-- 
Stefan Richter
-=====-==--- --=- ===-=
http://arcgraph.de/sr/
--

Those machines can netboot, or boot from USB. Might be easier for you.

For that sort of test, usually, I just netboot a zImage with built-in
initrd, using an initrd with all the tools I need on it.

(A way to build that sort of initrd or initramfs is OpenWRT though I use
a home made one).

You can netboot any newworld mac with that OF command:

boot enet:tftp_server_ip,filename

(you need to make sure you have a DHCP around to give it an address).

However, make _SURE_ you use the file arch/powerpc/boot/zImage.pmac (and
not any other zImage).

To build one with an initrd included, put the initrd in
arch/powerpc/boot as "ramdisk.image.gz" and do make zImage.initrd.

That should result in a file name zImage.initrd.pmac that you can
netboot.

Cheers,
Ben.


--

First gen titanium (400/500 Mhz models) might... Paulus has one at home
I think, I'll chase that up here.

Cheers,
Ben.


--

Could you remove the OHCI1394_HCControl_postedWriteEnable flag from 
fw-ohci.c::ohci_enable() and test without and with the endianess patch?
-- 
Stefan Richter
-=====-==--- --=- ==-==
http://arcgraph.de/sr/
--

Sure, added to the todo list along with making sure things survive bus resets.

-- 
Jarod Wilson
jwilson@redhat.com
--

Hmm, no, tests with SBP-2 devices won't trigger that problem at all.
All of the requests from the device will be:
   - read requests to the host's config ROM, handled by the controller's
     physical response unit instead of the driver stack's response
     handlers,
   - read requests to ORBs and read and write requests to SCSI data
     buffers which are DMA-mapped at bus addresses below 4G, hence be
     handled by the physical response unit as well,
   - write requests to firewire-sbp2's status FIFO which is mapped into
     a FireWire address range for which PCI write posting is enabled,
     hence no response subaction will be generated (unified transaction)
     and therefore ohci->request_generation remain unused.

Alas I have no idea how to create a simple test setup which really 
triggers the questionable code.  Or wait, it should be triggered by 
replacing
	&fw_high_memory_region
by
	&fw_private_region
in drivers/firewire/fw-sbp2.c and testing with any SBP-2 device which is 
_not_ based on the PL3507 bridge chip.  This moves the status FIFO into 
an area outside of PCI write posting and forces the driver stack to 
generate response packets.  (Some PL-3507 only accept unified 
transactions, hence this test needs to be performed with other bridge 
chips.)
-- 
Stefan Richter
-=====-==--- --=- =-===
http://arcgraph.de/sr/
--

This indeed demonstrates the fix.  Any IO to SBP-2 devices fails with
timeouts.  Just removing the posted write enable bit in fw-ohci wasn't
sufficient to catch it though.  Maybe the controller has write posting
enabled by default.

However, this endianess bug was low-profile because there are currently
no kernelspace or userspace drivers for the firewire stack which need to
respond in split transactions.
-- 
Stefan Richter
-=====-==--- --== ----=
http://arcgraph.de/sr/

--

"echo br short | firecontrol" of course.

This test should actually not really be necessary because simply
plugging the SBP-2 device in should already cause enough generation
changes for the code to trip over an endianess bug before or after thhe
patch, me thinks.
-- 
Stefan Richter
-=====-==--- --=- =-===
http://arcgraph.de/sr/

--