Re: CVS commit: src/sys/dev/rasops
Hi, Oops, sorry for breakage! Can you try the attached patch please? Thanks, rin On 2019/07/30 1:32, Ryo ONODERA wrote: Hi, "Rin Okuyama" writes: Module Name:src Committed By: rin Date: Mon Jul 29 03:01:09 UTC 2019 Modified Files: src/sys/dev/rasops: rasops2.c rasops4.c rasops_putchar_width.h Log Message: Convert rasops2.c and rasops4.c to use rasops_putchar_width.h. Style. To generate a diff of this commit: cvs rdiff -u -r1.25 -r1.26 src/sys/dev/rasops/rasops2.c cvs rdiff -u -r1.19 -r1.20 src/sys/dev/rasops/rasops4.c cvs rdiff -u -r1.6 -r1.7 src/sys/dev/rasops/rasops_putchar_width.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Thanks for your hard work. After this commit, my laptop stops just after 1 character, [, is displayed. Latest src tree with partial reverts to src/sys/dev/rasops/rasops2.c 1.25 src/sys/dev/rasops/rasops4.c 1.19 src/sys/dev/rasops/rasops_putchar_width.h 1.6 works fine. Could you take a look at my problem? The dmesg on my laptop is as follows: 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 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 8.99.51 (DTRACE7) #8: Tue Jul 30 01:23:26 JST 2019 ryoon@brownie:/usr/world/8.99/amd64/obj/sys/arch/amd64/compile/DTRACE7 total memory = 16132 MB avail memory = 15639 MB cpu_rng: RDSEED timecounter: Timecounters tick every 10.000 msec Kernelized RAIDframe activated running cgd selftest aes-xts-256 aes-xts-512 done acpibat* disabled acpibat* already disabled timecounter: Timecounter "i8254" frequency 1193182 Hz quality 100 efi: systbl at pa 2ff15018 HP HP Spectre x360 Convertible 13-ae0xx ( ) mainbus0 (root) ACPI: RSDP 0x2F36 24 (v02 HPQOEM) ACPI: XSDT 0x2F3600C0 FC (v01 HPQOEM SLIC-MPC 01072009 HP 00010013) ACPI: FACP 0x2F39AC60 000114 (v06 HPQOEM SLIC-MPC 01072009 HP 00010013) ACPI: DSDT 0x2F360248 03AA13 (v02 HPQOEM 83B9 01072009 ACPI 20160422) ACPI: FACS 0x2F7EAF00 40 ACPI: APIC 0x2F39AD78 BC (v03 HPQOEM 83B9 01072009 HP 00010013) ACPI: FPDT 0x2F39AE38 44 (v01 HPQOEM 83B9 01072009 HP 00010013) ACPI: FIDT 0x2F39AE80 9C (v01 HPQOEM 83B9 01072009 HP 00010013) ACPI: MCFG 0x2F39AF20 3C (v01 HPQOEM 83B9 01072009 HP 0097) ACPI: SSDT 0x2F39AF60 0003A3 (v01 HPQOEM 83B9 1000 ACPI 20160422) ACPI: SSDT 0x2F39B308 005D97 (v02 HPQOEM 83B9 1000 ACPI 20160422) ACPI: MSDM 0x2F3A10A0 55 (v03 HPQOEM SLIC-MPC 0001 HP 00010013) ACPI: SSDT 0x2F3A10F8 003156 (v02 HPQOEM 83B9 3000 ACPI 20160422) ACPI: HPET 0x2F3A4250 38 (v01 HPQOEM 83B9 0001 HP 005F) ACPI: SSDT 0x2F3A4288 24 (v02 HPQOEM 83B9 ACPI 20160422) ACPI: UEFI 0x2F3A42B0 42 (v01 HPQOEM 83B9 0002 HP 0113) ACPI: SSDT 0x2F3A42F8 0017AE (v02 HPQOEM 83B9 3000 ACPI 20160422) ACPI: LPIT 0x2F3A5AA8 94 (v01 HPQOEM 83B9 HP 005F) ACPI: SSDT 0x2F3A5B40 000141 (v02 HPQOEM 83B9 ACPI 20160422) ACPI: SSDT 0x2F3A5C88 00029F (v02 HPQOEM 83B9 ACPI 20160422) ACPI: SSDT 0x2F3A5F28 0011E7 (v02 HPQOEM 83B9 1000 ACPI 20160422) ACPI: SSDT 0x2F3A7110 00023D (v02 HPQOEM 83B9 ACPI 20160422) ACPI: DBGP 0x2F3A7350 34 (v01 HPQOEM 83B9 0002 HP 005F) ACPI: DBG2 0x2F3A7388 54 (v00 HPQOEM 83B9 0002 HP 005F) ACPI: DMAR 0x2F3A73E0 000114 (v01 HPQOEM 83B9 0001 HP 0001) ACPI: NHLT 0x2F3A74F8 2D (v00 HPQOEM 83B9 0002 HP 0113) ACPI: SSDT 0x2F3A7528 66 (v01 HPQOEM 83B9 0001 ACPI 20160422) ACPI: TPM2 0x2F3A7590 34 (v03 HPQOEM 83B9 0001 HP ) ACPI: ASF! 0x2F3A75C8 A0 (v32 HPQOEM 83B9 0001 HP 000F4240) ACPI: WSMT 0x2F3A7668 28 (v01 HPQOEM 83B9 01072009 HP 00010013) ACPI: BGRT 0x2F3A7690 38 (v01 HPQOEM 83B9 01072009 HP 00010013) ACPI: 11 ACPI AML tables successfully acquired and loaded ioapic0 at mainbus0 apid 2: pa 0xfec0, version 0x20, 120 pins x2APIC available but disabled by DMAR table cpu0 at mainbus0 apid 0 cpu0: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz, id 0x806ea cpu0: package 0, core 0, smt 0 cpu1 at mainbus0 apid 2 cpu1: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz, id 0x806ea cpu1: package 0, core 1, smt 0 cpu2 at mainbus0 apid 4 cpu2: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz, id 0x806ea cpu2: package 0, core 2, smt 0 cpu3 at mainbus0 apid 6 cpu3: Intel(R) Core(TM) i7-8550U
Re: CVS commit: src/sys/dev/rasops
Hi, "Rin Okuyama" writes: > Module Name: src > Committed By: rin > Date: Mon Jul 29 03:01:09 UTC 2019 > > Modified Files: > src/sys/dev/rasops: rasops2.c rasops4.c rasops_putchar_width.h > > Log Message: > Convert rasops2.c and rasops4.c to use rasops_putchar_width.h. > Style. > > > To generate a diff of this commit: > cvs rdiff -u -r1.25 -r1.26 src/sys/dev/rasops/rasops2.c > cvs rdiff -u -r1.19 -r1.20 src/sys/dev/rasops/rasops4.c > cvs rdiff -u -r1.6 -r1.7 src/sys/dev/rasops/rasops_putchar_width.h > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. Thanks for your hard work. After this commit, my laptop stops just after 1 character, [, is displayed. Latest src tree with partial reverts to src/sys/dev/rasops/rasops2.c 1.25 src/sys/dev/rasops/rasops4.c 1.19 src/sys/dev/rasops/rasops_putchar_width.h 1.6 works fine. Could you take a look at my problem? The dmesg on my laptop is as follows: 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 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 8.99.51 (DTRACE7) #8: Tue Jul 30 01:23:26 JST 2019 ryoon@brownie:/usr/world/8.99/amd64/obj/sys/arch/amd64/compile/DTRACE7 total memory = 16132 MB avail memory = 15639 MB cpu_rng: RDSEED timecounter: Timecounters tick every 10.000 msec Kernelized RAIDframe activated running cgd selftest aes-xts-256 aes-xts-512 done acpibat* disabled acpibat* already disabled timecounter: Timecounter "i8254" frequency 1193182 Hz quality 100 efi: systbl at pa 2ff15018 HP HP Spectre x360 Convertible 13-ae0xx ( ) mainbus0 (root) ACPI: RSDP 0x2F36 24 (v02 HPQOEM) ACPI: XSDT 0x2F3600C0 FC (v01 HPQOEM SLIC-MPC 01072009 HP 00010013) ACPI: FACP 0x2F39AC60 000114 (v06 HPQOEM SLIC-MPC 01072009 HP 00010013) ACPI: DSDT 0x2F360248 03AA13 (v02 HPQOEM 83B9 01072009 ACPI 20160422) ACPI: FACS 0x2F7EAF00 40 ACPI: APIC 0x2F39AD78 BC (v03 HPQOEM 83B9 01072009 HP 00010013) ACPI: FPDT 0x2F39AE38 44 (v01 HPQOEM 83B9 01072009 HP 00010013) ACPI: FIDT 0x2F39AE80 9C (v01 HPQOEM 83B9 01072009 HP 00010013) ACPI: MCFG 0x2F39AF20 3C (v01 HPQOEM 83B9 01072009 HP 0097) ACPI: SSDT 0x2F39AF60 0003A3 (v01 HPQOEM 83B9 1000 ACPI 20160422) ACPI: SSDT 0x2F39B308 005D97 (v02 HPQOEM 83B9 1000 ACPI 20160422) ACPI: MSDM 0x2F3A10A0 55 (v03 HPQOEM SLIC-MPC 0001 HP 00010013) ACPI: SSDT 0x2F3A10F8 003156 (v02 HPQOEM 83B9 3000 ACPI 20160422) ACPI: HPET 0x2F3A4250 38 (v01 HPQOEM 83B9 0001 HP 005F) ACPI: SSDT 0x2F3A4288 24 (v02 HPQOEM 83B9 ACPI 20160422) ACPI: UEFI 0x2F3A42B0 42 (v01 HPQOEM 83B9 0002 HP 0113) ACPI: SSDT 0x2F3A42F8 0017AE (v02 HPQOEM 83B9 3000 ACPI 20160422) ACPI: LPIT 0x2F3A5AA8 94 (v01 HPQOEM 83B9 HP 005F) ACPI: SSDT 0x2F3A5B40 000141 (v02 HPQOEM 83B9 ACPI 20160422) ACPI: SSDT 0x2F3A5C88 00029F (v02 HPQOEM 83B9 ACPI 20160422) ACPI: SSDT 0x2F3A5F28 0011E7 (v02 HPQOEM 83B9 1000 ACPI 20160422) ACPI: SSDT 0x2F3A7110 00023D (v02 HPQOEM 83B9 ACPI 20160422) ACPI: DBGP 0x2F3A7350 34 (v01 HPQOEM 83B9 0002 HP 005F) ACPI: DBG2 0x2F3A7388 54 (v00 HPQOEM 83B9 0002 HP 005F) ACPI: DMAR 0x2F3A73E0 000114 (v01 HPQOEM 83B9 0001 HP 0001) ACPI: NHLT 0x2F3A74F8 2D (v00 HPQOEM 83B9 0002 HP 0113) ACPI: SSDT 0x2F3A7528 66 (v01 HPQOEM 83B9 0001 ACPI 20160422) ACPI: TPM2 0x2F3A7590 34 (v03 HPQOEM 83B9 0001 HP ) ACPI: ASF! 0x2F3A75C8 A0 (v32 HPQOEM 83B9 0001 HP 000F4240) ACPI: WSMT 0x2F3A7668 28 (v01 HPQOEM 83B9 01072009 HP 00010013) ACPI: BGRT 0x2F3A7690 38 (v01 HPQOEM 83B9 01072009 HP 00010013) ACPI: 11 ACPI AML tables successfully acquired and loaded ioapic0 at mainbus0 apid 2: pa 0xfec0, version 0x20, 120 pins x2APIC available but disabled by DMAR table cpu0 at mainbus0 apid 0 cpu0: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz, id 0x806ea cpu0: package 0, core 0, smt 0 cpu1 at mainbus0 apid 2 cpu1: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz, id 0x806ea cpu1: package 0, core 1, smt 0 cpu2 at mainbus0 apid 4 cpu2: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz, id 0x806ea cpu2: package 0, core 2, smt 0 cpu3 at mainbus0 apid 6 cpu3: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz, id 0x806ea cpu3: package 0, core 3, smt 0 cpu4 at mainbus0 apid 1 cpu4: Intel(R)
Re: CVS commit: src/sys/dev/rasops
On Sun, Jul 28, 2019 at 02:18:14 -0400, Michael wrote: > On Sat, 27 Jul 2019 21:35:04 +0200 > Joerg Sonnenberger wrote: > > > On Fri, Jul 26, 2019 at 07:54:15AM -0300, Jared McNeill wrote: > > > On Fri, 26 Jul 2019, Rin Okuyama wrote: > > > > > > > Also, convert loop of byte-wise copy into memset. > > > > > > I am a bit concerned about this change because IIRC there are platforms > > > where memset etc. cannot be used on device memory. > > > > memset can access memory twice or alignment depending on the platform. > > The question is whether we care about that for the frame buffers though. > > One hazard I can think of is if there's endian-twiddling involved. Some > graphics chip / host bridge combinations will only convert accesses up > to 32bit properly, resulting in 64bit accesses having their upper/lower > 32bit parts swapped on delivery. That should only bite with memcpy() > though. I remember running into something like this with igsfb(4) on krups. -uwe
Re: CVS commit: src/sys/dev/rasops
Hello, On Sat, 27 Jul 2019 21:35:04 +0200 Joerg Sonnenberger wrote: > On Fri, Jul 26, 2019 at 07:54:15AM -0300, Jared McNeill wrote: > > On Fri, 26 Jul 2019, Rin Okuyama wrote: > > > > > Also, convert loop of byte-wise copy into memset. > > > > I am a bit concerned about this change because IIRC there are platforms > > where memset etc. cannot be used on device memory. > > memset can access memory twice or alignment depending on the platform. > The question is whether we care about that for the frame buffers though. One hazard I can think of is if there's endian-twiddling involved. Some graphics chip / host bridge combinations will only convert accesses up to 32bit properly, resulting in 64bit accesses having their upper/lower 32bit parts swapped on delivery. That should only bite with memcpy() though. have fun Michael
Re: CVS commit: src/sys/dev/rasops
This part has already been reverted: http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/rasops/rasops.c#rev1.92 IMO, the fail-safe alternate is preferable over tiny optimization and simplification with possible dangers. Thanks, rin On 2019/07/28 4:35, Joerg Sonnenberger wrote: On Fri, Jul 26, 2019 at 07:54:15AM -0300, Jared McNeill wrote: On Fri, 26 Jul 2019, Rin Okuyama wrote: Also, convert loop of byte-wise copy into memset. I am a bit concerned about this change because IIRC there are platforms where memset etc. cannot be used on device memory. memset can access memory twice or alignment depending on the platform. The question is whether we care about that for the frame buffers though. Joerg
Re: CVS commit: src/sys/dev/rasops
Thank you for pointing it out! I didn't know that. I will revert this part soon. rin On 2019/07/26 19:54, Jared McNeill wrote: On Fri, 26 Jul 2019, Rin Okuyama wrote: Also, convert loop of byte-wise copy into memset. I am a bit concerned about this change because IIRC there are platforms where memset etc. cannot be used on device memory.
Re: CVS commit: src/sys/dev/rasops
On Fri, 26 Jul 2019, Rin Okuyama wrote: Also, convert loop of byte-wise copy into memset. I am a bit concerned about this change because IIRC there are platforms where memset etc. cannot be used on device memory.
Re: CVS commit: src/sys/dev
Hi, O.k. I will commit your fix later. Thank you. Yorick Hardy writes: > Dear Ryo, > > On 2019-07-25, Ryo ONODERA wrote: >> Hi, >> >> Yorick Hardy writes: >> >> > Dear Ryo, >> > >> > On 2019-07-09, Ryo ONODERA wrote: >> >> Module Name: src >> >> Committed By: ryoon >> >> Date: Tue Jul 9 12:52:51 UTC 2019 >> >> >> >> Modified Files: >> >> src/sys/dev/hid: hidms.c hidms.h >> >> src/sys/dev/i2c: ims.c >> >> >> >> Log Message: >> >> Add tpcalib, touch panel calibration to ims(4) >> >> >> >> Suggested by ryo@ at Japan NetBSD Users' Group BOF 2019-07-06. >> >> >> >> >> >> To generate a diff of this commit: >> >> cvs rdiff -u -r1.2 -r1.3 src/sys/dev/hid/hidms.c >> >> cvs rdiff -u -r1.1 -r1.2 src/sys/dev/hid/hidms.h >> >> cvs rdiff -u -r1.1 -r1.2 src/sys/dev/i2c/ims.c >> >> >> >> Please note that diffs are not public domain; they are subject to the >> >> copyright notices on the relevant files. >> > >> > I think this change has the side effect that ums(4) devices which >> > report absolute coordinates seem to stop "moving" because >> > tcpalib_init is never called for these devices. >> > >> > Is the patch below okay? Or, should hidms call tcpalib_init? >> > (I tested the patch with an ACECAD digimemo.) >> >> Sorry for breakage. >> It looks good to me. >> And I feel tpcalib_init should be called from ums.c like your patch. >> Could you commit it? > > I am very sorry, but I retired my commit privilege because I was > contributing very little to NetBSD. Otherwise I would be happy to do so! > >> Thank you. >> >> > -- >> > Kind regards, >> > >> > Yorick Hardy >> > >> > Index: sys/dev/usb/ums.c >> > === >> > RCS file: /cvsroot/src/sys/dev/usb/ums.c,v >> > retrieving revision 1.93 >> > diff -u -r1.93 ums.c >> > --- sys/dev/usb/ums.c 5 May 2019 03:17:54 - 1.93 >> > +++ sys/dev/usb/ums.c 24 Jul 2019 21:29:35 - >> > @@ -192,6 +192,7 @@ >> >} >> >} >> > >> > + tpcalib_init(>sc_ms.sc_tpcalib); >> >hidms_attach(self, >sc_ms, _accessops); >> > } >> > >> >> -- >> Ryo ONODERA // r...@tetera.org >> PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3 > > -- > Kind regards, > > Yorick Hardy -- Ryo ONODERA // r...@tetera.org PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
Re: CVS commit: src/sys/dev
Dear Ryo, On 2019-07-25, Ryo ONODERA wrote: > Hi, > > Yorick Hardy writes: > > > Dear Ryo, > > > > On 2019-07-09, Ryo ONODERA wrote: > >> Module Name: src > >> Committed By: ryoon > >> Date: Tue Jul 9 12:52:51 UTC 2019 > >> > >> Modified Files: > >>src/sys/dev/hid: hidms.c hidms.h > >>src/sys/dev/i2c: ims.c > >> > >> Log Message: > >> Add tpcalib, touch panel calibration to ims(4) > >> > >> Suggested by ryo@ at Japan NetBSD Users' Group BOF 2019-07-06. > >> > >> > >> To generate a diff of this commit: > >> cvs rdiff -u -r1.2 -r1.3 src/sys/dev/hid/hidms.c > >> cvs rdiff -u -r1.1 -r1.2 src/sys/dev/hid/hidms.h > >> cvs rdiff -u -r1.1 -r1.2 src/sys/dev/i2c/ims.c > >> > >> Please note that diffs are not public domain; they are subject to the > >> copyright notices on the relevant files. > > > > I think this change has the side effect that ums(4) devices which > > report absolute coordinates seem to stop "moving" because > > tcpalib_init is never called for these devices. > > > > Is the patch below okay? Or, should hidms call tcpalib_init? > > (I tested the patch with an ACECAD digimemo.) > > Sorry for breakage. > It looks good to me. > And I feel tpcalib_init should be called from ums.c like your patch. > Could you commit it? I am very sorry, but I retired my commit privilege because I was contributing very little to NetBSD. Otherwise I would be happy to do so! > Thank you. > > > -- > > Kind regards, > > > > Yorick Hardy > > > > Index: sys/dev/usb/ums.c > > === > > RCS file: /cvsroot/src/sys/dev/usb/ums.c,v > > retrieving revision 1.93 > > diff -u -r1.93 ums.c > > --- sys/dev/usb/ums.c 5 May 2019 03:17:54 - 1.93 > > +++ sys/dev/usb/ums.c 24 Jul 2019 21:29:35 - > > @@ -192,6 +192,7 @@ > > } > > } > > > > + tpcalib_init(>sc_ms.sc_tpcalib); > > hidms_attach(self, >sc_ms, _accessops); > > } > > > > -- > Ryo ONODERA // r...@tetera.org > PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3 -- Kind regards, Yorick Hardy
Re: CVS commit: src/sys/dev
Hi, Yorick Hardy writes: > Dear Ryo, > > On 2019-07-09, Ryo ONODERA wrote: >> Module Name: src >> Committed By:ryoon >> Date:Tue Jul 9 12:52:51 UTC 2019 >> >> Modified Files: >> src/sys/dev/hid: hidms.c hidms.h >> src/sys/dev/i2c: ims.c >> >> Log Message: >> Add tpcalib, touch panel calibration to ims(4) >> >> Suggested by ryo@ at Japan NetBSD Users' Group BOF 2019-07-06. >> >> >> To generate a diff of this commit: >> cvs rdiff -u -r1.2 -r1.3 src/sys/dev/hid/hidms.c >> cvs rdiff -u -r1.1 -r1.2 src/sys/dev/hid/hidms.h >> cvs rdiff -u -r1.1 -r1.2 src/sys/dev/i2c/ims.c >> >> Please note that diffs are not public domain; they are subject to the >> copyright notices on the relevant files. > > I think this change has the side effect that ums(4) devices which > report absolute coordinates seem to stop "moving" because > tcpalib_init is never called for these devices. > > Is the patch below okay? Or, should hidms call tcpalib_init? > (I tested the patch with an ACECAD digimemo.) Sorry for breakage. It looks good to me. And I feel tpcalib_init should be called from ums.c like your patch. Could you commit it? Thank you. > -- > Kind regards, > > Yorick Hardy > > Index: sys/dev/usb/ums.c > === > RCS file: /cvsroot/src/sys/dev/usb/ums.c,v > retrieving revision 1.93 > diff -u -r1.93 ums.c > --- sys/dev/usb/ums.c 5 May 2019 03:17:54 - 1.93 > +++ sys/dev/usb/ums.c 24 Jul 2019 21:29:35 - > @@ -192,6 +192,7 @@ > } > } > > + tpcalib_init(>sc_ms.sc_tpcalib); > hidms_attach(self, >sc_ms, _accessops); > } > -- Ryo ONODERA // r...@tetera.org PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
Re: CVS commit: src/sys/dev
Dear Ryo, On 2019-07-09, Ryo ONODERA wrote: > Module Name: src > Committed By: ryoon > Date: Tue Jul 9 12:52:51 UTC 2019 > > Modified Files: > src/sys/dev/hid: hidms.c hidms.h > src/sys/dev/i2c: ims.c > > Log Message: > Add tpcalib, touch panel calibration to ims(4) > > Suggested by ryo@ at Japan NetBSD Users' Group BOF 2019-07-06. > > > To generate a diff of this commit: > cvs rdiff -u -r1.2 -r1.3 src/sys/dev/hid/hidms.c > cvs rdiff -u -r1.1 -r1.2 src/sys/dev/hid/hidms.h > cvs rdiff -u -r1.1 -r1.2 src/sys/dev/i2c/ims.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. I think this change has the side effect that ums(4) devices which report absolute coordinates seem to stop "moving" because tcpalib_init is never called for these devices. Is the patch below okay? Or, should hidms call tcpalib_init? (I tested the patch with an ACECAD digimemo.) -- Kind regards, Yorick Hardy Index: sys/dev/usb/ums.c === RCS file: /cvsroot/src/sys/dev/usb/ums.c,v retrieving revision 1.93 diff -u -r1.93 ums.c --- sys/dev/usb/ums.c 5 May 2019 03:17:54 - 1.93 +++ sys/dev/usb/ums.c 24 Jul 2019 21:29:35 - @@ -192,6 +192,7 @@ } } + tpcalib_init(>sc_ms.sc_tpcalib); hidms_attach(self, >sc_ms, _accessops); }
Re: CVS commit: src/sys/dev/sdmmc
On 24/07/2019 06:45, SAITOH Masanobu wrote: [snip] @@ -27,7 +27,7 @@ /* CMD52 arguments */ #define SD_ARG_CMD52_READ (0<<31) -#define SD_ARG_CMD52_WRITE (1<<31) +#define SD_ARG_CMD52_WRITE (1UL<<31) #define SD_ARG_CMD52_FUNC_SHIFT 28 #define SD_ARG_CMD52_FUNC_MASK0x7 #define SD_ARG_CMD52_EXCHANGE (1<<27) mmm __BIT(3)
Re: CVS commit: src/sys
On Tue, Jul 23, 2019 at 10:13:38AM -0700, Jason Thorpe wrote: > > On Jul 23, 2019, at 10:07 AM, Jason Thorpe wrote: > > > > > > > >> On Jul 23, 2019, at 9:14 AM, Rin Okuyama wrote: > >> > >> Ah, you are right. We leaks uninitialized memory via mmap. > >> > >> However, I'm not sure that it is safe to write DMA buffer > >> above sc->sc_vramsize after bus_dmamap_load? > >> > >> Thoughts, ARM experts? > > Also, didn't Jared make a change to the Allwinner u-boot configs that ensure > the frame buffer is page-aligned? Should we make that change everywhere? I might be mis-quoting Jared, but he said something like: For u-boot, a page is 4K. For us, a page is 8K.
Re: CVS commit: src/sys
> On Jul 23, 2019, at 10:07 AM, Jason Thorpe wrote: > > > >> On Jul 23, 2019, at 9:14 AM, Rin Okuyama wrote: >> >> Ah, you are right. We leaks uninitialized memory via mmap. >> >> However, I'm not sure that it is safe to write DMA buffer >> above sc->sc_vramsize after bus_dmamap_load? >> >> Thoughts, ARM experts? Also, didn't Jared make a change to the Allwinner u-boot configs that ensure the frame buffer is page-aligned? Should we make that change everywhere? > > Since fundamental memory allocation is done on page boundaries, would it > suffice to simply ensure that the "leaked" memory is zero'd out first? I > mean, nothing else can use it for anything meaninful... > > -- thorpej > -- thorpej
Re: CVS commit: src/sys
> On Jul 23, 2019, at 9:14 AM, Rin Okuyama wrote: > > Ah, you are right. We leaks uninitialized memory via mmap. > > However, I'm not sure that it is safe to write DMA buffer > above sc->sc_vramsize after bus_dmamap_load? > > Thoughts, ARM experts? Since fundamental memory allocation is done on page boundaries, would it suffice to simply ensure that the "leaked" memory is zero'd out first? I mean, nothing else can use it for anything meaninful... -- thorpej
Re: CVS commit: src/sys
Ah, you are right. We leaks uninitialized memory via mmap.
However, I'm not sure that it is safe to write DMA buffer
above sc->sc_vramsize after bus_dmamap_load?
Thoughts, ARM experts?
Thanks,
rin
On 2019/07/24 0:22, m...@netbsd.org wrote:
I think we might be leaking uninitialized kernel memory.
in tifb,
if (bus_dmamem_alloc(sc->sc_dmat, sc->sc_vramsize, 0, 0,
sc->sc_dmamem, 1, , BUS_DMA_NOWAIT) != 0) {
bus_dma* rounds up to PAGE_SIZE chunks.
memset((void *)sc->sc_vramaddr, 0, sc->sc_vramsize);
We zero the not-rounded-up size.
What do you think?
On Tue, Jul 23, 2019 at 02:34:12PM +, Rin Okuyama wrote:
Module Name:src
Committed By: rin
Date: Tue Jul 23 14:34:12 UTC 2019
Modified Files:
src/sys/arch/arm/omap: tifb.c
src/sys/arch/luna68k/dev: lunafb.c
src/sys/dev/fdt: simplefb.c
Log Message:
For drivers whose framebuffer is located not page-aligned, permit
offset of mmap up to (length of framebuffer) + (page offset of base
address of framebuffer). This is necessary in order to map the
highest page of framebuffer correctly, see,
http://cvsweb.netbsd.org/bsdweb.cgi/xsrc/external/mit/xf86-video-wsfb/dist/src/wsfb_driver.c#rev1.35
Re: CVS commit: src/sys
I think we might be leaking uninitialized kernel memory.
in tifb,
if (bus_dmamem_alloc(sc->sc_dmat, sc->sc_vramsize, 0, 0,
sc->sc_dmamem, 1, , BUS_DMA_NOWAIT) != 0) {
bus_dma* rounds up to PAGE_SIZE chunks.
memset((void *)sc->sc_vramaddr, 0, sc->sc_vramsize);
We zero the not-rounded-up size.
What do you think?
On Tue, Jul 23, 2019 at 02:34:12PM +, Rin Okuyama wrote:
> Module Name: src
> Committed By: rin
> Date: Tue Jul 23 14:34:12 UTC 2019
>
> Modified Files:
> src/sys/arch/arm/omap: tifb.c
> src/sys/arch/luna68k/dev: lunafb.c
> src/sys/dev/fdt: simplefb.c
>
> Log Message:
> For drivers whose framebuffer is located not page-aligned, permit
> offset of mmap up to (length of framebuffer) + (page offset of base
> address of framebuffer). This is necessary in order to map the
> highest page of framebuffer correctly, see,
> http://cvsweb.netbsd.org/bsdweb.cgi/xsrc/external/mit/xf86-video-wsfb/dist/src/wsfb_driver.c#rev1.35
Re: CVS commit: src/sys/dev/usb
Can you add printfs in these two functions to dump 'bLength'?
I've reverted the change for now. I found these bugs a week ago while
manually crafting incorrect USB packets in Qemu's USB driver. It caused
memory corruptions in the NetBSD guest, which were detected by KASAN.
Fixing the length checks eliminated the corruptions, while still
allowing correctly formed USB devices to attach.
Le 06/07/2019 à 09:54, Thomas Klausner a écrit :
It could be a coincidence, but with yesterday's kernel my
non-malicious USB keyboard (Cherry G230) worked and today it doesn't.
-uhidev0 at uhub5 port 1 configuration 1 interface 0
-uhidev0: vendor 046a (0x46a) product 0023 (0x23), rev 2.00/2.20, addr 1,
iclass 3/1
-ukbd0 at uhidev0: 8 Variable keys, 6 Array codes
-wskbd0 at ukbd0: console keyboard
-uhidev1 at uhub5 port 1 configuration 1 interface 1
-uhidev1: vendor 046a (0x46a) product 0023 (0x23), rev 2.00/2.20, addr 1,
iclass 3/0
-uhidev1: 2 report ids
-uhid0 at uhidev1 reportid 1: input=2, output=0, feature=0
-uhid1 at uhidev1 reportid 2: input=1, output=0, feature=0
+uhub5: port 1, set config at addr 1 failed
+uhub5: autoconfiguration error: device problem, disabling port 1
Thomas
On Sat, Jul 06, 2019 at 05:05:54AM +, Maxime Villard wrote:
Module Name:src
Committed By: maxv
Date: Sat Jul 6 05:05:53 UTC 2019
Modified Files:
src/sys/dev/usb: usb_subr.c
Log Message:
Fix two length checks, otherwise a malicious USB key plugged in the
system could trigger overflows, seen with KASAN.
To generate a diff of this commit:
cvs rdiff -u -r1.230 -r1.231 src/sys/dev/usb/usb_subr.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/dev/usb/usb_subr.c
diff -u src/sys/dev/usb/usb_subr.c:1.230 src/sys/dev/usb/usb_subr.c:1.231
--- src/sys/dev/usb/usb_subr.c:1.230Tue Feb 12 14:17:44 2019
+++ src/sys/dev/usb/usb_subr.c Sat Jul 6 05:05:53 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: usb_subr.c,v 1.230 2019/02/12 14:17:44 rin Exp $ */
+/* $NetBSD: usb_subr.c,v 1.231 2019/07/06 05:05:53 maxv Exp $ */
/*$FreeBSD: src/sys/dev/usb/usb_subr.c,v 1.18 1999/11/17 22:33:47 n_hibma
Exp $ */
/*
@@ -32,7 +32,7 @@
*/
#include
-__KERNEL_RCSID(0, "$NetBSD: usb_subr.c,v 1.230 2019/02/12 14:17:44 rin Exp $");
+__KERNEL_RCSID(0, "$NetBSD: usb_subr.c,v 1.231 2019/07/06 05:05:53 maxv Exp
$");
#ifdef _KERNEL_OPT
#include "opt_compat_netbsd.h"
@@ -366,8 +366,8 @@ usbd_find_idesc(usb_config_descriptor_t
altidx, curaidx);
DPRINTFN(4, "len=%jd type=%jd", d->bLength, d->bDescriptorType,
0, 0);
- if (d->bLength == 0) /* bad descriptor */
- break;
+ if (d->bLength < USB_INTERFACE_DESCRIPTOR_SIZE)
+ break; /* bad descriptor */
p += d->bLength;
if (p <= end && d->bDescriptorType == UDESC_INTERFACE) {
if (d->bInterfaceNumber != lastidx) {
@@ -402,8 +402,8 @@ usbd_find_edesc(usb_config_descriptor_t
curidx = -1;
for (p = (char *)d + d->bLength; p < end; ) {
e = (usb_endpoint_descriptor_t *)p;
- if (e->bLength == 0) /* bad descriptor */
- break;
+ if (e->bLength < USB_ENDPOINT_DESCRIPTOR_SIZE)
+ break; /* bad descriptor */
p += e->bLength;
if (p <= end && e->bDescriptorType == UDESC_INTERFACE)
return NULL;
Re: CVS commit: src/sys/dev/usb
On Fri, Jun 28, 2019 at 1:57 AM matthew green wrote: > > Module Name:src > Committed By: mrg > Date: Fri Jun 28 01:57:43 UTC 2019 > > Modified Files: > src/sys/dev/usb: if_axen.c if_cdce.c if_ure.c > > Log Message: > more smp cleanup for ure(4)/axen(4)/cdce(4): Thank you for working! > - convert IFF_ALLMULTI to ETHER_F_ALLMULTI, using ETHER_LOCK() > - remove IFF_OACTIVE use, and simply check the ring count in start cdce_start_locked() has the last one. > - assert/take more locks > - XXX: IFF_RUNNING is not properly protected (all driver problem) > - fix axen_timer setting so it actually runs > - document a locking issue in stop callback: > stop is called with the softc lock held, but the lock order > in all other places is ifnet -> softc -> rx -> tx, so taking > ifnet lock when softc lock is held would be problematic ure_init_locked() calls ure_stop() with sc->ure_lock held, so it would cause 'locking against myself' perhaps when ifconfig such as IFF_DEBUG while the interface is up. > - in rxeof check for stopping/dying more often. i managed to > trigger a pagefault in cdce_rxeof() when yanking an active > device as it attempted to usbd_setup_xfer() on closed pipes. > - add missing USBD_MPSAFE and cdce_stopping resetting for cdce(4) > > between this and other recent clean ups increase performance of > these drivers mostly. some numbers (in mbit/sec): > > old:new: > driver in out in+out in out in+out > -- --- -- -- --- -- > cdce39 32 44 38 33 54 > axen44 34 45 48 37 42 > ure 36 34 35 36 38 38 > > i'm not sure why axen drops a little with in+out. cdce is > helped quite a lot, and ure a little. it is disappointing that > ure does not outperform cdce -- it's the same actual hardware, > and the device-specific (ure) should outperform the generic > cdce driver... > > > To generate a diff of this commit: > cvs rdiff -u -r1.48 -r1.49 src/sys/dev/usb/if_axen.c > cvs rdiff -u -r1.50 -r1.51 src/sys/dev/usb/if_cdce.c > cvs rdiff -u -r1.12 -r1.13 src/sys/dev/usb/if_ure.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/dev/usb
Mmh no I see, the min descriptor length check we should add is 3 bytes, and my
check should be moved below in the idesc branch. I'll re-fix that next week.
Le 06/07/2019 à 10:04, Maxime Villard a écrit :
Can you add printfs in these two functions to dump 'bLength'?
I've reverted the change for now. I found these bugs a week ago while
manually crafting incorrect USB packets in Qemu's USB driver. It caused
memory corruptions in the NetBSD guest, which were detected by KASAN.
Fixing the length checks eliminated the corruptions, while still
allowing correctly formed USB devices to attach.
Le 06/07/2019 à 09:54, Thomas Klausner a écrit :
It could be a coincidence, but with yesterday's kernel my
non-malicious USB keyboard (Cherry G230) worked and today it doesn't.
-uhidev0 at uhub5 port 1 configuration 1 interface 0
-uhidev0: vendor 046a (0x46a) product 0023 (0x23), rev 2.00/2.20, addr 1,
iclass 3/1
-ukbd0 at uhidev0: 8 Variable keys, 6 Array codes
-wskbd0 at ukbd0: console keyboard
-uhidev1 at uhub5 port 1 configuration 1 interface 1
-uhidev1: vendor 046a (0x46a) product 0023 (0x23), rev 2.00/2.20, addr 1,
iclass 3/0
-uhidev1: 2 report ids
-uhid0 at uhidev1 reportid 1: input=2, output=0, feature=0
-uhid1 at uhidev1 reportid 2: input=1, output=0, feature=0
+uhub5: port 1, set config at addr 1 failed
+uhub5: autoconfiguration error: device problem, disabling port 1
Thomas
On Sat, Jul 06, 2019 at 05:05:54AM +, Maxime Villard wrote:
Module Name: src
Committed By: maxv
Date: Sat Jul 6 05:05:53 UTC 2019
Modified Files:
src/sys/dev/usb: usb_subr.c
Log Message:
Fix two length checks, otherwise a malicious USB key plugged in the
system could trigger overflows, seen with KASAN.
To generate a diff of this commit:
cvs rdiff -u -r1.230 -r1.231 src/sys/dev/usb/usb_subr.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/dev/usb/usb_subr.c
diff -u src/sys/dev/usb/usb_subr.c:1.230 src/sys/dev/usb/usb_subr.c:1.231
--- src/sys/dev/usb/usb_subr.c:1.230 Tue Feb 12 14:17:44 2019
+++ src/sys/dev/usb/usb_subr.c Sat Jul 6 05:05:53 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: usb_subr.c,v 1.230 2019/02/12 14:17:44 rin Exp $ */
+/* $NetBSD: usb_subr.c,v 1.231 2019/07/06 05:05:53 maxv Exp $ */
/* $FreeBSD: src/sys/dev/usb/usb_subr.c,v 1.18 1999/11/17 22:33:47 n_hibma
Exp $ */
/*
@@ -32,7 +32,7 @@
*/
#include
-__KERNEL_RCSID(0, "$NetBSD: usb_subr.c,v 1.230 2019/02/12 14:17:44 rin Exp $");
+__KERNEL_RCSID(0, "$NetBSD: usb_subr.c,v 1.231 2019/07/06 05:05:53 maxv Exp
$");
#ifdef _KERNEL_OPT
#include "opt_compat_netbsd.h"
@@ -366,8 +366,8 @@ usbd_find_idesc(usb_config_descriptor_t
altidx, curaidx);
DPRINTFN(4, "len=%jd type=%jd", d->bLength, d->bDescriptorType,
0, 0);
- if (d->bLength == 0) /* bad descriptor */
- break;
+ if (d->bLength < USB_INTERFACE_DESCRIPTOR_SIZE)
+ break; /* bad descriptor */
p += d->bLength;
if (p <= end && d->bDescriptorType == UDESC_INTERFACE) {
if (d->bInterfaceNumber != lastidx) {
@@ -402,8 +402,8 @@ usbd_find_edesc(usb_config_descriptor_t
curidx = -1;
for (p = (char *)d + d->bLength; p < end; ) {
e = (usb_endpoint_descriptor_t *)p;
- if (e->bLength == 0) /* bad descriptor */
- break;
+ if (e->bLength < USB_ENDPOINT_DESCRIPTOR_SIZE)
+ break; /* bad descriptor */
p += e->bLength;
if (p <= end && e->bDescriptorType == UDESC_INTERFACE)
return NULL;
Re: CVS commit: src/sys/dev/usb
It could be a coincidence, but with yesterday's kernel my
non-malicious USB keyboard (Cherry G230) worked and today it doesn't.
-uhidev0 at uhub5 port 1 configuration 1 interface 0
-uhidev0: vendor 046a (0x46a) product 0023 (0x23), rev 2.00/2.20, addr 1,
iclass 3/1
-ukbd0 at uhidev0: 8 Variable keys, 6 Array codes
-wskbd0 at ukbd0: console keyboard
-uhidev1 at uhub5 port 1 configuration 1 interface 1
-uhidev1: vendor 046a (0x46a) product 0023 (0x23), rev 2.00/2.20, addr 1,
iclass 3/0
-uhidev1: 2 report ids
-uhid0 at uhidev1 reportid 1: input=2, output=0, feature=0
-uhid1 at uhidev1 reportid 2: input=1, output=0, feature=0
+uhub5: port 1, set config at addr 1 failed
+uhub5: autoconfiguration error: device problem, disabling port 1
Thomas
On Sat, Jul 06, 2019 at 05:05:54AM +, Maxime Villard wrote:
> Module Name: src
> Committed By: maxv
> Date: Sat Jul 6 05:05:53 UTC 2019
>
> Modified Files:
> src/sys/dev/usb: usb_subr.c
>
> Log Message:
> Fix two length checks, otherwise a malicious USB key plugged in the
> system could trigger overflows, seen with KASAN.
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.230 -r1.231 src/sys/dev/usb/usb_subr.c
>
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
>
> Modified files:
>
> Index: src/sys/dev/usb/usb_subr.c
> diff -u src/sys/dev/usb/usb_subr.c:1.230 src/sys/dev/usb/usb_subr.c:1.231
> --- src/sys/dev/usb/usb_subr.c:1.230 Tue Feb 12 14:17:44 2019
> +++ src/sys/dev/usb/usb_subr.cSat Jul 6 05:05:53 2019
> @@ -1,4 +1,4 @@
> -/* $NetBSD: usb_subr.c,v 1.230 2019/02/12 14:17:44 rin Exp $ */
> +/* $NetBSD: usb_subr.c,v 1.231 2019/07/06 05:05:53 maxv Exp $ */
> /* $FreeBSD: src/sys/dev/usb/usb_subr.c,v 1.18 1999/11/17 22:33:47 n_hibma
> Exp $ */
>
> /*
> @@ -32,7 +32,7 @@
> */
>
> #include
> -__KERNEL_RCSID(0, "$NetBSD: usb_subr.c,v 1.230 2019/02/12 14:17:44 rin Exp
> $");
> +__KERNEL_RCSID(0, "$NetBSD: usb_subr.c,v 1.231 2019/07/06 05:05:53 maxv Exp
> $");
>
> #ifdef _KERNEL_OPT
> #include "opt_compat_netbsd.h"
> @@ -366,8 +366,8 @@ usbd_find_idesc(usb_config_descriptor_t
> altidx, curaidx);
> DPRINTFN(4, "len=%jd type=%jd", d->bLength, d->bDescriptorType,
> 0, 0);
> - if (d->bLength == 0) /* bad descriptor */
> - break;
> + if (d->bLength < USB_INTERFACE_DESCRIPTOR_SIZE)
> + break; /* bad descriptor */
> p += d->bLength;
> if (p <= end && d->bDescriptorType == UDESC_INTERFACE) {
> if (d->bInterfaceNumber != lastidx) {
> @@ -402,8 +402,8 @@ usbd_find_edesc(usb_config_descriptor_t
> curidx = -1;
> for (p = (char *)d + d->bLength; p < end; ) {
> e = (usb_endpoint_descriptor_t *)p;
> - if (e->bLength == 0) /* bad descriptor */
> - break;
> + if (e->bLength < USB_ENDPOINT_DESCRIPTOR_SIZE)
> + break; /* bad descriptor */
> p += e->bLength;
> if (p <= end && e->bDescriptorType == UDESC_INTERFACE)
> return NULL;
>
Re: CVS commit: src/sys/external/bsd/drm2/nouveau
On Fri, Jul 05, 2019 at 10:25:50PM +0200, Thomas Klausner wrote: > On Wed, Jul 03, 2019 at 10:56:09PM +, m...@netbsd.org wrote: > > On Wed, Jul 03, 2019 at 08:47:22PM +, Thomas Klausner wrote: > > > Module Name: src > > > Committed By: wiz > > > Date: Wed Jul 3 20:47:22 UTC 2019 > > > > > > Modified Files: > > > src/sys/external/bsd/drm2/nouveau: nouveau_pci.c > > > > > > Log Message: > > > Improve nouveau pci attachment code so it waits for the availability of / > > > before trying to load firmware. > > > > > > > Which firmware is it loading? we should probably ship it... > > I've tried to provide the nvidia firmware from Debian's > firmware-nonfree_20161130.orig.tar.xz > > Putting the files into > > /usr/libdata/firmware/nvidia/gm206/gr > > or > > /usr/libdata/firmware/nouveau/gm206/gr > > isn't enough, it still doesn't find them. Is the full searched path printed > somewhere? > Thomas nouveau code doesn't make it super obvious, but try /usr/libdata/firmware/nouveau/nvidia/gm206/gr/ If that fails, printf drvname and imgname in sys/dev/firmload.c:firmware_open This is our usual source for firmware, https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/
Re: CVS commit: src/sys/external/bsd/drm2/nouveau
On Wed, Jul 03, 2019 at 08:47:22PM +, Thomas Klausner wrote: > Module Name: src > Committed By: wiz > Date: Wed Jul 3 20:47:22 UTC 2019 > > Modified Files: > src/sys/external/bsd/drm2/nouveau: nouveau_pci.c > > Log Message: > Improve nouveau pci attachment code so it waits for the availability of / > before trying to load firmware. > Which firmware is it loading? we should probably ship it...
Re: CVS commit: src/sys/dev/ic
Awesome, Jared - many thanks for this! On Fri, 28 Jun 2019 at 08:08, Jared D. McNeill wrote: > Module Name:src > Committed By: jmcneill > Date: Fri Jun 28 15:08:47 UTC 2019 > > Modified Files: > src/sys/dev/ic: nvme.c nvmevar.h > > Log Message: > Fix a performance issue where one busy queue can starve all other queues. > > In normal operations with multiple queues, the nvme driver will attempt > to schedule I/O requests on the submitting CPU. This breaks down when any > one of the queues becomes full; the driver returns EAGAIN to the disk > layer, which causes the disk layer to stop submitting more requests until > the blocked request is consumed. When space becomes available in the full > queue, it pulls the next buffer from the bufq and fills the queue again, > until finally hitting EAGAIN and preventing other queues from processing > requests. > > Two changes here to fix the problem: > > - When processing requests from the bufq, attempt to assign them to the >queue associated with the CPU that originated the request. > - If that queue is busy, try to find another queue with available space >before returning EAGAIN. This way, only when all queues are full will >the disk layer stop submitting more requests. > > Now for some real numbers. On a Rockchip RK3399 board (6 CPUs), with 6 > concurrent readers: > > Old code: > 4294967296 bytes transferred in 52.420 secs (81933752 bytes/sec) > 4294967296 bytes transferred in 53.969 secs (79582117 bytes/sec) > 4294967296 bytes transferred in 55.391 secs (77539082 bytes/sec) > 4294967296 bytes transferred in 55.649 secs (77179595 bytes/sec) > 4294967296 bytes transferred in 56.102 secs (76556402 bytes/sec) > 4294967296 bytes transferred in 72.901 secs (58915066 bytes/sec) > > New code: > 4294967296 bytes transferred in 37.171 secs (115546186 bytes/sec) > 4294967296 bytes transferred in 37.611 secs (114194445 bytes/sec) > 4294967296 bytes transferred in 37.655 secs (114061009 bytes/sec) > 4294967296 bytes transferred in 38.247 secs (112295534 bytes/sec) > 4294967296 bytes transferred in 38.496 secs (111569183 bytes/sec) > 4294967296 bytes transferred in 38.595 secs (111282997 bytes/sec) > > > To generate a diff of this commit: > cvs rdiff -u -r1.43 -r1.44 src/sys/dev/ic/nvme.c > cvs rdiff -u -r1.19 -r1.20 src/sys/dev/ic/nvmevar.h > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. > >
Re: CVS commit: src/sys/kern
> On Jun 29, 2019, at 2:29 AM, Maxime Villard wrote: [...] > Ah because now I should teach people C programming? No, but you should be helpful and assume that the people working on this project are not trying to annoy you or ignore you. If would have been better to explain what the bugs are, instead of sending a "there are more bugs" message. This saves everyone time. You could have also sent me a patch and asking if this is right and optionally ask for permission to commit it. This is typically how people collaborate. Perhaps the reason you are not forgiving to other peoples' mistakes is because you make much fewer of them, but that does not give you the right to mistreat others. Even the simple sentence you typed above has a hostility tone in it, and will make whoever answers you be on the defensive (or more commonly do nothing to avoid conflict as most people do). > You don't even understand what happened. What happened is that, one more time, > for the 10^n-th time, Christos demonstrated complete carelessness, to the > point he did not even spend one second of brain time trying to understand the > issues I pointed him to in the very first mail of this thread. You are making assumptions about someone else's understanding of the situation. How would you know that they don't understand? I think that the issue is that your are putting yourself in their position and then judging their behavior based on how you'd behave in that situation. This results to "your" truth, but not to "their" truth. In this case, perhaps I could have spent more time, found the bug and not introduced a new one. But why would I intentionally make things worse? Wouldn't I have to fix them again? I tried to fix things to the best of my abilities and time and failed. Should I be insulted for it? Let's just remember how it all started. I was not even thinking about touching any code that day. Someone wanted an explanation about how this function worked and I went I wrote a comment about it. The rest is all history. > On the contrary, > he tried to dismiss them, make a joke of them, and then proceeded to add even > more bugs than there already were, not paying a single attention to what he > was doing. It's not like it was some kind of highly complicated subsystem to > deal with, either. I tried to make a joke because you seem to decide to change other people's code without asking. You don't like debugging code, you remove it; there is no discussion with the author (and this is not the first time). This is not how people work together. I also made the joke because I did not see the bug, and removing debugging lines is usually not helpful to find a bug. Also since you changed the code, you could have fixed it; why didn't you? > These would seem like entirely acceptable mistakes (part of being human, etc), > if Christos didn't already have a very long history of committing this kind of > absolute nonsense in the kernel. This level of systematic carelessness simply > is not acceptable, and that is why I got really upset. The rest is off-list. Yes, and I take full responsibility for them and I try to do better. I doubt that I will ever be perfect. I find and fix other people's bugs or the time but I don't get upset with them or insult them. In fact I used to be snarky, mentioning in the commit message the person who introduced the bug as a joke (hi $author), but then I realized it was not funny and created a toxic environment so I stopped doing it. If you did not like what I was doing and you believe that I am harming the project with my careless behavior, you should have (and you probably finally did) complained to board instead. christos
Re: CVS commit: src/sys/kern
Le 29/06/2019 à 02:12, Hisashi T Fujinaka a écrit : On Thu, 27 Jun 2019, Maxime Villard wrote: Le 27/06/2019 ? 20:56, Christos Zoulas a ?crit : On Jun 27, 8:30pm, m...@m00nbsd.net (Maxime Villard) wrote: -- Subject: Re: CVS commit: src/sys/kern | Le 27/06/2019 ? 19:07, Christos Zoulas a ??crit? : | > Module Name: src | > Committed By: christos | > Date: Thu Jun 27 17:07:51 UTC 2019 | > | > Modified Files: | > src/sys/kern: kern_exec.c | > | > Log Message: | > Return an error if the path was too long. Pointed out by maxv | > | > | > To generate a diff of this commit: | > cvs rdiff -u -r1.474 -r1.475 src/sys/kern/kern_exec.c | > | > Please note that diffs are not public domain; they are subject to the | > copyright notices on the relevant files. | | Did you just introduce an even bigger bug? I don't know, is this the most efficient way to communicate it if I did? This got to be a fucking joke. I find you to be rude and boorish. If you're so smart, why can't you help people with the problems instead of insulting them? Ah because now I should teach people C programming? You don't even understand what happened. What happened is that, one more time, for the 10^n-th time, Christos demonstrated complete carelessness, to the point he did not even spend one second of brain time trying to understand the issues I pointed him to in the very first mail of this thread. On the contrary, he tried to dismiss them, make a joke of them, and then proceeded to add even more bugs than there already were, not paying a single attention to what he was doing. It's not like it was some kind of highly complicated subsystem to deal with, either. These would seem like entirely acceptable mistakes (part of being human, etc), if Christos didn't already have a very long history of committing this kind of absolute nonsense in the kernel. This level of systematic carelessness simply is not acceptable, and that is why I got really upset. The rest is off-list.
Re: CVS commit: src/sys/kern
On Thu, 27 Jun 2019, Maxime Villard wrote: Le 27/06/2019 ? 20:56, Christos Zoulas a ?crit : On Jun 27, 8:30pm, m...@m00nbsd.net (Maxime Villard) wrote: -- Subject: Re: CVS commit: src/sys/kern | Le 27/06/2019 ? 19:07, Christos Zoulas a ??crit? : | > Module Name: src | > Committed By:christos | > Date:Thu Jun 27 17:07:51 UTC 2019 | > | > Modified Files: | > src/sys/kern: kern_exec.c | > | > Log Message: | > Return an error if the path was too long. Pointed out by maxv | > | > | > To generate a diff of this commit: | > cvs rdiff -u -r1.474 -r1.475 src/sys/kern/kern_exec.c | > | > Please note that diffs are not public domain; they are subject to the | > copyright notices on the relevant files. | | Did you just introduce an even bigger bug? I don't know, is this the most efficient way to communicate it if I did? This got to be a fucking joke. I find you to be rude and boorish. If you're so smart, why can't you help people with the problems instead of insulting them? -- Hisashi T Fujinaka - ht...@twofifty.com BSEE + BSChem + BAEnglish + MSCS + $2.50 = coffee
Re: CVS commit: src/sys/arch/amd64/amd64
In article , Maxime Villard wrote: > >This isn't correct, with USER_LDT the 32bit LWPs may have non-default segregs, >besides it is really dumb to mix 32 and 64bit code, part of the reasons why >I dropped the thing Yes, it is still missing the check that the compat_netbsd32 function had. Before you disabled the code it was possible to debug a 32 bit process with a 64 bit debugger. This is still useful because trying to debug a 32 bit process with a 32 bit debugger on a 64 system is extremely difficult to get it right because the 32 bit debugger needs to know somehow that it is running on a 64 bit system in order to mangle the paths properly and load the appropriate shared libraries. I think that the choice if we are going to let this work or not does not belong to the opinion of a single person, but to the developer base of NetBSD or the core group. christos
Re: CVS commit: src/sys/compat/sys
On Fri, Jun 28, 2019 at 05:03:37AM -0700, Jason Thorpe wrote: > > > On Jun 26, 2019, at 7:10 PM, matthew green wrote: > > > >> Always include the 32 bit structure and definitions on _LP64 regardless > >> of compat32 being on or off, because we want the headers to work when > >> compiling modular kernels. Of course the 32 bit structs do not make sense > >> on platforms that don't have 32 bit modes (alpha), but we don't have > >> a define for that and it does not hurt. > > > > i've been using _LP64 && !__alpha__ for this when it strikes. > > > > sub-optimal, but also easy to grep and find :-) > > Perhaps we should define "_LP64_ONLY" in for this type of > situation? > I'm a really huge fan of keeping structs the same across archs when it doesn't cost us very much. It's been a real blessing that netbsd is mostly consistent when porting programming languages, which often end up embedding a list of structs and their sizes, generated by very fragile code (or by hand!)
Re: CVS commit: src/sys/compat/sys
> On Jun 26, 2019, at 7:10 PM, matthew green wrote: > >> Always include the 32 bit structure and definitions on _LP64 regardless >> of compat32 being on or off, because we want the headers to work when >> compiling modular kernels. Of course the 32 bit structs do not make sense >> on platforms that don't have 32 bit modes (alpha), but we don't have >> a define for that and it does not hurt. > > i've been using _LP64 && !__alpha__ for this when it strikes. > > sub-optimal, but also easy to grep and find :-) Perhaps we should define "_LP64_ONLY" in for this type of situation? -- thorpej
Re: CVS commit: src/sys/kern
On Fri, Jun 28, 2019 at 06:43:45AM +1000, matthew green wrote: > "Maxime Villard" writes: > > Module Name:src > > Committed By: maxv > > Date: Thu Jun 27 19:56:10 UTC 2019 > > > > Modified Files: > > src/sys/kern: kern_exec.c > > > > Log Message: > > Fix this fucking shit once and for all, for fuck's sake. > > this is unacceptable behaviour. > > please stop being so rude and childish. I totally aggree with matthew here. Maxime, please try to write professional, technical and readable descriptions in your commit logs. Also avoid personal attacks by all means. Use them on the phone or by yelling at your keyboard, but not on the mailing lists or in commit logs. Martin
Re: CVS commit: src/sys/kern
Le 27/06/2019 à 20:56, Christos Zoulas a écrit : On Jun 27, 8:30pm, m...@m00nbsd.net (Maxime Villard) wrote: -- Subject: Re: CVS commit: src/sys/kern | Le 27/06/2019 à 19:07, Christos Zoulas a écrit : | > Module Name: src | > Committed By:christos | > Date:Thu Jun 27 17:07:51 UTC 2019 | > | > Modified Files: | > src/sys/kern: kern_exec.c | > | > Log Message: | > Return an error if the path was too long. Pointed out by maxv | > | > | > To generate a diff of this commit: | > cvs rdiff -u -r1.474 -r1.475 src/sys/kern/kern_exec.c | > | > Please note that diffs are not public domain; they are subject to the | > copyright notices on the relevant files. | | Did you just introduce an even bigger bug? I don't know, is this the most efficient way to communicate it if I did? This got to be a fucking joke.
Re: CVS commit: src/sys/kern
Le 27/06/2019 à 21:56, Maxime Villard a écrit : Module Name:src Committed By: maxv Date: Thu Jun 27 19:56:10 UTC 2019 Modified Files: src/sys/kern: kern_exec.c Log Message: Fix this fucking shit once and for all, for fuck's sake. To generate a diff of this commit: cvs rdiff -u -r1.476 -r1.477 src/sys/kern/kern_exec.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. When I see this cocktail of systematic carelessness, gross incompetence and glaring lack of engineering spirit in writing software, I can only understand why modern airplanes crash. Christos' commit access should be revoked.
Re: CVS commit: src/sys/kern
In article <20190627195610.c4be7f...@cvs.netbsd.org>, Maxime Villard wrote: >-=-=-=-=-=- > >Module Name: src >Committed By: maxv >Date: Thu Jun 27 19:56:10 UTC 2019 > >Modified Files: > src/sys/kern: kern_exec.c > >Log Message: >Fix this fucking shit once and for all, for fuck's sake. This is exactly what I was referring to about poor communication in my previous replay. A better commit message would have been: Actually set the error returned. The previous commit made things worse because the function would return a 0 and free the pathbuf crashing the kernel. This would have been informative, non-insulting and probably would have taught the author to be more careful in the future. Best, christos
Re: CVS commit: src/sys/kern
On Jun 27, 9:26pm, m...@m00nbsd.net (Maxime Villard) wrote: -- Subject: Re: CVS commit: src/sys/kern | > I don't know, is this the most efficient way to communicate it if I did? | | This got to be a fucking joke. Well, you know the bug, and I think it is more valuable for me to spend time trying to get you to communicate better instead of reading the code. Reading the code and finding that one bug will not correct the issue that you choose to communicate in an oblique and hostile manner. christos
re: CVS commit: src/sys/compat/sys
> Always include the 32 bit structure and definitions on _LP64 regardless > of compat32 being on or off, because we want the headers to work when > compiling modular kernels. Of course the 32 bit structs do not make sense > on platforms that don't have 32 bit modes (alpha), but we don't have > a define for that and it does not hurt. i've been using _LP64 && !__alpha__ for this when it strikes. sub-optimal, but also easy to grep and find :-) .mrg.
Re: CVS commit: src/sys/kern
On Jun 27, 8:30pm, m...@m00nbsd.net (Maxime Villard) wrote: -- Subject: Re: CVS commit: src/sys/kern | Le 27/06/2019 à 19:07, Christos Zoulas a écrit : | > Module Name:src | > Committed By: christos | > Date: Thu Jun 27 17:07:51 UTC 2019 | > | > Modified Files: | > src/sys/kern: kern_exec.c | > | > Log Message: | > Return an error if the path was too long. Pointed out by maxv | > | > | > To generate a diff of this commit: | > cvs rdiff -u -r1.474 -r1.475 src/sys/kern/kern_exec.c | > | > Please note that diffs are not public domain; they are subject to the | > copyright notices on the relevant files. | | Did you just introduce an even bigger bug? I don't know, is this the most efficient way to communicate it if I did? christos
Re: CVS commit: src/sys/kern
Le 27/06/2019 à 19:07, Christos Zoulas a écrit : Module Name:src Committed By: christos Date: Thu Jun 27 17:07:51 UTC 2019 Modified Files: src/sys/kern: kern_exec.c Log Message: Return an error if the path was too long. Pointed out by maxv To generate a diff of this commit: cvs rdiff -u -r1.474 -r1.475 src/sys/kern/kern_exec.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Did you just introduce an even bigger bug?
Re: CVS commit: src/sys/kern
Le 26/06/2019 à 23:21, Christos Zoulas a écrit : In article <20190626202859.b5ccef...@cvs.netbsd.org>, Maxime Villard wrote: -=-=-=-=-=- Module Name:src Committed By: maxv Date: Wed Jun 26 20:28:59 UTC 2019 Modified Files: src/sys/kern: kern_exec.c Log Message: Remove useless debugging messages which achieved nothing but hiding bugs. I apologize for adding debugging code that made the code too complicated for you to understand and caused you to wrongly accuse me for forgetting to handle another error condition. Fortunately, you saw fit to remove this debugging code as you have done in the past. Eventually there will be no debugging code in the tree, because the code will be bug free and "obvious". Looks like I'm not the one who gets confused by debugging code after all. The rest of my answer will be off-list
Re: CVS commit: src/sys/kern
On Jun 27, 6:59pm, m...@m00nbsd.net (Maxime Villard) wrote: -- Subject: Re: CVS commit: src/sys/kern | Yet it seems pretty obvious to me. As you explained in the comment, the | function is supposed to return an absolute path. Here, however, it does | not return an absolute path: | | if (len + 1 >= MAXPATHLEN) | goto out; | | Nor does it initialize 'offs'. Notice, in addition, the "XXX: GCC" you put | in the caller, as if GCC was wrong in reporting that because of the | aforementioned branch, 'offs' wasn't getting initialized properly. | | So, has this become intentional, or not? Because it seems clear it wasn't | intentional when you initially committed it. No, it is not intentional and it is a bug. Thanks for pointing it out. Initially the code used to tolerate non-absolute paths (and being able to execute binaries without resolving the full path) but eventually having the full path became necessary (to support the sysctl to get the executable path for example, or for $ORIGIN). This was not enforced in exec_makepathbuf(), but it is enforced later pathexec() which makes little sense since if you can coerce exec_makepathbuf() to return a relative path you can potentially crash the kernel. Thanks for pointing it out and it would have been helpful if you communicated the specific issue in the first place. christos
Re: CVS commit: src/sys/arch/amd64/amd64
Le 27/06/2019 à 04:00, Christos Zoulas a écrit : Module Name:src Committed By: christos Date: Thu Jun 27 02:00:31 UTC 2019 Modified Files: src/sys/arch/amd64/amd64: machdep.c Log Message: Although this is correct, I will let maxv commit it. Still waiting. To generate a diff of this commit: cvs rdiff -u -r1.333 -r1.334 src/sys/arch/amd64/amd64/machdep.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. This isn't correct, with USER_LDT the 32bit LWPs may have non-default segregs, besides it is really dumb to mix 32 and 64bit code, part of the reasons why I dropped the thing
Re: CVS commit: src/sys/kern
Le 26/06/2019 à 22:33, Christos Zoulas a écrit : On Jun 26, 10:30pm, m...@m00nbsd.net (Maxime Villard) wrote: -- Subject: Re: CVS commit: src/sys/kern | Le 25/06/2019 à 23:32, Christos Zoulas a écrit : | > Module Name: src | > Committed By:christos | > Date:Tue Jun 25 21:32:58 UTC 2019 | > | > Modified Files: | > src/sys/kern: kern_exec.c | > | > Log Message: | > Fail if getcwd fails. Pointed out by maxv@ | > | > | > To generate a diff of this commit: | > cvs rdiff -u -r1.471 -r1.472 src/sys/kern/kern_exec.c | > | > Please note that diffs are not public domain; they are subject to the | > copyright notices on the relevant files. | | You still left one error condition unhandled. Is this intentional, or did | you just forget that one too? I don't see it. Yet it seems pretty obvious to me. As you explained in the comment, the function is supposed to return an absolute path. Here, however, it does not return an absolute path: if (len + 1 >= MAXPATHLEN) goto out; Nor does it initialize 'offs'. Notice, in addition, the "XXX: GCC" you put in the caller, as if GCC was wrong in reporting that because of the aforementioned branch, 'offs' wasn't getting initialized properly. So, has this become intentional, or not? Because it seems clear it wasn't intentional when you initially committed it.
Re: CVS commit: src/sys/dev/pci/ixgbe
On 2019/06/27 18:56, SAITOH Masanobu wrote: > Module Name: src > Committed By: msaitoh > Date: Thu Jun 27 09:56:39 UTC 2019 > > Modified Files: > src/sys/dev/pci/ixgbe: ixv.c > > Log Message: > Don't call set_vfta() if any VLAN is attached. s/is/is not/ > XXX pullup-8. > > > To generate a diff of this commit: > cvs rdiff -u -r1.115 -r1.116 src/sys/dev/pci/ixgbe/ixv.c > > 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/kern
On 26.06.2019 23:21, Christos Zoulas wrote: > In article <20190626202859.b5ccef...@cvs.netbsd.org>, > Maxime Villard wrote: >> -=-=-=-=-=- >> >> Module Name: src >> Committed By:maxv >> Date:Wed Jun 26 20:28:59 UTC 2019 >> >> Modified Files: >> src/sys/kern: kern_exec.c >> >> Log Message: >> Remove useless debugging messages which achieved nothing but hiding bugs. > > I apologize for adding debugging code that made the code too complicated > for you to understand and caused you to wrongly accuse me for forgetting > to handle another error condition. Fortunately, you saw fit to remove this > debugging code as you have done in the past. Eventually there will be no > debugging code in the tree, because the code will be bug free and "obvious". > > christos > I have no opinion on debug messages, but could we implement fexecve() please, as we keep patching these code paths now? We have got already a reserved syscall and ATF tests in tree. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
In article <20190626202859.b5ccef...@cvs.netbsd.org>, Maxime Villard wrote: >-=-=-=-=-=- > >Module Name: src >Committed By: maxv >Date: Wed Jun 26 20:28:59 UTC 2019 > >Modified Files: > src/sys/kern: kern_exec.c > >Log Message: >Remove useless debugging messages which achieved nothing but hiding bugs. I apologize for adding debugging code that made the code too complicated for you to understand and caused you to wrongly accuse me for forgetting to handle another error condition. Fortunately, you saw fit to remove this debugging code as you have done in the past. Eventually there will be no debugging code in the tree, because the code will be bug free and "obvious". christos
re: CVS commit: src/sys/kern
"Maxime Villard" writes: > Module Name: src > Committed By: maxv > Date: Wed Jun 26 20:28:59 UTC 2019 > > Modified Files: > src/sys/kern: kern_exec.c > > Log Message: > Remove useless debugging messages which achieved nothing but hiding bugs. considering the author of these changes just was in this code, why did you go against policy and simply revert their changes instead of opening a discussion? please un-revert and start a discussion. what you think of as "useless" here is debateable. thanks. .mrg.
Re: CVS commit: src/sys/kern
On Jun 26, 10:30pm, m...@m00nbsd.net (Maxime Villard) wrote: -- Subject: Re: CVS commit: src/sys/kern | Le 25/06/2019 à 23:32, Christos Zoulas a écrit : | > Module Name:src | > Committed By: christos | > Date: Tue Jun 25 21:32:58 UTC 2019 | > | > Modified Files: | > src/sys/kern: kern_exec.c | > | > Log Message: | > Fail if getcwd fails. Pointed out by maxv@ | > | > | > To generate a diff of this commit: | > cvs rdiff -u -r1.471 -r1.472 src/sys/kern/kern_exec.c | > | > Please note that diffs are not public domain; they are subject to the | > copyright notices on the relevant files. | | You still left one error condition unhandled. Is this intentional, or did | you just forget that one too? I don't see it. christos
Re: CVS commit: src/sys/kern
Le 25/06/2019 à 23:32, Christos Zoulas a écrit : Module Name:src Committed By: christos Date: Tue Jun 25 21:32:58 UTC 2019 Modified Files: src/sys/kern: kern_exec.c Log Message: Fail if getcwd fails. Pointed out by maxv@ To generate a diff of this commit: cvs rdiff -u -r1.471 -r1.472 src/sys/kern/kern_exec.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. You still left one error condition unhandled. Is this intentional, or did you just forget that one too?
Re: CVS commit: src/sys/kern
Le 25/06/2019 à 20:06, Christos Zoulas a écrit : Module Name:src Committed By: christos Date: Tue Jun 25 18:06:29 UTC 2019 Modified Files: src/sys/kern: kern_exec.c Log Message: add a comment explaining what this does. To generate a diff of this commit: cvs rdiff -u -r1.469 -r1.470 src/sys/kern/kern_exec.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. While you're at it, it would be nice to also explain whether the fact that we don't actually return an error in the two last error conditions is intentional or not. Because, it does look a lot like in these cases it is returning garbage.
Re: CVS commit: src/sys/dev/usb
On Tue, Jun 25, 2019 at 4:03 PM Nick Hudson wrote: > > On 24/06/2019 10:40, Ryota Ozaki wrote: > > On Mon, Jun 24, 2019 at 6:27 PM matthew green wrote: > >> > >>> Only KERNEL_LOCK (and some splsoftnet) is required for the network stack > >>> now. Remaining splnets are for network drivers. (softnet_lock is also > >>> required > >>> in some cases but it's another story...) > >> > >> great! i studied the code and i couldn't find any issues > >> in any of the relevant paths, so i'm glad to hear it's > >> supposed to be like this. > >> > >> for one particular case (ether_ioctl) how is this diff: > >> > >> - * Common ioctls for Ethernet interfaces. Note, we must be > >> - * called at splnet(). > >> + * Common ioctls for Ethernet interfaces. > >> + * > >> + * Non IFEF_MPSAFE drivers must call this function at at least called > >> + * at splsoftnet(). > >> > >> or should they also be with kernel lock? > > > > Yes. > > > > Also I think splnet is still needed for ether_ioctl for sure because > > it has to care not only the network stack but also network drivers. > > If a driver is made MP safe and regardless of if it is marked > IFEF_MPSAFE then I think the splnet part can be dropped. That is, > struct ifnet is either KERNEL_LOCK / IFNET_LOCK AND driver mutex > protected. The driver mutexes provide appropriate mutex exclusion > between threads and interrupts. > > Would you agree? Oh, agree. I assumed non MP-safe drivers, not non IFEF_MPSAFE drivers. ozaki-r
Re: CVS commit: src/sys/dev/usb
On 24/06/2019 10:40, Ryota Ozaki wrote: On Mon, Jun 24, 2019 at 6:27 PM matthew green wrote: Only KERNEL_LOCK (and some splsoftnet) is required for the network stack now. Remaining splnets are for network drivers. (softnet_lock is also required in some cases but it's another story...) great! i studied the code and i couldn't find any issues in any of the relevant paths, so i'm glad to hear it's supposed to be like this. for one particular case (ether_ioctl) how is this diff: - * Common ioctls for Ethernet interfaces. Note, we must be - * called at splnet(). + * Common ioctls for Ethernet interfaces. + * + * Non IFEF_MPSAFE drivers must call this function at at least called + * at splsoftnet(). or should they also be with kernel lock? Yes. Also I think splnet is still needed for ether_ioctl for sure because it has to care not only the network stack but also network drivers. If a driver is made MP safe and regardless of if it is marked IFEF_MPSAFE then I think the splnet part can be dropped. That is, struct ifnet is either KERNEL_LOCK / IFNET_LOCK AND driver mutex protected. The driver mutexes provide appropriate mutex exclusion between threads and interrupts. Would you agree? Thanks, Nick
Re: CVS commit: src/sys/dev/usb
On Mon, Jun 24, 2019 at 9:39 AM Ryota Ozaki wrote: > > On Mon, Jun 24, 2019 at 6:27 PM matthew green wrote: > > > > > Only KERNEL_LOCK (and some splsoftnet) is required for the network stack > > > now. Remaining splnets are for network drivers. (softnet_lock is also > > > required > > > in some cases but it's another story...) > > > > great! i studied the code and i couldn't find any issues > > in any of the relevant paths, so i'm glad to hear it's > > supposed to be like this. > > > > for one particular case (ether_ioctl) how is this diff: > > > > - * Common ioctls for Ethernet interfaces. Note, we must be > > - * called at splnet(). > > + * Common ioctls for Ethernet interfaces. > > + * > > + * Non IFEF_MPSAFE drivers must call this function at at least called > > + * at splsoftnet(). > > > > or should they also be with kernel lock? > > Yes. > > Also I think splnet is still needed for ether_ioctl for sure because > it has to care not only the network stack but also network drivers. > > ozaki-r Thank you all for detailed explanations. I hope the source would be -DNET_MPSAFE by default. Thanks,
Re: CVS commit: src/sys/dev/usb
On Mon, Jun 24, 2019 at 6:27 PM matthew green wrote: > > > Only KERNEL_LOCK (and some splsoftnet) is required for the network stack > > now. Remaining splnets are for network drivers. (softnet_lock is also > > required > > in some cases but it's another story...) > > great! i studied the code and i couldn't find any issues > in any of the relevant paths, so i'm glad to hear it's > supposed to be like this. > > for one particular case (ether_ioctl) how is this diff: > > - * Common ioctls for Ethernet interfaces. Note, we must be > - * called at splnet(). > + * Common ioctls for Ethernet interfaces. > + * > + * Non IFEF_MPSAFE drivers must call this function at at least called > + * at splsoftnet(). > > or should they also be with kernel lock? Yes. Also I think splnet is still needed for ether_ioctl for sure because it has to care not only the network stack but also network drivers. ozaki-r
re: CVS commit: src/sys/dev/usb
> Only KERNEL_LOCK (and some splsoftnet) is required for the network stack > now. Remaining splnets are for network drivers. (softnet_lock is also > required > in some cases but it's another story...) great! i studied the code and i couldn't find any issues in any of the relevant paths, so i'm glad to hear it's supposed to be like this. for one particular case (ether_ioctl) how is this diff: - * Common ioctls for Ethernet interfaces. Note, we must be - * called at splnet(). + * Common ioctls for Ethernet interfaces. + * + * Non IFEF_MPSAFE drivers must call this function at at least called + * at splsoftnet(). or should they also be with kernel lock? thanks.
Re: CVS commit: src/sys/dev/usb
On Mon, Jun 24, 2019 at 5:26 PM Manuel Bouyer wrote: > > On Mon, Jun 24, 2019 at 08:39:08AM +0100, Nick Hudson wrote: > > > > > > On 24/06/2019 04:30, matthew green wrote: > > > > > splnet is obsolete in modern USB network drivers. > > > > > all the code runs at softipl. > > > > > > > > > > removing spl was done entirely on purpose. > > > > > > > > I saw the comment of ether_ioctl in sys/net/if_ethersubr.c > > > > > Note, we must be called at splnet(). > > > > so I asked. > > > > > > that comment is true for old style drivers, but looking at other > > > drivers they also only skip this for NET_MPSAFE kernel builds. > > > > > > Nick, do we need to make these go back to non-mpsafe stuff for > > > networking if !NET_MPSAFE? > > > > I'm really not sure. > > > > splnet is to prevent IPL_NET interrupt handlers (common for most > > drivers) from running. USB ethernet devices don't have these, however. > > All handling of device to host communications is done via USB callbacks > > which run at splsoftserial (aka splusb). > > Actually, I think splnet() ( + KERNEL_LOCK) is used to protect the network > stack when NET_MPSAFE is not defined. Only KERNEL_LOCK (and some splsoftnet) is required for the network stack now. Remaining splnets are for network drivers. (softnet_lock is also required in some cases but it's another story...) > > For example pppoe(4) uses it when NET_MPSAFE is not defined. Because there was a driver, lmc(4), that calls pppoe from hardware interrupt handler, but now it's removed, so splnet is not required anymore and splsoftnet is enough now (as per knakahara@). ozaki-r
Re: CVS commit: src/sys/dev/usb
On Mon, Jun 24, 2019 at 08:39:08AM +0100, Nick Hudson wrote: > > > On 24/06/2019 04:30, matthew green wrote: > > > > splnet is obsolete in modern USB network drivers. > > > > all the code runs at softipl. > > > > > > > > removing spl was done entirely on purpose. > > > > > > I saw the comment of ether_ioctl in sys/net/if_ethersubr.c > > > > Note, we must be called at splnet(). > > > so I asked. > > > > that comment is true for old style drivers, but looking at other > > drivers they also only skip this for NET_MPSAFE kernel builds. > > > > Nick, do we need to make these go back to non-mpsafe stuff for > > networking if !NET_MPSAFE? > > I'm really not sure. > > splnet is to prevent IPL_NET interrupt handlers (common for most > drivers) from running. USB ethernet devices don't have these, however. > All handling of device to host communications is done via USB callbacks > which run at splsoftserial (aka splusb). Actually, I think splnet() ( + KERNEL_LOCK) is used to protect the network stack when NET_MPSAFE is not defined. For example pppoe(4) uses it when NET_MPSAFE is not defined. -- Manuel Bouyer NetBSD: 26 ans d'experience feront toujours la difference --
Re: CVS commit: src/sys/dev/usb
On 24/06/2019 04:30, matthew green wrote: splnet is obsolete in modern USB network drivers. all the code runs at softipl. removing spl was done entirely on purpose. I saw the comment of ether_ioctl in sys/net/if_ethersubr.c Note, we must be called at splnet(). so I asked. that comment is true for old style drivers, but looking at other drivers they also only skip this for NET_MPSAFE kernel builds. Nick, do we need to make these go back to non-mpsafe stuff for networking if !NET_MPSAFE? I'm really not sure. splnet is to prevent IPL_NET interrupt handlers (common for most drivers) from running. USB ethernet devices don't have these, however. All handling of device to host communications is done via USB callbacks which run at splsoftserial (aka splusb). Perhaps splusb is required, but then it's unclear to me what is actually being protected which isn't already protected by mutex(es). eg, look what wm(4) idoes with WM_MPSAFE usage. are we getting ahead of ourselves in usb? :) Maybe, but I don't think so. Nick
Re: CVS commit: src/sys/dev/usb
On Mon, Jun 24, 2019 at 01:30:09PM +1000, matthew green wrote: > > > splnet is obsolete in modern USB network drivers. > > > all the code runs at softipl. > > > > > > removing spl was done entirely on purpose. > > > > I saw the comment of ether_ioctl in sys/net/if_ethersubr.c > > > Note, we must be called at splnet(). > > so I asked. > > that comment is true for old style drivers, but looking at other > drivers they also only skip this for NET_MPSAFE kernel builds. > > Nick, do we need to make these go back to non-mpsafe stuff for > networking if !NET_MPSAFE? > > eg, look what wm(4) idoes with WM_MPSAFE usage. are we getting > ahead of ourselves in usb? :) I think so. It NET_MPSAFE is not defined, AFAIK the network layers still need to be called at splnet(). -- Manuel Bouyer NetBSD: 26 ans d'experience feront toujours la difference --
re: CVS commit: src/sys/dev/usb
> > splnet is obsolete in modern USB network drivers. > > all the code runs at softipl. > > > > removing spl was done entirely on purpose. > > I saw the comment of ether_ioctl in sys/net/if_ethersubr.c > > Note, we must be called at splnet(). > so I asked. that comment is true for old style drivers, but looking at other drivers they also only skip this for NET_MPSAFE kernel builds. Nick, do we need to make these go back to non-mpsafe stuff for networking if !NET_MPSAFE? eg, look what wm(4) idoes with WM_MPSAFE usage. are we getting ahead of ourselves in usb? :) .mrg.
Re: CVS commit: src/sys/dev/usb
On Sun, Jun 23, 2019 at 10:40 PM matthew green wrote: > > sc dying writes: > > hi, > > > > On Sat, Jun 22, 2019 at 9:54 AM matthew green wrote: > > > > > > Module Name:src > > > Committed By: mrg > > > Date: Sat Jun 22 09:53:56 UTC 2019 > > > > > > Modified Files: > > > src/sys/dev/usb: if_axen.c > > > > > > Log Message: > > > mark this driver MPSAFE for usb tasks and pipes, if(4), and callouts. > > > remove remaining redundant spl calls. > > > > Should ether_ioctl be wrapped with splnet? > > ure, cdce and usmsc also need splnet. > > splnet is obsolete in modern USB network drivers. > all the code runs at softipl. > > removing spl was done entirely on purpose. I saw the comment of ether_ioctl in sys/net/if_ethersubr.c > Note, we must be called at splnet(). so I asked. > > > Should if_percpuq_enqueue be called with rxlock held? > > I'm not sure, but I cannot find the reason it is called > > after rxlock is unlocked. > > if_percpuq_enqueue() wants to be called with no locks held. > > > .mrg.
re: CVS commit: src/sys/dev/usb
sc dying writes: > hi, > > On Sat, Jun 22, 2019 at 9:54 AM matthew green wrote: > > > > Module Name:src > > Committed By: mrg > > Date: Sat Jun 22 09:53:56 UTC 2019 > > > > Modified Files: > > src/sys/dev/usb: if_axen.c > > > > Log Message: > > mark this driver MPSAFE for usb tasks and pipes, if(4), and callouts. > > remove remaining redundant spl calls. > > Should ether_ioctl be wrapped with splnet? > ure, cdce and usmsc also need splnet. splnet is obsolete in modern USB network drivers. all the code runs at softipl. removing spl was done entirely on purpose. > Should if_percpuq_enqueue be called with rxlock held? > I'm not sure, but I cannot find the reason it is called > after rxlock is unlocked. if_percpuq_enqueue() wants to be called with no locks held. .mrg.
Re: CVS commit: src/sys/dev/usb
hi, On Sun, Jun 23, 2019 at 2:14 AM matthew green wrote: > > Module Name:src > Committed By: mrg > Date: Sun Jun 23 02:14:15 UTC 2019 > > Modified Files: > src/sys/dev/usb: if_cdce.c if_ure.c if_urevar.h > > Log Message: > make cdce(4) and ure(4) usb and mpsafe: > > - introduce locking ala smsc(4)/axen(4) style > - convert to mpsafe interfaces > - add tick task to cdce(4) to deal with missing watchdog, and > actually make the watchdog do something > - convert DELAY() to usbd_delay_ms() in cdce(4) and don't increase > the time in a potentially unbounded way > - remove spl calls Should not ure_init_locked check ure_stopping? If ure_stopping == true, no one clear it. (But, it works anyway because ure_stop_locked does not set ure_stopping.)
Re: CVS commit: src/sys/dev/usb
hi, On Sat, Jun 22, 2019 at 9:54 AM matthew green wrote: > > Module Name:src > Committed By: mrg > Date: Sat Jun 22 09:53:56 UTC 2019 > > Modified Files: > src/sys/dev/usb: if_axen.c > > Log Message: > mark this driver MPSAFE for usb tasks and pipes, if(4), and callouts. > remove remaining redundant spl calls. Should ether_ioctl be wrapped with splnet? ure, cdce and usmsc also need splnet. Should if_percpuq_enqueue be called with rxlock held? I'm not sure, but I cannot find the reason it is called after rxlock is unlocked. Thanks,
Re: CVS commit: src/sys
On Sun, Jun 23, 2019 at 01:13:20PM +, m...@netbsd.org wrote: > On Mon, May 06, 2019 at 08:05:03AM +, Kamil Rytarowski wrote: > > KSI_INIT_TRAP(); > > ksi.ksi_lid = l->l_lid; > > - ksi.ksi_info._signo = signo; > > - ksi.ksi_info._code = trapno; > > + ksi.ksi_signo = signo; > > + ksi.ksi_code = trapno; > > > I suspect no longer setting ksi.ksi_info._signo will cause compatibility > issues. Sorry, misread.
Re: CVS commit: src/sys
On Mon, May 06, 2019 at 08:05:03AM +, Kamil Rytarowski wrote: > KSI_INIT_TRAP(); > ksi.ksi_lid = l->l_lid; > - ksi.ksi_info._signo = signo; > - ksi.ksi_info._code = trapno; > + ksi.ksi_signo = signo; > + ksi.ksi_code = trapno; I suspect no longer setting ksi.ksi_info._signo will cause compatibility issues. Also, we need to adapt netbsd32_ksi32_to_ksi and netbsd32_ksi_to_ksi32.
Re: CVS commit: src/sys/arch
On 19.06.2019 06:41, m...@netbsd.org wrote: > On Tue, Jun 18, 2019 at 09:18:13PM +, Kamil Rytarowski wrote: >> Module Name: src >> Committed By:kamil >> Date:Tue Jun 18 21:18:13 UTC 2019 >> >> Modified Files: >> src/sys/arch/aarch64/include: ptrace.h >> src/sys/arch/alpha/include: ptrace.h >> src/sys/arch/amd64/include: ptrace.h >> src/sys/arch/arm/include: ptrace.h >> src/sys/arch/hppa/include: ptrace.h >> src/sys/arch/i386/include: ptrace.h >> src/sys/arch/ia64/include: ptrace.h >> src/sys/arch/m68k/include: ptrace.h >> src/sys/arch/mips/include: ptrace.h >> src/sys/arch/or1k/include: ptrace.h >> src/sys/arch/powerpc/include: ptrace.h >> src/sys/arch/riscv/include: ptrace.h >> src/sys/arch/sh3/include: ptrace.h >> src/sys/arch/sparc/include: ptrace.h >> src/sys/arch/vax/include: ptrace.h >> >> Log Message: >> Introduce PTRACE_REG_FP() a helper macro to retrieve the frame pointer >> >> The macro is dummy for ia64 (the FP register is unknown and can change >> freely) and sparc/sparc64 (not stored in struct reg). > > Wouldn't it be better not to declare PTRACE_REG_FP for the cases where > obtaining it is more complicated? > > e.g. someone who hasn't seen this commit and wants to use PTRACE_REG_FP > thinks that they can just use it, and until they specifically test ia64 > and sparc64 they won't know it doesn't behave correctly. > FP isn't reliable on any platform so every person has to be prepared. For meaningful backtraces there is need to use DWARF or CTF. Returning 0 doesn't make it much worse than on other CPUs. It's used in simpler debuggers (my use-case is edb-debugger) and in tests mainly. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/arch
On Tue, Jun 18, 2019 at 09:18:13PM +, Kamil Rytarowski wrote: > Module Name: src > Committed By: kamil > Date: Tue Jun 18 21:18:13 UTC 2019 > > Modified Files: > src/sys/arch/aarch64/include: ptrace.h > src/sys/arch/alpha/include: ptrace.h > src/sys/arch/amd64/include: ptrace.h > src/sys/arch/arm/include: ptrace.h > src/sys/arch/hppa/include: ptrace.h > src/sys/arch/i386/include: ptrace.h > src/sys/arch/ia64/include: ptrace.h > src/sys/arch/m68k/include: ptrace.h > src/sys/arch/mips/include: ptrace.h > src/sys/arch/or1k/include: ptrace.h > src/sys/arch/powerpc/include: ptrace.h > src/sys/arch/riscv/include: ptrace.h > src/sys/arch/sh3/include: ptrace.h > src/sys/arch/sparc/include: ptrace.h > src/sys/arch/vax/include: ptrace.h > > Log Message: > Introduce PTRACE_REG_FP() a helper macro to retrieve the frame pointer > > The macro is dummy for ia64 (the FP register is unknown and can change > freely) and sparc/sparc64 (not stored in struct reg). Wouldn't it be better not to declare PTRACE_REG_FP for the cases where obtaining it is more complicated? e.g. someone who hasn't seen this commit and wants to use PTRACE_REG_FP thinks that they can just use it, and until they specifically test ia64 and sparc64 they won't know it doesn't behave correctly.
Re: CVS commit: src/sys/conf
Hi, chris...@zoulas.com (Christos Zoulas) writes: > On Jun 18, 9:37pm, r...@tetera.org (Ryo ONODERA) wrote: > -- Subject: Re: CVS commit: src/sys/conf > > | chris...@astron.com (Christos Zoulas) writes: > | > | > Yes, the question becomes should be part of the default GENERIC build? > | > I think not (like before). > | > | I feel that chfs should be disabled by default. > | And hopefully it should be documented that flash(4) is required > | in filesystems.config or similar place. > > Yes, I agree. Can you supply a patch (documentation)? > - there is no nand9 man page? I will try to write the man page. Thank you. > christos -- Ryo ONODERA // r...@tetera.org PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
Re: CVS commit: src/sys/conf
On Jun 18, 9:37pm, r...@tetera.org (Ryo ONODERA) wrote: -- Subject: Re: CVS commit: src/sys/conf | chris...@astron.com (Christos Zoulas) writes: | | > Yes, the question becomes should be part of the default GENERIC build? | > I think not (like before). | | I feel that chfs should be disabled by default. | And hopefully it should be documented that flash(4) is required | in filesystems.config or similar place. Yes, I agree. Can you supply a patch (documentation)? - there is no nand9 man page? christos
Re: CVS commit: src/sys/conf
chris...@astron.com (Christos Zoulas) writes: > Yes, the question becomes should be part of the default GENERIC build? > I think not (like before). I feel that chfs should be disabled by default. And hopefully it should be documented that flash(4) is required in filesystems.config or similar place. Thank you. > christos > -- Ryo ONODERA // r...@tetera.org PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
Re: CVS commit: src/sys/conf
In article <87muigi0qk@brownie.elements.tetera.org>, Ryo ONODERA wrote: >Hi, > >"Christos Zoulas" writes: > >> Module Name: src >> Committed By:christos >> Date:Mon Jun 17 17:06:39 UTC 2019 >> >> Modified Files: >> src/sys/conf: filesystems.config >> >> Log Message: >> comment out CHFS to fix build issues > >I have the following lines in my local kernel config file >and it fixes the build issue. > >+# Flash subsystem >+flash* at flashbus? >+ >+# NAND subsystem >+nand* at nandbus? >+ >+pseudo-device nandemulator Yes, the question becomes should be part of the default GENERIC build? I think not (like before). christos
Re: CVS commit: src/sys/conf
Hi, "Christos Zoulas" writes: > Module Name: src > Committed By: christos > Date: Mon Jun 17 17:06:39 UTC 2019 > > Modified Files: > src/sys/conf: filesystems.config > > Log Message: > comment out CHFS to fix build issues I have the following lines in my local kernel config file and it fixes the build issue. +# Flash subsystem +flash* at flashbus? + +# NAND subsystem +nand* at nandbus? + +pseudo-device nandemulator dmesg -t: nandemulator0: NAND emulator nand0 at nandemulator0: ONFI NAND Flash nand0: vendor: NETBSD, model: NANDEMULATOR nand0: manufacturer id: 0x00 (Unknown), device id: 0x00 nand0: page size: 2048 bytes, spare size: 64 bytes, block size: 131072 bytes nand0: LUN size: 256 blocks, LUNs: 1, total storage size: 32 MB nand0: column cycles: 2, row cycles: 3, width: x16 flash0 at nand0: partition, size 32 MB, offset 0 flash0: erase size 128 KB, page size 2048 bytes, write size 2048 bytes So it seems that chfs requires flash(4) device. Thank you. > To generate a diff of this commit: > cvs rdiff -u -r1.12 -r1.13 src/sys/conf/filesystems.config > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. > > Modified files: > > Index: src/sys/conf/filesystems.config > diff -u src/sys/conf/filesystems.config:1.12 > src/sys/conf/filesystems.config:1.13 > --- src/sys/conf/filesystems.config:1.12 Sun Jun 16 23:34:01 2019 > +++ src/sys/conf/filesystems.config Mon Jun 17 13:06:39 2019 > @@ -1,10 +1,10 @@ > -# $NetBSD: filesystems.config,v 1.12 2019/06/17 03:34:01 christos Exp $ > +# $NetBSD: filesystems.config,v 1.13 2019/06/17 17:06:39 christos Exp $ > #file-system ADOSFS # AmigaDOS-compatible file system > #options APPLE_UFS # Apple UFS support in FFS > #file-system AUTOFS # Automounter Filesystem > #pseudo-device autofs # experimental - AUTOFS > file-system CD9660 # ISO 9660 + Rock Ridge file system > -file-system CHFS# Chip File System > +#file-system CHFS# Chip File System > file-system CODA# Coda File System; also needs vcoda (below) > pseudo-devicevcoda # coda minicache <-> venus comm. > file-system EFS # Silicon Graphics Extent File System > -- Ryo ONODERA // r...@tetera.org PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
Re: CVS commit: src/sys/ufs
On Sun, 16 Jun 2019, Christos Zoulas wrote: No, there is still an issue. The change adds those files in the kernel when there are no filesystems present. Otherwise I just built a MODULAR kernel with FFS disabled and I get: Don't you get the same? Or don't you have quotas or extended attributes on in your kernel? I do not have QUOTA, QUOTA2, or UFS_EXTATTR defined in my kernel. I removed them when I removed all the file systems. It didn't seem to me to make much sense to include those options without their filesystem. Looking at the code it seems that we either need to disable the options if FFS is not also defined, or add a ``&& defined(FFS)'' to each of the #if in ufs_vfsops.c and ufs_vnops.c. FWIW, the ffs module always gets built with QUOTA and QUOTA2, but not with UFS_EXTATTR, so support for quotas is always included in the ffs module. (I have never tried to use quotas, so I don't know if they work this way.) christos ld: ufs_vfsops.o: in function `ufs_quotactl': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vfsops.c:140: un defined reference to `quota_handle_cmd' ld: ufs_vfsops.o: in function `ufs_init': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vfsops.c:270: undefined reference to `dqinit' ld: ufs_vfsops.o: in function `ufs_done': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vfsops.c:298: undefined reference to `dqdone' ld: ufs_vfsops.o: in function `ufs_init': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vfsops.c:276: undefined reference to `ufs_extattr_init' ld: ufs_vfsops.o: in function `ufs_reinit': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vfsops.c:284: undefined reference to `dqreinit' ld: ufs_vfsops.o: in function `ufs_done': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vfsops.c:305: undefined reference to `ufs_extattr_done' ld: ufs_vnops.o: in function `ufs_check_possible': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vnops.c:279: undefined reference to `chkdq' ld: ufs_vnops.o: in function `ufs_chown': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vnops.c:686: undefined reference to `chkdq' ld: /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vnops.c:687: undefined reference to `chkiq' ld: /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vnops.c:694: undefined reference to `chkdq' ld: /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vnops.c:704: undefined reference to `chkdq' ld: /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vnops.c:705: undefined reference to `chkiq' ld: ufs_vnops.o: in function `ufs_setattr': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vnops.c:559: undefined reference to `ufs_truncate_retry' ld: ufs_vnops.o: in function `ufs_chown': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vnops.c:695: undefined reference to `chkiq' ld: /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vnops.c:698: undefined reference to `chkdq' *** [netbsd] Error code 1 On Jun 16, 2019, at 10:00 PM, Paul Goyette wrote: On Sun, 16 Jun 2019, Paul Goyette wrote: If I'm understanding your changes correctly, a kernel will have a copy of ufs_quota.c built-in, whether or not the ffs code is built-in. So if you have a kernel with no ffs, that kernel will still have the quota code, and loading the ffs module will result in duplicate symbols. The error will occur at module load/link time, not at kernel build/link. OK, obviously I did not understand the changes correctly! I just did a re-build of my no-included-filesystem kernel and the files in question were not included, either. So there won't be any dup symbols when the module is eventually loaded. Sorry for the noise. ++--+---+ | 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 | ++--+---+ !DSPAM:5d06fc1163151467713922! ++--+---+ | 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 | ++--+---+
Re: CVS commit: src/sys/ufs
I did not rearrange where things live (or I don't understand what you mean by that). I just uncommented the no filesystem FFS from MODULAR, and tried to compile. Does it compile for you without my changes? christos > On Jun 16, 2019, at 8:10 PM, Paul Goyette wrote: > >
Re: CVS commit: src/sys/ufs
No, there is still an issue. The change adds those files in the kernel when there are no filesystems present. Otherwise I just built a MODULAR kernel with FFS disabled and I get: Don't you get the same? Or don't you have quotas or extended attributes on in your kernel? christos ld: ufs_vfsops.o: in function `ufs_quotactl': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vfsops.c:140: un defined reference to `quota_handle_cmd' ld: ufs_vfsops.o: in function `ufs_init': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vfsops.c:270: undefined reference to `dqinit' ld: ufs_vfsops.o: in function `ufs_done': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vfsops.c:298: undefined reference to `dqdone' ld: ufs_vfsops.o: in function `ufs_init': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vfsops.c:276: undefined reference to `ufs_extattr_init' ld: ufs_vfsops.o: in function `ufs_reinit': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vfsops.c:284: undefined reference to `dqreinit' ld: ufs_vfsops.o: in function `ufs_done': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vfsops.c:305: undefined reference to `ufs_extattr_done' ld: ufs_vnops.o: in function `ufs_check_possible': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vnops.c:279: undefined reference to `chkdq' ld: ufs_vnops.o: in function `ufs_chown': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vnops.c:686: undefined reference to `chkdq' ld: /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vnops.c:687: undefined reference to `chkiq' ld: /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vnops.c:694: undefined reference to `chkdq' ld: /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vnops.c:704: undefined reference to `chkdq' ld: /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vnops.c:705: undefined reference to `chkiq' ld: ufs_vnops.o: in function `ufs_setattr': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vnops.c:559: undefined reference to `ufs_truncate_retry' ld: ufs_vnops.o: in function `ufs_chown': /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vnops.c:695: undefined reference to `chkiq' ld: /usr/src/sys/arch/amd64/compile/MODULAR/../../../../ufs/ufs/ufs_vnops.c:698: undefined reference to `chkdq' *** [netbsd] Error code 1 > On Jun 16, 2019, at 10:00 PM, Paul Goyette wrote: > > On Sun, 16 Jun 2019, Paul Goyette wrote: > >> If I'm understanding your changes correctly, a kernel will have a copy >> of ufs_quota.c built-in, whether or not the ffs code is built-in. So >> if you have a kernel with no ffs, that kernel will still have the quota >> code, and loading the ffs module will result in duplicate symbols. The >> error will occur at module load/link time, not at kernel build/link. > > OK, obviously I did not understand the changes correctly! I just did a > re-build of my no-included-filesystem kernel and the files in question > were not included, either. So there won't be any dup symbols when the > module is eventually loaded. > > Sorry for the noise. > > > > ++--+---+ > | 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 | > ++--+---+
Re: CVS commit: src/sys/ufs
On Sun, 16 Jun 2019, Paul Goyette wrote: If I'm understanding your changes correctly, a kernel will have a copy of ufs_quota.c built-in, whether or not the ffs code is built-in. So if you have a kernel with no ffs, that kernel will still have the quota code, and loading the ffs module will result in duplicate symbols. The error will occur at module load/link time, not at kernel build/link. OK, obviously I did not understand the changes correctly! I just did a re-build of my no-included-filesystem kernel and the files in question were not included, either. So there won't be any dup symbols when the module is eventually loaded. Sorry for the noise. ++--+---+ | 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 | ++--+---+
Re: CVS commit: src/sys/ufs
On Sun, 16 Jun 2019, Christos Zoulas wrote: I did not rearrange where things live (or I don't understand what you mean by that). I just uncommented the no filesystem FFS from MODULAR, and tried to compile. The various sys/modules/*/Makefiles also specifically include the same files in the modules. For example, the ffs module has SRCS= ufs_bmap.c ufs_dirhash.c ufs_extattr.c ufs_inode.c \ ufs_lookup.c ufs_quota.c ufs_quota1.c ufs_quota2.c ufs_rename.c \ ufs_vfsops.c ufs_vnops.c ufs_wapbl.c quota2_subr.c If I'm understanding your changes correctly, a kernel will have a copy of ufs_quota.c built-in, whether or not the ffs code is built-in. So if you have a kernel with no ffs, that kernel will still have the quota code, and loading the ffs module will result in duplicate symbols. The error will occur at module load/link time, not at kernel build/link. Does it compile for you without my changes? Yes, it compiled and linked successfully for me, for both of it == GENERIC kernel with all standard filesystems and it == kernel with no filesystems of any sort included ++--+---+ | 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 | ++--+---+
Re: CVS commit: src/sys/ufs
On Sun, 16 Jun 2019, Paul Goyette wrote: Hmmm, I wonder why this is needed? I have NO built-in filesystems in my custom kernel, and it has not had any linkage problems in many years. # modstat | grep vfs ffs vfs boot -0 - wapbl kernfs vfs filesys a0 - - null vfs filesys a0 - layerfs procfs vfs filesys a0 - ptrace_common ptyfsvfs filesys a0 - - tmpfsvfs filesys a0 - - wapblvfs boot -1 - - # In any case, since these files will now be included in kernels that don't have the relevant file systems, you should also update the various sys/modules/*fs/Makefile to not include these files in the modules. And, since you've rearranged where these things live, you should also probably bump the kernel version... :) On Sun, 16 Jun 2019, Christos Zoulas wrote: Module Name:src Committed By: christos Date: Sun Jun 16 22:43:33 UTC 2019 Modified Files: src/sys/ufs: files.ufs Log Message: Include the fs scaffolding when none of the ffs/mfs/ext2fs/chfs is included so a MODULAR kernel links. To generate a diff of this commit: cvs rdiff -u -r1.43 -r1.44 src/sys/ufs/files.ufs Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. !DSPAM:5d06c65790118679131813! ++--+---+ | 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 | ++--+---+ ++--+---+ | 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 | ++--+---+
Re: CVS commit: src/sys/ufs
Hmmm, I wonder why this is needed? I have NO built-in filesystems in my custom kernel, and it has not had any linkage problems in many years. # modstat | grep vfs ffs vfs boot -0 - wapbl kernfs vfs filesys a0 - - null vfs filesys a0 - layerfs procfs vfs filesys a0 - ptrace_common ptyfsvfs filesys a0 - - tmpfsvfs filesys a0 - - wapblvfs boot -1 - - # On Sun, 16 Jun 2019, Christos Zoulas wrote: Module Name:src Committed By: christos Date: Sun Jun 16 22:43:33 UTC 2019 Modified Files: src/sys/ufs: files.ufs Log Message: Include the fs scaffolding when none of the ffs/mfs/ext2fs/chfs is included so a MODULAR kernel links. To generate a diff of this commit: cvs rdiff -u -r1.43 -r1.44 src/sys/ufs/files.ufs Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. !DSPAM:5d06c65790118679131813! ++--+---+ | 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 | ++--+---+
re: CVS commit: src/sys/dev/ic
> > also, what's IST_MPSAFE? sounds like some platform specific > > thing. > > Sure. Each platform should have it's own. Unless someone wants to make an > MI intr_establish. then it's not useful for MI driver. you quote spl(9): >At the time of writing, the global kernel_lock is automatically >acquired for interrupts at this level, in order to support >device drivers that do not provide their own multiprocessor syn- >chronization. A future release of the system may allow the >automatic acquisition of kernel_lock to be disabled for individ- >ual interrupt handlers. and then: > > in MI drivers IPL_VM vs IPL_SCHED is the mpsafe tag. > > Not according to spl(9). my point is that when you use IPL_VM you may end up taking the kernel lock even if you don't need it. it implies "not mpsafe". back when ad reduced things to the current level, the plan would have been to obsolete IPL_VM entirely and simply have one level between IPL_NONE and IPL_HIGH, but that was too hard to do all at once, and IPL_VM for old code was kept, with the advice to avoid IPL_VM for the future. note that pretty much all other modern platforms have obsoleted any ability to block some interrupts -- you either block or allow -- and they have been shown to remain performant, so while we can talk about the ability to block some but not higher level interupts from what i can tell about modern compupting, it doesn't really matter any more. (in my world view, the restriction differences between IPL_VM and IPL_SCHED should be removed, and kmem should be allowed in interrupt handlers -- we've had kmem_intr_alloc() as the real back end for over 10 years at this point, time to accept it as the reality.) > > if you're wanting to restore some level for drivers to use > > below IPL_SCHED, that may be a reasonable thing to consider > > (though it's been abandoned by most platforms AFAICT), but > > that will require a fairly large rearrangement of spl(9) > > and the interfaces -- either adding a new level entirely > > (IPL_VMSAFE? :-) or adding an mpsafe tag everywhere, so i'm > > really not sure it's a good idea. > > Why is the latter not possible? that latter is possible, but it's fairly huge change -- how many MI calls to *intr_establish() exist that will need to be udpate? .mrg.
Re: CVS commit: src/sys/dev/ic
On 31/05/2019 07:27, matthew green wrote: > Nick Hudson writes: >> On 30/05/2019 21:46, matthew green wrote: Committed By: tnn Date: Thu May 30 07:37:17 UTC 2019 Modified Files: src/sys/dev/ic: ssdfb.c Log Message: - include uvm.h before uvm_device.h - don't need IPL_SCHED here >>> >>> the IPL_SCHED change seems backwards to me. >>> >>> IPL_VM is the "this driver is not updated to MPSAFE yet" level, >>> but IPL_SCHED is the MPSAFE level. >>> >>> ie, we should be striving to remove any uses of IPL_VM, not >>> moving (back) to them. >> >> In my view we should be using IPL_VM + IST_MPSAFE (and not IPL_SCHED) if >> the driver is MPSAFE. > > why? > > IPL_VM is basically synonymous with !MPSAFE in the current > kernel -- lots of subsystems will take kernel lock if you > use IPL_VM. From spl(9)... Code running at this level endures the same restrictions as at IPL_SCHED, but may use the deprecated malloc(9) or endorsed pool_cache(9) interfaces to allocate memory. At the time of writing, the global kernel_lock is automatically acquired for interrupts at this level, in order to support device drivers that do not provide their own multiprocessor syn- chronization. A future release of the system may allow the automatic acquisition of kernel_lock to be disabled for individ- ual interrupt handlers. > also, what's IST_MPSAFE? sounds like some platform specific > thing. Sure. Each platform should have it's own. Unless someone wants to make an MI intr_establish. > in MI drivers IPL_VM vs IPL_SCHED is the mpsafe tag. Not according to spl(9). > if you're wanting to restore some level for drivers to use > below IPL_SCHED, that may be a reasonable thing to consider > (though it's been abandoned by most platforms AFAICT), but > that will require a fairly large rearrangement of spl(9) > and the interfaces -- either adding a new level entirely > (IPL_VMSAFE? :-) or adding an mpsafe tag everywhere, so i'm > really not sure it's a good idea. Why is the latter not possible? > > for now, IPL_VM should be avoided for MPSAFE code. At this point I disagree :) Nick
re: CVS commit: src/sys/dev/ic
Nick Hudson writes: > On 30/05/2019 21:46, matthew green wrote: > >> Committed By: tnn > >> Date: Thu May 30 07:37:17 UTC 2019 > >> > >> Modified Files: > >>src/sys/dev/ic: ssdfb.c > >> > >> Log Message: > >> - include uvm.h before uvm_device.h > >> - don't need IPL_SCHED here > > > > the IPL_SCHED change seems backwards to me. > > > > IPL_VM is the "this driver is not updated to MPSAFE yet" level, > > but IPL_SCHED is the MPSAFE level. > > > > ie, we should be striving to remove any uses of IPL_VM, not > > moving (back) to them. > > In my view we should be using IPL_VM + IST_MPSAFE (and not IPL_SCHED) if > the driver is MPSAFE. why? IPL_VM is basically synonymous with !MPSAFE in the current kernel -- lots of subsystems will take kernel lock if you use IPL_VM. also, what's IST_MPSAFE? sounds like some platform specific thing. in MI drivers IPL_VM vs IPL_SCHED is the mpsafe tag. if you're wanting to restore some level for drivers to use below IPL_SCHED, that may be a reasonable thing to consider (though it's been abandoned by most platforms AFAICT), but that will require a fairly large rearrangement of spl(9) and the interfaces -- either adding a new level entirely (IPL_VMSAFE? :-) or adding an mpsafe tag everywhere, so i'm really not sure it's a good idea. for now, IPL_VM should be avoided for MPSAFE code. .mrg.
Re: CVS commit: src/sys/dev/ic
On 30/05/2019 21:46, matthew green wrote: Committed By: tnn Date: Thu May 30 07:37:17 UTC 2019 Modified Files: src/sys/dev/ic: ssdfb.c Log Message: - include uvm.h before uvm_device.h - don't need IPL_SCHED here the IPL_SCHED change seems backwards to me. IPL_VM is the "this driver is not updated to MPSAFE yet" level, but IPL_SCHED is the MPSAFE level. ie, we should be striving to remove any uses of IPL_VM, not moving (back) to them. In my view we should be using IPL_VM + IST_MPSAFE (and not IPL_SCHED) if the driver is MPSAFE. Nick
re: CVS commit: src/sys/dev/ic
> Committed By: tnn > Date: Thu May 30 07:37:17 UTC 2019 > > Modified Files: > src/sys/dev/ic: ssdfb.c > > Log Message: > - include uvm.h before uvm_device.h > - don't need IPL_SCHED here the IPL_SCHED change seems backwards to me. IPL_VM is the "this driver is not updated to MPSAFE yet" level, but IPL_SCHED is the MPSAFE level. ie, we should be striving to remove any uses of IPL_VM, not moving (back) to them. thanks. .mrg.
Re: CVS commit: src/sys/arch
# Sorry for too delayed response.. At Mon, 6 May 2019 19:33:43 +0100, Sevan Janiyan wrote: > > What is the difference in m68k family? > > Enabled: amiga, cesfic, hp300, mac68k, news68k, next68k > > Disabled: atari, luna68k, mvme68k, sun3, x68k > > You can have systems with more than 32MB RAM on the ones which I enabled > (excluding mvme68k, sun3, x68k) > > I omitted atari because I thought it was only MILAN which supported high > memory (I was changing GENERIC, unless GENERIC wasn't present). > > I omitted luna68k because I didn't realise it can take more than 32MB > and didn't want to upset anyone. > > Do you think I should enable on the atari & luna68k ports? No, I don't think so either. Thank you for explanation. --- Tetsuya Isaki
Re: CVS commit: src/sys/arch/sparc64/sparc64
Oh, should've tested that. Survived kernels and distribution: diff --git a/sys/arch/sparc64/sparc64/db_trace.c b/sys/arch/sparc64/sparc64/db_trace.c index f5e35e79dd51..d94e5eb2d2ef 100644 --- a/sys/arch/sparc64/sparc64/db_trace.c +++ b/sys/arch/sparc64/sparc64/db_trace.c @@ -36,6 +36,7 @@ __KERNEL_RCSID(0, "$NetBSD: db_trace.c,v 1.52 2019/05/22 07:40:09 martin Exp $") #include #include #include +#include #include #include On Wed, May 22, 2019 at 03:02:13PM +0200, J. Hannken-Illjes wrote: > This breaks the build of usr.sbin/crash: > > /work/build/src/usr.sbin/crash/../../sys/arch/sparc64/sparc64/db_trace.c: In > function 'db_stack_trace_print': > /work/build/src/usr.sbin/crash/../../sys/arch/sparc64/sparc64/db_trace.c:166:37: > error: 'VM_MAX_KERNEL_ADDRESS' undeclared (first use in this function); did > you mean 'VM_MAXADDRESS'? > if (frame < KERNBASE || frame >= VM_MAX_KERNEL_ADDRESS) > ^ > VM_MAXADDRESS > /work/build/src/usr.sbin/crash/../../sys/arch/sparc64/sparc64/db_trace.c:166:37: > note: each undeclared identifier is reported only once for each function it > appears in > > -- > J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig > > > On 22. May 2019, at 09:40, Martin Husemann wrote: > > > > Module Name:src > > Committed By: martin > > Date: Wed May 22 07:40:09 UTC 2019 > > > > Modified Files: > > src/sys/arch/sparc64/sparc64: db_trace.c > > > > Log Message: > > Fix previous and use the original patch from PR port-sparc64/54221 > > instead (XXX should fix comments in param.h) > > > > > > To generate a diff of this commit: > > cvs rdiff -u -r1.51 -r1.52 src/sys/arch/sparc64/sparc64/db_trace.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/arch/sparc64/sparc64
This breaks the build of usr.sbin/crash: /work/build/src/usr.sbin/crash/../../sys/arch/sparc64/sparc64/db_trace.c: In function 'db_stack_trace_print': /work/build/src/usr.sbin/crash/../../sys/arch/sparc64/sparc64/db_trace.c:166:37: error: 'VM_MAX_KERNEL_ADDRESS' undeclared (first use in this function); did you mean 'VM_MAXADDRESS'? if (frame < KERNBASE || frame >= VM_MAX_KERNEL_ADDRESS) ^ VM_MAXADDRESS /work/build/src/usr.sbin/crash/../../sys/arch/sparc64/sparc64/db_trace.c:166:37: note: each undeclared identifier is reported only once for each function it appears in -- J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig > On 22. May 2019, at 09:40, Martin Husemann wrote: > > Module Name: src > Committed By: martin > Date: Wed May 22 07:40:09 UTC 2019 > > Modified Files: > src/sys/arch/sparc64/sparc64: db_trace.c > > Log Message: > Fix previous and use the original patch from PR port-sparc64/54221 > instead (XXX should fix comments in param.h) > > > To generate a diff of this commit: > cvs rdiff -u -r1.51 -r1.52 src/sys/arch/sparc64/sparc64/db_trace.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. > signature.asc Description: Message signed with OpenPGP
Re: CVS commit: src/sys
Date:Mon, 20 May 2019 16:35:45 -0400 From:"Christos Zoulas" Message-ID: <20190520203545.b41f6f...@cvs.netbsd.org> | Add a simple vasprintf() implementation that uses 2 passes, one to compute | the length and a second to place the data. Requested by rmind@ Shouldn't there be a va_end(cap) in there? For that the easy way would be to use cap for the kprintf() that calculates the length, and then use ap for the one that fills in the data, so that one can remain as "return kprintf();" and get the func call optimised into a jump). kre
Re: CVS commit: src/sys/arch/x86/x86
On 2019/05/16 13:20, Jason Thorpe wrote:
>
>
>> On May 15, 2019, at 9:19 PM, Maxime Villard wrote:
>>
>> Le 16/05/2019 à 04:36, SAITOH Masanobu a écrit :
>>> Module Name:src
>>> Committed By: msaitoh
>>> Date: Thu May 16 02:36:30 UTC 2019
>>> Modified Files:
>>> src/sys/arch/x86/x86: procfs_machdep.c
>>> Log Message:
>>> Use ci_feat_val[7] instead of directly getting cpuid 7 edx.
>>> To generate a diff of this commit:
>>> cvs rdiff -u -r1.28 -r1.29 src/sys/arch/x86/x86/procfs_machdep.c
>>> Please note that diffs are not public domain; they are subject to the
>>> copyright notices on the relevant files.
>>
>> The microcode updates CPUID7, if you read ci_feat_val you have the
>> initial value, not the updated value. So reading CPUID7 was the right
>> thing to do.
>
> Should the microcode update procedure fix up the cached copy?
I think so, but our code doesn't do it.
There are some code which copy a cpuid value to ci_feat_val[] and
modify some bits for workaround and use the copy. ci_feat_val[]
is better than trusting current cpuid values for consistency.
We should provide user an interface to know the current behavior
of the kernel. I think ci_feat_val[] is the data to explain the
behavior. If so, we should use ci_feat_val[] in x86/procfs_machdep.c.
But, sadly, ci_feat_val[] is not updated, so it might better to
use real cpuid value than ci_feat_val[]. The latest x86/procfs_machdep.c
(rev. 1.31) refers ci_feat_val[0..6] and not refer ci_feat_val[7].
It's inconsistent.
If we update ci_feat_val, we should have callback function to
reflect the change. Callback is not required for many bits but
it's required for some others.
I have a code locally to know which bit is changed after updating
microcode:
--
Index: x86/cpu_ucode_intel.c
===
RCS file: /cvsroot/src/sys/arch/x86/x86/cpu_ucode_intel.c,v
retrieving revision 1.17
diff -u -p -r1.17 cpu_ucode_intel.c
--- x86/cpu_ucode_intel.c 10 May 2019 18:21:01 - 1.17
+++ x86/cpu_ucode_intel.c 16 May 2019 04:28:13 -
@@ -178,9 +178,14 @@ cpu_ucode_intel_apply(struct cpu_ucode_s
{
uint32_t ucodetarget, oucodeversion, nucodeversion;
struct intel1_ucode_header *uh;
+ struct cpu_info *ci;
+ struct cpu_info oldci;
+ int i;
int platformid, cpuid, error;
size_t newbufsize = 0;
void *uha;
+ uint64_t msr = 0;
+ u_int descs[4];
if (sc->loader_version != CPU_UCODE_LOADER_INTEL1 ||
cpuno != CPU_UCODE_CURRENT_CPU)
@@ -221,15 +226,62 @@ cpu_ucode_intel_apply(struct cpu_ucode_s
intel_getcurrentucode(, );
cpuid = curcpu()->ci_index;
- kpreempt_enable();
-
if (nucodeversion != ucodetarget) {
+ kpreempt_enable();
error = EIO;
goto out;
}
- printf("cpu %d: ucode 0x%x->0x%x\n", cpuid, oucodeversion,
- nucodeversion);
+ ci = curcpu();
+ oldci = *ci;
+#if 1
+ /*
+* Update cpu_info.
+*
+* XXX cpu_probe() is currently not intended to call multiple time
+* on a cpu.
+*/
+ cpu_probe(curcpu());
+#endif
+ if (ci->ci_max_cpuid >= 7) {
+ x86_cpuid(7, descs);
+ if (descs[3] & CPUID_SEF_ARCH_CAP)
+ msr = rdmsr(MSR_IA32_ARCH_CAPABILITIES);
+ }
+ kpreempt_enable();
+
+ if ((ci->ci_max_cpuid >= 7) && (descs[3] & CPUID_SEF_ARCH_CAP))
+ printf("cpu%d: MSR_IA32_ARCH_CAPABILITIES=0x%lx\n", cpuid,
+ msr);
+ printf("cpu%d: ucode 0x%x->0x%x\n", cpuid,
+ oucodeversion, nucodeversion);
+ for (i = 0; i < __arraycount(ci->ci_feat_val); i++) {
+ uint32_t old, new, dif, set, unset;
+
+ old = oldci.ci_feat_val[i];
+ new = ci->ci_feat_val[i];
+ if (old != new) {
+ printf("cpu%d: ci_feat_val[%d] changed from %08x to "
+ "%08x\n", cpuid, i, old, new);
+ dif = old ^ new;
+ set = new & dif;
+ unset = old & dif;
+
+ if (set != 0)
+ printf("cpu%d: set: %08x\n", cpuid, set);
+ if (unset != 0)
+ printf("cpu%d: unset: %08x\n", cpuid, unset);
+
+ /*
+* Call hook if you want to do something.
+*
+* WARNING. If HyperThreading is enabled and the
+* microcode is updated to new one, another CPU's
+* feature bits also be changed and we cannot hook
+* for the CPU here.
+*/
+ }
+ }
out:
if (newbufsize != 0)
--
Re: CVS commit: src/sys/arch/x86/x86
On 2019/05/16 13:19, Maxime Villard wrote: > Le 16/05/2019 à 04:36, SAITOH Masanobu a écrit : >> Module Name: src >> Committed By: msaitoh >> Date: Thu May 16 02:36:30 UTC 2019 >> >> Modified Files: >> src/sys/arch/x86/x86: procfs_machdep.c >> >> Log Message: >> Use ci_feat_val[7] instead of directly getting cpuid 7 edx. >> >> >> To generate a diff of this commit: >> cvs rdiff -u -r1.28 -r1.29 src/sys/arch/x86/x86/procfs_machdep.c >> >> Please note that diffs are not public domain; they are subject to the >> copyright notices on the relevant files. > > The microcode updates CPUID7, if you read ci_feat_val you have the > initial value, not the updated value. So reading CPUID7 was the right > thing to do. We have no callback or something like it to reflect the change to ci_feat_val, so you're right. I'll revert it. -- --- SAITOH Masanobu (msai...@execsw.org msai...@netbsd.org)
Re: CVS commit: src/sys/arch/x86/x86
> On May 15, 2019, at 9:19 PM, Maxime Villard wrote: > > Le 16/05/2019 à 04:36, SAITOH Masanobu a écrit : >> Module Name: src >> Committed By:msaitoh >> Date:Thu May 16 02:36:30 UTC 2019 >> Modified Files: >> src/sys/arch/x86/x86: procfs_machdep.c >> Log Message: >> Use ci_feat_val[7] instead of directly getting cpuid 7 edx. >> To generate a diff of this commit: >> cvs rdiff -u -r1.28 -r1.29 src/sys/arch/x86/x86/procfs_machdep.c >> Please note that diffs are not public domain; they are subject to the >> copyright notices on the relevant files. > > The microcode updates CPUID7, if you read ci_feat_val you have the > initial value, not the updated value. So reading CPUID7 was the right > thing to do. Should the microcode update procedure fix up the cached copy? -- thorpej
Re: CVS commit: src/sys/arch/x86/x86
Le 16/05/2019 à 04:36, SAITOH Masanobu a écrit : Module Name:src Committed By: msaitoh Date: Thu May 16 02:36:30 UTC 2019 Modified Files: src/sys/arch/x86/x86: procfs_machdep.c Log Message: Use ci_feat_val[7] instead of directly getting cpuid 7 edx. To generate a diff of this commit: cvs rdiff -u -r1.28 -r1.29 src/sys/arch/x86/x86/procfs_machdep.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. The microcode updates CPUID7, if you read ci_feat_val you have the initial value, not the updated value. So reading CPUID7 was the right thing to do.
Re: CVS commit: src/sys
On Tue, May 14, 2019 at 10:54 PM Greg Troxel wrote: > > Ryota Ozaki writes: > > > On Tue, May 14, 2019 at 2:31 AM Jason Thorpe wrote: > >> > >> > >> > >> > On May 13, 2019, at 7:17 AM, Greg Troxel wrote: > >> > > >> > 2) Your option 2 seems to involve two things at once: > >> > > >> > - migration to lwp_specificadata > >> > - using DEBUG instead of DIAGNOSTIC to control the leak check feature > >> > > >> > I do not understand why changing the nature of the implementation is > >> > linked to how it is enabled. > >> > >> I think Ozaki-san saying that the 3% performance hit only happens > >> when lwp_specificdata is used, and hence that it would need to be > >> wrapped in DEBUG rather than DIAGNOSTIC. > > Now this is all making sense. > > >> The original negligible-impact implementation did NOT use > >> lwp_specificdata, and thus was fine for DIAGNOSTIC. I believe > >> Ozaki-san's preference is to use *this* implementation so that it > >> can be exposed to a wider audience. The lwp_specificdata approach > >> was only explored after someone else suggested a preference for it. > >> > >> At least, that's my understanding of the situation. > > > > Yes, your understanding is correct. Thank you for the clarification. > > So having a check under DIAGNOSTIC that you more or less can't measure > sounds just fine to me. I only meant to object to a 3% slowdown under > DIAGNOSTIC. > > And, if someone is inclined, having better checks with meaurable > slowdown under DEBUG sounds ok too, but my revised understanding is > unclear on whether that's helpful. (But I am only trying to keep > performance under DIAGNOSTIC high; I am unconcerned about DEBUG and > don't need to understand.) I've proposed another implementation a few days ago (*). That provides useful information for debugging in exchange for expensive overhead. It is enabled by PSREF_DEBUG. (*) https://mail-index.netbsd.org/tech-kern/2019/05/10/msg025049.html ozaki-r
Re: CVS commit: src/sys
Ryota Ozaki writes: >> And, if someone is inclined, having better checks with meaurable >> slowdown under DEBUG sounds ok too, but my revised understanding is >> unclear on whether that's helpful. (But I am only trying to keep >> performance under DIAGNOSTIC high; I am unconcerned about DEBUG and >> don't need to understand.) > > I've proposed another implementation a few days ago (*). That provides > useful information for debugging in exchange for expensive overhead. > It is enabled by PSREF_DEBUG. > > (*) https://mail-index.netbsd.org/tech-kern/2019/05/10/msg025049.html That sounds useful when needed. I don't mind if DEBUG turns on PSREF_DEBUG, but it might be that turning it on only when there is a known reproduction recipe for a leak is sensible. Thanks all for the useful and rational discussion.
Re: CVS commit: src/sys
Ryota Ozaki writes: > On Tue, May 14, 2019 at 2:31 AM Jason Thorpe wrote: >> >> >> >> > On May 13, 2019, at 7:17 AM, Greg Troxel wrote: >> > >> > 2) Your option 2 seems to involve two things at once: >> > >> > - migration to lwp_specificadata >> > - using DEBUG instead of DIAGNOSTIC to control the leak check feature >> > >> > I do not understand why changing the nature of the implementation is >> > linked to how it is enabled. >> >> I think Ozaki-san saying that the 3% performance hit only happens >> when lwp_specificdata is used, and hence that it would need to be >> wrapped in DEBUG rather than DIAGNOSTIC. Now this is all making sense. >> The original negligible-impact implementation did NOT use >> lwp_specificdata, and thus was fine for DIAGNOSTIC. I believe >> Ozaki-san's preference is to use *this* implementation so that it >> can be exposed to a wider audience. The lwp_specificdata approach >> was only explored after someone else suggested a preference for it. >> >> At least, that's my understanding of the situation. > > Yes, your understanding is correct. Thank you for the clarification. So having a check under DIAGNOSTIC that you more or less can't measure sounds just fine to me. I only meant to object to a 3% slowdown under DIAGNOSTIC. And, if someone is inclined, having better checks with meaurable slowdown under DEBUG sounds ok too, but my revised understanding is unclear on whether that's helpful. (But I am only trying to keep performance under DIAGNOSTIC high; I am unconcerned about DEBUG and don't need to understand.)
Re: CVS commit: src/sys
On Tue, May 14, 2019 at 2:31 AM Jason Thorpe wrote: > > > > > On May 13, 2019, at 7:17 AM, Greg Troxel wrote: > > > > 2) Your option 2 seems to involve two things at once: > > > > - migration to lwp_specificadata > > - using DEBUG instead of DIAGNOSTIC to control the leak check feature > > > > I do not understand why changing the nature of the implementation is > > linked to how it is enabled. > > I think Ozaki-san saying that the 3% performance hit only happens when > lwp_specificdata is used, and hence that it would need to be wrapped in DEBUG > rather than DIAGNOSTIC. > > The original negligible-impact implementation did NOT use lwp_specificdata, > and thus was fine for DIAGNOSTIC. I believe Ozaki-san's preference is to use > *this* implementation so that it can be exposed to a wider audience. The > lwp_specificdata approach was only explored after someone else suggested a > preference for it. > > At least, that's my understanding of the situation. Yes, your understanding is correct. Thank you for the clarification. ozaki-r
Re: CVS commit: src/sys/kern
On Tue, May 07, 2019 at 09:22:49AM +0800, Paul Goyette wrote: > Currently we have the global sysctl variable, but I wonder if it should be > made local to a particular emulation and/or to an individual process? It's a global flag because it has security impliciations. Making it per-emulation or even per-process would defeat the purpose of having it in first place. Joerg
Re: CVS commit: src/sys/arch/arm/sunxi
On Mon, May 13, 2019 at 04:55:17PM +, Manuel Bouyer wrote: > Module Name: src > Committed By: bouyer > Date: Mon May 13 16:55:17 UTC 2019 > > Modified Files: > src/sys/arch/arm/sunxi: sunxi_sata.c > > Log Message: > Use new magic values from linux for DMACR. While I couldn't measure any > significant difference with my old, slow laptop drive, linux commiter claims > a 3x write performance boost (from 40 to 120MB/s) and 200MB/s read with > a ssd. With this backported to netbsd-8 on a cubieboard2 with a (quite old) ssd: Before: /home/bouyer>dd if=/dev/zero of=test bs=1m count=2048 2048+0 records in 2048+0 records out 2147483648 bytes transferred in 69.385 secs (30950257 bytes/sec) /home/bouyer>dd if=test of=/dev/null bs=1m 2048+0 records in 2048+0 records out 2147483648 bytes transferred in 26.114 secs (82234956 bytes/sec) after /home/bouyer>dd if=/dev/zero of=test bs=1m count=2048 2048+0 records in 2048+0 records out 2147483648 bytes transferred in 44.674 secs (48070100 bytes/sec) /home/bouyer>dd if=test of=/dev/null bs=1m 2048+0 records in 2048+0 records out 2147483648 bytes transferred in 23.644 secs (90825733 bytes/sec) So it's clearly an improvement here. -- Manuel Bouyer NetBSD: 26 ans d'experience feront toujours la difference --
Re: CVS commit: src/sys
> On May 13, 2019, at 7:17 AM, Greg Troxel wrote: > > 2) Your option 2 seems to involve two things at once: > > - migration to lwp_specificadata > - using DEBUG instead of DIAGNOSTIC to control the leak check feature > > I do not understand why changing the nature of the implementation is > linked to how it is enabled. I think Ozaki-san saying that the 3% performance hit only happens when lwp_specificdata is used, and hence that it would need to be wrapped in DEBUG rather than DIAGNOSTIC. The original negligible-impact implementation did NOT use lwp_specificdata, and thus was fine for DIAGNOSTIC. I believe Ozaki-san's preference is to use *this* implementation so that it can be exposed to a wider audience. The lwp_specificdata approach was only explored after someone else suggested a preference for it. At least, that's my understanding of the situation. Now, choose wisely :-) -- thorpej
Re: CVS commit: src/sys
Ryota Ozaki writes: > On Sat, May 11, 2019 at 10:49 AM Greg Troxel wrote: >> >> I'm sorry for delaying this task. Finally I have benchmarked a revised >> >> patch >> >> (our benchmarking setups have been out of service for a couple of >> >> weeks...). >> >> >> >> Performance degradation of IP forwarding is 3%. Is it acceptable as >> >> DIAGNOSTIC? >> > >> > For DIAGNOSTIC should be fine. >> >> I think 3% is too much for DIAGNOSTIC. DEBUG, sure, and a specific >> option to turn it on seems fine. DIAGNOSTIC historically has been only >> for things that check invariants and assert, such that if you don't mind >> the crashes when detecting things, there is no performance reason to >> avoid it. > > So we have two options: > 1. Keep the original (enabled on DIAGNOSTIC) > - Its performance impact is negligible > 2. Migrate to use lwp_specificadata and enable the feature on DEBUG > (and something) I am not following two things: 1) You said that IP forwarding speed is reduced by 3%, and also that it is a negligble performance impact. In my view, 3% is not negligible. DIAGNOSTIC should be sufficiently small performance impact that essentially nobody wants to disable it to get better performance. I could see 0.3% as negligible. But everything adds up, so it's not just this, but the overall system with and without DIAGNOSTIC. 2) Your option 2 seems to involve two things at once: - migration to lwp_specificadata - using DEBUG instead of DIAGNOSTIC to control the leak check feature I do not understand why changing the nature of the implementation is linked to how it is enabled. Overall, I think checking features (other than simple KASSSERTs) should have their own ifdef, so people can individually enable them, and then additionally there would be "#ifdef DEBUG // #define PSREFLEAKDETECT" or something like that. > I prefer to 1. because I want the feature to be enabled by many users Sure, but many users prefer that their systems not be slowed down, too. If we add 10 more checks that each cause 3% slowdown, then we have a real problem. DIAGNOSTIC has always had the sense that it should turn undefined behavior into a panic, but not slow the system down, so that the only reason to avoid it is not liking the panics. > (I assume that users (of -current) tend to enable DIAGNOSTIC and not DEBUG). On -current, DIAGNOSTIC is on by default. When we branch 9, I expect that it will remain on until very close to release. I agree that most people do not have DEBUG on. > If the detector is not enabled, a psref leak appears as a form that is > difficult to know where the leak occurs; a fatal page fault occurs on > psref_target_destroy that is a completely different context. If > enabled, we can at least know a suspect LWP or softint handler and may > find a cause in luck. How often do these leaks happen? If DIAGNOSTIC without the new code will assert or crash on a leak, but in a non-specific way, we know they are fairly rare. People who encounter them can add the define to enable the more specific detector and continue running to catch the next leak. It seems that if leaks are common, then it should be easy to find them with a few people enabling the new detector. If they are very rare, then 3% forwarding speed is a large price to pay for finding them more specifically the first time, on systems where they are not expected to happen. Another possibility (that I don't advocate) is to set this up so that it is enabled on DIAGNOSTIC with current, but not enabled with DIAGNOSTIC on release branches. I am much more concerned with performance loss on releaes branches. Another option is to enable it in current, until the bugs are fixed and we don't see leaks, and then turn it off. Please don't take this as criticism of your new leak detector. We are getting lots of checking code to look for many kinds of problems. Some of it is expensive to varying degrees. Everyone benefits from having the bugs fixed once found, and it's hard to draw the line about what should be enabled by default.
