process: annotate locking for setitimer(2) state
Hi,
I want to annotate the locking for the per-process interval timers.
In the process struct, the ITIMER_REAL itimerspec and the ps_itimer_to
timeout are protected by the kernel lock. These should be annotated
with "K", right?
Also in the process struct, the ITIMER_VIRTUAL and ITIMER_PROF
itimerspecs are protected by the global itimer_mtx.
However, I don't think "itimer_mtx" isn't the best name for it, as it
doesn't protect state for *all* per-process interval timers. Just the
virtual ones.
Could I rename the mutex to "virtual_itimer_mtx"? Then I can annotate
the state protected by it with "V", as shown here in this patch.
Preferences? ok?
Index: kern/kern_time.c
===
RCS file: /cvs/src/sys/kern/kern_time.c,v
retrieving revision 1.134
diff -u -p -r1.134 kern_time.c
--- kern/kern_time.c8 Aug 2020 01:01:26 - 1.134
+++ kern/kern_time.c9 Aug 2020 00:41:10 -
@@ -488,7 +488,13 @@ out:
}
-struct mutex itimer_mtx = MUTEX_INITIALIZER(IPL_CLOCK);
+/*
+ * Global virtual interval timer mutex.
+ *
+ * Protects state for the per-process ITIMER_VIRTUAL and ITIMER_PROF
+ * interval timers.
+ */
+struct mutex virtual_itimer_mtx = MUTEX_INITIALIZER(IPL_CLOCK);
/*
* Get value of an interval timer. The process virtual and
@@ -526,10 +532,10 @@ sys_getitimer(struct proc *p, void *v, r
return (EINVAL);
itimer = >p_p->ps_timer[which];
memset(, 0, sizeof(aitv));
- mtx_enter(_mtx);
+ mtx_enter(_itimer_mtx);
TIMESPEC_TO_TIMEVAL(_interval, >it_interval);
TIMESPEC_TO_TIMEVAL(_value, >it_value);
- mtx_leave(_mtx);
+ mtx_leave(_itimer_mtx);
if (which == ITIMER_REAL) {
struct timeval now;
@@ -604,9 +610,9 @@ sys_setitimer(struct proc *p, void *v, r
}
pr->ps_timer[ITIMER_REAL] = aits;
} else {
- mtx_enter(_mtx);
+ mtx_enter(_itimer_mtx);
pr->ps_timer[which] = aits;
- mtx_leave(_mtx);
+ mtx_leave(_itimer_mtx);
}
return (0);
@@ -681,20 +687,20 @@ itimerdecr(struct itimerspec *itp, long
NSEC_TO_TIMESPEC(nsec, );
- mtx_enter(_mtx);
+ mtx_enter(_itimer_mtx);
timespecsub(>it_value, , >it_value);
if (itp->it_value.tv_sec >= 0 && timespecisset(>it_value)) {
- mtx_leave(_mtx);
+ mtx_leave(_itimer_mtx);
return (1);
}
if (!timespecisset(>it_interval)) {
timespecclear(>it_value);
- mtx_leave(_mtx);
+ mtx_leave(_itimer_mtx);
return (0);
}
while (itp->it_value.tv_sec < 0 || !timespecisset(>it_value))
timespecadd(>it_value, >it_interval, >it_value);
- mtx_leave(_mtx);
+ mtx_leave(_itimer_mtx);
return (0);
}
Index: sys/proc.h
===
RCS file: /cvs/src/sys/sys/proc.h,v
retrieving revision 1.297
diff -u -p -r1.297 proc.h
--- sys/proc.h 6 Jul 2020 13:33:09 - 1.297
+++ sys/proc.h 9 Aug 2020 00:41:11 -
@@ -150,9 +150,11 @@ struct unveil;
/*
* Locks used to protect struct members in this file:
* a atomic operations
+ * K kernel lock
* m this process' `ps_mtx'
* p this process' `ps_lock'
* R rlimit_lock
+ * V virtual_itimer_mtx
*/
struct process {
/*
@@ -216,7 +218,8 @@ struct process {
struct rusage *ps_ru; /* sum of stats for dead threads. */
struct tusage ps_tu; /* accumulated times. */
struct rusage ps_cru; /* sum of stats for reaped children */
- struct itimerspec ps_timer[3]; /* timers, indexed by ITIMER_* */
+ struct itimerspec ps_timer[3]; /* [K] ITIMER_REAL timer */
+ /* [V] ITIMER_{PROF,VIRTUAL} timers */
struct timeout ps_rucheck_to; /* [] resource limit check timer */
time_t ps_nextxcpu;/* when to send next SIGXCPU, */
/* in seconds of process runtime */
@@ -269,7 +272,7 @@ struct process {
int ps_refcnt; /* Number of references. */
struct timespec ps_start; /* starting uptime. */
- struct timeout ps_realit_to; /* real-time itimer trampoline. */
+ struct timeout ps_realit_to; /* [K] ITIMER_REAL timeout */
};
#defineps_session ps_pgrp->pg_session
Re: describe 'idle-timeout' exception in npppd.conf man page
On Sat, 8 Aug 2020 16:01:59 +0300 Vitaliy Makkoveev wrote: > On Sat, Aug 08, 2020 at 08:49:24PM +0900, YASUOKA Masahiko wrote: >> On Fri, 7 Aug 2020 22:19:05 +0300 >> Vitaliy Makkoveev wrote: >> > Some times ago we disabled in-kernel timeout for pppx(4) related >> > pipex(4) sessions. We did this for prevent use after free issue caused >> > by pipex_timer [1]. By default "idle-timeout" is not set in >> > npppd.conf(5) and I guess this is reason for we forgot to describe this >> > exception in npppd.conf(5). >> > >> > But looks like one user caught this [2]. So I propose to describe this >> > in BUGS section of npppd.conf(5). >> > >> > Also current "idle-timeout" description looks incorrect. If this option >> > is missing, there is not in-kernel timeout for this session, but >> > npppd(8) uses it's own timeout for. And we can't configure this value. >> > >> > YASUOKA, what do you think? May be we can kill in-kernel timeout feature >> > for pipex(4)?, and make npppd(8)'s idle timeout configurable by this >> > option? >> >> I think we should mention this to the man page until we fix it. >> So I'd like you to update the man page first. >> >> I'll try to review the problem. >> > > Thanks. I updated my diff with changes proposed by jmc@. Are you agree > with them? Yes. ok yasuoka >> > 1. >> > https://cvsweb.openbsd.org/src/sys/net/if_pppx.c?rev=1.78=text/x-cvsweb-markup >> > 2. https://marc.info/?l=openbsd-misc=159655468504864=2 >> > >> > >> > Index: usr.sbin/npppd/npppd/npppd.conf.5 >> > === >> > RCS file: /cvs/src/usr.sbin/npppd/npppd/npppd.conf.5,v >> > retrieving revision 1.27 >> > diff -u -p -r1.27 npppd.conf.5 >> > --- usr.sbin/npppd/npppd/npppd.conf.5 23 Apr 2020 21:10:54 - >> > 1.27 >> > +++ usr.sbin/npppd/npppd/npppd.conf.5 7 Aug 2020 19:17:00 - >> > @@ -699,3 +699,9 @@ The current version of >> > .Xr npppd 8 >> > does not support adding or removing tunnel settings or changing listener >> > settings (listen address, port and l2tp-ipsec-require). >> > +.Pp >> > +This time >> > +.Xr pppx 4 >> > +does not allow to create sessions with non null >> > +.Ic idle-timeout >> > +option. >> >
Re: vether(4): move `ifnet' out of KERNEL_LOCK()
On Sun, Aug 09, 2020 at 03:16:50AM +0300, Vitaliy Makkoveev wrote: > vether(4) is pretty dummy. Nothing denies it to be `IFXF_MPSAFE'. OK kn
vether(4): move `ifnet' out of KERNEL_LOCK()
vether(4) is pretty dummy. Nothing denies it to be `IFXF_MPSAFE'.
Index: sys/net/if_vether.c
===
RCS file: /cvs/src/sys/net/if_vether.c,v
retrieving revision 1.33
diff -u -p -r1.33 if_vether.c
--- sys/net/if_vether.c 28 Jul 2020 09:52:32 - 1.33
+++ sys/net/if_vether.c 9 Aug 2020 00:13:17 -
@@ -36,7 +36,7 @@
void vetherattach(int);
intvetherioctl(struct ifnet *, u_long, caddr_t);
-void vetherstart(struct ifnet *);
+void vetherqstart(struct ifqueue *);
intvether_clone_create(struct if_clone *, int);
intvether_clone_destroy(struct ifnet *);
intvether_media_change(struct ifnet *);
@@ -83,12 +83,12 @@ vether_clone_create(struct if_clone *ifc
ifp->if_softc = sc;
ifp->if_ioctl = vetherioctl;
- ifp->if_start = vetherstart;
+ ifp->if_qstart = vetherqstart;
ifq_set_maxlen(>if_snd, IFQ_MAXLEN);
ifp->if_hardmtu = ETHER_MAX_HARDMTU_LEN;
ifp->if_capabilities = IFCAP_VLAN_MTU;
- ifp->if_xflags = IFXF_CLONED;
+ ifp->if_xflags = IFXF_CLONED | IFXF_MPSAFE;
ifmedia_init(>sc_media, 0, vether_media_change,
vether_media_status);
@@ -117,15 +117,12 @@ vether_clone_destroy(struct ifnet *ifp)
* and we only need to discard the packets.
*/
void
-vetherstart(struct ifnet *ifp)
+vetherqstart(struct ifqueue *ifq)
{
+ struct ifnet*ifp = ifq->ifq_if;
struct mbuf *m;
- for (;;) {
- m = ifq_dequeue(>if_snd);
- if (m == NULL)
- return;
-
+ while ((m = ifq_dequeue(ifq)) != NULL) {
#if NBPFILTER > 0
if (ifp->if_bpf)
bpf_mtap(ifp->if_bpf, m, BPF_DIRECTION_OUT);
Re: video -c: showing auto white balance temperature
Hello Laurie,
On Sat, 8 Aug 2020 21:56:08 +0100
Laurence Tratt wrote:
> On Sat, Aug 08, 2020 at 09:30:18PM +0200, Marcus Glocker wrote:
>
> Hello Marcus,
>
> >> I like your patch, which is better than my original! My only very
> >> minor comment is whether we might want to document that it's safe
> >> to put "0" in a "V4L2_CID_AUTO" settings, since the lowest value
> >> one of those can be is 0x900 (judging by videoio.h)? At least, I
> >> had to look to make sure it was safe :)
> > Sorry, I lost you. Which control value exactly can be lowest
> > 0x900?
>
> In this bit of your diff:
>
> #define CTRL_SHARPNESS 6
> - { "sharpness", 0, V4L2_CID_SHARPNESS, 0, 0, 0, 0, 0 },
> + { "sharpness", 0, V4L2_CID_SHARPNESS, 0, 0, 0, 0, 0, 0 },
> #define CTRL_WHITE_BALANCE_TEMPERATURE 7
> { "white_balance_temperature",
> - 0, V4L2_CID_WHITE_BALANCE_TEMPERATURE, 0, 0,
> 0, 0, 0 },
> + 0, V4L2_CID_WHITE_BALANCE_TEMPERATURE,
> + V4L2_CID_AUTO_WHITE_BALANCE, 0, 0, 0, 0, 0
> },
>
> it wasn't immediately obvious to me that the first "0" after
> V4L2_CID_SHARPNESS wouldn't conflict with some other V4L2 setting.
> But it turns out that all the V4L2_CID values are defined relative to
> this:
>
> #define V4L2_CID_BASE (V4L2_CTRL_CLASS_USER
> | 0x900)
>
> so "0" can't accidentally clash with anything. It's not a big deal :)
Oh right, got it now :-)
Yes, we should be safe by using '0' to indicate that a control has no
auto control available without conflicting.
> >> One other thing has occurred to me -- but can be done in a future
> >> patch -- is that we probably want to be able to do:
> >>
> >> $ video white_balance_temperature=auto
> > I was thinking about that as well but just not decided yet if we
> > should introduce this or for now be compliant with the GUI where
> > you would require to reset all settings to get back to auto.
>
> In a sense the command-line is already more expressive (you can, for
> example, express arbitrary white balance temperatures when in the GUI
> you have to use increments of 10), so I'm inclined to think that
> allowing them to be more separate is no bad thing.
Well yes, true.
> [I personally think the GUI controls are very hard to use in
> practise: they change so slowly that I find it almost impossible to
> A/B anything. So I'm a bit biased against them!]
I see your point. Lets add this to our backlog, as we say in Agile :-|
>
> Laurie
>
Marcus
Re: video -c: showing auto white balance temperature
On Sat, Aug 08, 2020 at 09:30:18PM +0200, Marcus Glocker wrote:
Hello Marcus,
>> I like your patch, which is better than my original! My only very minor
>> comment is whether we might want to document that it's safe to put "0" in
>> a "V4L2_CID_AUTO" settings, since the lowest value one of those can be is
>> 0x900 (judging by videoio.h)? At least, I had to look to make sure it was
>> safe :)
> Sorry, I lost you. Which control value exactly can be lowest 0x900?
In this bit of your diff:
#define CTRL_SHARPNESS 6
- { "sharpness", 0, V4L2_CID_SHARPNESS, 0, 0, 0, 0, 0 },
+ { "sharpness", 0, V4L2_CID_SHARPNESS, 0, 0, 0, 0, 0, 0 },
#define CTRL_WHITE_BALANCE_TEMPERATURE 7
{ "white_balance_temperature",
- 0, V4L2_CID_WHITE_BALANCE_TEMPERATURE, 0, 0, 0, 0, 0 },
+ 0, V4L2_CID_WHITE_BALANCE_TEMPERATURE,
+ V4L2_CID_AUTO_WHITE_BALANCE, 0, 0, 0, 0, 0 },
it wasn't immediately obvious to me that the first "0" after
V4L2_CID_SHARPNESS wouldn't conflict with some other V4L2 setting. But it
turns out that all the V4L2_CID values are defined relative to this:
#define V4L2_CID_BASE (V4L2_CTRL_CLASS_USER | 0x900)
so "0" can't accidentally clash with anything. It's not a big deal :)
>> One other thing has occurred to me -- but can be done in a future
>> patch -- is that we probably want to be able to do:
>>
>> $ video white_balance_temperature=auto
> I was thinking about that as well but just not decided yet if we should
> introduce this or for now be compliant with the GUI where you would
> require to reset all settings to get back to auto.
In a sense the command-line is already more expressive (you can, for
example, express arbitrary white balance temperatures when in the GUI you
have to use increments of 10), so I'm inclined to think that allowing them
to be more separate is no bad thing.
[I personally think the GUI controls are very hard to use in practise: they
change so slowly that I find it almost impossible to A/B anything. So I'm a
bit biased against them!]
Laurie
Re: pms(4): disable parity checking for specific elantech fw
On Sat, 08 Aug 2020 17:08:01 +0200 sxv...@firemail.cc wrote: > So I recently installed OpenBSD on an EeePC 900HD with an Elantech v1 > touchpad (fw_version 0x20022). > This specific fw version for some reason sends inverted parity bits > on a cold boot, returning to normal after suspend & resume. That > leads to a failed parity check, dropped packets and ultimately a > broken driver. Can we maybe add a small comment explaining the inverted parity bits behaviour on cold boot for this firmware version? > Index: pms.c > === > RCS file: /cvs/src/sys/dev/pckbc/pms.c,v > retrieving revision 1.93 > diff -u -p -u -p -r1.93 pms.c > --- pms.c 4 Jul 2020 10:39:25 - 1.93 > +++ pms.c 8 Aug 2020 14:45:14 - > @@ -2292,7 +2292,7 @@ pms_sync_elantech_v1(struct pms_softc *s > } > > if (data < 0 || data >= nitems(elantech->parity) || > - elantech->parity[data] != p) > + (elantech->fw_version != 0x20022 && > elantech->parity[data] != p)) return (-1); > > return (0); >
pms(4): disable parity checking for specific elantech fw
So I recently installed OpenBSD on an EeePC 900HD with an Elantech v1 touchpad (fw_version 0x20022). This specific fw version for some reason sends inverted parity bits on a cold boot, returning to normal after suspend & resume. That leads to a failed parity check, dropped packets and ultimately a broken driver. Index: pms.c === RCS file: /cvs/src/sys/dev/pckbc/pms.c,v retrieving revision 1.93 diff -u -p -u -p -r1.93 pms.c --- pms.c 4 Jul 2020 10:39:25 - 1.93 +++ pms.c 8 Aug 2020 14:45:14 - @@ -2292,7 +2292,7 @@ pms_sync_elantech_v1(struct pms_softc *s } if (data < 0 || data >= nitems(elantech->parity) || - elantech->parity[data] != p) + (elantech->fw_version != 0x20022 && elantech->parity[data] != p)) return (-1); return (0);
Re: video -c: showing auto white balance temperature
On Sat, 8 Aug 2020 15:13:47 +0100 Laurence Tratt wrote: > On Sat, Aug 08, 2020 at 02:45:16PM +0200, Marcus Glocker wrote: > > Hello Marcus, > > > For now how about adding the according auto control id to our > > dev_ctrls structure? In a next step we could change > > dev_set_ctrl_auto_white_balance() to become a generic function > > dev_set_ctrl_auto() available for all auto controls. > > Agreed. > > I like your patch, which is better than my original! My only very > minor comment is whether we might want to document that it's safe to > put "0" in a "V4L2_CID_AUTO" settings, since the lowest value one of > those can be is 0x900 (judging by videoio.h)? At least, I had to look > to make sure it was safe :) Sorry, I lost you. Which control value exactly can be lowest 0x900? > One other thing has occurred to me -- but can be done in a future > patch -- is that we probably want to be able to do: > > $ video white_balance_temperature=auto I was thinking about that as well but just not decided yet if we should introduce this or for now be compliant with the GUI where you would require to reset all settings to get back to auto.
Re: $pexp in re.subr(8)
Stuart Henderson writes: > This means that the regular expression must match the full process > string. Equivalent to providing an expression with ^ at the start and > $ at the end of the. I see, so the documentation is already correct, sorry, and thanks for the explanation.
Re: describe 'idle-timeout' exception in npppd.conf man page
I did audit for "idle-timeout" option.
On Sat, Aug 08, 2020 at 08:49:24PM +0900, YASUOKA Masahiko wrote:
> On Fri, 7 Aug 2020 22:19:05 +0300
> Vitaliy Makkoveev wrote:
> > Some times ago we disabled in-kernel timeout for pppx(4) related
> > pipex(4) sessions. We did this for prevent use after free issue caused
> > by pipex_timer [1]. By default "idle-timeout" is not set in
> > npppd.conf(5) and I guess this is reason for we forgot to describe this
> > exception in npppd.conf(5).
> >
> > But looks like one user caught this [2]. So I propose to describe this
> > in BUGS section of npppd.conf(5).
> >
> > Also current "idle-timeout" description looks incorrect. If this option
> > is missing, there is not in-kernel timeout for this session, but
> > npppd(8) uses it's own timeout for. And we can't configure this value.
> >
I was a little wrong with this. This is a different timeout timer.
In my case `l2tp_ctrl_timeout' kills idle sessions. It's totally
npppd(8) related.
The case for "idle-timeout" described below.
> > YASUOKA, what do you think? May be we can kill in-kernel timeout feature
> > for pipex(4)?, and make npppd(8)'s idle timeout configurable by this
> > option?
>
> I think we should mention this to the man page until we fix it.
> So I'd like you to update the man page first.
>
> I'll try to review the problem.
>
We got this option from npppd.conf(5) and store it as `idle_timeout'
within `struct tunnconf'. While we set npppd(8) related session context
we set `timeout_sec' of `npppd_ppp' at npppd/ppp.c:169 by this value.
Also we initialize timeout timer at npppd/pppc.c:172. We have
ppp_reset_idle_timeout() routime which stops and restart this timer if
`idle_timeout > 0'.
cut begin
125 ppp_init(npppd *pppd, npppd_ppp *_this)
126 {
...
167
168 /* load the idle timer configuration */
169 _this->timeout_sec = conf->idle_timeout;
170
171 if (!evtimer_initialized(&_this->idle_event))
172 evtimer_set(&_this->idle_event, ppp_idle_timeout, _this);
173
632 ppp_reset_idle_timeout(npppd_ppp *_this)
633 {
...
636 evtimer_del(&_this->idle_event);
637 if (_this->timeout_sec > 0) {
638 tv.tv_usec = 0;
639 tv.tv_sec = _this->timeout_sec;
640
641 evtimer_add(&_this->idle_event, );
cut end
While we create pipex(4) session, we initialize request and pass this
this timeout value to kernel as `req->pr_timeout_sec = ppp->timeout_sec'
at npppd/npppd.c:1013.
If ioctl() at npppd/npppd.c:1153 was successful and in-kernel session
was created we check `timeout_sec' and disable npppd(8) related timer at
npppd/npppd.c:1178. But this timer was not started before.
cut begin
986 pipex_setup_common(npppd_ppp *ppp, struct pipex_session_req *req)
987 {
...
1013 req->pr_timeout_sec = ppp->timeout_sec;
1040 npppd_ppp_pipex_enable(npppd *_this, npppd_ppp *ppp)
1041 {
...
1059 pipex_setup_common(ppp, );
...
1153 if ((error = ioctl(_this->iface[ppp->ifidx].devf...
...
1175 if (ppp->timeout_sec > 0) {
1176 /* Stop the npppd's idle-timer. We use
pipex's idle-timer */
1177 ppp->timeout_sec = 0;
1178 ppp_reset_idle_timeout(ppp);
1179 }
cut end
So we have two cases:
1. "idle-timeout" is null or not set in npppd.conf(5)
npppd(8) related timer is initialized, but not started, in-kernel
timeout disabled.
2. "idle-timeout" is not null in npppd.conf(5)
npppd(8) related timer is initialized, but not started, in-kernel
timeout enabled for pppac(4) sessions.
So in any cases we never enable npppd(8) related timer.
We have some troubles with pppx(4) sessions: they have two parts:
pipex(4) session and pppx(4) related context. Session is a part of this
context. With in-kernel timer we destroy session within pipex(4) layer
and we can't destroy pppx(4) related part. That's the reason we disabled
this feature for pppx(4).
I propose to kill in-kernel timeout. This simplify code and make
pppac(4) and pppx(4) session usage more identical. Also it's easy to
start using npppd(8) related timer.
Do you have objections?
Re: video -c: showing auto white balance temperature
On Sat, Aug 08, 2020 at 02:45:16PM +0200, Marcus Glocker wrote: Hello Marcus, > For now how about adding the according auto control id to our dev_ctrls > structure? In a next step we could change > dev_set_ctrl_auto_white_balance() to become a generic function > dev_set_ctrl_auto() available for all auto controls. Agreed. I like your patch, which is better than my original! My only very minor comment is whether we might want to document that it's safe to put "0" in a "V4L2_CID_AUTO" settings, since the lowest value one of those can be is 0x900 (judging by videoio.h)? At least, I had to look to make sure it was safe :) One other thing has occurred to me -- but can be done in a future patch -- is that we probably want to be able to do: $ video white_balance_temperature=auto Laurie
Re: pppac(4) move ifnet out of KERNEL_LOCK()
Another update.
The whole "while ((m = ifq_dequeue(ifq)) != NULL)" wrapped by netlock as
it was made for pppx(4). This is to exclude per-packet lock/unlock in
output path.
Index: sys/net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.98
diff -u -p -r1.98 if_pppx.c
--- sys/net/if_pppx.c 28 Jul 2020 09:53:36 - 1.98
+++ sys/net/if_pppx.c 8 Aug 2020 13:28:04 -
@@ -1058,7 +1058,7 @@ static intpppac_ioctl(struct ifnet *, u
static int pppac_output(struct ifnet *, struct mbuf *, struct sockaddr *,
struct rtentry *);
-static voidpppac_start(struct ifnet *);
+static voidpppac_qstart(struct ifqueue *);
static inline struct pppac_softc *
pppac_lookup(dev_t dev)
@@ -1107,13 +1107,11 @@ pppacopen(dev_t dev, int flags, int mode
ifp->if_hdrlen = sizeof(uint32_t); /* for BPF */;
ifp->if_mtu = MAXMCLBYTES - sizeof(uint32_t);
ifp->if_flags = IFF_SIMPLEX | IFF_BROADCAST;
- ifp->if_xflags = IFXF_CLONED;
+ ifp->if_xflags = IFXF_CLONED | IFXF_MPSAFE;
ifp->if_rtrequest = p2p_rtrequest; /* XXX */
ifp->if_output = pppac_output;
- ifp->if_start = pppac_start;
+ ifp->if_qstart = pppac_qstart;
ifp->if_ioctl = pppac_ioctl;
- /* XXXSMP: be sure pppac_start() called under NET_LOCK() */
- ifq_set_maxlen(>if_snd, 1);
if_counters_alloc(ifp);
if_attach(ifp);
@@ -1382,10 +1380,10 @@ pppacclose(dev_t dev, int flags, int mod
klist_invalidate(>sc_wsel.si_note);
splx(s);
- pipex_iface_fini(>sc_pipex_iface);
-
if_detach(ifp);
+ pipex_iface_fini(>sc_pipex_iface);
+
LIST_REMOVE(sc, sc_entry);
free(sc, M_DEVBUF, sizeof(*sc));
@@ -1459,15 +1457,14 @@ drop:
}
static void
-pppac_start(struct ifnet *ifp)
+pppac_qstart(struct ifqueue *ifq)
{
+ struct ifnet *ifp = ifq->ifq_if;
struct pppac_softc *sc = ifp->if_softc;
struct mbuf *m;
- if (!ISSET(ifp->if_flags, IFF_RUNNING))
- return;
-
- while ((m = ifq_dequeue(>if_snd)) != NULL) {
+ NET_LOCK();
+ while ((m = ifq_dequeue(ifq)) != NULL) {
#if NBPFILTER > 0
if (ifp->if_bpf) {
bpf_mtap_af(ifp->if_bpf, m->m_pkthdr.ph_family, m,
@@ -1489,9 +1486,9 @@ pppac_start(struct ifnet *ifp)
mq_enqueue(>sc_mq, m); /* qdrop */
}
+ NET_UNLOCK();
if (!mq_empty(>sc_mq)) {
- KERNEL_ASSERT_LOCKED();
wakeup(sc);
selwakeup(>sc_rsel);
}
Re: describe 'idle-timeout' exception in npppd.conf man page
On Sat, Aug 08, 2020 at 08:49:24PM +0900, YASUOKA Masahiko wrote: > On Fri, 7 Aug 2020 22:19:05 +0300 > Vitaliy Makkoveev wrote: > > Some times ago we disabled in-kernel timeout for pppx(4) related > > pipex(4) sessions. We did this for prevent use after free issue caused > > by pipex_timer [1]. By default "idle-timeout" is not set in > > npppd.conf(5) and I guess this is reason for we forgot to describe this > > exception in npppd.conf(5). > > > > But looks like one user caught this [2]. So I propose to describe this > > in BUGS section of npppd.conf(5). > > > > Also current "idle-timeout" description looks incorrect. If this option > > is missing, there is not in-kernel timeout for this session, but > > npppd(8) uses it's own timeout for. And we can't configure this value. > > > > YASUOKA, what do you think? May be we can kill in-kernel timeout feature > > for pipex(4)?, and make npppd(8)'s idle timeout configurable by this > > option? > > I think we should mention this to the man page until we fix it. > So I'd like you to update the man page first. > > I'll try to review the problem. > Thanks. I updated my diff with changes proposed by jmc@. Are you agree with them? > > 1. > > https://cvsweb.openbsd.org/src/sys/net/if_pppx.c?rev=1.78=text/x-cvsweb-markup > > 2. https://marc.info/?l=openbsd-misc=159655468504864=2 > > > > > > Index: usr.sbin/npppd/npppd/npppd.conf.5 > > === > > RCS file: /cvs/src/usr.sbin/npppd/npppd/npppd.conf.5,v > > retrieving revision 1.27 > > diff -u -p -r1.27 npppd.conf.5 > > --- usr.sbin/npppd/npppd/npppd.conf.5 23 Apr 2020 21:10:54 - > > 1.27 > > +++ usr.sbin/npppd/npppd/npppd.conf.5 7 Aug 2020 19:17:00 - > > @@ -699,3 +699,9 @@ The current version of > > .Xr npppd 8 > > does not support adding or removing tunnel settings or changing listener > > settings (listen address, port and l2tp-ipsec-require). > > +.Pp > > +This time > > +.Xr pppx 4 > > +does not allow to create sessions with non null > > +.Ic idle-timeout > > +option. >
Re: video -c: showing auto white balance temperature
Hello Laurie,
On Wed, 5 Aug 2020 21:42:18 +0100
Laurence Tratt wrote:
> Following Marcus's commit of video(1) changes, the attached patch
> crudely solves the "-c output is misleading for
> white_balance_temperature" because we conflate
> auto_white_balance_temperature and white_balance_temperature (which
> are two separate UVC controls) into one control in video(1).
>
> With this patch we can tell people if white_balance_temperature is
> being automatically controlled or not:
>
> $ video white_balance_temperature=6500
> white_balance_temperature: 4000 -> 6500
> $ obj/video -c
> brightness=128
> contrast=32
> saturation=64
> hue=0
> gamma=120
> sharpness=2
> white_balance_temperature=6500
> $ obj/video -d
> $ obj/video -c
> brightness=128
> contrast=32
> saturation=64
> hue=0
> gamma=120
> sharpness=2
> white_balance_temperature=auto
>
> This patch raises several questions:
>
> 1) At the moment the only "auto" control we have is
> white_balance_temperature. If we gain control of
> zoom/pan/exposure (etc), it might be worth breaking out the common
> "auto" functionality?
For now how about adding the according auto control id to our dev_ctrls
structure? In a next step we could change
dev_set_ctrl_auto_white_balance() to become a generic function
dev_set_ctrl_auto() available for all auto controls.
> 2) Arguably the first command should look like:
>$ video white_balance_temperature=6500
>white_balance_temperature: auto -> 6500
I agree that feels more natural. Added in the below diff.
> 3) The output of "video -dv" is very different to "video -c": I
> suspect the former should look more like the latter for consistency.
Lets look at this separately.
> 4) "video -dc" doesn't seem to reset auto_white_balance_temperature?
Agree that combination should work - Will check it later.
>
> Laurie
Marcus
Index: video.c
===
RCS file: /cvs/xenocara/app/video/video.c,v
retrieving revision 1.33
diff -u -p -u -p -r1.33 video.c
--- video.c 5 Aug 2020 11:34:00 - 1.33
+++ video.c 8 Aug 2020 12:28:33 -
@@ -94,6 +94,7 @@ struct dev_ctrls {
char*name;
int supported;
int id;
+ int id_auto;
int def;
int min;
int max;
@@ -101,24 +102,25 @@ struct dev_ctrls {
int cur;
} ctrls[] = {
#define CTRL_BRIGHTNESS0
- { "brightness", 0, V4L2_CID_BRIGHTNESS, 0, 0, 0, 0, 0 },
+ { "brightness", 0, V4L2_CID_BRIGHTNESS, 0, 0, 0, 0, 0, 0 },
#define CTRL_CONTRAST 1
- { "contrast", 0, V4L2_CID_CONTRAST, 0, 0, 0, 0, 0 },
+ { "contrast", 0, V4L2_CID_CONTRAST, 0, 0, 0, 0, 0, 0 },
#define CTRL_SATURATION2
- { "saturation", 0, V4L2_CID_SATURATION, 0, 0, 0, 0, 0 },
+ { "saturation", 0, V4L2_CID_SATURATION, 0, 0, 0, 0, 0, 0 },
#define CTRL_HUE 3
- { "hue",0, V4L2_CID_HUE,0, 0, 0, 0, 0 },
+ { "hue",0, V4L2_CID_HUE,0, 0, 0, 0, 0, 0 },
#define CTRL_GAIN 4
- { "gain", 0, V4L2_CID_GAIN, 0, 0, 0, 0, 0 },
+ { "gain", 0, V4L2_CID_GAIN, 0, 0, 0, 0, 0, 0 },
#define CTRL_GAMMA 5
- { "gamma", 0, V4L2_CID_GAMMA, 0, 0, 0, 0, 0 },
+ { "gamma", 0, V4L2_CID_GAMMA, 0, 0, 0, 0, 0, 0 },
#define CTRL_SHARPNESS 6
- { "sharpness", 0, V4L2_CID_SHARPNESS, 0, 0, 0, 0, 0 },
+ { "sharpness", 0, V4L2_CID_SHARPNESS, 0, 0, 0, 0, 0, 0 },
#define CTRL_WHITE_BALANCE_TEMPERATURE 7
{ "white_balance_temperature",
- 0, V4L2_CID_WHITE_BALANCE_TEMPERATURE, 0, 0, 0, 0, 0 },
+ 0, V4L2_CID_WHITE_BALANCE_TEMPERATURE,
+ V4L2_CID_AUTO_WHITE_BALANCE, 0, 0, 0, 0, 0 },
#define CTRL_LAST 8
- { NULL, 0, 0, 0, 0, 0, 0, 0 }
+ { NULL, 0, 0, 0, 0, 0, 0, 0, 0 }
};
/* frame dimensions */
@@ -218,6 +220,7 @@ int dev_init(struct video *);
int dev_set_ctrl_abs(struct video *vid, int, int);
void dev_set_ctrl_rel(struct video *, int, int);
void dev_set_ctrl_auto_white_balance(struct video *, int, int);
+int dev_get_ctrl_auto(struct video *, int);
void dev_reset_ctrls(struct video *);
int parse_ctrl(struct video *, int, char **);
@@ -1102,6 +1105,24 @@ dev_set_ctrl_auto_white_balance(struct v
}
}
+int
+dev_get_ctrl_auto(struct video *vid, int ctrl)
+{
+ struct dev *d = >dev;
+ struct v4l2_control control;
+
+ if (!ctrls[ctrl].id_auto)
+ return 0;
+
+ control.id = ctrls[ctrl].id_auto;
+ if (ioctl(d->fd, VIDIOC_G_CTRL, ) != 0) {
+ warn("VIDIOC_G_CTRL");
+ return 0;
+ }
+
+ return (control.value);
+}
+
void
dev_reset_ctrls(struct video *vid)
{
@@ -1195,7 +1216,12 @@
Re: brconfig: strto*l -> strtonum()
On Sat, 08 Aug 2020 05:09:22 +0200, Klemens Nanni wrote: > Alternatively, we can avoid duplicating the ioctl specific min/max > values in strtonum(3) calls, just use the struct member type's *_MAX > defines and rely on the kernel for appropiate boundary checks - this is > what the code does now. I like this approach. OK millert@ - todd
Re: describe 'idle-timeout' exception in npppd.conf man page
On Fri, 7 Aug 2020 22:19:05 +0300 Vitaliy Makkoveev wrote: > Some times ago we disabled in-kernel timeout for pppx(4) related > pipex(4) sessions. We did this for prevent use after free issue caused > by pipex_timer [1]. By default "idle-timeout" is not set in > npppd.conf(5) and I guess this is reason for we forgot to describe this > exception in npppd.conf(5). > > But looks like one user caught this [2]. So I propose to describe this > in BUGS section of npppd.conf(5). > > Also current "idle-timeout" description looks incorrect. If this option > is missing, there is not in-kernel timeout for this session, but > npppd(8) uses it's own timeout for. And we can't configure this value. > > YASUOKA, what do you think? May be we can kill in-kernel timeout feature > for pipex(4)?, and make npppd(8)'s idle timeout configurable by this > option? I think we should mention this to the man page until we fix it. So I'd like you to update the man page first. I'll try to review the problem. > 1. > https://cvsweb.openbsd.org/src/sys/net/if_pppx.c?rev=1.78=text/x-cvsweb-markup > 2. https://marc.info/?l=openbsd-misc=159655468504864=2 > > > Index: usr.sbin/npppd/npppd/npppd.conf.5 > === > RCS file: /cvs/src/usr.sbin/npppd/npppd/npppd.conf.5,v > retrieving revision 1.27 > diff -u -p -r1.27 npppd.conf.5 > --- usr.sbin/npppd/npppd/npppd.conf.5 23 Apr 2020 21:10:54 - 1.27 > +++ usr.sbin/npppd/npppd/npppd.conf.5 7 Aug 2020 19:17:00 - > @@ -699,3 +699,9 @@ The current version of > .Xr npppd 8 > does not support adding or removing tunnel settings or changing listener > settings (listen address, port and l2tp-ipsec-require). > +.Pp > +This time > +.Xr pppx 4 > +does not allow to create sessions with non null > +.Ic idle-timeout > +option.
Re: allow TCP connections to IPv6 anycast addresses
On Sat, Aug 08 2020, Florian Obser wrote:
> On Fri, Aug 07, 2020 at 11:52:46PM +0200, Jeremie Courreges-Anglas wrote:
>> If you don't want to remove M_ACAST from sys/mbuf.h, can you please at
>> least change the comment? /* obsolete */ or something.
>
> Good point, I forgot to ask about what to do with the flag.
> I think we can remove it, from what I understand %b in printf(9) works
> fine with a sparse decoding string.
Should be fine indeed.
> It compiles but I have no idea how to test it in ddb.
show mbuf addr in a function that uses an mbuf?
> OK? Better to leave out the comment?
I think the comment can be dropped along with the #define. Userland
shouldn't be poking at this.
ok jca@
> diff --git sys/mbuf.h sys/mbuf.h
> index d52896d3be8..3ddd1b89d66 100644
> --- sys/mbuf.h
> +++ sys/mbuf.h
> @@ -190,7 +190,7 @@ struct mbuf {
> /* mbuf pkthdr flags, also in m_flags */
> #define M_VLANTAG0x0020 /* ether_vtag is valid */
> #define M_LOOP 0x0040 /* packet has been sent from local
> machine */
> -#define M_ACAST 0x0080 /* received as IPv6 anycast */
> + /* 0x0080 used to be M_ACAST */
> #define M_BCAST 0x0100 /* sent/received as link-level
> broadcast */
> #define M_MCAST 0x0200 /* sent/received as link-level
> multicast */
> #define M_CONF 0x0400 /* payload was encrypted
> (ESP-transport) */
> @@ -203,14 +203,13 @@ struct mbuf {
> #ifdef _KERNEL
> #define M_BITS \
> ("\20\1M_EXT\2M_PKTHDR\3M_EOR\4M_EXTWR\5M_PROTO1\6M_VLANTAG\7M_LOOP" \
> -"\10M_ACAST\11M_BCAST\12M_MCAST\13M_CONF\14M_AUTH\15M_TUNNEL" \
> +"\11M_BCAST\12M_MCAST\13M_CONF\14M_AUTH\15M_TUNNEL" \
> "\16M_ZEROIZE\17M_COMP\20M_LINK0")
> #endif
>
> /* flags copied when copying m_pkthdr */
> #define M_COPYFLAGS
> (M_PKTHDR|M_EOR|M_PROTO1|M_BCAST|M_MCAST|M_CONF|M_COMP|\
> - M_AUTH|M_LOOP|M_TUNNEL|M_LINK0|M_VLANTAG|M_ACAST|\
> - M_ZEROIZE)
> + M_AUTH|M_LOOP|M_TUNNEL|M_LINK0|M_VLANTAG|M_ZEROIZE)
>
> /* Checksumming flags */
> #define M_IPV4_CSUM_OUT 0x0001 /* IPv4 checksum needed */
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: allow TCP connections to IPv6 anycast addresses
On Fri, Aug 07, 2020 at 11:52:46PM +0200, Jeremie Courreges-Anglas wrote:
> If you don't want to remove M_ACAST from sys/mbuf.h, can you please at
> least change the comment? /* obsolete */ or something.
Good point, I forgot to ask about what to do with the flag.
I think we can remove it, from what I understand %b in printf(9) works
fine with a sparse decoding string.
It compiles but I have no idea how to test it in ddb.
OK? Better to leave out the comment?
diff --git sys/mbuf.h sys/mbuf.h
index d52896d3be8..3ddd1b89d66 100644
--- sys/mbuf.h
+++ sys/mbuf.h
@@ -190,7 +190,7 @@ struct mbuf {
/* mbuf pkthdr flags, also in m_flags */
#define M_VLANTAG 0x0020 /* ether_vtag is valid */
#define M_LOOP 0x0040 /* packet has been sent from local machine */
-#define M_ACAST0x0080 /* received as IPv6 anycast */
+ /* 0x0080 used to be M_ACAST */
#define M_BCAST0x0100 /* sent/received as link-level
broadcast */
#define M_MCAST0x0200 /* sent/received as link-level
multicast */
#define M_CONF 0x0400 /* payload was encrypted (ESP-transport) */
@@ -203,14 +203,13 @@ struct mbuf {
#ifdef _KERNEL
#define M_BITS \
("\20\1M_EXT\2M_PKTHDR\3M_EOR\4M_EXTWR\5M_PROTO1\6M_VLANTAG\7M_LOOP" \
-"\10M_ACAST\11M_BCAST\12M_MCAST\13M_CONF\14M_AUTH\15M_TUNNEL" \
+"\11M_BCAST\12M_MCAST\13M_CONF\14M_AUTH\15M_TUNNEL" \
"\16M_ZEROIZE\17M_COMP\20M_LINK0")
#endif
/* flags copied when copying m_pkthdr */
#defineM_COPYFLAGS
(M_PKTHDR|M_EOR|M_PROTO1|M_BCAST|M_MCAST|M_CONF|M_COMP|\
-M_AUTH|M_LOOP|M_TUNNEL|M_LINK0|M_VLANTAG|M_ACAST|\
-M_ZEROIZE)
+M_AUTH|M_LOOP|M_TUNNEL|M_LINK0|M_VLANTAG|M_ZEROIZE)
/* Checksumming flags */
#defineM_IPV4_CSUM_OUT 0x0001 /* IPv4 checksum needed */
--
I'm not entirely sure you are real.
