Re: uchcom(4): fix Rx jammed
On Mon, Jan 17, 2022 at 10:01:17PM +0900, SASANO Takayoshi wrote:
>
> Hi,
>
> Sometimes uchcom(4) looks Rx jammed.
> Here is the movie (at Mastodon) that the problem has occured:
> https://social.tchncs.de/@uaa/107637913051446044
>
> Receiving an exact wMaxPacketSize bytes bulk packet from CH34x causes
> this problem, because this means USB transaction is not finished.
>
> uchcom(4)'s ibufsize for ucom(4) is too large, it should be same as
> wMaxPacketsize for bulk-in endpoint.
I often encounter the same issue while installing. I have had a similar diff
in my tree. Tested:
uchcom0 at uhub0 port 4 configuration 1 interface 0 "QinHeng Electronics
USB2.0-Serial" rev 1.10/2.63 addr 5
uchcom0: CH341
ucom0 at uchcom0
ok kevlo@
> Here's the diff.
>
> Index: uchcom.c
> ===
> RCS file: /cvs/src/sys/dev/usb/uchcom.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 uchcom.c
> --- uchcom.c 19 Nov 2021 07:58:34 - 1.31
> +++ uchcom.c 17 Jan 2022 12:21:55 -
> @@ -113,7 +113,6 @@ int uchcomdebug = 0;
> #define UCHCOM_RESET_VALUE 0x501F /* line mode? */
> #define UCHCOM_RESET_INDEX 0xD90A /* baud rate? */
>
> -#define UCHCOMIBUFSIZE 256
> #define UCHCOMOBUFSIZE 256
>
> struct uchcom_softc
> @@ -141,6 +140,7 @@ struct uchcom_softc
> struct uchcom_endpoints
> {
> int ep_bulkin;
> + int ep_bulkin_size;
> int ep_bulkout;
> int ep_intr;
> int ep_intr_size;
> @@ -293,9 +293,9 @@ uchcom_attach(struct device *parent, str
> uca.portno = UCOM_UNK_PORTNO;
> uca.bulkin = endpoints.ep_bulkin;
> uca.bulkout = endpoints.ep_bulkout;
> - uca.ibufsize = UCHCOMIBUFSIZE;
> + uca.ibufsize = endpoints.ep_bulkin_size;
> uca.obufsize = UCHCOMOBUFSIZE;
> - uca.ibufsizepad = UCHCOMIBUFSIZE;
> + uca.ibufsizepad = endpoints.ep_bulkin_size;
> uca.opkthdrlen = 0;
> uca.device = dev;
> uca.iface = sc->sc_iface;
> @@ -349,7 +349,7 @@ int
> uchcom_find_endpoints(struct uchcom_softc *sc,
> struct uchcom_endpoints *endpoints)
> {
> - int i, bin=-1, bout=-1, intr=-1, isize=0;
> + int i, bin=-1, bout=-1, intr=-1, binsize=0, isize=0;
> usb_interface_descriptor_t *id;
> usb_endpoint_descriptor_t *ed;
>
> @@ -370,6 +370,7 @@ uchcom_find_endpoints(struct uchcom_soft
> } else if (UE_GET_DIR(ed->bEndpointAddress) == UE_DIR_IN &&
> UE_GET_XFERTYPE(ed->bmAttributes) == UE_BULK) {
> bin = ed->bEndpointAddress;
> + binsize = UGETW(ed->wMaxPacketSize);
> } else if (UE_GET_DIR(ed->bEndpointAddress) == UE_DIR_OUT &&
> UE_GET_XFERTYPE(ed->bmAttributes) == UE_BULK) {
> bout = ed->bEndpointAddress;
> @@ -402,6 +403,7 @@ uchcom_find_endpoints(struct uchcom_soft
> endpoints->ep_intr = intr;
> endpoints->ep_intr_size = isize;
> endpoints->ep_bulkin = bin;
> + endpoints->ep_bulkin_size = binsize;
> endpoints->ep_bulkout = bout;
>
> return 0;
>
> --
> SASANO Takayoshi (JG1UAA)
>
hardware checksum ix and ixl
Hi,
There were some problems with ix(4) and ixl(4) hardware checksumming
for the output path on strict alignment architectures.
I have merged jan@'s diffs and added some sanity checks and
workarounds.
- If the first mbuf is not aligned or not contigous, use m_copydata()
to extract the IP, IPv6, TCP header.
- If the header is in the first mbuf, use m_data for the fast path.
- Add netstat counter for invalid header chains. This makes
us aware when hardware checksumming fails.
- Add netstat counter for header copies. This indicates that
better storage allocation in the network stack is possible.
It also allows to recognize alignment problems on non-strict
architectures.
- There is not risk of crashes on sparc64.
Does this aproach make sense?
ix(4) works quite well, but finds some UDP packets that need copy.
ixl(4) has not been tested yet. I would like to have some feedback
for the idea first.
bluhm
Index: sys/dev/pci/if_ix.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/if_ix.c,v
retrieving revision 1.180
diff -u -p -r1.180 if_ix.c
--- sys/dev/pci/if_ix.c 27 Jul 2021 01:44:55 - 1.180
+++ sys/dev/pci/if_ix.c 25 Jan 2022 23:48:53 -
@@ -1878,8 +1878,8 @@ ixgbe_setup_interface(struct ix_softc *s
#if NVLAN > 0
ifp->if_capabilities |= IFCAP_VLAN_HWTAGGING;
#endif
-
ifp->if_capabilities |= IFCAP_CSUM_TCPv4 | IFCAP_CSUM_UDPv4;
+ ifp->if_capabilities |= IFCAP_CSUM_TCPv6 | IFCAP_CSUM_UDPv6;
/*
* Specify the media types supported by this sc and register
@@ -2437,12 +2437,6 @@ ixgbe_tx_ctx_setup(struct tx_ring *txr,
#else
struct ether_header *eh;
#endif
- struct ip *ip;
-#ifdef notyet
- struct ip6_hdr *ip6;
-#endif
- struct mbuf *m;
- int ipoff;
uint32_t vlan_macip_lens = 0, type_tucmd_mlhl = 0;
int ehdrlen, ip_hlen = 0;
uint16_t etype;
@@ -2511,29 +2505,46 @@ ixgbe_tx_ctx_setup(struct tx_ring *txr,
vlan_macip_lens |= ehdrlen << IXGBE_ADVTXD_MACLEN_SHIFT;
switch (etype) {
- case ETHERTYPE_IP:
- if (mp->m_pkthdr.len < ehdrlen + sizeof(*ip))
+ case ETHERTYPE_IP: {
+ struct ip *ip, ipdata;
+
+ if (mp->m_pkthdr.len < ehdrlen + sizeof(*ip)) {
+ ipstat_inc(ips_outbadcsum);
return (-1);
- m = m_getptr(mp, ehdrlen, );
- KASSERT(m != NULL && m->m_len - ipoff >= sizeof(*ip));
- ip = (struct ip *)(m->m_data + ipoff);
+ }
+ if (((mtod(mp, unsigned long) + ehdrlen) & ALIGNBYTES) == 0 &&
+ mp->m_len >= ehdrlen + sizeof(*ip)) {
+ ip = (struct ip *)(mp->m_data + ehdrlen);
+ } else {
+ ipstat_inc(ips_outcpycsum);
+ m_copydata(mp, ehdrlen, sizeof(ipdata), );
+ ip =
+ }
ip_hlen = ip->ip_hl << 2;
ipproto = ip->ip_p;
type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_IPV4;
break;
-#ifdef notyet
- case ETHERTYPE_IPV6:
- if (mp->m_pkthdr.len < ehdrlen + sizeof(*ip6))
+ }
+ case ETHERTYPE_IPV6: {
+ struct ip6_hdr *ip6, ip6data;
+
+ if (mp->m_pkthdr.len < ehdrlen + sizeof(*ip6)) {
+ ip6stat_inc(ip6s_outbadcsum);
return (-1);
- m = m_getptr(mp, ehdrlen, );
- KASSERT(m != NULL && m->m_len - ipoff >= sizeof(*ip6));
- ip6 = (struct ip6 *)(m->m_data + ipoff);
+ }
+ if (((mtod(mp, unsigned long) + ehdrlen) & ALIGNBYTES) == 0 &&
+ mp->m_len >= ehdrlen + sizeof(*ip6)) {
+ ip6 = (struct ip6_hdr *)(mp->m_data + ehdrlen);
+ } else {
+ ip6stat_inc(ip6s_outcpycsum);
+ m_copydata(mp, ehdrlen, sizeof(ip6data), );
+ ip6 =
+ }
ip_hlen = sizeof(*ip6);
- /* XXX-BZ this will go badly in case of ext hdrs. */
ipproto = ip6->ip6_nxt;
type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_IPV6;
break;
-#endif
+ }
default:
offload = FALSE;
break;
@@ -2552,6 +2563,10 @@ ixgbe_tx_ctx_setup(struct tx_ring *txr,
type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_L4T_UDP;
break;
default:
+ if (mp->m_pkthdr.csum_flags & M_TCP_CSUM_OUT)
+ tcpstat_inc(tcps_outbadcsum);
+ if (mp->m_pkthdr.csum_flags & M_UDP_CSUM_OUT)
+ udpstat_inc(udps_outbadcsum);
offload = FALSE;
break;
}
Index: sys/dev/pci/if_ixl.c
FAT32 short filename inconsistencies
Hi, CC afresh1@, with whom I've discussed this a little already. If I make a FAT32 filesystem on a thumb drive on OpenBSD (using the default parameters) and mount it (with the default options) and proceed to make some strategically named files: ``` # touch a B a B longlonglong OPENBSD_OPENBSD # ls B B OPENBSD_OPENBSD a a longlonglong ``` And now mount that filesystem (with default options) on a Linux box, we see that some of the files appear with different case from how they were created above: ``` # ls a a b b longlonglong OPENBSD_OPENBSD ``` After reading about the mess they made with FAT filesystems, what I think happened was: OpenBSD made "long filename" entries for names that either didn't fit in the 8.3 scheme, or which were not all upper-case (the assumption, I think, being that files with no long filename will be displayed all upper-case anyway, at least on OpenBSD). The problem is that entries with no long filename seem to be open to interpretation on different systems: - OpenBSD shows them upper-case - Linux (by default) shows them lower case. If on Linux you mount with `-o shortname=win95`, then you get the same interpretation as OpenBSD. But is there (or should there be) a way to make OpenBSD display the filenames as Linux would? If everything above is correct, I think that would mean either creating all files (even if they fit in 8.3) with a long filename, or changing OpenBSD's interpretation of short-only filenames to be lower case. I see no mount_msdos(8) options that may help. Note that `-l` is the default: > -lForce listing and generation of Windows 95/98 long filenames and > separate creation/modification/access dates. I'm not sure if this is completely accurate. If we were really forcing generation of long filenames, then the files `B` and `B` should have had long filename entries, but they didn't appear to. Should the sentence be suffixed "if the filename doesn't fit in the 8.3 scheme"? Or perhaps the behaviour `-l` should be changed to *really always* create long filenames regardless of the length? I don't know. Why do I ask? I need to copy lots of files, preserving case, to an SD card for use in an embedded Linux machine on which I don't have control over the filesystem type or mount options. [One hack that I think would work for me, would be to rename all of my files lower-case before copying to the FAT filesystem. This would force OpenBSD to create long filenames, but that may not be suitable for every user's circumstance] Cheers -- Best Regards Edd Barrett https://www.theunixzoo.co.uk
Re: syslogd(8): Add hostname parsing support
Thanks for looking into this.
On Sat, 2022-01-22 at 10:05 -0700, Theo de Raadt wrote:
> > Note that this only adds the parsing, the rest of the current behaviour
> > of stays the same. I have another diff in the pipeline for allowing the
> > hostname in the message.
>
> I object to this process.
>
> You want to add parsing code as a fait accompli. With no justification.
> Then later on, spring on us the code that uses it.
Sorry if my phrasing was off, but I do think the diff stands on its own
and I thought I made that case in the first paragraph. Even though I
include the mentioned follow up diffs below, let me try to rephrase my
case for it as a stand alone diff.
Right now we have 3 elements where syslog.conf can filter on: priority,
hostname, and progname. hostname currently always comes from the input
source:
- sendsyslog(2), /dev/klog, unix socket, vlogmsg(), and markit() use
LocalHostName
- UDP/TCP/TLS print reverse lookup or numerical if -n is specified
My previous parsemsg() diff didn't change any behaviour, but makes it
more clear what's going on. When a message is handled through
printline() it goes through parsemsg(). Here we first check for an
optional priority and timestamp. parsemsg_priority() is the original
priority parsing code and parsemsg_timestamp_{bsd,v1} are the original
code lifted from logmsg. After that we continue to parsemsg_prog, which
is also lifted from logmsg(). In other words, at no point do we look
for a hostname in a message.
So if someone were to set -h in syslogd or use any other syslog server
that includes a hostname in the message and would relay it to a
syslogd(8) it would first parse priority/timestamp (if available) and
then continue to interpret the "first word" as progname. So a message
like: "<13>Jan 25 20:35:44 myhost myprog: mymsg" send over localhost
would have myhost as progname and localhost (or 127.0.0.1 if -n is
used) as hostname. So if someone would have "!myprog" in their
syslog.conf it would not match (while it should) and someone who
would have "!myhost" in their syslog.conf would match, while it
shouldn't.
What my diff does is based mainly on syslog(3) (and happens to match
what I've mostly seen in the wild) by saying that a progname should end
in a ':' or a '['. This is also what NetBSD does. By this minor change
we can reliably determine what the progname is and by that merit
determine what the hostname is.
The hostname in the original diff is stored in struct msg and not looked
at again, effectively replacing the hostname with the address found by
the input source. This behaviour should be identical to FreeBSD, which
has the -H flag to preserve the hostname in the message. To make this
explicit, the example message previously would be written out as:
"<13>Jan 25 20:35:44 localhost myprog: mymsg" instead of the current
"<13>Jan 25 20:35:44 localhost myhost myprog: mymsg"
and "+localhost" would continue to keep working as always.
> What if we disagree
> with the code that uses it? Will you delete this parsing code which
> nothing uses?
Fixing the "!progname" case should be sufficient reason on its own and
no other.
>
> > - Timestamp: is easy to interpret, since it's a strict format.
> > No changes here.
>
> I believe "timestamp missing" is not strictly permitted. But this was
> common for a while, and in OpenBSD it is the default message format.
> This is a due to the desire to make syslog_r(3) be signal/re-entrant
> safe on top of sendsyslog(2). Then there is no good way of creating a
> timestamp string in the sender libc context. A timestamp is easily
> re-inserted by the receiving syslogd.
>
> > + for (i = 0; i < HOST_NAME_MAX; i++) {
>
> Unlike MAXHOSTNAMELEN/NI_MAXHOST, HOST_NAME_MAX storage does not include
> a NUL. You might have the loop right. Be careful.
I've defined it as "char m_hostname[HOST_NAME_MAX + 1];", so we have room
for the NUL.
>
> > +* fqdn: [[:alnum:]-.]
>
> That is not totally correct.
>
> hostnames very often also contain '_' in the middle positions, early
> RFCs said no, but around 1990-ish Vixie in particular had to face
> reality.. I was involved in that conversation, it seems so long ago.
>
> Your pattern is also incorrect in other ways, such as ".." is not legal,
> hostnames cannot start or end with '-' or '-', etc. The current accepted
> rules are encoded in the undocumented libc function __res_hnok in
> lib/libc/net/res_comp.c
I wasn't aware of res_hnok. I see that it's used in other applications as
well. I changed it to use strcspn to look for the terminating space and
try inet_pton and res_hnok to check if it's valid. This makes the code a
lot cleaner. Thanks.
>
> I don't know if false-identification of broken hostnames is bad or not
> I guess it depends what will happen with this information later on [ie.
> the part of your proposal that is being kept a secret].
The same thing that already happens. Writing it out to log-files and
using it to match on
Re: perl clang -Wcompound-token-split-by-macro
On Tue, Jan 25, 2022 at 09:20:55PM +0100, Alexander Bluhm wrote: > On Tue, Jan 25, 2022 at 12:05:48PM -0800, Andrew Hewus Fresh wrote: > > On Tue, Jan 25, 2022 at 06:45:12PM +0100, Alexander Bluhm wrote: > > > On Tue, Jan 25, 2022 at 05:13:01PM +0100, Alexander Bluhm wrote: > > > > On Sat, Jan 22, 2022 at 02:24:51AM +0100, Marc Espie wrote: > > > > > Or we can automate this with something like this: > > > > > > > > > > Our Devel::PPPort is too old. We ship with 3.57, p5-CDB_File and p5-Moose > > > ship with ppport.h generated by 3.62. > > > > We could update Devel::PPPort in base. > > This will fix the current problem. But we can always have an old > Devel::PPPort in base and a have module in ports that comes with > and needs a new ppport.h. > > Somehow the porter should have a mechanism to handle this. Only 2 > of 300 ports that I test have issues, so it is a rare action. Per > default espie@'s idea works well. If we manually fix 2 ports that > is fine for me. > > Or we replace ppport.h only if it is outdated. We could add espie's target to those two ports. > > I do plan to work on getting 5.34 in after I finish my fw_update(8) TODO > > list and get the "vendor lib" patch committed. I'm not sure that will > > make 7.1 though. > > Plans for new Perl are good to hear. Thanks for updating it regualry. Sure thing, some exciting stuff coming in the future. I think signatures may actually make it out of experimental status. l8rZ, -- andrew Adding manpower to a late software project makes it later.
in4_cksum changes, step 1
in4_cksum(), used to compute packet checksums for the legacy internet
protocol, has been hand-optimized for speed on most elderly platforms,
with the most recent pieces of silicon using a portable C
implementation.
Most of these implementations, in a not-so-uncommon case, need to
checksum an extra (``overlay'') header, and invoke memset or bzero on
such an overlay, prior to initializing it and checksumming it.
However, except for one byte, the zeroed parts are useless since they
will not change the checksum, so that memset/bzero call can be removed,
and the sum can omit the first 8 bytes which will always be zero.
The following diff implements that idea. Plus on sparc64 you get one
useless assembly instruction removed, for free, isn't that awesome?
Affected platforms: alpha, amd64, arm64, hppa, landisk, luna88k, macppc,
octeon, powerpc64, riscv64, sparc64. No need to test on armv7 and i386.
Index: sys/arch/alpha/alpha/in_cksum.c
===
RCS file: /OpenBSD/src/sys/arch/alpha/alpha/in_cksum.c,v
retrieving revision 1.9
diff -u -p -r1.9 in_cksum.c
--- sys/arch/alpha/alpha/in_cksum.c 21 Aug 2014 14:24:08 - 1.9
+++ sys/arch/alpha/alpha/in_cksum.c 25 Jan 2022 20:21:06 -
@@ -212,14 +212,13 @@ in4_cksum(struct mbuf *m, u_int8_t nxt,
panic("in4_cksum: bad mbuf chain");
#endif
- memset(, 0, sizeof(ipov));
-
- ipov.ih_len = htons(len);
+ ipov.ih_x1[8] = 0;
ipov.ih_pr = nxt;
+ ipov.ih_len = htons(len);
ipov.ih_src = mtod(m, struct ip *)->ip_src;
ipov.ih_dst = mtod(m, struct ip *)->ip_dst;
- sum += in_cksumdata((caddr_t) , sizeof(ipov));
+ sum += in_cksumdata((caddr_t) + 8, sizeof(ipov) - 8);
}
/* skip over unnecessary part */
Index: sys/arch/m88k/m88k/in_cksum.c
===
RCS file: /OpenBSD/src/sys/arch/m88k/m88k/in_cksum.c,v
retrieving revision 1.4
diff -u -p -r1.4 in_cksum.c
--- sys/arch/m88k/m88k/in_cksum.c 21 Aug 2014 14:24:08 - 1.4
+++ sys/arch/m88k/m88k/in_cksum.c 25 Jan 2022 20:21:07 -
@@ -95,19 +95,22 @@ in4_cksum(struct mbuf *m, uint8_t nxt, i
{
u_int16_t *w;
u_int sum = 0;
- struct ipovly ipov;
+ union {
+ struct ipovly ipov;
+ u_int16_t w[10];
+ } u;
if (nxt != 0) {
/* pseudo header */
- bzero(, sizeof(ipov));
- ipov.ih_len = htons(len);
- ipov.ih_pr = nxt;
- ipov.ih_src = mtod(m, struct ip *)->ip_src;
- ipov.ih_dst = mtod(m, struct ip *)->ip_dst;
- w = (u_int16_t *)
- /* assumes sizeof(ipov) == 20 */
- sum += w[0]; sum += w[1]; sum += w[2]; sum += w[3]; sum += w[4];
- sum += w[5]; sum += w[6]; sum += w[7]; sum += w[8]; sum += w[9];
+ u.ipov.ih_x1[8] = 0;
+ u.ipov.ih_pr = nxt;
+ u.ipov.ih_len = htons(len);
+ u.ipov.ih_src = mtod(m, struct ip *)->ip_src;
+ u.ipov.ih_dst = mtod(m, struct ip *)->ip_dst;
+ w = u.w;
+ /* assumes sizeof(ipov) == 20 and first 8 bytes are zeroes */
+ sum += w[4]; sum += w[5]; sum += w[6];
+ sum += w[7]; sum += w[8]; sum += w[9];
}
/* skip unnecessary part */
Index: sys/arch/powerpc/powerpc/in_cksum.c
===
RCS file: /OpenBSD/src/sys/arch/powerpc/powerpc/in_cksum.c,v
retrieving revision 1.10
diff -u -p -r1.10 in_cksum.c
--- sys/arch/powerpc/powerpc/in_cksum.c 22 Jul 2014 10:35:35 - 1.10
+++ sys/arch/powerpc/powerpc/in_cksum.c 25 Jan 2022 20:21:07 -
@@ -254,15 +254,15 @@ in4_cksum(struct mbuf *m, uint8_t nxt, i
if (nxt != 0) {
/* pseudo header */
- memset(, 0, sizeof(u.ipov));
- u.ipov.ih_len = htons(len);
+ u.ipov.ih_x1[8] = 0;
u.ipov.ih_pr = nxt;
+ u.ipov.ih_len = htons(len);
u.ipov.ih_src = mtod(m, struct ip *)->ip_src;
u.ipov.ih_dst = mtod(m, struct ip *)->ip_dst;
w = u.w;
- /* assumes sizeof(ipov) == 20 */
- sum += w[0]; sum += w[1]; sum += w[2]; sum += w[3]; sum += w[4];
- sum += w[5]; sum += w[6]; sum += w[7]; sum += w[8]; sum += w[9];
+ /* assumes sizeof(ipov) == 20 and first 8 bytes are zeroes */
+ sum += w[4]; sum += w[5]; sum += w[6];
+ sum += w[7]; sum += w[8]; sum += w[9];
}
/* skip unnecessary part */
Index: sys/arch/sparc64/sparc64/in4_cksum.c
===
RCS file:
Re: perl clang -Wcompound-token-split-by-macro
On Tue, Jan 25, 2022 at 12:05:48PM -0800, Andrew Hewus Fresh wrote: > On Tue, Jan 25, 2022 at 06:45:12PM +0100, Alexander Bluhm wrote: > > On Tue, Jan 25, 2022 at 05:13:01PM +0100, Alexander Bluhm wrote: > > > On Sat, Jan 22, 2022 at 02:24:51AM +0100, Marc Espie wrote: > > > > Or we can automate this with something like this: > > > > > > > Our Devel::PPPort is too old. We ship with 3.57, p5-CDB_File and p5-Moose > > ship with ppport.h generated by 3.62. > > We could update Devel::PPPort in base. This will fix the current problem. But we can always have an old Devel::PPPort in base and a have module in ports that comes with and needs a new ppport.h. Somehow the porter should have a mechanism to handle this. Only 2 of 300 ports that I test have issues, so it is a rare action. Per default espie@'s idea works well. If we manually fix 2 ports that is fine for me. Or we replace ppport.h only if it is outdated. > I do plan to work on getting 5.34 in after I finish my fw_update(8) TODO > list and get the "vendor lib" patch committed. I'm not sure that will > make 7.1 though. Plans for new Perl are good to hear. Thanks for updating it regualry. > > ppport.h -- Perl/Pollution/Portability Version 3.57 > > Automatically created by Devel::PPPort running under perl 5.032001. > > > > ppport.h -- Perl/Pollution/Portability Version 3.62 > > Automatically created by Devel::PPPort running under perl 5.032000. > > > > ppport.h -- Perl/Pollution/Portability Version 3.62 > > Automatically created by Devel::PPPort running under perl 5.033008. > > > > bluhm > >
Re: perl clang -Wcompound-token-split-by-macro
On Tue, Jan 25, 2022 at 06:45:12PM +0100, Alexander Bluhm wrote: > On Tue, Jan 25, 2022 at 05:13:01PM +0100, Alexander Bluhm wrote: > > On Sat, Jan 22, 2022 at 02:24:51AM +0100, Marc Espie wrote: > > > Or we can automate this with something like this: > > > > Our Devel::PPPort is too old. We ship with 3.57, p5-CDB_File and p5-Moose > ship with ppport.h generated by 3.62. We could update Devel::PPPort in base. I do plan to work on getting 5.34 in after I finish my fw_update(8) TODO list and get the "vendor lib" patch committed. I'm not sure that will make 7.1 though. > > ppport.h -- Perl/Pollution/Portability Version 3.57 > Automatically created by Devel::PPPort running under perl 5.032001. > > ppport.h -- Perl/Pollution/Portability Version 3.62 > Automatically created by Devel::PPPort running under perl 5.032000. > > ppport.h -- Perl/Pollution/Portability Version 3.62 > Automatically created by Devel::PPPort running under perl 5.033008. > > bluhm >
Re: Properly check if ACPI devices are enabled
> Date: Tue, 25 Jan 2022 19:44:23 +0100 (CET) > From: Mark Kettenis > > > Date: Tue, 25 Jan 2022 07:01:41 +0100 > > From: Anton Lindqvist > > > > On Mon, Jan 24, 2022 at 08:40:36PM +0100, Mark Kettenis wrote: > > > > Date: Mon, 24 Jan 2022 20:19:46 +0100 > > > > From: Anton Lindqvist > > > > > > > > On Mon, Jan 24, 2022 at 05:31:49PM +0100, Mark Kettenis wrote: > > > > > Currently we attach ACPI devices that are present in a machine. > > > > > However, in some cases ACPI devices can be present, but not enabled. > > > > > Attaching a device driver to devices that are not enabled is not a > > > > > good idea since reading and writing from/to its registers will fail > > > > > and the driver will malfunction in interesting ways. Such as a com(4) > > > > > serial port that is misdetected and hangs the kernel when it is > > > > > actually opened. > > > > > > > > > > The diff below makes sure we only enable devices that are actually > > > > > enabled. This may cause some devices to disappear in OpenBSD. > > > > > However those devices should have been unusable anyway, so that isn't > > > > > an issue. > > > > > > > > > > ok? > > > > > > > > According to the ACPI specification[1]: > > > > > > > > > A device can only decode its hardware resources if both bits 0 and 1 > > > > > are > > > > > set. If the device is not present (bit [0] cleared) or not enabled > > > > > (bit > > > > > [1] cleared), then the device must not decode its resources. > > > > > > Just before that it says: > > > > > > If bit [0] is cleared, then bit 1 must also be cleared (in other > > > words, a device that is not present cannot be enabled). > > > > > > > Should we therefore check for presence of both STA_PRESENT and > > > > STA_ENABLED? > > > > > > So according to the ACPI specification we don't need to do that. > > > Should we do it just to be safe? > > > > I would still vote for it. > > Since several people pointed this out, here is a diff that checks both > bits. > > ok? No. As a fellow developer pointed out, this is wrong. Correct diff below. Index: dev/acpi/acpi.c === RCS file: /cvs/src/sys/dev/acpi/acpi.c,v retrieving revision 1.405 diff -u -p -r1.405 acpi.c --- dev/acpi/acpi.c 12 Jan 2022 11:18:30 - 1.405 +++ dev/acpi/acpi.c 25 Jan 2022 18:59:52 - @@ -3321,7 +3321,7 @@ acpi_foundhid(struct aml_node *node, voi return (0); sta = acpi_getsta(sc, node->parent); - if ((sta & STA_PRESENT) == 0) + if ((sta & (STA_PRESENT | STA_ENABLED)) != (STA_PRESENT | STA_ENABLED)) return (0); if (aml_evalinteger(sc, node->parent, "_CCA", 0, NULL, ))
Re: Properly check if ACPI devices are enabled
> Date: Tue, 25 Jan 2022 07:01:41 +0100 > From: Anton Lindqvist > > On Mon, Jan 24, 2022 at 08:40:36PM +0100, Mark Kettenis wrote: > > > Date: Mon, 24 Jan 2022 20:19:46 +0100 > > > From: Anton Lindqvist > > > > > > On Mon, Jan 24, 2022 at 05:31:49PM +0100, Mark Kettenis wrote: > > > > Currently we attach ACPI devices that are present in a machine. > > > > However, in some cases ACPI devices can be present, but not enabled. > > > > Attaching a device driver to devices that are not enabled is not a > > > > good idea since reading and writing from/to its registers will fail > > > > and the driver will malfunction in interesting ways. Such as a com(4) > > > > serial port that is misdetected and hangs the kernel when it is > > > > actually opened. > > > > > > > > The diff below makes sure we only enable devices that are actually > > > > enabled. This may cause some devices to disappear in OpenBSD. > > > > However those devices should have been unusable anyway, so that isn't > > > > an issue. > > > > > > > > ok? > > > > > > According to the ACPI specification[1]: > > > > > > > A device can only decode its hardware resources if both bits 0 and 1 are > > > > set. If the device is not present (bit [0] cleared) or not enabled (bit > > > > [1] cleared), then the device must not decode its resources. > > > > Just before that it says: > > > > If bit [0] is cleared, then bit 1 must also be cleared (in other > > words, a device that is not present cannot be enabled). > > > > > Should we therefore check for presence of both STA_PRESENT and > > > STA_ENABLED? > > > > So according to the ACPI specification we don't need to do that. > > Should we do it just to be safe? > > I would still vote for it. Since several people pointed this out, here is a diff that checks both bits. ok? Index: dev/acpi/acpi.c === RCS file: /cvs/src/sys/dev/acpi/acpi.c,v retrieving revision 1.405 diff -u -p -r1.405 acpi.c --- dev/acpi/acpi.c 12 Jan 2022 11:18:30 - 1.405 +++ dev/acpi/acpi.c 25 Jan 2022 18:43:25 - @@ -3321,7 +3321,7 @@ acpi_foundhid(struct aml_node *node, voi return (0); sta = acpi_getsta(sc, node->parent); - if ((sta & STA_PRESENT) == 0) + if ((sta & (STA_PRESENT | STA_ENABLED)) == (STA_PRESENT | STA_ENABLED)) return (0); if (aml_evalinteger(sc, node->parent, "_CCA", 0, NULL, ))
Re: perl clang -Wcompound-token-split-by-macro
On Tue, Jan 25, 2022 at 05:13:01PM +0100, Alexander Bluhm wrote:
> On Sat, Jan 22, 2022 at 02:24:51AM +0100, Marc Espie wrote:
> > Or we can automate this with something like this:
> >
> > Index: perl.port.mk
> > ===
> > RCS file: /cvs/ports/infrastructure/mk/perl.port.mk,v
> > retrieving revision 1.32
> > diff -u -p -r1.32 perl.port.mk
> > --- perl.port.mk12 Dec 2021 19:25:39 - 1.32
> > +++ perl.port.mk21 Jan 2022 17:39:18 -
> > @@ -56,6 +56,11 @@ MODPERL_pre-configure = for f in ${MODPE
> > ${MODPERL_BIN_ADJ} ${WRKSRC}/$${f}; done
> > .endif
> >
> > +MODPERL_gen = cd ${WRKDIST} && \
> > + if test -f ppport.h; then \
> > + perl -MDevel::PPPort -e'Devel::PPPort::WriteFile'; \
> > + fi
> > +
> > .if ${CONFIGURE_STYLE:L:Mmodbuild}
> > MODPERL_configure = \
> > cd ${WRKSRC}; ${SETENV} ${CONFIGURE_ENV} \
>
> I have tried it with my Perl ports testing site.
> http://bluhm.genua.de/portstest/results/regress-ot29.html
>
> Logfile size went from 19.2 to 2.9 MB. Before I had 17590
> compound-token-split-by-macro warnings, now there are 464 left.
>
> databases/p5-DBI and devel/p5-YAML-XS do something special and still
> generate warnings.
>
> databases/p5-CDB_File and devel/p5-Moose fail to build.
> http://bluhm.genua.de/portstest/results/2022-01-25T13%3A50%3A36Z/logs/databases/p5-CDB_File/make.log
> http://bluhm.genua.de/portstest/results/2022-01-25T13%3A50%3A36Z/logs/devel/p5-Moose/make.log
>
> CDB_File.xs:74:5: error: function-like macro 'PERL_VERSION_LE' is not defined
> #if PERL_VERSION_LE(5,13,7)
> ^
> CDB_File.xs:322:5: error: function-like macro 'PERL_VERSION_GT' is not defined
> #if PERL_VERSION_GT(5,13,7)
> ^
> mop.c:13:5: error: function-like macro 'PERL_VERSION_GE' is not defined
> #if PERL_VERSION_GE(5,10,0)
> ^
Our Devel::PPPort is too old. We ship with 3.57, p5-CDB_File and p5-Moose
ship with ppport.h generated by 3.62.
ppport.h -- Perl/Pollution/Portability Version 3.57
Automatically created by Devel::PPPort running under perl 5.032001.
ppport.h -- Perl/Pollution/Portability Version 3.62
Automatically created by Devel::PPPort running under perl 5.032000.
ppport.h -- Perl/Pollution/Portability Version 3.62
Automatically created by Devel::PPPort running under perl 5.033008.
bluhm
Re: perl clang -Wcompound-token-split-by-macro
On Sat, Jan 22, 2022 at 02:24:51AM +0100, Marc Espie wrote:
> Or we can automate this with something like this:
>
> Index: perl.port.mk
> ===
> RCS file: /cvs/ports/infrastructure/mk/perl.port.mk,v
> retrieving revision 1.32
> diff -u -p -r1.32 perl.port.mk
> --- perl.port.mk 12 Dec 2021 19:25:39 - 1.32
> +++ perl.port.mk 21 Jan 2022 17:39:18 -
> @@ -56,6 +56,11 @@ MODPERL_pre-configure = for f in ${MODPE
> ${MODPERL_BIN_ADJ} ${WRKSRC}/$${f}; done
> .endif
>
> +MODPERL_gen = cd ${WRKDIST} && \
> + if test -f ppport.h; then \
> + perl -MDevel::PPPort -e'Devel::PPPort::WriteFile'; \
> + fi
> +
> .if ${CONFIGURE_STYLE:L:Mmodbuild}
> MODPERL_configure = \
> cd ${WRKSRC}; ${SETENV} ${CONFIGURE_ENV} \
I have tried it with my Perl ports testing site.
http://bluhm.genua.de/portstest/results/regress-ot29.html
Logfile size went from 19.2 to 2.9 MB. Before I had 17590
compound-token-split-by-macro warnings, now there are 464 left.
databases/p5-DBI and devel/p5-YAML-XS do something special and still
generate warnings.
databases/p5-CDB_File and devel/p5-Moose fail to build.
http://bluhm.genua.de/portstest/results/2022-01-25T13%3A50%3A36Z/logs/databases/p5-CDB_File/make.log
http://bluhm.genua.de/portstest/results/2022-01-25T13%3A50%3A36Z/logs/devel/p5-Moose/make.log
CDB_File.xs:74:5: error: function-like macro 'PERL_VERSION_LE' is not defined
#if PERL_VERSION_LE(5,13,7)
^
CDB_File.xs:322:5: error: function-like macro 'PERL_VERSION_GT' is not defined
#if PERL_VERSION_GT(5,13,7)
^
mop.c:13:5: error: function-like macro 'PERL_VERSION_GE' is not defined
#if PERL_VERSION_GE(5,10,0)
^
bluhm
Re: request for testing: malloc and large allocations
On Sat, Jan 22, 2022 at 09:25:25AM +0100, Otto Moerbeek wrote: > On Mon, Jan 17, 2022 at 08:42:47AM +0100, Otto Moerbeek wrote: > > > On Sun, Jan 09, 2022 at 02:54:43PM +0100, Otto Moerbeek wrote: > > > > > Hi, > > > > > > currently malloc does cache a number of free'ed regions up to 128k in > > > size. This cache is indexed by size (in # of pages), so it is very > > > quick to check. > > > > > > Some programs allocate and deallocate larger allocations in a frantic > > > way. Accodomate those programs by also keeping a cache of regions > > > betwen 128k and 2M, in a cache of variable sized regions. > > > > > > My test case speeds up about twice. A make build gets a small speedup. > > > > > > This has been tested by myself on amd64 quite intensively. I am asking > > > for more tests, especialy on more "exotic" platforms. I wil do arm64 > > > myself soon. Test can be running your favorite programs, doing make > > > builds or running tests in regress/lib/libc/malloc. > > > > > > Thanks in advance! > > > > > > I received several success reports and one failure on macppc report > > from Miod. I'm investiging that report to see if this diff can be > > blamed. > > > > In the meantime: keep on testing! > > > > -Otto > > So far I have been unable to reproduce. I would like confirmation > either way from somebody else having a macppc machine. Any volunteer? > > Thanks, > > -Otto It turns out the macppc troubles have ben resolved by an uvm commit. -Otto
Re: fix active scan on iwm and iwx
> Date: Tue, 25 Jan 2022 10:00:45 +0100 > From: Stefan Sperling > > On Tue, Jan 25, 2022 at 09:32:21AM +0100, Mark Kettenis wrote: > > Happened again while still on a Jan 16 snapshot kernel. So it is not > > related to that diff. > > > > Here is the panic message and backtrace: > > > > panic: kernel diagnostic assertion "sc->task_refs.refs == 0" failed: file > > "/usr/src/sys/dev/pci/if_iwm.c", line 9981 > > Stopped at db_enter+0x10: popq%rbp > > TIDPIDUID PRFLAGS PFLAGS CPU COMMAND > > *120744 85293 0 0x3 00K ifconfig > > db_enter() at db_enter+0x10 > > panic() at panic+0xbf > > __assert() at __assert+0x25 > > iwm_init() at iwm_init+0x254 > > iwm_ioctl() at iwm_ioctl+0xf9 > > ifioctl() at ifioctl+0x92b > > soo_ioctl() at soo_ioctl+0x161 > > sys_ioctl() at sys_ioctl+0x2c4 > > syscall() at syscall+0x374 > > Xsyscall() at Xsyscall+0x128 > > end of kernel > > > > Please try this patch. > > Upon resume we, set the task ref count to 1 in anticipation of > the newstate task that will be triggered to move into SCAN state. > iwm_add_task would bump the refcount to 2. The task would decrease > refcount again when it is done, and refcnt_finalize() in iwm_stop() > would eventually let the counter drop back to zero. > > Now, for some reason on your system the device is not responding to > the driver's attempt to claim ownership. This looks like maybe some > problem with the bus the device is attached to. I am not in a position > to debug that issue, perhaps you could try? In any case, iwx_wakeup() > errors out early, with task refcount 1 but with no task scheduled and > IFF_RUNNING not set (meaning ioctl will not call iwm_stop()). Yes, something still seems to be not 100% in the resume path. I'll see what I can do, but it happens infrequently. It may actually have started with the drm update. So maybe it is just a timing issue that happens because something in drm holds the kernel lock a bit longer during resume... > Later, you run ifconfig, the ioctl handler runs, and calls iwm_init(). > This function expects that we are in a clean initial state (as after boot), > such that iwm_stop() was called beforehand to clear out any tasks, dropping > the task ref counter back to zero. > > The KASSERT triggers but for the wrong reason: We don't have outstanding > tasks, we have a bad reference counter. Only setting the ref counter to 1 if > we are about to launch a task during resume should fix it, and this matches > what iwx(4) is doing: Running this now. May take some time to reproduce the issue though. > diff d26399562c831a7212cebc57463cc9931ff8aff2 /usr/src > blob - 937f2cc28f6c85502031e4c9efa0a02c75fd1a6d > file + sys/dev/pci/if_iwm.c > --- sys/dev/pci/if_iwm.c > +++ sys/dev/pci/if_iwm.c > @@ -11719,8 +11719,6 @@ iwm_wakeup(struct iwm_softc *sc) > struct ifnet *ifp = >sc_ic.ic_if; > int err; > > - refcnt_init(>task_refs); > - > err = iwm_start_hw(sc); > if (err) > return err; > @@ -11729,6 +11727,7 @@ iwm_wakeup(struct iwm_softc *sc) > if (err) > return err; > > + refcnt_init(>task_refs); > ifq_clr_oactive(>if_snd); > ifp->if_flags |= IFF_RUNNING; > >
Re: fix active scan on iwm and iwx
On Tue, Jan 25, 2022 at 09:32:21AM +0100, Mark Kettenis wrote: > Happened again while still on a Jan 16 snapshot kernel. So it is not > related to that diff. > > Here is the panic message and backtrace: > > panic: kernel diagnostic assertion "sc->task_refs.refs == 0" failed: file > "/usr/src/sys/dev/pci/if_iwm.c", line 9981 > Stopped at db_enter+0x10: popq%rbp > TIDPIDUID PRFLAGS PFLAGS CPU COMMAND > *120744 85293 0 0x3 00K ifconfig > db_enter() at db_enter+0x10 > panic() at panic+0xbf > __assert() at __assert+0x25 > iwm_init() at iwm_init+0x254 > iwm_ioctl() at iwm_ioctl+0xf9 > ifioctl() at ifioctl+0x92b > soo_ioctl() at soo_ioctl+0x161 > sys_ioctl() at sys_ioctl+0x2c4 > syscall() at syscall+0x374 > Xsyscall() at Xsyscall+0x128 > end of kernel > Please try this patch. Upon resume we, set the task ref count to 1 in anticipation of the newstate task that will be triggered to move into SCAN state. iwm_add_task would bump the refcount to 2. The task would decrease refcount again when it is done, and refcnt_finalize() in iwm_stop() would eventually let the counter drop back to zero. Now, for some reason on your system the device is not responding to the driver's attempt to claim ownership. This looks like maybe some problem with the bus the device is attached to. I am not in a position to debug that issue, perhaps you could try? In any case, iwx_wakeup() errors out early, with task refcount 1 but with no task scheduled and IFF_RUNNING not set (meaning ioctl will not call iwm_stop()). Later, you run ifconfig, the ioctl handler runs, and calls iwm_init(). This function expects that we are in a clean initial state (as after boot), such that iwm_stop() was called beforehand to clear out any tasks, dropping the task ref counter back to zero. The KASSERT triggers but for the wrong reason: We don't have outstanding tasks, we have a bad reference counter. Only setting the ref counter to 1 if we are about to launch a task during resume should fix it, and this matches what iwx(4) is doing: diff d26399562c831a7212cebc57463cc9931ff8aff2 /usr/src blob - 937f2cc28f6c85502031e4c9efa0a02c75fd1a6d file + sys/dev/pci/if_iwm.c --- sys/dev/pci/if_iwm.c +++ sys/dev/pci/if_iwm.c @@ -11719,8 +11719,6 @@ iwm_wakeup(struct iwm_softc *sc) struct ifnet *ifp = >sc_ic.ic_if; int err; - refcnt_init(>task_refs); - err = iwm_start_hw(sc); if (err) return err; @@ -11729,6 +11727,7 @@ iwm_wakeup(struct iwm_softc *sc) if (err) return err; + refcnt_init(>task_refs); ifq_clr_oactive(>if_snd); ifp->if_flags |= IFF_RUNNING;
Re: fix active scan on iwm and iwx
> Date: Fri, 21 Jan 2022 16:18:28 +0100 (CET) > From: Mark Kettenis > > > Date: Fri, 21 Jan 2022 16:05:49 +0100 > > From: Stefan Sperling > > > > On Sun, Jan 16, 2022 at 07:38:11PM +0100, Mark Kettenis wrote: > > > > Date: Sun, 16 Jan 2022 19:28:06 +0100 > > > > From: Stefan Sperling > > > > > > > > On Sun, Jan 16, 2022 at 03:50:55PM +0100, Mark Kettenis wrote: > > > > > However, running this diff I had a problem after resuming my laptop > > > > > twice. After resume the interface didn't work and I found the > > > > > following in dmesg: > > > > > > > > > > iwm0: could not initialize hardware > > > > > > > > > > I tried to reset the interface by bringing it down and up again, which > > > > > crashed the machine. It must have been in ddb since typing "bo re" > > > > > made it reset. Unfortunately I don't have further information since I > > > > > was in X. > > > > > > > > Did you try reproducing this problem without the patch in place? > > > > It would be good to know whether this problem is being introduced by > > > > this patch. I don't believe this is likely, my bet would be that this > > > > is an existing problem. But it would be good to know for sure. > > > > > > Yes. I switched back to regular snapshots. Will keep you posted. > > > > > > > Any news? > > > > I have unsuccessfully tried to reproduce this problem on a laptop > > with a 9560 iwm device, via both S3 suspend and hibernate. > > I do not have a 9260 device in a machine which can suspend, unfortunately. > > > > This was not a problem that occurred for you consistently, was it? > > If so, even if you have not yet seen the failure without the patch, > > I would like to commit this patch to unblock further progress. If the > > error happens for more people afterwards we could investigate further. > > Hopefully someone will be able to provide a trace from ddb. > > Didn't happen again after switching back to a snapshot kernel. It did > happen somewhat frequently. But what may have triggered it is that I > suspended while using my phone as a hotspot. > > I'm ok if you want to move ahead. I can recognize the issue and get a > proper backtrace the next time this happens. Happened again while still on a Jan 16 snapshot kernel. So it is not related to that diff. Here is the panic message and backtrace: panic: kernel diagnostic assertion "sc->task_refs.refs == 0" failed: file "/usr/src/sys/dev/pci/if_iwm.c", line 9981 Stopped at db_enter+0x10: popq%rbp TIDPIDUID PRFLAGS PFLAGS CPU COMMAND *120744 85293 0 0x3 00K ifconfig db_enter() at db_enter+0x10 panic() at panic+0xbf __assert() at __assert+0x25 iwm_init() at iwm_init+0x254 iwm_ioctl() at iwm_ioctl+0xf9 ifioctl() at ifioctl+0x92b soo_ioctl() at soo_ioctl+0x161 sys_ioctl() at sys_ioctl+0x2c4 syscall() at syscall+0x374 Xsyscall() at Xsyscall+0x128 end of kernel
