Re: xhci zero length transfers 'leak' one transfer buffer count
On 09/10/20(Fri) 12:37, Jonathon Fletcher wrote:
> In xhci_xfer_get_trb, the count of transfer buffers in the pipe
> (xp->free_trbs) is always decremented but the count of transfer buffers used
> in the transfer (xx->ntrb) is not incremented for zero-length transfers. The
> result of this is that, at the end of a zero length transfer, xp->free_trbs
> has 'lost' one.
>
> Over time, this mismatch of unconditional decrement (xp->free_trbs) vs
> conditional increment (xx->ntrb) results in xhci_device_*_start returning
> USBD_NOMEM.
>
> The patch below works around this by only decrementing xp->free_trbs in the
> cases when xx->ntrb is incremented.
Did you consider incrementing xx->ntrb instead?
With the diff below the produced TRB isn't accounted which might lead to
and off-by-one.
> Index: xhci.c
> ===
> RCS file: /cvs/src/sys/dev/usb/xhci.c,v
> retrieving revision 1.119
> diff -u -p -u -r1.119 xhci.c
> --- xhci.c31 Jul 2020 19:27:57 - 1.119
> +++ xhci.c9 Oct 2020 19:11:45 -
> @@ -1836,7 +1836,6 @@ xhci_xfer_get_trb(struct xhci_softc *sc,
> struct xhci_xfer *xx = (struct xhci_xfer *)xfer;
>
> KASSERT(xp->free_trbs >= 1);
> - xp->free_trbs--;
> *togglep = xp->ring.toggle;
>
> switch (last) {
> @@ -1847,11 +1846,13 @@ xhci_xfer_get_trb(struct xhci_softc *sc,
> xp->pending_xfers[xp->ring.index] = xfer;
> xx->index = -2;
> xx->ntrb += 1;
> + xp->free_trbs--;
> break;
> case 1: /* This will terminate a chain. */
> xp->pending_xfers[xp->ring.index] = xfer;
> xx->index = xp->ring.index;
> xx->ntrb += 1;
> + xp->free_trbs--;
> break;
> }
>
>
Re: Non-const basename: usr.bin/patch
On Sat, 10 Oct 2020 16:50:56 +0200, Christian Weisgerber wrote: > Here's a fix to accommodate a basename(3) that takes a non-const > parameter and may in fact modify the string buffer. This is > originally from Joerg Sonnenberger for DragonFly BSD and has since > spread to the other BSDs. > https://gitweb.dragonflybsd.org/dragonfly.git/commitdiff/41871674d0079dec70d5 > 5eb824f39d07dc7b3310 > (The corresponding change to inp.c is no longer applicable as that > code has been removed.) OK millert@ - todd
Re: Non-const basename: usr.bin/compress
On Sat, 10 Oct 2020 22:24:58 +0200, Christian Weisgerber wrote: > Accommodate POSIX basename(3) that takes a non-const parameter and > may in fact modify the string buffer. > > Following martijn@'s comment for the sed diff, we don't need to > check for truncation because the "in" path has already been validated > by a preceding open(2). OK millert@ - todd
powerpc ld.lld fix
On powerpc with the secure-plt ABI we need a .got section, even if the
_GLOBAL_OFFSET_TABLE_ symbol isn't referenced. This is needed because
the first three entries of the GOT are used by the dynamic linker.
With this fix I can build executables of all flavours (including
-static/-nopie).
ok?
Index: gnu/llvm/lld/ELF/SyntheticSections.cpp
===
RCS file: /cvs/src/gnu/llvm/lld/ELF/SyntheticSections.cpp,v
retrieving revision 1.1.1.1
diff -u -p -r1.1.1.1 SyntheticSections.cpp
--- gnu/llvm/lld/ELF/SyntheticSections.cpp 3 Aug 2020 14:32:29 -
1.1.1.1
+++ gnu/llvm/lld/ELF/SyntheticSections.cpp 10 Oct 2020 21:13:59 -
@@ -604,6 +604,8 @@ GotSection::GotSection()
// ElfSym::globalOffsetTable.
if (ElfSym::globalOffsetTable && !target->gotBaseSymInGotPlt)
numEntries += target->gotHeaderEntriesNum;
+ else if (config->emachine == EM_PPC)
+numEntries += target->gotHeaderEntriesNum;
}
void GotSection::addEntry(Symbol &sym) {
Non-const basename: usr.bin/compress
Accommodate POSIX basename(3) that takes a non-const parameter and
may in fact modify the string buffer.
Following martijn@'s comment for the sed diff, we don't need to
check for truncation because the "in" path has already been validated
by a preceding open(2).
ok?
Index: usr.bin/compress/main.c
===
RCS file: /cvs/src/usr.bin/compress/main.c,v
retrieving revision 1.96
diff -u -p -r1.96 main.c
--- usr.bin/compress/main.c 28 Jun 2019 13:35:00 - 1.96
+++ usr.bin/compress/main.c 10 Oct 2020 20:10:44 -
@@ -481,6 +481,7 @@ docompress(const char *in, char *out, co
{
#ifndef SMALL
u_char buf[Z_BUFSIZE];
+ char namebuf[PATH_MAX];
char *name;
int error, ifd, ofd, oreg;
void *cookie;
@@ -534,7 +535,8 @@ docompress(const char *in, char *out, co
}
if (!pipin && storename) {
- name = basename(in);
+ strlcpy(namebuf, in, sizeof(namebuf));
+ name = basename(namebuf);
mtime = (u_int32_t)sb->st_mtime;
}
if ((cookie = method->wopen(ofd, name, bits, mtime)) == NULL) {
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: Non-const basename: usr.bin/sed
On Sat, 2020-10-10 at 17:21 +0200, Christian Weisgerber wrote:
> Changing basename(3) and dirname(3) to the POSIX-mandated non-const
> parameters produces this warnings when compiling sed(1):
>
> /usr/src/usr.bin/sed/main.c:401:16: warning: passing 'const char *' to
> parameter
> of type 'char *' discards qualifiers
> [-Wincompatible-pointer-types-discards-qua
> lifiers]
>
> Here's a fix to accommodate a basename(3) that takes a non-const
> parameter and may in fact modify the string buffer. Based on FreeBSD
> like the surrounding in-place editing code.
>
> OK?
>
Wouldn't the following diff be a little simpler?
No need to check the return value of strlcpy, since we do a lstat
before, which can return a ENAMETOOLONG.
martijn@
Index: main.c
===
RCS file: /cvs/src/usr.bin/sed/main.c,v
retrieving revision 1.40
diff -u -p -r1.40 main.c
--- main.c 8 Dec 2018 23:11:24 - 1.40
+++ main.c 10 Oct 2020 17:51:53 -
@@ -343,6 +343,7 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
{
struct stat sb;
size_t len;
+ char dirbuf[PATH_MAX];
char *p;
int c, fd;
static int firstfile;
@@ -397,8 +398,9 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
if (len > sizeof(oldfname))
error(FATAL, "%s: name too long",
fname);
}
- len = snprintf(tmpfname, sizeof(tmpfname),
"%s/sedXX",
- dirname(fname));
+ strlcpy(dirbuf, fname, sizeof(dirbuf));
+ len = snprintf(tmpfname, sizeof(tmpfname),
+ "%s/sedXX", dirname(dirbuf));
if (len >= sizeof(tmpfname))
error(FATAL, "%s: name too long", fname);
if ((fd = mkstemp(tmpfname)) == -1)
Re: tree.h: returning void, legal but weird
> Date: Sat, 10 Oct 2020 18:37:50 +0200
> From: Otto Moerbeek
>
> OK?
ok kettenis@
> Index: tree.h
> ===
> RCS file: /cvs/src/sys/sys/tree.h,v
> retrieving revision 1.29
> diff -u -p -r1.29 tree.h
> --- tree.h30 Jul 2017 19:27:20 - 1.29
> +++ tree.h10 Oct 2020 16:36:15 -
> @@ -910,25 +910,25 @@ _name##_RBT_PARENT(struct _type *elm)
> __unused static inline void \
> _name##_RBT_SET_LEFT(struct _type *elm, struct _type *left) \
> {\
> - return _rb_set_left(_name##_RBT_TYPE, elm, left); \
> + _rb_set_left(_name##_RBT_TYPE, elm, left); \
> }\
> \
> __unused static inline void \
> _name##_RBT_SET_RIGHT(struct _type *elm, struct _type *right)
> \
> {\
> - return _rb_set_right(_name##_RBT_TYPE, elm, right); \
> + _rb_set_right(_name##_RBT_TYPE, elm, right);\
> }\
> \
> __unused static inline void \
> _name##_RBT_SET_PARENT(struct _type *elm, struct _type *parent)
> \
> {\
> - return _rb_set_parent(_name##_RBT_TYPE, elm, parent); \
> + _rb_set_parent(_name##_RBT_TYPE, elm, parent); \
> }\
> \
> __unused static inline void \
> _name##_RBT_POISON(struct _type *elm, unsigned long poison) \
> {\
> - return _rb_poison(_name##_RBT_TYPE, elm, poison); \
> + _rb_poison(_name##_RBT_TYPE, elm, poison); \
> }\
> \
> __unused static inline int \
>
>
tree.h: returning void, legal but weird
OK?
-Otto
Index: tree.h
===
RCS file: /cvs/src/sys/sys/tree.h,v
retrieving revision 1.29
diff -u -p -r1.29 tree.h
--- tree.h 30 Jul 2017 19:27:20 - 1.29
+++ tree.h 10 Oct 2020 16:36:15 -
@@ -910,25 +910,25 @@ _name##_RBT_PARENT(struct _type *elm)
__unused static inline void\
_name##_RBT_SET_LEFT(struct _type *elm, struct _type *left)\
{ \
- return _rb_set_left(_name##_RBT_TYPE, elm, left); \
+ _rb_set_left(_name##_RBT_TYPE, elm, left); \
} \
\
__unused static inline void\
_name##_RBT_SET_RIGHT(struct _type *elm, struct _type *right) \
{ \
- return _rb_set_right(_name##_RBT_TYPE, elm, right); \
+ _rb_set_right(_name##_RBT_TYPE, elm, right);\
} \
\
__unused static inline void\
_name##_RBT_SET_PARENT(struct _type *elm, struct _type *parent)
\
{ \
- return _rb_set_parent(_name##_RBT_TYPE, elm, parent); \
+ _rb_set_parent(_name##_RBT_TYPE, elm, parent); \
} \
\
__unused static inline void\
_name##_RBT_POISON(struct _type *elm, unsigned long poison)\
{ \
- return _rb_poison(_name##_RBT_TYPE, elm, poison); \
+ _rb_poison(_name##_RBT_TYPE, elm, poison); \
} \
\
__unused static inline int \
Non-const basename: usr.bin/sed
Changing basename(3) and dirname(3) to the POSIX-mandated non-const
parameters produces this warnings when compiling sed(1):
/usr/src/usr.bin/sed/main.c:401:16: warning: passing 'const char *' to parameter
of type 'char *' discards qualifiers [-Wincompatible-pointer-types-discards-qua
lifiers]
Here's a fix to accommodate a basename(3) that takes a non-const
parameter and may in fact modify the string buffer. Based on FreeBSD
like the surrounding in-place editing code.
OK?
Index: main.c
===
RCS file: /cvs/src/usr.bin/sed/main.c,v
retrieving revision 1.40
diff -u -p -r1.40 main.c
--- main.c 8 Dec 2018 23:11:24 - 1.40
+++ main.c 10 Oct 2020 15:16:12 -
@@ -343,6 +343,7 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
{
struct stat sb;
size_t len;
+ char *dirbuf;
char *p;
int c, fd;
static int firstfile;
@@ -397,8 +398,11 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
if (len > sizeof(oldfname))
error(FATAL, "%s: name too long",
fname);
}
+ if ((dirbuf = strdup(fname)) == NULL)
+ error(FATAL, "%s", strerror(errno));
len = snprintf(tmpfname, sizeof(tmpfname),
"%s/sedXX",
- dirname(fname));
+ dirname(dirbuf));
+ free(dirbuf);
if (len >= sizeof(tmpfname))
error(FATAL, "%s: name too long", fname);
if ((fd = mkstemp(tmpfname)) == -1)
--
Christian "naddy" Weisgerber na...@mips.inka.de
Non-const basename: usr.bin/patch
Changing basename(3) and dirname(3) to the POSIX-mandated non-const
parameters produces these warnings when compiling patch(1):
/usr/src/usr.bin/patch/backupfile.c:61:34: warning: passing 'const char *' to pa
rameter of type 'char *' discards qualifiers [-Wincompatible-pointer-types-disca
rds-qualifiers]
/usr/src/usr.bin/patch/backupfile.c:64:16: warning: passing 'const char *' to pa
rameter of type 'char *' discards qualifiers [-Wincompatible-pointer-types-disca
rds-qualifiers]
Here's a fix to accommodate a basename(3) that takes a non-const
parameter and may in fact modify the string buffer. This is
originally from Joerg Sonnenberger for DragonFly BSD and has since
spread to the other BSDs.
https://gitweb.dragonflybsd.org/dragonfly.git/commitdiff/41871674d0079dec70d55eb824f39d07dc7b3310
(The corresponding change to inp.c is no longer applicable as that
code has been removed.)
OK?
Index: usr.bin/patch/backupfile.c
===
RCS file: /cvs/src/usr.bin/patch/backupfile.c,v
retrieving revision 1.21
diff -u -p -r1.21 backupfile.c
--- usr.bin/patch/backupfile.c 26 Nov 2013 13:19:07 - 1.21
+++ usr.bin/patch/backupfile.c 10 Oct 2020 14:36:58 -
@@ -53,21 +53,32 @@ static void invalid_arg(const char *, co
char *
find_backup_file_name(const char *file)
{
- char*dir, *base_versions;
+ char*dir, *base_versions, *tmp_file;
int highest_backup;
if (backup_type == simple)
return concat(file, simple_backup_suffix);
- base_versions = concat(basename(file), ".~");
+ tmp_file = strdup(file);
+ if (tmp_file == NULL)
+ return NULL;
+ base_versions = concat(basename(tmp_file), ".~");
+ free(tmp_file);
if (base_versions == NULL)
return NULL;
- dir = dirname(file);
+ tmp_file = strdup(file);
+ if (tmp_file == NULL) {
+ free(base_versions);
+ return NULL;
+ }
+ dir = dirname(tmp_file);
if (dir == NULL) {
free(base_versions);
+ free(tmp_file);
return NULL;
}
highest_backup = max_backup_version(base_versions, dir);
free(base_versions);
+ free(tmp_file);
if (backup_type == numbered_existing && highest_backup == 0)
return concat(file, simple_backup_suffix);
return make_version_name(file, highest_backup + 1);
--
Christian "naddy" Weisgerber na...@mips.inka.de
Re: Lenovo X1 gen 8 touchpad interrupt: pchgpio(4)
Hi, thank you for your help! I attach the output below. interrupt total rate irq0/clock 30834 99 irq96/pchgpio0 0 0 irq97/acpi0 615 1 irq144/inteldrm0 1388 4 irq98/xhci0 40 0 irq114/iwx0 3011 9 irq109/dwiic0 0 0 irq100/dwiic1 22 0 irq101/ppb0 0 0 irq102/nvme0 17981 58 irq103/ppb1 0 0 irq104/ppb3 0 0 irq105/ppb4 0 0 irq106/ppb5 0 0 irq107/xhci1 0 0 irq108/ppb6 0 0 irq176/azalia0 29 0 irq109/ichiic0 0 0 irq115/em0 613 1 irq145/pckbc0 279 0 irq146/pckbc0 867 2 Total 55679 179 On 10/10/20 14:10, Mark Kettenis wrote: You initially misapplied the patch (probably by being in the wrong directory) but fixed that later. Given the dmesg output, you ended up with something that builts as intended. Unfortunately the driver isn't good enough, so I'll have to go back and see if I can figure out what's going wrong. Before I do that, can you send me the output of "vmstat -zi" for a kernel with my diff applied? Thanks, Mark
WANTLIB problems and possible solution: the libset design
I want to introduce a new mechanism (libsets) to supplement wantlib... Note that the keyword already exists in pkg_add. the story so far: wantlib evolved from lib-depends basically, you tell a given package that it requires some libs, (WANTLIB) and pkg_create will register those libs in the package as a set of @wantlib annotations, thus hardcoding the major/minor of each library. There's some sanity check as well, both during building packages and installing them, which is that all wantlib must be "reachable": looking through all run dependencies (recursively) of the package the libraries must actually exist in a dependency (recursively) of the package or in base/x11. Whenever you update, things must go strictly forward: if the package version changes, that's enough. Otherwise, we do look at version numbers of direct dependencies AND at wantlib version: if things go backward, then we don't update. If things are the same, we don't update either. Altogether this is a "signature" for the package version: - the package version itself - the version of each direct dependency - the version of each wantlib The problem: In case the set of wantlib changes, you have to bump the revision of all dependent packages, otherwise register-plist (or pkg_add) will complain that package signatures are non comparable. This is a problem if a basic gnome package (say gnomeajasper) adds a libfoobar, because suddenly, all users (direct AND indirect) of that package MUST be updated to - include foobar in their WANTLIB - have their REVISION bumped. The new design: The idea behind "libset" is to be able to specify a "set" of wantlib that corresponds to our package, AND to just write WANTLIB wrt that libset for that specific set of libraries. In practice: - the basic package would get a libset declaration @libset mylibset-1.0 liba,libb,libc - direct and indirect users would have +mylibset in their WANTLIB (the specific character used is up for debate... (mylibset) would be nice as well, but it would interfere with everything shell-quoting this would have several effects: - when making the package, pkg_create would scan dependent packages for the libset declaration, and add the corresponding wantlib - the resulting package signature would ALSO include +mylibset-.10 in their signature. For updates, things change slightly - dependencies must be going forward - libsets MUST be going forward (you still need to bump REVISION if you add a whole libset to a package) - wantlib MUST be going forward, BUT there is a bit of libset comparison: if a wantlib is ONLY mentioned in a new version of the libset compared to the old (or vice-versa), then it is no longer an error, and versions are STILL comparable. (*) This is more or less the basic principle. Note that (*) is a bit tricky: this only concerns wantlib which are exclusively part of a given libset... because if the wantlib is also mentioned elsewhere, then it still has to be given properly. As for libset versions proper, we would go with the exact same version scheme as for packages. The reason for this would lie with bsd.port.mk: I think it's likely that each package would declare ONE single libset at most, so there is zero issue in naming the libset exactly like the package, so a libset declaration would be LIBSET = liba libb libc in the case above, it would become LIBSET = liba libb libc libfoobar and lead to @libset gnomeajaspec:liba,libb,libc,libfoobar (note that I've written "lib" everywhere to make things obvious, but as usual, this should rather be LIBSET = a b c foobar) (semantics for multi-packages left up to the astute reader). (even though it's currently redundant and matches pkgnames, I prefer to have a separate namespace for libset, just in case we eventually want to add several libsets for a given package...) I don't think we need to make libset recursive (e.g., have a libset include another libset) and also, I have zero idea how to implement that. As far as I can figure out, to change a libset, you would only need to bump the rev of the basic port and add to its LIBSET declaration. If wanted, this could even be used for base/X11, we would mostly need to have a smallish libset declarations for (say) all the X11 libraries that are commonly used for something.
Re: Lenovo X1 gen 8 touchpad interrupt: pchgpio(4)
You initially misapplied the patch (probably by being in the wrong directory) but fixed that later. Given the dmesg output, you ended up with something that builts as intended. Unfortunately the driver isn't good enough, so I'll have to go back and see if I can figure out what's going wrong. Before I do that, can you send me the output of "vmstat -zi" for a kernel with my diff applied? Thanks, Mark
