Re: CVS commit: src/usr.bin/resize

2021-02-28 Thread Rhialto
On Sun 28 Feb 2021 at 09:04:28 +, matthew green wrote:
> Module Name:  src
> Committed By: mrg
> Date: Sun Feb 28 09:04:28 UTC 2021
> 
> Modified Files:
>   src/usr.bin/resize: Makefile
> 
> Log Message:
> disable the rule to copy resize.c since it fails on r/o src trees:
> 
> cp: /home/source/ab/HEAD/src/usr.bin/resize/resize.c: Permission denied

Also, copying files on top of version-controlled files is probably not a
good idea, it will cause confusion at the next update.

Also my build failed because the xsrc version wants to #include
 and that couldn't be found. The -DRESIZE_ONLY doesn't help
because the xsrc version didn't check it. (I see that the currently
checked it version does)

-Olaf.
-- 
___ Q: "What's an anagram of Banach-Tarski?"  -- Olaf "Rhialto" Seibert
\X/ A: "Banach-Tarski Banach-Tarski." -- rhialto at falu dot nl


signature.asc
Description: PGP signature


re: CVS commit: src/share/man/man4/man4.i386

2021-02-27 Thread matthew green
"Nia Alarie" writes:
> Module Name:  src
> Committed By: nia
> Date: Fri Feb 26 10:56:48 UTC 2021
>
> Modified Files:
>   src/share/man/man4/man4.i386: intro.4
>
> Log Message:
> Remove extremely outdated list of supported devices

be nice to have a link to isa(4), eisa(4), and pci(4), and any
other relevant bus-manuals i can't think of right now, since i
now won't find eg, sb(4) via i386/intro(4) and hyper links.

thanks.


.mrg.


Re: CVS commit: src/external/bsd/nvi/usr.bin/nvi

2021-02-25 Thread Christos Zoulas
Too bad, looks like they just made a copy of the FreeBSD changes. I will revert.

christos

> On Feb 25, 2021, at 6:47 PM, Rin Okuyama  wrote:
> 
> Hi,
> 
> This does not work since nvi requires non-standard wregex API, whose
> ``char *'' arguments are replaced by ``wchar_t *'' ones:
> 
> https://mail-index.netbsd.org/tech-userlevel/2008/08/06/msg000960.html
> https://mail-index.netbsd.org/tech-userlevel/2008/08/06/msg000967.html
> 
> In principle, we can rewrite nvi to use standard regex API, but this
> causes wide to multibyte char conversion *every time* for text search
> (internal encoding of nvi is wide char). I'm not sure whether this is
> acceptable for users of slow machines, who merely want to edit ASCII
> texts.
> 
> Thanks,
> rin
> 
> On 2021/02/26 6:56, Christos Zoulas wrote:
>> Module Name: src
>> Committed By:christos
>> Date:Thu Feb 25 21:56:35 UTC 2021
>> Modified Files:
>>  src/external/bsd/nvi/usr.bin/nvi: Makefile
>> Log Message:
>> we don't need the extra copy wide-regex anymore.
>> To generate a diff of this commit:
>> cvs rdiff -u -r1.22 -r1.23 src/external/bsd/nvi/usr.bin/nvi/Makefile
>> 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/external/bsd/nvi/usr.bin/nvi

2021-02-25 Thread Rin Okuyama

Hi,

This does not work since nvi requires non-standard wregex API, whose
``char *'' arguments are replaced by ``wchar_t *'' ones:

https://mail-index.netbsd.org/tech-userlevel/2008/08/06/msg000960.html
https://mail-index.netbsd.org/tech-userlevel/2008/08/06/msg000967.html

In principle, we can rewrite nvi to use standard regex API, but this
causes wide to multibyte char conversion *every time* for text search
(internal encoding of nvi is wide char). I'm not sure whether this is
acceptable for users of slow machines, who merely want to edit ASCII
texts.

Thanks,
rin

On 2021/02/26 6:56, Christos Zoulas wrote:

Module Name:src
Committed By:   christos
Date:   Thu Feb 25 21:56:35 UTC 2021

Modified Files:
src/external/bsd/nvi/usr.bin/nvi: Makefile

Log Message:
we don't need the extra copy wide-regex anymore.


To generate a diff of this commit:
cvs rdiff -u -r1.22 -r1.23 src/external/bsd/nvi/usr.bin/nvi/Makefile

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


Re: CVS commit: src/lib/libc/regex

2021-02-25 Thread Christos Zoulas
In article <5c9e716-7ec1-9c7d-a7cb-21f08946...@invisible.ca>,
Jared McNeill   wrote:
>Building tools on macOS:
>
>/Users/jmcneill/netbsd/git-src/tools/compat/../../lib/libc/regex/regcomp.c:1585:8:
> 
>error: implicit declaration of function 'reallocarray' is invalid
>   in C99 [-Werror,-Wimplicit-function-declaration]
> ncs = reallocarray(p->g->sets, p->g->ncsets + 1, sizeof(*ncs));
>   ^
>/Users/jmcneill/netbsd/git-src/tools/compat/../../lib/libc/regex/regcomp.c:1585:8:
> 
>note: did you mean 'reallocarr'?
>/Users/jmcneill/netbsd/git-src/tools/compat/compat_defs.h:556:5: note: 
>'reallocarr' declared here
>int reallocarr(void *, size_t, size_t);
> ^

Fixed, thanks!

christos



Re: CVS commit: src/lib/libc/regex

2021-02-25 Thread Jared McNeill

Building tools on macOS:

/Users/jmcneill/netbsd/git-src/tools/compat/../../lib/libc/regex/regcomp.c:1585:8: 
error: implicit declaration of function 'reallocarray' is invalid

  in C99 [-Werror,-Wimplicit-function-declaration]
ncs = reallocarray(p->g->sets, p->g->ncsets + 1, sizeof(*ncs));
  ^
/Users/jmcneill/netbsd/git-src/tools/compat/../../lib/libc/regex/regcomp.c:1585:8: 
note: did you mean 'reallocarr'?
/Users/jmcneill/netbsd/git-src/tools/compat/compat_defs.h:556:5: note: 
'reallocarr' declared here

int reallocarr(void *, size_t, size_t);
^


On Tue, 23 Feb 2021, Christos Zoulas wrote:


Module Name:src
Committed By:   christos
Date:   Tue Feb 23 22:14:59 UTC 2021

Modified Files:
src/lib/libc/regex: cname.h engine.c re_format.7 regcomp.c regerror.c
regex.3 regex2.h regexec.c regfree.c utils.h
Removed Files:
src/lib/libc/regex: cclass.h

Log Message:
sync with FreeBSD:
   - NLS support
   - GNU extensions
   - bug fixes


To generate a diff of this commit:
cvs rdiff -u -r1.7 -r0 src/lib/libc/regex/cclass.h
cvs rdiff -u -r1.7 -r1.8 src/lib/libc/regex/cname.h
cvs rdiff -u -r1.24 -r1.25 src/lib/libc/regex/engine.c
cvs rdiff -u -r1.12 -r1.13 src/lib/libc/regex/re_format.7
cvs rdiff -u -r1.38 -r1.39 src/lib/libc/regex/regcomp.c
cvs rdiff -u -r1.23 -r1.24 src/lib/libc/regex/regerror.c
cvs rdiff -u -r1.26 -r1.27 src/lib/libc/regex/regex.3
cvs rdiff -u -r1.13 -r1.14 src/lib/libc/regex/regex2.h
cvs rdiff -u -r1.22 -r1.23 src/lib/libc/regex/regexec.c
cvs rdiff -u -r1.15 -r1.16 src/lib/libc/regex/regfree.c
cvs rdiff -u -r1.6 -r1.7 src/lib/libc/regex/utils.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/arch/aarch64/aarch64

2021-02-23 Thread Jared McNeill

On Mon, 22 Feb 2021, Ryo Shimizu wrote:


I think this condition is not necessary since cpu_idle() is just called from 
idle_loop(),
and ci_intr_depth is always zero at this time.


Ah yes, my mistake! Please feel free to revert this commit as part of 
your proposed change.


Re: CVS commit: src/sys/arch/aarch64/aarch64

2021-02-22 Thread Jason Thorpe


> On Feb 22, 2021, at 11:49 AM, Ryo Shimizu  wrote:
> 
> Ah, You are quite right!
> idle/# lwp is provided and assigned for each CPU, so curcpu() obtained from
> idle lwp was always the same.
> So, there's no need to move curcpu() to after DISABLE_INTERRUPT.

Please make sure to add a comment explaining why!

-- thorpej



Re: CVS commit: src/sys/arch/aarch64/aarch64

2021-02-22 Thread Ryo Shimizu


>> In addition, because of the possibility of kpreemption (but aarch64 has =
>no KPREEMPT yet),
>> the acquisition of curcpu() is moved to after DISABLE_INTERRUPT and got =
>the following.
>>
>[snip]
>
>
>>
>> Is this ok?
>>
>
>Looks good - I wonder if the fact that curcpu is an invariant for the
>idlelwp helps here too?

Ah, You are quite right!
idle/# lwp is provided and assigned for each CPU, so curcpu() obtained from
idle lwp was always the same.
So, there's no need to move curcpu() to after DISABLE_INTERRUPT.

Thanks
-- 
ryo shimizu


Re: CVS commit: src/sys/arch/aarch64/aarch64

2021-02-22 Thread Nick Hudson

On 22/02/2021 10:40, Ryo Shimizu wrote:



Module Name:src
Committed By:   jmcneill
Date:   Sun Feb 21 23:37:10 UTC 2021

Modified Files:
src/sys/arch/aarch64/aarch64: idle_machdep.S

Log Message:
When waking from cpu_idle(), only call dosoftints if ci_intr_depth == 0


To generate a diff of this commit:
cvs rdiff -u -r1.7 -r1.8 src/sys/arch/aarch64/aarch64/idle_machdep.S


I think this condition is not necessary since cpu_idle() is just called from 
idle_loop(),
and ci_intr_depth is always zero at this time.

After thinking about it, I realized that there is no need to even increment 
intr_depth.

   curcpu()->ci_ntr_depth = 1;
   ARM_IRQ_HANDLER();
   curcpu()->ci_ntr_depth = 0;


In addition, because of the possibility of kpreemption (but aarch64 has no 
KPREEMPT yet),
the acquisition of curcpu() is moved to after DISABLE_INTERRUPT and got the 
following.


[snip]




Is this ok?



Looks good - I wonder if the fact that curcpu is an invariant for the
idlelwp helps here too?

Nick


Re: CVS commit: src/sys/arch/aarch64/aarch64

2021-02-22 Thread Ryo Shimizu


>Module Name:   src
>Committed By:  jmcneill
>Date:  Sun Feb 21 23:37:10 UTC 2021
>
>Modified Files:
>   src/sys/arch/aarch64/aarch64: idle_machdep.S
>
>Log Message:
>When waking from cpu_idle(), only call dosoftints if ci_intr_depth == 0
>
>
>To generate a diff of this commit:
>cvs rdiff -u -r1.7 -r1.8 src/sys/arch/aarch64/aarch64/idle_machdep.S

I think this condition is not necessary since cpu_idle() is just called from 
idle_loop(),
and ci_intr_depth is always zero at this time.

After thinking about it, I realized that there is no need to even increment 
intr_depth.

  curcpu()->ci_ntr_depth = 1;
  ARM_IRQ_HANDLER();
  curcpu()->ci_ntr_depth = 0;


In addition, because of the possibility of kpreemption (but aarch64 has no 
KPREEMPT yet),
the acquisition of curcpu() is moved to after DISABLE_INTERRUPT and got the 
following.

cvs -q diff -aU10 -a -p idle_machdep.S
Index: idle_machdep.S
===
RCS file: /src/cvs/cvsroot-netbsd/src/sys/arch/aarch64/aarch64/idle_machdep.S,v
retrieving revision 1.8
diff -a -U 10 -a -p -r1.8 idle_machdep.S
--- idle_machdep.S  21 Feb 2021 23:37:09 -  1.8
+++ idle_machdep.S  22 Feb 2021 10:16:25 -
@@ -67,40 +67,36 @@ ENTRY(cpu_idle)
stp x29, x30, [sp, #TF_X29] /* save x29,x30 */
 #ifdef DDB
add x29, sp, #TF_X29/* link frame for backtrace */
 #endif
 
/* fill the minimum required trapframe */
mov x2, #SPSR_M_EL1H/* what our spsr should be */
str x2, [sp, #TF_SPSR]
adr x0, 1f
str x0, [sp, #TF_PC]/* CLKF_PC refer to tf_pc */
-
-   mrs x1, tpidr_el1   /* get curlwp */
-   ldr x1, [x1, #L_CPU]/* get curcpu */
-   ldr w28, [x1, #CI_INTR_DEPTH]   /* w28 = ci->ci_intr_depth */
-   add w2, w28, #1 /* w2 = intr_depth + 1 */
-
mov x0, sp  /* get pointer to trapframe */
+   mrs x1, tpidr_el1   /* get curlwp */
 
DISABLE_INTERRUPT
-   wfi
+   ldr x1, [x1, #L_CPU]/* get curcpu */
+   mov w2, #1
+   str w2, [x1, #CI_INTR_DEPTH]/* ci->ci_intr_depth = 1 */
 
-   str w2, [x1, #CI_INTR_DEPTH]/* ci->ci_intr_depth++ */
+   wfi
bl  ARM_IRQ_HANDLER /* irqhandler(trapframe) */
 1:
mrs x1, tpidr_el1   /* get curlwp */
ldr x1, [x1, #L_CPU]/* get curcpu */
-   str w28, [x1, #CI_INTR_DEPTH]   /* ci->ci_intr_depth = old */
+   str wzr, [x1, #CI_INTR_DEPTH]   /* ci->ci_intr_depth = 0 */
 
 #if defined(__HAVE_FAST_SOFTINTS) && !defined(__HAVE_PIC_FAST_SOFTINTS)
-   cbnzw28, 1f /* Skip if intr_depth > 0 */
ldr w3, [x1, #CI_SOFTINTS]  /* Get pending softint mask */
/* CPL should be 0 */
ldr w2, [x1, #CI_CPL]   /* Get current priority level */
lsr w3, w3, w2  /* shift mask by cpl */
cbz w3, 1f
bl  _C_LABEL(dosoftints)/* dosoftints() */
 1:
 #endif /* __HAVE_FAST_SOFTINTS && !__HAVE_PIC_FAST_SOFTINTS */
 
ldr x28, [sp, #TF_X28]  /* restore x28 */


Is this ok?
-- 
ryo shimizu


Re: CVS commit: src/lib/libc/gen

2021-02-17 Thread David Holland
On Wed, Feb 17, 2021 at 11:51:04PM +, David A. Holland wrote:
 > Also, Someone(TM) should check if POSIX permits this or if we ought to
 > improve the implementation.

Unsurprisingly, POSIX is silent. It just says "rewinddir shall also
cause the directory stream to refer to the current state of the
corresponding directory, as a call to opendir() would have done,"
basically the same text we had previously.

This could be read as _mandating_ reopening it, but that is almost
certainly not what they intended, so I think we're ok.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/opendir.html
https://pubs.opengroup.org/onlinepubs/9699919799/functions/rewinddir.html

-- 
David A. Holland
dholl...@netbsd.org


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-17 Thread Christos Zoulas
In article <5912ca9e-b4e7-423d-a45d-f4693d1c9...@zoulas.com>,
Christos Zoulas   wrote:
>-=-=-=-=-=-

Here's the final changes

- Make ALIGNED_POINTER use __alignof(t) instead of sizeof(t). This is more
  correct because it works with non-primitive types and provides the ABI
  alignment for the type the compiler will use.
- Remove all the *_HDR_ALIGNMENT macros and asserts
- Replace POINTER_ALIGNED_P with ACCESSIBLE_POINTER which is identical to
  ALIGNED_POINTER, but returns that the pointer is always aligned if the
  CPU supports unaligned accesses.

Index: sys/mbuf.h
===
RCS file: /cvsroot/src/sys/sys/mbuf.h,v
retrieving revision 1.231
diff -u -u -r1.231 mbuf.h
--- sys/mbuf.h  17 Feb 2021 22:32:04 -  1.231
+++ sys/mbuf.h  17 Feb 2021 23:54:31 -
@@ -843,15 +843,21 @@
m->m_pkthdr.rcvif_index = n->m_pkthdr.rcvif_index;
 }
 
+#define M_GET_ALIGNED_HDR(m, type, linkhdr) \
+m_get_aligned_hdr((m), __alignof(type) - 1, sizeof(type), (linkhdr))
+
 static __inline int
-m_get_aligned_hdr(struct mbuf **m, int align, size_t hlen, bool linkhdr)
+m_get_aligned_hdr(struct mbuf **m, int mask, size_t hlen, bool linkhdr)
 {
-   if (POINTER_ALIGNED_P(mtod(*m, void *), align) == 0) {
-   --align;// Turn into mask
+#ifndef __NO_STRICT_ALIGNMENT
+   if (((uintptr_t)mtod(*m, void *) & mask) != 0)
*m = m_copyup(*m, hlen, 
- linkhdr ? (max_linkhdr + align) & ~align : 0);
-   } else if (__predict_false((size_t)(*m)->m_len < hlen))
+ linkhdr ? (max_linkhdr + mask) & ~mask : 0);
+   else
+#endif
+   if (__predict_false((size_t)(*m)->m_len < hlen))
*m = m_pullup(*m, hlen);
+
return *m == NULL;
 }
 
Index: sys/param.h
===
RCS file: /cvsroot/src/sys/sys/param.h,v
retrieving revision 1.689
diff -u -u -r1.689 param.h
--- sys/param.h 17 Feb 2021 22:32:04 -  1.689
+++ sys/param.h 17 Feb 2021 23:54:31 -
@@ -281,16 +281,24 @@
 #defineALIGN(p)(((uintptr_t)(p) + ALIGNBYTES) & 
~ALIGNBYTES)
 #endif
 #ifndef ALIGNED_POINTER
-#defineALIGNED_POINTER(p,t)uintptr_t)(p)) & (sizeof(t) - 1)) 
== 0)
+#defineALIGNED_POINTER(p,t)uintptr_t)(p)) & (__alignof(t) - 
1)) == 0)
 #endif
 #ifndef ALIGNED_POINTER_LOAD
 #defineALIGNED_POINTER_LOAD(q,p,t) (*(q) = *((const t *)(p)))
 #endif
 
+/*
+ * Return if the pointer p is accessible for type t. For primitive types
+ * this means that the pointer itself can be dereferenced; for structures
+ * and unions this means that any field can be dereferenced. On CPUs
+ * that allow unaligned pointer access, we always return that the pointer
+ * is accessible to prevent unnecessary copies, although this might not be
+ * necessarily faster.
+ */
 #ifdef __NO_STRICT_ALIGNMENT
-#definePOINTER_ALIGNED_P(p, a) 1
+#defineACCESSIBLE_POINTER(p, t)1
 #else
-#definePOINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)) 
== 0)
+#defineACCESSIBLE_POINTER(p, t)ALIGNED_POINTER(p, t)
 #endif
 
 /*
Index: net/if_arp.h
===
RCS file: /cvsroot/src/sys/net/if_arp.h,v
retrieving revision 1.42
diff -u -u -r1.42 if_arp.h
--- net/if_arp.h17 Feb 2021 22:32:04 -  1.42
+++ net/if_arp.h17 Feb 2021 23:54:31 -
@@ -72,8 +72,6 @@
uint8_t  ar_tpa[];  /* target protocol address */
 #endif
 };
-#defineARP_HDR_ALIGNMENT   __alignof(struct arphdr)
-__CTASSERT(ARP_HDR_ALIGNMENT == 2);
 
 static __inline uint8_t *
 ar_data(struct arphdr *ap)
Index: net/if_bridge.c
===
RCS file: /cvsroot/src/sys/net/if_bridge.c,v
retrieving revision 1.178
diff -u -u -r1.178 if_bridge.c
--- net/if_bridge.c 14 Feb 2021 20:58:34 -  1.178
+++ net/if_bridge.c 17 Feb 2021 23:54:31 -
@@ -2806,7 +2806,7 @@
if (*mp == NULL)
return -1;
 
-   if (m_get_aligned_hdr(, IP_HDR_ALIGNMENT, sizeof(*ip), true) != 0) {
+   if (M_GET_ALIGNED_HDR(, struct ip, true) != 0) {
/* XXXJRT new stat, please */
ip_statinc(IP_STAT_TOOSMALL);
goto bad;
@@ -2900,7 +2900,7 @@
 * it.  Otherwise, if it is aligned, make sure the entire base
 * IPv6 header is in the first mbuf of the chain.
 */
-   if (m_get_aligned_hdr(, IP6_HDR_ALIGNMENT, sizeof(*ip6), true) != 0) {
+   if (M_GET_ALIGNED_HDR(, struct ip6_hdr, true) != 0) {
struct ifnet *inifp = m_get_rcvif_NOMPSAFE(m);
/* XXXJRT new stat, please */
ip6_statinc(IP6_STAT_TOOSMALL);
Index: netinet/if_arp.c
===
RCS file: 

Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-17 Thread Christos Zoulas


> On Feb 17, 2021, at 4:20 PM, Valery Ushakov  wrote:
> 
> On Wed, Feb 17, 2021 at 17:49:15 -, Christos Zoulas wrote:
> 
> This incrementally improves wrong things b/c you still have the "is
> aligned" and "ought to be aligned" conflated in one.

The incremental patch improves things and does not make things worse.
I will address the __NO_STRICT_ALIGNMENT issue separately. I am planning
to propose something in tech-kern once I have it working to my liking. This
patch helps me because it aligns the semantics of the two macros better.

christos


signature.asc
Description: Message signed with OpenPGP


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-17 Thread Valery Ushakov
On Wed, Feb 17, 2021 at 17:49:15 -, Christos Zoulas wrote:

> In article ,
> Valery Ushakov   wrote:
> 
> >But to get back to my main point, PLEASE, can we stop making random
> >aimless changes without prior discussion?
> 
> Here's the change I'd like to make:
> - pass the alignment instead of the mask (as Roy asked and to match the
>   other macro)
> - use alignof to determine that alignment and CTASSERT what we expect
> - remove unused macros
> 
> This incrementally improves things.
[...]
> diff -u -p -u -r1.688 param.h
> --- sys/param.h   15 Feb 2021 19:46:53 -  1.688
> +++ sys/param.h   17 Feb 2021 17:45:55 -
> @@ -290,7 +290,7 @@
>  #ifdef __NO_STRICT_ALIGNMENT
>  #define  POINTER_ALIGNED_P(p, a) 1
>  #else
> -#define  POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & (a)) == 0)
> +#define  POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)) 
> == 0)
>  #endif
>  
>  /*

This incrementally improves wrong things b/c you still have the "is
aligned" and "ought to be aligned" conflated in one.


-uwe


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-17 Thread Christos Zoulas
In article ,
Valery Ushakov   wrote:

>But to get back to my main point, PLEASE, can we stop making random
>aimless changes without prior discussion?

Here's the change I'd like to make:
- pass the alignment instead of the mask (as Roy asked and to match the
  other macro)
- use alignof to determine that alignment and CTASSERT what we expect
- remove unused macros

This incrementally improves things.

christos

Index: net/if_arp.h
===
RCS file: /cvsroot/src/sys/net/if_arp.h,v
retrieving revision 1.41
diff -u -p -u -r1.41 if_arp.h
--- net/if_arp.h16 Feb 2021 10:20:56 -  1.41
+++ net/if_arp.h17 Feb 2021 17:45:55 -
@@ -72,7 +72,8 @@ structarphdr {
uint8_t  ar_tpa[];  /* target protocol address */
 #endif
 };
-#defineARP_HDR_ALIGNMENT   1
+#defineARP_HDR_ALIGNMENT   __alignof(struct arphdr)
+__CTASSERT(ARP_HDR_ALIGNMENT == 2);
 
 static __inline uint8_t *
 ar_data(struct arphdr *ap)
Index: netinet/icmp_private.h
===
RCS file: /cvsroot/src/sys/netinet/icmp_private.h,v
retrieving revision 1.4
diff -u -p -u -r1.4 icmp_private.h
--- netinet/icmp_private.h  14 Feb 2021 20:58:35 -  1.4
+++ netinet/icmp_private.h  17 Feb 2021 17:45:55 -
@@ -44,7 +44,6 @@ extern percpu_t *icmpstat_percpu;
 
 #defineICMP_STATINC(x) _NET_STATINC(icmpstat_percpu, x)
 
-#defineICMP_HDR_ALIGNMENT  3
 #endif /* _KERNEL_ */
 
 #endif /* !_NETINET_ICMP_PRIVATE_H_ */
Index: netinet/igmp_var.h
===
RCS file: /cvsroot/src/sys/netinet/igmp_var.h,v
retrieving revision 1.26
diff -u -p -u -r1.26 igmp_var.h
--- netinet/igmp_var.h  14 Feb 2021 20:58:35 -  1.26
+++ netinet/igmp_var.h  17 Feb 2021 17:45:55 -
@@ -105,8 +105,6 @@
  */
 #defineIGMP_RANDOM_DELAY(X)(cprng_fast32() % (X) + 1)
 
-#defineIGMP_HDR_ALIGNMENT  3
-
 void   igmp_init(void);
 void   igmp_input(struct mbuf *, int, int);
 intigmp_joingroup(struct in_multi *);
Index: netinet/ip_private.h
===
RCS file: /cvsroot/src/sys/netinet/ip_private.h,v
retrieving revision 1.4
diff -u -p -u -r1.4 ip_private.h
--- netinet/ip_private.h14 Feb 2021 20:58:35 -  1.4
+++ netinet/ip_private.h17 Feb 2021 17:45:55 -
@@ -43,7 +43,8 @@ externpercpu_t *ipstat_percpu;
 #defineIP_STATINC(x)   _NET_STATINC(ipstat_percpu, x)
 #defineIP_STATDEC(x)   _NET_STATDEC(ipstat_percpu, x)
 
-#defineIP_HDR_ALIGNMENT3
+#defineIP_HDR_ALIGNMENT__alignof(struct ip)
+__CTASSERT(IP_HDR_ALIGNMENT == 4);
 #endif /* _KERNEL */
 
 #endif /* !_NETINET_IP_PRIVATE_H_ */
Index: netinet/tcp_private.h
===
RCS file: /cvsroot/src/sys/netinet/tcp_private.h,v
retrieving revision 1.4
diff -u -p -u -r1.4 tcp_private.h
--- netinet/tcp_private.h   14 Feb 2021 20:58:35 -  1.4
+++ netinet/tcp_private.h   17 Feb 2021 17:45:55 -
@@ -43,7 +43,8 @@ externpercpu_t *tcpstat_percpu;
 #defineTCP_STATINC(x)  _NET_STATINC(tcpstat_percpu, x)
 #defineTCP_STATADD(x, v)   _NET_STATADD(tcpstat_percpu, x, v)
 
-#defineTCP_HDR_ALIGNMENT   3
+#defineTCP_HDR_ALIGNMENT   __alignof(struct tcphdr)
+__CTASSERT(TCP_HDR_ALIGNMENT == 4);
 #endif /* _KERNEL */
 
 #endif /* !_NETINET_TCP_PRIVATE_H_ */
Index: netinet/udp_private.h
===
RCS file: /cvsroot/src/sys/netinet/udp_private.h,v
retrieving revision 1.4
diff -u -p -u -r1.4 udp_private.h
--- netinet/udp_private.h   14 Feb 2021 20:58:35 -  1.4
+++ netinet/udp_private.h   17 Feb 2021 17:45:55 -
@@ -39,7 +39,8 @@ externpercpu_t *udpstat_percpu;
 
 #defineUDP_STATINC(x)  _NET_STATINC(udpstat_percpu, x)
 
-#defineUDP_HDR_ALIGNMENT   3
+#defineUDP_HDR_ALIGNMENT   __alignof(struct udphdr)
+__CTASSERT(UDP_HDR_ALIGNMENT == 2);
 #endif /* _KERNEL */
 
 #endif /* !_NETINET_UDP_PRIVATE_H_ */
Index: netinet6/ip6_private.h
===
RCS file: /cvsroot/src/sys/netinet6/ip6_private.h,v
retrieving revision 1.4
diff -u -p -u -r1.4 ip6_private.h
--- netinet6/ip6_private.h  14 Feb 2021 20:58:35 -  1.4
+++ netinet6/ip6_private.h  17 Feb 2021 17:45:55 -
@@ -43,7 +43,8 @@ externpercpu_t *ip6stat_percpu;
 #defineIP6_STATINC(x)  _NET_STATINC(ip6stat_percpu, x)
 #defineIP6_STATDEC(x)  _NET_STATDEC(ip6stat_percpu, x)
 
-#defineIP6_HDR_ALIGNMENT   3
+#defineIP6_HDR_ALIGNMENT   __alignof(struct ip6_hdr)
+__CTASSERT(IP6_HDR_ALIGNMENT == 4);

Re: CVS commit: src/sys

2021-02-17 Thread J. Hannken-Illjes
Jared,

please test if the attached diff is sufficient.

--
J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig



uvm_swap.c.diff
Description: Binary data

> On 17. Feb 2021, at 13:07, Jared McNeill  wrote:
> 
> I noticed this on reboot since this change:
> 
> [ 3636.2891122] turning off swap...stopping swap on /swap failed with error 16
> [ 3636.2991109] stopping swap on /swap2 failed with error 16
> 
> Can you have a look?
> 
> Thanks!
> Jared
> 
> 
> On Tue, 16 Feb 2021, Juergen Hannken-Illjes wrote:
> 
>> Module Name: src
>> Committed By:hannken
>> Date:Tue Feb 16 09:56:32 UTC 2021
>> 
>> Modified Files:
>>  src/sys/kern: vfs_mount.c
>>  src/sys/uvm: uvm_swap.c
>> 
>> Log Message:
>> Reorganize uvm_swap_shutdown() a bit, make sure the vnode gets
>> locked and referenced across the call to swap_off() and finally
>> use it from vfs_unmountall1() to remove swap after unmounting
>> the last file system.
>> 
>> Adresses PR kern/54969 (Disk cache is no longer flushed on shutdown)
>> 
>> 
>> To generate a diff of this commit:
>> cvs rdiff -u -r1.85 -r1.86 src/sys/kern/vfs_mount.c
>> cvs rdiff -u -r1.200 -r1.201 src/sys/uvm/uvm_swap.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

2021-02-17 Thread Jared McNeill

I noticed this on reboot since this change:

 [ 3636.2891122] turning off swap...stopping swap on /swap failed with error 16
 [ 3636.2991109] stopping swap on /swap2 failed with error 16

Can you have a look?

Thanks!
Jared


On Tue, 16 Feb 2021, Juergen Hannken-Illjes wrote:


Module Name:src
Committed By:   hannken
Date:   Tue Feb 16 09:56:32 UTC 2021

Modified Files:
src/sys/kern: vfs_mount.c
src/sys/uvm: uvm_swap.c

Log Message:
Reorganize uvm_swap_shutdown() a bit, make sure the vnode gets
locked and referenced across the call to swap_off() and finally
use it from vfs_unmountall1() to remove swap after unmounting
the last file system.

Adresses PR kern/54969 (Disk cache is no longer flushed on shutdown)


To generate a diff of this commit:
cvs rdiff -u -r1.85 -r1.86 src/sys/kern/vfs_mount.c
cvs rdiff -u -r1.200 -r1.201 src/sys/uvm/uvm_swap.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.




Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Valery Ushakov
On Tue, Feb 16, 2021 at 18:51:43 -0500, Christos Zoulas wrote:

> On Feb 17,  2:20am, u...@stderr.spb.ru (Valery Ushakov) wrote:
> -- Subject: Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
> 
> | Well, it was you who did the definion of ALIGNED_POINTER centralized
> | and overridable :)
> | 
> |   revision 1.400
> |   date: 2012-01-25 00:03:36 +0400;  author: christos;  state: Exp;  lines: 
> +26 -1;
> |   Use and define ALIGN() ALIGN_POINTER() and STACK_ALIGN() consistently,
> |   and avoid definining them in 10 different places if not needed.
> 
> If you look a bit deeper into that change, it merged many MD copies
> of this macro into one, and I needed the override for the archs that
> were different.
> 
> | ALIGNED_POINTER is overriden on x86 to be always true.  Surprisingly
> | it is not overriden for m68k and vax that are __NO_STRICT_ALIGNMENT.
> | That is most likely an oversight, but that will probably require some
> | cvs archaeology to confirm.  Some uses of ALIGNED_POINTER are inside
> | an __NO_STRICT_ALIGNMENT #ifdef.
> 
> This is a mess as you can see. The problem is that in each case we
> need to determine if this macro is used to test alignment to determine
> access restrictions on certain architectures (most cases), or it
> is done for performance/protocol requirements.
> 
> I hope that nothing falls into the second category and then we can
> use a single macro that uses a combination of __NO_STRICT_ALIGNMENT
> and __alignof() which my guess is that it did not exist at the time
> the macro was invented, and thus it used sizeof() and limited it to
> integral types.
> 
> | We don't even seem to be sure about its semantics, as far as I can
> | tell (see bus space comments in my mail).
> | 
> | That's even more of a reason to stop doing aimless random changes
> | without getting some kind of understanding first.  The last thing we
> | need is ALIGNED_POINTER and POINTER_ALIGNED macros with slighly
> | different semantics both of which are counter-intuitive to begin with
> | (and riastradh@ even had to add a verbose comment for that).
> 
> This change was not aimless. I wanted to remove the multiple copies of
> the m_copyup/m_pullup code into one function. To do that I needed the
> alignment as a value, not a predicate macro (that was again copied in
> ~10 places). When I tried to centralize it, I wanted to do the minimal
> change so I would not screw it up (and I did end up screwing it up).
> 
> This is a good opportunity now to clean this up even more and provide
> sane alignment macros.

We should start by at least separating the concerns.  The test for
"aligned at power of two boundary" and the actual intent/purpose of
that test ("stuff needs to be aligned for us to safely do $FOO").
Then we can test the alignment check without jumping through
ridiculous hoops.  That should have been done earlier for the
ALIGNED_POINTER change, but yeah, I can see how in the heat of the
moment that change was just "preserve the status quo" and that's
absolutely fine.  But doing the same thing now with the alignment test
and the purpose of that alignment test conflacted once again under a
different but confusignly similar name is negligent (if anything we
will run out of half way decent names for the just-the-alignment-test
macro, b/c all of them will be loaded with some additional accidental
semantic).


-uwe


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Valery Ushakov
On Wed, Feb 17, 2021 at 00:51:03 +0100, Roland Illig wrote:

> 17.02.2021 00:25:10 Valery Ushakov :
> > On Tue, Feb 16, 2021 at 23:54:46 +0100, Roland Illig wrote:
> >> Yes, it does.  That's what the "#undef __NO_STRICT_ALIGNMENT" in the
> >> test is for.
> >>
> >> I intentionally placed it between  (which defines that
> >> macro on x86 and some other platforms) and  (which uses the
> >> macro to switch between the boring "everything is correctly aligned" and
> >> the more interesting formula suggested here.
> >
> > This is wrong on so many levels.  What is even the point of a test
> > that doesn't test the thing as it is actually defined and used in the
> > code?
> 
> The point of the test is to verify that the "complicated" formula
> produces correct results.  That's what several commits tried to
> fix.  If this test had been there from the beginning, none of the
> wrong formulas would have passed it.  That's the whole point.
> 
> The point of the test was intentionally not to test the actual
> behavior on each platform but to test the same formula, independent
> from the platform, and to do this, I somehow needed access to that
> formula.  Testing the actually used formula per platform could be
> added as another test.  I just wanted to avoid the obviously wrong
> formulas to go unnoticed in the code.  That's the point of the test,
> and that's exactly what it achieves.  Therefore I don't see anything
> wrong with it.

The very fact that you need to undefine an unspecified macro at an
unspecified time to get to the "formula" points to a problem.  We
shouldn't be pretending that it's not, and provide the false decorum
of "oh, but it's covered with a test, so it's ok".

-uwe


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Christos Zoulas
On Feb 17,  2:20am, u...@stderr.spb.ru (Valery Ushakov) wrote:
-- Subject: Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

| Well, it was you who did the definion of ALIGNED_POINTER centralized
| and overridable :)
| 
|   revision 1.400
|   date: 2012-01-25 00:03:36 +0400;  author: christos;  state: Exp;  lines: 
+26 -1;
|   Use and define ALIGN() ALIGN_POINTER() and STACK_ALIGN() consistently,
|   and avoid definining them in 10 different places if not needed.

If you look a bit deeper into that change, it merged many MD copies
of this macro into one, and I needed the override for the archs that
were different.

| ALIGNED_POINTER is overriden on x86 to be always true.  Surprisingly
| it is not overriden for m68k and vax that are __NO_STRICT_ALIGNMENT.
| That is most likely an oversight, but that will probably require some
| cvs archaeology to confirm.  Some uses of ALIGNED_POINTER are inside
| an __NO_STRICT_ALIGNMENT #ifdef.

This is a mess as you can see. The problem is that in each case we
need to determine if this macro is used to test alignment to determine
access restrictions on certain architectures (most cases), or it
is done for performance/protocol requirements.

I hope that nothing falls into the second category and then we can
use a single macro that uses a combination of __NO_STRICT_ALIGNMENT
and __alignof() which my guess is that it did not exist at the time
the macro was invented, and thus it used sizeof() and limited it to
integral types.

| We don't even seem to be sure about its semantics, as far as I can
| tell (see bus space comments in my mail).
| 
| That's even more of a reason to stop doing aimless random changes
| without getting some kind of understanding first.  The last thing we
| need is ALIGNED_POINTER and POINTER_ALIGNED macros with slighly
| different semantics both of which are counter-intuitive to begin with
| (and riastradh@ even had to add a verbose comment for that).

This change was not aimless. I wanted to remove the multiple copies of
the m_copyup/m_pullup code into one function. To do that I needed the
alignment as a value, not a predicate macro (that was again copied in
~10 places). When I tried to centralize it, I wanted to do the minimal
change so I would not screw it up (and I did end up screwing it up).

This is a good opportunity now to clean this up even more and provide
sane alignment macros.

christos


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Roland Illig
17.02.2021 00:25:10 Valery Ushakov :
> On Tue, Feb 16, 2021 at 23:54:46 +0100, Roland Illig wrote:
>> Yes, it does.  That's what the "#undef __NO_STRICT_ALIGNMENT" in the
>> test is for.
>>
>> I intentionally placed it between  (which defines that
>> macro on x86 and some other platforms) and  (which uses the
>> macro to switch between the boring "everything is correctly aligned" and
>> the more interesting formula suggested here.
>
> This is wrong on so many levels.  What is even the point of a test
> that doesn't test the thing as it is actually defined and used in the
> code?

The point of the test is to verify that the "complicated" formula produces 
correct results.  That's what several commits tried to fix.  If this test had 
been there from the beginning, none of the wrong formulas would have passed it. 
 That's the whole point.

The point of the test was intentionally not to test the actual behavior on each 
platform but to test the same formula, independent from the platform, and to do 
this, I somehow needed access to that formula.  Testing the actually used 
formula per platform could be added as another test.  I just wanted to avoid 
the obviously wrong formulas to go unnoticed in the code.  That's the point of 
the test, and that's exactly what it achieves.  Therefore I don't see anything 
wrong with it.

Roland


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Roland Illig

On 16.02.2021 23:32, Jason Thorpe wrote:



On Feb 16, 2021, at 1:56 PM, Roland Illig  wrote:

A downside of this test is that the macro from  gets only
evaluated at compile time of the test.  The test therefore cannot cover
future updates to  without being reinstalled itself.  But
at least for the release builds, it ensures a correct definition.


You can make this get evaluated at run-time by passing in a non-constant 
argument.


I wanted to make the test even more flexible: react to the then-current
installed version of the macro in .  To do that, the test
would need to be compiled at runtime, requiring a C compiler on the
target machine, and I'm not sure that's a valid assumption.

And maybe that's something that shouldn't be done at all, updating the
system header without the corresponding test suite.  So it's probably
fine as it is now, notwithstanding the discussion about whether the
macro is needed at all in this prominent place.

It surprised me though that neither Google nor GitHub found the macro
name POINTER_ALIGNED_P anywhere outside the NetBSD repository.  I would
have expected this name to be already used by some projects, given that
it is so straight-forward.

Roland


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Valery Ushakov
On Tue, Feb 16, 2021 at 17:53:09 -0500, Christos Zoulas wrote:

> In this case "type" is a struct and we have __alignof() to handle
> it, but does this give the right answer?
> 
> Also ALIGNED_POINTER is not conditional to __NO_STRICT_ALIGNMENT and
> can be overriden (the opposite goes for POINTER_ALIGNED_P) I am all
> for having one macro, but how can we satisfy all the different
> semantics?

Well, it was you who did the definion of ALIGNED_POINTER centralized
and overridable :)

  revision 1.400
  date: 2012-01-25 00:03:36 +0400;  author: christos;  state: Exp;  lines: +26 
-1;
  Use and define ALIGN() ALIGN_POINTER() and STACK_ALIGN() consistently,
  and avoid definining them in 10 different places if not needed.

ALIGNED_POINTER is overriden on x86 to be always true.  Surprisingly
it is not overriden for m68k and vax that are __NO_STRICT_ALIGNMENT.
That is most likely an oversight, but that will probably require some
cvs archaeology to confirm.  Some uses of ALIGNED_POINTER are inside
an __NO_STRICT_ALIGNMENT #ifdef.

We don't even seem to be sure about its semantics, as far as I can
tell (see bus space comments in my mail).

That's even more of a reason to stop doing aimless random changes
without getting some kind of understanding first.  The last thing we
need is ALIGNED_POINTER and POINTER_ALIGNED macros with slighly
different semantics both of which are counter-intuitive to begin with
(and riastradh@ even had to add a verbose comment for that).


-uwe


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Valery Ushakov
On Tue, Feb 16, 2021 at 23:54:46 +0100, Roland Illig wrote:

> On 16.02.2021 23:40, Valery Ushakov wrote:
> > On Tue, Feb 16, 2021 at 22:56:19 +0100, Roland Illig wrote:
> > 
> > > On 16.02.2021 19:46, Roy Marples wrote:
> > > > I suggest we change POINTER_ALIGNED_P to accept the alignment value we
> > > > want rather than the bitwise test we supply, like so:
> > > > 
> > > > #define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)))
> > > 
> > > To make sure that this macro doesn't get broken again, I have written
> > > the attached tests.  I will commit them after making sure I got the
> > > changes to distrib/sets/lists/tests/mi correct.  All the rest I already
> > > tested successfully.
> > 
> > I don't see any proposal to change the meaning of this macro to
> > actually require the alignment even for arches without strict
> > alignment.  Does the attached test really passes on e.g. x86 where the
> > macro is always true?  E.g. this one:
> > 
> > > + if (POINTER_ALIGNED_P(p + 1, 2))
> > > + atf_tc_fail("p + 1 must not be aligned to 2");
> 
> Yes, it does.  That's what the "#undef __NO_STRICT_ALIGNMENT" in the
> test is for.
> 
> I intentionally placed it between  (which defines that
> macro on x86 and some other platforms) and  (which uses the
> macro to switch between the boring "everything is correctly aligned" and
> the more interesting formula suggested here.

This is wrong on so many levels.  What is even the point of a test
that doesn't test the thing as it is actually defined and used in the
code?

-uwe


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Roland Illig

On 16.02.2021 23:40, Valery Ushakov wrote:

On Tue, Feb 16, 2021 at 22:56:19 +0100, Roland Illig wrote:


On 16.02.2021 19:46, Roy Marples wrote:

I suggest we change POINTER_ALIGNED_P to accept the alignment value we
want rather than the bitwise test we supply, like so:

#define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)))


To make sure that this macro doesn't get broken again, I have written
the attached tests.  I will commit them after making sure I got the
changes to distrib/sets/lists/tests/mi correct.  All the rest I already
tested successfully.


I don't see any proposal to change the meaning of this macro to
actually require the alignment even for arches without strict
alignment.  Does the attached test really passes on e.g. x86 where the
macro is always true?  E.g. this one:


+   if (POINTER_ALIGNED_P(p + 1, 2))
+   atf_tc_fail("p + 1 must not be aligned to 2");


Yes, it does.  That's what the "#undef __NO_STRICT_ALIGNMENT" in the
test is for.

I intentionally placed it between  (which defines that
macro on x86 and some other platforms) and  (which uses the
macro to switch between the boring "everything is correctly aligned" and
the more interesting formula suggested here.


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Christos Zoulas
In this case "type" is a struct and we have __alignof() to handle it, but does 
this give the
right answer? Also ALIGNED_POINTER is not conditional to __NO_STRICT_ALIGNMENT
and can be overriden (the opposite goes for POINTER_ALIGNED_P)  I am all for 
having one
macro, but how can we satisfy all the different semantics?

christos


signature.asc
Description: Message signed with OpenPGP


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Valery Ushakov
On Tue, Feb 16, 2021 at 22:56:19 +0100, Roland Illig wrote:

> On 16.02.2021 19:46, Roy Marples wrote:
> > I suggest we change POINTER_ALIGNED_P to accept the alignment value we
> > want rather than the bitwise test we supply, like so:
> > 
> > #define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)))
> 
> That's a good definition, clear, simple, obvious, without any surprises.

Now, replace the value "a" with the type "t" and change (a) to
sizeof(t) and you will get ALIGNED_POINTER from just above.


> To make sure that this macro doesn't get broken again, I have written
> the attached tests.  I will commit them after making sure I got the
> changes to distrib/sets/lists/tests/mi correct.  All the rest I already
> tested successfully.

I don't see any proposal to change the meaning of this macro to
actually require the alignment even for arches without strict
alignment.  Does the attached test really passes on e.g. x86 where the
macro is always true?  E.g. this one:

> + if (POINTER_ALIGNED_P(p + 1, 2))
> + atf_tc_fail("p + 1 must not be aligned to 2");


> Is my assumption correct that on each platform supported by NetBSD, a
> variable of type double gets aligned to a multiple of 8, even on the
> stack?

No.  E.g. on sh3 doubles are 8 bytes but are 4 bytes aligned.  I'm
almost sure some other ABI has that kind less strict alignment too,
but I don't remember.


> I wanted to keep the test as simple as possible, therefore I
> didn't want to call malloc just to get an aligned pointer.

You can use an ordinary array that is large enough and manually
find/construct a suitably aligned pointer value inside that array.


BUT, can we, PLEASE, stop making random breaking changes and at least
articulate first what is that that we want here?

POINTER_ALIGNED_P should have never been brought into existence in the
first place.

ALIGNED_POINTER seems to be used to define BUS_SPACE_ALIGNED_POINTER -
the real fun here is sys/arch/x86/include/bus_defs.h that defines
BUS_SPACE_ALIGNED_POINTER to be "really" aligned for BUS_SPACE_DEBUG,
which seems kinda suspicious to me.  BTW, can we really conclude from
the CPU's alignment requirements to some bus alignment requirements?

But to get back to my main point, PLEASE, can we stop making random
aimless changes without prior discussion?

To quote Vonnegut, "If I had of been a observer, I would of said we
was comical."


-uwe


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Jason Thorpe


> On Feb 16, 2021, at 1:56 PM, Roland Illig  wrote:
> 
> A downside of this test is that the macro from  gets only
> evaluated at compile time of the test.  The test therefore cannot cover
> future updates to  without being reinstalled itself.  But
> at least for the release builds, it ensures a correct definition.

You can make this get evaluated at run-time by passing in a non-constant 
argument.

-- thorpej



Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Roland Illig

On 16.02.2021 19:46, Roy Marples wrote:

I suggest we change POINTER_ALIGNED_P to accept the alignment value we
want rather than the bitwise test we supply, like so:

#define    POINTER_ALIGNED_P(p, a)    (((uintptr_t)(p) & ((a) - 1))


That's a good definition, clear, simple, obvious, without any surprises.

To make sure that this macro doesn't get broken again, I have written
the attached tests.  I will commit them after making sure I got the
changes to distrib/sets/lists/tests/mi correct.  All the rest I already
tested successfully.

Is my assumption correct that on each platform supported by NetBSD, a
variable of type double gets aligned to a multiple of 8, even on the
stack?  I wanted to keep the test as simple as possible, therefore I
didn't want to call malloc just to get an aligned pointer.

A downside of this test is that the macro from  gets only
evaluated at compile time of the test.  The test therefore cannot cover
future updates to  without being reinstalled itself.  But
at least for the release builds, it ensures a correct definition.

Roland
Index: distrib/sets/lists/tests/mi
===
RCS file: /cvsroot/src/distrib/sets/lists/tests/mi,v
retrieving revision 1.1019
diff -u -p -r1.1019 mi
--- distrib/sets/lists/tests/mi 16 Feb 2021 09:46:24 -  1.1019
+++ distrib/sets/lists/tests/mi 16 Feb 2021 21:45:40 -
@@ -4396,6 +4396,10 @@
 ./usr/tests/sys/rc/h_args  tests-sys-tests 
compattestfile,atf
 ./usr/tests/sys/rc/h_simpletests-sys-tests 
compattestfile,atf
 ./usr/tests/sys/rc/t_rc_d_cli  tests-sys-tests 
compattestfile,atf
+./usr/tests/sys/systests-sys-tests 
compattestfile,atf
+./usr/tests/sys/sys/Atffiletests-sys-tests 
compattestfile,atf
+./usr/tests/sys/sys/Kyuafile   tests-sys-tests 
compattestfile,atf,kyua
+./usr/tests/sys/sys/t_pointer_aligned_ptests-sys-tests 
compattestfile,atf
 ./usr/tests/sys/x86tests-sys-tests 
compattestfile,atf
 ./usr/tests/syscalltests-obsolete  
obsolete
 ./usr/tests/syscall/Atffiletests-obsolete  
obsolete
Index: tests/sys/Makefile
===
RCS file: /cvsroot/src/tests/sys/Makefile,v
retrieving revision 1.5
diff -u -p -r1.5 Makefile
--- tests/sys/Makefile  15 Oct 2020 17:44:44 -  1.5
+++ tests/sys/Makefile  16 Feb 2021 21:45:40 -
@@ -10,6 +10,7 @@ TESTS_SUBDIRS+=   netatalk
 TESTS_SUBDIRS+=netinet
 TESTS_SUBDIRS+=netinet6
 TESTS_SUBDIRS+=rc
+TESTS_SUBDIRS+=sys
 .if ${MACHINE} == amd64 || ${MACHINE} == i386
 TESTS_SUBDIRS+=x86
 .endif
Index: tests/sys/sys/Makefile
===
RCS file: tests/sys/sys/Makefile
diff -N tests/sys/sys/Makefile
--- /dev/null   1 Jan 1970 00:00:00 -
+++ tests/sys/sys/Makefile  16 Feb 2021 21:45:40 -
@@ -0,0 +1,11 @@
+# $NetBSD$
+
+TESTSDIR=  ${TESTSBASE}/sys/sys
+WARNS= 6
+
+TESTS_C=   t_pointer_aligned_p
+
+.PATH: ${NETBSDSRCDIR}/sys
+CPPFLAGS+= -I${NETBSDSRCDIR}/sys
+
+.include 
Index: tests/sys/sys/t_pointer_aligned_p.c
===
RCS file: tests/sys/sys/t_pointer_aligned_p.c
diff -N tests/sys/sys/t_pointer_aligned_p.c
--- /dev/null   1 Jan 1970 00:00:00 -
+++ tests/sys/sys/t_pointer_aligned_p.c 16 Feb 2021 21:45:40 -
@@ -0,0 +1,77 @@
+/* $NetBSD$*/
+
+/*-
+ * Copyright (c) 2021 The NetBSD Foundation, Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *notice, this list of conditions and the following disclaimer in the
+ *documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE NETBSD FOUNDATION, INC. AND CONTRIBUTORS
+ * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE FOUNDATION OR CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND 

Re: CVS commit: src/sys

2021-02-16 Thread Christos Zoulas
Yes, I agree. I am going to change that.

christos

> On Feb 16, 2021, at 1:46 PM, Roy Marples  wrote:
> 
> Hi Christos
> 
> On 14/02/2021 20:58, Christos Zoulas wrote:
>> Module Name: src
>> Committed By:christos
>> Date:Sun Feb 14 20:58:35 UTC 2021
>> Modified Files:
>>  src/sys/net: if_arp.h if_bridge.c
>>  src/sys/netinet: icmp_private.h if_arp.c igmp_var.h in_l2tp.c ip_flow.c
>>  ip_input.c ip_private.h tcp_input.c tcp_private.h udp_private.h
>>  udp_usrreq.c
>>  src/sys/netinet6: icmp6.c in6_l2tp.c ip6_flow.c ip6_input.c
>>  ip6_private.h udp6_usrreq.c
>>  src/sys/sys: mbuf.h param.h
>> Log Message:
>> - centralize header align and pullup into a single inline function
>> - use a single macro to align pointers and expose the alignment, instead
>>   of hard-coding 3 in 1/2 the macros.
>> - fix an issue in the ipv6 lt2p where it was aligning for ipv4 and pulling
>>   for ipv6.
> 
> -#ifdef __NO_STRICT_ALIGNMENT
> -#define  IP_HDR_ALIGNED_P(ip)1
> -#else
> -#define  IP_HDR_ALIGNED_P(ip)vaddr_t) (ip)) & 3) == 0)
> -#endif
> +#define  IP_HDR_ALIGNMENT3
> #endif /* _KERNEL */
> 
> While this is a like for like change, I feel that the meaning of 
> IP_HDR_ALIGNMENT is no longer clear as 3 without context makes no sense at 
> all.
> We know it should be aligned to 4 bytes.
> 
> I suggest we change POINTER_ALIGNED_P to accept the alignment value we want 
> rather than the bitwise test we supply, like so:
> 
> #define   POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)) 
> == 0)
> 
> Roy



signature.asc
Description: Message signed with OpenPGP


Re: CVS commit: src/sys

2021-02-16 Thread Roy Marples

Hi Christos

On 14/02/2021 20:58, Christos Zoulas wrote:

Module Name:src
Committed By:   christos
Date:   Sun Feb 14 20:58:35 UTC 2021

Modified Files:
src/sys/net: if_arp.h if_bridge.c
src/sys/netinet: icmp_private.h if_arp.c igmp_var.h in_l2tp.c ip_flow.c
ip_input.c ip_private.h tcp_input.c tcp_private.h udp_private.h
udp_usrreq.c
src/sys/netinet6: icmp6.c in6_l2tp.c ip6_flow.c ip6_input.c
ip6_private.h udp6_usrreq.c
src/sys/sys: mbuf.h param.h

Log Message:
- centralize header align and pullup into a single inline function
- use a single macro to align pointers and expose the alignment, instead
   of hard-coding 3 in 1/2 the macros.
- fix an issue in the ipv6 lt2p where it was aligning for ipv4 and pulling
   for ipv6.


-#ifdef __NO_STRICT_ALIGNMENT
-#defineIP_HDR_ALIGNED_P(ip)1
-#else
-#defineIP_HDR_ALIGNED_P(ip)vaddr_t) (ip)) & 3) == 0)
-#endif
+#defineIP_HDR_ALIGNMENT3
 #endif /* _KERNEL */

While this is a like for like change, I feel that the meaning of 
IP_HDR_ALIGNMENT is no longer clear as 3 without context makes no sense at all.

We know it should be aligned to 4 bytes.

I suggest we change POINTER_ALIGNED_P to accept the alignment value we want 
rather than the bitwise test we supply, like so:


#define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)) == 0)

Roy


Re: CVS commit: src/sys/netinet

2021-02-16 Thread Martin Husemann
On Tue, Feb 16, 2021 at 09:29:15AM +, Roy Marples wrote:
> In my testing on aarch64 and octeon (both of which I think are strict
> alignment) neither need pullups nor copyups as the mbuf already has enough
> and arphrd is aligned correctly already.

Ah, we asserted too much alignment - indeed, this variant works and I commited
it after testing on 32bit arm and sparc64.

Martin


Re: CVS commit: src/sys/netinet

2021-02-16 Thread Roy Marples

On 16/02/2021 09:20, Martin Husemann wrote:

On Tue, Feb 16, 2021 at 08:26:40AM +, Roy Marples wrote:

Is that because ARP_HDR_ALIGNMENT is forcing 4 byte alignment?


The KASSERT a few lines below triggerd, we need to be consistent.


For the purposes of using just the header we define I'm pretty sure we can
use 2 byte alignment and set ARP_HDR_ALIGNMENT to 1.


I can test (I have an alignment critical machine with non-ETHER_ALIGN'ing
network driver). Send me a patch, I lost track in the ongoing overhaul.


ARP_HDR_ALIGNED_P can now be removed from if_arp.c as well.


Not sure I understand what you mean here.



Index: net/if_arp.h
===
RCS file: /cvsroot/src/sys/net/if_arp.h,v
retrieving revision 1.40
diff -u -p -r1.40 if_arp.h
--- net/if_arp.h14 Feb 2021 20:58:34 -  1.40
+++ net/if_arp.h16 Feb 2021 09:26:23 -
@@ -72,7 +72,7 @@ structarphdr {
uint8_t  ar_tpa[];  /* target protocol address */
 #endif
 };
-#defineARP_HDR_ALIGNMENT   3
+#defineARP_HDR_ALIGNMENT   1

 static __inline uint8_t *
 ar_data(struct arphdr *ap)
Index: netinet/if_arp.c
===
RCS file: /cvsroot/src/sys/netinet/if_arp.c,v
retrieving revision 1.305
diff -u -p -r1.305 if_arp.c
--- netinet/if_arp.c16 Feb 2021 05:44:13 -  1.305
+++ netinet/if_arp.c16 Feb 2021 09:26:23 -
@@ -133,12 +133,6 @@ __KERNEL_RCSID(0, "$NetBSD: if_arp.c,v 1
  */
 #define ETHERTYPE_IPTRAILERS ETHERTYPE_TRAIL

-#ifdef __NO_STRICT_ALIGNMENT
-#defineARP_HDR_ALIGNED_P(ar)   1
-#else
-#defineARP_HDR_ALIGNED_P(ar)   vaddr_t) (ar)) & 1) == 0)
-#endif
-
 /* timers */
 static int arp_reachable = REACHABLE_TIME;
 static int arp_retrans = RETRANS_TIMER;


In my testing on aarch64 and octeon (both of which I think are strict alignment) 
neither need pullups nor copyups as the mbuf already has enough and arphrd is 
aligned correctly already.


Roy


Re: CVS commit: src/sys/netinet

2021-02-16 Thread Martin Husemann
On Tue, Feb 16, 2021 at 08:26:40AM +, Roy Marples wrote:
> Is that because ARP_HDR_ALIGNMENT is forcing 4 byte alignment?

The KASSERT a few lines below triggerd, we need to be consistent.

> For the purposes of using just the header we define I'm pretty sure we can
> use 2 byte alignment and set ARP_HDR_ALIGNMENT to 1.

I can test (I have an alignment critical machine with non-ETHER_ALIGN'ing
network driver). Send me a patch, I lost track in the ongoing overhaul.

> ARP_HDR_ALIGNED_P can now be removed from if_arp.c as well.

Not sure I understand what you mean here.

Martin


Re: CVS commit: src/sys/netinet

2021-02-16 Thread Roy Marples

On 16/02/2021 05:44, Martin Husemann wrote:

Module Name:src
Committed By:   martin
Date:   Tue Feb 16 05:44:14 UTC 2021

Modified Files:
src/sys/netinet: if_arp.c

Log Message:
Undo previous backout: alignment is needed here.
The reason for the previous backout was a misunderstanding (POINTER_ALIGNED_P
was broken, but the assertion fired even after it got fixed).


Is that because ARP_HDR_ALIGNMENT is forcing 4 byte alignment?
For the purposes of using just the header we define I'm pretty sure we can use 2 
byte alignment and set ARP_HDR_ALIGNMENT to 1.


ARP_HDR_ALIGNED_P can now be removed from if_arp.c as well.

Roy


Re: CVS commit: src/sys

2021-02-15 Thread David Young
On Sun, Feb 14, 2021 at 07:46:38PM +, Roy Marples wrote:
> On 13/02/2021 21:34, David Young wrote:
> > On Tue, Feb 09, 2021 at 07:02:32AM +, Roy Marples wrote:
> > > Hi David
> > > 
> > > On 03/02/2021 21:45, David Young wrote:
> > > > 
> > > > This change looks a little hasty to me.
> > > > 
> > > > It looks to me like some of these structs were __packed so that
> > > > they could be read/written directly from/to any offset in a packet
> > > > chain using mtod(), which does not pay any mind to the alignment
> > > > of `*t`:
> > > > 
> > > > #define mtod(m, t)  ((t)((m)->m_data))
> > > > 
> > > > I see gre_h is accessed in that way, just for one example.
> > > 
> > > Looking at the other places where this is handled, does this patch to 
> > > gre_h
> > > address your concerns?
> > > I tested this on aarch64 which does not define __NO_STRICT_ALIGNMENT and 
> > > it
> > > passes the KASSERT.
> > 
> > It is possible to simplify your patch a lot.  The GRE header is only 4
> > bytes long.  On the receive side, just perform the m_pullup like the
> > old code did and then memcpy to a `struct gre_h` on the stack.  On the
> > send side, construct the header on the stack and then memcpy it into the
> > mbuf.
> > 
> > The same general approach, of copying headers between mbufs the stack,
> > is probably plenty fast for virtually any size of header used in the
> > network stack.

Thank you.  I know you probably did not expect to spend so much time on
this.  I appreciate your efforts.

Dave

-- 
David Young
dyo...@pobox.comUrbana, IL(217) 721-9981


Re: CVS commit: src/lib/libc/locale

2021-02-15 Thread Joerg Sonnenberger
On Mon, Feb 15, 2021 at 04:37:15PM +0100, Thomas Klausner wrote:
> Hi!
> 
> Thanks for the man pages.
> 
> There is none for uselocale(3), which is heavily referenced from
> these, nor do I find a prototype in /usr/include...
> so how does one use these? :)

We don't support uselocale. You use this functions by passing the
locale_t to the *_l functions directly.

Joerg


Re: CVS commit: src/lib/libc/locale

2021-02-15 Thread Thomas Klausner
Hi!

Thanks for the man pages.

There is none for uselocale(3), which is heavily referenced from
these, nor do I find a prototype in /usr/include...
so how does one use these? :)
 Thomas

On Mon, Feb 15, 2021 at 09:35:04AM -0500, Christos Zoulas wrote:
> Module Name:  src
> Committed By: christos
> Date: Mon Feb 15 14:35:04 UTC 2021
> 
> Modified Files:
>   src/lib/libc/locale: Makefile.inc
> Added Files:
>   src/lib/libc/locale: duplocale.3 freelocale.3 newlocale.3
> 
> Log Message:
> Add missing man pages (from FreeBSD)
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.64 -r1.65 src/lib/libc/locale/Makefile.inc
> cvs rdiff -u -r0 -r1.1 src/lib/libc/locale/duplocale.3 \
> src/lib/libc/locale/freelocale.3 src/lib/libc/locale/newlocale.3
> 
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
> 

> Modified files:
> 
> Index: src/lib/libc/locale/Makefile.inc
> diff -u src/lib/libc/locale/Makefile.inc:1.64 
> src/lib/libc/locale/Makefile.inc:1.65
> --- src/lib/libc/locale/Makefile.inc:1.64 Sun Aug 18 16:03:48 2013
> +++ src/lib/libc/locale/Makefile.inc  Mon Feb 15 09:35:04 2021
> @@ -1,5 +1,5 @@
>  #from: @(#)Makefile.inc  5.1 (Berkeley) 2/18/91
> -#$NetBSD: Makefile.inc,v 1.64 2013/08/18 20:03:48 joerg Exp $
> +#$NetBSD: Makefile.inc,v 1.65 2021/02/15 14:35:04 christos Exp $
>  
>  # locale sources
>  .PATH: ${ARCHDIR}/locale ${.CURDIR}/locale
> @@ -21,7 +21,7 @@ CPPFLAGS.runetable.c+=  -I${LIBCDIR}/cit
>  CPPFLAGS.multibyte_c90.c+=   -I${LIBCDIR}/citrus
>  CPPFLAGS.multibyte_amd1.c+=  -I${LIBCDIR}/citrus
>  
> -MAN+=setlocale.3 nl_langinfo.3
> +MAN+=duplocale.3 freelocale.3 newlocale.3 setlocale.3 nl_langinfo.3 
>  
>  MAN+=mbtowc.3 mbstowcs.3 wctomb.3 wcstombs.3 mblen.3 \
>  
> 
> Added files:
> 
> Index: src/lib/libc/locale/duplocale.3
> diff -u /dev/null src/lib/libc/locale/duplocale.3:1.1
> --- /dev/null Mon Feb 15 09:35:04 2021
> +++ src/lib/libc/locale/duplocale.3   Mon Feb 15 09:35:04 2021
> @@ -0,0 +1,80 @@
> +.\" $NetBSD: duplocale.3,v 1.1 2021/02/15 14:35:04 christos Exp $
> +.\" Copyright (c) 2011 The FreeBSD Foundation
> +.\" All rights reserved.
> +.\"
> +.\" This documentation was written by David Chisnall under sponsorship from
> +.\" the FreeBSD Foundation.
> +.\"
> +.\" Redistribution and use in source and binary forms, with or without
> +.\" modification, are permitted provided that the following conditions
> +.\" are met:
> +.\" 1. Redistributions of source code must retain the above copyright
> +.\"notice, this list of conditions and the following disclaimer.
> +.\" 2. Redistributions in binary form must reproduce the above copyright
> +.\"notice, this list of conditions and the following disclaimer in the
> +.\"documentation and/or other materials provided with the distribution.
> +.\"
> +.\" THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND
> +.\" ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> +.\" IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR 
> PURPOSE
> +.\" ARE DISCLAIMED.  IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
> +.\" FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR 
> CONSEQUENTIAL
> +.\" DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> +.\" OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> +.\" HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, 
> STRICT
> +.\" LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> +.\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> +.\" SUCH DAMAGE.
> +.\"
> +.\" $FreeBSD: head/lib/libc/locale/duplocale.3 281925 2015-04-24 10:17:55Z 
> theraven $
> +.\"
> +.Dd September 17, 2011
> +.Dt DUPLOCALE 3
> +.Os
> +.Sh NAME
> +.Nm duplocale
> +.Nd duplicate an locale
> +.Sh LIBRARY
> +.Lb libc
> +.Sh SYNOPSIS
> +.In locale.h
> +.Ft locale_t
> +.Fn duplocale "locale_t locale"
> +.Sh DESCRIPTION
> +Duplicates an existing
> +.Fa locale_t
> +returning a new
> +.Fa locale_t
> +that refers to the same locale values but has an independent internal state.
> +Various functions, such as
> +.Xr mblen 3
> +require a persistent state.
> +These functions formerly used static variables and calls to them from 
> multiple
> +threads had undefined behavior.
> +They now use fields in the
> +.Fa locale_t
> +associated with the current thread by
> +.Xr uselocale 3 .
> +These calls are therefore only thread safe on threads with a unique 
> per-thread
> +locale.
> +The locale returned by this call must be freed with
> +.Xr freelocale 3 .
> +.Sh SEE ALSO
> +.Xr freelocale 3 ,
> +.Xr localeconv 3 ,
> +.Xr newlocale 3 ,
> +.\" .Xr querylocale 3 ,
> +.Xr uselocale 3 ,
> +.\" .Xr xlocale 3
> +.Sh STANDARDS
> +This function conforms to
> +.St -p1003.1-2008 .
> +.Sh BUGS
> +Ideally,
> +.Xr uselocale 3
> +should make a 

Re: CVS commit: src/sys

2021-02-14 Thread Roy Marples

On 13/02/2021 21:34, David Young wrote:

On Tue, Feb 09, 2021 at 07:02:32AM +, Roy Marples wrote:

Hi David

On 03/02/2021 21:45, David Young wrote:


This change looks a little hasty to me.

It looks to me like some of these structs were __packed so that
they could be read/written directly from/to any offset in a packet
chain using mtod(), which does not pay any mind to the alignment
of `*t`:

#define mtod(m, t)  ((t)((m)->m_data))

I see gre_h is accessed in that way, just for one example.


Looking at the other places where this is handled, does this patch to gre_h
address your concerns?
I tested this on aarch64 which does not define __NO_STRICT_ALIGNMENT and it
passes the KASSERT.


It is possible to simplify your patch a lot.  The GRE header is only 4
bytes long.  On the receive side, just perform the m_pullup like the
old code did and then memcpy to a `struct gre_h` on the stack.  On the
send side, construct the header on the stack and then memcpy it into the
mbuf.

The same general approach, of copying headers between mbufs the stack,
is probably plenty fast for virtually any size of header used in the
network stack.


Done


Re: CVS commit: src/sys/net

2021-02-14 Thread Roy Marples

On 13/02/2021 14:19, Jonathan A. Kollasch wrote:

On Sat, Feb 13, 2021 at 07:28:05AM +, Roy Marples wrote:

Module Name:src
Committed By:   roy
Date:   Sat Feb 13 07:28:05 UTC 2021

Modified Files:
src/sys/net: if_ether.h if_ethersubr.c

Log Message:
if_ether: Ensure that ether_header is aligned


To generate a diff of this commit:
cvs rdiff -u -r1.84 -r1.85 src/sys/net/if_ether.h
cvs rdiff -u -r1.289 -r1.290 src/sys/net/if_ethersubr.c


This appears to ensure the Ethernet header is naturally aligned on a
32-bit boundary.  The 16-bit ether_type field is the only thing in
ether_header that is wider than a uint8_t.

Many drivers will place the start of the receive buffer at an
ETHER_ALIGN (which is 2) offset to ensure the layer three header
is 32-bit aligned after the 14-byte Ethernet header.  Thus this
will result in always calling m_copyup() in ether_input() on strict
alignment platforms.


Reverted


Re: CVS commit: src/sys

2021-02-13 Thread David Young
On Tue, Feb 09, 2021 at 07:02:32AM +, Roy Marples wrote:
> Hi David
> 
> On 03/02/2021 21:45, David Young wrote:
> >
> > This change looks a little hasty to me.
> >
> > It looks to me like some of these structs were __packed so that
> > they could be read/written directly from/to any offset in a packet
> > chain using mtod(), which does not pay any mind to the alignment
> > of `*t`:
> >
> > #define mtod(m, t)  ((t)((m)->m_data))
> >
> > I see gre_h is accessed in that way, just for one example.
> 
> Looking at the other places where this is handled, does this patch to gre_h
> address your concerns?
> I tested this on aarch64 which does not define __NO_STRICT_ALIGNMENT and it
> passes the KASSERT.

It is possible to simplify your patch a lot.  The GRE header is only 4
bytes long.  On the receive side, just perform the m_pullup like the
old code did and then memcpy to a `struct gre_h` on the stack.  On the
send side, construct the header on the stack and then memcpy it into the
mbuf.

The same general approach, of copying headers between mbufs the stack,
is probably plenty fast for virtually any size of header used in the
network stack.

Dave

-- 
David Young
dyo...@pobox.comUrbana, IL(217) 721-9981


Re: CVS commit: src

2021-02-13 Thread Roland Illig

On 13.02.2021 17:20, Valery Ushakov wrote:

Thanks for working on this, but a couple of nitpicks:

On Sat, Feb 13, 2021 at 14:30:37 +, Roland Illig wrote:


In sysinst, the installation screen is indented with tabs.  Sysinst uses
msgc, which brings its own text layout engine.  This engine does not use
addbytes but addch.

[...]

This bug largely went unnoticed because most other applications use
addbytes instead, which worked fine all the time.


That doesn't make much sense b/c addbytes is not even a stadnard
interface.  It's an internal low-level function.


Ah, that explains why there is no manual page for addbytes.  I
double-checked the code, and internally the functions that operate on
plain char call addbytes eventually.


It had been introduced somewhere between NetBSD 8.0 and NetBSD 9.0.


As I have pointed out, this was introduced in:

revision 1.44
date: 2016-11-28 21:25:26 +0300;  author: christos;  state: Exp;  lines: +8 -4; 
 commitid: MHbzU41X9arGoVvz;
If we are inserting spaces to account for a tab, move the x position of the
cursor, otherwise this is a no-op (Carsten Kunze)


Unlike the file's content the commit logs are uneditable, so please,
try to make them accurate.  curses is a huge mess already as it is
(the standard API as such, not (just) our implementation) and etching
in stone potentially misleading information doesn't help...


Thanks for the advice, I will do that.


Re: CVS commit: src

2021-02-13 Thread Roland Illig

On 13.02.2021 20:00, Valery Ushakov wrote:

On Sat, Feb 13, 2021 at 14:30:37 +, Roland Illig wrote:


Libcurses can be built in 2 modes: with wide character support or
without (-DDISABLE_WCHAR).  The test suite only covers the variant with
wide characters.  The single-byte variant has to be tested manually.
Running sysinst with the single-byte libcurses produces the correct
layout.


Also, wide char support extends the original curses api, so the test
suite that covers the old api plus the new api obviously covers the
old api (though I don't think the test suite detects if the curses it
tests actually has wide char support, so it might fail to run, but
that's a different problem).


That's exactly what I wanted to say:  The libcurses test suite does not
detect whether libcurses supports wchar_t.  It just fails to link if
libcurses does not support the wchar_t API.

What I meant is that there are some #ifdef HAVE_WCHAR_T ... #else ...
#endif.  The #else part cannot be covered by the current test suite.  So
the API may be covered, but the implementation isn't.  Therefore the
manual test.


Re: CVS commit: src

2021-02-13 Thread Valery Ushakov
On Sat, Feb 13, 2021 at 14:30:37 +, Roland Illig wrote:

> Libcurses can be built in 2 modes: with wide character support or
> without (-DDISABLE_WCHAR).  The test suite only covers the variant with
> wide characters.  The single-byte variant has to be tested manually.
> Running sysinst with the single-byte libcurses produces the correct
> layout.

Also, wide char support extends the original curses api, so the test
suite that covers the old api plus the new api obviously covers the
old api (though I don't think the test suite detects if the curses it
tests actually has wide char support, so it might fail to run, but
that's a different problem).

-uwe


Re: CVS commit: src

2021-02-13 Thread Valery Ushakov
Thanks for working on this, but a couple of nitpicks:

On Sat, Feb 13, 2021 at 14:30:37 +, Roland Illig wrote:

> In sysinst, the installation screen is indented with tabs.  Sysinst uses
> msgc, which brings its own text layout engine.  This engine does not use
> addbytes but addch.
[...]
> This bug largely went unnoticed because most other applications use
> addbytes instead, which worked fine all the time.

That doesn't make much sense b/c addbytes is not even a stadnard
interface.  It's an internal low-level function.


> It had been introduced somewhere between NetBSD 8.0 and NetBSD 9.0.

As I have pointed out, this was introduced in:

revision 1.44
date: 2016-11-28 21:25:26 +0300;  author: christos;  state: Exp;  lines: +8 -4; 
 commitid: MHbzU41X9arGoVvz;
If we are inserting spaces to account for a tab, move the x position of the
cursor, otherwise this is a no-op (Carsten Kunze)


Unlike the file's content the commit logs are uneditable, so please,
try to make them accurate.  curses is a huge mess already as it is
(the standard API as such, not (just) our implementation) and etching
in stone potentially misleading information doesn't help...


-uwe


Re: CVS commit: src/sys/net

2021-02-13 Thread Jonathan A. Kollasch
On Sat, Feb 13, 2021 at 07:28:05AM +, Roy Marples wrote:
> Module Name:  src
> Committed By: roy
> Date: Sat Feb 13 07:28:05 UTC 2021
> 
> Modified Files:
>   src/sys/net: if_ether.h if_ethersubr.c
> 
> Log Message:
> if_ether: Ensure that ether_header is aligned
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.84 -r1.85 src/sys/net/if_ether.h
> cvs rdiff -u -r1.289 -r1.290 src/sys/net/if_ethersubr.c

This appears to ensure the Ethernet header is naturally aligned on a
32-bit boundary.  The 16-bit ether_type field is the only thing in
ether_header that is wider than a uint8_t.

Many drivers will place the start of the receive buffer at an
ETHER_ALIGN (which is 2) offset to ensure the layer three header
is 32-bit aligned after the 14-byte Ethernet header.  Thus this
will result in always calling m_copyup() in ether_input() on strict
alignment platforms.


Re: CVS commit: src/sys

2021-02-08 Thread Roy Marples

Hi David

On 03/02/2021 21:45, David Young wrote:
>
> This change looks a little hasty to me.
>
> It looks to me like some of these structs were __packed so that
> they could be read/written directly from/to any offset in a packet
> chain using mtod(), which does not pay any mind to the alignment
> of `*t`:
>
> #define mtod(m, t)  ((t)((m)->m_data))
>
> I see gre_h is accessed in that way, just for one example.

Looking at the other places where this is handled, does this patch to gre_h 
address your concerns?
I tested this on aarch64 which does not define __NO_STRICT_ALIGNMENT and it 
passes the KASSERT.


Roy

diff -r e3c82b1d9c2e sys/net/if_gre.c
--- a/sys/net/if_gre.c  Mon Feb 08 01:00:49 2021 +
+++ b/sys/net/if_gre.c  Tue Feb 09 06:55:44 2021 +
@@ -395,10 +395,26 @@
sc->sc_error_ev.ev_count++;
return;
}
-   if (m->m_len < sizeof(*gh) && (m = m_pullup(m, sizeof(*gh))) == NULL) {
-   GRE_DPRINTF(sc, "m_pullup failed\n");
-   sc->sc_pullup_ev.ev_count++;
-   return;
+
+   /* If the GRE header is not aligned, slurp it up into a new
+* mbuf with space for link headers, in the event we forward
+* it.  Otherwise, if it is aligned, make sure the entire
+* base GRE header is in the first mbuf of the chain.
+*/
+   if (GRE_HDR_ALIGNED_P(mtod(m, void *)) == 0) {
+   if ((m = m_copyup(m, sizeof(struct gre_h),
+   (max_linkhdr + 3) & ~3)) == NULL) {
+   /* XXXJRT new stat, please */
+   GRE_DPRINTF(sc, "m_copyup failed\n");
+   sc->sc_pullup_ev.ev_count++;
+   return;
+   }
+   } else if (__predict_false(m->m_len < sizeof(struct gre_h))) {
+   if ((m = m_pullup(m, sizeof(struct gre_h))) == NULL) {
+   GRE_DPRINTF(sc, "m_pullup failed\n");
+   sc->sc_pullup_ev.ev_count++;
+   return;
+   }
}
gh = mtod(m, const struct gre_h *);

@@ -940,7 +956,6 @@
 #endif

M_PREPEND(m, sizeof(*gh), M_DONTWAIT);
-
if (m == NULL) {
IF_DROP(>if_snd);
error = ENOBUFS;
@@ -948,6 +963,7 @@
}

gh = mtod(m, struct gre_h *);
+   KASSERT(GRE_HDR_ALIGNED_P(gh));
gh->flags = 0;
gh->ptype = etype;
/* XXX Need to handle IP ToS.  Look at how I handle IP TTL. */
diff -r e3c82b1d9c2e sys/net/if_gre.h
--- a/sys/net/if_gre.h  Mon Feb 08 01:00:49 2021 +
+++ b/sys/net/if_gre.h  Tue Feb 09 06:55:44 2021 +
@@ -131,6 +131,11 @@
Present if (rt_pres == 1)
  */
 };
+#ifdef __NO_STRICT_ALIGNMENT
+#defineGRE_HDR_ALIGNED_P(gh)   1
+#else
+#defineGRE_HDR_ALIGNED_P(gh)   vaddr_t) (gh)) & 3) == 0)
+#endif
 #ifdef __CTASSERT
 __CTASSERT(sizeof(struct gre_h) == 4);
 #endif


Re: CVS commit: src/share/man/man9

2021-02-08 Thread Tetsuya Isaki
At Sun, 7 Feb 2021 09:22:39 +,
nia wrote:
> > > -It is called at any time.
> > > +It can be called at any time.
> > 
> > The later sounds to me "You(developer of MD driver) can call
> > it at any time".  If so, it's incorrect.
> 
> Maybe "it can be called by the MI layer at any time" is clearer
> here, then? I can change it to that.

That's true, but sounds a bit redundant.
Because these all are callback functions called by the MI layer.

Is there any better text?
The MI layer can(will?) call this in the Opened phase or in the
Closed phase, or even in the Attach phase.  This means, for example,
that you (MD driver) cannot assume that you can prepare(initialize)
something in open() before this (since this can be called in the
Closed phase), and you cannot assume that it has returned from MI
attach (since this can be called in the Attach phase).

> > Is "only" a typo?  or is it better to remove it in English?
> 
> I think it's clear that conversion of other formats is not
> supported by the rest of the paragraph, so it doesn't need to
> be mentioned here, where the primary purpose of the sentence
> is to explain why you don't need to handle conversion in that
> case yourself.

If it's clear for readers, no problem to me.

Thanks,
---
Tetsuya Isaki 


Re: CVS commit: src/sys

2021-02-07 Thread Roy Marples

On 04/02/2021 20:18, matthew green wrote:

Roy Marples writes:

On 03/02/2021 21:45, David Young wrote:

On Wed, Feb 03, 2021 at 05:51:40AM +, Roy Marples wrote:

Module Name:src
Committed By:   roy
Date:   Wed Feb  3 05:51:40 UTC 2021

Modified Files:
src/sys/net: if_arp.h if_ether.h if_gre.h
src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h

Log Message:
Remove __packed from various network structures

They are already network aligned and adding the __packed attribute
just causes needless compiler warnings about accssing members of packed
objects.


This change looks a little hasty to me.

It looks to me like some of these structs were __packed so that
they could be read/written directly from/to any offset in a packet
chain using mtod(), which does not pay any mind to the alignment
of `*t`:

#define mtod(m, t)  ((t)((m)->m_data))

I see gre_h is accessed in that way, just for one example.  I don't
see any reason in principle that every gre_h accessed through mtod()
should be 16-bit aligned in its buffer, but that is the alignment
the compiler will expect if gre_h is not __packed.  If the actual
alignment ever differs from compiler's expectation, then there
could be a bus error or an unwanted byte rotation/shift.

It looks to me like there's a bit of cleanup to do elsewhere before
removing __packed from network structures.


ssh over a gre tunnel using erlite (mips64) and pinebook (aarch64) as both ends
seems to work fine. I also tested an amd64 endpoint.

Not that I disagree with your assessment that the code can always be improved.


i looked at removing __packed from these when GCC 9 came
around and really started complaining about them.  however,
i was not able to convince myself that all the users were
actually safe if __packed was removed.

in particular, 'struct ip' has 4-byte objects at offset
14, 18, 22, and 26.  code accessing data directly from the
network may fail, and eg, mtod() makes it virtually
impossible to check for this at compile time.  sanitizers
could check at run time.


Isn't that already solved here?
http://anonhg.netbsd.org/src/rev/9f66cecd950e

And if I'm not wrong, just ignoring the warning causes alignment failures as it 
stands? So it's just swapping one bad thing with another? If so, then there is 
no right answer other than doing similar alignment checks for our mbufs.


Roy


Re: CVS commit: src/share/man/man9

2021-02-07 Thread nia
On Sun, Feb 07, 2021 at 12:43:40PM +0900, Tetsuya Isaki wrote:
> > @@ -175,9 +175,9 @@
> >  .Vt audio_format_t
> >  structure according to given number
> >  .Va afp->index .
> > -If there is no format with given number, return
> > +If there is no format with the given number, return
> >  .Er EINVAL .
> > -It is called at any time.
> > +It can be called at any time.
> 
> The later sounds to me "You(developer of MD driver) can call
> it at any time".  If so, it's incorrect.

Maybe "it can be called by the MI layer at any time" is clearer
here, then? I can change it to that.

> 
> >  Similarly, if the driver supports
> >  .Dv SLINEAR_OE:16
> >  and the upper layer chooses it,
> > -the driver does not need to provide a conversion function.
> > -Because the upper layer only supports conversion between
> > +the driver does not need to provide a conversion function,
> > +because the upper layer supports conversion between
> 
> Is "only" a typo?  or is it better to remove it in English?
> 
> Thanks,
> ---
> Tetsuya Isaki 

I think it's clear that conversion of other formats is not
supported by the rest of the paragraph, so it doesn't need to
be mentioned here, where the primary purpose of the sentence
is to explain why you don't need to handle conversion in that
case yourself.

That's just my opinion, though - I'm not an English expert, there's
lots of rules I know but cannot explain.

Thanks,

Nia


Re: CVS commit: src/share/man/man9

2021-02-06 Thread Tetsuya Isaki
Hello,

At Sat, 6 Feb 2021 13:55:40 +,
Nia Alarie wrote:
> Module Name:  src
> Committed By: nia
> Date: Sat Feb  6 13:55:40 UTC 2021
> 
> Modified Files:
>   src/share/man/man9: audio.9
> 
> Log Message:
> Fix various typos, etc
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.59 -r1.60 src/share/man/man9/audio.9

> @@ -175,9 +175,9 @@
>  .Vt audio_format_t
>  structure according to given number
>  .Va afp->index .
> -If there is no format with given number, return
> +If there is no format with the given number, return
>  .Er EINVAL .
> -It is called at any time.
> +It can be called at any time.

The later sounds to me "You(developer of MD driver) can call
it at any time".  If so, it's incorrect.

>  Similarly, if the driver supports
>  .Dv SLINEAR_OE:16
>  and the upper layer chooses it,
> -the driver does not need to provide a conversion function.
> -Because the upper layer only supports conversion between
> +the driver does not need to provide a conversion function,
> +because the upper layer supports conversion between

Is "only" a typo?  or is it better to remove it in English?

Thanks,
---
Tetsuya Isaki 


Re: CVS commit: src/sys

2021-02-04 Thread Joerg Sonnenberger
On Thu, Feb 04, 2021 at 09:07:06PM +, Jaromir Dolecek wrote:
> Module Name:  src
> Committed By: jdolecek
> Date: Thu Feb  4 21:07:06 UTC 2021
> 
> Modified Files:
>   src/sys/kern: vfs_init.c vfs_subr.c
>   src/sys/sys: mount.h
> 
> Log Message:
> introduce vfs.generic.timestamp_precision sysctl to control precision
> of the timer used for vfs_timestamp(); default stays the same
> to use nanotime(9), but option is there to use the faster, albeit
> less precise methods

Most of this seems to be misguided at best. I mean it makes sense to
have a variant of getnanotime vs nanotime to avoid touching the
timecounter hardware. But the rest is just begging for hard to debug bug
reports for little to no gain. Especially limiting it to microseconds
doesn't buy anything on this level really.

Joerg


re: CVS commit: src/sys

2021-02-04 Thread matthew green
Roy Marples writes:
> On 03/02/2021 21:45, David Young wrote:
> > On Wed, Feb 03, 2021 at 05:51:40AM +, Roy Marples wrote:
> >> Module Name:   src
> >> Committed By:  roy
> >> Date:  Wed Feb  3 05:51:40 UTC 2021
> >>
> >> Modified Files:
> >>src/sys/net: if_arp.h if_ether.h if_gre.h
> >>src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
> >>ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h
> >>
> >> Log Message:
> >> Remove __packed from various network structures
> >>
> >> They are already network aligned and adding the __packed attribute
> >> just causes needless compiler warnings about accssing members of packed
> >> objects.
> > 
> > This change looks a little hasty to me.
> > 
> > It looks to me like some of these structs were __packed so that
> > they could be read/written directly from/to any offset in a packet
> > chain using mtod(), which does not pay any mind to the alignment
> > of `*t`:
> > 
> > #define mtod(m, t)  ((t)((m)->m_data))
> > 
> > I see gre_h is accessed in that way, just for one example.  I don't
> > see any reason in principle that every gre_h accessed through mtod()
> > should be 16-bit aligned in its buffer, but that is the alignment
> > the compiler will expect if gre_h is not __packed.  If the actual
> > alignment ever differs from compiler's expectation, then there
> > could be a bus error or an unwanted byte rotation/shift.
> > 
> > It looks to me like there's a bit of cleanup to do elsewhere before
> > removing __packed from network structures.
>
> ssh over a gre tunnel using erlite (mips64) and pinebook (aarch64) as both 
> ends 
> seems to work fine. I also tested an amd64 endpoint.
>
> Not that I disagree with your assessment that the code can always be improved.

i looked at removing __packed from these when GCC 9 came
around and really started complaining about them.  however,
i was not able to convince myself that all the users were
actually safe if __packed was removed.

in particular, 'struct ip' has 4-byte objects at offset
14, 18, 22, and 26.  code accessing data directly from the
network may fail, and eg, mtod() makes it virtually
impossible to check for this at compile time.  sanitizers
could check at run time.

i really support the _idea_ of this change, but i'm pretty
far from convinced we're ready to make it for all these
types, and we won't know unles the code actually exercises
these paths (ie, we may find a crash in years in some error
path that uses misaligned accesses.)

removing __packed as much as possible, replacing it with
specific __aligned(n) where necessary, are both goals that
i'm strongly in favour of.  eg, we could perhaps avoid the
'struct ip' issue mentioned above if we mark both the
ip_src and ip_dst members as __aligned(2).

.. but i remain unsure and worried about this change
without significant testing or code reading.


.mrg.


Re: CVS commit: src/sys/arch/hppa/gsc

2021-02-04 Thread Michael
Hello,

On Fri, 05 Feb 2021 00:12:35 +0900
Tetsuya Isaki  wrote:

> At Wed, 3 Feb 2021 13:37:24 -0500,
> Michael wrote:
> > > Committed By: isaki
> > > Date: Wed Feb  3 15:13:49 UTC 2021
> > > 
> > > Modified Files:
> > >   src/sys/arch/hppa/gsc: harmony.c
> > > 
> > > Log Message:
> > > Fix locking against myself.
> > > trigger_output will be called with sc_intr_lock held.
> > > From source code review, not tested.  
> > 
> > I just tested this on my C200 - playback works.  
> 
> Thank you for quick response!
> 
> > The sample rate seems off - everything plays too slow, but that's
> > probably completely unrelated.  
> 
> Oops, this was my mistake.
> I hope that harmony.c,v 1.9 will fix this problem.

Works perfectly now, thank you!

have fun
Michael



Re: CVS commit: src/sys

2021-02-04 Thread Roy Marples

On 03/02/2021 21:45, David Young wrote:

On Wed, Feb 03, 2021 at 05:51:40AM +, Roy Marples wrote:

Module Name:src
Committed By:   roy
Date:   Wed Feb  3 05:51:40 UTC 2021

Modified Files:
src/sys/net: if_arp.h if_ether.h if_gre.h
src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h

Log Message:
Remove __packed from various network structures

They are already network aligned and adding the __packed attribute
just causes needless compiler warnings about accssing members of packed
objects.


This change looks a little hasty to me.

It looks to me like some of these structs were __packed so that
they could be read/written directly from/to any offset in a packet
chain using mtod(), which does not pay any mind to the alignment
of `*t`:

#define mtod(m, t)  ((t)((m)->m_data))

I see gre_h is accessed in that way, just for one example.  I don't
see any reason in principle that every gre_h accessed through mtod()
should be 16-bit aligned in its buffer, but that is the alignment
the compiler will expect if gre_h is not __packed.  If the actual
alignment ever differs from compiler's expectation, then there
could be a bus error or an unwanted byte rotation/shift.

It looks to me like there's a bit of cleanup to do elsewhere before
removing __packed from network structures.


ssh over a gre tunnel using erlite (mips64) and pinebook (aarch64) as both ends 
seems to work fine. I also tested an amd64 endpoint.


Not that I disagree with your assessment that the code can always be improved.

Roy


Re: CVS commit: src/sys/arch/hppa/gsc

2021-02-04 Thread Tetsuya Isaki
Hello,

At Wed, 3 Feb 2021 13:37:24 -0500,
Michael wrote:
> > Committed By:   isaki
> > Date:   Wed Feb  3 15:13:49 UTC 2021
> > 
> > Modified Files:
> > src/sys/arch/hppa/gsc: harmony.c
> > 
> > Log Message:
> > Fix locking against myself.
> > trigger_output will be called with sc_intr_lock held.
> > From source code review, not tested.
> 
> I just tested this on my C200 - playback works.

Thank you for quick response!

> The sample rate seems off - everything plays too slow, but that's
> probably completely unrelated.

Oops, this was my mistake.
I hope that harmony.c,v 1.9 will fix this problem.
Sorry for breaking it.
---
Tetsuya Isaki 



Re: CVS commit: src/sys

2021-02-03 Thread David Young
On Wed, Feb 03, 2021 at 05:51:40AM +, Roy Marples wrote:
> Module Name:  src
> Committed By: roy
> Date: Wed Feb  3 05:51:40 UTC 2021
> 
> Modified Files:
>   src/sys/net: if_arp.h if_ether.h if_gre.h
>   src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
>   ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h
> 
> Log Message:
> Remove __packed from various network structures
> 
> They are already network aligned and adding the __packed attribute
> just causes needless compiler warnings about accssing members of packed
> objects.

This change looks a little hasty to me.

It looks to me like some of these structs were __packed so that
they could be read/written directly from/to any offset in a packet
chain using mtod(), which does not pay any mind to the alignment 
of `*t`:

#define mtod(m, t)  ((t)((m)->m_data))

I see gre_h is accessed in that way, just for one example.  I don't
see any reason in principle that every gre_h accessed through mtod()
should be 16-bit aligned in its buffer, but that is the alignment
the compiler will expect if gre_h is not __packed.  If the actual
alignment ever differs from compiler's expectation, then there
could be a bus error or an unwanted byte rotation/shift.

It looks to me like there's a bit of cleanup to do elsewhere before
removing __packed from network structures.

Dave

-- 
David Young
dyo...@pobox.comUrbana, IL(217) 721-9981


Re: CVS commit: src/sys

2021-02-03 Thread Kamil Rytarowski
On 03.02.2021 16:52, Roy Marples wrote:
> But you *have* to interogate the headers in order to work this out.

I noted how this could break. I'm right now not affected by this myself.
Please monitor gnats for reports.

I defer any decisions and discussions (if any) now to others.


Re: CVS commit: src/sys/arch/hppa/gsc

2021-02-03 Thread Michael
Hello,

On Wed, 3 Feb 2021 15:13:49 +
"Tetsuya Isaki"  wrote:

> Module Name:  src
> Committed By: isaki
> Date: Wed Feb  3 15:13:49 UTC 2021
> 
> Modified Files:
>   src/sys/arch/hppa/gsc: harmony.c
> 
> Log Message:
> Fix locking against myself.
> trigger_output will be called with sc_intr_lock held.
> From source code review, not tested.

I just tested this on my C200 - playback works.
The sample rate seems off - everything plays too slow, but that's
probably completely unrelated.

have fun
Michael


Re: CVS commit: src/sys

2021-02-03 Thread Roy Marples

On 03/02/2021 12:54, Kamil Rytarowski wrote:

This is still a valid usage and ABI breakage for userland. You cannot
blame a user for using system structures and headers that stop working
after an upgrade, at least before at least libc version bump.

For the record, I broke ABI here (it was the reverse situation, addition
of __packed).

https://github.com/NetBSD/src/commit/a833bd5cfdba983ecb5560512a3547f46f35f11e

I vote to revert and handling these structures with appropriate
functions that are aware of potentially misaligned data operations.

If we you or the project resist and insists on ABI breakage, it should
be boldly documented.


I don't buy this argument as we frequently do this:

r1.1
struct aaa {
uint16 a;
};


r1.2
struct oaaa {
uint16 a;
};

struct aaa {
uint16 a;
uint8 b;
};

Now if you choose to put your own stuff before and after in another struct, 
frankly you are really on your own.


Here's another example:

struct ehteripudp {
struct etherhdr;
struct iphdr;
struct udphdr;
};

This still works!
However, it's potentially broken as you are making the assumption there are no 
options after the ip header.


Infact this is how all the network headers have been designed.
From the start of the structure you can chain header structures together until 
you either reach options or data.


But you *have* to interogate the headers in order to work this out.

Roy


Re: CVS commit: src/sys

2021-02-03 Thread Joerg Sonnenberger
On Wed, Feb 03, 2021 at 11:03:25AM +0100, Kamil Rytarowski wrote:
> On 03.02.2021 06:51, Roy Marples wrote:
> > Module Name:src
> > Committed By:   roy
> > Date:   Wed Feb  3 05:51:40 UTC 2021
> > 
> > Modified Files:
> > src/sys/net: if_arp.h if_ether.h if_gre.h
> > src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
> > ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h
> > 
> > Log Message:
> > Remove __packed from various network structures
> > 
> > They are already network aligned and adding the __packed attribute
> > just causes needless compiler warnings about accssing members of packed
> > objects.
> > 
> 
> This changes the ABI for userland programs. With __packed, these
> structures, whenever contain at least 2-byte data, can be placed in a
> different location inside another structure now.

It doesn't break any of *our* ABI contracts. I also refuse us to be taken
hostage here for any (hypothetical) user program exposing internals of
the network stack on ABI boundaries. There is a very real price for the
__packed and *any* fix for the misuse has the same ABI impacts.

Joerg


Re: CVS commit: src/sys

2021-02-03 Thread Kamil Rytarowski
On 03.02.2021 13:39, Roy Marples wrote:
> On 03/02/2021 10:03, Kamil Rytarowski wrote:
>> On 03.02.2021 06:51, Roy Marples wrote:
>>> Module Name:    src
>>> Committed By:    roy
>>> Date:    Wed Feb  3 05:51:40 UTC 2021
>>>
>>> Modified Files:
>>> src/sys/net: if_arp.h if_ether.h if_gre.h
>>> src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h
>>> ip_icmp.h
>>>     ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h
>>>
>>> Log Message:
>>> Remove __packed from various network structures
>>>
>>> They are already network aligned and adding the __packed attribute
>>> just causes needless compiler warnings about accssing members of packed
>>> objects.
>>>
>>
>> This changes the ABI for userland programs. With __packed, these
>> structures, whenever contain at least 2-byte data, can be placed in a
>> different location inside another structure now.
>>
>> #include 
>> #include 
>> #include 
>> #include 
>>
>> struct aaa {
>>  uint16_t a;
>>  uint8_t b;
>>  uint8_t c;
>> } __packed;
>>
>> struct aaa2 {
>>  uint8_t x;
>>  struct aaa y;
>> };
>>
>> struct bbb {
>>  uint16_t a;
>>  uint8_t b;
>>  uint8_t c;
>> };
>>
>> struct bbb2 {
>>  uint8_t x;
>>  struct bbb y;
>> };
> 
> Assuming that struct aaa and bbb are from NetBSD headers and aaa2 and
> bbb2 are your own constructs then you just have yourself to blame.
> 

It is a valid code to contain packed data in a structure without the
packed attribute.

> struct bbb2 {
>   uint8_t x;
>   struct bbb y;
> } __packed;
> 
> Makes bbb2 the same as aaa2.
> 
>> Before I saw your commit, I wanted to ask to revert the following
>> changes:
>>
>> icmp6: Remove __packed attribute from icmp6 structures
>>
>> https://github.com/NetBSD/src/commit/427831ba4bdf388aecf3a378de8faf3a4d44a462
>>
>>
>> ip6: Remove __packed attribute from ip6 structures
>>
>> https://github.com/NetBSD/src/commit/e82879afd70b0e801e6ee53bd14c27be3dd1738f
>>
>>
>> The fallout can be subtle and hard to debug. Once, I removed __packed
>> from one innocent networking structure myself, qemu networking stopped
>> working at all.
> 
> How you use the structure is up to you.
> For the record, we were the only BSD to ever apply __packed to these
> structures and thanks to modern compilers emitting these wonderful
> warnings it proved to be a bad move.
> 

This is still a valid usage and ABI breakage for userland. You cannot
blame a user for using system structures and headers that stop working
after an upgrade, at least before at least libc version bump.

For the record, I broke ABI here (it was the reverse situation, addition
of __packed).

https://github.com/NetBSD/src/commit/a833bd5cfdba983ecb5560512a3547f46f35f11e

I vote to revert and handling these structures with appropriate
functions that are aware of potentially misaligned data operations.

If we you or the project resist and insists on ABI breakage, it should
be boldly documented.

> Roy



Re: CVS commit: src/sys

2021-02-03 Thread Roy Marples

On 03/02/2021 10:03, Kamil Rytarowski wrote:

On 03.02.2021 06:51, Roy Marples wrote:

Module Name:src
Committed By:   roy
Date:   Wed Feb  3 05:51:40 UTC 2021

Modified Files:
src/sys/net: if_arp.h if_ether.h if_gre.h
src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h

Log Message:
Remove __packed from various network structures

They are already network aligned and adding the __packed attribute
just causes needless compiler warnings about accssing members of packed
objects.



This changes the ABI for userland programs. With __packed, these
structures, whenever contain at least 2-byte data, can be placed in a
different location inside another structure now.

#include 
#include 
#include 
#include 

struct aaa {
 uint16_t a;
 uint8_t b;
 uint8_t c;
} __packed;

struct aaa2 {
 uint8_t x;
 struct aaa y;
};

struct bbb {
 uint16_t a;
 uint8_t b;
 uint8_t c;
};

struct bbb2 {
 uint8_t x;
 struct bbb y;
};


Assuming that struct aaa and bbb are from NetBSD headers and aaa2 and bbb2 are 
your own constructs then you just have yourself to blame.


struct bbb2 {
  uint8_t x;
  struct bbb y;
} __packed;

Makes bbb2 the same as aaa2.


Before I saw your commit, I wanted to ask to revert the following changes:

icmp6: Remove __packed attribute from icmp6 structures

https://github.com/NetBSD/src/commit/427831ba4bdf388aecf3a378de8faf3a4d44a462

ip6: Remove __packed attribute from ip6 structures

https://github.com/NetBSD/src/commit/e82879afd70b0e801e6ee53bd14c27be3dd1738f

The fallout can be subtle and hard to debug. Once, I removed __packed
from one innocent networking structure myself, qemu networking stopped
working at all.


How you use the structure is up to you.
For the record, we were the only BSD to ever apply __packed to these structures 
and thanks to modern compilers emitting these wonderful warnings it proved to be 
a bad move.


Roy


Re: CVS commit: src/sys

2021-02-03 Thread Roy Marples

On 03/02/2021 08:34, Joerg Sonnenberger wrote:

On Wed, Feb 03, 2021 at 05:51:40AM +, Roy Marples wrote:

Module Name:src
Committed By:   roy
Date:   Wed Feb  3 05:51:40 UTC 2021

Modified Files:
src/sys/net: if_arp.h if_ether.h if_gre.h
src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h

Log Message:
Remove __packed from various network structures

They are already network aligned and adding the __packed attribute
just causes needless compiler warnings about accssing members of packed
objects.


Please add a compile-time assert at least for _KERNEL that the size
matches the expectation in that case.


Done


Re: CVS commit: src/sys

2021-02-03 Thread Kamil Rytarowski
On 03.02.2021 06:51, Roy Marples wrote:
> Module Name:  src
> Committed By: roy
> Date: Wed Feb  3 05:51:40 UTC 2021
> 
> Modified Files:
>   src/sys/net: if_arp.h if_ether.h if_gre.h
>   src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
>   ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h
> 
> Log Message:
> Remove __packed from various network structures
> 
> They are already network aligned and adding the __packed attribute
> just causes needless compiler warnings about accssing members of packed
> objects.
> 

This changes the ABI for userland programs. With __packed, these
structures, whenever contain at least 2-byte data, can be placed in a
different location inside another structure now.

#include 
#include 
#include 
#include 

struct aaa {
uint16_t a;
uint8_t b;
uint8_t c;
} __packed;

struct aaa2 {
uint8_t x;
struct aaa y;
};

struct bbb {
uint16_t a;
uint8_t b;
uint8_t c;
};

struct bbb2 {
uint8_t x;
struct bbb y;
};

int
main()
{
printf("sizeof(aaa2) = %zu, alignof(aaa2) = %zu\n",
sizeof(struct aaa2), alignof(struct aaa2));
printf("sizeof(bbb2) = %zu, alignof(bbb2) = %zu\n",
sizeof(struct bbb2), alignof(struct bbb2));
}


$ ./a.out
sizeof(aaa) = 4, alignof(aaa) = 1
sizeof(bbb) = 4, alignof(bbb) = 2
sizeof(aaa2) = 5, alignof(aaa2) = 1
sizeof(bbb2) = 6, alignof(bbb2) = 2
Please try t access these members of the structs with memcpy(3),
memcmp(3), memset(3) etc rather than directly with * or [].

Before I saw your commit, I wanted to ask to revert the following changes:

icmp6: Remove __packed attribute from icmp6 structures

https://github.com/NetBSD/src/commit/427831ba4bdf388aecf3a378de8faf3a4d44a462

ip6: Remove __packed attribute from ip6 structures

https://github.com/NetBSD/src/commit/e82879afd70b0e801e6ee53bd14c27be3dd1738f

The fallout can be subtle and hard to debug. Once, I removed __packed
from one innocent networking structure myself, qemu networking stopped
working at all.


Re: CVS commit: src/sys

2021-02-03 Thread Joerg Sonnenberger
On Wed, Feb 03, 2021 at 05:51:40AM +, Roy Marples wrote:
> Module Name:  src
> Committed By: roy
> Date: Wed Feb  3 05:51:40 UTC 2021
> 
> Modified Files:
>   src/sys/net: if_arp.h if_ether.h if_gre.h
>   src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
>   ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h
> 
> Log Message:
> Remove __packed from various network structures
> 
> They are already network aligned and adding the __packed attribute
> just causes needless compiler warnings about accssing members of packed
> objects.

Please add a compile-time assert at least for _KERNEL that the size
matches the expectation in that case.

Joerg


Re: CVS commit: src/lib/libc/stdio

2021-02-01 Thread Robert Elz
Date:Mon, 1 Feb 2021 17:50:54 +
From:"Jaromir Dolecek" 
Message-ID:  <20210201175054.112e7f...@cvs.netbsd.org>

  | FreeBSD has a similar check, but they return EINVAL instead, feel
  | free to adjust if SUS or other standard mandates specific value

Not currently (unless the C standard says something) - but EINVAL
would be a better choice, fread() incorporates all errors from
fgetc() (and fwrite() from fputc()) and there EOVERFLOW is used to
indicate that the file offset exceeds the maximum possible file
size, which is quite a different problem.   EINVAL is currently
unused (at least in POSIX) but makes more sense for this problem
(which is am invalid arg combination).

FWIW, I submitted a defect report on this issue with the Austin Group
(POSIX maintainers) yesterday, though I am expecting they'll simply
punt it to the C standards group.

kre



Re: CVS commit: src/sys/dev/pci

2021-02-01 Thread J. Hannken-Illjes
> On 12. Jul 2020, at 21:05, Jaromir Dolecek  wrote:
> 
> Module Name:  src
> Committed By: jdolecek
> Date: Sun Jul 12 19:05:32 UTC 2020
> 
> Modified Files:
>   src/sys/dev/pci: if_bnx.c if_bnxvar.h
> 
> Log Message:
> enable MSI/MSI-X if supported by adapter
> 
> tested MSI-X with Broadcom NetXtreme II BCM5709 1000Base-T
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.95 -r1.96 src/sys/dev/pci/if_bnx.c
> cvs rdiff -u -r1.12 -r1.13 src/sys/dev/pci/if_bnxvar.h
> 
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.

With this change my Dell PowerEdge 2850 spits watchdog resets:

[  68.1828359] bnx0: Watchdog timeout -- resetting!
[  88.6042909] bnx0: Watchdog timeout -- resetting!
[ 119.0265230] bnx0: Watchdog timeout -- resetting!
[ 145.4484562] bnx0: Watchdog timeout -- resetting!

Dmesg before is:

[ 1.017306] pci4 at ppb3 bus 7
[ 1.017306] pci4: i/o space, memory space enabled, rd/line, wr/inv ok
[ 1.017306] bnx0 at pci4 dev 0 function 0: Broadcom NetXtreme II BCM5708 
1000Base-T
[ 1.017306] bnx0: Ethernet address 00:24:e8:67:4b:db
[ 1.017306] bnx0: ASIC BCM5708 B2 (0x57081020)
[ 1.017306] bnx0: PCI-X 64bit 133MHz
[ 1.017306] bnx0: B/C (4.6.0); Bufs (RX:2;TX:2); Flags (MFW); MFW (ipms 
1.6.0)
[ 1.017306] bnx0: Coal (RX:6,6,18,18; TX:20,20,80,80)
[ 1.017306] brgphy0 at bnx0 phy 1: BCM5708C 1000BASE-T media interface, 
rev. 6
[ 1.017306] brgphy0: 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, 
1000baseT, 1000baseT-FDX, auto
[ 1.017306] bnx0: interrupting at ioapic0 pin 16

while dmesg after is:

[ 1.017262] pci4 at ppb3 bus 7
[ 1.017262] pci4: i/o space, memory space enabled, rd/line, wr/inv ok
[ 1.017262] bnx0 at pci4 dev 0 function 0: Broadcom NetXtreme II BCM5708 
1000Base-T
[ 1.017262] bnx0: Ethernet address 00:24:e8:67:4b:db
[ 1.017262] bnx0: ASIC BCM5708 B2 (0x57081020)
[ 1.017262] bnx0: PCI-X 64bit 133MHz
[ 1.017262] bnx0: B/C (4.6.0); Bufs (RX:2;TX:2); Flags (MFW); MFW (ipms 
1.6.0)
[ 1.017262] bnx0: Coal (RX:6,6,18,18; TX:20,20,80,80)
[ 1.017262] brgphy0 at bnx0 phy 1: BCM5708C 1000BASE-T media interface, 
rev. 6
[ 1.017262] brgphy0: 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, 
1000baseT, 1000baseT-FDX, auto
[ 1.017262] bnx0: interrupting at msi0 vec 0

Pcictl dump gives (in the MSI-X case):

  PCI Message Signaled Interrupt
Message Control register: 0x0081
  MSI Enabled: on
  Multiple Message Capable: no (1 vector)
  Multiple Message Enabled: off (1 vector)
  64 Bit Address Capable: on
  Per-Vector Masking Capable: off
  Extended Message Data Capable: off
  Extended Message Data Enable: off
Message Address (lower) register: 0xfee0
Message Address (upper) register: 0x
Message Data register: 0x0064

Any ideas how to fix this issue?

--
J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany)


signature.asc
Description: Message signed with OpenPGP


Re: CVS commit: src/sys/net

2021-01-31 Thread Kengo NAKAHARA

Hi,

I have one question about the following commit.

Why stoeplitz_hash_ip4() and stoeplitz_hash_ip4port() argument types
are different between toeplitz.c and toeplitz.h?  They have in_addr_t
and in_port_t argument types in toeplitz.c, howerver uint32_t and
uint16_t in toeplitz.h.

On 2021/01/31 6:23, Jared D. McNeill wrote:

Module Name:src
Committed By:   jmcneill
Date:   Sat Jan 30 21:23:08 UTC 2021

Modified Files:
src/sys/net: files.net
Added Files:
src/sys/net: toeplitz.c toeplitz.h

Log Message:
Add symmetric toeplitz implementation with integration for NICs, from OpenBSD.


To generate a diff of this commit:
cvs rdiff -u -r1.29 -r1.30 src/sys/net/files.net
cvs rdiff -u -r0 -r1.1 src/sys/net/toeplitz.c src/sys/net/toeplitz.h

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


Thanks,


--
//
Internet Initiative Japan Inc.

Device Engineering Section,
Product Development Department,
Product Division,
Technology Unit

Kengo NAKAHARA 


Re: CVS commit: src/lib/libc/stdio

2021-01-31 Thread Robert Elz
Date:Sun, 31 Jan 2021 18:34:22 +0100
From:Joerg Sonnenberger 
Message-ID:  

  | That makes no sense. Just turn them into a short read, which is
  | something users have to deal with anyway.

I'm not sure I agree with that one.   If the user's size * nmemb
overflows a size_t then the user is attempting something entirely
insane (they cannot possibly have a buffer that big).  Reading
anything at all is most probably the wrong thing to do, as the
args are very likely simply erroneous.   EINVAL is not an unreasonable
approach, I wouldn't even object to simply abort().

kre



Re: CVS commit: src/lib/libc/stdio

2021-01-31 Thread Kamil Rytarowski
On 31.01.2021 18:34, Joerg Sonnenberger wrote:
> On Sun, Jan 31, 2021 at 05:27:28PM +0100, Kamil Rytarowski wrote:
>> On 31.01.2021 17:18, Jaromir Dolecek wrote:
>>> Module Name:src
>>> Committed By:   jdolecek
>>> Date:   Sun Jan 31 16:18:22 UTC 2021
>>>
>>> Modified Files:
>>> src/lib/libc/stdio: fread.c
>>>
>>> Log Message:
>>> for unbuffered I/O arrange for the destination buffer to be filled in one
>>> go, instead of triggering long series of 1 byte read(2)s; this speeds up
>>> fread() several order of magnitudes for this case, directly proportional
>>> to the size of the supplied buffer
>>>
>>> change adapted from OpenBSD rev. 1.19
>>>
>>> fixes PR lib/55808 by Roland Illig
>>>
>>
>> While there it would be cool to apply FreeBSD and OpenBSD hardening for
>> invalid size x nmemb, checking for overflow. I propose to return EINVAL
>> in such case.
> 
> That makes no sense. Just turn them into a short read, which is
> something users have to deal with anyway.
> 
> Joerg
> 

The purpose of this errno is to catch insage usage of API that in normal
circumstances never makes sense. With overflows (that are fine), the
error can be unnoticed.

Both FreeBSD and OpenBSD found this behavior as useful.


Re: CVS commit: src/lib/libc/stdio

2021-01-31 Thread Joerg Sonnenberger
On Sun, Jan 31, 2021 at 05:27:28PM +0100, Kamil Rytarowski wrote:
> On 31.01.2021 17:18, Jaromir Dolecek wrote:
> > Module Name:src
> > Committed By:   jdolecek
> > Date:   Sun Jan 31 16:18:22 UTC 2021
> > 
> > Modified Files:
> > src/lib/libc/stdio: fread.c
> > 
> > Log Message:
> > for unbuffered I/O arrange for the destination buffer to be filled in one
> > go, instead of triggering long series of 1 byte read(2)s; this speeds up
> > fread() several order of magnitudes for this case, directly proportional
> > to the size of the supplied buffer
> > 
> > change adapted from OpenBSD rev. 1.19
> > 
> > fixes PR lib/55808 by Roland Illig
> > 
> 
> While there it would be cool to apply FreeBSD and OpenBSD hardening for
> invalid size x nmemb, checking for overflow. I propose to return EINVAL
> in such case.

That makes no sense. Just turn them into a short read, which is
something users have to deal with anyway.

Joerg


Re: CVS commit: src/lib/libc/stdio

2021-01-31 Thread Kamil Rytarowski
On 31.01.2021 17:18, Jaromir Dolecek wrote:
> Module Name:  src
> Committed By: jdolecek
> Date: Sun Jan 31 16:18:22 UTC 2021
> 
> Modified Files:
>   src/lib/libc/stdio: fread.c
> 
> Log Message:
> for unbuffered I/O arrange for the destination buffer to be filled in one
> go, instead of triggering long series of 1 byte read(2)s; this speeds up
> fread() several order of magnitudes for this case, directly proportional
> to the size of the supplied buffer
> 
> change adapted from OpenBSD rev. 1.19
> 
> fixes PR lib/55808 by Roland Illig
> 

While there it would be cool to apply FreeBSD and OpenBSD hardening for
invalid size x nmemb, checking for overflow. I propose to return EINVAL
in such case.

I planned to submit it a while ago for discussion.


Re: CVS commit: src/usr.bin/make

2021-01-29 Thread Roland Illig

On 27.01.2021 20:54, Reinoud Zandijk wrote:

Hi,

On Tue, Jan 26, 2021 at 11:44:56PM +, Roland Illig wrote:

Module Name:src
Committed By:   rillig
Date:   Tue Jan 26 23:44:56 UTC 2021

Modified Files:
src/usr.bin/make: parse.c
src/usr.bin/make/unit-tests: include-main.exp include-subsub.mk

Log Message:
make(1): in -dp mode, print stack trace with each diagnostic


Maybe related but could you make printing of the 1st failing error message
easier to find in a parallel build? Say recording the command and output of
the offending command and print it at the end?


Sounds possible.  I'll look into it but cannot promise anything right
now.  Ideally this diagnostic information would be passed up to the
top-level make process, which would then print it at the very end.

Roland


Re: CVS commit: src/doc

2021-01-28 Thread Jason Thorpe


> On Jan 27, 2021, at 3:11 PM, Simon Burge  wrote:
> 
> It can run either big- or little-endian, but has virtually no IO support
> whatsoever - just a UART and an extremely simple eithernet driver.
> 
> I couldn't find any MIPS references to virtio with a quick look through
> the QEMU 5.0 sources.

In theory, you should be able to configure virtio @ pci on the Malta emulation, 
but in practice… *shrug*.

-- thorpej



Re: CVS commit: src/doc

2021-01-27 Thread Simon Burge
Hi Reinoud,

Reinoud Zandijk wrote:

> Hi Simon,
>
> On Wed, Jan 27, 2021 at 05:27:01AM +, Simon Burge wrote:
> > Module Name:src
> > Committed By:   simonb
> > Date:   Wed Jan 27 05:27:01 UTC 2021
> > 
> > Modified Files:
> > src/doc: CHANGES
> > 
> > Log Message:
> > Note support for QEMU "mipssim" emulator.
>
> Is this machine also *able* to run big endian? Or/and can it also use virtio
> over either FDT/ACPI or PCI?

It can run either big- or little-endian, but has virtually no IO support
whatsoever - just a UART and an extremely simple eithernet driver.

I couldn't find any MIPS references to virtio with a quick look through
the QEMU 5.0 sources.

Cheers,
Simon.


Re: CVS commit: src/doc

2021-01-27 Thread Reinoud Zandijk
Hi Simon,

On Wed, Jan 27, 2021 at 05:27:01AM +, Simon Burge wrote:
> Module Name:  src
> Committed By: simonb
> Date: Wed Jan 27 05:27:01 UTC 2021
> 
> Modified Files:
>   src/doc: CHANGES
> 
> Log Message:
> Note support for QEMU "mipssim" emulator.

Is this machine also *able* to run big endian? Or/and can it also use virtio
over either FDT/ACPI or PCI?

With regards,
Reinoud



Re: CVS commit: src/usr.bin/make

2021-01-27 Thread Reinoud Zandijk
Hi,

On Tue, Jan 26, 2021 at 11:44:56PM +, Roland Illig wrote:
> Module Name:  src
> Committed By: rillig
> Date: Tue Jan 26 23:44:56 UTC 2021
> 
> Modified Files:
>   src/usr.bin/make: parse.c
>   src/usr.bin/make/unit-tests: include-main.exp include-subsub.mk
> 
> Log Message:
> make(1): in -dp mode, print stack trace with each diagnostic

Maybe related but could you make printing of the 1st failing error message
easier to find in a parallel build? Say recording the command and output of
the offending command and print it at the end ?

Reinoud



Re: CVS commit: src/usr.bin/make

2021-01-26 Thread Martin Husemann
On Tue, Jan 26, 2021 at 11:39:52PM +0100, Roland Illig wrote:
> The code of usr.bin/make gets distributed to a wider audience by Simon's
> bmake distribution (http://www.crufty.net/help/sjg/bmake.html), that's
> where the requirement of supporting C89 compilers comes from.  At the
> time I committed this fix, Simon had managed to dig out an old Solaris 9
> installation with GCC, and these few changes were the only ones needed
> to let bmake run on that platform.  That sounded easy enough to me.

Also note that we need bmake during bootstrap of pkgsrc, and besides
finding a working compiler it is one of the early things you need to
make work on an ancient platform if you try to bring it to new use (I
have been there with Solaris 2.6 at one point, but gave up for other
reasons - and of course the machine in question now runs NetBSD (again)
[the Solaris adventure was a temporary thing anyway and as pkgsrc did
not help as a "plug and play" way to get a usable dev/debug environment
it was not worth pushing]).

But I must admit that the *commit log* of that change sounded way more scary
than the actual change is:

replace %zu with %u in printf calls

would be plain wrong and of course break (either at runtime or if lucky
at compile time) many, many platforms.

But

-   (void)fprintf(f, "\"%s\" line %zu: ", fname, lineno);
+   (void)fprintf(f, "\"%s\" line %u: ", fname, (unsigned)lineno);

is ok for portable code and "lineno" referencing (I guess) a makefile.
It could have been (long unsigned) and "%lu", but maybe portability to
systems where that would make a difference is a bit far stretched.

Martin


Re: CVS commit: src/sys/sys

2021-01-26 Thread Jason Thorpe


> On Jan 26, 2021, at 6:49 PM, Valery Ushakov  wrote:
> 
> It's hardly fair to characterize three people politely inquiring about
> that choice and pointing out a more standard way to spell what you
> want to express (that is perfectly in rhyme with the preceding table
> and is only one character longer too) as "angry hordes objecting".

Sorry, should have added the "/s" at the end.  Was kind of a long day, and this 
was a misguided attempt at decompression humor.

-- thorpej



Re: CVS commit: src/sys/sys

2021-01-26 Thread Valery Ushakov
On Wed, Jan 27, 2021 at 01:00:05 +, Jason R Thorpe wrote:

> Module Name:  src
> Committed By: thorpej
> Date: Wed Jan 27 01:00:05 UTC 2021
> 
> Modified Files:
>   src/sys/sys: device.h
> 
> Log Message:
> Define a macro to terminate the common usage of device_compatible_entry
> arrays, in order to placate the angry hordes objecting to use of a GCC
> extension.

It's hardly fair to characterize three people politely inquiring about
that choice and pointing out a more standard way to spell what you
want to express (that is perfectly in rhyme with the preceding table
and is only one character longer too) as "angry hordes objecting".

The point is moot anyway, since anonymous unions themselves only
officially appeared in C11.

-uwe


Re: CVS commit: src/usr.bin/make

2021-01-26 Thread Roland Illig

On 26.01.2021 21:04, Christos Zoulas wrote:

Yes, and I did not push for it for the exact reasons stated below:
There was only a handful of cases and those can be handled with casts or a 
macro for now.

I am questioning though the utility of maintaining compatibility with platforms 
that
do not support C99 20 years later, vs. using %u and casting or using a 
formatting macro.


The support for ancient platforms is not only with regards to "%zu" vs.
"%u", in util.c there is even support for platforms that lack getenv().
 Apparently nobody cared enough to remove these bits, even though
getenv is guaranteed to exist since C89 already.

The code of usr.bin/make gets distributed to a wider audience by Simon's
bmake distribution (http://www.crufty.net/help/sjg/bmake.html), that's
where the requirement of supporting C89 compilers comes from.  At the
time I committed this fix, Simon had managed to dig out an old Solaris 9
installation with GCC, and these few changes were the only ones needed
to let bmake run on that platform.  That sounded easy enough to me.

Roland


Re: CVS commit: src/usr.bin/make

2021-01-26 Thread Christos Zoulas
Yes, and I did not push for it for the exact reasons stated below:
There was only a handful of cases and those can be handled with casts or a 
macro for now.

I am questioning though the utility of maintaining compatibility with platforms 
that
do not support C99 20 years later, vs. using %u and casting or using a 
formatting macro.

christos

> On Jan 26, 2021, at 2:00 PM, Roland Illig  wrote:
> 
> On 26.01.2021 11:19, Rin Okuyama wrote:
>> Ping?
>> 
>> I don't think this is correct fix either.
>> Can you please revert this commit?
> 
> Sorry, I didn't get the first mail from Christos back in December,
> that's why I didn't take any action.
> 
> Why shouldn't the fix I did be correct?  I carefully checked the few
> places where I added the casts, and none of the numbers that are
> involved will ever be more than a billion.
> 
> 1.  The number of variables in a .for loop
> 
> 2.  The number of items in a .for loop
> 
> 3.  A single line of a text file in meta mode.  (Only in debug mode.)
> 
> 4.  The number of lines in a single makefile.
> 
> 5.  The capturing subexpression in a :C variable modifier, which only
> ever ranges from 0 to 9.
> 
> 6.  The number of words into which a variable value is split. (Only in
> debug mode.)
> 
> Given these, why should a simple %u not be appropriate?  I don't want to
> clutter the code with another preprocessor macro, therefore I'd like to
> keep the code the way it is right now.
> 
> I also don't see why this could ever have the slightest chance of
> "breaking everyone else".
> 
> For reference and easy viewing, here is the corresponding commit on
> GitHub:
> https://github.com/NetBSD/src/commit/ff11c7d3497a40c90ec70101ad72612e2f0884b2
> 
> Roland
> 
>> On 2020/12/15 7:57, Christos Zoulas wrote:
>>> In article <20201213212746.3cfc3f...@cvs.netbsd.org>,
>>> Roland Illig  wrote:
 -=-=-=-=-=-
 
 Module Name:src
 Committed By:rillig
 Date:Sun Dec 13 21:27:46 UTC 2020
 
 Modified Files:
 src/usr.bin/make: for.c meta.c parse.c var.c
 
 Log Message:
 make(1): replace %zu with %u in printf calls
 
 This is needed to compile bmake with GCC 2.8.1 on SunOS 5.9.
 
 To support ancient systems like this, the whole code of usr.bin/make is
 supposed to use only ISO C90 features, except for filemon, which is not
 used on these systems.
>>> 
>>> Please revert! This breaks everyone else. %zu is the format to
>>> print size_t.  Last I checked SunOS 5.9 has been dead since 2014
>>> and whoever is still using it might as well install a new compiler,
>>> or tie the box at the end of a long chain so it can find its true
>>> calling. If you really want to support it instead define MAKE_FMT_SIZE_T
>>> and conditionalize it properly for "ancient OS", windows, cygwin,
>>> mingwin, and regular folks (this does not even handle "ancient os"):
>>> 
>>>  https://github.com/file/file/blob/master/src/file.h#L55
>>> 
>>> I am still against it though...
>>> 
>>> christos
>>> 



signature.asc
Description: Message signed with OpenPGP


Re: CVS commit: src/usr.bin/make

2021-01-26 Thread Roland Illig

On 26.01.2021 11:19, Rin Okuyama wrote:

Ping?

I don't think this is correct fix either.
Can you please revert this commit?


Sorry, I didn't get the first mail from Christos back in December,
that's why I didn't take any action.

Why shouldn't the fix I did be correct?  I carefully checked the few
places where I added the casts, and none of the numbers that are
involved will ever be more than a billion.

1.  The number of variables in a .for loop

2.  The number of items in a .for loop

3.  A single line of a text file in meta mode.  (Only in debug mode.)

4.  The number of lines in a single makefile.

5.  The capturing subexpression in a :C variable modifier, which only
ever ranges from 0 to 9.

6.  The number of words into which a variable value is split. (Only in
debug mode.)

Given these, why should a simple %u not be appropriate?  I don't want to
clutter the code with another preprocessor macro, therefore I'd like to
keep the code the way it is right now.

I also don't see why this could ever have the slightest chance of
"breaking everyone else".

For reference and easy viewing, here is the corresponding commit on
GitHub:
https://github.com/NetBSD/src/commit/ff11c7d3497a40c90ec70101ad72612e2f0884b2

Roland


On 2020/12/15 7:57, Christos Zoulas wrote:

In article <20201213212746.3cfc3f...@cvs.netbsd.org>,
Roland Illig  wrote:

-=-=-=-=-=-

Module Name:    src
Committed By:    rillig
Date:    Sun Dec 13 21:27:46 UTC 2020

Modified Files:
src/usr.bin/make: for.c meta.c parse.c var.c

Log Message:
make(1): replace %zu with %u in printf calls

This is needed to compile bmake with GCC 2.8.1 on SunOS 5.9.

To support ancient systems like this, the whole code of usr.bin/make is
supposed to use only ISO C90 features, except for filemon, which is not
used on these systems.


Please revert! This breaks everyone else. %zu is the format to
print size_t.  Last I checked SunOS 5.9 has been dead since 2014
and whoever is still using it might as well install a new compiler,
or tie the box at the end of a long chain so it can find its true
calling. If you really want to support it instead define MAKE_FMT_SIZE_T
and conditionalize it properly for "ancient OS", windows, cygwin,
mingwin, and regular folks (this does not even handle "ancient os"):

 https://github.com/file/file/blob/master/src/file.h#L55

I am still against it though...

christos



Re: CVS commit: src/sys/dev/pci

2021-01-26 Thread Reinoud Zandijk
On Tue, Jan 26, 2021 at 05:51:42PM +0900, Rin Okuyama wrote:
> Hi,

> This seems not correct for me. Is the attached patch OK with you?

Well you spotted a bug indeed int he freeing section. I'll fix and commit it.
Thanks for reporting.

Reinoud


signature.asc
Description: PGP signature


Re: CVS commit: src/usr.bin/make

2021-01-26 Thread Rin Okuyama

Ping?

I don't think this is correct fix either.
Can you please revert this commit?

Thanks,
rin

On 2020/12/15 7:57, Christos Zoulas wrote:

In article <20201213212746.3cfc3f...@cvs.netbsd.org>,
Roland Illig  wrote:

-=-=-=-=-=-

Module Name:src
Committed By:   rillig
Date:   Sun Dec 13 21:27:46 UTC 2020

Modified Files:
src/usr.bin/make: for.c meta.c parse.c var.c

Log Message:
make(1): replace %zu with %u in printf calls

This is needed to compile bmake with GCC 2.8.1 on SunOS 5.9.

To support ancient systems like this, the whole code of usr.bin/make is
supposed to use only ISO C90 features, except for filemon, which is not
used on these systems.


Please revert! This breaks everyone else. %zu is the format to
print size_t.  Last I checked SunOS 5.9 has been dead since 2014
and whoever is still using it might as well install a new compiler,
or tie the box at the end of a long chain so it can find its true
calling. If you really want to support it instead define MAKE_FMT_SIZE_T
and conditionalize it properly for "ancient OS", windows, cygwin,
mingwin, and regular folks (this does not even handle "ancient os"):

 https://github.com/file/file/blob/master/src/file.h#L55

I am still against it though...

christos



Re: CVS commit: src/sys/dev/pci

2021-01-26 Thread Rin Okuyama

Hi,

On 2021/01/25 0:33, Reinoud Zandijk wrote:

Module Name:src
Committed By:   reinoud
Date:   Sun Jan 24 15:33:02 UTC 2021

Modified Files:
src/sys/dev/pci: virtio_pci.c

Log Message:
On error unmap the pci_mapreg_map()d regions using bus_space_unmap() as
suggested by jak@


To generate a diff of this commit:
cvs rdiff -u -r1.22 -r1.23 src/sys/dev/pci/virtio_pci.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


This seems not correct for me. Is the attached patch OK with you?

Thanks,
rin
Index: sys/dev/pci/virtio_pci.c
===
RCS file: /home/netbsd/src/sys/dev/pci/virtio_pci.c,v
retrieving revision 1.25
diff -p -u -r1.25 virtio_pci.c
--- sys/dev/pci/virtio_pci.c24 Jan 2021 15:59:35 -  1.25
+++ sys/dev/pci/virtio_pci.c26 Jan 2021 08:47:23 -
@@ -444,7 +444,7 @@ virtio_pci_attach_10(device_t self, void
bus_size_t bars[NMAPREG] = { 0 };
int bars_idx[NMAPREG] = { 0 };
struct virtio_pci_cap *caps[] = { , , ,  };
-   int i, j = 0, ret = 0;
+   int i, j, ret = 0;
 
if (virtio_pci_find_cap(psc, VIRTIO_PCI_CAP_COMMON_CFG,
, sizeof(common)))
@@ -471,7 +471,7 @@ virtio_pci_attach_10(device_t self, void
bars[bar] = len;
}
 
-   for (i = 0; i < __arraycount(bars); i++) {
+   for (i = j = 0; i < __arraycount(bars); i++) {
int reg;
pcireg_t type;
if (bars[i] == 0)
@@ -550,11 +550,12 @@ virtio_pci_attach_10(device_t self, void
 
 err:
/* undo our pci_mapreg_map()s */ 
-   for (i = 0; i < __arraycount(bars); i++) {
+   for (i = j = 0; i < __arraycount(bars); i++) {
if (bars[i] == 0)
continue;
bus_space_unmap(psc->sc_bars_iot[j], psc->sc_bars_ioh[j],
psc->sc_bars_iosize[j]);
+   j++;
}
return ret;
 }


Re: CVS commit: src/sys/arch

2021-01-25 Thread Jason Thorpe


> On Jan 25, 2021, at 9:45 AM, Robert Elz  wrote:
> 
>Date:Mon, 25 Jan 2021 08:19:44 -0800
>From:Jason Thorpe 
>Message-ID:  
> 
>  | Using { 0 } makes an assumption about the first member of the
>  | structure which is not guaranteed to remain true.
> 
> That's right, but you could explicitly init a named field, most likely
> the one that is tested to determine that this is the sentinel, eg: from
> one of the recent updates ...
> 
> static const struct device_compatible_entry compat_data[] = {
>{ .compat = "pnpPNP,401" },
> -   { 0 }
> +   { }
> };
> 
> that could instead be changed to
>   { .compat = NULL }
> 
> (or something similar to that).

I noticed this because of a different local change in my tree that makes the 
first field another anonymous union.

Anyhow, I'll go ahead and define a standard sentinel macro that can be used for 
the common { .compat = XXX } case, and fire up sed to fix up the tree.

-- thorpej



Re: CVS commit: src/sys/arch

2021-01-25 Thread Kamil Rytarowski
On 25.01.2021 17:19, Jason Thorpe wrote:
> 
>> On Jan 25, 2021, at 6:22 AM, Kamil Rytarowski  wrote:
>>
>> I have no problem with this change but I am curious why should we use "{
>> }"? It's a C GNU extension and C++ syntax.
> 
> Using { 0 } makes an assumption about the first member of the structure which 
> is not guaranteed to remain true.
> 

Both versions should work in the same way, except that {0} is the
standard variation and {} an extension.

I have got no preference for either.

> -- thorpej
> 




signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/sys/arch

2021-01-25 Thread Valery Ushakov
On Mon, Jan 25, 2021 at 20:28:52 +0300, Valery Ushakov wrote:

> On Mon, Jan 25, 2021 at 08:19:44 -0800, Jason Thorpe wrote:
> 
> > > On Jan 25, 2021, at 6:22 AM, Kamil Rytarowski  wrote:
> > > 
> > > I have no problem with this change but I am curious why should we use "{
> > > }"? It's a C GNU extension and C++ syntax.
> > 
> > Using { 0 } makes an assumption about the first member of the
> > structure which is not guaranteed to remain true.
> 
> The commit message says:
> 
> | Since we're using designated initialisers for compat data, we should
> | use a completely empty initializer for the sentinel. 
> 
> but that "should" is not true.  The code that checks that sentinel
> uses some particular member to access it, so, pedantically speaking,
> the initialization must designate that member in the sentinel
> initialization.  Yes, this is verbose and doesn't look as pretty, but
> that is what "should" happen here.  Using non-standard {} extension
> makes it look nicer, but is not a "should" kind of necessity.

PS: Forgot to add that C++ doesn't have designated initializers (or at
least it didn't last time I looked), so they are in a different
situation here and need an empty initializer list.  In C the only
difference it makes is, as far as I can tell, exactly this kind of an
array with a sentinel at the end and the difference is cosmetic.

-uwe


Re: CVS commit: src/sys/arch

2021-01-25 Thread Robert Elz
Date:Mon, 25 Jan 2021 08:19:44 -0800
From:Jason Thorpe 
Message-ID:  

  | Using { 0 } makes an assumption about the first member of the
  | structure which is not guaranteed to remain true.

That's right, but you could explicitly init a named field, most likely
the one that is tested to determine that this is the sentinel, eg: from
one of the recent updates ...

 static const struct device_compatible_entry compat_data[] = {
{ .compat = "pnpPNP,401" },
-   { 0 }
+   { }
 };

that could instead be changed to
{ .compat = NULL }

(or something similar to that).

kre



Re: CVS commit: src/sys/arch

2021-01-25 Thread Valery Ushakov
On Mon, Jan 25, 2021 at 08:19:44 -0800, Jason Thorpe wrote:

> > On Jan 25, 2021, at 6:22 AM, Kamil Rytarowski  wrote:
> > 
> > I have no problem with this change but I am curious why should we use "{
> > }"? It's a C GNU extension and C++ syntax.
> 
> Using { 0 } makes an assumption about the first member of the
> structure which is not guaranteed to remain true.

The commit message says:

| Since we're using designated initialisers for compat data, we should
| use a completely empty initializer for the sentinel. 

but that "should" is not true.  The code that checks that sentinel
uses some particular member to access it, so, pedantically speaking,
the initialization must designate that member in the sentinel
initialization.  Yes, this is verbose and doesn't look as pretty, but
that is what "should" happen here.  Using non-standard {} extension
makes it look nicer, but is not a "should" kind of necessity.

-uwe


Re: CVS commit: src/sys/arch

2021-01-25 Thread Jason Thorpe


> On Jan 25, 2021, at 6:22 AM, Kamil Rytarowski  wrote:
> 
> I have no problem with this change but I am curious why should we use "{
> }"? It's a C GNU extension and C++ syntax.

Using { 0 } makes an assumption about the first member of the structure which 
is not guaranteed to remain true.

-- thorpej



Re: CVS commit: src/sys/arch

2021-01-25 Thread Kamil Rytarowski
On 25.01.2021 15:20, Jason R Thorpe wrote:
> Module Name:  src
> Committed By: thorpej
> Date: Mon Jan 25 14:20:39 UTC 2021
> 
> Modified Files:
>   src/sys/arch/arm/altera: cycv_clkmgr.c
>   src/sys/arch/arm/amlogic: meson_pinctrl.c meson_pwm.c meson_thermal.c
>   meson_usbctrl.c meson_usbphy.c mesong12_clkc.c mesongx_mmc.c
>   mesongxbb_clkc.c
>   src/sys/arch/arm/broadcom: bcm2835_emmc.c bcm2835_intr.c
>   src/sys/arch/arm/fdt: gicv3_fdt.c pcihost_fdt.c
>   src/sys/arch/arm/nvidia: tegra_ahcisata.c tegra_nouveau.c tegra_pcie.c
>   tegra_pinmux.c tegra_soctherm.c tegra_xusb.c
>   src/sys/arch/arm/nxp: if_enet_imx.c imx6_pcie.c imx6_spi.c
>   imx8mq_usbphy.c imx_sdhc.c
>   src/sys/arch/arm/rockchip: rk3328_iomux.c rk3399_iomux.c rk_gmac.c
>   rk_i2s.c rk_pwm.c rk_tsadc.c rk_usb.c rk_v1crypto.c rk_vop.c
>   src/sys/arch/arm/samsung: exynos_dwcmmc.c exynos_pinctrl.c
>   exynos_platform.c exynos_usbdrdphy.c exynos_usbphy.c
>   src/sys/arch/arm/sociox: if_ave.c
>   src/sys/arch/arm/sunxi: sun4i_a10_ccu.c sun4i_dma.c sun6i_dma.c
>   sun8i_crypto.c sunxi_can.c sunxi_codec.c sunxi_de2_ccu.c
>   sunxi_dep.c sunxi_gpio.c sunxi_hdmi.c sunxi_hdmiphy.c sunxi_i2s.c
>   sunxi_lcdc.c sunxi_lradc.c sunxi_mixer.c sunxi_mmc.c sunxi_musb.c
>   sunxi_nmi.c sunxi_pwm.c sunxi_rsb.c sunxi_rtc.c sunxi_sid.c
>   sunxi_sramc.c sunxi_tcon.c sunxi_thermal.c sunxi_ts.c sunxi_twi.c
>   sunxi_usb3phy.c sunxi_usbphy.c sunxi_wdt.c
>   src/sys/arch/arm/ti: ti_dpll_clock.c ti_gpio.c ti_iic.c ti_omapintc.c
>   ti_omaptimer.c ti_sdhc.c
>   src/sys/arch/macppc/dev: deq.c lmu.c psoc.c smusat.c
>   src/sys/arch/mips/cavium/dev: octeon_cib.c octeon_intc.c
>   src/sys/arch/sparc64/dev: pcf8591_envctrl.c
> 
> Log Message:
> Since we're using designated initialisers for compat data, we should
> use a completely empty initializer for the sentinel.
> 

>  static const struct device_compatible_entry compat_data[] = {
>   { .compat = "ecadc" },
> -
> - { 0 }
> + { }
>  };
>  
>  static int
> 

I have no problem with this change but I am curious why should we use "{
}"? It's a C GNU extension and C++ syntax.



signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/sys/dev/pci

2021-01-25 Thread Jason Thorpe


> On Jan 24, 2021, at 10:20 PM, Martin Husemann  wrote:
> 
>> I kept getting a ?static after non-static declaration? error when building 
>> for aarch64.
> 
> I guess that was with outdated arm/include/bus_funcs.h and
> sys/bus_proto.h (or where was the previous declaration)?

I did a “cvs update” just before, *shrug*.  In any case, the problem was easily 
avoidable, and now it is avoided.

-- thorpej



Re: CVS commit: src/sys/dev/pci

2021-01-24 Thread Martin Husemann
On Sun, Jan 24, 2021 at 09:45:14PM -0800, Jason Thorpe wrote:
> 
> > On Jan 24, 2021, at 9:37 PM, Martin Husemann  wrote:
> > 
> > While I don't care for names, I would like to understand fallout in
> > details before hiding it - what exactly did not compile correctly?
> > At least the affected arm kernels worked for me in the state directly
> > before your commit.
> 
> I kept getting a ?static after non-static declaration? error when building 
> for aarch64.

I guess that was with outdated arm/include/bus_funcs.h and
sys/bus_proto.h (or where was the previous declaration)?

Martin


Re: CVS commit: src/sys/dev/pci

2021-01-24 Thread Jason Thorpe


> On Jan 24, 2021, at 9:37 PM, Martin Husemann  wrote:
> 
> While I don't care for names, I would like to understand fallout in
> details before hiding it - what exactly did not compile correctly?
> At least the affected arm kernels worked for me in the state directly
> before your commit.

I kept getting a “static after non-static declaration” error when building for 
aarch64.

-- thorpej



Re: CVS commit: src/sys/dev/pci

2021-01-24 Thread Martin Husemann
On Sun, Jan 24, 2021 at 03:34:08PM +, Jason R Thorpe wrote:
> Module Name:  src
> Committed By: thorpej
> Date: Sun Jan 24 15:34:08 UTC 2021
> 
> Modified Files:
>   src/sys/dev/pci: virtio_pci.c
> 
> Log Message:
> Redefining bus_space functions in drivers is a bad idea, and we just
> should't be in the habit of doing so.  Besides, the previous "solutions"
> still did not compile correctly, and this does, so let's be done with
> this nonsense, shall we?

While I don't care for names, I would like to understand fallout in
details before hiding it - what exactly did not compile correctly?
At least the affected arm kernels worked for me in the state directly
before your commit.

Martin


Re: CVS commit: src/sys/dev/pci

2021-01-23 Thread Jason Thorpe


> On Jan 23, 2021, at 5:40 PM, Christos Zoulas  wrote:
> 
>> it's a bad example.  someone might copy it into another
>> driver that _doesn't_ work with this version, but may seem
>> to fix a build error.
>> 
>> that's why i wanted to wrap the usage to make it clear if
>> someone were to copy it elsewhere.
> 
> I will add a comment.

Yah, I was gonna say, “big fat comment in order!"

-- thorpej



  1   2   3   4   5   6   7   8   9   10   >