Message-ID: <45768116.8040804@s5r6.in-berlin.de>
Date:	Wed, 06 Dec 2006 09:36:38 +0100
From:	Stefan Richter <stefanr@...6.in-berlin.de>
To:	Kristian Høgsberg <krh@...hat.com>
CC:	Alexey Dobriyan <adobriyan@...il.com>,
	linux-kernel@...r.kernel.org,
	linux1394-devel <linux1394-devel@...ts.sourceforge.net>
Subject: Re: [PATCH 0/3] New firewire stack

(Adding Cc: linux1394-devel)

Kristian Høgsberg wrote to linux-kernel:
> Alexey Dobriyan wrote:
>> On Tue, Dec 05, 2006 at 12:22:29AM -0500, Kristian Høgsberg wrote:
>>> I'm announcing an alternative firewire stack that I've been working
>>> on the last few weeks.
>>
>> Is mainline firewire so hopeless, that you've decided to rewrite it?
>> Could you show some ugly places in it?
> 
> Yes.  I'm not doing this lightheartedly.  It's a lot of work and it will
> introduce regressions and instability for a little while.
> 
> My main point about ohci1394 (the old stacks PCI driver) is, that if you
> really want to fix the issues with this driver, you have to shuffle the code
> around so much that you'll introduce as many regressions as a clean rewrite.
> The big problems in the ohci1394 drivers is the irq_handler, bus reset
> handling and config rom handling.  These are some of the strong points of
> fw-ohci.c:
> 
>  - Rock solid handling of generations and node IDs around bus resets.
>    The only way to handle this atomically is to pass the generation
>    count along all the way to the transmit function in the low-level
>    driver.  The linux1394 low level driver API is broken in this
>    respect.
> 
>  - Better handling of self ID receive and possible recursive bus
>    resets.  Successive bus resets could overwrite the self ID DMA
>    buffer, while we read out the contents.  The OHCI specification
>    recommends a method similiar to linux/seqlock.h for reading out
>    self IDs, to ensure we get a consistent result.
> 
>  - Much simpler bus reset handling; we only subscribe to the
>    selfIDComplete interrupt and don't use the troublesome busReset
>    interrupt.  Rely on async transmit context to not send data while
>    busReset event bit is set.
> 
>  - Atomic updates of config rom contents as specified in section 5.5.6
>    in the OHCI specification. The contents of the ConfigROMheader,
>    BusOptions and ConfigROMmap registers are updated atomically by the
>    controller after a reset.
> 
> The OHCI specification describes a number of the techniques to ensure race
> free operation for the above cases, but the ohci1394 driver generally doesn't
> use any of these.  If you want to see ugly code look at the ohci1394 irq
> handler.  Much of the uglyness comes from trying to handle the busReset
> interrupt, so that the mid-level linux1394 stack can fail I/O while the bus
> reset takes place.  Now, OHCI hardware already reliably fails I/O during bus
> reset, so there is no need to complicate the core stack with this extra state,
> and the OHCI driver becomes much simpler and more reliable, since we now just
> need to know when a bus reset has successfully completed.
> 
> Fixing this problem requires significant changes to the ohci1394 driver and
> the mid-level stack, and will destabilize things until we've figure out how to
> work around the odd flaky device out there.  Similar problems exists related
> to sending packets without bus reset races, updating the config rom, and
> reporting self ID packets and all require significant changes to the core
> stack and ohci1394.  All taken together the scale tips towards a rewrite.
> 
> Another point is the various streaming drivers.  There used to be 5 different
> userspace streaming APIs in the linux1394: raw1394, video1394, amdtp, dv1394
> and rawiso.  Recently, amdtp (audio streaming) has been removed, since with
> the rawiso interface, this can be done in userspace.  However the remaining 4
> interfaces have slightly disjoint feature sets and can't really be phased out.

The old iso API of (lib)raw1394 has been marked deprecated and
undocumented in libraw1394's documentation for some time, and will go
away in 2007.

Dv1394 might go away in 2007 too if there is enough effort to move
high-profile users over to rawiso a.k.a. the current iso API of
(lib)raw1394.

I suppose video1394 might get a viable migration path with your new
driver, if you and interested developers put effort into development
(and help with deployment) of a proper replacement.

>  In the long run, supporting 4 different interfaces that does almost the same
> thing isn't feasible.  The streaming interface in my new stack (only
> transmission implemented at this point) can replace all of these interfaces.

You have to look at the matter not only from the POV of API design but
also of deployment and support.

> Finally, some parts aren't actually rewritten, just ported over and
> refactored.  This is the case for the SBP-2 driver.  Functionally, my
> fw-sbp2.c is identical to sbp2.c in the current stack, but I've changed it to
> work with the new interfaces and cleaned up some of the redundancy.
> 
>> We can end up with two not quite working sets of firewire drivers your
>> way.
>>
> You can patch up the current stack to be less flaky, and Stefan has been doing
> a great job at that lately, but it's still fundamentally broken in the ways
> described above.
> 
> While my stack may less stable for the first couple of weeks, these are
> transient issues, such as, say, lack of big endian testing, that are easily
> fixed.  In the long run this new stack is much more maintainable and has a
> bigger potential for stability.
> 
> Kristian
> 

I have to say, the really really old bug reports which piled up for
ohci1394 (high latency of the reset event handler, streaming packets
being mistaken as selfID packets...) and the recently reported ohci1394
bugs (event mask being mysteriously blanked out...) and my lack of
progress with these speak for themselves. (I don't have enough insight
yet, nor enough spare time nor affected hardware to make reasonable
headway.)
-- 
Stefan Richter
-=====-=-==- ==-- --==-
http://arcgraph.de/sr/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists