Re: timeout(9): add clock-based timeouts (attempt 2)

2020-09-07 Thread Scott Cheloha
On Sat, Sep 05, 2020 at 01:11:59PM +0200, Mark Kettenis wrote:
> > Date: Fri, 4 Sep 2020 17:55:39 -0500
> > From: Scott Cheloha 
> > 
> > On Sat, Jul 25, 2020 at 08:46:08PM -0500, Scott Cheloha wrote:
> > > 
> > > [...]
> > > 
> > > I want to add clock-based timeouts to the kernel because tick-based
> > > timeouts suffer from a few problems:
> > > 
> > > [...]
> > > 
> > > Basically, ticks are a poor approximation for the system clock.  We
> > > should use the real thing where possible.
> > > 
> > > [...]
> > > 
> > > Thoughts on this approach?  Thoughts on the proposed API?
> > 
> > 6 week bump.
> > 
> > Attached is an rebased and streamlined diff.
> > 
> > Let's try again:
> > 
> > This patch adds support for timeouts scheduled against the hardware
> > timecounter.  I call these "kclock timeouts".  They are distinct from
> > the current tick-based timeouts because ticks are "software time", not
> > "real time".
> 
> So what's the end game here?  Are these kclock-based timeouts going to
> replace the tick-based timeouts at some point in the future?  I can
> see why you want to have both in parallel for a while, but long-term I
> don't think we want to keep both.

Ideally we would replace tick-based timeouts entirely with kclock
timeouts eventually.

There are a few roadblocks, though:

1. The scheduler is tick-based.  If you want to wait until the next
   tick, the easiest way to do that is with timeout_add(9) or tsleep(9).

2. Linux has ktimers, which is tick-based.  drm uses it.  Shouldn't
   we have a tick-based timeout interface for compatibility with them?
   We could fake it, like FreeBSD does, but doing so is probably more
   complicated than just keeping support for tick-based timeouts.

3. Scheduling a timeout with timeout_add(9) is fast.  Scheduling a
   timeout with timeout_in_nsec(9) involves a clock read.  It is slower.
   It is probably too slow for some code.

(1) will be overcome if ever the scheduler is no longer tick-based.

(2) is tricky.  Maybe you or jsg@ have an opinion?

(3) is somewhat easier to fix.  I intend to introduce a TIMEOUT_COARSE
flag in the future which causes timeout_in_nsec() to call
getnanouptime(9) instead of nanouptime(9).  Reading the timestamp is
faster than reading the clock.  You lose accuracy, but any code
worried about the overhead of reading the clock is probably not very
concerned with accuracy.

> We don't really want to do a wholesale conversion of APIs again I'd
> say.  So at some point the existing timeout_add_xxx() calls should be
> implemented in terms of "kclock timeouts".

We can do this, but we'll still need to change the calls that
reschedule a periodic timeout to use the dedicated rescheduling
interface.  Otherwise those periodic timeouts will drift.  They don't
currently drift because a tick is a very coarse unit of time.  With
nanosecond resolution we'll get drift.

> This implementation is still tick driven, so it doesn't really provide
> sub-tick resolution.

Yes, that's right.  Each timeout maintains nanosecond resolution for
its expiration time but will only actually run after hardclock(9) runs
and dumps the timeout to softclock().

We would need to implement a more flexible clock interrupt scheduler
to run timeouts in between hardclocks.

> What does that mean for testing this?  I mean if we spend a lot of time
> now to verify that subsystems can tolerate the more fine-grained timeouts,
> we need to that again when you switch from having a period interrupt driving
> the wheel to having a scheduled interrupt isn't it?

Yes.  But both changes can break things.

I think we should do kclock timeouts before sub-tick timeouts.  The
former is a lot less disruptive than the latter, as the timeouts still
run right after the hardclock.

And you need kclock timeouts to even test sub-tick timeouts anyway.

> > For now we have one kclock, KCLOCK_UPTIME, which corresponds to
> > nanouptime(9).  In the future I intend to add support for runtime and
> > UTC kclocks.
> 
> Do we really need that?  I suppose it helps implementing something
> like clock_nanosleep() with the TIMER_ABSTIME flag for various
> clock_id values?

Exactly.

FreeBSD decided to not support multiple clocks in their revamped
callout(9).  The result is a bit simpler (one clock) but in order to
implement absolute CLOCK_REALTIME sleeps for userspace they have this
flag for each thread that causes the thread to wake up and reschedule
itself whenever settimeofday(2) happens.

It's clever, but it seems messy to me.

I would rather support UTC timeouts as "first class citizens" of the
timeout subsystem.  Linux's hrtimers API supports UTC timeouts
explicitly.  I prefer their approach.

The advantage of real support is that the timeout(9) subsystem will
handle rebucketing UTC timeouts if the clock jumps backwards
transparently.  This is nice.

> > Why do we want kclock timeouts at all?
> > 
> > 1. Kclock timeouts expire at an actual time, not a tick.  They
> >have nanosecond resolution and 

Re: PATCH: Fix PCI Config Space union size on VMM

2020-09-07 Thread Jordan Hargrave
This code fixes the pci device union for accessing PCI config space >= 0x40

Running pcidump -xxx in a virtual machine would return garbage data due to 
union overlap

On Mon, Sep 07, 2020 at 05:52:55PM -0500, Jordan Hargrave wrote:
> Index: pci.h
> ===
> RCS file: /cvs/src/usr.sbin/vmd/pci.h,v
> retrieving revision 1.7
> diff -u -p -u -r1.7 pci.h
> --- pci.h 17 Sep 2017 23:07:56 -  1.7
> +++ pci.h 7 Sep 2020 22:48:09 -
> @@ -32,43 +32,44 @@ typedef int (*pci_iobar_fn_t)(int dir, u
>  void *, uint8_t);
>  typedef int (*pci_mmiobar_fn_t)(int dir, uint32_t ofs, uint32_t *data);
>  
> -union pci_dev {
> - uint32_t pd_cfg_space[PCI_CONFIG_SPACE_SIZE / 4];
>  
> - struct {
> - uint16_t pd_vid;
> - uint16_t pd_did;
> - uint16_t pd_cmd;
> - uint16_t pd_status;
> - uint8_t pd_rev;
> - uint8_t pd_prog_if;
> - uint8_t pd_subclass;
> - uint8_t pd_class;
> - uint8_t pd_cache_size;
> - uint8_t pd_lat_timer;
> - uint8_t pd_header_type;
> - uint8_t pd_bist;
> - uint32_t pd_bar[PCI_MAX_BARS];
> - uint32_t pd_cardbus_cis;
> - uint16_t pd_subsys_vid;
> - uint16_t pd_subsys_id;
> - uint32_t pd_exp_rom_addr;
> - uint8_t pd_cap;
> - uint32_t pd_reserved0 : 24;
> - uint32_t pd_reserved1;
> - uint8_t pd_irq;
> - uint8_t pd_int;
> - uint8_t pd_min_grant;
> - uint8_t pd_max_grant;
> +struct pci_dev {
> + union {
> + uint32_t pd_cfg_space[PCI_CONFIG_SPACE_SIZE / 4];
> + struct {
> + uint16_t pd_vid;
> + uint16_t pd_did;
> + uint16_t pd_cmd;
> + uint16_t pd_status;
> + uint8_t pd_rev;
> + uint8_t pd_prog_if;
> + uint8_t pd_subclass;
> + uint8_t pd_class;
> + uint8_t pd_cache_size;
> + uint8_t pd_lat_timer;
> + uint8_t pd_header_type;
> + uint8_t pd_bist;
> + uint32_t pd_bar[PCI_MAX_BARS];
> + uint32_t pd_cardbus_cis;
> + uint16_t pd_subsys_vid;
> + uint16_t pd_subsys_id;
> + uint32_t pd_exp_rom_addr;
> + uint8_t pd_cap;
> + uint32_t pd_reserved0 : 24;
> + uint32_t pd_reserved1;
> + uint8_t pd_irq;
> + uint8_t pd_int;
> + uint8_t pd_min_grant;
> + uint8_t pd_max_grant;
> + } __packed;
> + };
> + uint8_t pd_bar_ct;
> + pci_cs_fn_t pd_csfunc;
>  
> - uint8_t pd_bar_ct;
> - pci_cs_fn_t pd_csfunc;
> -
> - uint8_t pd_bartype[PCI_MAX_BARS];
> - uint32_t pd_barsize[PCI_MAX_BARS];
> - void *pd_barfunc[PCI_MAX_BARS];
> - void *pd_bar_cookie[PCI_MAX_BARS];
> - } __packed;
> + uint8_t pd_bartype[PCI_MAX_BARS];
> + uint32_t pd_barsize[PCI_MAX_BARS];
> + void *pd_barfunc[PCI_MAX_BARS];
> + void *pd_bar_cookie[PCI_MAX_BARS];
>  };
>  
>  struct pci {
> @@ -79,7 +80,7 @@ struct pci {
>   uint32_t pci_addr_reg;
>   uint32_t pci_data_reg;
>  
> - union pci_dev pci_devices[PCI_CONFIG_MAX_DEV];
> + struct pci_dev pci_devices[PCI_CONFIG_MAX_DEV];
>  };
>  
>  void pci_handle_address_reg(struct vm_run_params *);
> 



PATCH: Fix PCI Config Space union size on VMM

2020-09-07 Thread Jordan Hargrave
Index: pci.h
===
RCS file: /cvs/src/usr.sbin/vmd/pci.h,v
retrieving revision 1.7
diff -u -p -u -r1.7 pci.h
--- pci.h   17 Sep 2017 23:07:56 -  1.7
+++ pci.h   7 Sep 2020 22:48:09 -
@@ -32,43 +32,44 @@ typedef int (*pci_iobar_fn_t)(int dir, u
 void *, uint8_t);
 typedef int (*pci_mmiobar_fn_t)(int dir, uint32_t ofs, uint32_t *data);
 
-union pci_dev {
-   uint32_t pd_cfg_space[PCI_CONFIG_SPACE_SIZE / 4];
 
-   struct {
-   uint16_t pd_vid;
-   uint16_t pd_did;
-   uint16_t pd_cmd;
-   uint16_t pd_status;
-   uint8_t pd_rev;
-   uint8_t pd_prog_if;
-   uint8_t pd_subclass;
-   uint8_t pd_class;
-   uint8_t pd_cache_size;
-   uint8_t pd_lat_timer;
-   uint8_t pd_header_type;
-   uint8_t pd_bist;
-   uint32_t pd_bar[PCI_MAX_BARS];
-   uint32_t pd_cardbus_cis;
-   uint16_t pd_subsys_vid;
-   uint16_t pd_subsys_id;
-   uint32_t pd_exp_rom_addr;
-   uint8_t pd_cap;
-   uint32_t pd_reserved0 : 24;
-   uint32_t pd_reserved1;
-   uint8_t pd_irq;
-   uint8_t pd_int;
-   uint8_t pd_min_grant;
-   uint8_t pd_max_grant;
+struct pci_dev {
+   union {
+   uint32_t pd_cfg_space[PCI_CONFIG_SPACE_SIZE / 4];
+   struct {
+   uint16_t pd_vid;
+   uint16_t pd_did;
+   uint16_t pd_cmd;
+   uint16_t pd_status;
+   uint8_t pd_rev;
+   uint8_t pd_prog_if;
+   uint8_t pd_subclass;
+   uint8_t pd_class;
+   uint8_t pd_cache_size;
+   uint8_t pd_lat_timer;
+   uint8_t pd_header_type;
+   uint8_t pd_bist;
+   uint32_t pd_bar[PCI_MAX_BARS];
+   uint32_t pd_cardbus_cis;
+   uint16_t pd_subsys_vid;
+   uint16_t pd_subsys_id;
+   uint32_t pd_exp_rom_addr;
+   uint8_t pd_cap;
+   uint32_t pd_reserved0 : 24;
+   uint32_t pd_reserved1;
+   uint8_t pd_irq;
+   uint8_t pd_int;
+   uint8_t pd_min_grant;
+   uint8_t pd_max_grant;
+   } __packed;
+   };
+   uint8_t pd_bar_ct;
+   pci_cs_fn_t pd_csfunc;
 
-   uint8_t pd_bar_ct;
-   pci_cs_fn_t pd_csfunc;
-
-   uint8_t pd_bartype[PCI_MAX_BARS];
-   uint32_t pd_barsize[PCI_MAX_BARS];
-   void *pd_barfunc[PCI_MAX_BARS];
-   void *pd_bar_cookie[PCI_MAX_BARS];
-   } __packed;
+   uint8_t pd_bartype[PCI_MAX_BARS];
+   uint32_t pd_barsize[PCI_MAX_BARS];
+   void *pd_barfunc[PCI_MAX_BARS];
+   void *pd_bar_cookie[PCI_MAX_BARS];
 };
 
 struct pci {
@@ -79,7 +80,7 @@ struct pci {
uint32_t pci_addr_reg;
uint32_t pci_data_reg;
 
-   union pci_dev pci_devices[PCI_CONFIG_MAX_DEV];
+   struct pci_dev pci_devices[PCI_CONFIG_MAX_DEV];
 };
 
 void pci_handle_address_reg(struct vm_run_params *);



Re: incorrect result from getppid for ptraced processes

2020-09-07 Thread Mateusz Guzik
On 9/5/20, Philip Guenther  wrote:
> On Fri, Sep 4, 2020 at 2:59 PM Mateusz Guzik  wrote:
>
>> On 9/5/20, Philip Guenther  wrote:
>> > On Fri, Sep 4, 2020 at 1:06 PM Mateusz Guzik  wrote:
>> >
>> >> On 9/4/20, Vitaliy Makkoveev  wrote:
>> >> > On Fri, Sep 04, 2020 at 05:24:42PM +0200, Mateusz Guzik wrote:
>> >> >> getppid blindly follows the parent pointer and reads the pid.
>> >> >>
>> >> >> The problem is that ptrace reparents the traced process, so in
>> >> >> particular if you gdb -p $something, the target proc will start
>> seeing
>> >> >> gdb instead of its actual parent.
>> >> >>
>> >> >> There is a lot to say about the entire reparenting business or
>> storing
>> >> >> the original pid in ps_oppid (instead of some form of a reference
>> >> >> to
>> >> >> the process).
>> >> >>
>> >> >> However, I think the most feasible fix for now is the same thing
>> >> >> FreeBSD did: *always* store the actual parent pid in ps_oppid. This
>> >> >> means all repareting will keep updating it (most notably when
>> >> >> abandoning children on exit), while ptrace will skip that part.
>> >> >>
>> >> >> Side effect of such a change be that getppid will stop requiring
>> >> >> the
>> >> >> kernel lock.
>> >> >>
>> >> >
>> >> > Thanks for report. But we are in beta stage now so such modification
>> is
>> >> > impossible until next iteration.
>> >> >
>> >> > Since original parent identifier is stored as `ps_oppid' while
>> >> > process
>> >> > is traced we just return it to userland for this case. This is the
>> >> > way
>> >> > I
>> >> > propose to fix this bug for now.
>> >> >
>> >> > Comments? OKs?
>> >> >
>> >> > Index: sys/kern/kern_prot.c
>> >> > ===
>> >> > RCS file: /cvs/src/sys/kern/kern_prot.c,v
>> >> > retrieving revision 1.76
>> >> > diff -u -p -r1.76 kern_prot.c
>> >> > --- sys/kern/kern_prot.c  9 Jul 2019 12:23:25 -   1.76
>> >> > +++ sys/kern/kern_prot.c  4 Sep 2020 21:12:15 -
>> >> > @@ -84,7 +84,11 @@ int
>> >> >  sys_getppid(struct proc *p, void *v, register_t *retval)
>> >> >  {
>> >> >
>> >> > - *retval = p->p_p->ps_pptr->ps_pid;
>> >> > + if (p->p_p->ps_flags & PS_TRACED)
>> >> > + *retval = p->p_p->ps_oppid;
>> >> > + else
>> >> > + *retval = p->p_p->ps_pptr->ps_pid;
>> >> > +
>> >> >   return (0);
>> >> >  }
>> >>
>> >> This is definitely a bare minimum fix, but it does the job.
>> >>
>> >
>> > ptrace() has behaved like this for the life of OpenBSD and an
>> > indefinite
>> > number of years previous in the BSD releases.  What has happened that a
>> > definitely incomplete fix is needed Right Now?
>>
>> I don't see how this reads as a demand this is fixed Right Now.
>>
>
> I didn't call it a demand, but the point stands: what has changed?
>

Nothing has changed. I don't use the system apart from sometimes
testing stuff in a VM. There is 0 pressure on my end to fix this.

>
> I don't see how the fix is incomplete either. It can be done better
>> with more effort, but AFAICS the above results in correct behavior.
>>
>
> There are at least 2 other uses of ps_pptr->ps_pid that should also change,
> unless you like coredumps and ps disagreeing with getppid(), and someone
> needs to think how it affects doas.
>

I see, I did not grep for these.

-- 
Mateusz Guzik 



Re: acpiapplesmc(4)

2020-09-07 Thread Marcus Glocker
On Mon, 7 Sep 2020 20:50:20 +0200 (CEST)
Mark Kettenis  wrote:

> > Date: Mon, 7 Sep 2020 19:59:13 +0200
> > From: Marcus Glocker 
> > 
> > On Mon, 7 Sep 2020 19:25:00 +0200 (CEST)
> > Mark Kettenis  wrote:
> >   
> > > > Date: Mon, 7 Sep 2020 12:02:15 -0500
> > > > From: joshua stein 
> > > > 
> > > > On Mon, 07 Sep 2020 at 06:58:01 +0200, Marcus Glocker wrote:
> > > > > This is an initial driver for the Apple System Management
> > > > > Controller found in Intel based Apple computers.
> > > > > 
> > > > > The driver is currently missing support for the Sudden Motion
> > > > > Sensor (SMS), light sensor, and keyboard backlight since I
> > > > > don't have that hardware available to develop on.
> > > > > 
> > > > > On my iMac11,2 it can deliver fan and temperatures values:
> > > > > 
> > > > >   hw.sensors.acpiapplesmc0.temp0=24.00 degC (Airflow 1)
> > > > >   hw.sensors.acpiapplesmc0.temp1=33.00 degC (CPU Core 0)
> > > > >   hw.sensors.acpiapplesmc0.temp2=36.00 degC (CPU
> > > > > Heatsink) hw.sensors.acpiapplesmc0.temp3=40.00 degC (CPU Core
> > > > > 1) hw.sensors.acpiapplesmc0.temp4=47.00 degC (GPU)
> > > > >   hw.sensors.acpiapplesmc0.temp5=45.00 degC (GPU
> > > > > Heatsink) hw.sensors.acpiapplesmc0.temp6=59.00 degC (PCH)
> > > > >   hw.sensors.acpiapplesmc0.temp7=42.00 degC (Memory)
> > > > >   hw.sensors.acpiapplesmc0.temp8=45.00 degC (Mainboard
> > > > > Proximity) hw.sensors.acpiapplesmc0.fan0=998 RPM
> > > > >   hw.sensors.acpiapplesmc0.fan1=1132 RPM
> > > > >   hw.sensors.acpiapplesmc0.fan2=1198 RPM
> > > > > 
> > > > > Feedback, testers, OKs?
> > > > 
> > > > Are there machines where asmc(4) will also attach?
> > > 
> > > Good point.  My old Macmini1,1 has:
> > > 
> > > ...
> > > "APP0001" at acpi0 not configured
> > > ...
> > > asmc0 at isa0 port 0x300/32: rev 1.3f503, 137 keys
> > > ...
> > > 
> > > So yes, I'd say there are.
> > > 
> > > 
> > > Having an acpi attachment is probably better than doing isa
> > > probes. But we probably should consolidate the drivers.  
> > 
> > D'oh!  I wasn't even aware that we already have an asmc(4) driver
> > in our tree.  Shame on me :-|
> > 
> > Glancing over asmc(4) I don't think there is anything more that my
> > driver would support other than attaching over acpi(4).  Would it be
> > possible to only write an acpi glue which attaches to asmc(4)?  
> 
> I think we'd just want to turn it into an acpi(4) driver.  Or maybe
> dump it in favour of your driver.

Ok.  I'll give it a try to convert asmc(4) in to an acpi(4) driver and
see how it works here.



Re: acpiapplesmc(4)

2020-09-07 Thread Mark Kettenis
> Date: Mon, 7 Sep 2020 19:59:13 +0200
> From: Marcus Glocker 
> 
> On Mon, 7 Sep 2020 19:25:00 +0200 (CEST)
> Mark Kettenis  wrote:
> 
> > > Date: Mon, 7 Sep 2020 12:02:15 -0500
> > > From: joshua stein 
> > > 
> > > On Mon, 07 Sep 2020 at 06:58:01 +0200, Marcus Glocker wrote:  
> > > > This is an initial driver for the Apple System Management
> > > > Controller found in Intel based Apple computers.
> > > > 
> > > > The driver is currently missing support for the Sudden Motion
> > > > Sensor (SMS), light sensor, and keyboard backlight since I don't
> > > > have that hardware available to develop on.
> > > > 
> > > > On my iMac11,2 it can deliver fan and temperatures values:
> > > > 
> > > > hw.sensors.acpiapplesmc0.temp0=24.00 degC (Airflow 1)
> > > > hw.sensors.acpiapplesmc0.temp1=33.00 degC (CPU Core 0)
> > > > hw.sensors.acpiapplesmc0.temp2=36.00 degC (CPU Heatsink)
> > > > hw.sensors.acpiapplesmc0.temp3=40.00 degC (CPU Core 1)
> > > > hw.sensors.acpiapplesmc0.temp4=47.00 degC (GPU)
> > > > hw.sensors.acpiapplesmc0.temp5=45.00 degC (GPU Heatsink)
> > > > hw.sensors.acpiapplesmc0.temp6=59.00 degC (PCH)
> > > > hw.sensors.acpiapplesmc0.temp7=42.00 degC (Memory)
> > > > hw.sensors.acpiapplesmc0.temp8=45.00 degC (Mainboard
> > > > Proximity) hw.sensors.acpiapplesmc0.fan0=998 RPM
> > > > hw.sensors.acpiapplesmc0.fan1=1132 RPM
> > > > hw.sensors.acpiapplesmc0.fan2=1198 RPM
> > > > 
> > > > Feedback, testers, OKs?  
> > > 
> > > Are there machines where asmc(4) will also attach?  
> > 
> > Good point.  My old Macmini1,1 has:
> > 
> > ...
> > "APP0001" at acpi0 not configured
> > ...
> > asmc0 at isa0 port 0x300/32: rev 1.3f503, 137 keys
> > ...
> > 
> > So yes, I'd say there are.
> > 
> > 
> > Having an acpi attachment is probably better than doing isa probes.
> > But we probably should consolidate the drivers.
> 
> D'oh!  I wasn't even aware that we already have an asmc(4) driver in our
> tree.  Shame on me :-|
> 
> Glancing over asmc(4) I don't think there is anything more that my
> driver would support other than attaching over acpi(4).  Would it be
> possible to only write an acpi glue which attaches to asmc(4)?

I think we'd just want to turn it into an acpi(4) driver.  Or maybe
dump it in favour of your driver.



Re: [PATCH] Add common PCIE capability list

2020-09-07 Thread Mark Kettenis
> Date: Mon, 7 Sep 2020 13:33:14 -0500
> From: Jordan Hargrave 
> 
> Attaching the full diff

ok kettenis@

> On Mon, Sep 07, 2020 at 01:09:12PM -0500, Jordan Hargrave wrote:
> > On Thu, Sep 03, 2020 at 08:37:56PM +0200, Mark Kettenis wrote:
> > > > Date: Wed, 2 Sep 2020 15:19:55 +1000
> > > > From: Jonathan Gray 
> > > > 
> > > > On Tue, Sep 01, 2020 at 11:44:03PM -0500, Jordan Hargrave wrote:
> > > > > This patch adds a common function for scanning PCIE Express 
> > > > > Capability list
> > > > > The PCIE Capability list starts at 0x100 in extended PCI 
> > > > > configuration space.
> > > > 
> > > > This seems to only handle extended capabilities?
> > > > Something like pcie_get_ext_capability() would be a better name.
> > > 
> > > I think it should be pci_get_ext_capability() which follows the
> > > pattern set by pci_get_ht_capability().
> > > 
> > > > 
> > > > It is 'PCI Express' not 'PCIExpress'
> > > > 
> > > > 'ofs & 3' test doesn't make sense when PCI_PCIE_ECAP_NEXT() always
> > > > masks those bits.
> > > > 
> > > > > 
> > > > > ---
> > > > >  sys/dev/pci/pci.c| 28 
> > > > >  sys/dev/pci/pcivar.h |  2 ++
> > > > >  2 files changed, 30 insertions(+)
> > > > > 
> > > > > diff --git a/sys/dev/pci/pci.c b/sys/dev/pci/pci.c
> > > > > index bf75f875e..8f9a5ef7a 100644
> > > > > --- a/sys/dev/pci/pci.c
> > > > > +++ b/sys/dev/pci/pci.c
> > > > > @@ -677,6 +677,34 @@ pci_get_ht_capability(pci_chipset_tag_t pc, 
> > > > > pcitag_t tag, int capid,
> > > > >   return (0);
> > > > >  }
> > > > >  
> > > > > +int
> > > > > +pcie_get_capability(pci_chipset_tag_t pc, pcitag_t tag, int capid,
> > > > > +int *offset, pcireg_t *value)
> > > > > +{
> > > > > + pcireg_t reg;
> > > > > + unsigned int ofs;
> > > > > +
> > > > > + /* Make sure we support PCIExpress device */
> > > 
> > > PCI Express like jsg@ already mentioned.  Add a full stop at the end
> > > of the sentence.
> > > 
> > > > > + if (pci_get_capability(pc, tag, PCI_CAP_PCIEXPRESS, NULL, NULL) 
> > > > > == 0)
> > > > > + return (0);
> > > > > + /* Scan PCIExpress capabilities */
> > > 
> > > Drop this comment and replace with a blank line such that it matches
> > > pci_get_ht_capability().
> > > 
> > > > > + ofs = PCI_PCIE_ECAP;
> > > > > + while (ofs != 0) {
> > > > > + if ((ofs & 3) || (ofs < PCI_PCIE_ECAP))
> > > > > + return (0);
> > > 
> > > Make this check #ifdef DIAGNOSTIC like pci_get_ht_capability() doesn.
> > > Dropping the (ofs & 3) bit is indeed a good idea.
> > > 
> > > > > + reg = pci_conf_read(pc, tag, ofs);
> > > > > + if (PCI_PCIE_ECAP_ID(reg) == capid) {
> > > > > + if (offset)
> > > > > + *offset = ofs;
> > > > > + if (value)
> > > > > + *value = reg;
> > > > > + return (1);
> > > > > + }
> > > > > + ofs = PCI_PCIE_ECAP_NEXT(reg);
> > > > > + }
> > > 
> > > Blank line here please.
> > > 
> > > > > + return (0);
> > > > > +}
> > > > > +
> > > > >  uint16_t
> > > > >  pci_requester_id(pci_chipset_tag_t pc, pcitag_t tag)
> > > > >  {
> > > > > diff --git a/sys/dev/pci/pcivar.h b/sys/dev/pci/pcivar.h
> > > > > index bdfe0404f..0376ba992 100644
> > > > > --- a/sys/dev/pci/pcivar.h
> > > > > +++ b/sys/dev/pci/pcivar.h
> > > > > @@ -233,6 +233,8 @@ int   pci_io_find(pci_chipset_tag_t, 
> > > > > pcitag_t, int, bus_addr_t *,
> > > > >  int  pci_mem_find(pci_chipset_tag_t, pcitag_t, int, bus_addr_t *,
> > > > >   bus_size_t *, int *);
> > > > >  
> > > > > +int  pcie_get_capability(pci_chipset_tag_t, pcitag_t, int,
> > > > > + int *, pcireg_t *);
> > > > >  int  pci_get_capability(pci_chipset_tag_t, pcitag_t, int,
> > > > >   int *, pcireg_t *);
> > > > >  int  pci_get_ht_capability(pci_chipset_tag_t, pcitag_t, int,
> > > > > -- 
> > > > > 2.26.2
> > > > > 
> > > > > 
> > > > 
> > > > 
> > diff --git a/sys/dev/pci/pci.c b/sys/dev/pci/pci.c
> > index 8f9a5ef7a..f649b3e79 100644
> > --- a/sys/dev/pci/pci.c
> > +++ b/sys/dev/pci/pci.c
> > @@ -678,20 +678,22 @@ pci_get_ht_capability(pci_chipset_tag_t pc, pcitag_t 
> > tag, int capid,
> >  }
> >  
> >  int
> > -pcie_get_capability(pci_chipset_tag_t pc, pcitag_t tag, int capid,
> > +pci_get_ext_capability(pci_chipset_tag_t pc, pcitag_t tag, int capid,
> >  int *offset, pcireg_t *value)
> >  {
> > pcireg_t reg;
> > unsigned int ofs;
> >  
> > -   /* Make sure we support PCIExpress device */
> > +   /* Make sure we support PCI Express device */
> > if (pci_get_capability(pc, tag, PCI_CAP_PCIEXPRESS, NULL, NULL) == 0)
> > return (0);
> > -   /* Scan PCIExpress capabilities */
> > +   /* Scan PCI Express capabilities */
> > ofs = PCI_PCIE_ECAP;
> > while (ofs != 0) {
> > +#ifdef DIAGNOSTIC
> > if ((ofs & 3) || (ofs < PCI_PCIE_ECAP))
> >  

Re: [PATCH] Add common PCIE capability list

2020-09-07 Thread Jordan Hargrave


Attaching the full diff

On Mon, Sep 07, 2020 at 01:09:12PM -0500, Jordan Hargrave wrote:
> On Thu, Sep 03, 2020 at 08:37:56PM +0200, Mark Kettenis wrote:
> > > Date: Wed, 2 Sep 2020 15:19:55 +1000
> > > From: Jonathan Gray 
> > > 
> > > On Tue, Sep 01, 2020 at 11:44:03PM -0500, Jordan Hargrave wrote:
> > > > This patch adds a common function for scanning PCIE Express Capability 
> > > > list
> > > > The PCIE Capability list starts at 0x100 in extended PCI configuration 
> > > > space.
> > > 
> > > This seems to only handle extended capabilities?
> > > Something like pcie_get_ext_capability() would be a better name.
> > 
> > I think it should be pci_get_ext_capability() which follows the
> > pattern set by pci_get_ht_capability().
> > 
> > > 
> > > It is 'PCI Express' not 'PCIExpress'
> > > 
> > > 'ofs & 3' test doesn't make sense when PCI_PCIE_ECAP_NEXT() always
> > > masks those bits.
> > > 
> > > > 
> > > > ---
> > > >  sys/dev/pci/pci.c| 28 
> > > >  sys/dev/pci/pcivar.h |  2 ++
> > > >  2 files changed, 30 insertions(+)
> > > > 
> > > > diff --git a/sys/dev/pci/pci.c b/sys/dev/pci/pci.c
> > > > index bf75f875e..8f9a5ef7a 100644
> > > > --- a/sys/dev/pci/pci.c
> > > > +++ b/sys/dev/pci/pci.c
> > > > @@ -677,6 +677,34 @@ pci_get_ht_capability(pci_chipset_tag_t pc, 
> > > > pcitag_t tag, int capid,
> > > > return (0);
> > > >  }
> > > >  
> > > > +int
> > > > +pcie_get_capability(pci_chipset_tag_t pc, pcitag_t tag, int capid,
> > > > +int *offset, pcireg_t *value)
> > > > +{
> > > > +   pcireg_t reg;
> > > > +   unsigned int ofs;
> > > > +
> > > > +   /* Make sure we support PCIExpress device */
> > 
> > PCI Express like jsg@ already mentioned.  Add a full stop at the end
> > of the sentence.
> > 
> > > > +   if (pci_get_capability(pc, tag, PCI_CAP_PCIEXPRESS, NULL, NULL) 
> > > > == 0)
> > > > +   return (0);
> > > > +   /* Scan PCIExpress capabilities */
> > 
> > Drop this comment and replace with a blank line such that it matches
> > pci_get_ht_capability().
> > 
> > > > +   ofs = PCI_PCIE_ECAP;
> > > > +   while (ofs != 0) {
> > > > +   if ((ofs & 3) || (ofs < PCI_PCIE_ECAP))
> > > > +   return (0);
> > 
> > Make this check #ifdef DIAGNOSTIC like pci_get_ht_capability() doesn.
> > Dropping the (ofs & 3) bit is indeed a good idea.
> > 
> > > > +   reg = pci_conf_read(pc, tag, ofs);
> > > > +   if (PCI_PCIE_ECAP_ID(reg) == capid) {
> > > > +   if (offset)
> > > > +   *offset = ofs;
> > > > +   if (value)
> > > > +   *value = reg;
> > > > +   return (1);
> > > > +   }
> > > > +   ofs = PCI_PCIE_ECAP_NEXT(reg);
> > > > +   }
> > 
> > Blank line here please.
> > 
> > > > +   return (0);
> > > > +}
> > > > +
> > > >  uint16_t
> > > >  pci_requester_id(pci_chipset_tag_t pc, pcitag_t tag)
> > > >  {
> > > > diff --git a/sys/dev/pci/pcivar.h b/sys/dev/pci/pcivar.h
> > > > index bdfe0404f..0376ba992 100644
> > > > --- a/sys/dev/pci/pcivar.h
> > > > +++ b/sys/dev/pci/pcivar.h
> > > > @@ -233,6 +233,8 @@ int pci_io_find(pci_chipset_tag_t, pcitag_t, int, 
> > > > bus_addr_t *,
> > > >  intpci_mem_find(pci_chipset_tag_t, pcitag_t, int, bus_addr_t *,
> > > > bus_size_t *, int *);
> > > >  
> > > > +intpcie_get_capability(pci_chipset_tag_t, pcitag_t, int,
> > > > +   int *, pcireg_t *);
> > > >  intpci_get_capability(pci_chipset_tag_t, pcitag_t, int,
> > > > int *, pcireg_t *);
> > > >  intpci_get_ht_capability(pci_chipset_tag_t, pcitag_t, int,
> > > > -- 
> > > > 2.26.2
> > > > 
> > > > 
> > > 
> > > 
> diff --git a/sys/dev/pci/pci.c b/sys/dev/pci/pci.c
> index 8f9a5ef7a..f649b3e79 100644
> --- a/sys/dev/pci/pci.c
> +++ b/sys/dev/pci/pci.c
> @@ -678,20 +678,22 @@ pci_get_ht_capability(pci_chipset_tag_t pc, pcitag_t 
> tag, int capid,
>  }
>  
>  int
> -pcie_get_capability(pci_chipset_tag_t pc, pcitag_t tag, int capid,
> +pci_get_ext_capability(pci_chipset_tag_t pc, pcitag_t tag, int capid,
>  int *offset, pcireg_t *value)
>  {
>   pcireg_t reg;
>   unsigned int ofs;
>  
> - /* Make sure we support PCIExpress device */
> + /* Make sure we support PCI Express device */
>   if (pci_get_capability(pc, tag, PCI_CAP_PCIEXPRESS, NULL, NULL) == 0)
>   return (0);
> - /* Scan PCIExpress capabilities */
> + /* Scan PCI Express capabilities */
>   ofs = PCI_PCIE_ECAP;
>   while (ofs != 0) {
> +#ifdef DIAGNOSTIC
>   if ((ofs & 3) || (ofs < PCI_PCIE_ECAP))
>   return (0);
> +#endif
>   reg = pci_conf_read(pc, tag, ofs);
>   if (PCI_PCIE_ECAP_ID(reg) == capid) {
>   if (offset)
> @@ -702,6 +704,7 @@ pcie_get_capability(pci_chipset_tag_t pc, pcitag_t tag, 
> int capid,
>  

Re: [PATCH] Add common PCIE capability list

2020-09-07 Thread Jordan Hargrave
On Thu, Sep 03, 2020 at 08:37:56PM +0200, Mark Kettenis wrote:
> > Date: Wed, 2 Sep 2020 15:19:55 +1000
> > From: Jonathan Gray 
> > 
> > On Tue, Sep 01, 2020 at 11:44:03PM -0500, Jordan Hargrave wrote:
> > > This patch adds a common function for scanning PCIE Express Capability 
> > > list
> > > The PCIE Capability list starts at 0x100 in extended PCI configuration 
> > > space.
> > 
> > This seems to only handle extended capabilities?
> > Something like pcie_get_ext_capability() would be a better name.
> 
> I think it should be pci_get_ext_capability() which follows the
> pattern set by pci_get_ht_capability().
> 
> > 
> > It is 'PCI Express' not 'PCIExpress'
> > 
> > 'ofs & 3' test doesn't make sense when PCI_PCIE_ECAP_NEXT() always
> > masks those bits.
> > 
> > > 
> > > ---
> > >  sys/dev/pci/pci.c| 28 
> > >  sys/dev/pci/pcivar.h |  2 ++
> > >  2 files changed, 30 insertions(+)
> > > 
> > > diff --git a/sys/dev/pci/pci.c b/sys/dev/pci/pci.c
> > > index bf75f875e..8f9a5ef7a 100644
> > > --- a/sys/dev/pci/pci.c
> > > +++ b/sys/dev/pci/pci.c
> > > @@ -677,6 +677,34 @@ pci_get_ht_capability(pci_chipset_tag_t pc, pcitag_t 
> > > tag, int capid,
> > >   return (0);
> > >  }
> > >  
> > > +int
> > > +pcie_get_capability(pci_chipset_tag_t pc, pcitag_t tag, int capid,
> > > +int *offset, pcireg_t *value)
> > > +{
> > > + pcireg_t reg;
> > > + unsigned int ofs;
> > > +
> > > + /* Make sure we support PCIExpress device */
> 
> PCI Express like jsg@ already mentioned.  Add a full stop at the end
> of the sentence.
> 
> > > + if (pci_get_capability(pc, tag, PCI_CAP_PCIEXPRESS, NULL, NULL) == 0)
> > > + return (0);
> > > + /* Scan PCIExpress capabilities */
> 
> Drop this comment and replace with a blank line such that it matches
> pci_get_ht_capability().
> 
> > > + ofs = PCI_PCIE_ECAP;
> > > + while (ofs != 0) {
> > > + if ((ofs & 3) || (ofs < PCI_PCIE_ECAP))
> > > + return (0);
> 
> Make this check #ifdef DIAGNOSTIC like pci_get_ht_capability() doesn.
> Dropping the (ofs & 3) bit is indeed a good idea.
> 
> > > + reg = pci_conf_read(pc, tag, ofs);
> > > + if (PCI_PCIE_ECAP_ID(reg) == capid) {
> > > + if (offset)
> > > + *offset = ofs;
> > > + if (value)
> > > + *value = reg;
> > > + return (1);
> > > + }
> > > + ofs = PCI_PCIE_ECAP_NEXT(reg);
> > > + }
> 
> Blank line here please.
> 
> > > + return (0);
> > > +}
> > > +
> > >  uint16_t
> > >  pci_requester_id(pci_chipset_tag_t pc, pcitag_t tag)
> > >  {
> > > diff --git a/sys/dev/pci/pcivar.h b/sys/dev/pci/pcivar.h
> > > index bdfe0404f..0376ba992 100644
> > > --- a/sys/dev/pci/pcivar.h
> > > +++ b/sys/dev/pci/pcivar.h
> > > @@ -233,6 +233,8 @@ int   pci_io_find(pci_chipset_tag_t, pcitag_t, int, 
> > > bus_addr_t *,
> > >  int  pci_mem_find(pci_chipset_tag_t, pcitag_t, int, bus_addr_t *,
> > >   bus_size_t *, int *);
> > >  
> > > +int  pcie_get_capability(pci_chipset_tag_t, pcitag_t, int,
> > > + int *, pcireg_t *);
> > >  int  pci_get_capability(pci_chipset_tag_t, pcitag_t, int,
> > >   int *, pcireg_t *);
> > >  int  pci_get_ht_capability(pci_chipset_tag_t, pcitag_t, int,
> > > -- 
> > > 2.26.2
> > > 
> > > 
> > 
> > 
diff --git a/sys/dev/pci/pci.c b/sys/dev/pci/pci.c
index 8f9a5ef7a..f649b3e79 100644
--- a/sys/dev/pci/pci.c
+++ b/sys/dev/pci/pci.c
@@ -678,20 +678,22 @@ pci_get_ht_capability(pci_chipset_tag_t pc, pcitag_t tag, 
int capid,
 }
 
 int
-pcie_get_capability(pci_chipset_tag_t pc, pcitag_t tag, int capid,
+pci_get_ext_capability(pci_chipset_tag_t pc, pcitag_t tag, int capid,
 int *offset, pcireg_t *value)
 {
pcireg_t reg;
unsigned int ofs;
 
-   /* Make sure we support PCIExpress device */
+   /* Make sure we support PCI Express device */
if (pci_get_capability(pc, tag, PCI_CAP_PCIEXPRESS, NULL, NULL) == 0)
return (0);
-   /* Scan PCIExpress capabilities */
+   /* Scan PCI Express capabilities */
ofs = PCI_PCIE_ECAP;
while (ofs != 0) {
+#ifdef DIAGNOSTIC
if ((ofs & 3) || (ofs < PCI_PCIE_ECAP))
return (0);
+#endif
reg = pci_conf_read(pc, tag, ofs);
if (PCI_PCIE_ECAP_ID(reg) == capid) {
if (offset)
@@ -702,6 +704,7 @@ pcie_get_capability(pci_chipset_tag_t pc, pcitag_t tag, int 
capid,
}
ofs = PCI_PCIE_ECAP_NEXT(reg);
}
+
return (0);
 }
 
diff --git a/sys/dev/pci/pcivar.h b/sys/dev/pci/pcivar.h
index 0376ba992..1a19996f5 100644
--- a/sys/dev/pci/pcivar.h
+++ b/sys/dev/pci/pcivar.h
@@ -233,12 +233,12 @@ int   pci_io_find(pci_chipset_tag_t, pcitag_t, int, 
bus_addr_t *,
 intpci_mem_find(pci_chipset_tag_t, pcitag_t, int, bus_addr_t *,
bus_size_t *, int *);
 
-int

Re: acpiapplesmc(4)

2020-09-07 Thread Marcus Glocker
On Mon, 7 Sep 2020 19:25:00 +0200 (CEST)
Mark Kettenis  wrote:

> > Date: Mon, 7 Sep 2020 12:02:15 -0500
> > From: joshua stein 
> > 
> > On Mon, 07 Sep 2020 at 06:58:01 +0200, Marcus Glocker wrote:  
> > > This is an initial driver for the Apple System Management
> > > Controller found in Intel based Apple computers.
> > > 
> > > The driver is currently missing support for the Sudden Motion
> > > Sensor (SMS), light sensor, and keyboard backlight since I don't
> > > have that hardware available to develop on.
> > > 
> > > On my iMac11,2 it can deliver fan and temperatures values:
> > > 
> > >   hw.sensors.acpiapplesmc0.temp0=24.00 degC (Airflow 1)
> > >   hw.sensors.acpiapplesmc0.temp1=33.00 degC (CPU Core 0)
> > >   hw.sensors.acpiapplesmc0.temp2=36.00 degC (CPU Heatsink)
> > >   hw.sensors.acpiapplesmc0.temp3=40.00 degC (CPU Core 1)
> > >   hw.sensors.acpiapplesmc0.temp4=47.00 degC (GPU)
> > >   hw.sensors.acpiapplesmc0.temp5=45.00 degC (GPU Heatsink)
> > >   hw.sensors.acpiapplesmc0.temp6=59.00 degC (PCH)
> > >   hw.sensors.acpiapplesmc0.temp7=42.00 degC (Memory)
> > >   hw.sensors.acpiapplesmc0.temp8=45.00 degC (Mainboard
> > > Proximity) hw.sensors.acpiapplesmc0.fan0=998 RPM
> > >   hw.sensors.acpiapplesmc0.fan1=1132 RPM
> > >   hw.sensors.acpiapplesmc0.fan2=1198 RPM
> > > 
> > > Feedback, testers, OKs?  
> > 
> > Are there machines where asmc(4) will also attach?  
> 
> Good point.  My old Macmini1,1 has:
> 
> ...
> "APP0001" at acpi0 not configured
> ...
> asmc0 at isa0 port 0x300/32: rev 1.3f503, 137 keys
> ...
> 
> So yes, I'd say there are.
> 
> 
> Having an acpi attachment is probably better than doing isa probes.
> But we probably should consolidate the drivers.

D'oh!  I wasn't even aware that we already have an asmc(4) driver in our
tree.  Shame on me :-|

Glancing over asmc(4) I don't think there is anything more that my
driver would support other than attaching over acpi(4).  Would it be
possible to only write an acpi glue which attaches to asmc(4)?



Re: acpiapplesmc(4)

2020-09-07 Thread Mark Kettenis
> Date: Mon, 7 Sep 2020 12:02:15 -0500
> From: joshua stein 
> 
> On Mon, 07 Sep 2020 at 06:58:01 +0200, Marcus Glocker wrote:
> > This is an initial driver for the Apple System Management Controller
> > found in Intel based Apple computers.
> > 
> > The driver is currently missing support for the Sudden Motion Sensor
> > (SMS), light sensor, and keyboard backlight since I don't have that
> > hardware available to develop on.
> > 
> > On my iMac11,2 it can deliver fan and temperatures values:
> > 
> > hw.sensors.acpiapplesmc0.temp0=24.00 degC (Airflow 1)
> > hw.sensors.acpiapplesmc0.temp1=33.00 degC (CPU Core 0)
> > hw.sensors.acpiapplesmc0.temp2=36.00 degC (CPU Heatsink)
> > hw.sensors.acpiapplesmc0.temp3=40.00 degC (CPU Core 1)
> > hw.sensors.acpiapplesmc0.temp4=47.00 degC (GPU)
> > hw.sensors.acpiapplesmc0.temp5=45.00 degC (GPU Heatsink)
> > hw.sensors.acpiapplesmc0.temp6=59.00 degC (PCH)
> > hw.sensors.acpiapplesmc0.temp7=42.00 degC (Memory)
> > hw.sensors.acpiapplesmc0.temp8=45.00 degC (Mainboard Proximity)
> > hw.sensors.acpiapplesmc0.fan0=998 RPM
> > hw.sensors.acpiapplesmc0.fan1=1132 RPM
> > hw.sensors.acpiapplesmc0.fan2=1198 RPM
> > 
> > Feedback, testers, OKs?
> 
> Are there machines where asmc(4) will also attach?

Good point.  My old Macmini1,1 has:

...
"APP0001" at acpi0 not configured
...
asmc0 at isa0 port 0x300/32: rev 1.3f503, 137 keys
...

So yes, I'd say there are.


Having an acpi attachment is probably better than doing isa probes.
But we probably should consolidate the drivers.



Re: acpiapplesmc(4)

2020-09-07 Thread joshua stein
On Mon, 07 Sep 2020 at 06:58:01 +0200, Marcus Glocker wrote:
> This is an initial driver for the Apple System Management Controller
> found in Intel based Apple computers.
> 
> The driver is currently missing support for the Sudden Motion Sensor
> (SMS), light sensor, and keyboard backlight since I don't have that
> hardware available to develop on.
> 
> On my iMac11,2 it can deliver fan and temperatures values:
> 
>   hw.sensors.acpiapplesmc0.temp0=24.00 degC (Airflow 1)
>   hw.sensors.acpiapplesmc0.temp1=33.00 degC (CPU Core 0)
>   hw.sensors.acpiapplesmc0.temp2=36.00 degC (CPU Heatsink)
>   hw.sensors.acpiapplesmc0.temp3=40.00 degC (CPU Core 1)
>   hw.sensors.acpiapplesmc0.temp4=47.00 degC (GPU)
>   hw.sensors.acpiapplesmc0.temp5=45.00 degC (GPU Heatsink)
>   hw.sensors.acpiapplesmc0.temp6=59.00 degC (PCH)
>   hw.sensors.acpiapplesmc0.temp7=42.00 degC (Memory)
>   hw.sensors.acpiapplesmc0.temp8=45.00 degC (Mainboard Proximity)
>   hw.sensors.acpiapplesmc0.fan0=998 RPM
>   hw.sensors.acpiapplesmc0.fan1=1132 RPM
>   hw.sensors.acpiapplesmc0.fan2=1198 RPM
> 
> Feedback, testers, OKs?

Are there machines where asmc(4) will also attach?



Re: [WIP FAQ13] New section for webcam usage

2020-09-07 Thread Paco Esteban
Hi Stefan,

On Sun, 06 Sep 2020, Stefan Hagen wrote:

> Hello,
> 
> Lauries video(1) email to misc@ encouraged me to take this information
> and try to come up with a proposal to enhance the multimedia faq with
> information about webcam usage.
> 
> It's the first time I'm working on a faq article and I don't know about
> any style guides. I tried to align with the articles already written.
> Please educate me about the parts I missed.
> 
> I'm not sure where people working on documentation collaborate. So
> I'm trying my luck here on tech@.
> 
> My proposal is online and I'm also inlining it (not as patch for easier
> consumption): https://codevoid.de/h/wip_webcamfaq13.html
> 
> I'm sure some parts need refinement / better wording.
> 
> The part I'm mostly unsure about is how to correctly give the user
> permissions to the video devices. Most people probably do it the
> way I described (doas chmod $USER in .xsession).
> 
> Once it's good enough to get committed, I'll create a proper patch.
> 
> Any thoughts on it?

First, thanks for working on this.
I think it's a good idea, let's see what other devs say.

One quick comment on the "Using a webcam as user" section.  Besides
adding the chown line on .xsession you can use
/etc/X11/xenodm/GiveConsole too (not sure if this is "less preferred"
than your method).  I have this in mine:

if [ -c /dev/video0 ]; then
chown $USER /dev/video0
fi

Which should be accompanied with an entry on /etc/X11/xenodm/TakeConsole

if [ -c /dev/video0 ]; then
chown root /dev/video0
fi

In any case, the suggestion of a _video group seems cleaner to me, but
if it has not been done already I suspect there are good reasons for it
(did not dig into the archives, so I'm speculating here).

I'll take a deeper look later.

Cheers,
Paco.

> 
> Thanks in advance,
> Stefan
> 
> 
> 
> Using a Webcam
> 
> Supported Hardware
> 
> 
> Most webcams today work according to the USB Video Class
> (UVC) specification, which is supported by the  href="https://man.openbsd.org/uvideo;>uvideo(4) device driver and
> attaches to the https://man.openbsd.org/video.4;>video(4)
> device. The manpage lists some supported devices, but there is a
> good chance that other devices work as well. For example, webcams in
> Lenovo Thinkpads laptops do usually work.
> 
> 
> A supported webcam (or other video device) shows up in dmesg
> like this:
> 
> 
> uvideo0 at uhub0 port 8 configuration 1 interface 0 "Azurewave Integrated 
> Camera" rev 2.01/69.05 addr 10
> video0 at uvideo0
> uvideo1 at uhub0 port 8 configuration 1 interface 2 "Azurewave Integrated 
> Camera" rev 2.01/69.05 addr 10
> video1 at uvideo1
> 
> 
> 
> You see that an uvideo device was detected and has
> been attached to video0. This device will be accessible
> through /dev/video0.
> 
> 
> Modern laptops sometimes attach a second video device, which is the
> infrared camera for face recognition. The second camera does not produce
> a usable video stream.
> 
> 
> You can find the usable camera with the  href="https://man.openbsd.org/video;>video(1) command:
> 
> 
> $ video -q -f /dev/video0
> video device /dev/video0:
>   encodings: yuy2
>   frame sizes (width x height, in pixels) and rates (in frames per second):
> 320x180: 30
> 320x240: 30
> 352x288: 30
> 424x240: 30
> 640x360: 30
> 640x480: 30
> 848x480: 20
> 960x540: 15
> 1280x720: 10
>   controls: brightness, contrast, saturation, hue, gamma, sharpness, 
> white_balance_temperature
> 
> $ video -q -f /dev/video1
> video: /dev/video1 has no usable YUV encodings
> 
> 
> 
> The usable camera device shows supported resolutions and framerates.
> Note that the framerates only apply to the uncompressed YUY2 stream. More
> on that in Recording a webcam stream.
> 
> Using a webcam as user
> 
> 
> To use the webcam as regular user, you need to change the device
> permissions. Only root is allowed to access video devices by default.
> 
> 
> One way of allowing your user to access the video devices is to change
> the permissions from ~/.xsession. You can configure 
> https://man.openbsd.org/doas;>doas(1) to perform 
> https://man.openbsd.org/chmod;>chmod(1) as superuser
> without asking for a password for your user.
> 
> 
> Then add the following line to your ~/.xsession:
> 
> 
> doas chown $USER /dev/video0
> 
> 
> 
> If you're not using  href="https://man.openbsd.org/xenodm;>xenodm(1)
> and you are starting your X session with 
> https://man.openbsd.org/startx;>startx(1), you can 
> accomplish the same with 
> https://man.openbsd.org/fbtab;>fbtab(5).
> 
> 
> Example entry in /etc/fbtab:
> 
> 
> /dev/ttyC0 0600/dev/video0
> 
> 
> Recording a webcam stream
>  
> 
> This section uses ffplay and ffmpeg from
> graphics/ffmpeg. To find out what your camera is capable of, run the
> following command:
> 
> 
> $ ffplay -f v4l2 -list_formats all -i /dev/video0
> [...]
> [video4linux2,v4l2 @ 0x921f8420800] Compressed:   mjpeg