code review 5977045: nix: sync with plan 9 (issue 5977045)

[0in...@gmail.com]

Reviewers: Nixie,

Message:
Hello nixi...@gmail.com (cc: nix...@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/nix-os/

Description:
nix: sync with plan 9

Please review this at http://codereview.appspot.com/5977045/

Affected files:
M arm/include/u.h
M rc/bin/diffy
M rc/bin/patch/list
M sys/lib/kbmap/uk
M sys/man/2/encode
M sys/man/3/cons
M sys/man/6/ndb
M sys/man/6/vgadb
M sys/man/8/booting
M sys/man/8/mk9660
M sys/man/8/ndb
M sys/man/8/partfs
M sys/man/8/plan9.ini
M sys/man/8/update
M sys/man/8/vga
M sys/src/9/pc/kbd.c
M sys/src/9/pc/memory.c
M sys/src/9/pc/usbohci.c
M sys/src/9/pc/vgavmware.c
M sys/src/9/port/devssl.c
M sys/src/9/port/portclock.c
M sys/src/cmd/5a/a.h
M sys/src/cmd/5a/lex.c
M sys/src/cmd/5a/mkfile
M sys/src/cmd/5c/gc.h
M sys/src/cmd/5c/peep.c
M sys/src/cmd/5c/reg.c
M sys/src/cmd/5c/swt.c
M sys/src/cmd/5l/obj.c
M sys/src/cmd/bind.c
M sys/src/cmd/disk/9660/boot.c
M sys/src/cmd/disk/9660/cdrdwr.c
M sys/src/cmd/disk/9660/dump9660.c
M sys/src/cmd/disk/9660/iso9660.h
M sys/src/cmd/ndb/cs.c
M sys/src/cmd/ndb/dn.c
M sys/src/cmd/ndb/dns.c
M sys/src/cmd/samterm/mesg.c
M sys/src/cmd/upas/fs/imap4.c
M sys/src/cmd/upas/fs/pop3.c
M sys/src/cmd/usb/kb/hid.c
M sys/src/cmd/usb/kb/hid.h
M sys/src/cmd/usb/kb/kb.c
M sys/src/cmd/usb/usbd/usbd.c
M sys/src/libc/arm/getcallerpc.s
M sys/src/libc/arm/main9p.s
M sys/src/libc/arm/memmove.s
M sys/src/libc/arm/strchr.s
M sys/src/libc/arm/strcmp.s
M sys/src/libc/arm/strcpy.s
M sys/src/libc/arm/tas.s
M sys/src/libc/arm/vlop.s
M sys/src/libmach/5db.c
M sys/src/libmp/port/mpfactorial.c
M sys/src/libmp/port/strtomp.c

[David du Colombier]

I've just recreated the precedent CL (5933055) with
all the Plan 9 changes from 2012-03-28 to 2012-04-02,
and excluding the "sys/games/lib/fortunes" file,
as requested by Erik.

--
David du Colombier

[quan...@quanstro.net]

comment set 1 (i'm getting a chunk mismatch with the ndb man page. :-(.

http://codereview.appspot.com/5977045/diff/3/sys/man/2/encode
File sys/man/2/encode (right):

http://codereview.appspot.com/5977045/diff/3/sys/man/2/encode#newcode45
sys/man/2/encode:45: respectively.
the original patch had

$2n+1$, $(8n+4)/5+1$ and $(n+2)/3)*4 + 1$ bytes,

instead. the advantage of this is that it displays
correctly in text mode, and looks much better with
man -P, too. "over" seems out-of-place in an inline
equation.

http://codereview.appspot.com/5977045/diff/3/sys/man/2/encode#newcode88
sys/man/2/encode:88: .B /sys/src/libc/port/u[136][246].c
should list out the files at the original patch did.
this construct is misleading (u12.c?) and doesn't plumb.

.B /sys/src/libc/port/u16.c
.br
.B /sys/src/libc/port/u32.c
.br
.B /sys/src/libc/port/u64.c

http://codereview.appspot.com/5977045/

[quan...@quanstro.net]

second set of changes (sorry; more rietveld errors)

unless i've missed something, i don't think we
should accept the changes that add "9boot". this
doesn't appear to exist on sources.

http://codereview.appspot.com/5977045/diff/3/sys/man/6/vgadb
File sys/man/6/vgadb (right):

http://codereview.appspot.com/5977045/diff/3/sys/man/6/vgadb#newcode419
sys/man/6/vgadb:419: .IR 9boot (8),
there is no 9boot(8).

http://codereview.appspot.com/5977045/diff/3/sys/man/8/booting
File sys/man/8/booting (right):

http://codereview.appspot.com/5977045/diff/3/sys/man/8/booting#newcode28
sys/man/8/booting:28: or
some sort of copypasta here.

http://codereview.appspot.com/5977045/diff/3/sys/man/8/booting#newcode42
sys/man/8/booting:42: .I 9load
some sort of copypasta here.

http://codereview.appspot.com/5977045/diff/3/sys/man/8/booting#newcode291
sys/man/8/booting:291: .BR /sys/src/9/pcboot .
this doesn't exist.

http://codereview.appspot.com/5977045/diff/3/sys/man/8/ndb
File sys/man/8/ndb (right):

http://codereview.appspot.com/5977045/diff/3/sys/man/8/ndb#newcode756
sys/man/8/ndb:756: Print the names of all PCs that boot via PXE.
pcs aren't the only ones that pxe boot.

http://codereview.appspot.com/5977045/diff/3/sys/man/8/ndb#newcode759
sys/man/8/ndb:759: % ndb/query -a bootf /386/9boot sys
?? /386/9boot is not on sources.

http://codereview.appspot.com/5977045/diff/3/sys/man/8/partfs
File sys/man/8/partfs (right):

http://codereview.appspot.com/5977045/diff/3/sys/man/8/partfs#newcode62
sys/man/8/partfs:62: .IR 9boot (8))
again, 9boot.

http://codereview.appspot.com/5977045/diff/3/sys/man/8/update
File sys/man/8/update (right):

http://codereview.appspot.com/5977045/diff/3/sys/man/8/update#newcode123
sys/man/8/update:123: .IR mkfs (8),
again 9boot

http://codereview.appspot.com/5977045/diff/3/sys/man/8/vga
File sys/man/8/vga (right):

http://codereview.appspot.com/5977045/diff/3/sys/man/8/vga#newcode83
sys/man/8/vga:83: .IR 9boot (8).
again 9boot

http://codereview.appspot.com/5977045/diff/3/sys/man/8/vga#newcode207
sys/man/8/vga:207: .IR 9boot (8)
again 9boot

http://codereview.appspot.com/5977045/

[quan...@quanstro.net]

last chunk.

it would be ideal if someone else looked at the
ohci changes, which really do look wrong to me.

http://codereview.appspot.com/5977045/diff/3/sys/src/9/pc/usbohci.c
File sys/src/9/pc/usbohci.c (right):

http://codereview.appspot.com/5977045/diff/3/sys/src/9/pc/usbohci.c#newcode1264
sys/src/9/pc/usbohci.c:1264: ctlr->ohci->intrdisable = Mie;
i don't know what this could accomplish. ohci
needs to be on a level-triggered interrupt, and it
would be illegal for the h/w to retrigger the interrupt before the
processor executes eoi.

so disabling the interrupts in the interrupt routine
can't be right. it might be covering for a bug by
altering the timing.

http://codereview.appspot.com/5977045/diff/3/sys/src/9/pc/usbohci.c#newcode1265
sys/src/9/pc/usbohci.c:1265: coherence();
this coherence doesn't appear to be correct.
ohci->intrdisable is a pci non-posted write,
which is always coherent---the processor waits
for the ack.

http://codereview.appspot.com/5977045/diff/3/sys/src/9/pc/usbohci.c#newcode2499
sys/src/9/pc/usbohci.c:2499: coherence();
bogus coherence after non-posted writes.

http://codereview.appspot.com/5977045/diff/3/sys/src/cmd/upas/fs/imap4.c
File sys/src/cmd/upas/fs/imap4.c (right):

http://codereview.appspot.com/5977045/diff/3/sys/src/cmd/upas/fs/imap4.c#newcode419
sys/src/cmd/upas/fs/imap4.c:419: if(0 && (!imap->thumb ||
!okThumbprint(digest, imap->thumb))){
i use this. perhaps it would be better to put the
thumbprints under user control.

http://codereview.appspot.com/5977045/

[Nemo]

its illegal but some hw does it.

--
iphone kbd. excuse typos :)

[erik quanstrom]

On Tue Apr 3 12:28:09 EDT 2012, ne...@lsub.org wrote:
> its illegal but some hw does it.

you're asserting that a level-triggered interrupt can retrigger without
going low first.

to assert this, you're claiming an architecture violation on par with
a memory coherency violation.

could you please provide detail on how this could happen. it's quite
an audacious claim.

i think it's much more likely that these changes cover up another
bug elsewhere.

- erik

[Charles Forsyth]

I think there was a discussion of this on 9fans. I wondered about at the time, since I've worked

on hardware (an ethernet) where the documentation indeed said to do that even for level-triggered interrupts, but it was wrong.

The data sheet author had misunderstood the way the software would normally be handling interrupts.

It was obvious in that case because he'd included some other text explaining what (he thought) the effect would be.

[Charles Forsyth]

Did you really mean that? The whole point of level-triggered is that the transition isn't interesting,

it's the state.

[erik quanstrom]

On Tue Apr 3 12:54:04 EDT 2012, charles...@gmail.com wrote:

> Did you really mean that? The whole point of level-triggered is that the
> transition isn't interesting,
> it's the state.
>
> On 3 April 2012 17:36, erik quanstrom <quan...@quanstro.net> wrote:
>

> > you're asserting that a level-triggered interrupt can retrigger without
> > going low first.

what i wrote is what i think the code is asserting. as you note,
this is nonsense. i think we agree on that.

the coherences after non-posted writes also assert that a non-posted
pci mem write could pass up some other sort of memory transaction,
which doesn't make sense in the same sort of way. non-posted is
defined by the cpu waiting for the transaction to finish.

- erik

[Charles Forsyth]

I'd assumed the coherence calls were not so much for the PC, as for ohci/ehci on

a chip-specific bus on (say) an ARM multiprocessor, sharing the same source code.

[0in...@gmail.com]

Hello nixi...@gmail.com, quan...@quanstro.net (cc:
nix...@googlegroups.com),

Please take another look.

http://codereview.appspot.com/5977045/

[erik quanstrom]

did you remove the references to 9boot?

- erik

[0in...@gmail.com]

[David du Colombier]

After some private discussions with Erik, we concluded
it was less confusing to let the 9boot changes in the
manuals out for now.

--
David du Colombier

[erik quanstrom]

lgtm. let's get this over with.

i hope that the maintainer of the usb/ohci controller will take
another look. i think the coherence() are clearly wrong, and turning
off interrupts in the interrupt routine doesn't make sense, unless
there's something else going on—like a timing artifact, or perhaps
the register does more than just turn off interrupts.

- erik

[Charles Forsyth]

I agree with that. Which hardware was it? I've lost the original discussion.