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

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

[Harvey Harrison]

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();
-- 
1.6.1.rc2.306.ge5d5e

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

[Stefan Richter]

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.
-- 
Stefan Richter
-=====-==--- ==-- -===-
http://arcgraph.de/sr/

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

[Bill Fink]

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

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

[Stefan Richter]

Bill Fink wrote:
> On Sun, 14 Dec 2008, Stefan Richter wrote:
>> Looks like a bug + correct fix indeed.  Apparently dv1394 was never used on big
>> endian PCs.
...
> 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.

Could you try to apply the patch to your kernel to test it?  You can get
it in plaintext e.g. from
http://user.in-berlin.de/~s5r6/linux1394/submitted/843-ieee1934-dv1394-interrupt-enabling_disabling-broken-on-big-endian.patch

> And this shouldn't be phased out until the equivalent functionality is
> added to ffmpeg via the new APIs.

Or perhaps by a dv1394 replacement module which plugs to the V4L2 API.
-- 
Stefan Richter
-=====-==--- ==-- -===-
http://arcgraph.de/sr/

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

[Bill Fink]

On Sun, 14 Dec 2008, Stefan Richter wrote:

> Bill Fink wrote:
> > On Sun, 14 Dec 2008, Stefan Richter wrote:
> >> Looks like a bug + correct fix indeed.  Apparently dv1394 was never used on big
> >> endian PCs.
> ...
> > 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.
> 
> Could you try to apply the patch to your kernel to test it?  You can get
> it in plaintext e.g. from
> http://user.in-berlin.de/~s5r6/linux1394/submitted/843-ieee1934-dv1394-interrupt-enabling_disabling-broken-on-big-endian.patch

I had hoped to test this by now, but due to not feeling well and the
holidays, I haven't been able to.  I still hope to test this later,
but I can't say exactly when.

					-Happy Holidays to all

					-Bill

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

[Bill Fink]

On Tue, 23 Dec 2008, Bill Fink wrote:

> On Sun, 14 Dec 2008, Stefan Richter wrote:
> 
> > Bill Fink wrote:
> > > On Sun, 14 Dec 2008, Stefan Richter wrote:
> > >> Looks like a bug + correct fix indeed.  Apparently dv1394 was never used on big
> > >> endian PCs.
> > ...
> > > 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.
> > 
> > Could you try to apply the patch to your kernel to test it?  You can get
> > it in plaintext e.g. from
> > http://user.in-berlin.de/~s5r6/linux1394/submitted/843-ieee1934-dv1394-interrupt-enabling_disabling-broken-on-big-endian.patch
> 
> I had hoped to test this by now, but due to not feeling well and the
> holidays, I haven't been able to.  I still hope to test this later,
> but I can't say exactly when.

Good news.  I finally got a chance to test the patch on my kernel,
and live DV viewing using xine still worked fine.

						-Thanks

						-Bill

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

[Stefan Richter]

Bill Fink wrote on 2008-12-26:
> Good news.  I finally got a chance to test the patch on my kernel,
> and live DV viewing using xine still worked fine.

Thanks a lot.  If you don't mind, I'll add "Tested-by: Bill Fink
<billfink@mindspring.com>" to the changelog when I enqueue this for
Linus, and add a note that it actually worked for you before as well as
after this patch.
-- 
Stefan Richter
-=====-==--= ---= ---==
http://arcgraph.de/sr/

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

[Bill Fink]

On Sat, 03 Jan 2009, Stefan Richter wrote:

> Bill Fink wrote on 2008-12-26:
> > Good news.  I finally got a chance to test the patch on my kernel,
> > and live DV viewing using xine still worked fine.
> 
> Thanks a lot.  If you don't mind, I'll add "Tested-by: Bill Fink
> <billfink@mindspring.com>" to the changelog when I enqueue this for
> Linus, and add a note that it actually worked for you before as well as
> after this patch.

That's fine.  Although I admit to being mystified how it works both
before and after the patch, since the cpu_to_le32() calls that were
added should result in byte swapping on PPC that wasn't being done
before.  I guess that either the code paths involved aren't actually
being triggered by my xine DV viewing, or there's some fortuitous
palindromic setting of bits.

						-Bill

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

[Stefan Richter]

Bill Fink wrote:
> Although I admit to being mystified how it works both
> before and after the patch, since the cpu_to_le32() calls that were
> added should result in byte swapping on PPC that wasn't being done
> before.  I guess that either the code paths involved aren't actually
> being triggered by my xine DV viewing, or there's some fortuitous
> palindromic setting of bits.

I agree.  If I had more spare time for these things I would try to find
out what's really going on.  But since the patch really looks like the
right thing to do and doesn't regress, I will send it in without further
investigation.
-- 
Stefan Richter
-=====-==--= ---= --=--
http://arcgraph.de/sr/