Re: CVS commit: src/sys/kern
Date:Tue, 04 Oct 2022 10:09:35 -0400 From:Christos Zoulas Message-ID: <8dd220d16861eb3a890461bdf02d1...@zoulas.com> | I always forget the O_CLOEXEC is special | in that regard. I wish it was not, but it is difficult to fix. POSIX is adding O_CLOFORK in the next version (no guarantees I remembered the symbol name spelling correctly here) which will have essentially the same (open time) semantics (similar long term sematics as well, just applied at a different time). I assume we will need to add that at some point or other. | The question is how to find the vnode? Not really, I assume that part will be fairly easy (probably trivial), I just didn't have the energy to go work it out when sending that mail. We have the file descriptor, and I suspect the file* (need to check to make sure the right one is immediately available, but we can get it from the fd if not), we know it refers to a vnode (it came from vn_open()), so getting the vnode* from the file* is not something difficult, I think. | Perhaps it is easiest to fail the open call if O_EXLOCK or | O_SHLOCK are specified in a cloning open? That would be an option, and is better than just ignoring them, but better still would be to make them work. Since open_setfp() does nothing (much) when none of the relevant O_xxx flags that it tests are set (the fd open flags, as distinct from the fp ones), and the code calls VOP_UNLOCK(vp) after it, we know that vp is intended to be locked when open_setfp() is called (further confirmed as when any of the O_??LOCK flags is set, open_setfp() does a VOP_UNLOCK() and later a vn_lock() (which I am guessing is the inverse). Maybe all that's needed is a vn_lock() call (on the vp that we still need to fetch) and then call open_setfp() ? But this is all beyond what I know enough about to be sure, particularly to avoid doing anything which might deadlock, etc. kre ps: if this gets done properly, then special case code to handle O_NONBLOCK (and O_NOSIGPIPE, ...) in cloning device drivers won't be needed either, open_setfp() is where all of that is normally added to the file* for the fd being returned, it was not "simply happening" because that call is missing in the cloned device case.
Re: CVS commit: src/sys/kern
On 2022-10-01 3:39 pm, Robert Elz wrote:
Date:Sat, 1 Oct 2022 13:00:04 -0400
[stuff deleted]
Even when it is called, the code is:
fp->f_flag = flags & FMASK;
where FMASK is (from fcntl.h)
#define FMASK (FREAD|FWRITE|FCNTLFLAGS|FEXEC)
and
#define FCNTLFLAGS
(FAPPEND|FASYNC|FFSYNC|FNONBLOCK|FDSYNC|FRSYNC|FALTIO|\
FDIRECT|FNOSIGPIPE)
which all looks exactly as it should be to me - and note that O_CLOEXEC
(which has no F equivalent name - it doesn't need one) is not
there.
So, fp->f_flag isn't set at all (is probably 0), and even if it were
O_CLOEXEC would not be there, and should not be.
Thanks for pointing that out, I always forget the O_CLOEXEC is special
in that regard. I wish it was not, but it is difficult to fix.
For the vnode actually being opened, none of this matters, as the open
lasts as long (actually not as long) as the open() sys call - when that
returns, the device being opened has been closed again, so what the
file that refers to it looks like (or would have looked like) really
doesn't matter at all, it never becomes visible. That's my guess as
to why the open_setfp() call is missing in that case.
But what got forgotten (or deliberately was not done) was anything to
affect the modes of the clone which is opened.
| What does it mean when the open specifies O_CLOEXEC
| and ff->ff_exclose is false? Can that happen? Is that desirable?
It is what is currently happening whenever we open a cloning device.
(Or that is what it looks like to me). Desirable, no idea, I didn't
define the semantics of cloning device opens, nor which of the open
flags should apply to the clone that is created.
| I am fine with the locking to stay where it is. I guess it is
probably
| not needed after dup/clone, since the underlying vnode is shared...
Assuming you mean dup(2) (and dup2()) and clone(2) then no - those have
no way to pass the relevant locking flags, if you have a fd and want to
apply a lock, fcntl() is what does that (and the fcntl operations that
duplicate fds do not also apply locks).
The only issue is O_EXLOCK and O_SHLOCK (and O_CLOEXEC) for cloned
devices.
I suspect it makes sense for O_CLOEXEC to be applied in that case, it
makes little sense for open("/dev/ptmx", O_RDWR|O_CLOEXEC) to succeed,
returning an open fd which does not have cloexec set (which is the
issue,
along with O_NONBLOCK) which started all of this discussion.
The locking flags I am less sure about. I don't see how they can fail
to succeed if applied, as the vnode for the device has just been
created,
nothing else can possibly have any kind of lock on it. Whether
there's
any benefit in applying the lock so that the node is locked for later,
I don't know - but it certainly should do no harm to do that.
It seems clear to me that what we need is (something like)
Index: vfs_syscalls.c
===
RCS file: /cvsroot/src/sys/kern/vfs_syscalls.c,v
retrieving revision 1.555
diff -u -r1.555 vfs_syscalls.c
--- vfs_syscalls.c 12 Feb 2022 15:51:29 - 1.555
+++ vfs_syscalls.c 1 Oct 2022 19:27:15 -
@@ -1763,6 +1763,9 @@
error = fd_dupopen(dupfd, dupfd_move, flags, );
if (error)
return error;
+ error = open_setfp(l, fp, XXXvp, indx, flags);
+ if (error)
+ return error;
*fd = indx;
} else {
error = open_setfp(l, fp, vp, indx, flags);
where XXXvp needs to be extracted from somewhere (it isn't vp, as
vp==NULL)
except that what follows in the else case is ...
if (error)
return error;
VOP_UNLOCK(vp);
*fd = indx;
That VOP_UNLOCK(vp) is what is bothering me, It tells me that
open_setfp()
is expecting to be called with vp locked - but in the first case (the
cloning
case) there is no VOP_UNLOCK() call (and what's more, fd_dupopen()
cannot
do it, as it has no vp arg). That means, I believe, that when
vn_open()
returns in the normal case, vp is returned locked, but in the cloning
case
the vnode that was created for the clone is not locked.
I'm not sure what is the right way to find the vnode, or how it should
properly be locked so open_setfp() can do its thing. If I knew all of
that I would have made an attempt at fixing this already. We need
someone who really understands what is happening here, and the right
way to handle it all (which very likely is nothing like I just
suggested).
The question is how to find the vnode? Perhaps it is easiest to fail the
open call if O_EXLOCK or O_SHLOCK are specified in a cloning open? At
least
we will not silently ignore them?
Best,
christos
Re: CVS commit: src/sys/dev
On 2022/10/04 19:22, Taylor R Campbell wrote: >> Date: Tue, 4 Oct 2022 17:16:35 +0900 >> From: Masanobu SAITOH >> >> Before reverting changes, one of my machine which use serial console >> didn't print the Copyright message. >> >> Revert two revert commit, i.e. >> http://mail-index.netbsd.org/source-changes/2022/10/04/msg141389.html >> http://mail-index.netbsd.org/source-changes/2022/10/04/msg141388.html >> and applied consokfix.patch. >> [...] >> The boot message is not printed and I can see messages after /sbin/init. >> dmesg(1) shows the kernel messages. > > Can you please try with consokfix.patch _and_ consprintfix.patch? > > consprintfix.patch should restore the kernel messages on the console. It worked! -- --- SAITOH Masanobu (msai...@execsw.org msai...@netbsd.org)
Re: CVS commit: src/sys/dev
> Date: Tue, 4 Oct 2022 17:16:35 +0900 > From: Masanobu SAITOH > > Before reverting changes, one of my machine which use serial console > didn't print the Copyright message. > > Revert two revert commit, i.e. > http://mail-index.netbsd.org/source-changes/2022/10/04/msg141389.html > http://mail-index.netbsd.org/source-changes/2022/10/04/msg141388.html > and applied consokfix.patch. > [...] > The boot message is not printed and I can see messages after /sbin/init. > dmesg(1) shows the kernel messages. Can you please try with consokfix.patch _and_ consprintfix.patch? consprintfix.patch should restore the kernel messages on the console. >From 0f058a0e89e3f545a9020a2fb79dadd7ad89029a Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 4 Oct 2022 05:48:39 + Subject: [PATCH] squash! constty(4): Make MP-safe. - Fix reversed sense of conditional. --- sys/kern/subr_prf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sys/kern/subr_prf.c b/sys/kern/subr_prf.c index e87e6efc8501..53fb20c1d393 100644 --- a/sys/kern/subr_prf.c +++ b/sys/kern/subr_prf.c @@ -425,7 +425,7 @@ putone(int c, int flags, struct tty *tp) if ((flags & TOLOG) && c != '\0' && c != '\r' && c != 0177) logputchar(c); - if ((flags & TOCONS) && ctp != NULL && c != '\0') + if ((flags & TOCONS) && ctp == NULL && c != '\0') (*v_putc)(c); pserialize_read_exit(s);
Re: CVS commit: src/sys/dev
Hi. On 2022/10/04 16:24, Ryo ONODERA wrote: > Hi, > > Taylor R Campbell writes: > >>> Date: Tue, 04 Oct 2022 15:53:58 +0900 >>> From: Ryo ONODERA >>> >>> With this patch, it works fine for me. >>> There is no stall after genfb(4). >>> And I do not find any other problem so far. >> >> Thanks! There probably is another problem which is that kernel >> console printfs might stop appearing after a certain point, which the >> following patch might fix too -- I inadvertently reversed the sense >> of a conditional in the subr_prf.c changes. > > I have not encountered another problem yet. However with your > consprintfix.patch, it works fine for me too. Before reverting changes, one of my machine which use serial console didn't print the Copyright message. Revert two revert commit, i.e. http://mail-index.netbsd.org/source-changes/2022/10/04/msg141389.html http://mail-index.netbsd.org/source-changes/2022/10/04/msg141388.html and applied consokfix.patch. > NetBSD MBR boot > > NetBSD/x86 ffsv2 Primary Bootstrap > > 0 seconds. > booting hd0a:netbsd (howto 0xa) > 72567816+35522344+2226392 [945568+1555416+1162803]=0x6d7f2a8 > Loading /var/db/entropy-file > Tue Oct 4 17:07:44 JST 2022 > Starting root file system check: > /dev/rwd0a: file system is clean; not checking > Setting sysctl variables: ... The boot message is not printed and I can see messages after /sbin/init. dmesg(1) shows the kernel messages. Regards. > Thank you. > >> From 0f058a0e89e3f545a9020a2fb79dadd7ad89029a Mon Sep 17 00:00:00 2001 >> From: Taylor R Campbell >> Date: Tue, 4 Oct 2022 05:48:39 + >> Subject: [PATCH] squash! constty(4): Make MP-safe. >> >> - Fix reversed sense of conditional. >> --- >> sys/kern/subr_prf.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/sys/kern/subr_prf.c b/sys/kern/subr_prf.c >> index e87e6efc8501..53fb20c1d393 100644 >> --- a/sys/kern/subr_prf.c >> +++ b/sys/kern/subr_prf.c >> @@ -425,7 +425,7 @@ putone(int c, int flags, struct tty *tp) >> if ((flags & TOLOG) && >> c != '\0' && c != '\r' && c != 0177) >> logputchar(c); >> -if ((flags & TOCONS) && ctp != NULL && c != '\0') >> +if ((flags & TOCONS) && ctp == NULL && c != '\0') >> (*v_putc)(c); >> >> pserialize_read_exit(s); > -- --- SAITOH Masanobu (msai...@execsw.org msai...@netbsd.org)
Re: CVS commit: src/sys/arch
On 2022/10/04 16:38, matthew green wrote: "Rin Okuyama" writes: Module Name:src Committed By: rin Date: Tue Oct 4 07:24:32 UTC 2022 Modified Files: src/sys/arch/amiga/dev: ser.c src/sys/arch/sgimips/dev: scn.c Log Message: Remove unused extern declaration of constty. thank you. Welcome! can someone please find all the "extern ;" in .c files, and move them all into .h files. Well, it should be a tough work ;) % cd /usr/src/sys && find . -name '*.c' | grep -v /external/ | xargs grep 'extern[ ]' | wc -l 4170 Thanks, rin
re: CVS commit: src/sys/arch
"Rin Okuyama" writes: > Module Name: src > Committed By: rin > Date: Tue Oct 4 07:24:32 UTC 2022 > > Modified Files: > src/sys/arch/amiga/dev: ser.c > src/sys/arch/sgimips/dev: scn.c > > Log Message: > Remove unused extern declaration of constty. thank you. can someone please find all the "extern ;" in .c files, and move them all into .h files. please. .mrg.
Re: CVS commit: src/sys/dev
Hi, Taylor R Campbell writes: >> Date: Tue, 04 Oct 2022 15:53:58 +0900 >> From: Ryo ONODERA >> >> With this patch, it works fine for me. >> There is no stall after genfb(4). >> And I do not find any other problem so far. > > Thanks! There probably is another problem which is that kernel > console printfs might stop appearing after a certain point, which the > following patch might fix too -- I inadvertently reversed the sense > of a conditional in the subr_prf.c changes. I have not encountered another problem yet. However with your consprintfix.patch, it works fine for me too. Thank you. > From 0f058a0e89e3f545a9020a2fb79dadd7ad89029a Mon Sep 17 00:00:00 2001 > From: Taylor R Campbell > Date: Tue, 4 Oct 2022 05:48:39 + > Subject: [PATCH] squash! constty(4): Make MP-safe. > > - Fix reversed sense of conditional. > --- > sys/kern/subr_prf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sys/kern/subr_prf.c b/sys/kern/subr_prf.c > index e87e6efc8501..53fb20c1d393 100644 > --- a/sys/kern/subr_prf.c > +++ b/sys/kern/subr_prf.c > @@ -425,7 +425,7 @@ putone(int c, int flags, struct tty *tp) > if ((flags & TOLOG) && > c != '\0' && c != '\r' && c != 0177) > logputchar(c); > - if ((flags & TOCONS) && ctp != NULL && c != '\0') > + if ((flags & TOCONS) && ctp == NULL && c != '\0') > (*v_putc)(c); > > pserialize_read_exit(s); -- Ryo ONODERA // r...@tetera.org PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
Re: CVS commit: src/sys/dev
> Date: Tue, 04 Oct 2022 15:53:58 +0900 > From: Ryo ONODERA > > With this patch, it works fine for me. > There is no stall after genfb(4). > And I do not find any other problem so far. Thanks! There probably is another problem which is that kernel console printfs might stop appearing after a certain point, which the following patch might fix too -- I inadvertently reversed the sense of a conditional in the subr_prf.c changes. >From 0f058a0e89e3f545a9020a2fb79dadd7ad89029a Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 4 Oct 2022 05:48:39 + Subject: [PATCH] squash! constty(4): Make MP-safe. - Fix reversed sense of conditional. --- sys/kern/subr_prf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sys/kern/subr_prf.c b/sys/kern/subr_prf.c index e87e6efc8501..53fb20c1d393 100644 --- a/sys/kern/subr_prf.c +++ b/sys/kern/subr_prf.c @@ -425,7 +425,7 @@ putone(int c, int flags, struct tty *tp) if ((flags & TOLOG) && c != '\0' && c != '\r' && c != 0177) logputchar(c); - if ((flags & TOCONS) && ctp != NULL && c != '\0') + if ((flags & TOCONS) && ctp == NULL && c != '\0') (*v_putc)(c); pserialize_read_exit(s);
Re: CVS commit: src/sys/dev
Hi,
Taylor R Campbell writes:
>> Date: Tue, 04 Oct 2022 12:12:15 +0900
>> From: Ryo ONODERA
>>
>> "Taylor R Campbell" writes:
>>
>> > console(4), constty(4): Rip off the kernel lock.
>>
>> After introduction of MP-safe console/constty, my kernel stopped
>> just after genfb(4) detection.
>> LOCKDEBUG, DIAGNOSTIC, DEBUG options does not provide any additional
>> information with me.
>> Could you take a look at my problem?
>
> Sorry about that -- I've reverted this change and the MP-safe cons(4)
> change for now, but let's try to figure out what's wrong with them so
> I can reapply them and get the console paths out of the kernel lock
> for good.
No problem. And thanks for your quick response.
> Can you try the attached patch on top?
With this patch, it works fine for me.
There is no stall after genfb(4).
And I do not find any other problem so far.
$ cd /usr/src
$ TZ=UTC cvs up -dP -D2022-10-04
$ patch -p1 < ~/consokfix.patch
$ ./build.sh ... kernel=...
Thank you very much!!!
> From 2de03f1efbe5b73d42dc2f59730c17b99c04b3b9 Mon Sep 17 00:00:00 2001
> From: Taylor R Campbell
> Date: Tue, 4 Oct 2022 05:24:49 +
> Subject: [PATCH] squash! constty(4): Make MP-safe.
>
> - Fix initialization of ok in cn_redirect.
> ---
> sys/dev/cons.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/sys/dev/cons.c b/sys/dev/cons.c
> index f4f9a1602221..e621292a6b4a 100644
> --- a/sys/dev/cons.c
> +++ b/sys/dev/cons.c
> @@ -463,7 +463,7 @@ cn_redirect(dev_t *devp, int is_read, int *error, struct
> tty **ctpp)
> dev_t dev = *devp;
> struct tty *ctp;
> int s;
> - bool ok;
> + bool ok = false;
>
> *error = ENXIO;
> *ctpp = NULL;
> @@ -472,18 +472,17 @@ cn_redirect(dev_t *devp, int is_read, int *error,
> struct tty **ctpp)
> (cn_tab == NULL || (cn_tab->cn_pri != CN_REMOTE))) {
> if (is_read) {
> *error = 0;
> - ok = false;
> goto out;
> }
> tty_acquire(ctp);
> *ctpp = ctp;
> dev = ctp->t_dev;
> } else if (cn_tab == NULL) {
> - ok = false;
> goto out;
> } else {
> dev = cn_tab->cn_dev;
> }
> + ok = true;
> *devp = dev;
> out: pserialize_read_exit(s);
> return ok;
--
Ryo ONODERA // r...@tetera.org
PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
Re: CVS commit: src/sys/dev
> Date: Tue, 04 Oct 2022 12:12:15 +0900
> From: Ryo ONODERA
>
> "Taylor R Campbell" writes:
>
> > console(4), constty(4): Rip off the kernel lock.
>
> After introduction of MP-safe console/constty, my kernel stopped
> just after genfb(4) detection.
> LOCKDEBUG, DIAGNOSTIC, DEBUG options does not provide any additional
> information with me.
> Could you take a look at my problem?
Sorry about that -- I've reverted this change and the MP-safe cons(4)
change for now, but let's try to figure out what's wrong with them so
I can reapply them and get the console paths out of the kernel lock
for good.
Can you try the attached patch on top?
>From 2de03f1efbe5b73d42dc2f59730c17b99c04b3b9 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell
Date: Tue, 4 Oct 2022 05:24:49 +
Subject: [PATCH] squash! constty(4): Make MP-safe.
- Fix initialization of ok in cn_redirect.
---
sys/dev/cons.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/sys/dev/cons.c b/sys/dev/cons.c
index f4f9a1602221..e621292a6b4a 100644
--- a/sys/dev/cons.c
+++ b/sys/dev/cons.c
@@ -463,7 +463,7 @@ cn_redirect(dev_t *devp, int is_read, int *error, struct
tty **ctpp)
dev_t dev = *devp;
struct tty *ctp;
int s;
- bool ok;
+ bool ok = false;
*error = ENXIO;
*ctpp = NULL;
@@ -472,18 +472,17 @@ cn_redirect(dev_t *devp, int is_read, int *error, struct
tty **ctpp)
(cn_tab == NULL || (cn_tab->cn_pri != CN_REMOTE))) {
if (is_read) {
*error = 0;
- ok = false;
goto out;
}
tty_acquire(ctp);
*ctpp = ctp;
dev = ctp->t_dev;
} else if (cn_tab == NULL) {
- ok = false;
goto out;
} else {
dev = cn_tab->cn_dev;
}
+ ok = true;
*devp = dev;
out: pserialize_read_exit(s);
return ok;
Re: CVS commit: src/sys/dev
Hi, "Taylor R Campbell" writes: > Module Name: src > Committed By: riastradh > Date: Mon Oct 3 19:57:25 UTC 2022 > > Modified Files: > src/sys/dev: cons.c > > Log Message: > console(4), constty(4): Rip off the kernel lock. After introduction of MP-safe console/constty, my kernel stopped just after genfb(4) detection. LOCKDEBUG, DIAGNOSTIC, DEBUG options does not provide any additional information with me. Could you take a look at my problem? My dmesg just before MP-safe console.constty is here: Copyright (c) 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020, 2021, 2022 The NetBSD Foundation, Inc. All rights reserved. Copyright (c) 1982, 1986, 1989, 1991, 1993 The Regents of the University of California. All rights reserved. NetBSD 9.99.100 (DTRACE8) #16: Tue Oct 4 09:36:04 JST 2022 ryoon@brownie:/usr/world/9.99/amd64/obj/sys/arch/amd64/compile/DTRACE8 total memory = 15680 MB avail memory = 15137 MB timecounter: Timecounters tick every 10.000 msec Kernelized RAIDframe activated pms* disabled timecounter: Timecounter "i8254" frequency 1193182 Hz quality 100 efi: systbl at pa c9f7e018 mainbus0 (root) ACPI: RSDP 0xCDFFE014 24 (v02 HPQOEM) ACPI: XSDT 0xCDFA4228 000174 (v01 HPQOEM SLIC-MPC 0002 HP 0113) ACPI: FACP 0xCDFE 00010C (v05 HPQOEM SLIC-MPC 0002 HP 0004) ACPI: DSDT 0xCDFD2000 009607 (v01 HPQOEM 8929 0002 HP 0004) ACPI: FACS 0xCDEB3000 40 ACPI: UEFI 0xCDF7E000 000236 (v01 HPQOEM 8929 0001 HP 0004) ACPI: SSDT 0xCDFFC000 00020D (v01 HPQOEM 8929 0001 HP 0004) ACPI: SSDT 0xCDFF4000 0072B0 (v02 HPQOEM 8929 0002 HP 0004) ACPI: IVRS 0xCDFF3000 0001A4 (v02 HPQOEM 8929 0001 HP 0004) ACPI: SSDT 0xCDFEF000 003A21 (v01 HPQOEM 8929 0001 HP 0004) ACPI: SSDT 0xCDFEE000 000472 (v02 HPQOEM 8929 1000 HP 0004) ACPI: TPM2 0xCDFED000 4C (v04 HPQOEM 8929 0002 HP 0004) ACPI: SSDT 0xCDFEC000 00017D (v01 HPQOEM 8929 1000 HP 0004) ACPI: SSDT 0xCDFE4000 007B3B (v01 HPQOEM 8929 1000 HP 0004) ACPI: MSDM 0xCDFE3000 55 (v03 HPQOEM SLIC-MPC 0001 HP 0004) ACPI: ASF! 0xCDFE2000 A5 (v32 HPQOEM 8929 0002 HP 0004) ACPI: BOOT 0xCDFE1000 28 (v01 HPQOEM 8929 0002 HP 0004) ACPI: HPET 0xCDFDF000 38 (v01 HPQOEM 8929 0002 HP 0004) ACPI: APIC 0xCDFDE000 000138 (v03 HPQOEM 8929 0002 HP 0004) ACPI: MCFG 0xCDFDD000 3C (v01 HPQOEM 8929 0002 HP 0004) ACPI: SSDT 0xCDFD1000 C1 (v01 HPQOEM 8929 0002 HP 0004) ACPI: SSDT 0xCDFD 80 (v01 HPQOEM 8929 0002 HP 0004) ACPI: VFCT 0xCDFC2000 00D884 (v01 HPQOEM 8929 0001 HP 0004) ACPI: SSDT 0xCDFFD000 F8 (v01 HPQOEM 8929 1000 HP 0004) ACPI: SSDT 0xCDFC 5C (v02 HPQOEM 8929 1000 HP 0004) ACPI: SSDT 0xCDFB9000 005354 (v02 HPQOEM 8929 0001 HP 0004) ACPI: CRAT 0xCDFB8000 000EE8 (v01 HPQOEM 8929 0001 HP 0004) ACPI: CDIT 0xCDFB7000 29 (v01 HPQOEM 8929 0001 HP 0004) ACPI: SSDT 0xCDFB6000 000139 (v01 HPQOEM 8929 0001 HP 0004) ACPI: SSDT 0xCDFB5000 00028D (v01 HPQOEM 8929 0001 HP 0004) ACPI: SSDT 0xCDFB4000 000372 (v01 HPQOEM 8929 0001 HP 0004) ACPI: SSDT 0xCDFB3000 00021F (v01 HPQOEM 8929 0001 HP 0004) ACPI: SSDT 0xCDFB2000 000D53 (v01 HPQOEM 8929 0001 HP 0004) ACPI: SSDT 0xCDFB 0010C5 (v01 HPQOEM 8929 0001 HP 0004) ACPI: SSDT 0xCDFAC000 00362F (v01 HPQOEM 8929 0001 HP 0004) ACPI: FPDT 0xCDFAB000 44 (v01 HPQOEM SLIC-MPC 0002 HP 0004) ACPI: WSMT 0xCDFA9000 28 (v01 HPQOEM 8929 0001 HP 0004) ACPI: SSDT 0xCDFA8000 42 (v01 HPQOEM 8929 0001 HP 0004) ACPI: SSDT 0xCDFA7000 00020A (v01 HPQOEM 8929 0001 HP 0004) ACPI: SSDT 0xCDFA6000 0005AD (v01 HPQOEM 8929 0001 HP 0004) ACPI: SSDT 0xCDFA5000 0002E9 (v01 HPQOEM 8929 0001 HP 0004) ACPI: SSDT 0xCDFC1000 7D (v01 HPQOEM 8929 0001 HP 0004) ACPI: SSDT 0xCDFA3000 C7 (v01 HPQOEM 8929 0001 HP 0004) ACPI: SSDT 0xCDFA2000 0004DB (v01 HPQOEM 8929 0001 HP 0004) ACPI: BGRT 0xCDFDC000 38 (v01 HPQOEM 8929 0001 HP 0004) ACPI: 26 ACPI AML tables successfully acquired and loaded ioapic0 at mainbus0
Re: CVS commit: src/sys/kern
Date:Sat, 1 Oct 2022 13:00:04 -0400
From:Christos Zoulas
Message-ID: <8bd3a408-77fd-402c-8d6c-ad4b4a5e8...@zoulas.com>
| This is what I meant:
|
| RCS file: /cvsroot/src/sys/kern/kern_descrip.c,v
| retrieving revision 1.251
| diff -u -u -r1.251 kern_descrip.c
| --- kern_descrip.c 29 Jun 2021 22:40:53 - 1.251
| +++ kern_descrip.c 1 Oct 2022 16:56:44 -
| @@ -1162,6 +1162,7 @@
| KASSERT(ff->ff_allocated);
| KASSERT(fd_isused(fdp, fd));
| KASSERT(fd >= NDFDFILE || ff == (fdfile_t *)fdp->fd_dfdfile[fd]);
| + ff->ff_exclose = (fp->f_flag & O_CLOEXEC) != 0;
|
| /* No need to lock in order to make file initially visible. */
| ff->ff_file = fp;
That's not going to work in the situation we're in, as nothing will have
set O_CLOEXEC in fp->f_flag (or shouldn't have anyway, right, O_CLOEXEC isn't
that kind of a flag, it belongs to the descriptor, not the file*).
f_flag is set in vfs_syscalls.c in open_setfp() - which is never called
in the cloning device case, that's the underlying problem I believe.
Even when it is called, the code is:
fp->f_flag = flags & FMASK;
where FMASK is (from fcntl.h)
#define FMASK (FREAD|FWRITE|FCNTLFLAGS|FEXEC)
and
#define FCNTLFLAGS (FAPPEND|FASYNC|FFSYNC|FNONBLOCK|FDSYNC|FRSYNC|FALTIO|\
FDIRECT|FNOSIGPIPE)
which all looks exactly as it should be to me - and note that O_CLOEXEC
(which has no F equivalent name - it doesn't need one) is not there.
So, fp->f_flag isn't set at all (is probably 0), and even if it were
O_CLOEXEC would not be there, and should not be.
For the vnode actually being opened, none of this matters, as the open
lasts as long (actually not as long) as the open() sys call - when that
returns, the device being opened has been closed again, so what the
file that refers to it looks like (or would have looked like) really
doesn't matter at all, it never becomes visible. That's my guess as
to why the open_setfp() call is missing in that case.
But what got forgotten (or deliberately was not done) was anything to
affect the modes of the clone which is opened.
| What does it mean when the open specifies O_CLOEXEC
| and ff->ff_exclose is false? Can that happen? Is that desirable?
It is what is currently happening whenever we open a cloning device.
(Or that is what it looks like to me). Desirable, no idea, I didn't
define the semantics of cloning device opens, nor which of the open
flags should apply to the clone that is created.
| I am fine with the locking to stay where it is. I guess it is probably
| not needed after dup/clone, since the underlying vnode is shared...
Assuming you mean dup(2) (and dup2()) and clone(2) then no - those have
no way to pass the relevant locking flags, if you have a fd and want to
apply a lock, fcntl() is what does that (and the fcntl operations that
duplicate fds do not also apply locks).
The only issue is O_EXLOCK and O_SHLOCK (and O_CLOEXEC) for cloned devices.
I suspect it makes sense for O_CLOEXEC to be applied in that case, it
makes little sense for open("/dev/ptmx", O_RDWR|O_CLOEXEC) to succeed,
returning an open fd which does not have cloexec set (which is the issue,
along with O_NONBLOCK) which started all of this discussion.
The locking flags I am less sure about. I don't see how they can fail
to succeed if applied, as the vnode for the device has just been created,
nothing else can possibly have any kind of lock on it. Whether there's
any benefit in applying the lock so that the node is locked for later,
I don't know - but it certainly should do no harm to do that.
It seems clear to me that what we need is (something like)
Index: vfs_syscalls.c
===
RCS file: /cvsroot/src/sys/kern/vfs_syscalls.c,v
retrieving revision 1.555
diff -u -r1.555 vfs_syscalls.c
--- vfs_syscalls.c 12 Feb 2022 15:51:29 - 1.555
+++ vfs_syscalls.c 1 Oct 2022 19:27:15 -
@@ -1763,6 +1763,9 @@
error = fd_dupopen(dupfd, dupfd_move, flags, );
if (error)
return error;
+ error = open_setfp(l, fp, XXXvp, indx, flags);
+ if (error)
+ return error;
*fd = indx;
} else {
error = open_setfp(l, fp, vp, indx, flags);
where XXXvp needs to be extracted from somewhere (it isn't vp, as vp==NULL)
except that what follows in the else case is ...
if (error)
return error;
VOP_UNLOCK(vp);
*fd = indx;
That VOP_UNLOCK(vp) is what is bothering me, It tells me that open_setfp()
is expecting to be called with vp locked - but in the first case (the cloning
case) there is no VOP_UNLOCK() call (and what's more, fd_dupopen() cannot
do it, as it has no vp arg). That means, I
Re: CVS commit: src/sys/kern
> On Sep 30, 2022, at 11:02 PM, Robert Elz wrote: > >Date:Fri, 30 Sep 2022 20:15:07 -0400 >From:Christos Zoulas >Message-ID: > > | It does not need an extra flag (it looks in the file descriptor flags to > | find if it needs to set or not. > > One of us is confused. From where in this case does anything > get the exclose flag set? That's the whole question here. The > flags arg that is passed around has O_CLOEXEC set in it - you used > that in the call to fd_set_exclose() in kern/tty_ptm.c ... but where > you said that would be better done in fd_affix(). This is what I meant: RCS file: /cvsroot/src/sys/kern/kern_descrip.c,v retrieving revision 1.251 diff -u -u -r1.251 kern_descrip.c --- kern_descrip.c 29 Jun 2021 22:40:53 - 1.251 +++ kern_descrip.c 1 Oct 2022 16:56:44 - @@ -1162,6 +1162,7 @@ KASSERT(ff->ff_allocated); KASSERT(fd_isused(fdp, fd)); KASSERT(fd >= NDFDFILE || ff == (fdfile_t *)fdp->fd_dfdfile[fd]); + ff->ff_exclose = (fp->f_flag & O_CLOEXEC) != 0; /* No need to lock in order to make file initially visible. */ ff->ff_file = fp; > > That does not have access to the flags. So from where is it going > to get the close on exec info ? > > My reading of do_open() is that the O_CLOEXEC flag is never even > examined when a cloning device is opened, it doesn't get set on > the original fd (the cloner) or the cloned device (other than by > your recent modification for /dev/pmx). > > Did I misread the code? > > Or are you planning something different than it seemed? > > | to find other cases where we forgot to call fd_set_exclose() before calling > | fd_affix(). > > My point is that it should not be necessary to call fd_set_exclose() > in every (or any) cloning device driver. The open syscall handling > is where that should be done, just as it is for all the opens that > are not cloning devices. What does it mean when the open specifies O_CLOEXEC and ff->ff_exclose is false? Can that happen? Is that desirable? > Why be different? > > | It also does not need locking because the process can't access > | the descriptor before calling fd_affix. > > The locking I was referring to are the vnode locks/references in > do_open(), not anything related to the file struct or descriptor. > I just do not feel competent to get all of that correct in this > case (more complex than the normal case because of the extra vnode > involved) and would prefer if someone familiar with all of that > were to handle it - particularly in the extra error case that will > need to be handled, even if I cannot see how it would actually fire > in the case in question. I am fine with the locking to stay where it is. I guess it is probably not needed after dup/clone, since the underlying vnode is shared... christos signature.asc Description: Message signed with OpenPGP
Re: CVS commit: src/sys/kern
Date:Fri, 30 Sep 2022 20:15:07 -0400 From:Christos Zoulas Message-ID: | It does not need an extra flag (it looks in the file descriptor flags to | find if it needs to set or not. One of us is confused. From where in this case does anything get the exclose flag set? That's the whole question here. The flags arg that is passed around has O_CLOEXEC set in it - you used that in the call to fd_set_exclose() in kern/tty_ptm.c ... but where you said that would be better done in fd_affix(). That does not have access to the flags. So from where is it going to get the close on exec info ? My reading of do_open() is that the O_CLOEXEC flag is never even examined when a cloning device is opened, it doesn't get set on the original fd (the cloner) or the cloned device (other than by your recent modification for /dev/pmx). Did I misread the code? Or are you planning something different than it seemed? | to find other cases where we forgot to call fd_set_exclose() before calling | fd_affix(). My point is that it should not be necessary to call fd_set_exclose() in every (or any) cloning device driver. The open syscall handling is where that should be done, just as it is for all the opens that are not cloning devices. Why be different? | It also does not need locking because the process can't access | the descriptor before calling fd_affix. The locking I was referring to are the vnode locks/references in do_open(), not anything related to the file struct or descriptor. I just do not feel competent to get all of that correct in this case (more complex than the normal case because of the extra vnode involved) and would prefer if someone familiar with all of that were to handle it - particularly in the extra error case that will need to be handled, even if I cannot see how it would actually fire in the case in question. kre
Re: CVS commit: src/sys/kern
> On Sep 30, 2022, at 5:57 PM, Robert Elz wrote: > >Date:Fri, 30 Sep 2022 16:34:20 -0400 >From:Christos Zoulas >Message-ID: <232331ad-d501-4547-b730-03590c0c9...@zoulas.com> > > | How about handling exclose there? > > That would be possible, but why? We still need higher level code to > handle the locking, which can also handle cloexec -- the problem we > have now is simply that the relevant call is missing, I don't think adding > it will be hard, but it needs to be done by someone who understands the > locking requirements, and correct exit strategy in this case if an error > occurs (failing to successfully lock a newly created clone would seem to > be a very bizarre case, but still...) That is, I don't feel competent to > suggest the 3 or 4 lines that ought be added in do_open to fix this (for > just O_CLOEXEC it would be trivial there, as that cannot fail). > > Currently fd_affix (I mistakenly made it fp_affix in the last message...) > doesn't have a flags parameter, so to do it the way you suggest, we'd need > to alter its signature, bump to 9.99.101 (and I haven't yet gotten around > to making my kernel be 98.99.100 which I'm kind of planning to do ...) > and go alter all the calls everywhere, mostly just filling in an extra > arg with a 0. It does not need an extra flag (it looks in the file descriptor flags to find if it needs to set or not. In fact the first thing I thought was to add an assertion to make sure that the flags agrees with that is set in exclose, to find other cases where we forgot to call fd_set_exclose() before calling fd_affix(). It also does not need locking because the process can't access the descriptor before calling fd_affix. christos signature.asc Description: Message signed with OpenPGP
Re: CVS commit: src/sys/kern
On Sat, 1 Oct 2022, Robert Elz wrote: Currently fd_affix (I mistakenly made it fp_affix in the last message...) doesn't have a flags parameter, so to do it the way you suggest, we'd need to alter its signature, bump to 9.99.101 ... and add some COMPAT_09 goop for backward compability :) ... (and I haven't yet gotten around to making my kernel be 98.99.100 which I'm kind of planning to do ...) and go alter all the calls everywhere, mostly just filling in an extra arg with a 0. kre !DSPAM:63376773211686829812153! ++--+--+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses:| | (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com| | Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org | | & Network Engineer | | pgoyett...@gmail.com | ++--+--+
Re: CVS commit: src/sys/kern
Date:Fri, 30 Sep 2022 16:34:20 -0400 From:Christos Zoulas Message-ID: <232331ad-d501-4547-b730-03590c0c9...@zoulas.com> | How about handling exclose there? That would be possible, but why? We still need higher level code to handle the locking, which can also handle cloexec -- the problem we have now is simply that the relevant call is missing, I don't think adding it will be hard, but it needs to be done by someone who understands the locking requirements, and correct exit strategy in this case if an error occurs (failing to successfully lock a newly created clone would seem to be a very bizarre case, but still...) That is, I don't feel competent to suggest the 3 or 4 lines that ought be added in do_open to fix this (for just O_CLOEXEC it would be trivial there, as that cannot fail). Currently fd_affix (I mistakenly made it fp_affix in the last message...) doesn't have a flags parameter, so to do it the way you suggest, we'd need to alter its signature, bump to 9.99.101 (and I haven't yet gotten around to making my kernel be 98.99.100 which I'm kind of planning to do ...) and go alter all the calls everywhere, mostly just filling in an extra arg with a 0. kre
Re: CVS commit: src/sys/kern
> On Sep 30, 2022, at 10:13 AM, Robert Elz wrote: > >Date:Thu, 29 Sep 2022 16:47:06 - (UTC) >From:chris...@astron.com (Christos Zoulas) >Message-ID: > > | I think that the way to go is to: > | > | 1. Do the fd_set_exclose() in fd_affix(). That will remove most of the > calls > |to fd_set_exclose() *and* the open-coded versions of it. > | 2. Move the open_setfp locking initialization code to fd_affix() and do it > |if fp->f_type == DTYPE_VNODE. This should enable locking in all the > |appropriate cloners. > > I initially intended to reply and say that decisions where to put stuff > like that were for someone else (you, dholland, ...) rather than me, as > I haven't played around much at this level since before vnodes existed. > > But I have been thinking about it, and I disagree with that approach. > > fp_affix() has a job to do, and should be left to do it, without being > burdened by applying weird side effects, sometimes. The "do one thing > and do it well" philosophy applies to more than the commands. > > eg: currently fd_affix() is a void func, but to handle the lock flags > it would need to be able to fail, and return an error code. It would > also need to be able to sleep. That's just wrong. > > O_CLOEXEC and O_??LOCK are high level open() flags, and deserve to be > handled somewhere near the upper levels of the open syscall handling, > not buried in some utility function. That is the feedback that I wanted. But there were two parts to it. How about handling exclose there? It is just making sure that the value from flags is propagated to the exclose field. christos signature.asc Description: Message signed with OpenPGP
Re: CVS commit: src/sys/kern
Date:Thu, 29 Sep 2022 16:47:06 - (UTC) From:chris...@astron.com (Christos Zoulas) Message-ID: | I think that the way to go is to: | | 1. Do the fd_set_exclose() in fd_affix(). That will remove most of the calls |to fd_set_exclose() *and* the open-coded versions of it. | 2. Move the open_setfp locking initialization code to fd_affix() and do it |if fp->f_type == DTYPE_VNODE. This should enable locking in all the |appropriate cloners. I initially intended to reply and say that decisions where to put stuff like that were for someone else (you, dholland, ...) rather than me, as I haven't played around much at this level since before vnodes existed. But I have been thinking about it, and I disagree with that approach. fp_affix() has a job to do, and should be left to do it, without being burdened by applying weird side effects, sometimes. The "do one thing and do it well" philosophy applies to more than the commands. eg: currently fd_affix() is a void func, but to handle the lock flags it would need to be able to fail, and return an error code. It would also need to be able to sleep. That's just wrong. O_CLOEXEC and O_??LOCK are high level open() flags, and deserve to be handled somewhere near the upper levels of the open syscall handling, not buried in some utility function. kre
Re: CVS commit: src/sys/kern
In article <9275.1664462...@jacaranda.noi.kre.to>, Robert Elz wrote: >Date:Thu, 29 Sep 2022 08:18:28 -0400 >From:"Christos Zoulas" >Message-ID: <20220929121828.06edff...@cvs.netbsd.org> > > | Log Message: > | Add fd_set_exclose(). It is probably better to do this automatically in > | fd_affix()... > >Since that only affects /dev/ptmx I'd suggest fixing it generally for all >cloning devices (and handling O_??LOCK as well) would be a better method. I think that the way to go is to: 1. Do the fd_set_exclose() in fd_affix(). That will remove most of the calls to fd_set_exclose() *and* the open-coded versions of it. 2. Move the open_setfp locking initialization code to fd_affix() and do it if fp->f_type == DTYPE_VNODE. This should enable locking in all the appropriate cloners. Best, christos
Re: CVS commit: src/sys/kern
Date:Thu, 29 Sep 2022 08:18:28 -0400 From:"Christos Zoulas" Message-ID: <20220929121828.06edff...@cvs.netbsd.org> | Log Message: | Add fd_set_exclose(). It is probably better to do this automatically in | fd_affix()... Since that only affects /dev/ptmx I'd suggest fixing it generally for all cloning devices (and handling O_??LOCK as well) would be a better method. kre
Re: CVS commit: src/sys/arch/arm/ti
> Module Name:src > Committed By: riastradh > Date: Sun Sep 25 07:50:32 UTC 2022 > > Modified Files: > src/sys/arch/arm/ti: ti_fb.c ti_lcdc.c > > Log Message: > tilcdc(4): Set is_console on the drm device, not the fb child. > > The drm device is represented by a rockchip,display-subsystem node in > the device tree. The fb child is a purely software abstraction used > by drm. This was supposed to read: The drm device is represented by a ti,am33xx-tilcdc node in the device tree. The fb child is a purely software abstraction used by drm.
Re: CVS commit: src/sys/arch/arm/sunxi
> Module Name:src > Committed By: riastradh > Date: Sun Sep 25 07:50:23 UTC 2022 > > Modified Files: > src/sys/arch/arm/sunxi: sunxi_drm.c sunxi_fb.c > > Log Message: > sunxidrm: Set is_console on the drm device, not the fb child. > > The drm device is represented by a rockchip,display-subsystem node in > the device tree. The fb child is a purely software abstraction used > by drm. This was supposed to read: The drm device is represented by an allwinner,sun*i-*-display-engine node in the device tree. The fb child is a purely software abstraction used by drm.
Re: CVS commit: src/sys/dev/pci
Hi,
On 21/09/2022 08:56, matthew green wrote:
this asserts for me. perhaps ryo@ didn't have LOCKDEBUG?
I had LOCKDEBUG on, but not DEBUG.
https://nxr.netbsd.org/xref/src/sys/sys/systm.h#760
I see that adding options DEBUG does indeed cause a panic with
ASSERT_SLEEPABLE()...
ah! that would explain it. nick asked me to test switching
sc_mutex to IPL_SOFTCLOCK mutex, and this works, though it
exposed another bug in reboot that i also needed a fix for
(ifnet locked), see patch below with both fixes.
there's another with the rev 1.32 and rev 1.33+patch driver
when doing "ifconfig aq0 down up", i get this shortly after
and packets no longer flow:
aq0: watchdog timeout -- resetting
aq0: aq_handle_reset_work: INTR_MASK/STATUS = 0001/
aq0: aq_handle_reset_work: TXring[0] HEAD/TAIL=26/51
aq0: aq_handle_reset_work: TXring[1] HEAD/TAIL=153/170
aq0: aq_handle_reset_work: TXring[2] HEAD/TAIL=0/0
aq0: aq_handle_reset_work: TXring[3] HEAD/TAIL=0/0
aq0: aq_handle_reset_work: TXring[4] HEAD/TAIL=0/0
aq0: aq_handle_reset_work: TXring[5] HEAD/TAIL=0/0
aq0: aq_handle_reset_work: TXring[6] HEAD/TAIL=0/0
aq0: aq_handle_reset_work: TXring[7] HEAD/TAIL=0/0
I think this diff is slightly better and might even fix the problem
you're seeing with "ifconfig aq0 down up"
Nick
Index: sys/dev/pci/if_aq.c
===
RCS file: /cvsroot/src/sys/dev/pci/if_aq.c,v
retrieving revision 1.33
diff -u -p -r1.33 if_aq.c
--- sys/dev/pci/if_aq.c 16 Sep 2022 03:55:53 - 1.33
+++ sys/dev/pci/if_aq.c 21 Sep 2022 08:15:26 -
@@ -1278,7 +1278,7 @@ aq_attach(device_t parent, device_t self
int error;
sc->sc_dev = self;
- mutex_init(>sc_mutex, MUTEX_DEFAULT, IPL_NET);
+ mutex_init(>sc_mutex, MUTEX_DEFAULT, IPL_SOFTCLOCK);
mutex_init(>sc_mpi_mutex, MUTEX_DEFAULT, IPL_NET);
sc->sc_pc = pc = pa->pa_pc;
@@ -1588,7 +1588,9 @@ aq_detach(device_t self, int flags __unu
if (sc->sc_iosize != 0) {
if (ifp->if_softc != NULL) {
- aq_stop(ifp, 0);
+ IFNET_LOCK(ifp);
+ aq_stop(ifp, 1);
+ IFNET_UNLOCK(ifp);
}
for (i = 0; i < AQ_NINTR_MAX; i++) {
@@ -1616,8 +1618,6 @@ aq_detach(device_t self, int flags __unu
sc->sc_iosize = 0;
}
- callout_stop(>sc_tick_ch);
-
#if NSYSMON_ENVSYS > 0
if (sc->sc_sme != NULL) {
/* all sensors associated with this will also be detached */
re: CVS commit: src/sys/dev/pci
> >this asserts for me. perhaps ryo@ didn't have LOCKDEBUG?
>
> I had LOCKDEBUG on, but not DEBUG.
> https://nxr.netbsd.org/xref/src/sys/sys/systm.h#760
> I see that adding options DEBUG does indeed cause a panic with
> ASSERT_SLEEPABLE()...
ah! that would explain it. nick asked me to test switching
sc_mutex to IPL_SOFTCLOCK mutex, and this works, though it
exposed another bug in reboot that i also needed a fix for
(ifnet locked), see patch below with both fixes.
there's another with the rev 1.32 and rev 1.33+patch driver
when doing "ifconfig aq0 down up", i get this shortly after
and packets no longer flow:
aq0: watchdog timeout -- resetting
aq0: aq_handle_reset_work: INTR_MASK/STATUS = 0001/
aq0: aq_handle_reset_work: TXring[0] HEAD/TAIL=26/51
aq0: aq_handle_reset_work: TXring[1] HEAD/TAIL=153/170
aq0: aq_handle_reset_work: TXring[2] HEAD/TAIL=0/0
aq0: aq_handle_reset_work: TXring[3] HEAD/TAIL=0/0
aq0: aq_handle_reset_work: TXring[4] HEAD/TAIL=0/0
aq0: aq_handle_reset_work: TXring[5] HEAD/TAIL=0/0
aq0: aq_handle_reset_work: TXring[6] HEAD/TAIL=0/0
aq0: aq_handle_reset_work: TXring[7] HEAD/TAIL=0/0
.mrg.
Index: if_aq.c
===
RCS file: /cvsroot/src/sys/dev/pci/if_aq.c,v
retrieving revision 1.33
diff -p -u -r1.33 if_aq.c
--- if_aq.c 16 Sep 2022 03:55:53 - 1.33
+++ if_aq.c 21 Sep 2022 07:52:59 -
@@ -1278,7 +1278,7 @@ aq_attach(device_t parent, device_t self
int error;
sc->sc_dev = self;
- mutex_init(>sc_mutex, MUTEX_DEFAULT, IPL_NET);
+ mutex_init(>sc_mutex, MUTEX_DEFAULT, IPL_SOFTCLOCK);
mutex_init(>sc_mpi_mutex, MUTEX_DEFAULT, IPL_NET);
sc->sc_pc = pc = pa->pa_pc;
@@ -1588,7 +1588,9 @@ aq_detach(device_t self, int flags __unu
if (sc->sc_iosize != 0) {
if (ifp->if_softc != NULL) {
+ IFNET_LOCK(ifp);
aq_stop(ifp, 0);
+ IFNET_UNLOCK(ifp);
}
for (i = 0; i < AQ_NINTR_MAX; i++) {
Re: CVS commit: src/sys/dev/pci
>> Module Name: src >> Committed By:skrll >> Date:Fri Sep 16 03:55:53 UTC 2022 >> >> Modified Files: >> src/sys/dev/pci: if_aq.c >> >> Log Message: >> Some MP improvements >> >> - Remove use of IFF_OACTIVE >> >> - Remove use of if_timer and provide an MP safe multiqueue watchdog >> >> - Sprinkle some lock assertions. >> >> Tested by ryo@. Thanks. > >this asserts for me. perhaps ryo@ didn't have LOCKDEBUG? I had LOCKDEBUG on, but not DEBUG. https://nxr.netbsd.org/xref/src/sys/sys/systm.h#760 I see that adding options DEBUG does indeed cause a panic with ASSERT_SLEEPABLE()... -- ryo shimizu
re: CVS commit: src/sys/dev/pci
"Nick Hudson" writes: > Module Name: src > Committed By: skrll > Date: Fri Sep 16 03:55:53 UTC 2022 > > Modified Files: > src/sys/dev/pci: if_aq.c > > Log Message: > Some MP improvements > > - Remove use of IFF_OACTIVE > > - Remove use of if_timer and provide an MP safe multiqueue watchdog > > - Sprinkle some lock assertions. > > Tested by ryo@. Thanks. this asserts for me. perhaps ryo@ didn't have LOCKDEBUG? the problem is that aq_init() calls AQ_LOCK(sc) -- this is a spin mutex -- and then calls aq_init_locked(). however, aq_init_locked() calls ASSERT_SLEEPABLE() since it has a code path that calls callout_halt() (which wants to sleep.) ie, the function that expects to be called with a spin mutex held also calls ASSERT_SLEEPABLE(). even if i were to comment that call, the later call to callout_halt() is the real problem. the only way i saw to handle this without investing some other method to invoke the callout_halt() from another lwp was to change aq_stop_locked() to return a value that says that callout_halt is needed here. that needs to be passed upto aq_stop() as well as aq_init(), both of which call aq_stop_locked(). it needs a little re-arrange due to aq_init_locked() already returning a value for aq_init() to return directly (and aq_init() is where the mutex will be dropped, and it's safe to callout_halt().) for now i'm running with rev 1.32. thanks. .mrg.
Re: CVS commit: src/sys/dev/nvmm
Hi Taylor, Thanks for updating NVMM! Would it be a good idea to sync our code with DragonBSDs? AFAICS they kept the code compiling for NetBSD too. To have a common code base? With regards, Reinoud On Tue, Sep 13, 2022 at 08:10:04PM +, Taylor R Campbell wrote: > Module Name: src > Committed By: riastradh > Date: Tue Sep 13 20:10:04 UTC 2022 > > Modified Files: > src/sys/dev/nvmm: nvmm.c nvmm_internal.h > src/sys/dev/nvmm/x86: nvmm_x86_vmx.c > > Log Message: > nvmm(4): Add suspend/resume support. > > New MD nvmm_impl callbacks: > > - .suspend_interrupt forces all VMs on all physical CPUs to exit. > - .vcpu_suspend suspends an individual vCPU on a machine. > - .machine_suspend suspends an individual machine. > - .suspend suspends the whole system. > - .resume resumes the whole system. > - .machine_resume resumes an individual machine. > - .vcpu_resume resumes an indidivudal vCPU on a machine. > > Suspending nvmm: > > 1. causes new VM operations (ioctl and close) to block until resumed, > 2. uses .suspend_interrupt to interrupt any concurrent and force them >to return early, and then > 3. uses the various suspend callbacks to suspend all vCPUs, machines, >and the whole system -- all vCPUs before the machine they're on, >and all machines before the system. > > Resuming nvmm does the reverse of (3) -- resume system, resume each > machine and then the vCPUs on that machine -- and then unblocks > operations. > > Implemented only for x86-vmx for now: > > - suspend_interrupt triggers a TLB IPI to cause VM exits; > - vcpu_suspend issues VMCLEAR to force any in-CPU state to be written > to memory; > - machine_suspend does nothing; > - suspend does VMXOFF on all CPUs; > - resume does VMXON on all CPUs; > - machine_resume does nothing; and > - vcpu_resume just marks each vCPU as valid but inactive so > subsequent use will clear it and load it with vmptrld. > > x86-svm left as an exercise for the reader. > > > To generate a diff of this commit: > cvs rdiff -u -r1.46 -r1.47 src/sys/dev/nvmm/nvmm.c > cvs rdiff -u -r1.20 -r1.21 src/sys/dev/nvmm/nvmm_internal.h > cvs rdiff -u -r1.84 -r1.85 src/sys/dev/nvmm/x86/nvmm_x86_vmx.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files.
Re: CVS commit: src/sys/fs/union
Thanks for quick fix!
rin
On 2022/09/12 22:10, Christos Zoulas wrote:
Yup!
christos
On Sep 12, 2022, at 5:04 AM, Rin Okuyama wrote:
Hi,
On 2022/09/12 0:42, Christos Zoulas wrote:
@@ -420,11 +423,11 @@ union_statvfs(struct mount *mp, struct s
{
int error;
struct union_mount *um = MOUNTTOUNIONMOUNT(mp);
- struct statvfs *sbuf = malloc(sizeof(*sbuf), M_TEMP, M_WAITOK | M_ZERO);
+ struct statvfs *sbuf = kmem_alloc(sizeof(*sbuf), KM_SLEEP);
unsigned long lbsize;
#ifdef UNION_DIAGNOSTIC
- printf("union_statvfs(mp = %p, lvp = %p, uvp = %p)\n", mp,
+ printf("%s(mp = %p, lvp = %p, uvp = %p)\n", __func__, mp,
um->um_lowervp, um->um_uppervp);
#endif
kmem_zalloc() here?
Thanks,
rin
Re: CVS commit: src/sys/fs/union
Yup!
christos
> On Sep 12, 2022, at 5:04 AM, Rin Okuyama wrote:
>
> Hi,
>
> On 2022/09/12 0:42, Christos Zoulas wrote:
>> @@ -420,11 +423,11 @@ union_statvfs(struct mount *mp, struct s
>> {
>> int error;
>> struct union_mount *um = MOUNTTOUNIONMOUNT(mp);
>> -struct statvfs *sbuf = malloc(sizeof(*sbuf), M_TEMP, M_WAITOK | M_ZERO);
>> +struct statvfs *sbuf = kmem_alloc(sizeof(*sbuf), KM_SLEEP);
>> unsigned long lbsize;
>>#ifdef UNION_DIAGNOSTIC
>> -printf("union_statvfs(mp = %p, lvp = %p, uvp = %p)\n", mp,
>> +printf("%s(mp = %p, lvp = %p, uvp = %p)\n", __func__, mp,
>> um->um_lowervp, um->um_uppervp);
>> #endif
>>
>
> kmem_zalloc() here?
>
> Thanks,
> rin
signature.asc
Description: Message signed with OpenPGP
Re: CVS commit: src/sys/fs/union
Hi,
On 2022/09/12 0:42, Christos Zoulas wrote:
@@ -420,11 +423,11 @@ union_statvfs(struct mount *mp, struct s
{
int error;
struct union_mount *um = MOUNTTOUNIONMOUNT(mp);
- struct statvfs *sbuf = malloc(sizeof(*sbuf), M_TEMP, M_WAITOK | M_ZERO);
+ struct statvfs *sbuf = kmem_alloc(sizeof(*sbuf), KM_SLEEP);
unsigned long lbsize;
#ifdef UNION_DIAGNOSTIC
- printf("union_statvfs(mp = %p, lvp = %p, uvp = %p)\n", mp,
+ printf("%s(mp = %p, lvp = %p, uvp = %p)\n", __func__, mp,
um->um_lowervp, um->um_uppervp);
#endif
kmem_zalloc() here?
Thanks,
rin
Re: CVS commit: src/sys/kern
Reverted and alternative proposed on tech-kern! Sorry for the unilateral toe-stomping.
Re: CVS commit: src/sys/kern
On Sun, 7 Aug 2022, Paul Goyette wrote: On Sun, 7 Aug 2022, Taylor R Campbell wrote: Module Name:src Committed By: riastradh Date: Sun Aug 7 21:17:18 UTC 2022 Modified Files: src/sys/kern: kern_module.c Log Message: module(9): Disable module autounload by default. I don't know why this was ever enabled by default; many modules are still not safe to unload, let alone autounload. If any autounload is to happen by default, it should only be for modules that have opted into it in some way after audit. One reason for the current behavior involves the modules used for various emulations. When a file is executed, and none of the currently-loaded modules can "deal" with it, we load _all_ of the available emulation modules with the hope that one of them will "deal with" the new executable, and with the expectation that the remaining emulation modules will just "go away". Modules that are known to be unsafe to unload should declare that in their modcmd() unload (by returning EBUSY). After all, one might well expect that the module itself is the most likely place that the unloadable status would be known. Making no-autounload as the default seems like using a 20-pound sledge hammer on a carpet tack. I might also note that making such a fundamental behavior change when we're so close to the -10 release, without actually providing the suggested "opt-in" mechanism to retain current behavior, is not very user friendly. :-) At least this should be discussed (on tech-kern@, perhaps) before being unilaterally decided and committed. IIRC, we had this discussion some time ago, and the decision at that time was to retain current behavior. Unfortunately, I didn't save any pointer to that old discussion. :-( ++--+--+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses:| | (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com| | Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org | | & Network Engineer | | pgoyett...@gmail.com | ++--+--+ !DSPAM:62f0321a29502088639869! ++--+--+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses:| | (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com| | Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org | | & Network Engineer | | pgoyett...@gmail.com | ++--+--+
Re: CVS commit: src/sys/kern
On Sun, 7 Aug 2022, Taylor R Campbell wrote: Module Name:src Committed By: riastradh Date: Sun Aug 7 21:17:18 UTC 2022 Modified Files: src/sys/kern: kern_module.c Log Message: module(9): Disable module autounload by default. I don't know why this was ever enabled by default; many modules are still not safe to unload, let alone autounload. If any autounload is to happen by default, it should only be for modules that have opted into it in some way after audit. One reason for the current behavior involves the modules used for various emulations. When a file is executed, and none of the currently-loaded modules can "deal" with it, we load _all_ of the available emulation modules with the hope that one of them will "deal with" the new executable, and with the expectation that the remaining emulation modules will just "go away". Modules that are known to be unsafe to unload should declare that in their modcmd() unload (by returning EBUSY). After all, one might well expect that the module itself is the most likely place that the unloadable status would be known. Making no-autounload as the default seems like using a 20-pound sledge hammer on a carpet tack. ++--+--+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses:| | (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com| | Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org | | & Network Engineer | | pgoyett...@gmail.com | ++--+--+
Re: CVS commit: src/sys/dev/pci
On 03/08/2022 07:26, Kengo NAKAHARA wrote: Hi, On 2022/08/03 14:23, Nick Hudson wrote: Module Name: src Committed By: skrll Date: Wed Aug 3 05:23:30 UTC 2022 Modified Files: src/sys/dev/pci: if_wm.c Log Message: Add some KASSERTs around the locking protocol. Discussed with msaitoh@, knakahara@ and riastradh@ To generate a diff of this commit: cvs rdiff -u -r1.749 -r1.750 src/sys/dev/pci/if_wm.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Will you add "KASSERT(IFNET_LOCKED(ifp))" to all ethernet device driver init routine? If not, why the code is added wm(4) only? Good questions. While I don't see the problem with documenting the locking protocol this way and flushing out bugs with the KASSERTs I don't proposed to (personally) add them to every driver at this time. My motivation here is is that I'm making bge(4) MP safe and using wm(4) as a reference. It's even mentioned as a reference for the if_percpuq framework. https://nxr.netbsd.org/xref/src/sys/net/if.c#814 Perhaps as a driver is made MP safe it can also have similar KASSERTs added? Nick
Re: CVS commit: src/sys/dev/pci
Hi, On 2022/08/03 14:23, Nick Hudson wrote: Module Name:src Committed By: skrll Date: Wed Aug 3 05:23:30 UTC 2022 Modified Files: src/sys/dev/pci: if_wm.c Log Message: Add some KASSERTs around the locking protocol. Discussed with msaitoh@, knakahara@ and riastradh@ To generate a diff of this commit: cvs rdiff -u -r1.749 -r1.750 src/sys/dev/pci/if_wm.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Will you add "KASSERT(IFNET_LOCKED(ifp))" to all ethernet device driver init routine? If not, why the code is added wm(4) only? Thanks, -- // Internet Initiative Japan Inc. Device Engineering Section, Product Division, Technology Unit Kengo NAKAHARA
Re: CVS commit: src/sys/dev/ic
Oops, never mind, I hadn't yet seen the follow-up revert. > On Aug 1, 2022, at 8:29 AM, Jason Thorpe wrote: > > > >> On Aug 1, 2022, at 12:34 AM, Michael van Elst wrote: >> >> Module Name: src >> Committed By: mlelstv >> Date: Mon Aug 1 07:34:28 UTC 2022 >> >> Modified Files: >> src/sys/dev/ic: ahcisata_core.c bcmgenet.c nslm7x.c nvmereg.h nvmevar.h >> rtl8169.c tulip.c tulipreg.h >> >> Log Message: >> Also fix shift values for SCT constants. > > ??? > > diff -u src/sys/dev/ic/tulip.c:1.205 src/sys/dev/ic/tulip.c:1.206 > --- src/sys/dev/ic/tulip.c:1.205Sat Jun 25 02:46:15 2022 > +++ src/sys/dev/ic/tulip.c Mon Aug 1 07:34:28 2022 > @@ -1,4 +1,4 @@ > -/* $NetBSD: tulip.c,v 1.205 2022/06/25 02:46:15 tsutsui Exp $ */ > +/* $NetBSD: tulip.c,v 1.206 2022/08/01 07:34:28 mlelstv Exp $ */ > > /*- > * Copyright (c) 1998, 1999, 2000, 2002 The NetBSD Foundation, Inc. > @@ -36,7 +36,7 @@ > */ > > #include > -__KERNEL_RCSID(0, "$NetBSD: tulip.c,v 1.205 2022/06/25 02:46:15 tsutsui Exp > $"); > +__KERNEL_RCSID(0, "$NetBSD: tulip.c,v 1.206 2022/08/01 07:34:28 mlelstv Exp > $"); > > > #include > @@ -4394,7 +4394,7 @@ > */ > >/* XXX This should be auto-sense. */ > - ifmedia_set(>mii_media, IFM_ETHER | IFM_10_T); > + ifmedia_set(>mii_media, IFM_ETHER | IFM_10_5); > >tlp_print_media(sc); > } > > >> >> >> To generate a diff of this commit: >> cvs rdiff -u -r1.105 -r1.106 src/sys/dev/ic/ahcisata_core.c >> cvs rdiff -u -r1.11 -r1.12 src/sys/dev/ic/bcmgenet.c >> cvs rdiff -u -r1.74 -r1.75 src/sys/dev/ic/nslm7x.c >> cvs rdiff -u -r1.17 -r1.18 src/sys/dev/ic/nvmereg.h >> cvs rdiff -u -r1.24 -r1.25 src/sys/dev/ic/nvmevar.h >> cvs rdiff -u -r1.172 -r1.173 src/sys/dev/ic/rtl8169.c >> cvs rdiff -u -r1.205 -r1.206 src/sys/dev/ic/tulip.c >> cvs rdiff -u -r1.41 -r1.42 src/sys/dev/ic/tulipreg.h >> >> Please note that diffs are not public domain; they are subject to the >> copyright notices on the relevant files. >> > > -- thorpej -- thorpej
Re: CVS commit: src/sys/dev/ic
> On Aug 1, 2022, at 12:34 AM, Michael van Elst wrote: > > Module Name: src > Committed By: mlelstv > Date: Mon Aug 1 07:34:28 UTC 2022 > > Modified Files: > src/sys/dev/ic: ahcisata_core.c bcmgenet.c nslm7x.c nvmereg.h nvmevar.h >rtl8169.c tulip.c tulipreg.h > > Log Message: > Also fix shift values for SCT constants. ??? diff -u src/sys/dev/ic/tulip.c:1.205 src/sys/dev/ic/tulip.c:1.206 --- src/sys/dev/ic/tulip.c:1.205Sat Jun 25 02:46:15 2022 +++ src/sys/dev/ic/tulip.c Mon Aug 1 07:34:28 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: tulip.c,v 1.205 2022/06/25 02:46:15 tsutsui Exp $ */ +/* $NetBSD: tulip.c,v 1.206 2022/08/01 07:34:28 mlelstv Exp $ */ /*- * Copyright (c) 1998, 1999, 2000, 2002 The NetBSD Foundation, Inc. @@ -36,7 +36,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: tulip.c,v 1.205 2022/06/25 02:46:15 tsutsui Exp $"); +__KERNEL_RCSID(0, "$NetBSD: tulip.c,v 1.206 2022/08/01 07:34:28 mlelstv Exp $"); #include @@ -4394,7 +4394,7 @@ */ /* XXX This should be auto-sense. */ - ifmedia_set(>mii_media, IFM_ETHER | IFM_10_T); + ifmedia_set(>mii_media, IFM_ETHER | IFM_10_5); tlp_print_media(sc); } > > > To generate a diff of this commit: > cvs rdiff -u -r1.105 -r1.106 src/sys/dev/ic/ahcisata_core.c > cvs rdiff -u -r1.11 -r1.12 src/sys/dev/ic/bcmgenet.c > cvs rdiff -u -r1.74 -r1.75 src/sys/dev/ic/nslm7x.c > cvs rdiff -u -r1.17 -r1.18 src/sys/dev/ic/nvmereg.h > cvs rdiff -u -r1.24 -r1.25 src/sys/dev/ic/nvmevar.h > cvs rdiff -u -r1.172 -r1.173 src/sys/dev/ic/rtl8169.c > cvs rdiff -u -r1.205 -r1.206 src/sys/dev/ic/tulip.c > cvs rdiff -u -r1.41 -r1.42 src/sys/dev/ic/tulipreg.h > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. > -- thorpej
Re: CVS commit: src/sys/arch/m68k/m68k
At Tue, 26 Jul 2022 09:52:40 -0700, Chuck Silvers wrote: > > This commit breaks usr.sbin/crash on m68k. > > curlwp is defined only in _KERNEL. usr.sbin/crash defines _KMEMUSER > > but not _KERNEL. > > > > Would you look into? > > I fixed it now, sorry about that. Thank you! --- Tetsuya Isaki
Re: CVS commit: src/sys/dev/pci
Hi. On 2022/07/22 23:22, Hisashi T Fujinaka wrote: > On Fri, 22 Jul 2022, SAITOH Masanobu wrote: > >> Module Name: src >> Committed By: msaitoh >> Date: Fri Jul 22 05:23:50 UTC 2022 >> >> Modified Files: >> src/sys/dev/pci: if_wm.c if_wmreg.h >> >> Log Message: >> Add more statistics countes. >> >> - Add many statics counters that the chip has. > > Shouldn't you say which "the chip" you're talking about since wm seems > to handle so many? The current implementation is based on the FreeBSD and Linux. All counters I added to NetBSD are also counted on both FreeBSD and Linux. There are some difference between those two. I also noticed that some code are doubtful and wrote them the different way. To make the implementation perfect, I have to read (almost) all datasheets and test on some chips. It's not good to write only based on the datasheets. One of the reason is that, for example, a register is not listed in the statistics counters' table but the detail of the register is described later in the chapter. It's not clear whether the register is exist or not. The current implementation may not count some useful counters. The current implementation is not the final form but it's worth to have. I could have written the details of the current implementation when I committed, but I didn't really feel the need to write the details of the implementation in its half-way state. My apologies. Thanks. > I suppose this isn't git so you can't fix this easily. > >> - Attach event counters only if available. >> >> >> To generate a diff of this commit: >> cvs rdiff -u -r1.745 -r1.746 src/sys/dev/pci/if_wm.c >> cvs rdiff -u -r1.126 -r1.127 src/sys/dev/pci/if_wmreg.h >> >> Please note that diffs are not public domain; they are subject to the >> copyright notices on the relevant files. >> >> > -- --- SAITOH Masanobu (msai...@execsw.org msai...@netbsd.org)
Re: CVS commit: src/sys/arch/m68k/m68k
On Tue, Jul 26, 2022 at 05:25:01PM +0900, Tetsuya Isaki wrote: > At Mon, 25 Jul 2022 01:59:26 +, > Chuck Silvers wrote: > > Module Name:src > > Committed By: chs > > Date: Mon Jul 25 01:59:26 UTC 2022 > > > > Modified Files: > > src/sys/arch/m68k/m68k: db_trace.c > > > > Log Message: > > use the pcb of the thread we are tracing rather than always curlwp. > > > > > > To generate a diff of this commit: > > cvs rdiff -u -r1.59 -r1.60 src/sys/arch/m68k/m68k/db_trace.c > > This commit breaks usr.sbin/crash on m68k. > curlwp is defined only in _KERNEL. usr.sbin/crash defines _KMEMUSER > but not _KERNEL. > > Would you look into? I fixed it now, sorry about that. -Chuck
Re: CVS commit: src/sys/arch/m68k/m68k
At Mon, 25 Jul 2022 01:59:26 +, Chuck Silvers wrote: > Module Name: src > Committed By: chs > Date: Mon Jul 25 01:59:26 UTC 2022 > > Modified Files: > src/sys/arch/m68k/m68k: db_trace.c > > Log Message: > use the pcb of the thread we are tracing rather than always curlwp. > > > To generate a diff of this commit: > cvs rdiff -u -r1.59 -r1.60 src/sys/arch/m68k/m68k/db_trace.c This commit breaks usr.sbin/crash on m68k. curlwp is defined only in _KERNEL. usr.sbin/crash defines _KMEMUSER but not _KERNEL. Would you look into? Thanks, --- Tetsuya Isaki
re: CVS commit: src/sys/arch/aarch64/include
"Taylor R Campbell" writes: > Module Name: src > Committed By: riastradh > Date: Sun Jul 24 20:28:32 UTC 2022 > > Modified Files: > src/sys/arch/aarch64/include: lock.h > > Log Message: > aarch64/lock.h: Need for _HARDKERNEL. > > Add include guard while here. > > XXX Why does this aarch64-specific file have #ifdef __aarch64__? there're __aarch32__ / __arm32__ environments that supported on arm64 hosts. (clang better than gcc.) .mrg.
Re: CVS commit: src/sys/dev/pci
On Fri, 22 Jul 2022, SAITOH Masanobu wrote: Module Name:src Committed By: msaitoh Date: Fri Jul 22 05:23:50 UTC 2022 Modified Files: src/sys/dev/pci: if_wm.c if_wmreg.h Log Message: Add more statistics countes. - Add many statics counters that the chip has. Shouldn't you say which "the chip" you're talking about since wm seems to handle so many? I suppose this isn't git so you can't fix this easily. - Attach event counters only if available. To generate a diff of this commit: cvs rdiff -u -r1.745 -r1.746 src/sys/dev/pci/if_wm.c cvs rdiff -u -r1.126 -r1.127 src/sys/dev/pci/if_wmreg.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. -- Hisashi T Fujinaka - ht...@twofifty.com BSEE + BSChem + BAEnglish + MSCS + $2.50 = coffee
Re: CVS commit: src/sys/external/bsd/drm2/drm
> On Jul 19, 2022, at 10:14 PM, matthew green wrote: > > looks like only a small number of files check for "alpha" > vs "__alpha__" currently, and all can likely be switched. Yah, and some I should just fix. -- thorpej
re: CVS commit: src/sys/external/bsd/drm2/drm
"Taylor R Campbell" writes:
> Module Name: src
> Committed By: riastradh
> Date: Tue Jul 19 23:19:35 UTC 2022
>
> Modified Files:
> src/sys/external/bsd/drm2/drm: files.drmkms
>
> Log Message:
> drm: Undefine `alpha' in CPPFLAGS. Causes lotsa trouble!
>
> But don't undefine it outside drmkms; `#ifdef alpha' or equivalent is
> used elsewhere in-tree. (Maybe it should be replaced by __alpha__.)
welcome to -D${MACHINE}. this isn't the compiler, but our
build framework we inherited a long long time ago :-)
looks like only a small number of files check for "alpha"
vs "__alpha__" currently, and all can likely be switched.
.mrg.
re: CVS commit: src/sys/dev/pci
On Sat, 9 Jul 2022, matthew green wrote:
"Paul Goyette" writes:
Module Name:src
Committed By: pgoyette
Date: Fri Jul 8 17:32:19 UTC 2022
Modified Files:
src/sys/dev/pci: nvme_pci.c
Log Message:
devsw_ok needs to survive across invocations of nvme_modcmd() so
allocate it statically.
Should address remaining issues with kern/56914
if (error) {
+ devsw_ok = false;
shouldn't devsw_ok be "false" here already? seems more like
something to ASSERT() than assign.
Yeah, this is likely unnecessary now. It got there during a
debug iteration.
I will remove.
++--+--+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses:|
| (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com|
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org |
| & Network Engineer | | pgoyett...@gmail.com |
++--+--+
re: CVS commit: src/sys/dev/pci
"Paul Goyette" writes:
> Module Name: src
> Committed By: pgoyette
> Date: Fri Jul 8 17:32:19 UTC 2022
>
> Modified Files:
> src/sys/dev/pci: nvme_pci.c
>
> Log Message:
> devsw_ok needs to survive across invocations of nvme_modcmd() so
> allocate it statically.
>
> Should address remaining issues with kern/56914
if (error) {
+ devsw_ok = false;
shouldn't devsw_ok be "false" here already? seems more like
something to ASSERT() than assign.
.mrg.
Re: CVS commit: src/sys/arch/atari/dev
> Modified Files: > src/sys/arch/atari/dev: kbd.c > > Log Message: > gcc is not smart enough to track the equivalence of conditions used > here and warns about an unused value - initialize "code" always. Umm, complains only with -Os ... Anyway thanks for fixing. --- Izumi Tsutsui
Re: CVS commit: src/sys/arch/x68k/dev
I wrote: > Module Name: src > Committed By: tsutsui > Date: Sat Jun 25 03:18:38 UTC 2022 > > Modified Files: > src/sys/arch/x68k/dev: ite.c ite_tv.c itevar.h > > Log Message: > Add a minimum box drawing character support for x68k ite(4). Ah, I committed a wrong branch so these also include other misc cleanup that should be in a separate commit: - make local functions static - use C99 designated initializer for readability - use proper integer types - avoid reusing temporary variables for other purpose - misc KNF There is no functional change in them so I'll leave themas is (sorry for noise). --- Izumi Tsutsui
Re: CVS commit: src/sys/uvm
On 2022/06/09 1:55, Michael Lorenz wrote: Module Name:src Committed By: macallan Date: Wed Jun 8 16:55:00 UTC 2022 Modified Files: src/sys/uvm: uvm_map.c Log Message: initialize a variable to appease clang My bug! Thanks for fixing it. rin
Re: CVS commit: src/sys/dev/usb
Hi, On 2022/05/14 19:44, Taylor R Campbell wrote: > Module Name: src > Committed By: riastradh > Date: Sat May 14 19:44:37 UTC 2022 > > Modified Files: > src/sys/dev/usb: xhci.c > > Log Message: > xhci(4): Handle race between software abort and hardware stall. xhci_abortx is expected to stop given single xfer, but it actually stops all xfers in pipe. When usbd_ar_pipe stops the first xfer in up_queue of isoc pipe such as uvideo(4), HCI generates multiple Transfer Events (UVIDEO_NXFERS (3) for uvideo) in order xfers are posted. ux_status of first xfer is set to USBD_CANCELLED by usbd_xfer_abort, so usbd_xfer_trycomplete in xhci_event_transfer fails and usb_transfer_complete is not called (xhci_abortx does it instead). However, other two xfers has ux_status = USBD_IN_PROGRESS, depending on how quick events are generated, xhci_event_transfer may call usb_transfer_complete for them before xhci_abortx calls usb_transfer_complete. It may fire KASSERT failure "not start of queue."
Re: CVS commit: src/sys
Should be fixed now. On Sat, 4 Jun 2022, J. Hannken-Illjes wrote: On 4. Jun 2022, at 05:31, Paul Goyette wrote: Module Name:src Committed By: pgoyette Date: Sat Jun 4 03:31:10 UTC 2022 Modified Files: src/sys/dev: files.audio files.dev midi.c sequencer.c src/sys/modules: Makefile src/sys/modules/midi: Makefile src/sys/modules/sequencer: Makefile Added Files: src/sys/dev: midi_mod.c midi_seq_mod.c sequencer_mod.c src/sys/modules/midi_seq: Makefile midi_seq.ioconf Removed Files: src/sys/modules/midi: midi.ioconf src/sys/modules/sequencer: sequencer.ioconf Log Message: Combine the midi and sequencer modules into a single midi_seq module to avoid a circular dependency as noted in kern/56772. Retain minimal modules of the original names to accomodate auto-loading upon access to the /dev/xxx nodes. This breaks at least sparc64/GENERIC: sys/dev/sequencer.c:285:1: error: no previous prototype for 'sequencerattach' [-Werror=missing-prototypes] 285 | sequencerattach(int n) | ^~~ It has "midi* at midibus?" but no "pseudo-device sequencer". -- J. Hannken-Illjes - hann...@mailbox.org ++--+--+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses:| | (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com| | Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org | | & Network Engineer | | pgoyett...@gmail.com | ++--+--+
Re: CVS commit: src/sys
yeah, working on it. On Sat, 4 Jun 2022, J. Hannken-Illjes wrote: On 4. Jun 2022, at 05:31, Paul Goyette wrote: Module Name:src Committed By: pgoyette Date: Sat Jun 4 03:31:10 UTC 2022 Modified Files: src/sys/dev: files.audio files.dev midi.c sequencer.c src/sys/modules: Makefile src/sys/modules/midi: Makefile src/sys/modules/sequencer: Makefile Added Files: src/sys/dev: midi_mod.c midi_seq_mod.c sequencer_mod.c src/sys/modules/midi_seq: Makefile midi_seq.ioconf Removed Files: src/sys/modules/midi: midi.ioconf src/sys/modules/sequencer: sequencer.ioconf Log Message: Combine the midi and sequencer modules into a single midi_seq module to avoid a circular dependency as noted in kern/56772. Retain minimal modules of the original names to accomodate auto-loading upon access to the /dev/xxx nodes. This breaks at least sparc64/GENERIC: sys/dev/sequencer.c:285:1: error: no previous prototype for 'sequencerattach' [-Werror=missing-prototypes] 285 | sequencerattach(int n) | ^~~ It has "midi* at midibus?" but no "pseudo-device sequencer". -- J. Hannken-Illjes - hann...@mailbox.org ++--+--+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses:| | (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com| | Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org | | & Network Engineer | | pgoyett...@gmail.com | ++--+--+
Re: CVS commit: src/sys
> On 4. Jun 2022, at 05:31, Paul Goyette wrote: > > Module Name: src > Committed By: pgoyette > Date: Sat Jun 4 03:31:10 UTC 2022 > > Modified Files: > src/sys/dev: files.audio files.dev midi.c sequencer.c > src/sys/modules: Makefile > src/sys/modules/midi: Makefile > src/sys/modules/sequencer: Makefile > Added Files: > src/sys/dev: midi_mod.c midi_seq_mod.c sequencer_mod.c > src/sys/modules/midi_seq: Makefile midi_seq.ioconf > Removed Files: > src/sys/modules/midi: midi.ioconf > src/sys/modules/sequencer: sequencer.ioconf > > Log Message: > Combine the midi and sequencer modules into a single midi_seq module > to avoid a circular dependency as noted in kern/56772. Retain minimal > modules of the original names to accomodate auto-loading upon access > to the /dev/xxx nodes. This breaks at least sparc64/GENERIC: sys/dev/sequencer.c:285:1: error: no previous prototype for 'sequencerattach' [-Werror=missing-prototypes] 285 | sequencerattach(int n) | ^~~ It has "midi* at midibus?" but no "pseudo-device sequencer". -- J. Hannken-Illjes - hann...@mailbox.org signature.asc Description: Message signed with OpenPGP
Re: CVS commit: src/sys/dev/marvell
On 2022/05/21 19:27, Rin Okuyama wrote: Module Name:src Committed By: rin Date: Sat May 21 10:27:30 UTC 2022 Modified Files: src/sys/dev/marvell: if_mvgbe.c Log Message: m_freem() *after* bus_dmamap_sync() and bus_dmamap_load() for that mbuf. This is mandatory for some archs. To generate a diff of this commit: cvs rdiff -u -r1.64 -r1.65 src/sys/dev/marvell/if_mvgbe.c s/load/unload/ here. Thanks riastradh@ for pointing out. rin
Re: CVS commit: src/sys/arch
Oops, I missed it. I will fix soon. Thanks again, rin On 2022/05/07 17:24, Valery Ushakov wrote: On Sat, May 07, 2022 at 04:12:55 +, Rin Okuyama wrote: Module Name:src Committed By: rin Date: Sat May 7 04:12:55 UTC 2022 Modified Files: src/sys/arch/evbppc/dht: locore.S src/sys/arch/powerpc/include: spr.h Log Message: Instead of hard-coding SPR# for CCR0, define SPR_CCR0 in and use it. Idea from uwe@, thanks! (and sorry for delayed response!) Note that we already have it defined (along with bunch of others) in sys/arch/powerpc/include/ibm4xx/spr.h:74 -uwe
Re: CVS commit: src/sys/arch
On Sat, May 07, 2022 at 04:12:55 +, Rin Okuyama wrote: > Module Name: src > Committed By: rin > Date: Sat May 7 04:12:55 UTC 2022 > > Modified Files: > src/sys/arch/evbppc/dht: locore.S > src/sys/arch/powerpc/include: spr.h > > Log Message: > Instead of hard-coding SPR# for CCR0, define SPR_CCR0 in > and use it. > > Idea from uwe@, thanks! > (and sorry for delayed response!) Note that we already have it defined (along with bunch of others) in sys/arch/powerpc/include/ibm4xx/spr.h:74 -uwe
Re: CVS commit: src/sys/lib/libsa
On 2022/04/28 3:29, Christos Zoulas wrote: In article <20220427144850.61383f...@cvs.netbsd.org>, Rin Okuyama wrote: -=-=-=-=-=- Module Name:src Committed By: rin Date: Wed Apr 27 14:48:50 UTC 2022 Modified Files: src/sys/lib/libsa: ext2fs.c minixfs3.c stand.h ufs.c Log Message: Revert previous at the moment. This is wrong reasoning; 68020 and above (incl. 040 and 060) support 32-bit displacements for PC relative addressing (via "fully extension addressing mode" with null index register). I've still not figured out what goes wrong with amiga/boot(8) when compiled without -l option for gas(1)... But that was a nice change :-) Thanks! Actually, it should be useful for ancient archs. Now, amiga/boot(8) is fixed. I will enable this again. rin
Re: CVS commit: src/sys/lib/libsa
In article <20220427144850.61383f...@cvs.netbsd.org>, Rin Okuyama wrote: >-=-=-=-=-=- > >Module Name: src >Committed By: rin >Date: Wed Apr 27 14:48:50 UTC 2022 > >Modified Files: > src/sys/lib/libsa: ext2fs.c minixfs3.c stand.h ufs.c > >Log Message: >Revert previous at the moment. > >This is wrong reasoning; 68020 and above (incl. 040 and 060) support >32-bit displacements for PC relative addressing (via "fully extension >addressing mode" with null index register). > >I've still not figured out what goes wrong with amiga/boot(8) when >compiled without -l option for gas(1)... But that was a nice change :-) christos
Re: CVS commit: src/sys/arch/x68k/stand
> Module Name: src > Committed By: mlelstv > Date: Mon Apr 25 15:12:07 UTC 2022 > > Modified Files: > src/sys/arch/x68k/stand/boot: conf.c > src/sys/arch/x68k/stand/xxboot: Makefile.xxboot xx.c > > Log Message: > libsa now needs ioctl to support media with large sectors. Provide > missing functions. Only for bootloaders that load a kernel? FS_xxx ops in libsa is also used to load secondary bootloaders (not a kernel that might require module info) on several ports, including x86, and I guess in that case ioctl or the "fsmod" variable is not necessary as before. (I'm afraid "const char *fsmod" might require some ifdefs) --- Izumi Tsutsui
Re: CVS commit: src/sys/arch/amiga/stand/bootblock/elf2bb
On 2022/04/25 23:03, Rin Okuyama wrote:
Module Name:src
Committed By: rin
Date: Mon Apr 25 14:03:15 UTC 2022
Modified Files:
src/sys/arch/amiga/stand/bootblock/elf2bb: chksum.c elf2bb.c
Log Message:
Use htobe{16,32}(9) instead of be{16,32}toh(9) where appropriate.
No binary changes both for little and big endian machines.
Oops, "be{16,32}toh(3) instead of htobe{16,32}(3)", apparently...
Thanks,
rin
Re: CVS commit: src/sys/dev/raidframe
> On 16. Apr 2022, at 18:40, Andrius Varanavicius wrote: > > Module Name: src > Committed By: andvar > Date: Sat Apr 16 16:40:54 UTC 2022 > > Modified Files: > src/sys/dev/raidframe: rf_netbsdkintf.c > > Log Message: > Fix mistake in error branch locking caused by previous changes. > vput(vp) also unlocks vp, thus unlocking happens twice in error flow > causing kernel to panic with failed assertion lktype != LK_NONE > in vfs_vnode.c#778. Thanks riastradh with finding the issue. > > > To generate a diff of this commit: > cvs rdiff -u -r1.406 -r1.407 src/sys/dev/raidframe/rf_netbsdkintf.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. Thanks, replacing vput() with vrele() would have been even better ... -- J. Hannken-Illjes - hann...@mailbox.org signature.asc Description: Message signed with OpenPGP
Re: CVS commit: src/sys/fs/udf
Am Fri, Mar 18, 2022 at 04:06:18PM + schrieb Reinoud Zandijk: > Module Name: src > Committed By: reinoud > Date: Fri Mar 18 16:06:18 UTC 2022 > > Modified Files: > src/sys/fs/udf: ecma167-udf.h > > Log Message: > Replace the variable field data[0] to data[1] to avoid undefined behaviour. Just use a flexible array member, data[]. That gives essentially the same behavior as the ancient GNU extension of [0] but sanitizers knows what it means. That said, it is somewhat pointless in a packed data structure... Joerg
re: CVS commit: src/sys/external/bsd/drm/dist/bsd-core
> Log Message: > old drm: Use getticks(), not hardclock_ticks. > > Should delete this code, no idea if it even still compiles. it did last i tried, and it still worked on the r100/r200 systems that new drm is still problematic with. not objecting... .mrg.
Re: CVS commit: src/sys/dev/pci
On Sun, Feb 27, 2022 at 11:50:15AM +0100, Martin Husemann wrote: > On Sun, Feb 27, 2022 at 12:04:58AM +0100, Joerg Sonnenberger wrote: > > Personally, I prefer the use of the C extension in cases like this. The > > "portable" version is less readable... > > I would prefer the type correct C++ style variant (and making lint deal > with it, if it doesn't yet). At least gcc is happy with it. Ah, never mind - the inline function is void already - so effectively I agree with Jörg. Martin
Re: CVS commit: src/sys/dev/pci
On Sun, Feb 27, 2022 at 12:04:58AM +0100, Joerg Sonnenberger wrote:
> Personally, I prefer the use of the C extension in cases like this. The
> "portable" version is less readable...
I would prefer the type correct C++ style variant (and making lint deal
with it, if it doesn't yet). At least gcc is happy with it.
Index: if_wm.c
===
RCS file: /cvsroot/src/sys/dev/pci/if_wm.c,v
retrieving revision 1.728
diff -u -p -r1.728 if_wm.c
--- if_wm.c 26 Feb 2022 14:53:05 - 1.728
+++ if_wm.c 27 Feb 2022 10:48:11 -
@@ -10025,7 +10025,7 @@ wm_txrxintr_disable(struct wm_queue *wmq
struct wm_softc *sc = wmq->wmq_txq.txq_sc;
if (__predict_false(!wm_is_using_msix(sc))) {
- return wm_legacy_intr_disable(sc);
+ return (void)wm_legacy_intr_disable(sc);
}
if (sc->sc_type == WM_T_82574)
@@ -10046,7 +10046,7 @@ wm_txrxintr_enable(struct wm_queue *wmq)
wm_itrs_calculate(sc, wmq);
if (__predict_false(!wm_is_using_msix(sc))) {
- return wm_legacy_intr_enable(sc);
+ return (void)wm_legacy_intr_enable(sc);
}
/*
Re: CVS commit: src/sys/dev/pci
Am Sat, Feb 26, 2022 at 03:04:40PM + schrieb Roland Illig: > Module Name: src > Committed By: rillig > Date: Sat Feb 26 15:04:40 UTC 2022 > > Modified Files: > src/sys/dev/pci: if_wm.c > > Log Message: > if_wm.c: fix value return from void function > > lint complained: > if_wm.c(10028): error: > void function wm_txrxintr_disable cannot return value [213] > if_wm.c(10049): error: > void function wm_txrxintr_enable cannot return value [213] > > No functional change. Personally, I prefer the use of the C extension in cases like this. The "portable" version is less readable... Joerg
Re: CVS commit: src/sys/kern
> Date: Fri, 11 Feb 2022 16:50:16 -0800 > From: Jason Thorpe > > > On Feb 11, 2022, at 9:53 AM, Taylor R Campbell wrote: > > > > That is, this was presumably meant to ensure (A) happens-before (B). > > This relation is already guaranteed by ipi(9), so there is no need > > for any explicit memory barrier. > > Is this documented in ipi(9)? Is now!
Re: CVS commit: src/sys/kern
> On Feb 11, 2022, at 9:53 AM, Taylor R Campbell wrote: > > That is, this was presumably meant to ensure (A) happens-before (B). > This relation is already guaranteed by ipi(9), so there is no need > for any explicit memory barrier. Is this documented in ipi(9)? -- thorpej
Re: CVS commit: src/sys/kern
> Date: Sat, 5 Feb 2022 22:47:30 +0100 > From: Tobias Nygren > > On Sat, 5 Feb 2022 15:17:40 + > Martin Husemann wrote: > > > Modified Files: > > src/sys/kern: subr_autoconf.c > > > > Log Message: > > cfdriver_iattr_count() is only used in a KASSERT, so #ifdef DIAGNOSTIC it. > > This doesn't seem to work as intended. > In a kernel with "no options DIAGNOSTIC" I now get: > > /work/src/sys/kern/subr_autoconf.c: In function 'config_search_internal': > /work/src/sys/kern/subr_autoconf.c:1149:3: error: implicit declaration of > function 'cfdriver_iattr_count' [-Werror=implicit-function-declaration] I guess we need to decorate cfdriver_iattr_count with __diagused. (Point of the KASSERT change the other month was to eliminate most of these `oops I forgot to try a DIAGNOSTIC / !DIAGNOSTIC build' problems and reduce #ifdefs; thanks, clang...)
Re: CVS commit: src/sys/kern
On Sat, 5 Feb 2022 15:17:40 + Martin Husemann wrote: > Modified Files: > src/sys/kern: subr_autoconf.c > > Log Message: > cfdriver_iattr_count() is only used in a KASSERT, so #ifdef DIAGNOSTIC it. This doesn't seem to work as intended. In a kernel with "no options DIAGNOSTIC" I now get: /work/src/sys/kern/subr_autoconf.c: In function 'config_search_internal': /work/src/sys/kern/subr_autoconf.c:1149:3: error: implicit declaration of function 'cfdriver_iattr_count' [-Werror=implicit-function-declaration] 1149 | cfdriver_iattr_count(parent->dv_cfdriver) < 2); | ^~~~ /work/src/sys/lib/libkern/libkern.h:279:42: note: in definition of macro 'KASSERT' 279 | #define KASSERT(e) ((void)sizeof((long)(e)))
Re: CVS commit: src/sys/dev/fdt
Should fix PR 56650 > Module Name: src > Committed By: macallan > Date: Fri Jan 21 21:00:26 UTC 2022 > > Modified Files: > src/sys/dev/fdt: fdt_port.c > > Log Message: > when enumerating ports and endpoints treat missing 'reg' properties as zero > ok jmcneill: > Looking at Linux. If port or endpoint are missing a 'reg' property it > defaults to 0. > Please make our code do the same. > > see > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml > > with this my pinebook has a usable console again > > > To generate a diff of this commit: > cvs rdiff -u -r1.6 -r1.7 src/sys/dev/fdt/fdt_port.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. >
Re: CVS commit: src/sys/conf
> Modified Files: > src/sys/conf: copyright > > Log Message: > Welcome to 2022! Please don't forget pullup requests for release branches. (looks 2021 was missed in 9.2) --- Izumi Tsutsui
re: CVS commit: src/sys/conf
> - We only make a debuginstall rule to install netbsd-${KERNEL_CONFIG}.debug
> if MKDEBUG=yes
this part pollutes my $DESTDIR with non-distrib kernel info still,
which also means it requires a $DESTDIR to build a kernel now.
i think that we should not do this in the kernel build but inside
the sets build, and create the set with ./netbsd and the
./usr/libdata/debug/netbsd-$KERNEL.debug path stored in some temp
dir of distrib/sets/. that avoids issues of building stuff being
all different now, while allowing this to create sets sanely.
.mrg.
Re: CVS commit: src/sys/arch/i386/stand/efiboot
Emmanuel Dreyfus wrote:
> In src/sys/arch/i386/stand/lib/biosdisk.c
> int
> biosdisk_findpartition(int biosdev, daddr_t sector,
>int *partition, const char **part_name)
> {
> (...)
> /* default ot first partition */
> *partition = 0;
> *part_name = NULL;
>
> part_name is NULL, *part_name crashes. How do you avoid that?
Aha, I have this elsewhere in my zfs tree:
*partition = 0;
- *part_name = NULL;
+ if (part_name)
+ *part_name = NULL;
I'll commit that now (as well as the same check for the
NO_DISKLABEL && NO_GPT case. Thanks for the digging!
Cheers,
Simon.
Re: CVS commit: src/sys/arch/i386/stand/efiboot
Emmanuel Dreyfus wrote: > On Mon, Dec 27, 2021 at 01:08:15PM +1100, Simon Burge wrote: > > What crash did this fix? All the use of part_name by the > > called functions should check if it is NULL before trying > > to assign anything to *part_name. > > I do not recall the details now, but I had a crash because > of this. Please revert my change, I will get back to it when > I find some time. Thanks. I'll revert that now. If you have a way of preproducing this, I'm happy to have a look. Cheers, Simon.
Re: CVS commit: src/sys/arch/i386/stand/efiboot
On Mon, Dec 27, 2021 at 01:08:15PM +1100, Simon Burge wrote: > What crash did this fix? All the use of part_name by the > called functions should check if it is NULL before trying > to assign anything to *part_name. I do not recall the details now, but I had a crash because of this. Please revert my change, I will get back to it when I find some time. -- Emmanuel Dreyfus m...@netbsd.org
Re: CVS commit: src/sys/arch/i386/stand/efiboot
Hi Emmanuel, "Emmanuel Dreyfus" wrote: > Module Name: src > Committed By: manu > Date: Thu Nov 18 16:18:13 UTC 2021 > > Modified Files: > > src/sys/arch/i386/stand/efiboot: devopen.c > > Log Message: > > Fix crash because of NULL pointer reference What crash did this fix? All the use of part_name by the called functions should check if it is NULL before trying to assign anything to *part_name. This change has broken loading boot.cfg via the EFI path "esp:/EFI/NetBSD/boot.cfg" since the call to bios_boot() at https://nxr.netbsd.org/xref/src/sys/arch/i386/stand/efiboot/devopen.c#292 with a non-NULL last argument means devname gets updated and now points to the partition with a root filesystem rather than the EFI system partition. Cheers, Simon.
Re: CVS commit: src/sys/dev/pci
On 23/12/2021 17:05, Juergen Hannken-Illjes wrote: Module Name:src Committed By: hannken Date: Thu Dec 23 17:05:49 UTC 2021 Modified Files: src/sys/dev/pci: if_wm.c Log Message: Keep constants 32 bit, why does __BIT() return uintmax_t? PRIxBIT is available Nick
Re: CVS commit: src/sys/net
On Mon, 15 Nov 2021 07:07:06 + Shoichi YAMAGUCHI wrote: > Date: Mon Nov 15 07:07:06 UTC 2021 > > Modified Files: > src/sys/net: if_ether.h if_ethersubr.c if_vlan.c > src/sys/net/lagg: if_lagg.c > > Log Message: > introduced APIs to configure VLAN TAG to ethernet devices Hello, This change seems to have broke something MTU related when more than two VLANs are configured on the same NIC. The first VLAN inherits the parent's MTU but the subsequent ones have 4 byte too small MTU causing network problems. Kernel from 2021-11-15: # ifconfig |grep mtu wm0: flags=0x8843 mtu 1500 vlan100: flags=0x8843 mtu 1500 vlan101: flags=0x8843 mtu 1500 vlan102: flags=0x8843 mtu 1500 vlan103: flags=0x8843 mtu 1500 vlan104: flags=0x8843 mtu 1500 vlan105: flags=0x8843 mtu 1500 vlan106: flags=0x8843 mtu 1500 Kernel from 2021-11-16: wm0: flags=0x8843 mtu 1500 vlan100: flags=0x8843 mtu 1500 vlan101: flags=0x8843 mtu 1496 vlan102: flags=0x8843 mtu 1496 vlan103: flags=0x8843 mtu 1496 vlan104: flags=0x8843 mtu 1496 vlan105: flags=0x8843 mtu 1496 vlan106: flags=0x8843 mtu 1496 I could not force increase the MTU either: root@tinfoilhat-b:log> ifconfig vlan100 mtu 1500 root@tinfoilhat-b:log> ifconfig vlan101 mtu 1500 ifconfig: SIOCSIFMTU: Invalid argument Kind regards, -Tobias
Re: CVS commit: src/sys/dev/tprof
On 26/11/2021 13:24, Christos Zoulas wrote: Module Name:src Committed By: christos Date: Fri Nov 26 13:24:28 UTC 2021 Modified Files: src/sys/dev/tprof: tprof_armv7.c tprof_armv8.c Log Message: declare xc Thanks! Nick
Re: CVS commit: src/sys
On 2021/11/20 8:46, Rin Okuyama wrote: Enable this quirk for "C600/X79 AHCI". Also add commented out quirk entries for "Bay Trail SATA (AHCI)" and "Mobile AHCI SATA Controller", for which non-reproducible failures worked around by extra delays have been reported. Oops, I meant: "Mobile AHCI SATA Controller" --> "82801I Mobile AHCI SATA Controller" Thanks, rin
Re: CVS commit: src/sys/arch/arm/sa11x0
On 2021/11/09 8:57, Rin Okuyama wrote: Module Name:src Committed By: rin Date: Mon Nov 8 23:57:23 UTC 2021 Modified Files: src/sys/arch/arm/sa11x0: sa11x0_irq.S Log Message: irq_entry(): Do not clobber fp (= r11), in order not to confuse DDB. (snip) XXX Rewrite this function by C. There seems no particular reason to use assembler, and no major performance regression is expected. No reason to use assembler, if converted to use arm/arm32/irq_dispatch.S. Thanks, rin
Re: CVS commit: src/sys/arch
On Sun, 31 Oct 2021 16:23:48 + Nick Hudson wrote: > Modified Files: > src/sys/arch/aarch64/include: cpu.h cpufunc.h db_machdep.h ... > Log Message: > Rework Arm (32bit and 64bit) AP startup so that cpu_hatch doesn't sleep. Hi, I'm afraid this broke the userland build. I think db_machdep_init(...) should move to the ifdef _KERNEL block? compile crash/db_autoconf.o In file included from /usr/src/../obj/usr.sbin/crash/machine/db_machdep.h:4, from /usr/src/usr.sbin/crash/../../sys/ddb/ddb.h:35, from /usr/src/usr.sbin/crash/../../sys/ddb/db_autoconf.c:39: /usr/src/../obj/usr.sbin/crash/aarch64/db_machdep.h:231:29: error: 'struct cpu_info' declared inside parameter list will not be visible outside of this definition or declaration [-Werror] 231 | void db_machdep_init(struct cpu_info * const); -Tobias
Re: CVS commit: src/sys/dev/sbus
>Module Name: src >Committed By: macallan >Date: Sat Oct 30 05:37:39 UTC 2021 > >Modified Files: > src/sys/dev/sbus: mgx.c mgxreg.h > >Log Message: >actually mmap() the blitter registers when asked to, while there do some >magic number reduction > > >To generate a diff of this commit: >cvs rdiff -u -r1.17 -r1.18 src/sys/dev/sbus/mgx.c >cvs rdiff -u -r1.5 -r1.6 src/sys/dev/sbus/mgxreg.h @@ -684,6 +684,8 @@ uint32_t fg, bg; int x, y, wi, he, rv; +if (sc->sc_mode != WSDISPLAYIO_MODE_EMUL) return; + wi = font->fontwidth; he = font->fontheight; @@ -731,6 +733,8 @@ uint32_t fg, bg, scratch = ((sc->sc_stride * sc->sc_height) + 7) & ~7; int x, y, wi, he, len, i; +if (sc->sc_mode != WSDISPLAYIO_MODE_EMUL) return; + wi = font->fontwidth; he = font->fontheight; This seems to be a strange indent. Or debugging code mixed in? -- ryo shimizu
Re: CVS commit: src/sys/dev/pci
On Thu, Oct 21, 2021 at 05:32:28AM +, Shoichi YAMAGUCHI wrote: > Module Name: src > Committed By: yamaguchi > Date: Thu Oct 21 05:32:27 UTC 2021 > > Modified Files: > src/sys/dev/pci: virtio.c virtio_pci.c virtiovar.h > > Log Message: > divide setup routine of virtio interrupts > into establishment and device configuration > > > To generate a diff of this commit: > cvs rdiff -u -r1.49 -r1.50 src/sys/dev/pci/virtio.c > cvs rdiff -u -r1.30 -r1.31 src/sys/dev/pci/virtio_pci.c > cvs rdiff -u -r1.20 -r1.21 src/sys/dev/pci/virtiovar.h This seems to have broken the virtio_mmio build.
Re: CVS commit: src/sys/dev/pci
Hi, Thank you for your quick commit! Thanks, On 2021/10/18 17:15, Juergen Hannken-Illjes wrote: Module Name:src Committed By: hannken Date: Mon Oct 18 08:15:00 UTC 2021 Modified Files: src/sys/dev/pci: xmm7360.c Log Message: Use a local static variable to hold "pktq_rps_hash_default" like the other devices do. Kernel ALL/amd64 compiles again. OK: Kengo NAKAHARA To generate a diff of this commit: cvs rdiff -u -r1.12 -r1.13 src/sys/dev/pci/xmm7360.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. -- // Internet Initiative Japan Inc. Device Engineering Section, Product Division, Technology Unit Kengo NAKAHARA
Re: CVS commit: src/sys/arch/x86/x86
hehehe - porn iterators - love it! On Fri, 15 Oct 2021, Jason Thorpe wrote: I demand this change be reverted. (/s) On Oct 15, 2021, at 11:12 AM, Jared D. McNeill wrote: Module Name:src Committed By: jmcneill Date: Fri Oct 15 18:12:48 UTC 2021 Modified Files: src/sys/arch/x86/x86: tsc.c Log Message: Fix typo in comment: "porniters" -> "pointers" To generate a diff of this commit: cvs rdiff -u -r1.56 -r1.57 src/sys/arch/x86/x86/tsc.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. -- thorpej !DSPAM:6169cf82254421105921466! ++--+--+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses:| | (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com| | Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org | | & Network Engineer | | pgoyett...@gmail.com | ++--+--+
Re: CVS commit: src/sys/arch/x86/x86
I demand this change be reverted. (/s) > On Oct 15, 2021, at 11:12 AM, Jared D. McNeill wrote: > > Module Name: src > Committed By: jmcneill > Date: Fri Oct 15 18:12:48 UTC 2021 > > Modified Files: > src/sys/arch/x86/x86: tsc.c > > Log Message: > Fix typo in comment: "porniters" -> "pointers" > > > To generate a diff of this commit: > cvs rdiff -u -r1.56 -r1.57 src/sys/arch/x86/x86/tsc.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. > -- thorpej
Re: CVS commit: src/sys/dev/wscons
On Sun, Oct 10, 2021 at 10:44:03PM +0900, Izumi Tsutsui wrote: > > Module Name:src > > Committed By: nia > > Date: Tue Sep 28 06:14:28 UTC 2021 > > > > Modified Files: > > src/sys/dev/wscons: wsconsio.h wsmouse.c wsmousevar.h > > > > Log Message: > > wsmouse: add support for "precision scrolling" events and (GET|SET)PARAMS > > Could you please also update wsmouse(4) and wsmouse(9) man pages? > Even the current version lacks various info (especially about ioctl(2)) > but no reason to make it worse. > > Thanks, > --- > Izumi Tsutsui Sure, I had been meaning to anyway but I clearly forgot to commit those files...
Re: CVS commit: src/sys/arch
On 11/10/2021 08:14, Rin Okuyama wrote: Hi, On 2021/09/23 15:34, Nick Hudson wrote: Module Name: src Committed By: skrll Date: Thu Sep 23 06:34:00 UTC 2021 Modified Files: src/sys/arch/aarch64/aarch64: cpufunc.c src/sys/arch/arm/arm32: cpu.c Log Message: Print the cache information in similar formats and arm and aarch64, e.g. For classic ARM CPUs, info->[id]cache_sets are not set. This results in cpu0 at mainbus0 core 0: SA-1110 step B-5 (SA-1 V4 core) cpu0: DC enabled IC enabled WB enabled LABT cpu0: L1 16KB/32B 32-way (0 set) VIVT Instruction cache cpu0: L1 8KB/32B 32-way (0 set) write-back VIVT Data cache or cpu0 at mainbus0 core 0: ARM926EJ-S r0p0 (ARM9EJ-S V5TEJ core) cpu0: DC enabled IC enabled WB enabled LABT cpu0: L1 32KB/32B 1-way (0 set) VIVT Instruction cache cpu0: L1 32KB/32B 1-way (0 set) write-back-locking-C VIVT Data cache Can I commit the attached patch? Or initialize these variables somewhere else? I think your patch is fine. Thanks for fixing this. Nick
Re: CVS commit: src/sys/arch
Thanks for quick response. Committed :) rin On 2021/10/11 16:27, Nick Hudson wrote: On 11/10/2021 08:14, Rin Okuyama wrote: Hi, On 2021/09/23 15:34, Nick Hudson wrote: Module Name: src Committed By: skrll Date: Thu Sep 23 06:34:00 UTC 2021 Modified Files: src/sys/arch/aarch64/aarch64: cpufunc.c src/sys/arch/arm/arm32: cpu.c Log Message: Print the cache information in similar formats and arm and aarch64, e.g. For classic ARM CPUs, info->[id]cache_sets are not set. This results in cpu0 at mainbus0 core 0: SA-1110 step B-5 (SA-1 V4 core) cpu0: DC enabled IC enabled WB enabled LABT cpu0: L1 16KB/32B 32-way (0 set) VIVT Instruction cache cpu0: L1 8KB/32B 32-way (0 set) write-back VIVT Data cache or cpu0 at mainbus0 core 0: ARM926EJ-S r0p0 (ARM9EJ-S V5TEJ core) cpu0: DC enabled IC enabled WB enabled LABT cpu0: L1 32KB/32B 1-way (0 set) VIVT Instruction cache cpu0: L1 32KB/32B 1-way (0 set) write-back-locking-C VIVT Data cache Can I commit the attached patch? Or initialize these variables somewhere else? I think your patch is fine. Thanks for fixing this. Nick
Re: CVS commit: src/sys/arch
Hi, On 2021/09/23 15:34, Nick Hudson wrote: Module Name:src Committed By: skrll Date: Thu Sep 23 06:34:00 UTC 2021 Modified Files: src/sys/arch/aarch64/aarch64: cpufunc.c src/sys/arch/arm/arm32: cpu.c Log Message: Print the cache information in similar formats and arm and aarch64, e.g. For classic ARM CPUs, info->[id]cache_sets are not set. This results in cpu0 at mainbus0 core 0: SA-1110 step B-5 (SA-1 V4 core) cpu0: DC enabled IC enabled WB enabled LABT cpu0: L1 16KB/32B 32-way (0 set) VIVT Instruction cache cpu0: L1 8KB/32B 32-way (0 set) write-back VIVT Data cache or cpu0 at mainbus0 core 0: ARM926EJ-S r0p0 (ARM9EJ-S V5TEJ core) cpu0: DC enabled IC enabled WB enabled LABT cpu0: L1 32KB/32B 1-way (0 set) VIVT Instruction cache cpu0: L1 32KB/32B 1-way (0 set) write-back-locking-C VIVT Data cache Can I commit the attached patch? Or initialize these variables somewhere else? Thanks, rin Index: sys/arch/arm/arm32/cpu.c === RCS file: /home/netbsd/src/sys/arch/arm/arm32/cpu.c,v retrieving revision 1.149 diff -p -u -r1.149 cpu.c --- sys/arch/arm/arm32/cpu.c23 Sep 2021 06:34:00 - 1.149 +++ sys/arch/arm/arm32/cpu.c8 Oct 2021 00:11:34 - @@ -612,7 +612,9 @@ print_cache_info(device_t dv, struct arm level + 1, info->dcache_size / 1024, info->dcache_line_size, info->dcache_ways, - info->dcache_sets, + info->dcache_sets ? info->dcache_sets : + info->dcache_size / + (info->dcache_line_size * info->dcache_ways), wtnames[info->cache_type], info->dcache_type & CACHE_TYPE_PIxx ? 'P' : 'V', info->dcache_type & CACHE_TYPE_xxPT ? 'P' : 'V'); @@ -621,14 +623,18 @@ print_cache_info(device_t dv, struct arm level + 1, info->icache_size / 1024, info->icache_line_size, info->icache_ways, - info->icache_sets, + info->icache_sets ? info->icache_sets : + info->icache_size / + (info->icache_line_size * info->icache_ways), info->icache_type & CACHE_TYPE_PIxx ? 'P' : 'V', info->icache_type & CACHE_TYPE_xxPT ? 'P' : 'V'); aprint_normal_dev(dv, "L%u %dKB/%dB %d-way (%u set) %s %cI%cT Data cache\n", level + 1, info->dcache_size / 1024, info->dcache_line_size, info->dcache_ways, - info->dcache_sets, + info->dcache_sets ? info->dcache_sets : + info->dcache_size / + (info->dcache_line_size * info->dcache_ways), wtnames[info->cache_type], info->dcache_type & CACHE_TYPE_PIxx ? 'P' : 'V', info->dcache_type & CACHE_TYPE_xxPT ? 'P' : 'V');
Re: CVS commit: src/sys/dev/wscons
> Module Name: src > Committed By: nia > Date: Tue Sep 28 06:14:28 UTC 2021 > > Modified Files: > src/sys/dev/wscons: wsconsio.h wsmouse.c wsmousevar.h > > Log Message: > wsmouse: add support for "precision scrolling" events and (GET|SET)PARAMS Could you please also update wsmouse(4) and wsmouse(9) man pages? Even the current version lacks various info (especially about ioctl(2)) but no reason to make it worse. Thanks, --- Izumi Tsutsui
Re: CVS commit: src/sys/arch/arm/arm
Oops, forgot to mention: No binary changes. On 2021/10/07 18:58, Rin Okuyama wrote: Module Name:src Committed By: rin Date: Thu Oct 7 09:58:27 UTC 2021 Modified Files: src/sys/arch/arm/arm: cpufunc_asm_armv5_ec.S Log Message: Reduce diff with cpufunc_asm_armv5.S, from which this file was derived. To generate a diff of this commit: cvs rdiff -u -r1.6 -r1.7 src/sys/arch/arm/arm/cpufunc_asm_armv5_ec.S Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
Re: CVS commit: src/sys
> Date: Wed, 6 Oct 2021 17:44:22 +0200 > From: Reinoud Zandijk > > Sorry for my ignorance but I thought that the ksyms list was static? ie > read-only? Or is this change to support kernel modules symbols too and thus > need rw access control? The main kernel's symbols don't change, but the whole ksyms object consists of the main kernel's symbol table and every module's symbol table, which changes every time a module is loaded or unloaded. This has been the case ever since ksyms was first introduced in 2003. Previously callers in the kernel getting at ksyms were required to hold ksyms_lock, or have all other CPUs quiescent like in ddb, or, realistically, just pray no modules get loaded or unloaded. Now they can do it reliably and scalably in a pserialize read section.
Re: CVS commit: src/sys
On Wed, Sep 15, 2021 at 07:58:20PM +0900, Rin Okuyama wrote: > On 2021/09/11 19:09, Taylor R Campbell wrote: > > Module Name:src > > Committed By: riastradh > > Date: Sat Sep 11 10:09:55 UTC 2021 > > > > Modified Files: > > src/sys/arch/sparc64/sparc64: machdep.c > > src/sys/kern: kern_ksyms.c subr_csan.c subr_msan.c > > src/sys/sys: ksyms.h > > > > Log Message: > > ksyms: Use pserialize(9) for kernel access to ksyms. Sorry for my ignorance but I thought that the ksyms list was static? ie read-only? Or is this change to support kernel modules symbols too and thus need rw access control? With regards, Reinoud
Re: CVS commit: src/sys
Date:Wed, 29 Sep 2021 08:42:12 -0700 From:Jason Thorpe Message-ID: | Anything that depends on the new return value would have simply been | doing what the socket / fifo code was doing (groveling around in | selinfo internals), so it's not like they're broken as a result of | this change. I agree with that. The only potential issue I can see would be if we had an architecture where the ABI specifies that the register normally used to return a result is preserved across a call to a void function. That would cause problems with this change - but absent any such architecture there should be no issues requiring a bump. kre
