Re: [PATCH 2/2] ieee1934: dv1394 interrupt enabling/disabling broken on big-endian

[Bill Fink <billfink@mindspring.com>]

On Sun, 14 Dec 2008, Stefan Richter wrote:

> For linux1394-devel to know:
> 
> Harvey Harrison wrote at LKML:
> > After annotating the frame structs, this was left:
> > drivers/ieee1394/dv1394.c:2113:23: warning: invalid assignment: |=
> > drivers/ieee1394/dv1394.c:2113:23:    left side has type restricted __le32
> > drivers/ieee1394/dv1394.c:2113:23:    right side has type int
> > drivers/ieee1394/dv1394.c:2121:24: warning: invalid assignment: &=
> > drivers/ieee1394/dv1394.c:2121:24:    left side has type restricted __le32
> > drivers/ieee1394/dv1394.c:2121:24:    right side has type int
> > drivers/ieee1394/dv1394.c:2123:24: warning: invalid assignment: |=
> > drivers/ieee1394/dv1394.c:2123:24:    left side has type restricted __le32
> > drivers/ieee1394/dv1394.c:2123:24:    right side has type int
> > 
> > Which looks like a real bug on a big-endian arch as it would set/clear
> > the wrong bit.
> > 
> > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> > ---
> >  drivers/ieee1394/dv1394.c |    8 ++++----
> >  1 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/ieee1394/dv1394.c b/drivers/ieee1394/dv1394.c
> > index f258e61..a329e6b 100644
> > --- a/drivers/ieee1394/dv1394.c
> > +++ b/drivers/ieee1394/dv1394.c
> > @@ -2110,17 +2110,17 @@ static void ir_tasklet_func(unsigned long data)
> >  			f = video->frames[next_i / MAX_PACKETS];
> >  			next = &(f->descriptor_pool[next_i % MAX_PACKETS]);
> >  			next_dma = ((unsigned long) block - (unsigned long) f->descriptor_pool) + f->descriptor_pool_dma;
> > -			next->u.in.il.q[0] |= 3 << 20; /* enable interrupt */
> > -			next->u.in.il.q[2] = 0; /* disable branch */
> > +			next->u.in.il.q[0] |= cpu_to_le32(3 << 20); /* enable interrupt */
> > +			next->u.in.il.q[2] = cpu_to_le32(0); /* disable branch */
> >  
> >  			/* link previous to next */
> >  			prev_i = (next_i == 0) ? (MAX_PACKETS * video->n_frames - 1) : (next_i - 1);
> >  			f = video->frames[prev_i / MAX_PACKETS];
> >  			prev = &(f->descriptor_pool[prev_i % MAX_PACKETS]);
> >  			if (prev_i % (MAX_PACKETS/2)) {
> > -				prev->u.in.il.q[0] &= ~(3 << 20); /* no interrupt */
> > +				prev->u.in.il.q[0] &= ~cpu_to_le32(3 << 20); /* no interrupt */
> >  			} else {
> > -				prev->u.in.il.q[0] |= 3 << 20; /* enable interrupt */
> > +				prev->u.in.il.q[0] |= cpu_to_le32(3 << 20); /* enable interrupt */
> >  			}
> >  			prev->u.in.il.q[2] = cpu_to_le32(next_dma | 1); /* set Z=1 */
> >  			wmb();
> 
> Looks like a bug + correct fix indeed.  Apparently dv1394 was never used on big
> endian PCs.  Which is good, because we want to phase out dv1394 eventually.

I use dv1394 on a big-endian PPC system (2.6.15 kernel) to watch DV video
via xine, and this has always worked fine for me.  And this shouldn't be
phased out until the equivalent functionality is added to ffmpeg via the
new APIs.

						-Bill