Re: switchd: bzero -> memset

[Michael W. Bombardieri]

On Tue, Apr 03, 2018 at 10:20:42PM -0600, Theo de Raadt wrote:
> Michael W. Bombardieri  wrote:
> 
> > Hello,
> > 
> > switchd can just use memset instead of mixing memset with bzero.
> > But does the util.c change need to be sync'ed with other tools?
> 
> There are 418 calls to bzero in *bin/*/*c
> 
> bcopy isn't going to be removed from libc, and frankly it is
> diomatically simpler.
> 
> Would you believe in 15 years ago we have found calls where c and len
> were swapped, making it a no-op -- in security sensitive software?
Nice.
> 
> So I don't see the specific value in your proposal.

I agree, bzero is simpler and easier to grep for. But then grep'ing
is more complicated if you mix bzero and memset, so I guess it makes
sense to use one or the other at least within the same file.

Re: switchd: bzero -> memset

[Theo de Raadt]

Michael W. Bombardieri  wrote:

> Hello,
> 
> switchd can just use memset instead of mixing memset with bzero.
> But does the util.c change need to be sync'ed with other tools?

There are 418 calls to bzero in *bin/*/*c

bcopy isn't going to be removed from libc, and frankly it is
diomatically simpler.

Would you believe in 15 years ago we have found calls where c and len
were swapped, making it a no-op -- in security sensitive software?

So I don't see the specific value in your proposal.

switchd: bzero -> memset

[Michael W. Bombardieri]

Hello,

switchd can just use memset instead of mixing memset with bzero.
But does the util.c change need to be sync'ed with other tools?

- Michael


Index: ofp10.c
===
RCS file: /cvs/src/usr.sbin/switchd/ofp10.c,v
retrieving revision 1.19
diff -u -p -u -r1.19 ofp10.c
--- ofp10.c 2 Dec 2016 14:39:46 -   1.19
+++ ofp10.c 4 Apr 2018 04:00:19 -
@@ -342,7 +342,7 @@ ofp10_packet_match(struct packet *pkt, s
 {
struct ether_header *eh = pkt->pkt_eh;
 
-   bzero(m, sizeof(*m));
+   memset(m, 0, sizeof(*m));
m->m_wildcards = htonl(~flags);
 
if ((flags & (OFP10_WILDCARD_DL_SRC|OFP10_WILDCARD_DL_DST)) &&
@@ -377,7 +377,7 @@ ofp10_packet_in(struct switchd *sc, stru
if ((pin = ibuf_getdata(ibuf, sizeof(*pin))) == NULL)
return (-1);
 
-   bzero(, sizeof(pkt));
+   memset(, 0, sizeof(pkt));
len = ntohs(pin->pin_total_len);
srcport = ntohs(pin->pin_port);
 
Index: ofp13.c
===
RCS file: /cvs/src/usr.sbin/switchd/ofp13.c,v
retrieving revision 1.43
diff -u -p -u -r1.43 ofp13.c
--- ofp13.c 17 Jan 2017 09:21:50 -  1.43
+++ ofp13.c 4 Apr 2018 04:00:20 -
@@ -1029,7 +1029,7 @@ ofp13_packet_in(struct switchd *sc, stru
if (pin->pin_reason != OFP_PKTIN_REASON_NO_MATCH)
return (-1);
 
-   bzero(, sizeof(pkt));
+   memset(, 0, sizeof(pkt));
len = ntohs(pin->pin_total_len);
 
/* very basic way of getting the source port */
Index: switchd.c
===
RCS file: /cvs/src/usr.sbin/switchd/switchd.c,v
retrieving revision 1.15
diff -u -p -u -r1.15 switchd.c
--- switchd.c   9 Jan 2017 14:49:22 -   1.15
+++ switchd.c   4 Apr 2018 04:00:20 -
@@ -242,7 +242,7 @@ switchd_socket(struct sockaddr *sock, in
/*
 * Socket options
 */
-   bzero(, sizeof(lng));
+   memset(, 0, sizeof(lng));
if (setsockopt(s, SOL_SOCKET, SO_LINGER, , sizeof(lng)) == -1)
goto bad;
if (reuseport) {
Index: util.c
===
RCS file: /cvs/src/usr.sbin/switchd/util.c,v
retrieving revision 1.6
diff -u -p -u -r1.6 util.c
--- util.c  9 Jan 2017 14:49:22 -   1.6
+++ util.c  4 Apr 2018 04:00:20 -
@@ -196,7 +196,7 @@ prefixlen2mask6(uint8_t prefixlen, uint3
if (prefixlen > 128)
prefixlen = 128;
 
-   bzero(, sizeof(s6));
+   memset(, 0, sizeof(s6));
for (i = 0; i < prefixlen / 8; i++)
s6.s6_addr[i] = 0xff;
i = prefixlen % 8;
@@ -277,7 +277,7 @@ print_map(unsigned int type, struct cons
 
if (idx >= SWITCHD_CYCLE_BUFFERS)
idx = 0;
-   bzero(buf[idx], sizeof(buf[idx]));
+   memset(buf[idx], 0, sizeof(buf[idx]));
 
for (i = 0; map[i].cm_name != NULL; i++) {
if (map[i].cm_type == type) {

Re: TEST_LOG semantics

[Jeremie Courreges-Anglas]

On Tue, Apr 03 2018, Marc Espie  wrote:
> This exists only so that individual ports may log. Tweaking it anywhere
> is awkward (and will be more so with SEPARATE_BUILD).
>
> I just checked that the current ports tree does not abuse this.
>
> Okay ?

ok jca@

> Index: bsd.port.mk.5
> ===
> RCS file: /cvs/src/share/man/man5/bsd.port.mk.5,v
> retrieving revision 1.474
> diff -u -p -r1.474 bsd.port.mk.5
> --- bsd.port.mk.5 31 Jan 2018 17:43:33 -  1.474
> +++ bsd.port.mk.5 3 Apr 2018 21:09:43 -
> @@ -2921,6 +2921,7 @@ if port needs human interaction to run i
>  if the tests need an active X11 display to work.
>  .It Ev TEST_LOG
>  Command used to log the results of regression tests to TEST_LOGFILE.
> +Read-only.
>  .It Ev TEST_LOGFILE
>  Log file containing the results of regression tests.
>  .It Ev TEST_TARGET
>

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: [patch] RSA_new.3

[Theo Buehler]

> -typdef struct {
> +typedef struct {

Fixed, thanks

[patch] RSA_new.3

[Edgar Pettijohn]

typo

Index: RSA_new.3
===
RCS file: /cvs/src/lib/libcrypto/man/RSA_new.3,v
retrieving revision 1.4
diff -u -p -u -r1.4 RSA_new.3
--- RSA_new.3    11 Dec 2016 12:52:28 -    1.4
+++ RSA_new.3    3 Apr 2018 21:12:13 -
@@ -93,7 +93,7 @@ structure consists of several
 components.
 It can contain public as well as private RSA keys:
 .Bd -literal
-typdef struct {
+typedef struct {
 BIGNUM *n;        // public modulus
 BIGNUM *e;        // public exponent
 BIGNUM *d;        // private exponent

TEST_LOG semantics

[Marc Espie]

This exists only so that individual ports may log. Tweaking it anywhere
is awkward (and will be more so with SEPARATE_BUILD).

I just checked that the current ports tree does not abuse this.

Okay ?

Index: bsd.port.mk.5
===
RCS file: /cvs/src/share/man/man5/bsd.port.mk.5,v
retrieving revision 1.474
diff -u -p -r1.474 bsd.port.mk.5
--- bsd.port.mk.5   31 Jan 2018 17:43:33 -  1.474
+++ bsd.port.mk.5   3 Apr 2018 21:09:43 -
@@ -2921,6 +2921,7 @@ if port needs human interaction to run i
 if the tests need an active X11 display to work.
 .It Ev TEST_LOG
 Command used to log the results of regression tests to TEST_LOGFILE.
+Read-only.
 .It Ev TEST_LOGFILE
 Log file containing the results of regression tests.
 .It Ev TEST_TARGET

ksh: support 64 bit numbers on 32 bit archs

[Tobias Stoeckmann]

Hi,

this patch increases the number range on 32 bit architectures like
i386 to 64 bit. These are already supported on 64 bit architectures
due to using "long".

The rational behind this patch is to unify test/expr/ksh in allowing
64 bit integers, making variable handling more consistent in base
tools.

Also, keep in mind that 32 bit architectures also support large files,
so 64 bit file sizes in variables aren't totally out of the question.

When this is in, I want to merge the expr integer overflow checks
into ksh.

Verification of this patch is rather easy on amd64, because binary
does not change. I have extended regress tests to show that i386 can
handle 64 bit integers.

My i386 system is running with this patch and nothing broke so far.


Tobias

Index: bin/ksh/c_ksh.c
===
RCS file: /cvs/src/bin/ksh/c_ksh.c,v
retrieving revision 1.59
diff -u -p -u -p -r1.59 c_ksh.c
--- bin/ksh/c_ksh.c 15 Mar 2018 16:51:29 -  1.59
+++ bin/ksh/c_ksh.c 3 Apr 2018 18:00:31 -
@@ -1017,7 +1017,7 @@ int
 c_let(char **wp)
 {
int rv = 1;
-   long val;
+   int64_t val;
 
if (wp[1] == NULL) /* at ksh does this */
bi_errorf("no arguments");
Index: bin/ksh/c_sh.c
===
RCS file: /cvs/src/bin/ksh/c_sh.c,v
retrieving revision 1.62
diff -u -p -u -p -r1.62 c_sh.c
--- bin/ksh/c_sh.c  27 Dec 2017 13:02:57 -  1.62
+++ bin/ksh/c_sh.c  3 Apr 2018 18:00:31 -
@@ -33,7 +33,7 @@ c_shift(char **wp)
 {
struct block *l = genv->loc;
int n;
-   long val;
+   int64_t val;
char *arg;
 
if (ksh_getopt(wp, _opt, null) == '?')
Index: bin/ksh/c_test.c
===
RCS file: /cvs/src/bin/ksh/c_test.c,v
retrieving revision 1.24
diff -u -p -u -p -r1.24 c_test.c
--- bin/ksh/c_test.c26 Dec 2017 19:10:31 -  1.24
+++ bin/ksh/c_test.c3 Apr 2018 18:00:31 -
@@ -308,7 +308,7 @@ test_eval(Test_env *te, Test_op op, cons
case TO_INTLE: /* -le */
case TO_INTLT: /* -lt */
{
-   long v1, v2;
+   int64_t v1, v2;
 
if (!evaluate(opnd1, , KSH_RETURN_ERROR, false) ||
!evaluate(opnd2, , KSH_RETURN_ERROR, false)) {
Index: bin/ksh/c_ulimit.c
===
RCS file: /cvs/src/bin/ksh/c_ulimit.c,v
retrieving revision 1.27
diff -u -p -u -p -r1.27 c_ulimit.c
--- bin/ksh/c_ulimit.c  15 Mar 2018 16:51:29 -  1.27
+++ bin/ksh/c_ulimit.c  3 Apr 2018 18:00:31 -
@@ -22,6 +22,7 @@
 
 #include 
 #include 
+#include 
 #include 
 
 #include "sh.h"
@@ -139,7 +140,7 @@ set_ulimit(const struct limits *l, const
if (strcmp(v, "unlimited") == 0)
val = RLIM_INFINITY;
else {
-   long rval;
+   int64_t rval;
 
if (!evaluate(v, , KSH_RETURN_ERROR, false))
return 1;
@@ -187,6 +188,6 @@ print_ulimit(const struct limits *l, int
shprintf("unlimited\n");
else {
val /= l->factor;
-   shprintf("%ld\n", (long) val);
+   shprintf("%" PRIi64 "\n", (int64_t) val);
}
 }
Index: bin/ksh/edit.c
===
RCS file: /cvs/src/bin/ksh/edit.c,v
retrieving revision 1.64
diff -u -p -u -p -r1.64 edit.c
--- bin/ksh/edit.c  15 Mar 2018 16:51:29 -  1.64
+++ bin/ksh/edit.c  3 Apr 2018 18:00:31 -
@@ -80,10 +80,10 @@ check_sigwinch(void)
ws.ws_col;
 
if ((vp = typeset("COLUMNS", 0, 0, 0, 0)))
-   setint(vp, (long) ws.ws_col);
+   setint(vp, (int64_t) ws.ws_col);
}
if (ws.ws_row && (vp = typeset("LINES", 0, 0, 0, 0)))
-   setint(vp, (long) ws.ws_row);
+   setint(vp, (int64_t) ws.ws_row);
}
}
 }
Index: bin/ksh/eval.c
===
RCS file: /cvs/src/bin/ksh/eval.c,v
retrieving revision 1.59
diff -u -p -u -p -r1.59 eval.c
--- bin/ksh/eval.c  16 Jan 2018 22:52:32 -  1.59
+++ bin/ksh/eval.c  3 Apr 2018 18:00:31 -
@@ -732,7 +732,7 @@ varsub(Expand *xp, char *sp, char *word,
if (Flag(FNOUNSET) && c == 0 && !zero_ok)
errorf("%s: parameter not set", sp);
*stypep = 0; /* unqualified variable/string substitution */
-   xp->str = str_save(ulton((unsigned long)c, 10), ATEMP);
+   xp->str = str_save(u64ton((uint64_t)c, 10), ATEMP);
return XSUB;
}
 
Index: 

Re: hppa MI mutex

[Mark Kettenis]

> Date: Tue, 3 Apr 2018 12:36:18 +0200
> From: Martin Pieuchot 
> 
> On 03/04/18(Tue) 12:06, Mark Kettenis wrote:
> > > Date: Tue, 3 Apr 2018 11:16:45 +0200
> > > From: Martin Pieuchot 
> > > 
> > > Here's a diff to switch hppa to the MI mutex implementation.  I'm
> > > looking for testers, as I don't own such hardware.
> > > 
> > > Note that our MI mutex implementation relies on atomic_cas_ptr(9).  On
> > > hppa all the calls to this function are serialized on a single lock. I
> > > don't believe it will introduce contention at this point since the
> > > KERNEL_LOCK() is still the problem.  However if it becomes a bottleneck
> > > we could use multiple locks in a small hash table.
> > 
> > I don't have such hardware either, but I'm not in favour of doing
> > this.  PA-RISC is "challenged" but at least the locking primitive it
> > supplies is suitable for implementing spin locks.  Emulating CAS with
> > spin locks to implement a spin lock feels so wrong.  There are also
> > some concerns about guarantees of forward progress in such an
> > implementation.
> 
> Well I would prefer if we could solve these concerns because we are already
> using atomic operations.
> 
> So what are the options?
> 
> 1. Keep hppa mutex MD and start diverging with the MI mutex
> 
> 2. Add another MI layer of abstraction on top of the atomic*(9) API
>because of hppa
> 
> 3. Emulate the atomic*(9) API as well as we can on hppa
> 
> I picked 3 because I don't want to add more complexity for an arch
> which is not widely used.  Who care if it feels wrong to emulate a CAS
> with a hw spinlock?  I believed that what we need is locking primitives
> that we can debug and analyse.  If hppa can benefit from that and help us
> find bugs, that's even better :)

Option 1. doesn't rule this out.  Having most (but not all)
architectures using the MI implementation is still a win.  The hppa
mutex code is already written in C so it would be fairly easy do
adjust it to provide the equivalent diagnostics.  I can give that a go
if you want me to.

Re: kqueue EV_DISPATCH and EV_EOF interaction

[Lukas Larsson]

On Fri, Mar 30, 2018 at 1:51 AM, Mike Belopuhov  wrote:

> On Fri, Mar 30, 2018 at 01:21 +0200, Mike Belopuhov wrote:
> >
> > Hi,
> >
> > This appears to be an issue with reactivating disabled event sources
> > in kqueue_register.  Something along the lines of FreeBSD commits:
> >
> > https://svnweb.freebsd.org/base?view=revision=274560 and
> > https://reviews.freebsd.org/rS295786 where parent differential review
> > https://reviews.freebsd.org/D5307 has some additional comments.
> >
> > In any case, by either porting their code (#else branch) or slightly
> > adjusting our own (I think that should be enough), I can no longer
> > reproduce the issue you've reported.  Please test and report back if
> > that solves your original issue.  Either variants will require
> > rigorous testing and a thorough review.
> >
> > Cheers,
> > Mike
> >
>
> After a bit of tinkering, I think I can minimize the change even
> further.  Basically we just need to call the filter once and if
> there's some data available, it'll return true and we'll mark the
> knote as active.
>
> diff --git sys/kern/kern_event.c sys/kern/kern_event.c
> index fb9cad360b1..4e0949645cb 100644
> --- sys/kern/kern_event.c
> +++ sys/kern/kern_event.c
> @@ -671,10 +671,12 @@ kqueue_register(struct kqueue *kq, struct kevent
> *kev, struct proc *p)
> }
>
> if ((kev->flags & EV_ENABLE) && (kn->kn_status & KN_DISABLED)) {
> s = splhigh();
> kn->kn_status &= ~KN_DISABLED;
> +   if (kn->kn_fop->f_event(kn, 0))
> +   kn->kn_status |= KN_ACTIVE;
> if ((kn->kn_status & KN_ACTIVE) &&
> ((kn->kn_status & KN_QUEUED) == 0))
> knote_enqueue(kn);
> splx(s);
> }
>

Hello,

Thank you for your help and the patch. I've applied the smaller patch to
one of our test machines
and the small testcase I sent here on the list has been fixed. I also ran
our larger test suites where
I first found the issue and those work as well.

Lukas

Re: Earlier FREF() for sys_ioctl()

[Mark Kettenis]

> Date: Tue, 3 Apr 2018 16:48:09 +0200
> From: Martin Pieuchot 
> 
> Similar to other diffs, this one move a FREF() right after
> fd_getfile_mode(), ok?
> 
> Index: kern/sys_generic.c
> ===
> RCS file: /cvs/src/sys/kern/sys_generic.c,v
> retrieving revision 1.116
> diff -u -p -r1.116 sys_generic.c
> --- kern/sys_generic.c2 Jan 2018 06:38:45 -   1.116
> +++ kern/sys_generic.c3 Apr 2018 13:32:32 -
> @@ -393,29 +393,30 @@ sys_ioctl(struct proc *p, void *v, regis
>   struct file *fp;
>   struct filedesc *fdp;
>   u_long com = SCARG(uap, com);
> - int error;
> + int error = 0;
>   u_int size;
> - caddr_t data, memp;
> + caddr_t data, memp = NULL;
>   int tmp;
>  #define STK_PARAMS   128
>   long long stkbuf[STK_PARAMS / sizeof(long long)];
>  
>   fdp = p->p_fd;
> - fp = fd_getfile_mode(fdp, SCARG(uap, fd), FREAD|FWRITE);
> -
> - if (fp == NULL)
> + if ((fp = fd_getfile_mode(fdp, SCARG(uap, fd), FREAD|FWRITE)) == NULL)
>   return (EBADF);

I find that assignments within if statements make code harder to read.

Earlier FREF() for sys_ioctl()

[Martin Pieuchot]

Similar to other diffs, this one move a FREF() right after
fd_getfile_mode(), ok?

Index: kern/sys_generic.c
===
RCS file: /cvs/src/sys/kern/sys_generic.c,v
retrieving revision 1.116
diff -u -p -r1.116 sys_generic.c
--- kern/sys_generic.c  2 Jan 2018 06:38:45 -   1.116
+++ kern/sys_generic.c  3 Apr 2018 13:32:32 -
@@ -393,29 +393,30 @@ sys_ioctl(struct proc *p, void *v, regis
struct file *fp;
struct filedesc *fdp;
u_long com = SCARG(uap, com);
-   int error;
+   int error = 0;
u_int size;
-   caddr_t data, memp;
+   caddr_t data, memp = NULL;
int tmp;
 #define STK_PARAMS 128
long long stkbuf[STK_PARAMS / sizeof(long long)];
 
fdp = p->p_fd;
-   fp = fd_getfile_mode(fdp, SCARG(uap, fd), FREAD|FWRITE);
-
-   if (fp == NULL)
+   if ((fp = fd_getfile_mode(fdp, SCARG(uap, fd), FREAD|FWRITE)) == NULL)
return (EBADF);
+   FREF(fp);
 
if (fp->f_type == DTYPE_SOCKET) {
struct socket *so = fp->f_data;
 
-   if (so->so_state & SS_DNS)
-   return (EINVAL);
+   if (so->so_state & SS_DNS) {
+   error = EINVAL;
+   goto out;
+   }
}
 
error = pledge_ioctl(p, com, fp);
if (error)
-   return (error);
+   goto out;
 
switch (com) {
case FIONCLEX:
@@ -426,7 +427,7 @@ sys_ioctl(struct proc *p, void *v, regis
else
fdp->fd_ofileflags[SCARG(uap, fd)] |= UF_EXCLOSE;
fdpunlock(fdp);
-   return (0);
+   goto out;
}
 
/*
@@ -434,10 +435,10 @@ sys_ioctl(struct proc *p, void *v, regis
 * copied to/from the user's address space.
 */
size = IOCPARM_LEN(com);
-   if (size > IOCPARM_MAX)
-   return (ENOTTY);
-   FREF(fp);
-   memp = NULL;
+   if (size > IOCPARM_MAX) {
+   error = ENOTTY;
+   goto out;
+   }
if (size > sizeof (stkbuf)) {
memp = malloc(size, M_IOCTLOPS, M_WAITOK);
data = memp;
@@ -525,8 +526,7 @@ sys_ioctl(struct proc *p, void *v, regis
error = copyout(data, SCARG(uap, data), size);
 out:
FRELE(fp, p);
-   if (memp)
-   free(memp, M_IOCTLOPS, size);
+   free(memp, M_IOCTLOPS, size);
return (error);
 }
 

FREF() for namei()

[Martin Pieuchot]

namei() currently uses fd_getfile() without calling FREF()/FRELE()
because it doesn't sleep before grabbing a reference on the vnode.

This won't be enough as soon as we start unlocking some syscalls,
so add a FREF()/FRELE() dance.

Ok?

Index: kern/vfs_lookup.c
===
RCS file: /cvs/src/sys/kern/vfs_lookup.c,v
retrieving revision 1.65
diff -u -p -r1.65 vfs_lookup.c
--- kern/vfs_lookup.c   29 Aug 2017 02:51:27 -  1.65
+++ kern/vfs_lookup.c   3 Apr 2018 13:33:35 -
@@ -190,12 +190,15 @@ fail:
pool_put(_pool, cnp->cn_pnbuf);
return (EBADF);
}
+   FREF(fp);
dp = (struct vnode *)fp->f_data;
if (fp->f_type != DTYPE_VNODE || dp->v_type != VDIR) {
+   FRELE(fp, p);
pool_put(_pool, cnp->cn_pnbuf);
return (ENOTDIR);
}
vref(dp);
+   FRELE(fp, p);
}
for (;;) {
if (!dp->v_mount) {

Use FREF() & finishdup() in dupfdopen()

[Martin Pieuchot]

Here's another refactoring to properly reference count a 'struct file *'
just after calling fd_getfile().

Instead of incrementing `f_count' by hand, give the current reference
to finishdup() like it is done in other places in the same file.

Ok?

Index: kern/kern_descrip.c
===
RCS file: /cvs/src/sys/kern/kern_descrip.c,v
retrieving revision 1.144
diff -u -p -r1.144 kern_descrip.c
--- kern/kern_descrip.c 3 Apr 2018 09:00:03 -   1.144
+++ kern/kern_descrip.c 3 Apr 2018 13:30:47 -
@@ -1274,6 +1274,8 @@ dupfdopen(struct proc *p, int indx, int 
struct filedesc *fdp = p->p_fd;
int dupfd = p->p_dupfd;
struct file *wfp;
+   int error, flags;
+   register_t dummy;
 
fdpassertlocked(fdp);
 
@@ -1297,24 +1299,26 @@ dupfdopen(struct proc *p, int indx, int 
 */
if ((wfp = fd_getfile(fdp, dupfd)) == NULL)
return (EBADF);
+   FREF(wfp);
 
/*
 * Check that the mode the file is being opened for is a
 * subset of the mode of the existing descriptor.
 */
-   if (((mode & (FREAD|FWRITE)) | wfp->f_flag) != wfp->f_flag)
+   if (((mode & (FREAD|FWRITE)) | wfp->f_flag) != wfp->f_flag) {
+   FRELE(wfp, p);
return (EACCES);
-   if (wfp->f_count == LONG_MAX-2)
-   return (EDEADLK);
+   }
 
-   fdp->fd_ofiles[indx] = wfp;
-   fdp->fd_ofileflags[indx] = (fdp->fd_ofileflags[indx] & UF_EXCLOSE) |
-   (fdp->fd_ofileflags[dupfd] & ~UF_EXCLOSE);
+   flags = fdp->fd_ofileflags[indx] & UF_EXCLOSE;
if (ISSET(p->p_p->ps_flags, PS_PLEDGE))
-   fdp->fd_ofileflags[indx] |= UF_PLEDGED;
-   wfp->f_count++;
-   fd_used(fdp, indx);
-   return (0);
+   flags |= UF_PLEDGED;
+
+   /* finishdup() does FRELE */
+   error = finishdup(p, wfp, dupfd, indx, , 1);
+   if (error == 0)
+   fdp->fd_ofileflags[indx] |= flags;
+   return (error);
 }
 
 /*

Re: autoconf.9 consistency

[Ingo Schwarze]

Hi David,

David Gwynne wrote on Tue, Apr 03, 2018 at 02:01:33PM +1000:

> i landed on this manpage and got confused cos it looked different
> to the rest of them.

Note that the style of having multiple synopses in multiple sections
of a page is used in more than one page - though admittedly in few:

 * openssl(1)
 * ifconfig(8)
 * bus_dma(9)
 * kern(9)
 * pmap(9)
 * uvm(9)

In general, i don't like splitting the SYNOPSIS in this way much
because until a few years ago, OpenBSD was the only operating system
supporting it.  Now, all systems using mandoc support it, but groff
still doesn't, so it is still non-portable.  Besides, the syntax

  .nr nS 1

(as described in roff(7)) is exceedingly ugly and non-obvious,
originally an OpenBSD-specific hack.  So if we can reduce the usage
of this feature, i'm generally in favour, even though i don't have
much hope that openssl(1) and ifconfig(8) can survive without it.

> this shuffles it a bit to make it less different.
> the main change is to move the function prototypes up to the synopsis.

Sure, in the case at hand, that may be less surprising: The page is
short and the SYNOPSIS is only split into two parts.

> the other, less important change is to replace some headers with
> subheaders.

I know that jmc@ does not consider the difference between headers
and subheaders very important, in particular when splitting a large
DESCRIPTION into multiple parts.  I'm often a bit more in favour
of using subheaders than he is.  In this case, i like subheaders
better than separate sections, in particular since the page is short
enough such that the subsections won't be of excessive length.

> ok?

OK schwarze@
  Ingo


> Index: autoconf.9
> ===
> RCS file: /cvs/src/share/man/man9/autoconf.9,v
> retrieving revision 1.16
> diff -u -p -r1.16 autoconf.9
> --- autoconf.911 Dec 2015 16:07:02 -  1.16
> +++ autoconf.93 Apr 2018 03:58:20 -
> @@ -41,6 +41,21 @@
>  .Sh SYNOPSIS
>  .In sys/param.h
>  .In sys/device.h
> +.Ft "void *"
> +.Fn config_search "cfmatch_t func" "struct device *parent" "void *aux"
> +.Ft "void *"
> +.Fn config_rootsearch "cfmatch_t func" "char *rootname" "void *aux"
> +.Ft "struct device *"
> +.Fo config_found_sm
> +.Fa "struct device *parent"
> +.Fa "void *aux"
> +.Fa "cfprint_t print"
> +.Fa "cfmatch_t submatch"
> +.Fc
> +.Ft "struct device *"
> +.Fn config_found "struct device *parent" "void *aux" "cfprint_t print"
> +.Ft "struct device *"
> +.Fn config_rootfound "char *rootname" "void *aux"
>  .Sh DESCRIPTION
>  Autoconfiguration is the process of matching hardware devices with an
>  appropriate device driver.
> @@ -77,14 +92,7 @@ ends with a unit number.
>  The unit number identifies an instance of the driver.
>  Device data structures are allocated dynamically during autoconfiguration,
>  giving a unique address for each instance.
> -.Sh INDIRECT CONFIGURATION
> -.nr nS 1
> -.Ft "void *"
> -.Fn config_search "cfmatch_t func" "struct device *parent" "void *aux"
> -.Ft "void *"
> -.Fn config_rootsearch "cfmatch_t func" "char *rootname" "void *aux"
> -.nr nS 0
> -.Pp
> +.Ss Indirect Configuration
>  The
>  .Fn config_search
>  function performs indirect configuration of physical devices by iterating
> @@ -142,17 +150,7 @@ itself.
>  Note that this function is designed so that it can be used to apply an
>  arbitrary function to all potential children.
>  In this case callers may choose to ignore the return value.
> -.Sh DIRECT CONFIGURATION
> -.nr nS 1
> -.Ft "struct device *"
> -.Fn config_found_sm "struct device *parent" "void *aux" "cfprint_t print" \
> -"cfmatch_t submatch"
> -.Ft "struct device *"
> -.Fn config_found "struct device *parent" "void *aux" "cfprint_t print"
> -.Ft "struct device *"
> -.Fn config_rootfound "char *rootname" "void *aux"
> -.nr nS 0
> -.Pp
> +.Ss Direct Configuration
>  The
>  .Fn config_found_sm
>  function performs direct configuration on a physical device.

NFS mount & diskless tweak

[Martin Pieuchot]

Diff below changes the representation of a NFS mount point.  It is
adapted from NetBSD with some tweaks of mine and is required for
properly locking NFSnodes.

The idea is to keep a reference to the root vnode in 'struct nfsmount'
instead of doing a lookup every time VFS_ROOT() is called.  Calls to
nfs_root() in the diskless path also become useless and we don't have
to play with locking there.  Finally we look at the number of refcounts
when unmounting a filesystem, this is useful when debugging async races
as reported by bluhm@ on bugs@.

Note that nfs_root() already returns a "locked" node, that's why I'm
using vput(9) and not vrele(9).  However in the current context of NFS
these functions are identical since VOP_LOCK/UNLOCK are noop.

I have tested this on diskless & non-diskless, but I'm looking for more
tests 'cause this area is critical.

Comments?  Oks?

Index: nfs/nfs_node.c
===
RCS file: /cvs/src/sys/nfs/nfs_node.c,v
retrieving revision 1.66
diff -u -p -r1.66 nfs_node.c
--- nfs/nfs_node.c  28 Mar 2018 09:40:26 -  1.66
+++ nfs/nfs_node.c  3 Apr 2018 12:22:12 -
@@ -138,19 +138,7 @@ loop:
/* we now have an nfsnode on this vnode */
vp->v_flag &= ~VLARVAL;
np->n_vnode = vp;
-
rw_init(>n_commitlock, "nfs_commitlk");
-
-   /* 
-* Are we getting the root? If so, make sure the vnode flags
-* are correct 
-*/
-   if ((fhsize == nmp->nm_fhsize) && !bcmp(fh, nmp->nm_fh, fhsize)) {
-   if (vp->v_type == VNON)
-   vp->v_type = VDIR;
-   vp->v_flag |= VROOT;
-   }
-
np->n_fhp = >n_fh;
bcopy(fh, np->n_fhp, fhsize);
np->n_fhsize = fhsize;
Index: nfs/nfs_vfsops.c
===
RCS file: /cvs/src/sys/nfs/nfs_vfsops.c,v
retrieving revision 1.116
diff -u -p -r1.116 nfs_vfsops.c
--- nfs/nfs_vfsops.c10 Feb 2018 05:24:23 -  1.116
+++ nfs/nfs_vfsops.c3 Apr 2018 13:14:40 -
@@ -71,11 +71,13 @@ extern struct nfsstats nfsstats;
 extern int nfs_ticks;
 extern u_int32_t nfs_procids[NFS_NPROCS];
 
-intnfs_sysctl(int *, u_int, void *, size_t *, void *, size_t, 
struct proc *);
-intnfs_checkexp(struct mount *, struct mbuf *, int *, struct ucred 
**);
-struct mount   *nfs_mount_diskless(struct nfs_dlmount *, char *, int);
+intnfs_sysctl(int *, u_int, void *, size_t *, void *, size_t,
+   struct proc *);
+intnfs_checkexp(struct mount *, struct mbuf *, int *, struct ucred **);
+struct mount *nfs_mount_diskless(struct nfs_dlmount *, char *, int,
+   struct vnode **, struct proc *p);
 intmountnfs(struct nfs_args *, struct mount *, struct mbuf *,
-   const char *, char *);
+   const char *, char *, struct vnode **, struct proc *p);
 intnfs_quotactl(struct mount *, int, uid_t, caddr_t, struct proc *);
 intnfs_root(struct mount *, struct vnode **);
 intnfs_start(struct mount *, int, struct proc *);
@@ -123,15 +125,13 @@ nfs_statfs(struct mount *mp, struct stat
struct nfsmount *nmp = VFSTONFS(mp);
int error = 0, retattr;
struct ucred *cred;
-   struct nfsnode *np;
u_quad_t tquad;
 
info.nmi_v3 = (nmp->nm_flag & NFSMNT_NFSV3);
 
-   error = nfs_nget(mp, (nfsfh_t *)nmp->nm_fh, nmp->nm_fhsize, );
+   error = nfs_root(mp, );
if (error)
return (error);
-   vp = NFSTOV(np);
cred = crget();
cred->cr_ngroups = 0;
if (info.nmi_v3 && (nmp->nm_flag & NFSMNT_GOTFSINFO) == 0)
@@ -178,7 +178,7 @@ nfs_statfs(struct mount *mp, struct stat
copy_statfs_info(sbp, mp);
m_freem(info.nmi_mrep);
 nfsmout: 
-   vrele(vp);
+   vput(vp);
crfree(cred);
return (error);
 }
@@ -281,14 +281,14 @@ nfs_mountroot(void)
 */
if (nfs_boot_getfh(_diskless.nd_boot, "root", 
_diskless.nd_root, -1))
panic("nfs_mountroot: root");
-   mp = nfs_mount_diskless(_diskless.nd_root, "/", 0);
-   nfs_root(mp, );
+   mp = nfs_mount_diskless(_diskless.nd_root, "/", 0, , procp);
printf("root on %s\n", nfs_diskless.nd_root.ndm_host);
 
/*
 * Link it into the mount list.
 */
TAILQ_INSERT_TAIL(, mp, mnt_list);
+   rootvp = vp;
vfs_unbusy(mp);
 
/* Get root attributes (for the time). */
@@ -333,8 +333,8 @@ nfs_mountroot(void)
 */
error = nfs_boot_getfh(_diskless.nd_boot, "swap", 
_diskless.nd_swap, 5);
if (!error) {
-   mp = nfs_mount_diskless(_diskless.nd_swap, "/swap", 0);
-   nfs_root(mp, );
+   mp = nfs_mount_diskless(_diskless.nd_swap, "/swap", 0, ,
+   procp);
vfs_unbusy(mp);
 
/*
@@ -376,7 +376,8 @@ nfs_mountroot(void)
  * Internal version of mount system call for 

Re: [ed] fixing the list command

[Nils Reuße]

> Nils Reuße  hat am 27. Februar 2018 um 11:27 geschrieben:
> 
> 
> Hi there,
> 
> the 'l' command in base ed currently does not mark the end of line with a 
> '$'.  This is documented in the man page (POSIX wants each '$' escaped, and a 
> final '$' at EOL).  Interestingly, the code to mark EOL is already there, 
> just hidden by the BACKWARDS flag ed is compiled with.  Here is a small diff 
> that removes the BACKWARDS guard around the EOL marker.  The resulting 
> behavior is equal to GNU ed, base vi and vim (:set list).
> 
> Removing the BACKWARDS flag would fix it as well, but i don't see the need 
> for that.  There is some code that could be deleted in that case, though 
> (mostly different error messages).
> 
> Any comments?
> 
> Kind regards
> Nils

Now that both 6.3 and "Ed Mastery" by Michael W Lucas have been released, could 
this go in?

Nils

Re: Looking for testers for em(4) quirks patch

[Bryan Linton]

On 2018-04-02 11:52:08, Stefan Fritsch  wrote:
> Hi,
> 
> We have seen problems with em on i219V and i219LM. For example, "Hardware 
> Initialization Failed" if no cable is plugged in during boot, or watchdog 
> timeouts / hangs until next boot if the cable is removed while data is 
> transmitted.
> 
> This patch adds a bunch of quirks and fixes from freebsd.
> 
> It would be nice if people could give it a try on various em(4) hardware 
> to check if there are any regressions.
> 

I'm running it on an
em0 at pci0 dev 25 function 0 "Intel I217-LM" rev 0x04: msi, mac_type 
0x1d phy_type 0xb
in a Thinkpad T440p with no regressions seen so far.

I'm not sure if anything here touches the I217-LM chip or not, but
given some of the quirks in your list, I'm hoping that it might
fix this bug that I've been experiencing for a while now.

https://marc.info/?l=openbsd-bugs=150441840715439=2

It can take up to a week for it to show, so I'll run with the
patch for as long as I can and report back any findings.  As
stated above, I haven't seen any regressions.

So far so good!

-- 
Bryan

Re: hppa MI mutex

[Martin Pieuchot]

On 03/04/18(Tue) 12:06, Mark Kettenis wrote:
> > Date: Tue, 3 Apr 2018 11:16:45 +0200
> > From: Martin Pieuchot 
> > 
> > Here's a diff to switch hppa to the MI mutex implementation.  I'm
> > looking for testers, as I don't own such hardware.
> > 
> > Note that our MI mutex implementation relies on atomic_cas_ptr(9).  On
> > hppa all the calls to this function are serialized on a single lock. I
> > don't believe it will introduce contention at this point since the
> > KERNEL_LOCK() is still the problem.  However if it becomes a bottleneck
> > we could use multiple locks in a small hash table.
> 
> I don't have such hardware either, but I'm not in favour of doing
> this.  PA-RISC is "challenged" but at least the locking primitive it
> supplies is suitable for implementing spin locks.  Emulating CAS with
> spin locks to implement a spin lock feels so wrong.  There are also
> some concerns about guarantees of forward progress in such an
> implementation.

Well I would prefer if we could solve these concerns because we are already
using atomic operations.

So what are the options?

1. Keep hppa mutex MD and start diverging with the MI mutex

2. Add another MI layer of abstraction on top of the atomic*(9) API
   because of hppa

3. Emulate the atomic*(9) API as well as we can on hppa

I picked 3 because I don't want to add more complexity for an arch
which is not widely used.  Who care if it feels wrong to emulate a CAS
with a hw spinlock?  I believed that what we need is locking primitives
that we can debug and analyse.  If hppa can benefit from that and help us
find bugs, that's even better :)

Do you see another alternative?

Re: hppa MI mutex

[Mark Kettenis]

> Date: Tue, 3 Apr 2018 11:16:45 +0200
> From: Martin Pieuchot 
> 
> Here's a diff to switch hppa to the MI mutex implementation.  I'm
> looking for testers, as I don't own such hardware.
> 
> Note that our MI mutex implementation relies on atomic_cas_ptr(9).  On
> hppa all the calls to this function are serialized on a single lock. I
> don't believe it will introduce contention at this point since the
> KERNEL_LOCK() is still the problem.  However if it becomes a bottleneck
> we could use multiple locks in a small hash table.

I don't have such hardware either, but I'm not in favour of doing
this.  PA-RISC is "challenged" but at least the locking primitive it
supplies is suitable for implementing spin locks.  Emulating CAS with
spin locks to implement a spin lock feels so wrong.  There are also
some concerns about guarantees of forward progress in such an
implementation.

> Index: arch/hppa/conf/files.hppa
> ===
> RCS file: /cvs/src/sys/arch/hppa/conf/files.hppa,v
> retrieving revision 1.97
> diff -u -p -r1.97 files.hppa
> --- arch/hppa/conf/files.hppa 5 Jun 2017 18:59:07 -   1.97
> +++ arch/hppa/conf/files.hppa 12 Feb 2018 10:08:57 -
> @@ -298,7 +298,6 @@ file  arch/hppa/hppa/fpu.c
>  file arch/hppa/hppa/ipi.cmultiprocessor
>  file arch/hppa/hppa/lock_machdep.c   multiprocessor
>  file arch/hppa/hppa/machdep.c
> -file arch/hppa/hppa/mutex.c
>  file arch/hppa/hppa/pmap.c
>  file arch/hppa/hppa/process_machdep.c
>  file arch/hppa/hppa/sys_machdep.c
> Index: arch/hppa/hppa/genassym.cf
> ===
> RCS file: /cvs/src/sys/arch/hppa/hppa/genassym.cf,v
> retrieving revision 1.47
> diff -u -p -r1.47 genassym.cf
> --- arch/hppa/hppa/genassym.cf9 Feb 2015 08:20:13 -   1.47
> +++ arch/hppa/hppa/genassym.cf12 Feb 2018 10:15:56 -
> @@ -45,7 +45,7 @@ include 
>  include 
>  include 
>  include 
> -include 
> +include 
>  include 
>  include 
>  include 
> Index: arch/hppa/hppa/ipi.c
> ===
> RCS file: /cvs/src/sys/arch/hppa/hppa/ipi.c,v
> retrieving revision 1.5
> diff -u -p -r1.5 ipi.c
> --- arch/hppa/hppa/ipi.c  8 Sep 2017 05:36:51 -   1.5
> +++ arch/hppa/hppa/ipi.c  12 Feb 2018 10:16:01 -
> @@ -25,7 +25,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  
>  void hppa_ipi_nop(void);
> Index: arch/hppa/hppa/mutex.c
> ===
> RCS file: arch/hppa/hppa/mutex.c
> diff -N arch/hppa/hppa/mutex.c
> --- arch/hppa/hppa/mutex.c20 Apr 2017 13:57:29 -  1.16
> +++ /dev/null 1 Jan 1970 00:00:00 -
> @@ -1,156 +0,0 @@
> -/*   $OpenBSD: mutex.c,v 1.16 2017/04/20 13:57:29 visa Exp $ */
> -
> -/*
> - * Copyright (c) 2004 Artur Grabowski 
> - * All rights reserved. 
> - *
> - * Redistribution and use in source and binary forms, with or without 
> - * modification, are permitted provided that the following conditions 
> - * are met: 
> - *
> - * 1. Redistributions of source code must retain the above copyright 
> - *notice, this list of conditions and the following disclaimer. 
> - * 2. The name of the author may not be used to endorse or promote products
> - *derived from this software without specific prior written permission. 
> - *
> - * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES,
> - * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY
> - * AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> - * THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> - * EXEMPLARY, OR CONSEQUENTIAL  DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> - * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR 
> PROFITS;
> - * OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
> - * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
> - * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
> - * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
> - */
> -
> -#include 
> -#include 
> -#include 
> -#include 
> -
> -#include 
> -
> -#include 
> -
> -int __mtx_enter_try(struct mutex *);
> -
> -#ifdef MULTIPROCESSOR
> -/* Note: lock must be 16-byte aligned. */
> -#define __mtx_lock(mtx) ((int *)(((vaddr_t)mtx->mtx_lock + 0xf) & ~0xf))
> -#endif
> -
> -void
> -__mtx_init(struct mutex *mtx, int wantipl)
> -{
> -#ifdef MULTIPROCESSOR
> - mtx->mtx_lock[0] = 1;
> - mtx->mtx_lock[1] = 1;
> - mtx->mtx_lock[2] = 1;
> - mtx->mtx_lock[3] = 1;
> -#endif
> - mtx->mtx_wantipl = wantipl;
> - mtx->mtx_oldipl = IPL_NONE;
> - mtx->mtx_owner = NULL;
> -}
> -
> -#ifdef MULTIPROCESSOR
> -void
> -__mtx_enter(struct mutex *mtx)
> -{
> - while (__mtx_enter_try(mtx) == 0)
> - 

hppa MI mutex

[Martin Pieuchot]

Here's a diff to switch hppa to the MI mutex implementation.  I'm
looking for testers, as I don't own such hardware.

Note that our MI mutex implementation relies on atomic_cas_ptr(9).  On
hppa all the calls to this function are serialized on a single lock. I
don't believe it will introduce contention at this point since the
KERNEL_LOCK() is still the problem.  However if it becomes a bottleneck
we could use multiple locks in a small hash table.


Index: arch/hppa/conf/files.hppa
===
RCS file: /cvs/src/sys/arch/hppa/conf/files.hppa,v
retrieving revision 1.97
diff -u -p -r1.97 files.hppa
--- arch/hppa/conf/files.hppa   5 Jun 2017 18:59:07 -   1.97
+++ arch/hppa/conf/files.hppa   12 Feb 2018 10:08:57 -
@@ -298,7 +298,6 @@ filearch/hppa/hppa/fpu.c
 file   arch/hppa/hppa/ipi.cmultiprocessor
 file   arch/hppa/hppa/lock_machdep.c   multiprocessor
 file   arch/hppa/hppa/machdep.c
-file   arch/hppa/hppa/mutex.c
 file   arch/hppa/hppa/pmap.c
 file   arch/hppa/hppa/process_machdep.c
 file   arch/hppa/hppa/sys_machdep.c
Index: arch/hppa/hppa/genassym.cf
===
RCS file: /cvs/src/sys/arch/hppa/hppa/genassym.cf,v
retrieving revision 1.47
diff -u -p -r1.47 genassym.cf
--- arch/hppa/hppa/genassym.cf  9 Feb 2015 08:20:13 -   1.47
+++ arch/hppa/hppa/genassym.cf  12 Feb 2018 10:15:56 -
@@ -45,7 +45,7 @@ include 
 include 
 include 
 include 
-include 
+include 
 include 
 include 
 include 
Index: arch/hppa/hppa/ipi.c
===
RCS file: /cvs/src/sys/arch/hppa/hppa/ipi.c,v
retrieving revision 1.5
diff -u -p -r1.5 ipi.c
--- arch/hppa/hppa/ipi.c8 Sep 2017 05:36:51 -   1.5
+++ arch/hppa/hppa/ipi.c12 Feb 2018 10:16:01 -
@@ -25,7 +25,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 
 void hppa_ipi_nop(void);
Index: arch/hppa/hppa/mutex.c
===
RCS file: arch/hppa/hppa/mutex.c
diff -N arch/hppa/hppa/mutex.c
--- arch/hppa/hppa/mutex.c  20 Apr 2017 13:57:29 -  1.16
+++ /dev/null   1 Jan 1970 00:00:00 -
@@ -1,156 +0,0 @@
-/* $OpenBSD: mutex.c,v 1.16 2017/04/20 13:57:29 visa Exp $ */
-
-/*
- * Copyright (c) 2004 Artur Grabowski 
- * All rights reserved. 
- *
- * Redistribution and use in source and binary forms, with or without 
- * modification, are permitted provided that the following conditions 
- * are met: 
- *
- * 1. Redistributions of source code must retain the above copyright 
- *notice, this list of conditions and the following disclaimer. 
- * 2. The name of the author may not be used to endorse or promote products
- *derived from this software without specific prior written permission. 
- *
- * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES,
- * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY
- * AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
- * THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
- * EXEMPLARY, OR CONSEQUENTIAL  DAMAGES (INCLUDING, BUT NOT LIMITED TO,
- * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS;
- * OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
- * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
- * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
- * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
- */
-
-#include 
-#include 
-#include 
-#include 
-
-#include 
-
-#include 
-
-int __mtx_enter_try(struct mutex *);
-
-#ifdef MULTIPROCESSOR
-/* Note: lock must be 16-byte aligned. */
-#define __mtx_lock(mtx) ((int *)(((vaddr_t)mtx->mtx_lock + 0xf) & ~0xf))
-#endif
-
-void
-__mtx_init(struct mutex *mtx, int wantipl)
-{
-#ifdef MULTIPROCESSOR
-   mtx->mtx_lock[0] = 1;
-   mtx->mtx_lock[1] = 1;
-   mtx->mtx_lock[2] = 1;
-   mtx->mtx_lock[3] = 1;
-#endif
-   mtx->mtx_wantipl = wantipl;
-   mtx->mtx_oldipl = IPL_NONE;
-   mtx->mtx_owner = NULL;
-}
-
-#ifdef MULTIPROCESSOR
-void
-__mtx_enter(struct mutex *mtx)
-{
-   while (__mtx_enter_try(mtx) == 0)
-   ;
-}
-
-int
-__mtx_enter_try(struct mutex *mtx)
-{
-   struct cpu_info *ci = curcpu();
-   volatile int *lock = __mtx_lock(mtx);
-   int ret;
-   int s;
-
-   if (mtx->mtx_wantipl != IPL_NONE)
-   s = splraise(mtx->mtx_wantipl);
-
-#ifdef DIAGNOSTIC
-   if (__predict_false(mtx->mtx_owner == ci))
-   panic("mtx %p: locking against myself", mtx);
-#endif
-
-   asm volatile (
-   "ldcws  0(%2), %0"
-   : "=" (ret), "+m" (lock)
-   : "r" (lock)
-   );
-
-   if (ret) {
-   membar_enter();
-   mtx->mtx_owner = ci;
-  

Re: autoconf.9 consistency

[Jason McIntyre]

morning.

to be honest, I think it's fine as it is now. the intention was obviously to 
keep the two systems separate. but I don't strongly object to your diff either, 
so if you feel it is more useful to have it in a more standard format, fine.

jmc, sitting on the fence...


  Original Message  
From: da...@gwynne.id.au
Sent: 3 April 2018 5:01 am
To: j...@openbsd.org; schwa...@openbsd.org
Cc: tech@openbsd.org
Subject: autoconf.9 consistency

i landed on this manpage and got confused cos it looked different
to the rest of them. this shuffles it a bit to make it less different.

the main change is to move the function prototypes up to the synopsis.
the other, less important change is to replace some headers with
subheaders.

ok?

Index: autoconf.9
===
RCS file: /cvs/src/share/man/man9/autoconf.9,v
retrieving revision 1.16
diff -u -p -r1.16 autoconf.9
--- autoconf.9  11 Dec 2015 16:07:02 -  1.16
+++ autoconf.9  3 Apr 2018 03:58:20 -
@@ -41,6 +41,21 @@
.Sh SYNOPSIS
.In sys/param.h
.In sys/device.h
+.Ft "void *"
+.Fn config_search "cfmatch_t func" "struct device *parent" "void *aux"
+.Ft "void *"
+.Fn config_rootsearch "cfmatch_t func" "char *rootname" "void *aux"
+.Ft "struct device *"
+.Fo config_found_sm
+.Fa "struct device *parent"
+.Fa "void *aux"
+.Fa "cfprint_t print"
+.Fa "cfmatch_t submatch"
+.Fc
+.Ft "struct device *"
+.Fn config_found "struct device *parent" "void *aux" "cfprint_t print"
+.Ft "struct device *"
+.Fn config_rootfound "char *rootname" "void *aux"
.Sh DESCRIPTION
Autoconfiguration is the process of matching hardware devices with an
appropriate device driver.
@@ -77,14 +92,7 @@ ends with a unit number.
The unit number identifies an instance of the driver.
Device data structures are allocated dynamically during autoconfiguration,
giving a unique address for each instance.
-.Sh INDIRECT CONFIGURATION
-.nr nS 1
-.Ft "void *"
-.Fn config_search "cfmatch_t func" "struct device *parent" "void *aux"
-.Ft "void *"
-.Fn config_rootsearch "cfmatch_t func" "char *rootname" "void *aux"
-.nr nS 0
-.Pp
+.Ss Indirect Configuration
The
.Fn config_search
function performs indirect configuration of physical devices by iterating
@@ -142,17 +150,7 @@ itself.
Note that this function is designed so that it can be used to apply an
arbitrary function to all potential children.
In this case callers may choose to ignore the return value.
-.Sh DIRECT CONFIGURATION
-.nr nS 1
-.Ft "struct device *"
-.Fn config_found_sm "struct device *parent" "void *aux" "cfprint_t print" \
-    "cfmatch_t submatch"
-.Ft "struct device *"
-.Fn config_found "struct device *parent" "void *aux" "cfprint_t print"
-.Ft "struct device *"
-.Fn config_rootfound "char *rootname" "void *aux"
-.nr nS 0
-.Pp
+.Ss Direct Configuration
The
.Fn config_found_sm
function performs direct configuration on a physical device.

Re: Looking for testers for em(4) quirks patch

[Mike Larkin]

On Mon, Apr 02, 2018 at 11:52:08AM +0200, Stefan Fritsch wrote:
> Hi,
> 
> We have seen problems with em on i219V and i219LM. For example, "Hardware 
> Initialization Failed" if no cable is plugged in during boot, or watchdog 
> timeouts / hangs until next boot if the cable is removed while data is 
> transmitted.
> 
> This patch adds a bunch of quirks and fixes from freebsd.
> 
> It would be nice if people could give it a try on various em(4) hardware 
> to check if there are any regressions.
> 

No regressions on

em0 at pci0 dev 25 function 0 "Intel 82579LM" rev 0x04: msi, mac_type 0x1c 
phy_type 0xa

(my x230).

-ml

> Comments on the patch are of course welcome, too.
> 
> Cheers,
> Stefan
> 
> * Some em chips have a semaphore ("software flag") to synchronize access
>   to certain registers between OS and firmware (ME/AMT).
> 
>   Make the logic to get the flag match the logic in freebsd. This includes
>   higher timeouts and waiting for a previous unlock to complete before
>   trying a lock again.
> 
> * Another problem was that openbsd em driver calls em_get_software_flag
>   recursively, which causes the semaphore to be unlocked too early. Make
>   em_get_software_flag/em_release_software_flag handle this correctly.
>   Freebsd does not do this, but they have a mutex that probably allows
>   them to detect recursive calls to e1000_acquire_swflag_ich8lan().
>   Reworking the openbsd driver to not recursively get the semaphore would
>   be very invasive.
> 
> * Port the logic from freebsd to em_check_phy_reset_block(). A single
>   read does not seem to be reliable.
> 
> * Also, increase delay after reset to 20ms, which is the value in freebsd
>   for ich8lan.
> 
> * Add another magic 1ms delay that seems to help with some remaining issues
>   on an HP elitebook 820 G3. A printf() at the same place helps, too.
> 
> * Port a silicon errata workaround from FreeBSD.
> 
>   
> https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/i218-i219-ethernet-connection-spec-update.pdf?asset=9561
> 
> * While there, print mac+phy type in em_attach(). This makes it easier to
>   determine which quirks are hit/missing when comparing to freebsd.
> 
> * Also print em_init_hw() error code if something goes wrong.
> 
> 
> diff --git sys/dev/pci/if_em.c sys/dev/pci/if_em.c
> index ec8e35245ef..f6a1f1c3894 100644
> --- sys/dev/pci/if_em.c
> +++ sys/dev/pci/if_em.c
> @@ -557,6 +557,8 @@ em_attach(struct device *parent, struct device *self, 
> void *aux)
>   if (!defer)
>   em_update_link_status(sc);
>  
> + printf(", mac_type %#x phy_type %#x", sc->hw.mac_type,
> + sc->hw.phy_type);
>   printf(", address %s\n", ether_sprintf(sc->sc_ac.ac_enaddr));
>  
>   /* Indicate SOL/IDER usage */
> @@ -1860,8 +1862,8 @@ em_hardware_init(struct em_softc *sc)
>   INIT_DEBUGOUT("\nHardware Initialization Deferred ");
>   return (EAGAIN);
>   }
> - printf("\n%s: Hardware Initialization Failed\n",
> -DEVNAME(sc));
> + printf("\n%s: Hardware Initialization Failed: %d\n",
> +DEVNAME(sc), ret_val);
>   return (EIO);
>   }
>  
> @@ -2265,7 +2267,9 @@ em_initialize_transmit_unit(struct em_softc *sc)
>   EM_WRITE_REG(>hw, E1000_IOSFPC, reg_val);
>  
>   reg_val = E1000_READ_REG(>hw, TARC0);
> - reg_val |= E1000_TARC0_CB_MULTIQ_3_REQ;
> + /* i218-i219 Specification Update 1.5.4.5 */
> +reg_val &= ~E1000_TARC0_CB_MULTIQ_3_REQ;
> +reg_val |= E1000_TARC0_CB_MULTIQ_2_REQ;
>   E1000_WRITE_REG(>hw, TARC0, reg_val);
>   }
>  }
> diff --git sys/dev/pci/if_em_hw.c sys/dev/pci/if_em_hw.c
> index df0fa571736..d122e727875 100644
> --- sys/dev/pci/if_em_hw.c
> +++ sys/dev/pci/if_em_hw.c
> @@ -945,7 +945,9 @@ em_reset_hw(struct em_hw *hw)
>   }
>   em_get_software_flag(hw);
>   E1000_WRITE_REG(hw, CTRL, (ctrl | E1000_CTRL_RST));
> - msec_delay(5);
> + /* HW reset releases software_flag */
> + hw->sw_flag = 0;
> + msec_delay(20);
>  
>   /* Ungate automatic PHY configuration on non-managed 82579 */
>   if (hw->mac_type == em_pch2lan && !hw->phy_reset_disable &&
> @@ -1491,6 +1493,8 @@ em_init_hw(struct em_hw *hw)
>   /* Set the media type and TBI compatibility */
>   em_set_media_type(hw);
>  
> + /* Magic delay that improves problems with i219LM on HP Elitebook */
> + msec_delay(1);
>   /* Must be called after em_set_media_type because media_type is used */
>   em_initialize_hardware_bits(hw);
>  
> @@ -9539,9 +9543,18 @@ em_check_phy_reset_block(struct em_hw *hw)
>   DEBUGFUNC("em_check_phy_reset_block\n");
>  
>   if (IS_ICH8(hw->mac_type)) {
> - fwsm = E1000_READ_REG(hw, FWSM);
> - return (fwsm & 

Re: sparc64 softraid boot: workaround for memory leak

[Mike Larkin]

On Thu, Mar 29, 2018 at 09:21:16AM +0200, Mark Kettenis wrote:
> > Date: Wed, 28 Mar 2018 23:46:49 -0700
> > From: Mike Larkin 
> > 
> > On Thu, Mar 29, 2018 at 08:40:27AM +0200, Stefan Sperling wrote:
> > > On Mon, Mar 12, 2018 at 11:38:14AM +0100, Stefan Sperling wrote:
> > > > I haven't had any feedback on the previous diff. However, I felt it
> > > > was a bit of a hack, so I tried to come up with a cleaner solution.
> > > 
> > > Anyone? Can this go in now? I hope to get this tested across
> > > many sparc64 machines during the 6.4 release cycle.
> > > 
> > 
> > Wish I could help but my T5240 has never been able to run any ldoms. 
> > Something
> > about mismatched firmware versions. ldomctl(8) just generates interrupt
> > storms.
> 
> Even with all the fixes that stsp@ made during the 6.3 release cycle?
> 

It does look like some things have been fixed. I still can't launch ldoms
but I believe this due to some local error, not anything in the code. All the
commands work now, it's just that after reboot the ldom isn't started.

I'll see if I can diagnose more this week, but that machine is a beast and
really noisy.

-ml

> > > > The diff below opens the softraid boot chunk early on and keeps the
> > > > handle open until a kernel has been loaded from the softraid volume.
> > > > This means sr_strategy() does not have to worry about obtaining a
> > > > handle from the firmware anymore.
> > > > 
> > > > Also consolidate some repeated lines of code in sr_strategy().
> > > > 
> > > > Tested on a T5220 ldom guest (CRYPTO) and a v240 machine (RAID1).
> > > > 
> > > > Index: boot.c
> > > > ===
> > > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/boot.c,v
> > > > retrieving revision 1.27
> > > > diff -u -p -r1.27 boot.c
> > > > --- boot.c  11 Sep 2016 17:53:26 -  1.27
> > > > +++ boot.c  12 Mar 2018 10:26:23 -
> > > > @@ -248,6 +248,8 @@ loadfile(int fd, char *args)
> > > > close(fd);
> > > >  
> > > >  #ifdef SOFTRAID
> > > > +   if (bootdev_dip)
> > > > +   OF_close(bootdev_dip->sr_handle);
> > > > sr_clear_keys();
> > > >  #endif
> > > > chain(entry, args, ssym, esym);
> > > > @@ -283,12 +285,11 @@ fail:
> > > >  }
> > > >  
> > > >  #ifdef SOFTRAID
> > > > -/* Set bootdev_dip to the software boot volume, if specified. */
> > > > +/* Set bootdev_dip to the softraid boot volume, if specified. */
> > > >  static int
> > > >  srbootdev(const char *bootline)
> > > >  {
> > > > struct sr_boot_volume *bv;
> > > > -   struct diskinfo *dip;
> > > > int unit;
> > > >  
> > > > bootdev_dip = NULL;
> > > > @@ -320,9 +321,23 @@ srbootdev(const char *bootline)
> > > > return EPERM;
> > > >  
> > > > if (bv->sbv_diskinfo == NULL) {
> > > > +   struct sr_boot_chunk *bc;
> > > > +   struct diskinfo *dip, *bc_dip;
> > > > +   int sr_handle;
> > > > +
> > > > +   /* All reads will come from the boot chunk. */
> > > > +   bc = sr_vol_boot_chunk(bv);
> > > > +   if (bc == NULL)
> > > > +   return ENXIO;
> > > > +   bc_dip = (struct diskinfo *)bc->sbc_diskinfo;
> > > > +   sr_handle = OF_open(bc_dip->path);
> > > > +   if (sr_handle == -1)
> > > > +   return EIO;
> > > > +
> > > > dip = alloc(sizeof(struct diskinfo));
> > > > bzero(dip, sizeof(*dip));
> > > > dip->sr_vol = bv;
> > > > +   dip->sr_handle = sr_handle;
> > > > bv->sbv_diskinfo = dip;
> > > > }
> > > >  
> > > > @@ -331,7 +346,8 @@ srbootdev(const char *bootline)
> > > >  
> > > > /* Attempt to read disklabel. */
> > > > bv->sbv_part = 'c';
> > > > -   if (sr_getdisklabel(bv, >disklabel)) {
> > > > +   if (sr_getdisklabel(bv, _dip->disklabel)) {
> > > > +   OF_close(bootdev_dip->sr_handle);
> > > > free(bv->sbv_diskinfo, sizeof(struct diskinfo));
> > > > bv->sbv_diskinfo = NULL;
> > > > bootdev_dip = NULL;
> > > > Index: disk.h
> > > > ===
> > > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/disk.h,v
> > > > retrieving revision 1.1
> > > > diff -u -p -r1.1 disk.h
> > > > --- disk.h  26 Nov 2014 20:30:41 -  1.1
> > > > +++ disk.h  12 Mar 2018 10:26:23 -
> > > > @@ -36,7 +36,10 @@
> > > >  struct diskinfo {
> > > > char path[256];
> > > > struct disklabel disklabel;
> > > > +
> > > > +   /* Used during softraid boot. */
> > > >