sparse - make __CHECK_ENDIAN__ default enabled?

sparse - make __CHECK_ENDIAN__ default enabled?

[Sam Ravnborg]

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

Re: sparse - make __CHECK_ENDIAN__ default enabled?

[Harvey Harrison]

On Wed, 2008-02-20 at 23:03 +0100, Sam Ravnborg wrote:
> 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?

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?

I'll look them over tonight and see how bad it would be.


> 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.

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

[PATCH 0/2] firewire: endinaness warnings (was Re: sparse - make __CHECK_ENDIAN__ default enabled?)

[Stefan Richter]

On 20 Feb, Harvey Harrison wrote on LKML:
> On Wed, 2008-02-20 at 23:03 +0100, Sam Ravnborg wrote:
>> 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?
> 
> 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?
> 
> I'll look them over tonight and see how bad it would be.

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/

[PATCH 1/2] firewire: endianess fix

[Stefan Richter]

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/

Re: [PATCH 1/2] firewire: endianess fix

[Stefan Richter]

I wrote:
> If possible, also test whether the device remains accessible after
> forcing a bus reset, e.g. by "echo br short > firecontrol".

"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/

Re: [PATCH 1/2] firewire: endianess fix

[Stefan Richter]

I wrote:
> 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?

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/

Re: [PATCH 1/2] firewire: endianess fix

[Stefan Richter]

On 23 Feb, I wrote:
>> This needs to be tested on different big endian PCs, if possible with
>> the Apple Uninorth FireWire controller and other types of controllers.

I tested it myself now with VT6306 on PPC32.

> 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

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/

Re: [PATCH 1/2] firewire: endianess fix

[Jarod Wilson]

On Saturday 23 February 2008 06:24:17 am Stefan Richter wrote:
> 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?

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

> 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.

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

Re: [PATCH 1/2] firewire: endianess fix

[Stefan Richter]

Jarod Wilson wrote:
> Works just fine with the Apple UniNorth controller in my powerbook in cursory 
> testing.

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/

Re: [PATCH 1/2] firewire: endianess fix

[Jarod Wilson]

On Wednesday 27 February 2008 03:08:32 pm Stefan Richter wrote:
> Jarod Wilson wrote:
> > Works just fine with the Apple UniNorth controller in my powerbook in
> > cursory testing.
>
> Could you remove the OHCI1394_HCControl_postedWriteEnable flag from
> fw-ohci.c::ohci_enable() and test without and with the endianess patch?

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

-- 
Jarod Wilson
jwilson@redhat.com

Re: [PATCH 1/2] firewire: endianess fix

[Benjamin Herrenschmidt]

On Wed, 2008-02-27 at 14:58 -0500, Jarod Wilson wrote:
> On Saturday 23 February 2008 06:24:17 am Stefan Richter wrote:
> > 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?
> 
> Works just fine with the Apple UniNorth controller in my powerbook in cursory 
> testing. Tested with multiple sbp2 hard disks, plugging and unplugging, 
> mounting and unmounting, etc.

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.

Re: [PATCH 1/2] firewire: endianess fix

[Jarod Wilson]

On Wednesday 27 February 2008 09:40:22 pm Benjamin Herrenschmidt wrote:
> On Wed, 2008-02-27 at 14:58 -0500, Jarod Wilson wrote:
> > On Saturday 23 February 2008 06:24:17 am Stefan Richter wrote:
> > > 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?
> >
> > Works just fine with the Apple UniNorth controller in my powerbook in
> > cursory testing. Tested with multiple sbp2 hard disks, plugging and
> > unplugging, mounting and unmounting, etc.
>
> Which specific rev/version of the uninorth controller ?

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

(Its a circa 2004 Aluminum 15" PowerBook G4 @ 1.67GHz, fwiw).

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

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

Re: [PATCH 1/2] firewire: endianess fix

[Benjamin Herrenschmidt]

> 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.

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.

Re: [PATCH 1/2] firewire: endianess fix

[Jarod Wilson]

On Thursday 28 February 2008 01:25:59 am Benjamin Herrenschmidt wrote:
> > 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.
>
> 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.

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

Re: [PATCH 1/2] firewire: endianess fix

[Benjamin Herrenschmidt]

On Thu, 2008-02-28 at 13:42 -0500, Jarod Wilson wrote:
> On Thursday 28 February 2008 01:25:59 am Benjamin Herrenschmidt wrote:
> > > 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.
> >
> > 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.
> 
> 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...

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.

Re: [PATCH 1/2] firewire: endianess fix

[Jarod Wilson]

Benjamin Herrenschmidt wrote:
> On Thu, 2008-02-28 at 13:42 -0500, Jarod Wilson wrote:
>> On Thursday 28 February 2008 01:25:59 am Benjamin Herrenschmidt wrote:
>>>> 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.
>>> 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.
>> 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...
> 
> 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.

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

Re: [PATCH 1/2] firewire: endianess fix

[Benjamin Herrenschmidt]

> 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).

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

Cheers,
Ben.

Re: [PATCH 1/2] firewire: endianess fix

[Paul Mackerras]

Jarod Wilson writes:

> 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).

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.

Re: [PATCH 1/2] firewire: endianess fix

[Stefan Richter]

Paul Mackerras wrote:
> Jarod Wilson writes:
>> 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).
> 
> 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. :)

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/

Re: [PATCH 1/2] firewire: endianess fix

[Benjamin Herrenschmidt]

> 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.

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.

Re: [PATCH 1/2] firewire: endianess fix

[Jarod Wilson]

On Friday 29 February 2008 06:26:34 am Paul Mackerras wrote:
> Jarod Wilson writes:
> > 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).
>
> 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. :)

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

Re: [PATCH 1/2] firewire: endianess fix

[Gabriel Paubert]

On Fri, Feb 29, 2008 at 12:48:33AM -0500, Jarod Wilson wrote:
> Benjamin Herrenschmidt wrote:
> > On Thu, 2008-02-28 at 13:42 -0500, Jarod Wilson wrote:
> >> On Thursday 28 February 2008 01:25:59 am Benjamin Herrenschmidt wrote:
> >>>> 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.
> >>> 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.
> >> 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...
> > 
> > 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.
> 
> 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).

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

Re: [PATCH 1/2] firewire: endianess fix

[Stefan Richter]

Gabriel Paubert wrote:
> 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...).

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
FireWire is dead.  You can get the patches from kernel.org's
linux1394-2.6.git (master branch, close to Linus's latest linux-2.6.git)
or http://me.in-berlin.de/~s5r6/linux1394/updates/ (for a few kernel.org
kernels).

The old stack as found in any remotely recent 2.6 kernel should be OK
out of the box without patches... in theory.  I wouldn't be surprised of
until now unreported bugs or old reported but forgotten bugs though.
-- 
Stefan Richter
-=====-==--- --== ---==
http://arcgraph.de/sr/

Re: [PATCH 1/2] firewire: endianess fix

[Gabriel Paubert]

On Mon, Mar 03, 2008 at 03:35:01PM +0100, Stefan Richter wrote:
> Gabriel Paubert wrote:
> > 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...).
> 
> 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.  :-)

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
on this machine, battery level reads at -1% from /proc/apm.
> 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.

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.

Re: [PATCH 1/2] firewire: endianess fix

[Stefan Richter]

Gabriel Paubert wrote:
>>> 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. 
...
> 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.

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

Re: [PATCH 1/2] firewire: endianess fix

[Gabriel Paubert]

On Thu, Mar 06, 2008 at 12:26:00AM +0100, Stefan Richter wrote:
> Gabriel Paubert wrote:
> >>>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. 
> ...
> >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.
> 
> That's great.  Thanks for testing.

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

Re: [PATCH 1/2] firewire: endianess fix

[Jarod Wilson]

On Wednesday 27 February 2008 02:58:28 pm Jarod Wilson wrote:
> On Saturday 23 February 2008 06:24:17 am Stefan Richter wrote:
> > 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?
>
> Works just fine with the Apple UniNorth controller in my powerbook in
> cursory testing. Tested with multiple sbp2 hard disks, plugging and
> unplugging, mounting and unmounting, etc.
>
> > 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.
>
> Forgot to check that it survived bus resets. Will try to double-check that
> tonight.

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


-- 
Jarod Wilson
jwilson@redhat.com

Re: [PATCH 1/2] firewire: endianess fix

[Benjamin Herrenschmidt]

On Sat, 2008-02-23 at 12:24 +0100, Stefan Richter wrote:
> 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?

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.

Re: [PATCH 1/2] firewire: endianess fix

[Stefan Richter]

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

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/

[PATCH 2/2] firewire: endianess annotations

[Stefan Richter]

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 +=
-			(le32_to_cpu(*(u32 *)(p + 4)) >> 16) & 0xffff;
+			(le32_to_cpu(*(__le32 *)(p + 4)) >> 16) & 0xffff;
 		p += ctx->base.header_size + 4;
 	}
 	ctx->header_length = i;
Index: linux/drivers/firewire/fw-transaction.c
===================================================================
--- linux.orig/drivers/firewire/fw-transaction.c
+++ linux/drivers/firewire/fw-transaction.c
@@ -751,7 +751,7 @@ handle_topology_map(struct fw_card *card
 		    void *payload, size_t length, void *callback_data)
 {
 	int i, start, end;
-	u32 *map;
+	__be32 *map;
 
 	if (!TCODE_IS_READ_REQUEST(tcode)) {
 		fw_send_response(card, request, RCODE_TYPE_ERROR);
Index: linux/drivers/firewire/fw-transaction.h
===================================================================
--- linux.orig/drivers/firewire/fw-transaction.h
+++ linux/drivers/firewire/fw-transaction.h
@@ -85,12 +85,12 @@
 static inline void
 fw_memcpy_from_be32(void *_dst, void *_src, size_t size)
 {
-	u32 *dst = _dst;
-	u32 *src = _src;
+	u32    *dst = _dst;
+	__be32 *src = _src;
 	int i;
 
 	for (i = 0; i < size / 4; i++)
-		dst[i] = cpu_to_be32(src[i]);
+		dst[i] = be32_to_cpu(src[i]);
 }
 
 static inline void

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

Re: [PATCH 0/2] firewire: endinaness warnings (was Re: sparse - make __CHECK_ENDIAN__ default enabled?)

[Jarod Wilson]

On Saturday 23 February 2008 06:23:30 am Stefan Richter wrote:
> On 20 Feb, Harvey Harrison wrote on LKML:
> > On Wed, 2008-02-20 at 23:03 +0100, Sam Ravnborg wrote:
> >> 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?
> >
> > 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?
> >
> > I'll look them over tonight and see how bad it would be.
>
> 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.

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

Re: sparse - make __CHECK_ENDIAN__ default enabled?

[Harvey Harrison]

On Wed, 2008-02-20 at 23:03 +0100, Sam Ravnborg wrote:
> 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?

>From a quick test, the same places that spew sparse warnings, spew
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

Re: sparse - make __CHECK_ENDIAN__ default enabled?

[Sam Ravnborg]

On Wed, Feb 20, 2008 at 02:39:46PM -0800, Harvey Harrison wrote:
> On Wed, 2008-02-20 at 23:03 +0100, Sam Ravnborg wrote:
> > 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?
> 
> >From a quick test, the same places that spew sparse warnings, spew
> 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.

OK - thanks for looking into it.

	Sam