m68k: compile fix - ADBREQ_RAW missing declaration

PATCH In review Al Viro 5 participants 14 messages Dec 15, 2005 – Dec 25, 2005 Linux for power macintosh

lore

Share & tools

lore

Al Viro

author

2005-12-15 08:55 UTC #0 lore raw

PATCH 2/3 m68k: compile fix - ADBREQ_RAW missing declaration

another compile fix, pulled straight from m68k CVS

Signed-off-by Al Viro <[email protected]>

---

 drivers/macintosh/adb.c |    8 +++++---
 include/linux/adb.h     |    1 +
 2 files changed, 6 insertions(+), 3 deletions(-)

d39fe026c456804272741a7c62ae380257e9a879

drivers/macintosh/adb.c +5 -3

index d2ead17..6b7591e 100644
@@ -476,13 +476,15 @@ adb_request(struct adb_request *req, voi
 		use_sreq = 1;
 	} else
 		use_sreq = 0;
-	req->nbytes = nbytes+1;
+	i = (flags & ADBREQ_RAW) ? 0 : 1;
+	req->nbytes = nbytes+i;
 	req->done = done;
 	req->reply_expected = flags & ADBREQ_REPLY;
 	req->data[0] = ADB_PACKET;
 	va_start(list, nbytes);
-	for (i = 0; i < nbytes; ++i)
-		req->data[i+1] = va_arg(list, int);
+	while (i < req->nbytes) {
+		req->data[i++] = va_arg(list, int);
+	}
 	va_end(list);
 
 	if (flags & ADBREQ_NOSEND)

include/linux/adb.h +1 -0

index e9fdc63..aad7b1c 100644
@@ -76,6 +76,7 @@ struct adb_driver {
 #define ADBREQ_REPLY	1	/* expect reply */
 #define ADBREQ_SYNC	2	/* poll until done */
 #define ADBREQ_NOSEND	4	/* build the request, but don't send it */
+#define ADBREQ_RAW	8	/* send raw packet (don't prepend ADB_PACKET) */
 
 /* Messages sent thru the client_list notifier. You should NOT stop
    the operation, at least not with this version */

Signature · 1 line

0.99.9.GIT

Roman Zippel

2005-12-15 12:00 UTC #1 lore raw

Hi,

Quoted 1 line · Al Viro

another compile fix, pulled straight from m68k CVS

Thanks, but if you pull changes out of CVS could you please keep the author intact? CVS may be bad, but it's not that bad.

bye, Roman

Al Viro

author

2005-12-15 17:16 UTC #2 lore raw

Quoted 8 lines · Roman Zippel

Hi,
Quoted 1 line · Al Viro
another compile fix, pulled straight from m68k CVS
Thanks, but if you pull changes out of CVS could you please keep the author intact? CVS may be bad, but it's not that bad.

CVS users, OTOH...
Mon Oct 22 09:34:34 2001 UTC (4 years, 1 month ago) by zippel
Branches: MAIN
CVS tags: m68k-2_5_9, m68k-2_5_8, m68k-2_5_7, m68k-2_5_65,
[snip]
m68k-2_4_13, m68k-2_4_12
Branch point for: m68k-2_4
Diff to previous 1.1: preferred, colored
Changes since revision 1.1: +1 -0 lines
import Geert's 2.4.12 m68k patch

and the same for drivers/macintosh part.

So who should I put as the author? You or Geert (or whatever attributions might have been in said big patch)? Incidentally, ADBREQ_RAW had leaked into mainline (sans definition) in 2.3.45-pre2, which was Feb 13 2000, i.e. more than 1.5 year before your commit, so there's quite a chunk of history missing...

I'm serious, BTW - I certainly would have no problem preserving attribution, but it simply hadn't been there. CVS logs are only as good as the data being put there by committers...

Al Viro

author

2005-12-15 17:47 UTC #3 lore raw

Quoted 9 lines · Al Viro

So who should I put as the author? You or Geert (or whatever attributions might have been in said big patch)? Incidentally, ADBREQ_RAW had leaked into mainline (sans definition) in 2.3.45-pre2, which was Feb 13 2000, i.e. more than 1.5 year before your commit, so there's quite a chunk of history missing...
I'm serious, BTW - I certainly would have no problem preserving attribution, but it simply hadn't been there. CVS logs are only as good as the data being put there by committers...

With some archaeology... It looks like drivers/macintosh part is from Geert (with chunks from benh? not sure) circa Dec 2000; adb.h is a missing piece of earlier patch (one that had leaked in Feb 2000, $DEITY knowns how much older it is)...

Roman Zippel

2005-12-15 17:51 UTC #6 lore raw

In reply to Al Viro

Hi,

Quoted 5 lines · Al Viro

So who should I put as the author? You or Geert (or whatever attributions might have been in said big patch)? Incidentally, ADBREQ_RAW had leaked into mainline (sans definition) in 2.3.45-pre2, which was Feb 13 2000, i.e. more than 1.5 year before your commit, so there's quite a chunk of history missing...

I'd say Geert, but it probably comes from the Mac tree. Anyway, it wouldn't be such a bad idea to ask him first why it's in his postponed queue:

http://linux-m68k-cvs.ubb.ca/~geert/linux-m68k-2.6.x-merging/POSTPONED/130-adbraw.diff

My guess it needs some ack from the ppc people.

bye, Roman

Geert Uytterhoeven

2005-12-15 17:54 UTC #4 lore raw

In reply to Al Viro

Quoted 7 lines · Al Viro

Quoted 6 lines · Al Viro
So who should I put as the author? You or Geert (or whatever attributions might have been in said big patch)? Incidentally, ADBREQ_RAW had leaked into mainline (sans definition) in 2.3.45-pre2, which was Feb 13 2000, i.e. more than 1.5 year before your commit, so there's quite a chunk of history missing...

Quoted 8 lines

Quoted 3 lines
I'm serious, BTW - I certainly would have no problem preserving attribution, but it simply hadn't been there. CVS logs are only as good as the data being put there by committers...
With some archaeology... It looks like drivers/macintosh part is from Geert (with chunks from benh? not sure) circa Dec 2000; adb.h is a missing piece of earlier patch (one that had leaked in Feb 2000, $DEITY knowns how much older it is)...

Nah, I never did Mac development ;-)

IIRC, this change originally came from maclinux CVS (which predates Linux/m68k CVS by several years) via email. If you wait a few hours, the original author probably will start yelling ;-)

Alternatively, I can dig up my email archives from before 2000 to identify him....

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Al Viro

author

2005-12-15 17:55 UTC #7 lore raw

In reply to Roman Zippel

Quoted 10 lines · Roman Zippel

Hi,
Quoted 5 lines · Al Viro
So who should I put as the author? You or Geert (or whatever attributions might have been in said big patch)? Incidentally, ADBREQ_RAW had leaked into mainline (sans definition) in 2.3.45-pre2, which was Feb 13 2000, i.e. more than 1.5 year before your commit, so there's quite a chunk of history missing...

Quoted 7 lines

I'd say Geert, but it probably comes from the Mac tree. Anyway, it wouldn't be such a bad idea to ask him first why it's in his postponed queue:
http://linux-m68k-cvs.ubb.ca/~geert/linux-m68k-2.6.x-merging/POSTPONED/130-adbraw.diff
My guess it needs some ack from the ppc people.

It doesn't - behaviour in case when ADBREQ_RAW is not passed in flags had been obviously unchanged. And only m68k passes ADBREQ_RAW in there. So no, it doesn't affect ppc at all.

Geert Uytterhoeven

2005-12-15 18:01 UTC #8 lore raw

Quoted 11 lines · Al Viro

Quoted 10 lines · Roman Zippel
Quoted 5 lines · Al Viro
So who should I put as the author? You or Geert (or whatever attributions might have been in said big patch)? Incidentally, ADBREQ_RAW had leaked into mainline (sans definition) in 2.3.45-pre2, which was Feb 13 2000, i.e. more than 1.5 year before your commit, so there's quite a chunk of history missing...
I'd say Geert, but it probably comes from the Mac tree. Anyway, it wouldn't be such a bad idea to ask him first why it's in his postponed queue:

Indeed, usually there's a good reason for being in that state instead of not being merged ;-)

Quoted 7 lines

Quoted 3 lines
http://linux-m68k-cvs.ubb.ca/~geert/linux-m68k-2.6.x-merging/POSTPONED/130-adbraw.diff
My guess it needs some ack from the ppc people.
It doesn't - behaviour in case when ADBREQ_RAW is not passed in flags had been obviously unchanged. And only m68k passes ADBREQ_RAW in there. So no, it doesn't affect ppc at all.

Even if behavior is unchanged, this doesn't mean that people like their code being modified behind their back...

Anyway, last time I tried to bring this up with the union of Mac and PowerMac guys, no one seemed to remember why ADBREQ_RAW was really needed...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Al Viro

author

2005-12-15 18:14 UTC #9 lore raw

Quoted 7 lines · Geert Uytterhoeven

Even if behavior is unchanged, this doesn't mean that people like their code being modified behind their back...
Anyway, last time I tried to bring this up with the union of Mac and PowerMac guys, no one seemed to remember why ADBREQ_RAW was really needed...
From my reading of the code it's a way for mac/misc.c to send a packet that

starts with CUDA_PACKET or PMU_PACKET instead of ADB_PACKET, but otherwise is the same as normal adb_request() ones...

Used for access to timer, nvram, etc. - looks like that puppy used to use the same protocol for more than just ADB and the first byte of packet really selects the destination...

Al Viro

author

2005-12-15 18:58 UTC #10 lore raw

Quoted 7 lines · Al Viro

Quoted 1 line
From my reading of the code it's a way for mac/misc.c to send a packet that
starts with CUDA_PACKET or PMU_PACKET instead of ADB_PACKET, but otherwise is the same as normal adb_request() ones...
Used for access to timer, nvram, etc. - looks like that puppy used to use the same protocol for more than just ADB and the first byte of packet really selects the destination...

After reading some more... Is there any reason why mac/misc.c can't simply use cuda_request() and pmu_request() instead? At least for read/write for time and nvram we end up with identical sequence of operations anyway - if you expand the calls in
adb_request((struct adb_request *) &req, NULL,
ADBREQ_RAW|ADBREQ_SYNC,
2, CUDA_PACKET, CUDA_GET_TIME);
[m68k]
and

        if (cuda_request(&req, NULL, 2, CUDA_PACKET, CUDA_GET_TIME) < 0)
		/* bail out */
        while (!req.complete)
                cuda_poll();

[ppc]
until you get to call of cuda_write(), you'll see the same code. Come to think of that... Shouldn't the ifdefs for CONFIG_ADB_PMU in there be for CONFIG_ADB_PMU68? The former depends on PMAC_PPC, so it's not particulary useful thing to check on m68k...

Brad Boyer

2005-12-15 20:05 UTC #11 lore raw

Quoted 22 lines · Al Viro

Quoted 7 lines · Al Viro
Quoted 1 line
From my reading of the code it's a way for mac/misc.c to send a packet that
starts with CUDA_PACKET or PMU_PACKET instead of ADB_PACKET, but otherwise is the same as normal adb_request() ones...
Used for access to timer, nvram, etc. - looks like that puppy used to use the same protocol for more than just ADB and the first byte of packet really selects the destination...
After reading some more... Is there any reason why mac/misc.c can't simply use cuda_request() and pmu_request() instead? At least for read/write for time and nvram we end up with identical sequence of operations anyway - if you expand the calls in
adb_request((struct adb_request *) &req, NULL,
ADBREQ_RAW|ADBREQ_SYNC,
2, CUDA_PACKET, CUDA_GET_TIME);
[m68k]
and
        if (cuda_request(&req, NULL, 2, CUDA_PACKET, CUDA_GET_TIME) < 0)
		/* bail out */
        while (!req.complete)
                cuda_poll();

Quoted 5 lines

[ppc]
until you get to call of cuda_write(), you'll see the same code. Come to think of that... Shouldn't the ifdefs for CONFIG_ADB_PMU in there be for CONFIG_ADB_PMU68? The former depends on PMAC_PPC, so it's not particulary useful thing to check on m68k...

Honestly, the RTC/PRAM code is completely broken in 2.6 for mac68k anyway. We definitely shouldn't be using CONFIG_ADB_PMU in the m68k code. I suspect that wasn't found due to the fact that the pmu68k driver has never been reliable enough to use, so everyone blames that for stuff breaking. Changing that use of CONFIG_ADB_PMU to CONFIG_ADB_PMU68K seems like the correct thing to do in this case.

I would like to stop using adb_request in mac/misc.c as well, but it's not as simple as just changing it to use cuda_request and pmu_request. That should do it for the cuda and pmu based models, but the egret (Mac IIsi and friends) based models get left out by that fix. If noone else looks at it before me, I'll check this out after I fix some other stuff related to m68k mac support.

Brad Boyer
[email protected]

Finn Thain

2005-12-16 01:21 UTC #5 lore raw

In reply to Al Viro

Quoted 14 lines · Al Viro

With some archaeology... It looks like drivers/macintosh part is from Geert (with chunks from benh? not sure) circa Dec 2000; adb.h is a missing piece of earlier patch (one that had leaked in Feb 2000, $DEITY knowns how much older it is)...
drivers/macintosh/adb.c +1 -1
@@ -476,13 +476,15 @@ adb_request(struct adb_request *req, voi
                use_sreq = 1;
        } else
                use_sreq = 0;
-       req->nbytes = nbytes+1;
+       i = (flags & ADBREQ_RAW) ? 0 : 1;

drivers/macintosh/adb.c adb_request(struct adb_request *req, voi · line 487

Quoted 10 lines

drivers/macintosh/adb.c +4 -2

+       req->nbytes = nbytes+i;
        req->done = done;
        req->reply_expected = flags & ADBREQ_REPLY;
        req->data[0] = ADB_PACKET;
        va_start(list, nbytes);
-       for (i = 0; i < nbytes; ++i)
-               req->data[i+1] = va_arg(list, int);
+       while (i < req->nbytes) {
+               req->data[i++] = va_arg(list, int);
+       }

According to the the Linx-mac68k repo, one line of that change belongs to Joshua M. Thompson, while the rest of that code was committed by Ray Knight, (the changelog says it was a merge from 2.4.0).

http://cvs.sourceforge.net/viewcvs.py/linux-mac68k/linux-mac68k/drivers/macintosh/adb.c?annotate=1.25

Quoted 12 lines

drivers/macintosh/adb.c +0 -0

        va_end(list);
 
        if (flags & ADBREQ_NOSEND)

include/linux/adb.h +1 -0

index e9fdc63..aad7b1c 100644
@@ -76,6 +76,7 @@ struct adb_driver {
 #define ADBREQ_REPLY   1       /* expect reply */
 #define ADBREQ_SYNC    2       /* poll until done */
 #define ADBREQ_NOSEND  4       /* build the request, but don't send it */
+#define ADBREQ_RAW     8       /* send raw packet (don't prepend

Quoted 2 lines

ADB_PACKET) */

Credit for that change is due to Joshua M. Thompson.

http://cvs.sourceforge.net/viewcvs.py/linux-mac68k/linux-mac68k/include/linux/adb.h?annotate=1.7

-f

Al Viro

author

2005-12-22 05:07 UTC #12 lore raw

In reply to Brad Boyer

Quoted 6 lines · Brad Boyer

I would like to stop using adb_request in mac/misc.c as well, but it's not as simple as just changing it to use cuda_request and pmu_request. That should do it for the cuda and pmu based models, but the egret (Mac IIsi and friends) based models get left out by that fix. If noone else looks at it before me, I'll check this out after I fix some other stuff related to m68k mac support.

OK... AFAICS, patch I've just posted to l-k and linux-m68k (subject:
[PATCH 04/36] m68k: switch mac/misc.c to direct use of appropriate cuda/pmu/maciisi requests) should so it. Do you have any problems with that variant?

Brad Boyer

2005-12-25 02:26 UTC #13 lore raw

Quoted 8 lines · Al Viro

Quoted 6 lines · Brad Boyer
I would like to stop using adb_request in mac/misc.c as well, but it's not as simple as just changing it to use cuda_request and pmu_request. That should do it for the cuda and pmu based models, but the egret (Mac IIsi and friends) based models get left out by that fix. If noone else looks at it before me, I'll check this out after I fix some other stuff related to m68k mac support.

Quoted 4 lines

OK... AFAICS, patch I've just posted to l-k and linux-m68k (subject:
[PATCH 04/36] m68k: switch mac/misc.c to direct use of appropriate cuda/pmu/maciisi requests) should so it. Do you have any problems with that variant?

I don't have any big issues with it. However, I am not at home right now and can't actually try it out on real hardware. It looks like it should work as well as the version it is replacing. I'm pretty sure the IIsi case was broken before. I think we should go ahead and commit this to get rid of the issues it fixes, and then come back to it later and take another pass at it to fix the fact that it has never worked in 2.6 on some models. It's better than nothing.

Brad Boyer
[email protected]