Re: macppc kernel and clang
ok deraadt
Next up, to figure out the right plan for ofw.
Thank you so much for figuring out these two details.
How are the bootblocks faring?
And userland?
George Koehler wrote:
> On Mon, 16 Mar 2020 12:54:52 +0100 (CET)
> Mark Kettenis wrote:
>
> > I had a look at what NetBSD does, and they use '%L0' instead of
> > '%0+1'. As far as I can tell this works on both gcc and clang. The
> > diff below produces a working kernel when building with gcc. Not yet
> > in a position to test a clang-built kernel myself yet. But if this
> > produces a working kernel with clang as well, I'd prefer this diff
> > instead of yours.
>
> Yes, the clang kernel is working with your %L0 diff and the noinline
> stuff. I now prefer your %L0 diff, ok gkoehler@
>
> "mftb %L0" becomes "mftb ${0:L}" in LLVM IR (clang -S -emit-llvm),
> then LLVM handles the 'L' in PPCAsmPrinter::PrintAsmOperand in
> /usr/src/gnu/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
>
> > Index: arch/powerpc/include/cpu.h
> > ===
> > RCS file: /cvs/src/sys/arch/powerpc/include/cpu.h,v
> > retrieving revision 1.65
> > diff -u -p -r1.65 cpu.h
> > --- arch/powerpc/include/cpu.h 23 Mar 2019 05:27:53 - 1.65
> > +++ arch/powerpc/include/cpu.h 16 Mar 2020 11:30:42 -
> > @@ -336,7 +336,7 @@ ppc_mftb(void)
> > u_long scratch;
> > u_int64_t tb;
> >
> > - __asm volatile ("1: mftbu %0; mftb %0+1; mftbu %1;"
> > + __asm volatile ("1: mftbu %0; mftb %L0; mftbu %1;"
> > " cmpw 0,%0,%1; bne 1b" : "=r"(tb), "=r"(scratch));
> > return tb;
> > }
>
>
> --
> George Koehler
>
Re: macppc kernel and clang
On Mon, 16 Mar 2020 12:54:52 +0100 (CET)
Mark Kettenis wrote:
> I had a look at what NetBSD does, and they use '%L0' instead of
> '%0+1'. As far as I can tell this works on both gcc and clang. The
> diff below produces a working kernel when building with gcc. Not yet
> in a position to test a clang-built kernel myself yet. But if this
> produces a working kernel with clang as well, I'd prefer this diff
> instead of yours.
Yes, the clang kernel is working with your %L0 diff and the noinline
stuff. I now prefer your %L0 diff, ok gkoehler@
"mftb %L0" becomes "mftb ${0:L}" in LLVM IR (clang -S -emit-llvm),
then LLVM handles the 'L' in PPCAsmPrinter::PrintAsmOperand in
/usr/src/gnu/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
> Index: arch/powerpc/include/cpu.h
> ===
> RCS file: /cvs/src/sys/arch/powerpc/include/cpu.h,v
> retrieving revision 1.65
> diff -u -p -r1.65 cpu.h
> --- arch/powerpc/include/cpu.h23 Mar 2019 05:27:53 - 1.65
> +++ arch/powerpc/include/cpu.h16 Mar 2020 11:30:42 -
> @@ -336,7 +336,7 @@ ppc_mftb(void)
> u_long scratch;
> u_int64_t tb;
>
> - __asm volatile ("1: mftbu %0; mftb %0+1; mftbu %1;"
> + __asm volatile ("1: mftbu %0; mftb %L0; mftbu %1;"
> " cmpw 0,%0,%1; bne 1b" : "=r"(tb), "=r"(scratch));
> return tb;
> }
--
George Koehler
change to pfsync(4) reduces a PF_LOCK scope
Hello,
patch below is a fairly large change, which reduces a scope of PF_LOCK().
Whenever pf(4) needs to send state table update to its peer, it must grab
a PF_LOCK() to serialize access to internal pfsync(4) queues. The patch
below adds a mutex to every queue pfsync's queue, so PF_LOCK() is no longer
required for outbound updates. The inbound updates still require PF_LOCK() (+
state lock) in order to safely insert a state received from remote peer.
Hrvoje has been testing the diff for some time and he could see some issues,
which seems to be fixed now. Time has come to kindly ask more people to test
the diff and report issues back (either tech@ or bugs@).
It's worth to test patch as-is, to make sure no issues are introduced to
default compile time settings.
Brave hearts may enable 'WITH_PF_LOCK' option like that:
8<---8<---8<--8<
--- a/sys/arch/amd64/conf/GENERIC.MP
+++ b/sys/arch/amd64/conf/GENERIC.MP
@@ -3,6 +3,7 @@
include "arch/amd64/conf/GENERIC"
option MULTIPROCESSOR
+option WITH_PF_LOCK
#optionMP_LOCKDEBUG
#optionWITNESS
8<---8<---8<--8<
The tweak above enables the changes. Also keep in mind the tweak above
is not enough to get parallel execution of PF. Few more small changes
around IP input is required to enable that. I'll send another diff as
a reply to this email later tomorrow.
thanks and
regards
sashan
8<---8<---8<--8<
diff --git a/sys/net/if_pfsync.c b/sys/net/if_pfsync.c
index 34dd370078a..c7b01bab9fa 100644
--- a/sys/net/if_pfsync.c
+++ b/sys/net/if_pfsync.c
@@ -210,9 +210,11 @@ struct pfsync_softc {
struct ipsc_template;
struct pf_state_queuesc_qs[PFSYNC_S_COUNT];
+ struct mutex sc_mtx[PFSYNC_S_COUNT];
size_t sc_len;
struct pfsync_upd_reqs sc_upd_req_list;
+ struct mutex sc_upd_req_mtx;
int sc_initial_bulk;
int sc_link_demoted;
@@ -220,6 +222,7 @@ struct pfsync_softc {
int sc_defer;
struct pfsync_deferrals sc_deferrals;
u_intsc_deferred;
+ struct mutex sc_deferrals_mtx;
void*sc_plus;
size_t sc_pluslen;
@@ -234,6 +237,7 @@ struct pfsync_softc {
struct timeout sc_bulk_tmo;
TAILQ_HEAD(, tdb)sc_tdb_q;
+ struct mutex sc_tdb_mtx;
struct task sc_ltask;
struct task sc_dtask;
@@ -241,6 +245,16 @@ struct pfsync_softc {
struct timeout sc_tmo;
};
+struct pfsync_snapshot {
+ struct pfsync_softc *sn_sc;
+ struct pf_state_queuesn_qs[PFSYNC_S_COUNT];
+ struct pfsync_upd_reqs sn_upd_req_list;
+ TAILQ_HEAD(, tdb)sn_tdb_q;
+ size_t sn_len;
+ void*sn_plus;
+ size_t sn_pluslen;
+};
+
struct pfsync_softc*pfsyncif = NULL;
struct cpumem *pfsynccounters;
@@ -276,6 +290,10 @@ void pfsync_bulk_start(void);
void pfsync_bulk_status(u_int8_t);
void pfsync_bulk_update(void *);
void pfsync_bulk_fail(void *);
+
+void pfsync_grab_snapshot(struct pfsync_snapshot *, struct pfsync_softc *);
+void pfsync_drop_snapshot(struct pfsync_snapshot *);
+
#ifdef WITH_PF_LOCK
void pfsync_send_dispatch(void *);
void pfsync_send_pkt(struct mbuf *);
@@ -307,6 +325,13 @@ pfsync_clone_create(struct if_clone *ifc, int unit)
struct pfsync_softc *sc;
struct ifnet *ifp;
int q;
+ static const char *mtx_names[] = {
+ "iack_mtx",
+ "upd_c_mtx",
+ "del_mtx",
+ "ins_mtx",
+ "upd_mtx",
+ "" };
if (unit != 0)
return (EINVAL);
@@ -314,18 +339,23 @@ pfsync_clone_create(struct if_clone *ifc, int unit)
pfsync_sync_ok = 1;
sc = malloc(sizeof(*pfsyncif), M_DEVBUF, M_WAITOK|M_ZERO);
- for (q = 0; q < PFSYNC_S_COUNT; q++)
+ for (q = 0; q < PFSYNC_S_COUNT; q++) {
TAILQ_INIT(>sc_qs[q]);
+ mtx_init_flags(>sc_mtx[q], IPL_SOFTNET, mtx_names[q], 0);
+ }
pool_init(>sc_pool, PFSYNC_PLSIZE, 0, IPL_SOFTNET, 0, "pfsync",
NULL);
TAILQ_INIT(>sc_upd_req_list);
+ mtx_init(>sc_upd_req_mtx, IPL_SOFTNET);
TAILQ_INIT(>sc_deferrals);
+ mtx_init(>sc_deferrals_mtx, IPL_SOFTNET);
task_set(>sc_ltask, pfsync_syncdev_state, sc);
task_set(>sc_dtask, pfsync_ifdetach, sc);
sc->sc_deferred = 0;
TAILQ_INIT(>sc_tdb_q);
+ mtx_init(>sc_tdb_mtx, IPL_SOFTNET);
sc->sc_len = PFSYNC_MINPKT;
sc->sc_maxupdates = 128;
Re: smtpd: handle buf == NULL from m_get_string
On Tue, 17 Mar 2020 00:16:27 +0100, Tobias Heider wrote: > m_get_string(m, ) may set 'buf == NULL', which would lead > to strlen(NULL) in m_get_envelope. > > I chose fatalx because that's what seems to be the common way to handle > errors in mproc but I don't know the code base to well. Sure. OK millert@ - todd
smtpd: handle buf == NULL from m_get_string
m_get_string(m, ) may set 'buf == NULL', which would lead
to strlen(NULL) in m_get_envelope.
I chose fatalx because that's what seems to be the common way to handle
errors in mproc but I don't know the code base to well.
Index: mproc.c
===
RCS file: /mount/openbsd/cvs/src/usr.sbin/smtpd/mproc.c,v
retrieving revision 1.35
diff -u -p -r1.35 mproc.c
--- mproc.c 3 Oct 2019 05:50:28 - 1.35
+++ mproc.c 16 Mar 2020 23:05:21 -
@@ -637,6 +637,8 @@ m_get_envelope(struct msg *m, struct env
m_get_evpid(m, );
m_get_string(m, );
+ if (buf == NULL)
+ fatalx("empty envelope buffer");
if (!envelope_load_buffer(evp, buf, strlen(buf)))
fatalx("failed to retrieve envelope");
Re: smtpd: mail.lmtp uninitialzed stack access
On Mon, Mar 16, 2020 at 04:54:19PM -0600, Todd C. Miller wrote:
> On Mon, 16 Mar 2020 23:46:35 +0100, Tobias Heider wrote:
>
> > In main() mail.lmtp checks 'if (argc == 0 && session.rcptto == NULL)'
> > after getopt(). If neither an 'r' nor an 'u' option was specified,
> > 'session.rcptto' seems to be uninitialized.
> > The obvious solution would be to NULL initialize 'struct session'.
>
> I'd rather we just explicitly set session.rcptto to NULL.
Fair enough. Makes sense to initialize it with the other fields.
>
> Index: mail.lmtp.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/mail.lmtp.c,v
> retrieving revision 1.13
> diff -u -p -u -r1.13 mail.lmtp.c
> --- mail.lmtp.c 15 Mar 2020 16:34:57 - 1.13
> +++ mail.lmtp.c 16 Mar 2020 22:53:35 -
> @@ -64,6 +64,7 @@ main(int argc, char *argv[])
>
> session.lhlo = "localhost";
> session.mailfrom = getenv("SENDER");
> + session.rcptto = NULL;
>
> while ((ch = getopt(argc, argv, "d:l:f:ru")) != -1) {
> switch (ch) {
Re: smtpd: mail.lmtp uninitialzed stack access
On Mon, 16 Mar 2020 23:46:35 +0100, Tobias Heider wrote:
> In main() mail.lmtp checks 'if (argc == 0 && session.rcptto == NULL)'
> after getopt(). If neither an 'r' nor an 'u' option was specified,
> 'session.rcptto' seems to be uninitialized.
> The obvious solution would be to NULL initialize 'struct session'.
I'd rather we just explicitly set session.rcptto to NULL.
- todd
Index: mail.lmtp.c
===
RCS file: /cvs/src/usr.sbin/smtpd/mail.lmtp.c,v
retrieving revision 1.13
diff -u -p -u -r1.13 mail.lmtp.c
--- mail.lmtp.c 15 Mar 2020 16:34:57 - 1.13
+++ mail.lmtp.c 16 Mar 2020 22:53:35 -
@@ -64,6 +64,7 @@ main(int argc, char *argv[])
session.lhlo = "localhost";
session.mailfrom = getenv("SENDER");
+ session.rcptto = NULL;
while ((ch = getopt(argc, argv, "d:l:f:ru")) != -1) {
switch (ch) {
smtpd: mail.lmtp uninitialzed stack access
In main() mail.lmtp checks 'if (argc == 0 && session.rcptto == NULL)'
after getopt(). If neither an 'r' nor an 'u' option was specified,
'session.rcptto' seems to be uninitialized.
The obvious solution would be to NULL initialize 'struct session'.
ok?
Index: mail.lmtp.c
===
RCS file: /mount/openbsd/cvs/src/usr.sbin/smtpd/mail.lmtp.c,v
retrieving revision 1.12
diff -u -p -r1.12 mail.lmtp.c
--- mail.lmtp.c 2 Feb 2020 22:13:48 - 1.12
+++ mail.lmtp.c 16 Mar 2020 22:40:22 -
@@ -57,7 +57,7 @@ main(int argc, char *argv[])
int ch;
int conn;
const char *destination = "localhost";
- struct session session;
+ struct session session = { 0 };
if (! geteuid())
errx(EX_TEMPFAIL, "mail.lmtp: may not be executed as root");
Re: unbound 1.10.0
On 2020-03-16 06:01, Renaud Allard wrote: On 3/15/20 9:53 PM, Stuart Henderson wrote: On 2020/03/15 19:05, Renaud Allard wrote: On 15/03/2020 17:36, Stuart Henderson wrote: Lots of churn again.. most of the new + are related to the new rpz and serve-stale support. I've been running it for a few days with my usual setup with no problems, haven't tried the new things yet. Anyone want to test? I have had a lot of stalling issues with unbound 1.9.4 being a DoT server. It went better with 1.9.6, but there are still stalling issues. I am now trying your patch on 6.6-stable to see if it solves the stalling issues. I would be interested to know if it helps, but I don't think all that much has changed for DoT in this release so I think it's unlikely. Honestly I would just put dnsdist in front if you want good DoT (or DoH) service, it is solid. From my time limited testing of about 15 hours, I got no stall at all with 1.10.0 on any of the 3 servers tested. Down from at least 10 times in this kind of timeframe with 1.9.4 and 4-5 with 1.9.6. I think the problem is more related to tcp sessions themselves than DoT. That's good to hear, I've had a number of issues with DoT stalling on 1.9.4 and 1.9.6. I've found having multiple entries in my forwarders section has somewhat mitigated the issue. I wonder if the stalling problem is related to this: https://github.com/NLnetLabs/unbound/issues/47
in6_ifattach: strncpy to strlcpy
Using strncpy with sizeof(string) may result in a non-nul-terminated string at 'dst'. This is not too problematic here because if_xname is the same size as 'ifra_name' and should always be NUL terminated. I would still like to replace strncpy with strlcpy which implicitly includes the null byte in the 'size' argument, just to be on the safe side. ok? Index: netinet6/in6_ifattach.c === RCS file: /mount/openbsd/cvs/src/sys/netinet6/in6_ifattach.c,v retrieving revision 1.115 diff -u -p -r1.115 in6_ifattach.c --- netinet6/in6_ifattach.c 8 Nov 2019 07:16:29 - 1.115 +++ netinet6/in6_ifattach.c 16 Mar 2020 18:49:32 - @@ -245,7 +245,7 @@ in6_ifattach_linklocal(struct ifnet *ifp * configure link-local address. */ bzero(, sizeof(ifra)); - strncpy(ifra.ifra_name, ifp->if_xname, sizeof(ifra.ifra_name)); + strlcpy(ifra.ifra_name, ifp->if_xname, sizeof(ifra.ifra_name)); ifra.ifra_addr.sin6_family = AF_INET6; ifra.ifra_addr.sin6_len = sizeof(struct sockaddr_in6); ifra.ifra_addr.sin6_addr.s6_addr16[0] = htons(0xfe80); @@ -320,7 +320,7 @@ in6_ifattach_loopback(struct ifnet *ifp) return (0); bzero(, sizeof(ifra)); - strncpy(ifra.ifra_name, ifp->if_xname, sizeof(ifra.ifra_name)); + strlcpy(ifra.ifra_name, ifp->if_xname, sizeof(ifra.ifra_name)); ifra.ifra_prefixmask.sin6_len = sizeof(struct sockaddr_in6); ifra.ifra_prefixmask.sin6_family = AF_INET6; ifra.ifra_prefixmask.sin6_addr = in6mask128;
Re: umcs(4) out-of-bound read
> Date: Mon, 16 Mar 2020 20:51:54 +0100
> From: Martin Pieuchot
>
> If the given `rate' is bigger than the last element of the array there's
> an out-of-bound read and `divisor' will contain garbage.
>
> Diff below fix this issue reported by coverity, CID 1453258.
>
> Ok?
ok kettenis@
> Index: umcs.c
> ===
> RCS file: /cvs/src/sys/dev/usb/umcs.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 umcs.c
> --- umcs.c9 Oct 2017 08:26:16 - 1.6
> +++ umcs.c16 Mar 2020 18:56:28 -
> @@ -451,16 +451,16 @@ umcs_calc_baudrate(uint32_t rate, uint16
> return (-1);
>
> for (i = 0; i < divisors_len - 1; i++) {
> - if (rate > umcs_baudrate_divisors[i] &&
> - rate <= umcs_baudrate_divisors[i + 1])
> - break;
> + if (rate > umcs_baudrate_divisors[i] &&
> + rate <= umcs_baudrate_divisors[i + 1]) {
> + *divisor = umcs_baudrate_divisors[i + 1] / rate;
> + /* 0x00 .. 0x70 */
> + *clk = i << UMCS_SPx_CLK_SHIFT;
> + return (0);
> + }
> }
>
> - *divisor = umcs_baudrate_divisors[i + 1] / rate;
> - /* 0x00 .. 0x70 */
> - *clk = i << UMCS_SPx_CLK_SHIFT;
> -
> - return (0);
> + return (-1);
> }
>
> int
>
>
umcs(4) out-of-bound read
If the given `rate' is bigger than the last element of the array there's
an out-of-bound read and `divisor' will contain garbage.
Diff below fix this issue reported by coverity, CID 1453258.
Ok?
Index: umcs.c
===
RCS file: /cvs/src/sys/dev/usb/umcs.c,v
retrieving revision 1.6
diff -u -p -r1.6 umcs.c
--- umcs.c 9 Oct 2017 08:26:16 - 1.6
+++ umcs.c 16 Mar 2020 18:56:28 -
@@ -451,16 +451,16 @@ umcs_calc_baudrate(uint32_t rate, uint16
return (-1);
for (i = 0; i < divisors_len - 1; i++) {
-if (rate > umcs_baudrate_divisors[i] &&
-rate <= umcs_baudrate_divisors[i + 1])
-break;
+ if (rate > umcs_baudrate_divisors[i] &&
+ rate <= umcs_baudrate_divisors[i + 1]) {
+ *divisor = umcs_baudrate_divisors[i + 1] / rate;
+ /* 0x00 .. 0x70 */
+ *clk = i << UMCS_SPx_CLK_SHIFT;
+ return (0);
+ }
}
- *divisor = umcs_baudrate_divisors[i + 1] / rate;
- /* 0x00 .. 0x70 */
- *clk = i << UMCS_SPx_CLK_SHIFT;
-
- return (0);
+ return (-1);
}
int
Re: Fix brightness control on ASUS 1005PXD
On Mon, Mar 16, 2020 at 10:16:34AM +0100, Patrick Wildt wrote:
> Otherwise, if we want to do that in acpivout_get_brightness(),
> I guess we can update acpivout_select_brightness() and its caller
> to remove the check for -1, since there will be no -1 anymore?
>
Sure, I missed those. Here's a new diff with the checks
removed. Tested and works as expected on my machne.
Index: acpivout.c
===
RCS file: /cvs/src/sys/dev/acpi/acpivout.c,v
retrieving revision 1.19
diff -u -p -r1.19 acpivout.c
--- acpivout.c 8 Feb 2020 19:08:17 - 1.19
+++ acpivout.c 16 Mar 2020 14:42:17 -
@@ -227,8 +227,10 @@ acpivout_get_brightness(struct acpivout_
aml_freevalue();
DPRINTF(("%s: BQC = %d\n", DEVNAME(sc), level));
- if (level < sc->sc_bcl[0] || level > sc->sc_bcl[sc->sc_bcl_len -1])
- level = -1;
+ if (level < sc->sc_bcl[0])
+ level = sc->sc_bcl[0];
+ else if (level > sc->sc_bcl[sc->sc_bcl_len - 1])
+ level = sc->sc_bcl[sc->sc_bcl_len - 1];
return (level);
}
@@ -239,9 +241,6 @@ acpivout_select_brightness(struct acpivo
int nindex, level;
level = sc->sc_brightness;
- if (level == -1)
- return level;
-
nindex = acpivout_find_brightness(sc, nlevel);
if (sc->sc_bcl[nindex] == level) {
if (nlevel > level && (nindex + 1 < sc->sc_bcl_len))
@@ -375,13 +374,11 @@ acpivout_set_param(struct wsdisplay_para
}
if (sc != NULL && sc->sc_bcl_len != 0) {
nindex = acpivout_select_brightness(sc, dp->curval);
- if (nindex != -1) {
- sc->sc_brightness = sc->sc_bcl[nindex];
- acpi_addtask(sc->sc_acpi,
- acpivout_set_brightness, sc, 0);
- acpi_wakeup(sc->sc_acpi);
- return 0;
- }
+ sc->sc_brightness = sc->sc_bcl[nindex];
+ acpi_addtask(sc->sc_acpi,
+ acpivout_set_brightness, sc, 0);
+ acpi_wakeup(sc->sc_acpi);
+ return 0;
}
return -1;
default:
Re: Fix brightness control on ASUS 1005PXD
On Mon, Mar 16, 2020 at 10:16:34AM +0100, Patrick Wildt wrote: > On Sat, Mar 14, 2020 at 04:28:26AM +0100, Alexandre Ratchov wrote: > > On ASUS 1001PXD, _BQC returns an out of range value which makes > > acpivout_get_brightness() return -1, in turn breaking the > > display.brightness control (wsconsctl displays a mangled value). > > > > This diff ignores the out of range value and makes the brighness > > control just work again. > > > > OK? > > With the current code _BQC is only called on attach, where we > probably want a value higher than the lowest one. I wonder if > maybe we should move this check into the attach functions, and > in case of an error like this, set a reasonable brightness? Just to be sure that I understand, do you suggest to change the brightness at attach time?
Re: unbound 1.10.0
On 3/15/20 9:53 PM, Stuart Henderson wrote: On 2020/03/15 19:05, Renaud Allard wrote: On 15/03/2020 17:36, Stuart Henderson wrote: Lots of churn again.. most of the new + are related to the new rpz and serve-stale support. I've been running it for a few days with my usual setup with no problems, haven't tried the new things yet. Anyone want to test? I have had a lot of stalling issues with unbound 1.9.4 being a DoT server. It went better with 1.9.6, but there are still stalling issues. I am now trying your patch on 6.6-stable to see if it solves the stalling issues. I would be interested to know if it helps, but I don't think all that much has changed for DoT in this release so I think it's unlikely. Honestly I would just put dnsdist in front if you want good DoT (or DoH) service, it is solid. From my time limited testing of about 15 hours, I got no stall at all with 1.10.0 on any of the 3 servers tested. Down from at least 10 times in this kind of timeframe with 1.9.4 and 4-5 with 1.9.6. I think the problem is more related to tcp sessions themselves than DoT. smime.p7s Description: S/MIME Cryptographic Signature
ktrwriteraw & vget
vget(9) might fail, stop right away if that happens.
CID 1453020 Unchecked return value.
ok?
Index: kern//kern_ktrace.c
===
RCS file: /cvs/src/sys/kern/kern_ktrace.c,v
retrieving revision 1.100
diff -u -p -r1.100 kern_ktrace.c
--- kern//kern_ktrace.c 6 Oct 2019 16:24:14 - 1.100
+++ kern//kern_ktrace.c 16 Mar 2020 12:58:19 -
@@ -649,7 +649,9 @@ ktrwriteraw(struct proc *curp, struct vn
auio.uio_iovcnt++;
auio.uio_resid += kth->ktr_len;
}
- vget(vp, LK_EXCLUSIVE | LK_RETRY);
+ error = vget(vp, LK_EXCLUSIVE | LK_RETRY);
+ if (error)
+ return (error);
error = VOP_WRITE(vp, , IO_UNIT|IO_APPEND, cred);
if (!error) {
vput(vp);
Re: macppc kernel and clang
> Date: Sat, 14 Mar 2020 23:33:37 -0400
> From: George Koehler
>
> Hello tech@ list!
>
> With this diff, it becomes possible to build macppc kernel with
> base-clang. The default compiler for macppc is base-gcc.
>
> rgc mailed the ppc@ list to report problems with
> clang kernel, and got a working kernel with clang -Oz (to stop clang
> inlining some functions in openfirm.c) and my modified ppc_mftb().
> In this diff, I use __attribute__((noinline)) so I don't need -Oz.
> I built my kernel with
>
> # clang CC=clang COMPILER_VERSION=clang
>
> because someone has already prepared the Makefile for macppc to pass
> different flags when COMPILER_VERSION is clang. I booted my clang
> kernel and it worked well enough to build another kernel with the
> same diff, but with gcc.
>
> In the second half of the diff, I change ppc_mftb(), because
> "mftb %0+1" doesn't always work in clang. For example, clang can
> put "=r"(tb) in register pair r27:r29, but 27+1 isn't 29. I don't
> know the syntax for the 2nd register of a pair, so I instead split
> "=r"(tb) into 2 variables. We use ppc_mftb() as timecounter.
>
> (gcc and clang emit different machine code for "mftbu" and "mftb", but
> both forms work for me. "objdump -dlr obj/clock.o" ppc_mftb() would
> show "mftb" from gcc or "mfspr" from clang. Power ISA 2.03 says that
> mfspr time base can't work on 601 or POWER3. A few Old World Mac
> models had PowerPC 601, but we only run on New World models.)
>
> Is the ppc_mftb() change by itself OK to commit?
I had a look at what NetBSD does, and they use '%L0' instead of
'%0+1'. As far as I can tell this works on both gcc and clang. The
diff below produces a working kernel when building with gcc. Not yet
in a position to test a clang-built kernel myself yet. But if this
produces a working kernel with clang as well, I'd prefer this diff
instead of yours.
> I am less sure about the first half of the diff, the noinline part.
> I don't like the design of this code. These functions call ofw_stack()
> like
>
> noinline int
> OF_peer(int phandle)
> {
> static struct ... args = ...;
>
> ofw_stack();
> args.phandle = phandle;
> if (openfirmware() == -1)
> return 0;
> return args.sibling;
> }
>
> but ofw_stack() is assembly code that changes the cpu's msr, moves the
> stack pointer %r1 to a different stack, copies the stack frame of
> OF_peer() to this new stack, and hijacks the saved return address so
> that, when OF_peer() returns, it switches back to the old stack and
> restores the msr. If clang inlines a function like OF_peer(), it no
> longer has its own stack frame, so the return-hijack stops working.
>
> We don't have retguard on powerpc, but if OF_peer() would use
> retguard to protect its return address, then the return-hijack might
> become impossible.
>
> Perhaps the code should look like
>
> msr = do_something_to_msr();
> args.phandle = phandle;
> if (openfirmware() == -1)
> ret = 0;
> else
> ret = args.sibling;
> ppc_mtmsr(msr);
> return ret;
>
> and openfirmware() should do the stack-switching, but I have not yet
> tried to make such a change. (I suspect that we need the msr to
> disable interrupts and protect the static args.)
Interrupts have to be blocked while we're not on the normal kernel stack.
I'm still working out for myself how this all works. NetBSD's code
stull uses ofw_stack(), so looking at their code doesn't help us.
FreeBSD seems to solve this in quite a different way, but tries to
support many different variations of firmware.
Cheers,
Mark
Index: arch/powerpc/include/cpu.h
===
RCS file: /cvs/src/sys/arch/powerpc/include/cpu.h,v
retrieving revision 1.65
diff -u -p -r1.65 cpu.h
--- arch/powerpc/include/cpu.h 23 Mar 2019 05:27:53 - 1.65
+++ arch/powerpc/include/cpu.h 16 Mar 2020 11:30:42 -
@@ -336,7 +336,7 @@ ppc_mftb(void)
u_long scratch;
u_int64_t tb;
- __asm volatile ("1: mftbu %0; mftb %0+1; mftbu %1;"
+ __asm volatile ("1: mftbu %0; mftb %L0; mftbu %1;"
" cmpw 0,%0,%1; bne 1b" : "=r"(tb), "=r"(scratch));
return tb;
}
Re: Start point to learn OpenBSD programming
On Mon Mar 16, 2020 at 07:23:15AM +, Martin wrote: > Hello list, > > The best way for beginner to start with OpenbBSD programming? > > Martin http://www.grenadille.net/post/2019/10/21/10-projects-to-start-contributing-to-OpenBSD Rafael
Re: acpitoshiba: remove dead code
> On 16 Mar 2020, at 09:49, Stefan Sperling wrote: > > On Mon, Mar 16, 2020 at 09:29:43AM +0100, Jasper Lievisse Adriaanse wrote: >> Hi, >> >> The type of brightness and video_output is uint32_t; therefore it >> can never be less than 0 (which is what HCI_LCD_BRIGHTNESS_MIN and >> HCI_VIDEO_OUTPUT_CYCLE_MIN are defined to). So trim the checks by >> removig the impossible cases. >> >> Coverity CID 1453109, 1453169 >> >> OK? > > These values ultimately come from aml_val2int() which returns int64_t, > i.e. a signed value. This driver currently copies that value to a uint32_t, > shifts it, and this shifted value ends up being copied to an int, and then > again to a uint32_t, which is then range-checked. > I don't understand why a range check is done only that late in the game, > after multiple integer type conversions that seem to be performed for no > good reason other than that the expected range will fit into an int. > > So an alternative fix could be to switch brightness values in this > driver to int64_t everywhere, i.e. in toshiba_get_brightness(), > toshiba_find_brightness(), toshiba_set_brightness(), etc. > Then you can keep all the code as it is. > > Or read an int64_t with aml_val2int(), shift and range-check it right away, > and only convert to a narrower type if the value is in the expected range. > Else error. Right, it seems that other drivers either use an uint64_t with associated shifts or int to match wsdisplay_params->curval. Also, it seems this passing around of ‘brightness’ could be avoided by making it a member of acpitoshiba_softc; same goes for 'video_output'.
Re: Fix brightness control on ASUS 1005PXD
On Sat, Mar 14, 2020 at 04:28:26AM +0100, Alexandre Ratchov wrote:
> On ASUS 1001PXD, _BQC returns an out of range value which makes
> acpivout_get_brightness() return -1, in turn breaking the
> display.brightness control (wsconsctl displays a mangled value).
>
> This diff ignores the out of range value and makes the brighness
> control just work again.
>
> OK?
With the current code _BQC is only called on attach, where we
probably want a value higher than the lowest one. I wonder if
maybe we should move this check into the attach functions, and
in case of an error like this, set a reasonable brightness?
Otherwise, if we want to do that in acpivout_get_brightness(),
I guess we can update acpivout_select_brightness() and its caller
to remove the check for -1, since there will be no -1 anymore?
> Index: acpivout.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpivout.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 acpivout.c
> --- acpivout.c8 Feb 2020 19:08:17 - 1.19
> +++ acpivout.c14 Mar 2020 03:19:02 -
> @@ -227,8 +227,10 @@ acpivout_get_brightness(struct acpivout_
> aml_freevalue();
> DPRINTF(("%s: BQC = %d\n", DEVNAME(sc), level));
>
> - if (level < sc->sc_bcl[0] || level > sc->sc_bcl[sc->sc_bcl_len -1])
> - level = -1;
> + if (level < sc->sc_bcl[0])
> + level = sc->sc_bcl[0];
> + else if (level > sc->sc_bcl[sc->sc_bcl_len - 1])
> + level = sc->sc_bcl[sc->sc_bcl_len - 1];
>
> return (level);
> }
>
Re: acpitoshiba: remove dead code
On Mon, Mar 16, 2020 at 09:29:43AM +0100, Jasper Lievisse Adriaanse wrote: > Hi, > > The type of brightness and video_output is uint32_t; therefore it > can never be less than 0 (which is what HCI_LCD_BRIGHTNESS_MIN and > HCI_VIDEO_OUTPUT_CYCLE_MIN are defined to). So trim the checks by > removig the impossible cases. > > Coverity CID 1453109, 1453169 > > OK? These values ultimately come from aml_val2int() which returns int64_t, i.e. a signed value. This driver currently copies that value to a uint32_t, shifts it, and this shifted value ends up being copied to an int, and then again to a uint32_t, which is then range-checked. I don't understand why a range check is done only that late in the game, after multiple integer type conversions that seem to be performed for no good reason other than that the expected range will fit into an int. So an alternative fix could be to switch brightness values in this driver to int64_t everywhere, i.e. in toshiba_get_brightness(), toshiba_find_brightness(), toshiba_set_brightness(), etc. Then you can keep all the code as it is. Or read an int64_t with aml_val2int(), shift and range-check it right away, and only convert to a narrower type if the value is in the expected range. Else error. > Index: acpi/acpitoshiba.c > === > RCS file: /cvs/src/sys/dev/acpi/acpitoshiba.c,v > retrieving revision 1.12 > diff -u -p -r1.12 acpitoshiba.c > --- acpi/acpitoshiba.c13 Oct 2019 10:56:31 - 1.12 > +++ acpi/acpitoshiba.c11 Mar 2020 11:35:23 - > @@ -438,9 +438,8 @@ toshiba_set_brightness(struct acpitoshib > for (i = 0; i < HCI_WORDS; ++i) > args[i].type = AML_OBJTYPE_INTEGER; > > - if ((*brightness < HCI_LCD_BRIGHTNESS_MIN) || > - (*brightness > HCI_LCD_BRIGHTNESS_MAX)) > -return (HCI_FAILURE); > + if (*brightness > HCI_LCD_BRIGHTNESS_MAX) > + return (HCI_FAILURE); > > *brightness <<= HCI_LCD_BRIGHTNESS_SHIFT; > > @@ -534,8 +533,7 @@ toshiba_set_video_output(struct acpitosh > > bzero(args, sizeof(args)); > > - if ((*video_output < HCI_VIDEO_OUTPUT_CYCLE_MIN) || > - (*video_output > HCI_VIDEO_OUTPUT_CYCLE_MAX)) > + if (*video_output > HCI_VIDEO_OUTPUT_CYCLE_MAX) > return (HCI_FAILURE); > > *video_output |= HCI_VIDEO_OUTPUT_FLAG; > -- > jasper > >
Re: acpitoshiba: remove dead code
On Mon, Mar 16, 2020 at 09:29:43AM +0100, Jasper Lievisse Adriaanse wrote:
> Hi,
>
> The type of brightness and video_output is uint32_t; therefore it
> can never be less than 0 (which is what HCI_LCD_BRIGHTNESS_MIN and
> HCI_VIDEO_OUTPUT_CYCLE_MIN are defined to). So trim the checks by
> removig the impossible cases.
>
> Coverity CID 1453109, 1453169
>
> OK?
I find such changes actually bad. Currently the code is clear doing a
real range check, After the change you will wonder why
HCI_LCD_BRIGHTNESS_MIN is not in that check and need to start digging.
Now I do understand that the code right now is not correct since this is a
signed vs unsigned comparison but please look at more then just that bit.
e.g.
int
toshiba_find_brightness(struct acpitoshiba_softc *sc, int *new_blevel)
{
int ret, current_blevel;
...
ret = toshiba_set_brightness(sc, _blevel);
}
So this function is passing a signed int to a function expecting unsinged
int
int
toshiba_fn_key_brightness_down(struct acpitoshiba_softc *sc)
{
uint32_t brightness_level;
...
if (brightness_level-- == HCI_LCD_BRIGHTNESS_MIN)
brightness_level = HCI_LCD_BRIGHTNESS_MIN;
else
ret = toshiba_set_brightness(sc, _level);
}
This code is also strange, especially since brightness_level is set but
not used again. I think this driver needs a bit more cleanup.
> Index: acpi/acpitoshiba.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpitoshiba.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 acpitoshiba.c
> --- acpi/acpitoshiba.c13 Oct 2019 10:56:31 - 1.12
> +++ acpi/acpitoshiba.c11 Mar 2020 11:35:23 -
> @@ -438,9 +438,8 @@ toshiba_set_brightness(struct acpitoshib
> for (i = 0; i < HCI_WORDS; ++i)
> args[i].type = AML_OBJTYPE_INTEGER;
>
> - if ((*brightness < HCI_LCD_BRIGHTNESS_MIN) ||
> - (*brightness > HCI_LCD_BRIGHTNESS_MAX))
> -return (HCI_FAILURE);
> + if (*brightness > HCI_LCD_BRIGHTNESS_MAX)
> + return (HCI_FAILURE);
>
> *brightness <<= HCI_LCD_BRIGHTNESS_SHIFT;
>
> @@ -534,8 +533,7 @@ toshiba_set_video_output(struct acpitosh
>
> bzero(args, sizeof(args));
>
> - if ((*video_output < HCI_VIDEO_OUTPUT_CYCLE_MIN) ||
> - (*video_output > HCI_VIDEO_OUTPUT_CYCLE_MAX))
> + if (*video_output > HCI_VIDEO_OUTPUT_CYCLE_MAX)
> return (HCI_FAILURE);
>
> *video_output |= HCI_VIDEO_OUTPUT_FLAG;
> --
> jasper
>
--
:wq Claudio
acpitoshiba: remove dead code
Hi, The type of brightness and video_output is uint32_t; therefore it can never be less than 0 (which is what HCI_LCD_BRIGHTNESS_MIN and HCI_VIDEO_OUTPUT_CYCLE_MIN are defined to). So trim the checks by removig the impossible cases. Coverity CID 1453109, 1453169 OK? Index: acpi/acpitoshiba.c === RCS file: /cvs/src/sys/dev/acpi/acpitoshiba.c,v retrieving revision 1.12 diff -u -p -r1.12 acpitoshiba.c --- acpi/acpitoshiba.c 13 Oct 2019 10:56:31 - 1.12 +++ acpi/acpitoshiba.c 11 Mar 2020 11:35:23 - @@ -438,9 +438,8 @@ toshiba_set_brightness(struct acpitoshib for (i = 0; i < HCI_WORDS; ++i) args[i].type = AML_OBJTYPE_INTEGER; - if ((*brightness < HCI_LCD_BRIGHTNESS_MIN) || - (*brightness > HCI_LCD_BRIGHTNESS_MAX)) - return (HCI_FAILURE); + if (*brightness > HCI_LCD_BRIGHTNESS_MAX) + return (HCI_FAILURE); *brightness <<= HCI_LCD_BRIGHTNESS_SHIFT; @@ -534,8 +533,7 @@ toshiba_set_video_output(struct acpitosh bzero(args, sizeof(args)); - if ((*video_output < HCI_VIDEO_OUTPUT_CYCLE_MIN) || - (*video_output > HCI_VIDEO_OUTPUT_CYCLE_MAX)) + if (*video_output > HCI_VIDEO_OUTPUT_CYCLE_MAX) return (HCI_FAILURE); *video_output |= HCI_VIDEO_OUTPUT_FLAG; -- jasper
Start point to learn OpenBSD programming
Hello list, The best way for beginner to start with OpenbBSD programming? Martin
