upd(4): force boolean indicator to be 0 or 1

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

upd(4): force boolean indicator to be 0 or 1

Boudewijn Dijkstra
>Synopsis: boolean indicators in sensorsd.conf(5) are too cumbersome
>Category: system
>Environment:
        System      : OpenBSD 6.6
        Details     : OpenBSD 6.6 (GENERIC.MP) #372: Sat Oct 12 10:56:27 MDT 2019
                         [hidden email]:/usr/src/sys/arch/amd64/compile/GENERIC.MP

        Architecture: OpenBSD.amd64
        Machine     : amd64
>Description:
        Some upd(4) devices use -1 for "On" and some use 1.  sysctl(8) and senso
rsd(8) hide this detail from the user, which makes it difficult to define low an
d high values in sensorsd.conf(5).

Also see https://marc.info/?l=openbsd-misc&m=144529176814155&w=2
>How-To-Repeat:
        (hardware-dependent)
>Fix:
        upd_sensor_update() should use only 0 and 1 for boolean indicators.

Index: sys/dev/usb/upd.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/upd.c,v
retrieving revision 1.26
diff -u -p -u -r1.26 upd.c
--- sys/dev/usb/upd.c 8 Apr 2017 02:57:25 -0000 1.26
+++ sys/dev/usb/upd.c 27 Feb 2020 15:47:23 -0000
@@ -406,26 +406,30 @@ upd_sensor_update(struct upd_softc *sc,
  struct upd_sensor *child;
  int64_t hdata, adjust;
 
- switch (HID_GET_USAGE(sensor->hitem.usage)) {
- case HUB_REL_STATEOF_CHARGE:
- case HUB_ABS_STATEOF_CHARGE:
- case HUB_REM_CAPACITY:
- case HUB_FULLCHARGE_CAPACITY:
- adjust = 1000; /* scale adjust */
- break;
- case HUB_ATRATE_TIMETOFULL:
- case HUB_ATRATE_TIMETOEMPTY:
- case HUB_RUNTIMETO_EMPTY:
- /* spec says minutes, not seconds */
- adjust = 1000000000LL;
- break;
- default:
- adjust = 1; /* no scale adjust */
- break;
+ hdata = hid_get_data(buf, len, &sensor->hitem.loc);
+ if (sensor->ksensor.type == SENSOR_INDICATOR)
+ sensor->ksensor.value = hdata ? 1 : 0;
+ else {
+ switch (HID_GET_USAGE(sensor->hitem.usage)) {
+ case HUB_REL_STATEOF_CHARGE:
+ case HUB_ABS_STATEOF_CHARGE:
+ case HUB_REM_CAPACITY:
+ case HUB_FULLCHARGE_CAPACITY:
+ adjust = 1000; /* scale adjust */
+ break;
+ case HUB_ATRATE_TIMETOFULL:
+ case HUB_ATRATE_TIMETOEMPTY:
+ case HUB_RUNTIMETO_EMPTY:
+ /* spec says minutes, not seconds */
+ adjust = 1000000000LL;
+ break;
+ default:
+ adjust = 1; /* no scale adjust */
+ break;
+ }
+ sensor->ksensor.value = hdata * adjust;
  }
 
- hdata = hid_get_data(buf, len, &sensor->hitem.loc);
- sensor->ksensor.value = hdata * adjust;
  sensor->ksensor.status = SENSOR_S_OK;
  sensor->ksensor.flags &= ~SENSOR_FINVALID;
 



dmesg:
OpenBSD 6.6 (GENERIC.MP) #372: Sat Oct 12 10:56:27 MDT 2019
    [hidden email]:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 4144320512 (3952MB)
avail mem = 4006014976 (3820MB)
User Kernel Config
UKC> disable amdgpu
236 amdgpu* disabled
UKC> boot
Unknown command, try help
UKC> exit
Continuing...
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 3.0 @ 0xea850 (53 entries)
bios0: vendor American Megatrends Inc. version "1.00" date 06/28/2017
bios0: Micro-Star International Co., Ltd MS-7B38
acpi0 at bios0: ACPI 6.0
acpi0: sleep states S0 S3 S4 S5
acpi0: tables DSDT FACP APIC FPDT FIDT SSDT CRAT CDIT SSDT MCFG HPET UEFI SSDT IVRS SSDT SSDT SSDT
acpi0: wakeup devices GPP1(S4) GPP2(S4) GPP3(S4) PTXH(S4) OBLW(S3) GPP4(S4) EHC1(S4) XHC0(S4) GFX0(S4) GFX1(S4) GFX2(S4) GFX3(S4) GFX4(S4)
acpitimer0 at acpi0: 3579545 Hz, 32 bits
acpimadt0 at acpi0 addr 0xfee00000: PC-AT compat
cpu0 at mainbus0: apid 16 (boot processor)
cpu0: AMD A10-9700 RADEON R7, 10 COMPUTE CORES 4C+6G, 3493.85 MHz, 15-65-01
cpu0: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,PCLMUL,MWAIT,SSSE3,FMA3,CX16,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,MASSE,3DNOWP,OSVW,IBS,XOP,SKINIT,WDT,FMA4,TCE,NODEID,TBM,CPCTR,DBKP,PERFTSC,MWAITX,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,XSAVEOPT
cpu0: 96KB 64b/line 3-way I-cache, 32KB 64b/line 8-way D-cache, 1MB 64b/line 16-way L2 cache
cpu0: ITLB 48 4KB entries fully associative, 24 4MB entries fully associative
cpu0: DTLB 64 4KB entries fully associative, 64 4MB entries fully associative
cpu0: smt 0, core 0, package 0
mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges
cpu0: apic clock running at 99MHz
cpu0: mwait min=64, max=64, IBE
cpu1 at mainbus0: apid 17 (application processor)
cpu1: AMD A10-9700 RADEON R7, 10 COMPUTE CORES 4C+6G, 3493.45 MHz, 15-65-01
cpu1: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,PCLMUL,MWAIT,SSSE3,FMA3,CX16,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,MASSE,3DNOWP,OSVW,IBS,XOP,SKINIT,WDT,FMA4,TCE,NODEID,TBM,CPCTR,DBKP,PERFTSC,MWAITX,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,XSAVEOPT
cpu1: 96KB 64b/line 3-way I-cache, 32KB 64b/line 8-way D-cache, 1MB 64b/line 16-way L2 cache
cpu1: ITLB 48 4KB entries fully associative, 24 4MB entries fully associative
cpu1: DTLB 64 4KB entries fully associative, 64 4MB entries fully associative
cpu1: smt 1, core 0, package 0
cpu2 at mainbus0: apid 18 (application processor)
cpu2: AMD A10-9700 RADEON R7, 10 COMPUTE CORES 4C+6G, 3493.46 MHz, 15-65-01
cpu2: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,PCLMUL,MWAIT,SSSE3,FMA3,CX16,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,MASSE,3DNOWP,OSVW,IBS,XOP,SKINIT,WDT,FMA4,TCE,NODEID,TBM,CPCTR,DBKP,PERFTSC,MWAITX,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,XSAVEOPT
cpu2: 96KB 64b/line 3-way I-cache, 32KB 64b/line 8-way D-cache, 1MB 64b/line 16-way L2 cache
cpu2: ITLB 48 4KB entries fully associative, 24 4MB entries fully associative
cpu2: DTLB 64 4KB entries fully associative, 64 4MB entries fully associative
cpu2: smt 0, core 1, package 0
cpu3 at mainbus0: apid 19 (application processor)
cpu3: AMD A10-9700 RADEON R7, 10 COMPUTE CORES 4C+6G, 3493.46 MHz, 15-65-01
cpu3: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,PCLMUL,MWAIT,SSSE3,FMA3,CX16,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,MASSE,3DNOWP,OSVW,IBS,XOP,SKINIT,WDT,FMA4,TCE,NODEID,TBM,CPCTR,DBKP,PERFTSC,MWAITX,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,XSAVEOPT
cpu3: 96KB 64b/line 3-way I-cache, 32KB 64b/line 8-way D-cache, 1MB 64b/line 16-way L2 cache
cpu3: ITLB 48 4KB entries fully associative, 24 4MB entries fully associative
cpu3: DTLB 64 4KB entries fully associative, 64 4MB entries fully associative
cpu3: smt 1, core 1, package 0
ioapic0 at mainbus0: apid 0 pa 0xfec00000, version 21, 24 pins
ioapic1 at mainbus0: apid 1 pa 0xfec01000, version 21, 32 pins
acpimcfg0 at acpi0
acpimcfg0: addr 0xf8000000, bus 0-63
acpihpet0 at acpi0: 14318180 Hz
acpiprt0 at acpi0: bus 0 (PCI0)
acpiprt1 at acpi0: bus -1 (GPP0)
acpiprt2 at acpi0: bus -1 (GPP1)
acpiprt3 at acpi0: bus -1 (GPP2)
acpiprt4 at acpi0: bus 5 (GPP3)
acpiprt5 at acpi0: bus -1 (GPP4)
acpiprt6 at acpi0: bus -1 (GFX0)
acpiprt7 at acpi0: bus -1 (GFX2)
acpiprt8 at acpi0: bus -1 (GFX3)
acpiprt9 at acpi0: bus -1 (GFX4)
acpicpu0 at acpi0: C2(0@400 io@0x1771), C1(@1 halt!), PSS
acpicpu1 at acpi0: C2(0@400 io@0x1771), C1(@1 halt!), PSS
acpicpu2 at acpi0: C2(0@400 io@0x1771), C1(@1 halt!), PSS
acpicpu3 at acpi0: C2(0@400 io@0x1771), C1(@1 halt!), PSS
acpipwrres0 at acpi0: P0U3, resource for XHC0
acpipwrres1 at acpi0: P3U3, resource for XHC0
acpipwrres2 at acpi0: P0U2, resource for EHC1
acpipwrres3 at acpi0: P3U2, resource for EHC1
acpipwrres4 at acpi0: P0SD, resource for SDIO
acpipwrres5 at acpi0: P3SD, resource for SDIO
acpipwrres6 at acpi0: P0ST, resource for SATA
acpipwrres7 at acpi0: P3ST, resource for SATA
acpipci0 at acpi0 PCI0: 0x00000010 0x00000011 0x00000000
acpicmos0 at acpi0
acpibtn0 at acpi0: PWRB
"PNP0C14" at acpi0 not configured
"AMDIF030" at acpi0 not configured
"PNP0C14" at acpi0 not configured
acpivideo0 at acpi0: VGA_
acpivideo1 at acpi0: VGA_
acpivideo2 at acpi0: VGA_
cpu0: 3493 MHz: speeds: 3500 3200 2900 2400 1900 1400 MHz
pci0 at mainbus0 bus 0
pchb0 at pci0 dev 0 function 0 "AMD AMD64 15h Root Complex" rev 0x00
"AMD AMD64 15h IOMMU" rev 0x00 at pci0 dev 0 function 2 not configured
vga1 at pci0 dev 1 function 0 "ATI Carrizo" rev 0xe2
wsdisplay0 at vga1 mux 1: console (80x25, vt100 emulation)
wsdisplay0: screen 1-5 added (80x25, vt100 emulation)
azalia0 at pci0 dev 1 function 1 "ATI Radeon HD Audio" rev 0x00: msi
azalia0: no supported codecs
pchb1 at pci0 dev 2 function 0 "AMD AMD64 15h Host" rev 0x00
ppb0 at pci0 dev 2 function 4 "AMD AMD64 15h PCIE" rev 0x00: msi
pci1 at ppb0 bus 5
xhci0 at pci1 dev 0 function 0 vendor "AMD", unknown product 0x43bc rev 0x02: msi, xHCI 1.10
usb0 at xhci0: USB revision 3.0
uhub0 at usb0 configuration 1 interface 0 "AMD xHCI root hub" rev 3.00/1.00 addr 1
ahci0 at pci1 dev 0 function 1 vendor "AMD", unknown product 0x43b8 rev 0x02: msi, AHCI 1.3.1
ahci0: port busy after first PMP probe FIS
ahci0: port busy after first PMP probe FIS
ahci0: port 0: 6.0Gb/s
ahci0: port busy after first PMP probe FIS
ahci0: port 5: 1.5Gb/s
scsibus1 at ahci0: 32 targets
sd0 at scsibus1 targ 0 lun 0: <ATA, WDC WDS120G2G0A-, UE45> naa.5001b448b9b26461
sd0: 114480MB, 512 bytes/sector, 234455040 sectors, thin
cd0 at scsibus1 targ 5 lun 0: <HL-DT-ST, DVDRAM GH24NSD1, LG00> removable
ppb1 at pci1 dev 0 function 2 vendor "AMD", unknown product 0x43b3 rev 0x02
pci2 at ppb1 bus 6
ppb2 at pci2 dev 4 function 0 "AMD 300 Series PCIE" rev 0x02: msi
pci3 at ppb2 bus 30
re0 at pci3 dev 0 function 0 "Realtek 8168" rev 0x15: RTL8168H/8111H (0x5400), msi, address 30:9c:23:26:02:e4
rgephy0 at re0 phy 7: RTL8251 PHY, rev. 0
ppb3 at pci2 dev 5 function 0 "AMD 300 Series PCIE" rev 0x02: msi
pci4 at ppb3 bus 31
ppb4 at pci2 dev 6 function 0 "AMD 300 Series PCIE" rev 0x02: msi
pci5 at ppb4 bus 32
ppb5 at pci2 dev 7 function 0 "AMD 300 Series PCIE" rev 0x02: msi
pci6 at ppb5 bus 33
pchb2 at pci0 dev 3 function 0 "AMD AMD64 15h Host" rev 0x00
"AMD AMD64 15h PSP 2.0" rev 0x00 at pci0 dev 8 function 0 not configured
pchb3 at pci0 dev 9 function 0 "AMD AMD64 15h Host" rev 0x00
azalia1 at pci0 dev 9 function 2 "AMD AMD64 15h HD Audio" rev 0x00: apic 1 int 22
azalia1: codecs: Realtek/0x0887
audio0 at azalia1
xhci1 at pci0 dev 16 function 0 "AMD FCH xHCI" rev 0x20: msi, xHCI 1.0
usb1 at xhci1: USB revision 3.0
uhub1 at usb1 configuration 1 interface 0 "AMD xHCI root hub" rev 3.00/1.00 addr 1
ahci1 at pci0 dev 17 function 0 "AMD FCH AHCI" rev 0x49: apic 0 int 19, AHCI 1.3
scsibus2 at ahci1: 32 targets
ehci0 at pci0 dev 18 function 0 "AMD FCH USB2" rev 0x49: apic 0 int 18
usb2 at ehci0: USB revision 2.0
uhub2 at usb2 configuration 1 interface 0 "AMD EHCI root hub" rev 2.00/1.00 addr 1
"AMD FCH SMBus" rev 0x4a at pci0 dev 20 function 0 not configured
pcib0 at pci0 dev 20 function 3 "AMD FCH LPC" rev 0x11
pchb4 at pci0 dev 24 function 0 "AMD AMD64 15h Link Cfg" rev 0x00
pchb5 at pci0 dev 24 function 1 "AMD AMD64 15h Address Map" rev 0x00
pchb6 at pci0 dev 24 function 2 "AMD AMD64 15h DRAM Cfg" rev 0x00
pchb7 at pci0 dev 24 function 3 "AMD AMD64 15h Misc Cfg" rev 0x00
pchb8 at pci0 dev 24 function 4 "AMD AMD64 15h CPU Power" rev 0x00
pchb9 at pci0 dev 24 function 5 "AMD AMD64 15h Misc Cfg" rev 0x00
isa0 at pcib0
isadma0 at isa0
com0 at isa0 port 0x3f8/8 irq 4: ns16550a, 16 byte fifo
com1 at isa0 port 0x2f8/8 irq 3: ns16550a, 16 byte fifo
pckbc0 at isa0 port 0x60/5 irq 1 irq 12
pckbd0 at pckbc0 (kbd slot)
wskbd0 at pckbd0: console keyboard, using wsdisplay0
pcppi0 at isa0 port 0x61
spkr0 at pcppi0
uhidev0 at uhub1 port 6 configuration 1 interface 0 "NOVATEK USB Keyboard" rev 1.10/1.04 addr 2
uhidev0: iclass 3/1
ukbd0 at uhidev0: 8 variable keys, 6 key codes
wskbd1 at ukbd0 mux 1
wskbd1: connecting to wsdisplay0
uhidev1 at uhub1 port 6 configuration 1 interface 1 "NOVATEK USB Keyboard" rev 1.10/1.04 addr 2
uhidev1: iclass 3/0, 2 report ids
uhid0 at uhidev1 reportid 1: input=1, output=0, feature=0
uhid1 at uhidev1 reportid 2: input=3, output=0, feature=0
uhub3 at uhub2 port 1 configuration 1 interface 0 "Advanced Micro Devices product 0x7900" rev 2.00/0.18 addr 2
vscsi0 at root
scsibus3 at vscsi0: 256 targets
softraid0 at root
scsibus4 at softraid0: 256 targets
root on sd0a (383552eab63dd034.a) swap on sd0b dump on sd0b

usbdevs:
Controller /dev/usb0:
addr 01: 1022:0000 AMD, xHCI root hub
         super speed, self powered, config 1, rev 1.00
         driver: uhub0
Controller /dev/usb1:
addr 01: 1022:0000 AMD, xHCI root hub
         super speed, self powered, config 1, rev 1.00
         driver: uhub1
addr 02: 051d:0003 American Power Conversion, Smart-UPS 1000 FW:UPS 09.2 / ID=18
         full speed, power 2 mA, config 1, rev 1.06, iSerial AS1504122090
         driver: uhidev0
Controller /dev/usb2:
addr 01: 1022:0000 AMD, EHCI root hub
         high speed, self powered, config 1, rev 1.00
         driver: uhub2
addr 02: 0438:7900 Advanced Micro Devices, product 0x7900
         high speed, self powered, config 1, rev 0.18
         driver: uhub3

Reply | Threaded
Open this post in threaded view
|

Re: upd(4): force boolean indicator to be 0 or 1

Martin Pieuchot
On 27/02/20(Thu) 16:58, [hidden email] wrote:

> >Synopsis: boolean indicators in sensorsd.conf(5) are too cumbersome
> >Category: system
> >Environment:
> System      : OpenBSD 6.6
> Details     : OpenBSD 6.6 (GENERIC.MP) #372: Sat Oct 12 10:56:27 MDT 2019
> [hidden email]:/usr/src/sys/arch/amd64/compile/GENERIC.MP
>
> Architecture: OpenBSD.amd64
> Machine     : amd64
> >Description:
> Some upd(4) devices use -1 for "On" and some use 1.  sysctl(8) and senso
> rsd(8) hide this detail from the user, which makes it difficult to define low an
> d high values in sensorsd.conf(5).

Which device reports "-1" for which usage?  Is this from any
specification or is it a workaround for your device?

Diff looks fine, although we could do simpler, see below.

Index: upd.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/upd.c,v
retrieving revision 1.26
diff -u -p -r1.26 upd.c
--- upd.c 8 Apr 2017 02:57:25 -0000 1.26
+++ upd.c 27 Feb 2020 16:25:24 -0000
@@ -425,7 +425,10 @@ upd_sensor_update(struct upd_softc *sc,
  }
 
  hdata = hid_get_data(buf, len, &sensor->hitem.loc);
- sensor->ksensor.value = hdata * adjust;
+ if (sensor->ksensor.type == SENSOR_INDICATOR)
+ sensor->ksensor.value = hdata ? 1 : 0;
+ else
+ sensor->ksensor.value = hdata * adjust;
  sensor->ksensor.status = SENSOR_S_OK;
  sensor->ksensor.flags &= ~SENSOR_FINVALID;
 

Reply | Threaded
Open this post in threaded view
|

Re: upd(4): force boolean indicator to be 0 or 1

Boudewijn Dijkstra
Op Thu, 27 Feb 2020 17:30:34 +0100 schreef Martin Pieuchot  
<[hidden email]>:

> On 27/02/20(Thu) 16:58, [hidden email] wrote:
>> >Synopsis: boolean indicators in sensorsd.conf(5) are too cumbersome
>> >Category: system
>> >Environment:
>> System      : OpenBSD 6.6
>> Details     : OpenBSD 6.6 (GENERIC.MP) #372: Sat Oct 12 10:56:27 MDT  
>> 2019  
>> [hidden email]:/usr/src/sys/arch/amd64/compile/GENERIC.MP
>>
>> Architecture: OpenBSD.amd64
>> Machine     : amd64
>> >Description:
>> Some upd(4) devices use -1 for "On" and some use 1.  sysctl(8) and  
>> sensorsd(8) hide this detail from the user, which makes it difficult to  
>> define low and high values in sensorsd.conf(5).
>
> Which device reports "-1" for which usage?  Is this from any
> specification or is it a workaround for your device?

In the misc@ thread I linked it was reported that different devices use  
different values.  My device happens to report -1 for "On".  Given how  
sensorsd.conf currently works, it would be most convenient if 0 and 1 were  
the only possible values.

> Diff looks fine, although we could do simpler, see below.
>
> Index: upd.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/upd.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 upd.c
> --- upd.c 8 Apr 2017 02:57:25 -0000 1.26
> +++ upd.c 27 Feb 2020 16:25:24 -0000
> @@ -425,7 +425,10 @@ upd_sensor_update(struct upd_softc *sc,
>   }
> hdata = hid_get_data(buf, len, &sensor->hitem.loc);
> - sensor->ksensor.value = hdata * adjust;
> + if (sensor->ksensor.type == SENSOR_INDICATOR)
> + sensor->ksensor.value = hdata ? 1 : 0;
> + else
> + sensor->ksensor.value = hdata * adjust;
>   sensor->ksensor.status = SENSOR_S_OK;
>   sensor->ksensor.flags &= ~SENSOR_FINVALID;

Your diff is indeed simpler, but I thought it would be cleaner to not  
assign 'adjust' when it's not needed.


--
Boudewijn Dijkstra
Indes-IDS B.V.
+31 345 545 535

Reply | Threaded
Open this post in threaded view
|

Re: upd(4): force boolean indicator to be 0 or 1

Martin Pieuchot
On 28/02/20(Fri) 10:02, Boudewijn Dijkstra wrote:

> Op Thu, 27 Feb 2020 17:30:34 +0100 schreef Martin Pieuchot
> <[hidden email]>:
> > On 27/02/20(Thu) 16:58, [hidden email] wrote:
> > > >Synopsis: boolean indicators in sensorsd.conf(5) are too cumbersome
> > > >Category: system
> > > >Environment:
> > > System      : OpenBSD 6.6
> > > Details     : OpenBSD 6.6 (GENERIC.MP) #372: Sat Oct 12 10:56:27
> > > MDT 2019
> > > [hidden email]:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> > >
> > > Architecture: OpenBSD.amd64
> > > Machine     : amd64
> > > >Description:
> > > Some upd(4) devices use -1 for "On" and some use 1.  sysctl(8) and
> > > sensorsd(8) hide this detail from the user, which makes it difficult
> > > to define low and high values in sensorsd.conf(5).
> >
> > Which device reports "-1" for which usage?  Is this from any
> > specification or is it a workaround for your device?
>
> In the misc@ thread I linked it was reported that different devices use
> different values.  My device happens to report -1 for "On".  Given how
> sensorsd.conf currently works, it would be most convenient if 0 and 1 were
> the only possible values.

You're rephrasing your diff in words.  My question is: can there be any
drawback to this approach?  Did you check the spec?  Why is your UPS
returning -1 and not 1 in this case?  Is this the right place to fix the
bug?

> > Diff looks fine, although we could do simpler, see below.
> >
> > Index: upd.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/usb/upd.c,v
> > retrieving revision 1.26
> > diff -u -p -r1.26 upd.c
> > --- upd.c 8 Apr 2017 02:57:25 -0000 1.26
> > +++ upd.c 27 Feb 2020 16:25:24 -0000
> > @@ -425,7 +425,10 @@ upd_sensor_update(struct upd_softc *sc,
> >   }
> > hdata = hid_get_data(buf, len, &sensor->hitem.loc);
> > - sensor->ksensor.value = hdata * adjust;
> > + if (sensor->ksensor.type == SENSOR_INDICATOR)
> > + sensor->ksensor.value = hdata ? 1 : 0;
> > + else
> > + sensor->ksensor.value = hdata * adjust;
> >   sensor->ksensor.status = SENSOR_S_OK;
> >   sensor->ksensor.flags &= ~SENSOR_FINVALID;
>
> Your diff is indeed simpler, but I thought it would be cleaner to not assign
> 'adjust' when it's not needed.

That's an improvement indeed, but it isn't related to the bug you're
trying to fix ;)

Reply | Threaded
Open this post in threaded view
|

Re: upd(4): force boolean indicator to be 0 or 1

Boudewijn Dijkstra
Op Fri, 28 Feb 2020 11:14:43 +0100 schreef Martin Pieuchot  
<[hidden email]>:

> On 28/02/20(Fri) 10:02, Boudewijn Dijkstra wrote:
>> Op Thu, 27 Feb 2020 17:30:34 +0100 schreef Martin Pieuchot
>> <[hidden email]>:
>> > On 27/02/20(Thu) 16:58, [hidden email] wrote:
>> > > >Synopsis: boolean indicators in sensorsd.conf(5) are too cumbersome
>> > > >Category: system
>> > > >Environment:
>> > > System      : OpenBSD 6.6
>> > > Details     : OpenBSD 6.6 (GENERIC.MP) #372: Sat Oct 12 10:56:27
>> > > MDT 2019  
>> [hidden email]:/usr/src/sys/arch/amd64/compile/GENERIC.MP
>> > >
>> > > Architecture: OpenBSD.amd64
>> > > Machine     : amd64
>> > > >Description:
>> > > Some upd(4) devices use -1 for "On" and some use 1.  sysctl(8) and
>> > > sensorsd(8) hide this detail from the user, which makes it difficult
>> > > to define low and high values in sensorsd.conf(5).
>> >
>> > Which device reports "-1" for which usage?  Is this from any
>> > specification or is it a workaround for your device?
>>
>> In the misc@ thread I linked it was reported that different devices use
>> different values.  My device happens to report -1 for "On".  Given how
>> sensorsd.conf currently works, it would be most convenient if 0 and 1  
>> were the only possible values.
>
> You're rephrasing your diff in words.  My question is: can there be any
> drawback to this approach?

They're boolean indicators, I don't think there can be any.  sensorsd(8)  
is the only program in base that can use upd(4) sensors.  sysctl(8)  
already treats them as booleans.  Obviously some people will have to  
change their sensorsd.conf(5) if this goes in.

> Did you check the spec?

I checked "Universal Serial Bus Usage Tables for HID Power Devices"
https://www.usb.org/sites/default/files/documents/pdcv10.pdf
For every boolean indicator it specifies two allowed values: 0 and 1.

> Why is your UPS returning -1 and not 1 in this case?

No idea.  It's working fine.  Some further testing revealed that On==-1  
(and Off==0) for all the indicators that I can easily toggle (Charging,  
Discharging, ACPresent).

> Is this the right place to fix the bug?

I think so. It's a violation of HID Power, not generic HID, so it should  
be fixed in a place where HID Power data is (first) interpreted.

>> > Diff looks fine, although we could do simpler, see below.
>> >
>> > Index: upd.c
>> > ===================================================================
>> > RCS file: /cvs/src/sys/dev/usb/upd.c,v
>> > retrieving revision 1.26
>> > diff -u -p -r1.26 upd.c
>> > --- upd.c 8 Apr 2017 02:57:25 -0000 1.26
>> > +++ upd.c 27 Feb 2020 16:25:24 -0000
>> > @@ -425,7 +425,10 @@ upd_sensor_update(struct upd_softc *sc,
>> >   }
>> > hdata = hid_get_data(buf, len, &sensor->hitem.loc);
>> > - sensor->ksensor.value = hdata * adjust;
>> > + if (sensor->ksensor.type == SENSOR_INDICATOR)
>> > + sensor->ksensor.value = hdata ? 1 : 0;
>> > + else
>> > + sensor->ksensor.value = hdata * adjust;
>> >   sensor->ksensor.status = SENSOR_S_OK;
>> >   sensor->ksensor.flags &= ~SENSOR_FINVALID;
>>
>> Your diff is indeed simpler, but I thought it would be cleaner to not  
>> assign 'adjust' when it's not needed.
>
> That's an improvement indeed, but it isn't related to the bug you're
> trying to fix ;)



--
Boudewijn Dijkstra
Indes-IDS B.V.
+31 345 545 535

Reply | Threaded
Open this post in threaded view
|

Re: upd(4): force boolean indicator to be 0 or 1

Martin Pieuchot
On 28/02/20(Fri) 12:34, Boudewijn Dijkstra wrote:

> Op Fri, 28 Feb 2020 11:14:43 +0100 schreef Martin Pieuchot
> <[hidden email]>:
> > On 28/02/20(Fri) 10:02, Boudewijn Dijkstra wrote:
> > > Op Thu, 27 Feb 2020 17:30:34 +0100 schreef Martin Pieuchot
> > > <[hidden email]>:
> > > > On 27/02/20(Thu) 16:58, [hidden email] wrote:
> > > > > >Synopsis: boolean indicators in sensorsd.conf(5) are too cumbersome
> > > > > >Category: system
> > > > > >Environment:
> > > > > System      : OpenBSD 6.6
> > > > > Details     : OpenBSD 6.6 (GENERIC.MP) #372: Sat Oct 12 10:56:27
> > > > > MDT 2019
> > > [hidden email]:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> > > > >
> > > > > Architecture: OpenBSD.amd64
> > > > > Machine     : amd64
> > > > > >Description:
> > > > > Some upd(4) devices use -1 for "On" and some use 1.  sysctl(8) and
> > > > > sensorsd(8) hide this detail from the user, which makes it difficult
> > > > > to define low and high values in sensorsd.conf(5).
> > > >
> > > > Which device reports "-1" for which usage?  Is this from any
> > > > specification or is it a workaround for your device?
> > >
> > > In the misc@ thread I linked it was reported that different devices use
> > > different values.  My device happens to report -1 for "On".  Given how
> > > sensorsd.conf currently works, it would be most convenient if 0 and
> > > 1 were the only possible values.
> >
> > You're rephrasing your diff in words.  My question is: can there be any
> > drawback to this approach?
>
> They're boolean indicators, I don't think there can be any.  sensorsd(8) is
> the only program in base that can use upd(4) sensors.  sysctl(8) already
> treats them as booleans.  Obviously some people will have to change their
> sensorsd.conf(5) if this goes in.
>
> > Did you check the spec?
>
> I checked "Universal Serial Bus Usage Tables for HID Power Devices"
> https://www.usb.org/sites/default/files/documents/pdcv10.pdf
> For every boolean indicator it specifies two allowed values: 0 and 1.
>
> > Why is your UPS returning -1 and not 1 in this case?
>
> No idea.  It's working fine.  Some further testing revealed that On==-1 (and
> Off==0) for all the indicators that I can easily toggle (Charging,
> Discharging, ACPresent).
>
> > Is this the right place to fix the bug?
>
> I think so. It's a violation of HID Power, not generic HID, so it should be
> fixed in a place where HID Power data is (first) interpreted.

Thanks for checking.  I committed the simpler diff.  If you have any
other improvement for upd(4) or any other part of the system you're
welcome to submit them to tech@.

Thanks again,
Martin

Reply | Threaded
Open this post in threaded view
|

Re: upd(4): force boolean indicator to be 0 or 1

Theo de Raadt-2
In reply to this post by Martin Pieuchot
Martin Pieuchot <[hidden email]> wrote:

> On 28/02/20(Fri) 10:02, Boudewijn Dijkstra wrote:
> > Op Thu, 27 Feb 2020 17:30:34 +0100 schreef Martin Pieuchot
> > <[hidden email]>:
> > > On 27/02/20(Thu) 16:58, [hidden email] wrote:
> > > > >Synopsis: boolean indicators in sensorsd.conf(5) are too cumbersome
> > > > >Category: system
> > > > >Environment:
> > > > System      : OpenBSD 6.6
> > > > Details     : OpenBSD 6.6 (GENERIC.MP) #372: Sat Oct 12 10:56:27
> > > > MDT 2019
> > > > [hidden email]:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> > > >
> > > > Architecture: OpenBSD.amd64
> > > > Machine     : amd64
> > > > >Description:
> > > > Some upd(4) devices use -1 for "On" and some use 1.  sysctl(8) and
> > > > sensorsd(8) hide this detail from the user, which makes it difficult
> > > > to define low and high values in sensorsd.conf(5).
> > >
> > > Which device reports "-1" for which usage?  Is this from any
> > > specification or is it a workaround for your device?
> >
> > In the misc@ thread I linked it was reported that different devices use
> > different values.  My device happens to report -1 for "On".  Given how
> > sensorsd.conf currently works, it would be most convenient if 0 and 1 were
> > the only possible values.
>
> You're rephrasing your diff in words.  My question is: can there be any
> drawback to this approach?  Did you check the spec?  Why is your UPS
> returning -1 and not 1 in this case?  Is this the right place to fix the
> bug?

I don't understand this line of questioning.

He observes a -1 value, and the code can trivially deal with it.

If the spec says it should return 0 or 1 only, what are you going to do
not cope with the value, and tell him to warantee it?

How about believing he's seeing non-0 delivered as -1.