Re: uhidpp(4): add support for Bolt receivers and Unified Battery feature
On Sat, Nov 26, 2022 at 1:47 AM Anton Lindqvist wrote:
> Hi,
> This diff adds supports for the following to uhidpp:
>
> 1. Bolt receivers, they use a different set of registers for querying
>paired devices.
>
> 2. The Unified Battery feature, this is a more competent feature
>function used to report battery status compared to the Battery
>feature which is already supported by uhidpp.
>
> Makes battery sensors appear for my Logitech Lift mouse and Paul de
> Weerd's MX Anywhere 3 mouse.
>
> If you have a Logitech HID++ device currently lacking sensors under
> sysctl hw.sensors, please give this diff a try. Send me your dmesg and
> output of sysctl hw.sensors after moving the mouse to ensure a connect
> interrupt has fired off. Note that the diff must be applied on top of
> revision 1.37 of sys/dev/usb/uhidpp.c.
>
> Comments? OK?
>
>
> @@ -193,17 +202,20 @@ struct uhidpp_device {
> uint8_t d_features;
> #define UHIDPP_DEVICE_FEATURE_ROOT 0x01
> #define UHIDPP_DEVICE_FEATURE_BATTERY 0x02
> +#define UHIDPP_DEVICE_FEATURE_UNIFIIED_BATTERY 0x04
>
>
>
Do you actually mean "unifiied" with an extra "i" here?
--david
Re: cron(8): add '@' interval time specifier
On Thu, Jul 15, 2021 at 11:20 AM Leo Unglaub wrote: > Hey, > this is a very clean solution to a very common problem that i have. A > lot of tasks that i run from cron have very different completion times. > Right now i use the -s [1] option in crontab to make sure only one task > is running at once and so that they don't overlap if they take longer to > complete than the interval i schedule them. But the problem with the -s > workflow is that the interval can get messed up if the interval is > smaller than the execution period. This will basically "skip" a run. > > With your patch this can be avioded in a very clean way. I have your > patch a try and for my local testing usecase it worked as expected. This > would clean up a lot of things in my personal workflow. Thank you very > mich for the patch Job, much appreciated! > > Thanks and greetings > Leo > > [1] https://man.openbsd.org/crontab.5#s > > On 10/07/2021 16:07, Job Snijders wrote: > > The below patch adds a new kind of time specifier: an interval (in > > minutes). When used, cron(8) will schedule the next instance of a job > > after the previous job has completed and a full interval has passed. > > > > A crontab(5) configured as following: > > > > $ crontab -l > > @3 sleep 100 > > > > Will result in this schedule: > > > > Jul 10 13:38:17 vurt cron[96937]: (CRON) STARTUP (V5.0) > > Jul 10 13:42:01 vurt cron[79385]: (job) CMD (sleep 100) > > Jul 10 13:47:01 vurt cron[3165]: (job) CMD (sleep 100) > > Jul 10 13:52:01 vurt cron[40539]: (job) CMD (sleep 100) > > Jul 10 13:57:01 vurt cron[84504]: (job) CMD (sleep 100) > > > > A use case could be running rpki-client more frequently than once an > > hour: > > > > @15 -n rpki-client && bgpctl reload > > > > The above is equivalent to: > > > > * * * * * -sn sleep 900 && rpki-client && bgpctl reload > > > > I borrowed the idea from FreeBSD's cron [1]. A difference between the > > below changeset and the freebsd implementation is that they specify the > > interval in seconds, while the below specifies in minutes. I was able > > to leverage the 'singleton' infrastructure. And removed a comment that > > reads like a TODO nobody is going to do. > > > > Thoughts? > Can't this type of problem also be solved with a shell script wrapper? An infinite loop that does work and then sleeps? Or perhaps a script that upon work completion, schedules itself in the future with at(1)? These could be kicked off initially with a @reboot crontab entry. Or maybe I'm missing something that cron adds to the equation, in which case, apologies. --david
Re: radeondrm(4) unaligned access fix
On Mon, Oct 26, 2020 at 3:18 PM Mark Kettenis wrote: > > Date: Sun, 25 Oct 2020 10:42:38 +0100 (CET) > > From: Mark Kettenis > > > > While making radeondrm(4) work on powerpc64 I'm running into an > > interesting unaligned access issue. > > > > Modern POWER CPUs generally support unaligned access. Normal load and > > store unstructions work fine with addresses that aren't naturally > > aligned when operating on cached memory. As a result, clang will > > optimize code by replacing two 32-bit store instructions with a single > > 64-bit store instruction even if there is only 32-bit alignment. > > > > However, this doesn't work for memory that is mapped uncachable. And > > there is some code in radeondrm(4) (and also in amdgpu(4)) that > > generates alignment exceptions because it is writing to bits of video > > memory that are mapped through the graphics aperture. > > > > There are two ways to fix this. The compiler won't apply this > > optimization if memory is accessed through pointers that are marked > > volatile. Hence the fix below. In my opinion that is the right fix > > as rdev->uvd.cpu_addr is a volatile pointer and that aspect shouldn't > > be dropped. The downside of this approach is that we may need to > > maintain some additional local fixes. > > > > The alternative is to emulate the access in the kernel. I fear that > > is what Linux does, which is why they don't notice this issue. As > > such, this issue may crop up in more places and the emulation would > > catch them all. But I'm a bit reluctant to add this emulation since > > it may hide bugs in other parts of our kernel. > > > > Thoughts? ok? > > There is more code in radeondrm(4) and amdgpu(4) that is affected by > this issues and some of it isn't easy to "volatilize". > > There is an llvm option to enforce strict alignment, but it isn't > exposed as a proper option by clang. I'm still investigating the use > of that option, but meanwhile I think I'll commit the attached diff > such that the kernel side of things works and I can look at what needs > to happen on the userland side. > Disclaimer: I'm not an expert in this area. Seems odd that this problem doesn't affect loads too... Would it be difficult to also validate that the destination address is mapped uncached, so that normal userland alignment bugs will still get caught? --david
Re: shrinking and growing reallocs: a theoretical? bad case for performance
On Mon, Aug 31, 2020 at 2:41 AM Otto Moerbeek wrote: > Hi, > > A question from Theo made me think about realloc and come up with a > particular bad case for performance. I do not know if it happens in > practice, but it was easy to create a test program to hit the case. > > We're talking allocation >= a page here. Smaller allocation follow > different rules. > > If an allocation is grown by realloc, I first tries to extend the > allocation by mapping pages next to the existing allocation. Since > the location of pages is randomized, chanches are high that next to an > allocation there are unmapped pages so the grow will work out. > > If realloc needs to shrink the allocation it puts the high pages no > longer needed in the malloc cache. There they can be re-used by other > allocations. But if that happens, next a grow of first allocation will > fail: the pages are already mapped. So realloc needs to do a new > allocation followed by a copy and a cleanup of the original. > > So this strategy of a shrinking realloc to of put unneeded pages into > the cache can work against us, plus it has the consequence that use of > realloc leads to allocations close to each other: no free guard pages. > If I am interpreting this correctly, realloc could be used to groom/shape the heap such that future allocations are less random and more predictable? --david
Re: symmetric toeplitz hashing
On Fri, Jun 12, 2020 at 9:41 AM Theo Buehler wrote:
> I finally found the time to think about the mathematics of this some
> more and I'm now convinced that it's a sound construction. I hope that
> one or the other observation below will be useful for you.
>
> The hash as it is now can be proved to produce values in the full range
> of uint16_t, so that's good.
>
> As we discussed already, you can simplify the construction further.
>
[snip]
> Next, I don't particularly like this magic STOEPLITZ_KEYSEED number, but
> I guess we can live with it.
>
> Another option would be to generate the key seed randomly. You will get
> a "good" hash that spreads out over all 16 bit values if and only if the
> random value has an odd number of binary digits.
>
> I haven't thought hard about this, but I don't immediately see a better
> way of generating such numbers than:
>
> int
> stoeplitz_good_seed(uint16_t seed)
> {
> int ones = 0;
>
> while (seed > 0) {
> ones += seed % 2;
> seed /= 2;
> }
>
> return ones % 2;
> }
>
> uint16_t
> stoeplitz_seed_init(void)
> {
> uint16_tseed;
>
> do {
> seed = arc4random() & UINT16_MAX;
> } while (!stoeplitz_good_seed(seed));
>
> return seed;
> }
>
> This will loop as long as it needs to get a good toeplitz key seed.
> Each time there is a 50% chance that it will find one, so this will
> need to loop n times with probability 1 / 2**n. This is basically the
> same situation as for arc4random_uniform() with an upper bound that is
> not a power of two.
>
I neither read nor understand the math but assuming you've described it
correctly, you can do this more simply by realizing that a random bad seed
can be made into a good seed by toggling one (random) bit. You also
replace stoeplitz_good_seed() with __builtin_parity() and perhaps leverage
some intrinsics?
uint16_t
stoeplitz_seed_init(void) {
uint16_t seed;
seed = arc4random() & UINT16_MAX;
if (!stoeplitz_good_seed(seed))
seed ^= 1 << (arc4random() % 16);
}
--david
Re: bgpd, Adj-RIB-Out support
On Wed, Oct 31, 2018 at 6:58 PM Sebastian Benoit wrote: On a phone, saw some typos and such, sorry no diff. [benoit@border2:~]$ cat before > RDE memory statistics > 715727 IPv4 unicast network entries using 27.3M of memory > 58953 IPv6 unicast network entries using 3.1M of memory >1549171 rib entries using 94.6M of memory >2897130 prefix entries using 265M of memory > 562423 BGP path attribute entries using 60.1M of memory > 149579 BGP AS-PATH attribute entries using 6.1M of memory, >and holding 562423 references > 37007 BGP attributes entries using 1.4M of memory >and holding 880668 references Inconsistent comma usage above > 37006 BGP attributes using 912K of memory > 61964 as-set elements in 58064 tables using 950K of memory > 103150 prefix-set elments using 4.3M of memory elments => elements RIB using 459M of memory > Sets using 5.2M of memory > > RDE hash statistics > path hash: size 131072, 562423 entires > min 0 max 20 avg/std-dev = 4.291/2.364 > aspath hash: size 131072, 149579 entires > min 0 max 8 avg/std-dev = 1.141/0.835 > attr hash: size 16384, 37007 entires > min 0 max 10 avg/std-dev = 2.259/1.378 > entires => entries —david
Re: bsd.rd failure in VirtualBox
On Sun, Sep 16, 2018 at 1:15 PM, Johan Huldtgren wrote: > On 2018/09/16 10:52, David Higgs wrote: >> On Sun, Sep 16, 2018 at 10:17 AM, David Higgs wrote: >>> On Sat, Sep 15, 2018 at 10:05 PM, Philip Guenther >>> wrote: >>>> On Sat, Sep 15, 2018 at 11:59 AM David Higgs wrote: >>>>> >>>>> I often use VirtualBox (version 5.2.18 on OS X) to familiarize myself >>>>> with new features in snapshots, before upgrading my physical hardware. >>>>> >>>>> This afternoon, I tried updating bsd.rd (amd64, 6.4-beta RAMDISK_CD >>>>> #281) and wasn't able to successfully boot it. I had to rely on the >>>>> video capture ability of VirtualBox to even notice there was a panic >>>>> (typed out below) before it rebooted to the "BIOS" splash screen. >>>> >>>> ... >>>>> >>>>> Also attached is the dmesg from a prior working snapshot. I haven't >>>>> tried updating since this prior snapshot, so I don't have further >>>>> insight into when the issue first appeared. >>>> >>>> >>>> Thank you for the complete and clear report! >>>> >>>> I have a diff in the amd64 snapshots to use the CPU's PCID support in many >>>> cases and this VirtualBox setup found a bug in it. I've generated a new >>>> diff that should fix this, so a future snap should fix this, though when >>>> that'll happend depends on the snap builder's schedule. >>>> >>> >>> Not sure if the fix made it into RAMDISK_CD #282, but this panic is >>> slightly different. I haven't tried reproducing to see if the panic >>> message differs between boots. >>> >>> >>> root on rd0a swap on rd0b dump on rd0b >>> uvm_fault(0xff011f73ac60, 0x208, 0, 1) -> e >>> fatal page fault in supervisor mode >>> trap type 6 code 0 rip 8135510b cs 8 rflags 10246 cr2 208 cpl >>> 0 rsp 800022026c90 >>> gsbase 0x81870ff0 kgsbase 0x0 >>> panic: trap type 6, code=0, pc=8135510b >>> syncing disk... done >>> >>> dump to dev 17,1 not possible >>> rebooting... >>> >>> >>> Hope this helps. >>> >> >> FWIW, the vbox capture feature is pretty buggy - it doesn't create the >> file when it says it is recording, and it frequently crashes. It is >> possible the panic above is from #281 instead, because I deleted the >> video before I realizing this. >> >> Below is definitely from #282. >> >> >> Welcome to the OpenBSD/amd64 6.4 installation program. >> fatal protection fault in supervisor mode >> trap type 4 code 0 rip 810f4244 cs 8 rflags 10286 cr2 6c1fed >> cpl a rsp 800022098800 >> gsbase 0x81870ff0 kgsbase 0x0 >> panic: trap type 4, code 0, pc=0x 810f4244 >> syncing disks... done >> >> dump to dev 17,1 not possible >> rebooting... >> >> >> Hope this is actually useful and not another stupid VirtualBox bug. > > I see this an almost identical panic on real hardware too, the only difference > being the string after 'rsp' > > Welcome to the OpenBSD/amd64 6.4 installation program. > fatal protection fault in supervisor mode > trap type 4 code 0 rip 810f4244 cs 8 rflags 10286 cr2 6c1fed cpl a > rsp 8000220ba9e0 > gsbase 0x81870ff0 kgsbase 0x0 > panic: trap type 4, code 0, pc=810f4244 > syncing disks... done > > dump to dev 17,1 not possible > rebooting... > > Below is first the working dmesg snapshot, and then one from booting bsd.rd, > note > the ACPI error about not being able to load tables, that's not there on the > working > snap. That might be the culprit at least in my case? > FYI - successfully booted with RAMDISK_CD #283 and installed a successfully-booting GENERIC #284 within VirtualBox. Thanks again. --david
Re: bsd.rd failure in VirtualBox
On Sat, Sep 15, 2018 at 10:05 PM, Philip Guenther wrote: > On Sat, Sep 15, 2018 at 11:59 AM David Higgs wrote: >> >> I often use VirtualBox (version 5.2.18 on OS X) to familiarize myself >> with new features in snapshots, before upgrading my physical hardware. >> >> This afternoon, I tried updating bsd.rd (amd64, 6.4-beta RAMDISK_CD >> #281) and wasn't able to successfully boot it. I had to rely on the >> video capture ability of VirtualBox to even notice there was a panic >> (typed out below) before it rebooted to the "BIOS" splash screen. > > ... >> >> Also attached is the dmesg from a prior working snapshot. I haven't >> tried updating since this prior snapshot, so I don't have further >> insight into when the issue first appeared. > > > Thank you for the complete and clear report! > > I have a diff in the amd64 snapshots to use the CPU's PCID support in many > cases and this VirtualBox setup found a bug in it. I've generated a new > diff that should fix this, so a future snap should fix this, though when > that'll happend depends on the snap builder's schedule. > Not sure if the fix made it into RAMDISK_CD #282, but this panic is slightly different. I haven't tried reproducing to see if the panic message differs between boots. root on rd0a swap on rd0b dump on rd0b uvm_fault(0xff011f73ac60, 0x208, 0, 1) -> e fatal page fault in supervisor mode trap type 6 code 0 rip 8135510b cs 8 rflags 10246 cr2 208 cpl 0 rsp 800022026c90 gsbase 0x81870ff0 kgsbase 0x0 panic: trap type 6, code=0, pc=8135510b syncing disk... done dump to dev 17,1 not possible rebooting... Hope this helps. --david
bsd.rd failure in VirtualBox
I often use VirtualBox (version 5.2.18 on OS X) to familiarize myself with new features in snapshots, before upgrading my physical hardware. This afternoon, I tried updating bsd.rd (amd64, 6.4-beta RAMDISK_CD #281) and wasn't able to successfully boot it. I had to rely on the video capture ability of VirtualBox to even notice there was a panic (typed out below) before it rebooted to the "BIOS" splash screen. Welcome to the OpenBSD/amd64 6.4 installation program. fatal protection fault in supervisor mode trap type 4 code 0 rip 810f24e4 cs 8 rflags 10206 cr2 6c1fed cpl a rsp 800022098a10 gsbase 0x8186eff0 kgsbase 0x0 panic: trap type 4, code 0, pc=0x810f24e4 syncing disks... done dump to dev 17,1 not possible rebooting... Also attached is the dmesg from a prior working snapshot. I haven't tried updating since this prior snapshot, so I don't have further insight into when the issue first appeared. --david OpenBSD 6.4-beta (GENERIC) #250: Sun Aug 26 00:10:38 MDT 2018 dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC real mem = 4278124544 (4079MB) avail mem = 4139393024 (3947MB) mpath0 at root scsibus0 at mpath0: 256 targets mainbus0 at root bios0 at mainbus0: SMBIOS rev. 2.5 @ 0xe1000 (10 entries) bios0: vendor innotek GmbH version "VirtualBox" date 12/01/2006 bios0: innotek GmbH VirtualBox acpi0 at bios0: rev 2 acpi0: sleep states S0 S5 acpi0: tables DSDT FACP APIC SSDT acpi0: wakeup devices acpitimer0 at acpi0: 3579545 Hz, 32 bits acpimadt0 at acpi0 addr 0xfee0: PC-AT compat cpu0 at mainbus0: apid 0 (boot processor) cpu0: Intel(R) Core(TM) i5-6360U CPU @ 2.00GHz, 1962.87 MHz, 06-4e-03 cpu0: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,SSE3,PCLMUL,MWAIT,SSSE3,CX16,PCID,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,RDRAND,NXE,RDTSCP,LONG,LAHF,ABM,3DNOWP,ITSC,FSGSBASE,AVX2,INVPCID,RDSEED,CLFLUSHOPT,L1DF,MELTDOWN cpu0: 256KB 64b/line 8-way L2 cache cpu0: smt 0, core 0, package 0 mtrr: CPU supports MTRRs but not enabled by BIOS cpu0: apic clock running at 999MHz cpu0: mwait min=64, max=64 ioapic0 at mainbus0: apid 1 pa 0xfec0, version 20, 24 pins, remapped acpiprt0 at acpi0: bus 0 (PCI0) acpicpu0 at acpi0: C1(@1 halt!) acpibat0 at acpi0: BAT0 model "1" serial 0 type VBOX oem "innotek" acpiac0 at acpi0: AC unit offline acpivideo0 at acpi0: GFX0 pci0 at mainbus0 bus 0 pchb0 at pci0 dev 0 function 0 "Intel 82441FX" rev 0x02 pcib0 at pci0 dev 1 function 0 "Intel 82371SB ISA" rev 0x00 pciide0 at pci0 dev 1 function 1 "Intel 82371AB IDE" rev 0x01: DMA, channel 0 configured to compatibility, channel 1 configured to compatibility wd0 at pciide0 channel 0 drive 0: wd0: 128-sector PIO, LBA, 20480MB, 41943040 sectors wd0(pciide0:0:0): using PIO mode 4, Ultra-DMA mode 2 atapiscsi0 at pciide0 channel 1 drive 0 scsibus1 at atapiscsi0: 2 targets cd0 at scsibus1 targ 0 lun 0: ATAPI 5/cdrom removable cd0(pciide0:1:0): using PIO mode 4, Ultra-DMA mode 2 vga1 at pci0 dev 2 function 0 "InnoTek VirtualBox Graphics Adapter" rev 0x00 wsdisplay0 at vga1 mux 1: console (80x25, vt100 emulation) wsdisplay0: screen 1-5 added (80x25, vt100 emulation) em0 at pci0 dev 3 function 0 "Intel 82540EM" rev 0x02: apic 1 int 19, address 08:00:27:36:05:97 "InnoTek VirtualBox Guest Service" rev 0x00 at pci0 dev 4 function 0 not configured piixpm0 at pci0 dev 7 function 0 "Intel 82371AB Power" rev 0x08: apic 1 int 23 iic0 at piixpm0 isa0 at pcib0 isadma0 at isa0 pckbc0 at isa0 port 0x60/5 irq 1 irq 12 pckbd0 at pckbc0 (kbd slot) wskbd0 at pckbd0: console keyboard, using wsdisplay0 pms0 at pckbc0 (aux slot) wsmouse0 at pms0 mux 0 pcppi0 at isa0 port 0x61 spkr0 at pcppi0 vscsi0 at root scsibus2 at vscsi0: 256 targets softraid0 at root scsibus3 at softraid0: 256 targets root on wd0a (4f192ba57dbd4eac.a) swap on wd0b dump on wd0b
Re: fix ld -Z on powerpc
On Tue, Aug 18, 2015 at 6:22 AM, Mark Kettenis mark.kette...@xs4all.nl wrote: Date: Tue, 18 Aug 2015 05:03:19 + From: Miod Vallat m...@online.fr I spent some time today figuting out why the binutils update broke ld -Z on powerpc. Turns out to be a fairly thorny issue. The new binutils discard empty setions. As a result the .gotpad0 and .gotpad1 sections have disappeared. And a s a consequence the __got_start and __got_end symbols are now absolute symbols as the section they referenced to is no longer there. For example, an older libc has: 845: 000eeb68 0 NOTYPE GLOBAL DEFAULT 17 __got_start whereas -current has: 810: 000eeb58 0 NOTYPE GLOBAL DEFAULT ABS __got_start On powerpc, crt0.o has weak references to __got_start and __got_end. When building a binary with ld -Z, these are resolved to the absolute symbols from libc. At runtime we then use these values, relocated as if they were addresses within the binary itself, to change protections and flush the instruction cache. This is very likely to result in a segmentation fault. There is probably a linker bug here, as it doesn't make any sense for the linker to pick the address of these symbols from libc and stick it into the binary. But I'm not sure about this. And it isn't all that obvious what the fix would be. Is the bug that the symbols end up as absolute? Or is the problem that it sibsequently resolves these to the values from libc.so? Wouldn't something like that address the problem better? Unfortunately not. Using PROVIDE() makes the __got_start/end symbols disappear from shared libraries. If I use: GOTSTART=__got_start = .; I end up with the same absolute symbols as before. You really have to tie the symbols to a specific section that isn't discarded to make them end up as normal symbols. And that is difficult since we try to make __got_start/end cover multiple sections, some of which may or may not be present. I still think my diff to remove code is the way to go. As I explained, it isn't really clear which set of symbols the references in crt0.o will actually resolve to. And the secure-plt work will require changes to the code anyway to make sure we don't end up with an executable GOT after all. Would KEEP statements in a linker script be a suitable workaround? https://sourceware.org/binutils/docs/ld/Input-Section-Keep.html#Input-Section-Keep --david
upd(4) man page update
If there’s no further work on upd(4) prior to 5.8, at least make the man page reflect present reality. - Update list of supported sensors, re-sorted by source file occurrence - Explain why manual sensorsd.conf(5) intervention can be necessary - Link to HID power specs - Prefer “a UPS” over “an UPS”, matches HID docs above - Other misc linguistic bikeshedding Apologies, this is my first experience with mdoc(7). --david Index: upd.4 === RCS file: /cvs/src/share/man/man4/upd.4,v retrieving revision 1.3 diff -u -p -r1.3 upd.4 --- upd.4 19 Mar 2014 09:22:25 - 1.3 +++ upd.4 26 Jun 2015 02:46:15 - @@ -25,17 +25,23 @@ .Sh DESCRIPTION The .Nm -driver exposes data from USB Power Devices (such as an UPS), -as hardware sensors via -.Xr sysctl 3 . +driver provides support for monitoring various sensors provided by +USB Power Devices (such as a UPS). +Supported sensor values are made available via the +.Xr sysctl 8 interface. .Pp -The following sensors are provided by the -.Nm -driver, which can be monitored using -.Xr sensorsd 8 : +The following sensors are supported by the driver: .Pp .Bl -item -offset indent -compact .It +BatteryPresent +.It +ShutdownImminent +.It +ACPresent +.It +Overload +.It RelativeStateOfCharge .It AbsoluteStateOfCharge @@ -48,19 +54,23 @@ Charging .It Discharging .It -BatteryPresent +AtRateTimeToFull .It -ShutdownImminent +AtRateTimeToEmpty .It -ACPresent +RunTimeToEmpty .It -AtRateTimeToFull +NeedReplacement .El .Sh SEE ALSO .Xr intro 4 , .Xr uhidev 4 , .Xr sensorsd 8 , +.Xr sensorsd.conf 5 , .Xr sysctl 8 +.Pp +The USB Power Devices specification can be found at +.Lk http://www.usb.org/developers/hidpage/ .Sh HISTORY The .Nm @@ -71,3 +81,18 @@ The .Nm driver was written by .An Andre de Oliveira . +.Sh BUGS +The +.Nm +driver does not indicate device health via aggregate or +individual sensor status. +Users who wish to monitor +.Nm +status using +.Xr sensorsd 8 +must manually establish +.Dq high +and +.Dq low +limits for sensor values of interest via +.Xr sensorsd.conf 5 .
Re: Simple upd(4) sensors
On Wed, Jun 10, 2015 at 5:23 AM, Martin Pieuchot m...@openbsd.org wrote: On 02/06/15(Tue) 22:36, David Higgs wrote: Here are some new sensors for upd(4) devices. All exist on my device except AtRateTimeToEmpty, which still seemed a logical addition given that AtRateTimeToFull is already present. - AtRateTimeToEmpty - RunTimeToEmpty - NeedReplacement - Overload Nice. If anyone had an AtRate sensor, it was probably producing meaningless output. The relevant spec [0] indicates that these are in minutes, and my device appears to be using seconds; the (previously unscaled) sensor value expects nanoseconds! [0] http://www.usb.org/developers/hidpage/pdcv10.pdf And lastly, the NeedReplacement report has nothing to do with the System Management Bus (SMB), so rename the constant. Nothing else in the tree appears to use it, so hopefully this is safe. Feedback and lsusb -v output is welcome, as usual. Have you got any feedback from upd(4) users? Update, someone was nice enough to test my diff, output below. Looks nearly identical to my own APC device, which isn't too surprising. --david snip Sensors before $ sysctl hw.sensors.upd0 hw.sensors.upd0.indicator0=On (BatteryPresent), OK hw.sensors.upd0.indicator1=On (Charging), OK hw.sensors.upd0.indicator2=Off (Discharging), OK hw.sensors.upd0.indicator3=Off (ShutdownImminent), OK hw.sensors.upd0.indicator4=On (ACPresent), OK hw.sensors.upd0.percent0=95.00% (RemainingCapacity), OK hw.sensors.upd0.percent1=100.00% (FullChargeCapacity), OK and after $ sysctl hw.sensors.upd0 hw.sensors.upd0.indicator0=On (BatteryPresent), OK hw.sensors.upd0.indicator1=On (Charging), OK hw.sensors.upd0.indicator2=Off (Discharging), OK hw.sensors.upd0.indicator3=Off (NeedReplacement), OK hw.sensors.upd0.indicator4=Off (ShutdownImminent), OK hw.sensors.upd0.indicator5=On (ACPresent), OK hw.sensors.upd0.indicator6=Off (Overload), OK hw.sensors.upd0.percent0=93.00% (RemainingCapacity), OK hw.sensors.upd0.percent1=100.00% (FullChargeCapacity), OK hw.sensors.upd0.timedelta0=1275.00 secs (RunTimeToEmpty), OK $ dmesg | tail -n 3 uhidev2 at uhub2 port 4 configuration 1 interface 0 American Power Conversion Back-UPS CS 500 FW:808.q8.I USB FW:q8 rev 1.10/0.06 addr 2 uhidev2: iclass 3/0, 98 report ids upd0 at uhidev2
Re: Simple upd(4) sensors
On Wed, Jun 10, 2015 at 5:23 AM, Martin Pieuchot m...@openbsd.org wrote: On 02/06/15(Tue) 22:36, David Higgs wrote: Here are some new sensors for upd(4) devices. All exist on my device except AtRateTimeToEmpty, which still seemed a logical addition given that AtRateTimeToFull is already present. - AtRateTimeToEmpty - RunTimeToEmpty - NeedReplacement - Overload Nice. If anyone had an AtRate sensor, it was probably producing meaningless output. The relevant spec [0] indicates that these are in minutes, and my device appears to be using seconds; the (previously unscaled) sensor value expects nanoseconds! [0] http://www.usb.org/developers/hidpage/pdcv10.pdf And lastly, the NeedReplacement report has nothing to do with the System Management Bus (SMB), so rename the constant. Nothing else in the tree appears to use it, so hopefully this is safe. Feedback and lsusb -v output is welcome, as usual. Have you got any feedback from upd(4) users? Not as of yet. Theo reminded me that upd(4) usability needs to improve, so before I add any more sensors I will next be looking into man page updates and/or improved sensor status (e.g. WARN, CRITICAL) so that you need less configuration voodoo to get useful behavior out of sensorsd(8). Let me know if these new sensors should wait, in which case I can cut a separate diff for the other fixes and build upon that instead. --david
Simple upd(4) sensors
Here are some new sensors for upd(4) devices. All exist on my device except
AtRateTimeToEmpty, which still seemed a logical addition given that
AtRateTimeToFull is already present.
- AtRateTimeToEmpty
- RunTimeToEmpty
- NeedReplacement
- Overload
If anyone had an AtRate sensor, it was probably producing meaningless output.
The relevant spec [0] indicates that these are in minutes, and my device
appears to be using seconds; the (previously unscaled) sensor value expects
nanoseconds!
[0] http://www.usb.org/developers/hidpage/pdcv10.pdf
And lastly, the NeedReplacement report has nothing to do with the System
Management Bus (SMB), so rename the constant. Nothing else in the tree appears
to use it, so hopefully this is safe.
Feedback and lsusb -v output is welcome, as usual.
--david
--- a/upd.c
+++ b/upd.c
@@ -66,7 +66,13 @@ static struct upd_usage_entry upd_usage_
{ HUP_BATTERY, HUB_DISCHARGING,
SENSOR_INDICATOR,Discharging },
{ HUP_BATTERY, HUB_ATRATE_TIMETOFULL,
- SENSOR_TIMEDELTA,AtRateTimeToFull }
+ SENSOR_TIMEDELTA,AtRateTimeToFull },
+ { HUP_BATTERY, HUB_ATRATE_TIMETOEMPTY,
+ SENSOR_TIMEDELTA,AtRateTimeToEmpty },
+ { HUP_BATTERY, HUB_RUNTIMETO_EMPTY,
+ SENSOR_TIMEDELTA,RunTimeToEmpty },
+ { HUP_BATTERY, HUB_NEED_REPLACEMENT,
+ SENSOR_INDICATOR,NeedReplacement },
};
static struct upd_usage_entry upd_usage_roots[] = {
{ HUP_BATTERY, HUB_BATTERY_PRESENT,
@@ -75,7 +81,9 @@ static struct upd_usage_entry upd_usage_
{ HUP_POWER,HUP_SHUTDOWN_IMMINENT,
SENSOR_INDICATOR,ShutdownImminent },
{ HUP_BATTERY, HUB_AC_PRESENT,
- SENSOR_INDICATOR,ACPresent }
+ SENSOR_INDICATOR,ACPresent },
+ { HUP_POWER,HUP_OVERLOAD,
+ SENSOR_INDICATOR,Overload },
};
#define UPD_MAX_SENSORS(nitems(upd_usage_batdep) +
nitems(upd_usage_roots))
@@ -410,6 +418,12 @@ upd_sensor_update(struct upd_softc *sc,
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 = 10LL;
+ break;
default:
adjust = 1; /* no scale adjust */
break;
--- a/usbhid.h
+++ b/usbhid.h
@@ -213,7 +213,7 @@ struct usb_hid_descriptor {
#define HUB_CONDITIONING_FLAG 0x0048
#define HUB_ATRATE_OK 0x0049
#define HUB_SMB_ERROR_CODE 0x004a
-#define HUB_SMB_NEED_REPLACE 0x004b
+#define HUB_NEED_REPLACEMENT 0x004b
#define HUB_ATRATE_TIMETOFULL 0x0060
#define HUB_ATRATE_TIMETOEMPTY 0x0061
#define HUB_AVERAGE_CURRENT0x0062
Re: UPD regression with
On May 11, 2015, at 9:02 PM, David Higgs hig...@gmail.com wrote:
On May 11, 2015, at 8:21 PM, David Higgs hig...@gmail.com
mailto:hig...@gmail.com wrote:
On Mon, May 11, 2015 at 8:07 PM, Alexander Hall alexan...@beard.se
mailto:alexan...@beard.sewrote:
Upgrading to the latest snapshot, I noticed my upd sensors had been
disturbingly crippled.
uhidev0 at uhub4 port 1 configuration 1 interface 0 EATON Eaton 3S rev
2.00/1.00 addr 2
uhidev0: iclass 3/0, 32 report ids
upd0 at uhidev0
Diff below is what happens from upd.c r1.13 to r1.14.
-hw.sensors.upd0.indicator0=On (ACPresent), OK
-hw.sensors.upd0.indicator1=On (Charging), OK
-hw.sensors.upd0.indicator2=Off (Discharging), OK
-hw.sensors.upd0.indicator3=Off (ShutdownImminent), OK
-hw.sensors.upd0.percent0=100.00% (FullChargeCapacity), OK
-hw.sensors.upd0.percent1=100.00% (RemainingCapacity), OK
+hw.sensors.upd0.indicator0=Off (ShutdownImminent), OK
+hw.sensors.upd0.indicator1=On (ACPresent), OK
Is this an expected fallout? Can I provide more info to assist? Full
dmesg (latest snap + vanilla current kernel w/ upd.c r1.13) follows.
It seems your device doesn't have a BatteryPresent report, or it is somehow
getting mangled. Can you run lsusb -v on your device and check if Battery
Present is shown?
If it is not, we'll have to make sensor dependencies less strict. I'll
start thinking about how to do this.
Alternatively, you could try the following diff, which flattens the sensor
dependency tree when a parent sensor isn’t available.
Below is a diff which has the correct number of parentheses and actually
compiles. Sorry for the noise.
--david
--- a/upd.c
+++ b/upd.c
@@ -225,8 +225,12 @@ upd_attach_sensor_tree(struct upd_softc
for (i = 0; i nentries; i++) {
entry = entries + i;
- if (!upd_lookup_usage_entry(desc, size, entry, item))
+ if (!upd_lookup_usage_entry(desc, size, entry, item)) {
+ /* dependency missing, add children to parent */
+ upd_attach_sensor_tree(sc, desc, size,
+ entry-nchildren, entry-children, queue);
continue;
+ }
DPRINTF((%s: found %s on repid=%d\n, DEVNAME(sc),
entry-usage_name, item.report_ID));
Re: UPD regression with
On Mon, May 11, 2015 at 8:07 PM, Alexander Hall alexan...@beard.se wrote: Upgrading to the latest snapshot, I noticed my upd sensors had been disturbingly crippled. uhidev0 at uhub4 port 1 configuration 1 interface 0 EATON Eaton 3S rev 2.00/1.00 addr 2 uhidev0: iclass 3/0, 32 report ids upd0 at uhidev0 Diff below is what happens from upd.c r1.13 to r1.14. -hw.sensors.upd0.indicator0=On (ACPresent), OK -hw.sensors.upd0.indicator1=On (Charging), OK -hw.sensors.upd0.indicator2=Off (Discharging), OK -hw.sensors.upd0.indicator3=Off (ShutdownImminent), OK -hw.sensors.upd0.percent0=100.00% (FullChargeCapacity), OK -hw.sensors.upd0.percent1=100.00% (RemainingCapacity), OK +hw.sensors.upd0.indicator0=Off (ShutdownImminent), OK +hw.sensors.upd0.indicator1=On (ACPresent), OK Is this an expected fallout? Can I provide more info to assist? Full dmesg (latest snap + vanilla current kernel w/ upd.c r1.13) follows. It seems your device doesn't have a BatteryPresent report, or it is somehow getting mangled. Can you run lsusb -v on your device and check if Battery Present is shown? If it is not, we'll have to make sensor dependencies less strict. I'll start thinking about how to do this. Thanks for the report. --david
Re: UPD regression with
On May 11, 2015, at 8:21 PM, David Higgs hig...@gmail.com wrote:
On Mon, May 11, 2015 at 8:07 PM, Alexander Hall alexan...@beard.se
mailto:alexan...@beard.sewrote:
Upgrading to the latest snapshot, I noticed my upd sensors had been
disturbingly crippled.
uhidev0 at uhub4 port 1 configuration 1 interface 0 EATON Eaton 3S rev
2.00/1.00 addr 2
uhidev0: iclass 3/0, 32 report ids
upd0 at uhidev0
Diff below is what happens from upd.c r1.13 to r1.14.
-hw.sensors.upd0.indicator0=On (ACPresent), OK
-hw.sensors.upd0.indicator1=On (Charging), OK
-hw.sensors.upd0.indicator2=Off (Discharging), OK
-hw.sensors.upd0.indicator3=Off (ShutdownImminent), OK
-hw.sensors.upd0.percent0=100.00% (FullChargeCapacity), OK
-hw.sensors.upd0.percent1=100.00% (RemainingCapacity), OK
+hw.sensors.upd0.indicator0=Off (ShutdownImminent), OK
+hw.sensors.upd0.indicator1=On (ACPresent), OK
Is this an expected fallout? Can I provide more info to assist? Full
dmesg (latest snap + vanilla current kernel w/ upd.c r1.13) follows.
It seems your device doesn't have a BatteryPresent report, or it is somehow
getting mangled. Can you run lsusb -v on your device and check if Battery
Present is shown?
If it is not, we'll have to make sensor dependencies less strict. I'll start
thinking about how to do this.
Alternatively, you could try the following diff, which flattens the sensor
dependency tree when a parent sensor isn’t available.
Thanks.
--david
--- a/upd.c
+++ b/upd.c
@@ -225,8 +225,12 @@ upd_attach_sensor_tree(struct upd_softc
for (i = 0; i nentries; i++) {
entry = entries + i;
- if (!upd_lookup_usage_entry(desc, size, entry, item))
+ if (!upd_lookup_usage_entry(desc, size, entry, item) {
+ /* dependency missing, add children to parent */
+ upd_attach_sensor_tree(sc, desc, size,
+ entry-nchildren, entry-children, queue);
continue;
+ }
DPRINTF((%s: found %s on repid=%d\n, DEVNAME(sc),
entry-usage_name, item.report_ID));
Re: Async upd(4) - patch 7/7
On Apr 30, 2015, at 7:09 AM, Martin Pieuchot m...@openbsd.org wrote:
On 24/04/15(Fri) 20:48, David Higgs wrote:
This is the final patch in the series.
Utilize the pending flags and report callback for their intended purpose -
to process async behavior.
Apply splusb() to ensure report callbacks can't fire before their data
structures have been properly updated. This only needs to happen in
upd_refresh(); all other calls to upd_request_children() are from a report
callback.
This is some good work. I don't think your patch #6 makes sense without
#7, so I merged them.
I don't think you need a pending flags for the sensors, since what you
are querying are the reports. You basically do not want to send to
requests for the same report at once.
I still disagree. However, the fix for this isn’t difficult and can be handled
in a future diff.
I'd also take the simpler approach of not deactivating all sensors if
you fail to *submit* a transfer. When such thing happens it's either
because the device/bus is going away or because we cannot allocate
memory.
Even in these situations, invalidating all affected sensors seems an easy and
consistent thing to do.
I tweaked your diff below, included some comments and your copyright.
This is untested but I hope you'll correct/improve it.
Your tweaks were good, so I tweaked it further:
- When submit fails, invalidate affected sensors as described above.
- When invalidating sensors, do it recursively.
- When battery is not present, invalidate children but not BatteryPresent.
Let me know what you think.
--david
--- a/upd.c
+++ b/upd.c
@@ -1,6 +1,7 @@
/* $OpenBSD: upd.c,v 1.19 2015/04/30 10:09:31 mpi Exp $ */
/*
+ * Copyright (c) 2015 David Higgs hig...@gmail.com
* Copyright (c) 2014 Andre de Oliveira an...@openbsd.org
*
* Permission to use, copy, modify, and distribute this software for any
@@ -78,25 +79,28 @@ static struct upd_usage_entry upd_usage_
};
#define UPD_MAX_SENSORS(nitems(upd_usage_batdep) +
nitems(upd_usage_roots))
+SLIST_HEAD(upd_sensor_head, upd_sensor);
+
struct upd_report {
- size_t size;
- SLIST_HEAD(, upd_sensor)sensors;
+ size_t size; /* Size of the report */
+ struct upd_sensor_head sensors;/* List in dependency order */
+ int pending;/* Waiting for an answer */
};
-SLIST_HEAD(upd_sensor_head, upd_sensor);
struct upd_sensor {
- struct ksensor ksensor;
- struct hid_item hitem;
- int attached;
- struct upd_sensor_head children;
- SLIST_ENTRY(upd_sensor) dep_next;
- SLIST_ENTRY(upd_sensor) rep_next;
+ struct ksensor ksensor;
+ struct hid_item hitem;
+ int attached; /* Is there a matching report */
+ struct upd_sensor_head children; /* list of children sensors */
+ SLIST_ENTRY(upd_sensor) dep_next; /* next in the child list */
+ SLIST_ENTRY(upd_sensor) rep_next; /* next in the report list */
};
struct upd_softc {
struct uhidevsc_hdev;
int sc_num_sensors;
u_intsc_max_repid;
+ char sc_buf[256];
/* sensor framework */
struct ksensordevsc_sensordev;
@@ -112,11 +116,13 @@ void upd_attach_sensor_tree(struct upd_s
struct upd_usage_entry *, struct upd_sensor_head *);
int upd_detach(struct device *, int);
-void upd_refresh(void *);
-void upd_update_sensors(struct upd_softc *, uint8_t *, unsigned int, int);
-void upd_update_sensor_value(struct upd_softc *, struct upd_sensor *,
-uint8_t *, int);
void upd_intr(struct uhidev *, void *, uint);
+void upd_refresh(void *);
+void upd_request_children(struct upd_softc *, struct upd_sensor_head *);
+void upd_update_report_cb(void *, int, void *, int);
+
+void upd_sensor_invalidate(struct upd_softc *, struct upd_sensor *);
+void upd_sensor_update(struct upd_softc *, struct upd_sensor *, uint8_t *,
int);
int upd_lookup_usage_entry(void *, int, struct upd_usage_entry *,
struct hid_item *);
struct upd_sensor *upd_lookup_sensor(struct upd_softc *, int, int);
@@ -126,10 +132,7 @@ struct cfdriver upd_cd = {
};
const struct cfattach upd_ca = {
- sizeof(struct upd_softc),
- upd_match,
- upd_attach,
- upd_detach
+ sizeof(struct upd_softc), upd_match, upd_attach, upd_detach
};
int
@@ -273,42 +276,50 @@ upd_detach(struct device *self, int flag
sensor = sc-sc_sensors[i];
if (sensor-attached)
sensor_detach(sc-sc_sensordev, sensor-ksensor);
- DPRINTF((upd_detach: %s\n, sensor-ksensor.desc));
}
free(sc-sc_reports, M_USBDEV, 0);
free(sc
Re: Async upd(4) - patch 7/7
On Thu, Apr 30, 2015 at 7:09 AM, Martin Pieuchot m...@openbsd.org wrote: On 24/04/15(Fri) 20:48, David Higgs wrote: This is the final patch in the series. Utilize the pending flags and report callback for their intended purpose - to process async behavior. Apply splusb() to ensure report callbacks can't fire before their data structures have been properly updated. This only needs to happen in upd_refresh(); all other calls to upd_request_children() are from a report callback. This is some good work. I don't think your patch #6 makes sense without #7, so I merged them. Sounds good, thanks for all the help. I don't think you need a pending flags for the sensors, since what you are querying are the reports. You basically do not want to send to requests for the same report at once. In general the code tries to avoid requesting reports more than once, but this prevents sensor dependencies from causing some nasty problems. In the example below, I don't want the update of C to re-update A and trigger an infinite loop by requesting B. Sensor A (report 1) - Sensor B (report 2) - Sensor C (report 1) The alternative is to allow inconsistent sensor values. If the value of B affects the value of C, I would rather perform a 2nd request for report 1 once I have the value of B, than to put garbage/bogus values into C. I prefer to keep sensor values consistent, to ensure nobody's sensorsd starts to panic and because I doubt many real devices have this kind of annoying HID descriptor layout. I'd also take the simpler approach of not deactivating all sensors if you fail to *submit* a transfer. When such thing happens it's either because the device/bus is going away or because we cannot allocate memory. This sounds fine for the bus/device case or if M_NOWAIT would trigger a panic. How about if the usb device itself runs out of memory/buffers; will the rest of the system stay happy? Again, I prefer to err on the side of consistency and invalidate child sensors so sensorsd isn't looking at stale values. I tweaked your diff below, included some comments and your copyright. This is untested but I hope you'll correct/improve it. I'll take a look over the next week. I'm also curious, which new reports/sensors do you want to add? I haven't looked at this in some months. My consumer-grade device had several useful sensors that aren't currently handled. There are some internal reports (CapacityMode, AtRate) that aren't suitable as sensors, but might fix other sensor values being mis-reported. And there are interactive things that can be done via reports - silencing the audible alarm, requesting self-tests - that would be good as sysctls or ioctls someday. I'll be happy to look at lsusb output if anyone has a device in mind. Thanks. --david
Re: Map upd(4) sensors to reports
On Fri, Apr 24, 2015 at 8:43 PM, David Higgs hig...@gmail.com wrote: Associate sensors with the reports they are updated by. Only the reports that have sensors will be enabled, so the enabled field becomes redundant. An important but subtle side-effect is that because the sensor tree is constructed depth-first, each report SLIST will contain sensors in dependency order. This is patch 5/7, by the way.
Map upd(4) sensors to reports
Associate sensors with the reports they are updated by. Only the reports that
have sensors will be enabled, so the enabled field becomes redundant.
An important but subtle side-effect is that because the sensor tree is
constructed depth-first, each report SLIST will contain sensors in dependency
order.
--david
--- a/upd.c
+++ b/upd.c
@@ -79,8 +79,8 @@ static struct upd_usage_entry upd_usage_
#define UPD_MAX_SENSORS(nitems(upd_usage_batdep) +
nitems(upd_usage_roots))
struct upd_report {
- size_t size;
- int enabled;
+ size_t size;
+ SLIST_HEAD(, upd_sensor)sensors;
};
SLIST_HEAD(upd_sensor_head, upd_sensor);
@@ -90,6 +90,7 @@ struct upd_sensor {
int attached;
struct upd_sensor_head children;
SLIST_ENTRY(upd_sensor) dep_next;
+ SLIST_ENTRY(upd_sensor) rep_next;
};
struct upd_softc {
@@ -183,6 +184,8 @@ upd_attach(struct device *parent, struct
sc-sc_reports = mallocarray(sc-sc_max_repid,
sizeof(struct upd_report), M_USBDEV, M_WAITOK | M_ZERO);
+ for (i = 0; i sc-sc_max_repid; i++)
+ SLIST_INIT(sc-sc_reports[i].sensors);
sc-sc_sensors = mallocarray(UPD_MAX_SENSORS,
sizeof(struct upd_sensor), M_USBDEV, M_WAITOK | M_ZERO);
for (i = 0; i UPD_MAX_SENSORS; i++)
@@ -214,6 +217,7 @@ upd_attach_sensor_tree(struct upd_softc
struct hid_item item;
struct upd_usage_entry *entry;
struct upd_sensor*sensor;
+ struct upd_report*report;
int i;
for (i = 0; i nentries; i++) {
@@ -243,11 +247,11 @@ upd_attach_sensor_tree(struct upd_softc
upd_attach_sensor_tree(sc, desc, size, entry-nchildren,
entry-children, sensor-children);
- if (sc-sc_reports[item.report_ID].enabled)
- continue;
- sc-sc_reports[item.report_ID].size = hid_report_size(desc,
- size, item.kind, item.report_ID);
- sc-sc_reports[item.report_ID].enabled = 1;
+ report = sc-sc_reports[item.report_ID];
+ if (SLIST_EMPTY(report-sensors))
+ report-size = hid_report_size(desc,
+ size, item.kind, item.report_ID);
+ SLIST_INSERT_HEAD(report-sensors, sensor, rep_next);
}
}
@@ -288,7 +292,7 @@ upd_refresh(void *arg)
for (repid = 0; repid sc-sc_max_repid; repid++) {
report = sc-sc_reports[repid];
- if (!report-enabled)
+ if (SLIST_EMPTY(report-sensors))
continue;
memset(buf, 0x0, sizeof(buf));
Async upd(4) - patch 7/7
This is the final patch in the series.
Utilize the pending flags and report callback for their intended purpose - to
process async behavior.
Apply splusb() to ensure report callbacks can't fire before their data
structures have been properly updated. This only needs to happen in
upd_refresh(); all other calls to upd_request_children() are from a report
callback.
--david
--- a/upd.c
+++ b/upd.c
@@ -99,6 +99,7 @@ struct upd_softc {
struct uhidevsc_hdev;
int sc_num_sensors;
u_intsc_max_repid;
+ char sc_buf[256];
/* sensor framework */
struct ksensordevsc_sensordev;
@@ -290,27 +291,12 @@ void
upd_refresh(void *arg)
{
struct upd_softc*sc = arg;
- struct upd_report *report;
- uint8_t buf[256];
- int repid, actlen, done;
+ int s;
- /* request root sensors */
+ /* request root sensors, do not let async handlers fire yet */
+ s = splusb();
upd_request_children(sc, sc-sc_root_sensors, 1);
-
- /* repeat until all reports queried */
- do {
- done = 1;
- for (repid = 0; repid sc-sc_max_repid; repid++) {
- report = sc-sc_reports[repid];
- if (!report-pending)
- continue;
- memset(buf, 0x0, sizeof(buf));
- actlen = uhidev_get_report(sc-sc_hdev.sc_parent,
- UHID_FEATURE_REPORT, repid, buf, report-size);
- upd_update_report_cb(sc, repid, buf, actlen);
- done = 0;
- }
- } while (!done);
+ splx(s);
}
void
@@ -336,7 +322,14 @@ upd_request_children(struct upd_softc *s
} else if (report-pending)
/* already requested */
sensor-pending = 1;
- else {
+ else if (uhidev_get_report_async(sc-sc_hdev.sc_parent,
+ UHID_FEATURE_REPORT, repid, sc-sc_buf,
+ report-size, sc, upd_update_report_cb) 0) {
+ DPRINTF((%s: %s request of repid %d failed\n,
+ DEVNAME(sc), sensor-ksensor.desc, repid));
+ sensor-pending = 1;
+ upd_update_report_cb(sc, repid, NULL, -1);
+ } else {
DPRINTF((%s: %s requests repid %d\n,
DEVNAME(sc), sensor-ksensor.desc, repid));
sensor-pending = 1;
upd(4) sensor lookup order - patch 3/7
Locate sensors in table order - by parsing the USB descriptor multiple times -
instead of in the order they exist in the USB descriptor.
This will greatly ease construction of a sensor dependency tree, in the next
diff.
--david
--- a/upd.c
+++ b/upd.c
@@ -86,7 +86,6 @@ struct upd_softc {
struct uhidevsc_hdev;
int sc_num_sensors;
u_intsc_max_repid;
- u_intsc_max_sensors;
/* sensor framework */
struct ksensordevsc_sensordev;
@@ -104,7 +103,8 @@ void upd_update_sensors(struct upd_softc
void upd_update_sensor_value(struct upd_softc *, struct upd_sensor *,
uint8_t *, int);
void upd_intr(struct uhidev *, void *, uint);
-struct upd_usage_entry *upd_lookup_usage_entry(const struct hid_item *);
+int upd_lookup_usage_entry(void *, int, struct upd_usage_entry *,
+struct hid_item *);
struct upd_sensor *upd_lookup_sensor(struct upd_softc *, int, int);
struct cfdriver upd_cd = {
@@ -124,9 +124,9 @@ upd_match(struct device *parent, void *m
struct uhidev_attach_arg *uha = (struct uhidev_attach_arg *)aux;
int size;
void *desc;
- struct hid_data *hdata;
struct hid_item item;
int ret = UMATCH_NONE;
+ int i;
if (uha-reportid != UHIDEV_CLAIM_ALLREPORTID)
return (ret);
@@ -138,14 +138,12 @@ upd_match(struct device *parent, void *m
* look for at least one sensor of our table
*/
uhidev_get_report_desc(uha-parent, desc, size);
- for (hdata = hid_start_parse(desc, size, hid_feature);
-hid_get_item(hdata, item); ) {
- if (upd_lookup_usage_entry(item) != NULL) {
+ for (i = 0; i nitems(upd_usage_table); i++)
+ if (upd_lookup_usage_entry(desc, size,
+ upd_usage_table + i, item)) {
ret = UMATCH_VENDOR_PRODUCT;
break;
}
- }
- hid_end_parse(hdata);
return (ret);
}
@@ -156,17 +154,16 @@ upd_attach(struct device *parent, struct
struct upd_softc *sc = (struct upd_softc *)self;
struct uhidev_attach_arg *uha = (struct uhidev_attach_arg *)aux;
struct hid_item item;
- struct hid_data *hdata;
struct upd_usage_entry *entry;
struct upd_sensor*sensor;
int size;
+ int i;
void *desc;
sc-sc_hdev.sc_intr = upd_intr;
sc-sc_hdev.sc_parent = uha-parent;
sc-sc_reports = NULL;
sc-sc_sensors = NULL;
- sc-sc_max_sensors = nitems(upd_usage_table);
strlcpy(sc-sc_sensordev.xname, DEVNAME(sc),
sizeof(sc-sc_sensordev.xname));
@@ -177,27 +174,20 @@ upd_attach(struct device *parent, struct
sc-sc_reports = mallocarray(sc-sc_max_repid,
sizeof(struct upd_report), M_USBDEV, M_WAITOK | M_ZERO);
- sc-sc_sensors = mallocarray(sc-sc_max_sensors,
+ sc-sc_sensors = mallocarray(nitems(upd_usage_table),
sizeof(struct upd_sensor), M_USBDEV, M_WAITOK | M_ZERO);
- size = sc-sc_max_sensors * sizeof(struct upd_sensor);
sc-sc_num_sensors = 0;
- uhidev_get_report_desc(uha-parent, desc, size);
- for (hdata = hid_start_parse(desc, size, hid_feature);
-hid_get_item(hdata, item)
-sc-sc_num_sensors sc-sc_max_sensors; ) {
- DPRINTF((upd: repid=%d\n, item.report_ID));
- if (item.kind != hid_feature ||
- item.report_ID 0 ||
- item.report_ID = sc-sc_max_repid)
- continue;
- if ((entry = upd_lookup_usage_entry(item)) == NULL)
+ uhidev_get_report_desc(uha-parent, desc, size);
+ for (i = 0; i nitems(upd_usage_table); i++) {
+ entry = upd_usage_table[i];
+ if (!upd_lookup_usage_entry(desc, size, entry, item))
continue;
- /* filter repeated usages, avoid duplicated sensors */
- sensor = upd_lookup_sensor(sc, entry-usage_pg,
- entry-usage_id);
- if (sensor != NULL)
+ DPRINTF((%s: found %s on repid=%d\n, DEVNAME(sc),
+ entry-usage_name, item.report_ID));
+ if (item.report_ID 0 ||
+ item.report_ID = sc-sc_max_repid)
continue;
sensor = sc-sc_sensors[sc-sc_num_sensors];
@@ -219,7 +209,6 @@ upd_attach(struct device *parent, struct
size, item.kind, item.report_ID);
sc-sc_reports[item.report_ID].enabled = 1;
}
- hid_end_parse(hdata);
DPRINTF((upd: sc_num_sensors=%d\n, sc-sc_num_sensors));
upd(4) patchset
Huge thanks to all devs who provided initial feedback in spite of my inconsistent development pace, especially mpi. This was mainly a bugfix/infrastructure effort. There's still plenty of work to do on new features - better sensor units, new sensors, maybe some sysctls. Happy to accept bug reports, comments, questions, and feedback. Meta-feedback is good too - patches too small/crappy, coding style requests, etc. --david
upd(4) sensor value - patch 2/7
Split out sensor value update now, since sensor updating will become more
complex.
Also, avoid potential signed/unsigned and truncation errors. Sensor values are
int64_t wide, so put them to work.
--david
--- a/upd.c
+++ b/upd.c
@@ -101,6 +101,8 @@ int upd_detach(struct device *, int);
void upd_refresh(void *);
void upd_update_sensors(struct upd_softc *, uint8_t *, unsigned int, int);
+void upd_update_sensor_value(struct upd_softc *, struct upd_sensor *,
+uint8_t *, int);
void upd_intr(struct uhidev *, void *, uint);
struct upd_usage_entry *upd_lookup_usage_entry(const struct hid_item *);
struct upd_sensor *upd_lookup_sensor(struct upd_softc *, int, int);
@@ -324,8 +326,7 @@ upd_update_sensors(struct upd_softc *sc,
int repid)
{
struct upd_sensor *sensor;
- ulong hdata, batpres;
- ulong adjust;
+ ulong batpres;
int i;
sensor = upd_lookup_sensor(sc, HUP_BATTERY, HUB_BATTERY_PRESENT);
@@ -348,27 +349,35 @@ upd_update_sensors(struct upd_softc *sc,
}
}
- 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;
- default:
- adjust = 1; /* no scale adjust */
- break;
- }
+ upd_update_sensor_value(sc, sensor, buf, len);
+ }
+}
- hdata = hid_get_data(buf, len, sensor-hitem.loc);
+void
+upd_update_sensor_value(struct upd_softc *sc, struct upd_sensor *sensor,
+uint8_t *buf, int len)
+{
+ int64_t hdata, adjust;
- sensor-ksensor.value = hdata * adjust;
- sensor-ksensor.status = SENSOR_S_OK;
- sensor-ksensor.flags = ~SENSOR_FINVALID;
- DPRINTF((%s: hidget data: %lu\n, DEVNAME(sc), hdata));
+ 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;
+ default:
+ adjust = 1; /* no scale adjust */
+ break;
}
-}
+ 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;
+ DPRINTF((%s: %s hidget data: %lld\n, DEVNAME(sc),
+ sensor-ksensor.desc, hdata));
+}
void
upd_intr(struct uhidev *uh, void *p, uint len)
DEVNAME for upd(4) - patch 1/7
Should be pretty straightforward.
--david
--- a/upd.c
+++ b/upd.c
@@ -39,6 +39,8 @@
#define DPRINTF(x)
#endif
+#define DEVNAME(sc)((sc)-sc_hdev.sc_dev.dv_xname)
+
struct upd_usage_entry {
uint8_t usage_pg;
uint8_t usage_id;
@@ -164,12 +166,12 @@ upd_attach(struct device *parent, struct
sc-sc_sensors = NULL;
sc-sc_max_sensors = nitems(upd_usage_table);
- strlcpy(sc-sc_sensordev.xname, sc-sc_hdev.sc_dev.dv_xname,
+ strlcpy(sc-sc_sensordev.xname, DEVNAME(sc),
sizeof(sc-sc_sensordev.xname));
sc-sc_max_repid = uha-parent-sc_nrepid;
DPRINTF((\nupd: devname=%s sc_max_repid=%d\n,
- sc-sc_hdev.sc_dev.dv_xname, sc-sc_max_repid));
+ DEVNAME(sc), sc-sc_max_repid));
sc-sc_reports = mallocarray(sc-sc_max_repid,
sizeof(struct upd_report), M_USBDEV, M_WAITOK | M_ZERO);
@@ -363,8 +365,7 @@ upd_update_sensors(struct upd_softc *sc,
sensor-ksensor.value = hdata * adjust;
sensor-ksensor.status = SENSOR_S_OK;
sensor-ksensor.flags = ~SENSOR_FINVALID;
- DPRINTF((%s: hidget data: %lu\n,
- sc-sc_sensordev.xname, hdata));
+ DPRINTF((%s: hidget data: %lu\n, DEVNAME(sc), hdata));
}
}
Build upd(4) sensor tree - patch 4/7
Divide sensors into two tables - normal sensors and battery dependent sensors.
Build the sensor dependency tree - every sensor has an SLIST of dependent
children.
Also, don’t bother looking for battery dependent sensors at device attach, it
doesn’t seem helpful.
(Someone please correct me if static structure elements aren’t initialized to
zero.)
--david
--- a/upd.c
+++ b/upd.c
@@ -23,6 +23,7 @@
#include sys/kernel.h
#include sys/malloc.h
#include sys/device.h
+#include sys/queue.h
#include sys/sensors.h
#include dev/usb/hid.h
@@ -46,9 +47,11 @@ struct upd_usage_entry {
uint8_t usage_id;
enum sensor_typesenstype;
char*usage_name; /* sensor string */
+ int nchildren;
+ struct upd_usage_entry *children;
};
-static struct upd_usage_entry upd_usage_table[] = {
+static struct upd_usage_entry upd_usage_batdep[] = {
{ HUP_BATTERY, HUB_REL_STATEOF_CHARGE,
SENSOR_PERCENT, RelativeStateOfCharge },
{ HUP_BATTERY, HUB_ABS_STATEOF_CHARGE,
@@ -61,25 +64,32 @@ static struct upd_usage_entry upd_usage_
SENSOR_INDICATOR,Charging },
{ HUP_BATTERY, HUB_DISCHARGING,
SENSOR_INDICATOR,Discharging },
+ { HUP_BATTERY, HUB_ATRATE_TIMETOFULL,
+ SENSOR_TIMEDELTA,AtRateTimeToFull }
+};
+static struct upd_usage_entry upd_usage_roots[] = {
{ HUP_BATTERY, HUB_BATTERY_PRESENT,
- SENSOR_INDICATOR,BatteryPresent },
+ SENSOR_INDICATOR,BatteryPresent,
+ nitems(upd_usage_batdep), upd_usage_batdep },
{ HUP_POWER,HUP_SHUTDOWN_IMMINENT,
SENSOR_INDICATOR,ShutdownImminent },
{ HUP_BATTERY, HUB_AC_PRESENT,
- SENSOR_INDICATOR,ACPresent },
- { HUP_BATTERY, HUB_ATRATE_TIMETOFULL,
- SENSOR_TIMEDELTA,AtRateTimeToFull }
+ SENSOR_INDICATOR,ACPresent }
};
+#define UPD_MAX_SENSORS(nitems(upd_usage_batdep) +
nitems(upd_usage_roots))
struct upd_report {
size_t size;
int enabled;
};
+SLIST_HEAD(upd_sensor_head, upd_sensor);
struct upd_sensor {
- struct ksensor ksensor;
- struct hid_item hitem;
- int attached;
+ struct ksensor ksensor;
+ struct hid_item hitem;
+ int attached;
+ struct upd_sensor_head children;
+ SLIST_ENTRY(upd_sensor) dep_next;
};
struct upd_softc {
@@ -92,10 +102,13 @@ struct upd_softc {
struct sensor_task *sc_sensortask;
struct upd_report *sc_reports;
struct upd_sensor *sc_sensors;
+ struct upd_sensor_head sc_root_sensors;
};
int upd_match(struct device *, void *, void *);
void upd_attach(struct device *, struct device *, void *);
+void upd_attach_sensor_tree(struct upd_softc *, void *, int, int,
+struct upd_usage_entry *, struct upd_sensor_head *);
int upd_detach(struct device *, int);
void upd_refresh(void *);
@@ -134,13 +147,11 @@ upd_match(struct device *parent, void *m
DPRINTF((upd: vendor=0x%04x, product=0x%04x\n, uha-uaa-vendor,
uha-uaa-product));
- /*
-* look for at least one sensor of our table
-*/
+ /* need at least one sensor from root of tree */
uhidev_get_report_desc(uha-parent, desc, size);
- for (i = 0; i nitems(upd_usage_table); i++)
+ for (i = 0; i nitems(upd_usage_roots); i++)
if (upd_lookup_usage_entry(desc, size,
- upd_usage_table + i, item)) {
+ upd_usage_roots + i, item)) {
ret = UMATCH_VENDOR_PRODUCT;
break;
}
@@ -153,9 +164,6 @@ upd_attach(struct device *parent, struct
{
struct upd_softc *sc = (struct upd_softc *)self;
struct uhidev_attach_arg *uha = (struct uhidev_attach_arg *)aux;
- struct hid_item item;
- struct upd_usage_entry *entry;
- struct upd_sensor*sensor;
int size;
int i;
void *desc;
@@ -164,6 +172,7 @@ upd_attach(struct device *parent, struct
sc-sc_hdev.sc_parent = uha-parent;
sc-sc_reports = NULL;
sc-sc_sensors = NULL;
+ SLIST_INIT(sc-sc_root_sensors);
strlcpy(sc-sc_sensordev.xname, DEVNAME(sc),
sizeof(sc-sc_sensordev.xname));
@@ -174,13 +183,41 @@ upd_attach(struct device *parent, struct
sc-sc_reports = mallocarray(sc-sc_max_repid,
sizeof(struct upd_report), M_USBDEV, M_WAITOK | M_ZERO);
- sc-sc_sensors = mallocarray(nitems(upd_usage_table),
+ sc-sc_sensors = mallocarray(UPD_MAX_SENSORS,
sizeof(struct upd_sensor), M_USBDEV, M_WAITOK | M_ZERO);
-
upd(4) report order - patch 6/7
This is the big change that puts all the previous work together.
When a sensor update is needed, mark its report as pending; do this in
dependency order. When a report fails to query/reply, mark it and its children
as invalid. When the BatteryPresent says there is no battery, mark its
children as invalid too.
If BatteryPresent is on a repid that comes numerically after a child, the
do/while loop will try again until there are no more pending reports.
If BatteryPresent is on the same repid as a child, the report-sensors list
will have already put it in the correct order so the parent will update before
the child.
If BatteryPresent=false invalidates children that belong to an already-pending
report, they will not be updated, because sensor-pending will no longer be set.
--david
--- a/upd.c
+++ b/upd.c
@@ -81,6 +81,7 @@ static struct upd_usage_entry upd_usage_
struct upd_report {
size_t size;
SLIST_HEAD(, upd_sensor)sensors;
+ int pending;
};
SLIST_HEAD(upd_sensor_head, upd_sensor);
@@ -91,6 +92,7 @@ struct upd_sensor {
struct upd_sensor_head children;
SLIST_ENTRY(upd_sensor) dep_next;
SLIST_ENTRY(upd_sensor) rep_next;
+ int pending;
};
struct upd_softc {
@@ -113,7 +115,9 @@ void upd_attach_sensor_tree(struct upd_s
int upd_detach(struct device *, int);
void upd_refresh(void *);
-void upd_update_sensors(struct upd_softc *, uint8_t *, unsigned int, int);
+void upd_request_children(struct upd_softc *, struct upd_sensor_head *, int);
+void upd_update_report_cb(void *, int, void *, int);
+void upd_update_sensor(struct upd_softc *, struct upd_sensor *, uint8_t *,
int);
void upd_update_sensor_value(struct upd_softc *, struct upd_sensor *,
uint8_t *, int);
void upd_intr(struct uhidev *, void *, uint);
@@ -285,30 +289,59 @@ upd_detach(struct device *self, int flag
void
upd_refresh(void *arg)
{
- struct upd_softc*sc = (struct upd_softc *)arg;
+ struct upd_softc*sc = arg;
struct upd_report *report;
uint8_t buf[256];
- int repid, actlen;
+ int repid, actlen, done;
- for (repid = 0; repid sc-sc_max_repid; repid++) {
- report = sc-sc_reports[repid];
- if (SLIST_EMPTY(report-sensors))
- continue;
+ /* request root sensors */
+ upd_request_children(sc, sc-sc_root_sensors, 1);
- memset(buf, 0x0, sizeof(buf));
- actlen = uhidev_get_report(sc-sc_hdev.sc_parent,
- UHID_FEATURE_REPORT, repid, buf, report-size);
-
- if (actlen == -1) {
- DPRINTF((upd: failed to get report id=%02x\n, repid));
- continue;
+ /* repeat until all reports queried */
+ do {
+ done = 1;
+ for (repid = 0; repid sc-sc_max_repid; repid++) {
+ report = sc-sc_reports[repid];
+ if (!report-pending)
+ continue;
+ memset(buf, 0x0, sizeof(buf));
+ actlen = uhidev_get_report(sc-sc_hdev.sc_parent,
+ UHID_FEATURE_REPORT, repid, buf, report-size);
+ upd_update_report_cb(sc, repid, buf, actlen);
+ done = 0;
}
+ } while (!done);
+}
+
+void
+upd_request_children(struct upd_softc *sc, struct upd_sensor_head *queue,
+int valid)
+{
+ struct upd_sensor *sensor;
+ struct upd_report *report;
+ int repid;
- /* Deal with buggy firmwares. */
- if (actlen report-size)
- report-size = actlen;
+ SLIST_FOREACH(sensor, queue, dep_next) {
+ repid = sensor-hitem.report_ID;
+ report = sc-sc_reports[repid];
- upd_update_sensors(sc, buf, report-size, repid);
+ if (sensor-pending)
+ DPRINTF((%s: %s still pending (repid=%d)\n,
+ DEVNAME(sc), sensor-ksensor.desc, repid));
+ else if (!valid) {
+ DPRINTF((%s: marking %s invalid\n,
+ DEVNAME(sc), sensor-ksensor.desc));
+ sensor-pending = 1;
+ upd_update_sensor(sc, sensor, NULL, -1);
+ } else if (report-pending)
+ /* already requested */
+ sensor-pending = 1;
+ else {
+ DPRINTF((%s: %s requests repid %d\n,
+ DEVNAME(sc), sensor-ksensor.desc, repid));
+ sensor-pending = 1;
+ report-pending = 1;
+ }
}
}
upd(4) with async reports
Features of this diff:
- All upd(4) reports are queried asynchronously
- Use pending status to prevent duplicate usb queries
- No apparent changes from end-user point of view
Note: depends on the previous upd(4) LIST diff, not yet committed.
As usual, feedback and comments are welcome.
--david
--- a/upd.c
+++ b/upd.c
@@ -73,17 +73,20 @@ static struct upd_usage_entry upd_usage_
struct upd_report {
size_t size;
int enabled;
+ int pending;
};
struct upd_sensor {
struct ksensor ksensor;
struct hid_item hitem;
LIST_ENTRY(upd_sensor) next;
+ int pending;
};
struct upd_softc {
struct uhidevsc_hdev;
u_intsc_max_repid;
+ char sc_buf[256];
/* sensor framework */
struct ksensordevsc_sensordev;
@@ -98,7 +101,9 @@ void upd_attach(struct device *, struct
int upd_detach(struct device *, int);
void upd_refresh(void *);
-void upd_update_sensors(struct upd_softc *, uint8_t *, unsigned int, int);
+void upd_request_sensor_refresh(struct upd_softc *, struct upd_sensor *);
+void upd_update_report_cb(void *, int, void *, int);
+void upd_update_sensor(struct upd_softc *, struct upd_sensor *, uint8_t *,
int);
void upd_intr(struct uhidev *, void *, uint);
struct upd_usage_entry *upd_lookup_usage_entry(const struct hid_item *);
struct upd_sensor *upd_lookup_sensor(struct upd_softc *, int, int);
@@ -258,30 +263,68 @@ void
upd_refresh(void *arg)
{
struct upd_softc*sc = (struct upd_softc *)arg;
- struct upd_report *report;
- uint8_t buf[256];
- int repid, actlen;
+ struct upd_sensor *sensor;
- for (repid = 0; repid sc-sc_max_repid; repid++) {
- report = sc-sc_reports[repid];
- if (!report-enabled)
- continue;
+ LIST_FOREACH(sensor, sc-sc_sensorlist, next)
+ upd_request_sensor_refresh(sc, sensor);
+}
- memset(buf, 0x0, sizeof(buf));
- actlen = uhidev_get_report(sc-sc_hdev.sc_parent,
- UHID_FEATURE_REPORT, repid, buf, report-size);
+void
+upd_request_sensor_refresh(struct upd_softc *sc, struct upd_sensor *sensor)
+{
+ struct upd_report *report;
+ int repid;
- if (actlen == -1) {
- DPRINTF((upd: failed to get report id=%02x\n, repid));
- continue;
- }
+ repid = sensor-hitem.report_ID;
+ report = sc-sc_reports[repid];
+ if (sensor-pending)
+ DPRINTF((%s: sensor %s (repid=%d) still pending\n,
+ sc-sc_sensordev.xname, sensor-ksensor.desc, repid));
+ else if (report-pending)
+ /* already requested */
+ sensor-pending = 1;
+ else if (uhidev_get_report_async(sc-sc_hdev.sc_parent,
+ UHID_FEATURE_REPORT, repid, sc-sc_buf, report-size,
+ sc, upd_update_report_cb) 0) {
+ DPRINTF((%s: %s requests repid %d\n,
+ sc-sc_sensordev.xname, sensor-ksensor.desc, repid));
+ report-pending = 1;
+ sensor-pending = 1;
+ } else {
+ DPRINTF((%s: failed to request %s (repid=%d)\n,
+ sc-sc_sensordev.xname, sensor-ksensor.desc, repid));
+ sensor-ksensor.status = SENSOR_S_UNKNOWN;
+ sensor-ksensor.flags |= SENSOR_FINVALID;
+ }
+}
- /* Deal with buggy firmwares. */
- if (actlen report-size)
- report-size = actlen;
+void
+upd_update_report_cb(void *priv, int repid, void *data, int len)
+{
+ struct upd_softc*sc = priv;
+ struct upd_report *report;
+ struct upd_sensor *sensor;
- upd_update_sensors(sc, buf, report-size, repid);
+ /* handle buggy firmware */
+ report = sc-sc_reports[repid];
+ if (len 0 report-size != len) {
+ DPRINTF((%s: adjusting size of repid %d\n,
+ sc-sc_sensordev.xname, repid));
+ report-size = len;
}
+
+ LIST_FOREACH(sensor, sc-sc_sensorlist, next) {
+ if (sensor-hitem.report_ID == repid) {
+ if (len 0)
+ upd_update_sensor(sc, sensor, data, len);
+ else {
+ sensor-ksensor.status = SENSOR_S_UNKNOWN;
+ sensor-ksensor.flags |= SENSOR_FINVALID;
+ }
+ sensor-pending = 0;
+ }
+ }
+ report-pending = 0;
}
struct upd_usage_entry *
@@ -313,32 +356,22 @@ upd_lookup_sensor(struct upd_softc *sc,
}
void
-upd_update_sensors(struct upd_softc *sc, uint8_t
Re: Use LIST for upd(4) sensors
On Wed, Apr 1, 2015 at 7:52 AM, Martin Pieuchot m...@openbsd.org wrote: On 31/03/15(Tue) 23:06, David Higgs wrote: This was much more straightforward than expected. - Replace an array with a LIST of allocated sensors. - Remove or rescope variables counting sensors. - Allocated sensors are always attached. - Drop an unnecessary size calculation. Do you need this for an upcoming change? I fail to see the benefit to have a malloc(9) call per item, afaik theses items are really small and you have at most 10 of them... No, I just extrapolated too much from your suggestion to put attached sensors on a LIST. I'll adjust and resubmit. Thanks. --david
Use LIST for upd(4) sensors
This was much more straightforward than expected.
- Replace an array with a LIST of allocated sensors.
- Remove or rescope variables counting sensors.
- Allocated sensors are always attached.
- Drop an unnecessary size calculation.
Thanks.
--david
--- a/upd.c
+++ b/upd.c
@@ -23,6 +23,7 @@
#include sys/kernel.h
#include sys/malloc.h
#include sys/device.h
+#include sys/queue.h
#include sys/sensors.h
#include dev/usb/hid.h
@@ -77,20 +78,18 @@ struct upd_report {
struct upd_sensor {
struct ksensor ksensor;
struct hid_item hitem;
- int attached;
+ LIST_ENTRY(upd_sensor) next;
};
struct upd_softc {
struct uhidevsc_hdev;
- int sc_num_sensors;
u_intsc_max_repid;
- u_intsc_max_sensors;
/* sensor framework */
struct ksensordevsc_sensordev;
struct sensor_task *sc_sensortask;
struct upd_report *sc_reports;
- struct upd_sensor *sc_sensors;
+ LIST_HEAD(, upd_sensor) sc_sensors;
};
int upd_match(struct device *, void *, void *);
@@ -155,14 +154,14 @@ upd_attach(struct device *parent, struct
struct hid_data *hdata;
struct upd_usage_entry *entry;
struct upd_sensor*sensor;
+ int num_sensors;
int size;
void *desc;
sc-sc_hdev.sc_intr = upd_intr;
sc-sc_hdev.sc_parent = uha-parent;
sc-sc_reports = NULL;
- sc-sc_sensors = NULL;
- sc-sc_max_sensors = nitems(upd_usage_table);
+ LIST_INIT(sc-sc_sensors);
strlcpy(sc-sc_sensordev.xname, sc-sc_hdev.sc_dev.dv_xname,
sizeof(sc-sc_sensordev.xname));
@@ -173,14 +172,11 @@ upd_attach(struct device *parent, struct
sc-sc_reports = mallocarray(sc-sc_max_repid,
sizeof(struct upd_report), M_USBDEV, M_WAITOK | M_ZERO);
- sc-sc_sensors = mallocarray(sc-sc_max_sensors,
- sizeof(struct upd_sensor), M_USBDEV, M_WAITOK | M_ZERO);
- size = sc-sc_max_sensors * sizeof(struct upd_sensor);
- sc-sc_num_sensors = 0;
+ num_sensors = 0;
uhidev_get_report_desc(uha-parent, desc, size);
for (hdata = hid_start_parse(desc, size, hid_feature);
hid_get_item(hdata, item)
-sc-sc_num_sensors sc-sc_max_sensors; ) {
+num_sensors nitems(upd_usage_table); ) {
DPRINTF((upd: repid=%d\n, item.report_ID));
if (item.kind != hid_feature ||
item.report_ID 0 ||
@@ -196,7 +192,8 @@ upd_attach(struct device *parent, struct
if (sensor != NULL)
continue;
- sensor = sc-sc_sensors[sc-sc_num_sensors];
+ sensor = malloc(sizeof(struct upd_sensor), M_USBDEV,
+ M_WAITOK | M_ZERO);
memcpy(sensor-hitem, item, sizeof(struct hid_item));
strlcpy(sensor-ksensor.desc, entry-usage_name,
sizeof(sensor-ksensor.desc));
@@ -205,8 +202,8 @@ upd_attach(struct device *parent, struct
sensor-ksensor.status = SENSOR_S_UNKNOWN;
sensor-ksensor.value = 0;
sensor_attach(sc-sc_sensordev, sensor-ksensor);
- sensor-attached = 1;
- sc-sc_num_sensors++;
+ LIST_INSERT_HEAD(sc-sc_sensors, sensor, next);
+ num_sensors++;
if (sc-sc_reports[item.report_ID].enabled)
continue;
@@ -216,7 +213,7 @@ upd_attach(struct device *parent, struct
sc-sc_reports[item.report_ID].enabled = 1;
}
hid_end_parse(hdata);
- DPRINTF((upd: sc_num_sensors=%d\n, sc-sc_num_sensors));
+ DPRINTF((upd: num_sensors=%d\n, num_sensors));
sc-sc_sensortask = sensor_task_register(sc, upd_refresh, 6);
if (sc-sc_sensortask == NULL) {
@@ -235,7 +232,7 @@ upd_detach(struct device *self, int flag
{
struct upd_softc*sc = (struct upd_softc *)self;
struct upd_sensor *sensor;
- int i;
+ struct upd_sensor *t;
if (sc-sc_sensortask != NULL) {
wakeup(sc-sc_sensortask);
@@ -244,15 +241,14 @@ upd_detach(struct device *self, int flag
sensordev_deinstall(sc-sc_sensordev);
- for (i = 0; i sc-sc_num_sensors; i++) {
- sensor = sc-sc_sensors[i];
- if (sensor-attached)
- sensor_detach(sc-sc_sensordev, sensor-ksensor);
+ LIST_FOREACH_SAFE(sensor, sc-sc_sensors, next, t) {
+ sensor_detach(sc-sc_sensordev, sensor-ksensor);
DPRINTF((upd_detach: %s\n, sensor-ksensor.desc));
+ LIST_REMOVE(sensor, next);
+ free(sensor, M_USBDEV, sizeof(struct upd_sensor));
Simplify upd(4) attach
First in what will probably be a slow trickle, as I split my original big diff
into more-easily reviewed chunks and test each in sequence.
This patch makes upd_attach less confusing:
1. Ignore all bogus report IDs up front, they shouldn't be queried.
2. When a sensor is attached, it will be found by upd_lookup_sensor, so don’t
check attached.
Thanks.
--david
--- a/upd.c
+++ b/upd.c
@@ -183,7 +183,8 @@ upd_attach(struct device *parent, struct
sc-sc_num_sensors sc-sc_max_sensors; ) {
DPRINTF((upd: repid=%d\n, item.report_ID));
if (item.kind != hid_feature ||
- item.report_ID 0)
+ item.report_ID 0 ||
+ item.report_ID = sc-sc_max_repid)
continue;
if ((entry = upd_lookup_usage_entry(item)) == NULL)
@@ -192,11 +193,10 @@ upd_attach(struct device *parent, struct
/* filter repeated usages, avoid duplicated sensors */
sensor = upd_lookup_sensor(sc, entry-usage_pg,
entry-usage_id);
- if (sensor sensor-attached)
+ if (sensor != NULL)
continue;
sensor = sc-sc_sensors[sc-sc_num_sensors];
- /* keep our copy of hid_item */
memcpy(sensor-hitem, item, sizeof(struct hid_item));
strlcpy(sensor-ksensor.desc, entry-usage_name,
sizeof(sensor-ksensor.desc));
@@ -208,8 +208,7 @@ upd_attach(struct device *parent, struct
sensor-attached = 1;
sc-sc_num_sensors++;
- if (item.report_ID = sc-sc_max_repid ||
- sc-sc_reports[item.report_ID].enabled)
+ if (sc-sc_reports[item.report_ID].enabled)
continue;
sc-sc_reports[item.report_ID].size = hid_report_size(desc,
Re: Async upd(4)
On Mon, Mar 9, 2015 at 6:04 AM, Martin Pieuchot mpieuc...@nolizard.org
wrote:
On 05/03/15(Thu) 12:25, David Higgs wrote:
On Mar 3, 2015, at 8:44 AM, David Higgs hig...@gmail.com wrote:
With much help from mpi@, I have made a first big step towards
improving upd(4). I'm not sure when tree lock ends, but I'm still happy to
accept feedback if right now isn't the time to commit. There's plenty more
to do, but I'd like to get this ironed out before moving on.
New behavior with this diff:
- Leverage new USB async reporting (must have up-to-date tree)
- Sensor dependencies ensure reports are gathered in useful order
- Track pending reports to minimize USB queries
- Relevant sensors immediately invalidated when battery is removed
(instead of waiting for the next refresh)
Things that may need sanity checks:
- Simplified upd_attach; the old code seemed to have redundant /
confusing logic
- Integer promotion/truncation with batpres and hdata/adjust variables
Below is an overhauled diff with some additional considerations I came
up with.
Improvements on the previous version:
- ACPresent no longer depends on BatteryPresent
- Sensor dependencies now handled manually, so dynamic behavior can be
added later.
- Avoid hypothetical cases where certain USB report layouts could
trigger:
- Infinite loops in sensor dependencies.
- Updating sensor contents using stale information.
- Unnecessary sensor invalidation.
- Redundant USB transfers.
As before, comments, questions, and feedback are welcome.
That's great, some comments inline.
int upd_match(struct device *, void *, void *);
void upd_attach(struct device *, struct device *, void *);
int upd_detach(struct device *, int);
void upd_refresh(void *);
-void upd_update_sensors(struct upd_softc *, uint8_t *, unsigned int,
int);
-void upd_intr(struct uhidev *, void *, uint);
+void upd_request_sensor_refresh(struct upd_softc *, uint8_t, uint8_t,
int);
+void upd_update_sensor_cb(void *, int, void *, int);
+void upd_mark_sensor_invalid(struct upd_softc *, struct upd_sensor *,
int);
struct upd_usage_entry *upd_lookup_usage_entry(const struct hid_item *);
struct upd_sensor *upd_lookup_sensor(struct upd_softc *, int, int);
+int upd_battery_present(struct upd_softc *);
+void upd_update_batpres_sensor(struct upd_softc *, struct upd_sensor *,
+uint8_t *, int);
+void upd_update_batdep_sensor(struct upd_softc *, struct upd_sensor *,
+uint8_t *, int);
+void upd_update_sensor_value(struct upd_softc *, struct upd_sensor *,
+uint8_t *, int);
+void upd_intr(struct uhidev *, void *, uint);
+
+static struct upd_usage_entry upd_usage_table[] = {
+ { HUP_BATTERY, HUB_REL_STATEOF_CHARGE,
+ SENSOR_PERCENT, RelativeStateOfCharge,
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_ABS_STATEOF_CHARGE,
+ SENSOR_PERCENT, AbsoluteStateOfCharge,
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_REM_CAPACITY,
+ SENSOR_PERCENT, RemainingCapacity,
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_FULLCHARGE_CAPACITY,
+ SENSOR_PERCENT, FullChargeCapacity,
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_CHARGING,
+ SENSOR_INDICATOR, Charging,
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_DISCHARGING,
+ SENSOR_INDICATOR, Discharging,
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_BATTERY_PRESENT,
+ SENSOR_INDICATOR, BatteryPresent,
+ upd_update_batpres_sensor },
+ { HUP_POWER,HUP_SHUTDOWN_IMMINENT,
+ SENSOR_INDICATOR, ShutdownImminent,
+ upd_update_sensor_value },
+ { HUP_BATTERY, HUB_AC_PRESENT,
+ SENSOR_INDICATOR, ACPresent,
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_ATRATE_TIMETOFULL,
+ SENSOR_TIMEDELTA, AtRateTimeToFull,
+ upd_update_batdep_sensor }
+};
I see that all the HUP_BATTERY sensors have the same dependency. Why
not have a table for items without dependency (the parents) and a child
table per parent? This way you would have only one upd_update_sensor
function. This is really the key of this driver. Why a flat list like
you have right now you need much more code, flags and customs functions.
This does seem simpler; I'll give it a shot.
struct cfdriver upd_cd = {
NULL, upd, DV_DULL
@@ -183,38 +207,30 @@ upd_attach(struct device *parent, struct
sc-sc_num_sensors sc-sc_max_sensors; ) {
DPRINTF((upd: repid=%d\n, item.report_ID));
if (item.kind != hid_feature ||
- item.report_ID 0)
+ item.report_ID 0 ||
+ item.report_ID = sc-sc_max_repid)
continue;
-
if ((entry
Re: Async upd(4)
On Mar 3, 2015, at 8:44 AM, David Higgs hig...@gmail.com wrote:
With much help from mpi@, I have made a first big step towards improving
upd(4). I’m not sure when tree lock ends, but I’m still happy to accept
feedback if right now isn’t the time to commit. There’s plenty more to do,
but I’d like to get this ironed out before moving on.
New behavior with this diff:
- Leverage new USB async reporting (must have up-to-date tree)
- Sensor dependencies ensure reports are gathered in useful order
- Track pending reports to minimize USB queries
- Relevant sensors immediately invalidated when battery is removed (instead
of waiting for the next refresh)
Things that may need sanity checks:
- Simplified upd_attach; the old code seemed to have redundant / confusing
logic
- Integer promotion/truncation with batpres and hdata/adjust variables
Below is an overhauled diff with some additional considerations I came up with.
Improvements on the previous version:
- ACPresent no longer depends on BatteryPresent
- Sensor dependencies now handled manually, so dynamic behavior can be added
later.
- Avoid hypothetical cases where certain USB report layouts could trigger:
- Infinite loops in sensor dependencies.
- Updating sensor contents using stale information.
- Unnecessary sensor invalidation.
- Redundant USB transfers.
As before, comments, questions, and feedback are welcome.
--david
Index: upd.c
===
RCS file: /cvs/src/sys/dev/usb/upd.c,v
retrieving revision 1.13
diff -u -p -r1.13 upd.c
--- upd.c 11 Jan 2015 03:08:38 - 1.13
+++ upd.c 5 Mar 2015 17:06:19 -
@@ -39,45 +39,17 @@
#define DPRINTF(x)
#endif
-struct upd_usage_entry {
- uint8_t usage_pg;
- uint8_t usage_id;
- enum sensor_typesenstype;
- char*usage_name; /* sensor string */
-};
-
-static struct upd_usage_entry upd_usage_table[] = {
- { HUP_BATTERY, HUB_REL_STATEOF_CHARGE,
- SENSOR_PERCENT, RelativeStateOfCharge },
- { HUP_BATTERY, HUB_ABS_STATEOF_CHARGE,
- SENSOR_PERCENT, AbsoluteStateOfCharge },
- { HUP_BATTERY, HUB_REM_CAPACITY,
- SENSOR_PERCENT, RemainingCapacity },
- { HUP_BATTERY, HUB_FULLCHARGE_CAPACITY,
- SENSOR_PERCENT, FullChargeCapacity },
- { HUP_BATTERY, HUB_CHARGING,
- SENSOR_INDICATOR,Charging },
- { HUP_BATTERY, HUB_DISCHARGING,
- SENSOR_INDICATOR,Discharging },
- { HUP_BATTERY, HUB_BATTERY_PRESENT,
- SENSOR_INDICATOR,BatteryPresent },
- { HUP_POWER,HUP_SHUTDOWN_IMMINENT,
- SENSOR_INDICATOR,ShutdownImminent },
- { HUP_BATTERY, HUB_AC_PRESENT,
- SENSOR_INDICATOR,ACPresent },
- { HUP_BATTERY, HUB_ATRATE_TIMETOFULL,
- SENSOR_TIMEDELTA,AtRateTimeToFull }
-};
-
struct upd_report {
size_t size;
- int enabled;
+ int pending;
};
struct upd_sensor {
struct ksensor ksensor;
struct hid_item hitem;
+ struct upd_usage_entry *entry;
int attached;
+ int pending;
};
struct upd_softc {
@@ -85,6 +57,7 @@ struct upd_softc {
int sc_num_sensors;
u_intsc_max_repid;
u_intsc_max_sensors;
+ char sc_buf[256];
/* sensor framework */
struct ksensordevsc_sensordev;
@@ -93,15 +66,66 @@ struct upd_softc {
struct upd_sensor *sc_sensors;
};
+struct upd_usage_entry {
+ uint8_t usage_pg;
+ uint8_t usage_id;
+ enum sensor_typesenstype;
+ char*usage_name; /* sensor string */
+ void (*update_sensor)(struct upd_softc *, struct upd_sensor *,
+ uint8_t *, int);
+};
+
int upd_match(struct device *, void *, void *);
void upd_attach(struct device *, struct device *, void *);
int upd_detach(struct device *, int);
void upd_refresh(void *);
-void upd_update_sensors(struct upd_softc *, uint8_t *, unsigned int, int);
-void upd_intr(struct uhidev *, void *, uint);
+void upd_request_sensor_refresh(struct upd_softc *, uint8_t, uint8_t, int);
+void upd_update_sensor_cb(void *, int, void *, int);
+void upd_mark_sensor_invalid(struct upd_softc *, struct upd_sensor *, int);
struct upd_usage_entry *upd_lookup_usage_entry(const struct hid_item *);
struct upd_sensor *upd_lookup_sensor(struct upd_softc *, int, int);
+int upd_battery_present(struct upd_softc *);
+void upd_update_batpres_sensor(struct upd_softc *, struct upd_sensor *,
+uint8_t *, int);
+void upd_update_batdep_sensor(struct upd_softc *, struct upd_sensor *,
+uint8_t *, int);
+void
Async upd(4)
With much help from mpi@, I have made a first big step towards improving
upd(4). I’m not sure when tree lock ends, but I’m still happy to accept
feedback if right now isn’t the time to commit. There’s plenty more to do, but
I’d like to get this ironed out before moving on.
New behavior with this diff:
- Leverage new USB async reporting (must have up-to-date tree)
- Sensor dependencies ensure reports are gathered in useful order
- Track pending reports to minimize USB queries
- Relevant sensors immediately invalidated when battery is removed (instead of
waiting for the next refresh)
Things that may need sanity checks:
- Simplified upd_attach; the old code seemed to have redundant / confusing logic
- Integer promotion/truncation with batpres and hdata/adjust variables
Suggestions, bug reports, and feedback welcome.
--david
Index: upd.c
===
RCS file: /cvs/src/sys/dev/usb/upd.c,v
retrieving revision 1.13
diff -u -p -r1.13 upd.c
--- upd.c 11 Jan 2015 03:08:38 - 1.13
+++ upd.c 20 Feb 2015 02:28:04 -
@@ -39,44 +39,15 @@
#define DPRINTF(x)
#endif
-struct upd_usage_entry {
- uint8_t usage_pg;
- uint8_t usage_id;
- enum sensor_typesenstype;
- char*usage_name; /* sensor string */
-};
-
-static struct upd_usage_entry upd_usage_table[] = {
- { HUP_BATTERY, HUB_REL_STATEOF_CHARGE,
- SENSOR_PERCENT, RelativeStateOfCharge },
- { HUP_BATTERY, HUB_ABS_STATEOF_CHARGE,
- SENSOR_PERCENT, AbsoluteStateOfCharge },
- { HUP_BATTERY, HUB_REM_CAPACITY,
- SENSOR_PERCENT, RemainingCapacity },
- { HUP_BATTERY, HUB_FULLCHARGE_CAPACITY,
- SENSOR_PERCENT, FullChargeCapacity },
- { HUP_BATTERY, HUB_CHARGING,
- SENSOR_INDICATOR,Charging },
- { HUP_BATTERY, HUB_DISCHARGING,
- SENSOR_INDICATOR,Discharging },
- { HUP_BATTERY, HUB_BATTERY_PRESENT,
- SENSOR_INDICATOR,BatteryPresent },
- { HUP_POWER,HUP_SHUTDOWN_IMMINENT,
- SENSOR_INDICATOR,ShutdownImminent },
- { HUP_BATTERY, HUB_AC_PRESENT,
- SENSOR_INDICATOR,ACPresent },
- { HUP_BATTERY, HUB_ATRATE_TIMETOFULL,
- SENSOR_TIMEDELTA,AtRateTimeToFull }
-};
-
struct upd_report {
size_t size;
- int enabled;
+ int pending;
};
struct upd_sensor {
struct ksensor ksensor;
struct hid_item hitem;
+ struct upd_usage_entry *entry;
int attached;
};
@@ -85,6 +56,7 @@ struct upd_softc {
int sc_num_sensors;
u_intsc_max_repid;
u_intsc_max_sensors;
+ char sc_buf[256];
/* sensor framework */
struct ksensordevsc_sensordev;
@@ -93,6 +65,17 @@ struct upd_softc {
struct upd_sensor *sc_sensors;
};
+struct upd_usage_entry {
+ uint8_t usage_pg;
+ uint8_t usage_id;
+ uint8_t parent_pg;
+ uint8_t parent_id;
+ enum sensor_typesenstype;
+ char*usage_name; /* sensor string */
+ void (*update_sensor)(struct upd_softc *, struct upd_sensor *,
+ uint8_t *, int);
+};
+
int upd_match(struct device *, void *, void *);
void upd_attach(struct device *, struct device *, void *);
int upd_detach(struct device *, int);
@@ -100,8 +83,58 @@ int upd_detach(struct device *, int);
void upd_refresh(void *);
void upd_update_sensors(struct upd_softc *, uint8_t *, unsigned int, int);
void upd_intr(struct uhidev *, void *, uint);
+void upd_refresh_sensor_tree(struct upd_softc *, uint8_t, uint8_t, int);
+void upd_get_report_async_cb(void *, int, void *, int);
struct upd_usage_entry *upd_lookup_usage_entry(const struct hid_item *);
struct upd_sensor *upd_lookup_sensor(struct upd_softc *, int, int);
+void upd_update_batdep_sensor(struct upd_softc *, struct upd_sensor *,
+uint8_t *, int);
+void upd_update_sensor_value(struct upd_softc *, struct upd_sensor *,
+uint8_t *, int);
+
+/* dependency order only for developer sanity */
+static struct upd_usage_entry upd_usage_table[] = {
+ { HUP_POWER,HUP_SHUTDOWN_IMMINENT,
+ HUP_UNDEFINED, 0,
+ SENSOR_INDICATOR, ShutdownImminent,
+ upd_update_sensor_value },
+ { HUP_BATTERY, HUB_BATTERY_PRESENT,
+ HUP_UNDEFINED, 0,
+ SENSOR_INDICATOR, BatteryPresent,
+ upd_update_sensor_value },
+ { HUP_BATTERY, HUB_AC_PRESENT,
+ HUP_UNDEFINED, 0,
+ SENSOR_INDICATOR, ACPresent,
+ upd_update_sensor_value },
+ { HUP_BATTERY,
Re: USBD_NO_COPY problems
On Feb 13, 2015, at 7:29 AM, David Higgs hig...@gmail.com wrote:
On Friday, February 13, 2015, Martin Pieuchot mpieuc...@nolizard.org wrote:
On 13/02/15(Fri) 00:28, David Higgs wrote:
I guess nobody else has tried calling uhidev_get_report_async() yet. :)
First I was getting a NULL pointer deref in the uhidev async callback.
Then I realized that due to USBD_NO_COPY, xfer-buffer was always
NULL. Next, I tried to use the DMA buffer, but I ended up in DDB in a
very cryptic way. I believe this is because the DMA buffer isn't
available when the callback is invoked.
For the async callback to get a valid dmabuf, it needs to be invoked
prior to usb_freemem() in usbd_transfer_complete(). The xfer-status
determination would need to move up too. I'd do this myself but I
don't understand the logic and ordering of pipe-repeat stuff, and am
concerned about unintentionally breaking other devices.
This is partially my fault, because I tested the original diff that
added the USBD_NO_COPY semantics to verify that it didn't break my
synchronous code paths, but hadn't yet written anything for upd(4) to
check the async ones.
Does the diff below help?
Partially but not enough. I had already figured out that I needed that to
solve the NULL pointer dereference. See my 2nd paragraph above.
OK, I figured out my issue - the crazy DDB backtrace is produced when you
execute a NULL callback.
It still doesn’t seem legal for the callback to access DMA buffer contents
after they are “freed”. I assume this won’t work in all cases (host
controllers / architectures / cache behaviors), but I don’t experience any
problems in my i386 VM. I tried reordering parts of usbd_transfer_complete(),
but DIAGNOSTIC code became very unhappy with the results.
Fortunately, the diff below doesn’t touch that code path and just fixes the
uhidev layer. My async upd(4) changes will be forthcoming in a different
thread.
Thanks.
--david
Index: uhidev.c
===
RCS file: /cvs/src/sys/dev/usb/uhidev.c,v
retrieving revision 1.69
diff -u -p -r1.69 uhidev.c
--- uhidev.c22 Jan 2015 10:27:47 - 1.69
+++ uhidev.c20 Feb 2015 02:27:29 -
@@ -53,6 +53,7 @@
#include dev/usb/usbdi.h
#include dev/usb/usbdi_util.h
#include dev/usb/usbdivar.h
+#include dev/usb/usb_mem.h
#include dev/usb/hid.h
#include dev/usb/usb_quirks.h
@@ -747,15 +748,17 @@ void
uhidev_get_report_async_cb(struct usbd_xfer *xfer, void *priv, usbd_status err)
{
struct uhidev_async_info *info = priv;
+ char *buf;
int len = -1;
if (err == USBD_NORMAL_COMPLETION || err == USBD_SHORT_XFER) {
len = xfer-actlen;
+ buf = KERNADDR(xfer-dmabuf, 0);
if (info-id 0) {
len--;
- memcpy(info-data, xfer-buffer + 1, len);
+ memcpy(info-data, buf + 1, len);
} else {
- memcpy(info-data, xfer-buffer, len);
+ memcpy(info-data, buf, len);
}
}
info-callback(info-priv, info-id, info-data, len);
@@ -803,7 +806,7 @@ uhidev_get_report_async(struct uhidev_so
USETW(req.wIndex, sc-sc_ifaceno);
USETW(req.wLength, len);
- if (usbd_request_async(xfer, req, priv, uhidev_get_report_async_cb)) {
+ if (usbd_request_async(xfer, req, info, uhidev_get_report_async_cb)) {
free(info, M_TEMP, sizeof(*info));
actlen = -1;
}
Re: USBD_NO_COPY problems
On Feb 13, 2015, at 7:29 AM, David Higgs hig...@gmail.com wrote:
On Friday, February 13, 2015, Martin Pieuchot mpieuc...@nolizard.org wrote:
On 13/02/15(Fri) 00:28, David Higgs wrote:
I guess nobody else has tried calling uhidev_get_report_async() yet. :)
First I was getting a NULL pointer deref in the uhidev async callback.
Then I realized that due to USBD_NO_COPY, xfer-buffer was always
NULL. Next, I tried to use the DMA buffer, but I ended up in DDB in a
very cryptic way. I believe this is because the DMA buffer isn't
available when the callback is invoked.
For the async callback to get a valid dmabuf, it needs to be invoked
prior to usb_freemem() in usbd_transfer_complete(). The xfer-status
determination would need to move up too. I'd do this myself but I
don't understand the logic and ordering of pipe-repeat stuff, and am
concerned about unintentionally breaking other devices.
This is partially my fault, because I tested the original diff that
added the USBD_NO_COPY semantics to verify that it didn't break my
synchronous code paths, but hadn't yet written anything for upd(4) to
check the async ones.
Does the diff below help?
Partially but not enough. I had already figured out that I needed that to
solve the NULL pointer dereference. See my 2nd paragraph above.
Below is the diff I’m currently running with, seeing the same problems. This
may actually be indicative of some error in my big upd(4) update - which I
didn’t include - but I’m not sure how to find out for sure because I’m not sure
how to interpret the DDB trace (hand-copied, may have errors). There were two
async requests pending when I got kicked into the debugger. Any help or
suggestions would be very welcome at this point.
Thanks.
--david
uvm_fault(0xd565f480, 0x0, 0, 1) - e
kernel: page fault trap, code = 0
Stopped at 0:uvm_fault(0xd565f480, 0x0, 0, 1) - e
kernel: page fault trap, code = 0
Stopped at db_read_bytes+0x17:movzbl 0(%esi,%ecx,1),%eax
ddb trace
db_read_bytes(0,1,f375ea44,0,f375ea54) at db_read_bytes+0x17
db_get_value(0,1,0,0,d09e1ada) at db_get_value+0x38
db_disasm(0,0,d03cc470,d03cc495,d09b6b38,f375eb14,0,0,f375eb14) at
db_disasm+0x31
db_print_loc_and_inst(0,f375,eb2c,f375eb38,d03cc495,d09e1acb) at
db_print_loc_and_inst+0x3e
db_trap(6,0,58,0,f375eb74) at db_trap+0x89
kdb_trap(6,0,f375ebe4,1,e) at kdb_trap+0xcc
trap() at trap+0x2e5
— trap (number 0) —
Bad frame pointer: 0xd56641e0
0:
ddb
Index: uhidev.c
===
RCS file: /cvs/src/sys/dev/usb/uhidev.c,v
retrieving revision 1.69
diff -u -p -r1.69 uhidev.c
--- uhidev.c22 Jan 2015 10:27:47 - 1.69
+++ uhidev.c14 Feb 2015 15:18:47 -
@@ -53,6 +53,7 @@
#include dev/usb/usbdi.h
#include dev/usb/usbdi_util.h
#include dev/usb/usbdivar.h
+#include dev/usb/usb_mem.h
#include dev/usb/hid.h
#include dev/usb/usb_quirks.h
@@ -747,15 +748,17 @@ void
uhidev_get_report_async_cb(struct usbd_xfer *xfer, void *priv, usbd_status err)
{
struct uhidev_async_info *info = priv;
+ char *buf;
int len = -1;
if (err == USBD_NORMAL_COMPLETION || err == USBD_SHORT_XFER) {
len = xfer-actlen;
+ buf = KERNADDR(xfer-dmabuf, 0);
if (info-id 0) {
len--;
- memcpy(info-data, xfer-buffer + 1, len);
+ memcpy(info-data, buf + 1, len);
} else {
- memcpy(info-data, xfer-buffer, len);
+ memcpy(info-data, buf, len);
}
}
info-callback(info-priv, info-id, info-data, len);
Index: usbdi.c
===
RCS file: /cvs/src/sys/dev/usb/usbdi.c,v
retrieving revision 1.80
diff -u -p -r1.80 usbdi.c
--- usbdi.c 12 Feb 2015 05:07:52 - 1.80
+++ usbdi.c 14 Feb 2015 15:18:47 -
@@ -748,6 +748,15 @@ usb_transfer_complete(struct usbd_xfer *
memcpy(xfer-buffer, KERNADDR(xfer-dmabuf, 0), xfer-actlen);
}
+ if (!xfer-status xfer-actlen xfer-length
+ !(xfer-flags USBD_SHORT_XFER_OK)) {
+ DPRINTFN(-1,(usb_transfer_complete: short transfer %d%d\n,
+ xfer-actlen, xfer-length));
+ xfer-status = USBD_SHORT_XFER;
+ }
+ if (xfer-callback)
+ xfer-callback(xfer, xfer-priv, xfer-status);
+
/* if we allocated the buffer in usbd_transfer() we free it here. */
if (xfer-rqflags URQ_AUTO_DMABUF) {
if (!pipe-repeat) {
@@ -774,22 +783,7 @@ usb_transfer_complete(struct usbd_xfer *
[pipe-endpoint-edesc-bmAttributes UE_XFERTYPE];
xfer-done = 1;
- if (!xfer-status xfer-actlen xfer-length
- !(xfer-flags USBD_SHORT_XFER_OK)) {
- DPRINTFN(-1,(usb_transfer_complete: short
Re: USBD_NO_COPY problems
On Friday, February 13, 2015, Martin Pieuchot mpieuc...@nolizard.org
wrote:
On 13/02/15(Fri) 00:28, David Higgs wrote:
I guess nobody else has tried calling uhidev_get_report_async() yet. :)
First I was getting a NULL pointer deref in the uhidev async callback.
Then I realized that due to USBD_NO_COPY, xfer-buffer was always
NULL. Next, I tried to use the DMA buffer, but I ended up in DDB in a
very cryptic way. I believe this is because the DMA buffer isn't
available when the callback is invoked.
For the async callback to get a valid dmabuf, it needs to be invoked
prior to usb_freemem() in usbd_transfer_complete(). The xfer-status
determination would need to move up too. I'd do this myself but I
don't understand the logic and ordering of pipe-repeat stuff, and am
concerned about unintentionally breaking other devices.
This is partially my fault, because I tested the original diff that
added the USBD_NO_COPY semantics to verify that it didn't break my
synchronous code paths, but hadn't yet written anything for upd(4) to
check the async ones.
Does the diff below help?
Partially but not enough. I had already figured out that I needed that
to solve the NULL pointer dereference. See my 2nd paragraph above.
Index: uhidev.c
===
RCS file: /cvs/src/sys/dev/usb/uhidev.c,v
retrieving revision 1.69
diff -u -p -r1.69 uhidev.c
--- uhidev.c22 Jan 2015 10:27:47 - 1.69
+++ uhidev.c13 Feb 2015 05:58:06 -
@@ -55,6 +55,7 @@
#include dev/usb/usbdivar.h
#include dev/usb/hid.h
#include dev/usb/usb_quirks.h
+#include dev/usb/usb_mem.h
#include dev/usb/uhidev.h
@@ -747,15 +748,17 @@ void
uhidev_get_report_async_cb(struct usbd_xfer *xfer, void *priv,
usbd_status err)
{
struct uhidev_async_info *info = priv;
+ char *buffer;
int len = -1;
if (err == USBD_NORMAL_COMPLETION || err == USBD_SHORT_XFER) {
+ buffer = KERNADDR(xfer-dmabuf, 0);
len = xfer-actlen;
if (info-id 0) {
len--;
- memcpy(info-data, xfer-buffer + 1, len);
+ memcpy(info-data, buffer + 1, len);
} else {
- memcpy(info-data, xfer-buffer, len);
+ memcpy(info-data, buffer, len);
}
}
info-callback(info-priv, info-id, info-data, len);
USBD_NO_COPY problems
I guess nobody else has tried calling uhidev_get_report_async() yet. :) First I was getting a NULL pointer deref in the uhidev async callback. Then I realized that due to USBD_NO_COPY, xfer-buffer was always NULL. Next, I tried to use the DMA buffer, but I ended up in DDB in a very cryptic way. I believe this is because the DMA buffer isn't available when the callback is invoked. For the async callback to get a valid dmabuf, it needs to be invoked prior to usb_freemem() in usbd_transfer_complete(). The xfer-status determination would need to move up too. I'd do this myself but I don't understand the logic and ordering of pipe-repeat stuff, and am concerned about unintentionally breaking other devices. This is partially my fault, because I tested the original diff that added the USBD_NO_COPY semantics to verify that it didn't break my synchronous code paths, but hadn't yet written anything for upd(4) to check the async ones. Thanks. --david
Re: Fewer malloc() memcpy() in USB land
On Fri, Jan 9, 2015 at 2:07 PM, Martin Pieuchot mpieuc...@nolizard.org wrote:
On 09/01/15(Fri) 12:36, David Higgs wrote:
On Fri, Jan 9, 2015 at 9:00 AM, Martin Pieuchot mpieuc...@nolizard.org
wrote:
As pointed out by David Higgs, uhidev report functions do a lot of
memory allocations and copies between buffers. This is not uncommon
in USB land due to way DMA buffers are handled.
One way to reduce the number of copies is to submit a transfer with
the USBD_NO_COPY flag. Without it the kernel does two to three copies:
(stack/driver array --) malloc'ed array -- DMA reacheable array
This is not limited to uhidev functions. That's also done for every
transfer sent through an interrupt pipe or for every control request
with data.
I'd like to slowly change the stack such that USBD_NO_COPY becomes the
default behavior. This would first reduces the number of buffer copies
and then allow umass(4) to directly use the DMA reachable buffers
provided by the SCSI stack, which should *at least* reduce the CPU usage
when using USB drives.
Here's a diff about uhidev(4) to illustrate the changes I'm talking
about.
Comments, ok?
See inline. Reviewed to the best of my ability/eyesight via
variable-width webmail.
[...]
+
+ if (id 0)
+ len++;
+
+ buf = usbd_alloc_buffer(xfer, len);
+ if (buf == NULL)
+ return (-1);
You forgot to free xfer in this error path.
Indeed!
@@ -586,6 +585,7 @@ usbd_status
usbd_clear_endpoint_stall_async(struct usbd_pipe *pipe)
{
struct usbd_device *dev = pipe-device;
+ struct usbd_xfer *xfer;
usb_device_request_t req;
usbd_status err;
@@ -596,7 +596,12 @@ usbd_clear_endpoint_stall_async(struct u
USETW(req.wValue, UF_ENDPOINT_HALT);
USETW(req.wIndex, pipe-endpoint-edesc-bEndpointAddress);
USETW(req.wLength, 0);
- err = usbd_do_request_async(dev, req, 0, 0, 0);
+
+ xfer = usbd_alloc_xfer(dev);
+ if (xfer == NULL)
+ return (USBD_NOMEM);
+
+ err = usbd_request_async(xfer, req, NULL, NULL);
Missing xfer cleanup here too, I think.
Here it's fine, as soon as you call usbd_request_async() the xfer will be
freed.
Updated diff below.
The updated diff appears to works for me and I don't see any further
issues upon review. No behavior changes when applied to a snapshot +
HEAD kernel as of this evening. Tested via repeated sysctl reads of
upd(4) sensor info and a few device detach/attach events.
--david
Re: Fewer malloc() memcpy() in USB land
On Fri, Jan 9, 2015 at 9:00 AM, Martin Pieuchot mpieuc...@nolizard.org wrote:
As pointed out by David Higgs, uhidev report functions do a lot of
memory allocations and copies between buffers. This is not uncommon
in USB land due to way DMA buffers are handled.
One way to reduce the number of copies is to submit a transfer with
the USBD_NO_COPY flag. Without it the kernel does two to three copies:
(stack/driver array --) malloc'ed array -- DMA reacheable array
This is not limited to uhidev functions. That's also done for every
transfer sent through an interrupt pipe or for every control request
with data.
I'd like to slowly change the stack such that USBD_NO_COPY becomes the
default behavior. This would first reduces the number of buffer copies
and then allow umass(4) to directly use the DMA reachable buffers
provided by the SCSI stack, which should *at least* reduce the CPU usage
when using USB drives.
Here's a diff about uhidev(4) to illustrate the changes I'm talking
about.
Comments, ok?
See inline. Reviewed to the best of my ability/eyesight via
variable-width webmail.
Index: uhidev.c
===
RCS file: /cvs/src/sys/dev/usb/uhidev.c,v
retrieving revision 1.68
diff -u -p -r1.68 uhidev.c
--- uhidev.c9 Jan 2015 12:09:51 - 1.68
+++ uhidev.c9 Jan 2015 13:31:36 -
@@ -671,18 +671,28 @@ int
uhidev_set_report_async(struct uhidev_softc *sc, int type, int id, void
*data,
int len)
{
+ struct usbd_xfer *xfer;
usb_device_request_t req;
- char *buf = data;
int actlen = len;
+ char *buf;
+
+ xfer = usbd_alloc_xfer(sc-sc_udev);
+ if (xfer == NULL)
+ return (-1);
+
+ if (id 0)
+ len++;
+
+ buf = usbd_alloc_buffer(xfer, len);
+ if (buf == NULL)
+ return (-1);
You forgot to free xfer in this error path.
/* Prepend the reportID. */
if (id 0) {
- len++;
- buf = malloc(len, M_TEMP, M_NOWAIT);
- if (buf == NULL)
- return (-1);
buf[0] = id;
memcpy(buf + 1, data, len - 1);
+ } else {
+ memcpy(buf, data, len);
}
req.bmRequestType = UT_WRITE_CLASS_INTERFACE;
@@ -691,17 +701,9 @@ uhidev_set_report_async(struct uhidev_so
USETW(req.wIndex, sc-sc_ifaceno);
USETW(req.wLength, len);
- if (usbd_do_request_async(sc-sc_udev, req, buf, NULL, NULL))
+ if (usbd_request_async(xfer, req, NULL, NULL))
actlen = -1;
- /*
-* Since report requests are write-only it is safe to free
-* the buffer right after submitting the transfer because
-* it won't be used afterward.
-*/
- if (id 0)
- free(buf, M_TEMP, len);
-
return (actlen);
}
@@ -750,11 +752,11 @@ uhidev_get_report_async_cb(struct usbd_x
if (info-id 0) {
len--;
memcpy(info-data, xfer-buffer + 1, len);
+ } else {
+ memcpy(info-data, xfer-buffer, len);
}
}
info-callback(info-priv, info-id, info-data, len);
- if (info-id 0)
- free(xfer-buffer, M_TEMP, xfer-length);
free(info, M_TEMP, sizeof(*info));
usbd_free_xfer(xfer);
}
@@ -763,42 +765,48 @@ int
uhidev_get_report_async(struct uhidev_softc *sc, int type, int id, void
*data,
int len, void *priv, void (*callback)(void *, int, void *, int))
{
+ struct usbd_xfer *xfer;
usb_device_request_t req;
struct uhidev_async_info *info;
- char *buf = data;
int actlen = len;
+ void *buf;
+
+ xfer = usbd_alloc_xfer(sc-sc_udev);
+ if (xfer == NULL)
+ return (-1);
+
+ if (id 0)
+ len++;
+
+ buf = usbd_alloc_buffer(xfer, len);
+ if (buf == NULL) {
+ usbd_free_xfer(xfer);
+ return (-1);
+ }
info = malloc(sizeof(*info), M_TEMP, M_NOWAIT);
- if (info == NULL)
+ if (info == NULL) {
+ usbd_free_xfer(xfer);
return (-1);
+ }
info-callback = callback;
info-priv = priv;
info-data = data;
info-id = id;
- if (id 0) {
- len++;
- buf = malloc(len, M_TEMP, M_NOWAIT|M_ZERO);
- if (buf == NULL) {
- free(info, M_TEMP, sizeof(*info));
- return (-1);
- }
- }
-
req.bmRequestType = UT_READ_CLASS_INTERFACE;
req.bRequest = UR_GET_REPORT;
USETW2(req.wValue, type, id);
USETW(req.wIndex, sc-sc_ifaceno);
USETW(req.wLength, len
Re: Fix upd(4) sensor refresh
On Fri, Dec 19, 2014 at 8:55 AM, Martin Pieuchot mpieuc...@nolizard.org wrote: On 19/12/14(Fri) 08:04, David Higgs wrote: Split upd_update_sensors() behavior to gather all values prior to updating sensor contents. Because sensors were updated in report_ID order, it could take multiple passes of upd_refresh() to update some sensors. Specifically, battery-related sensors prior to a change in BatteryPresent would be stale/incorrect until the 2nd call to upd_refresh(). Also, change a confusing DPRINTF of report_ID from un-prefixed hex to decimal, to be consistent. What you're saying is that querying HUB_BATTERY_PRESENT after 6 sensors that depend on its value does not make sense, right? Correct. Well in this case I don't think that sensors should be queried in reportID order. I even don't think that sensors should be queried at all if we're going to discard the result anyway. It might not matter that much right now, but if the number of queried sensors grow s significantly we might end up waiting a lot of time in upd_refresh() prior to updating anything. Querying by report_ID minimizes the number of requests, because multiple sensors can be batched into the same report_ID. I don't know if there are any rules that determine how items are batched, so there may be pathologically bad results. This fix is needed for future improvements, where output or behavior of certain sensors depends on the current value of more than one report (e.g. RemainingCapacity should actually depend on CapacityMode). But do you need to read the value of CapacityMode more than once, at the begging or when you modify it? True. CapacityMode shouldn't automatically change and can be read once (and/or maybe updated) at attach-time. I see no reason to expose this setting for users to change. Anyway, when we discussed that with Andre, I suggested to have a table containing all sensors depending on an other one. This way you can start by querying HUB_BATTERY_PRESENT and if it is present, query the others. Another advantage is that it will simplify the code because all the sensors in a table will be in the same page. This sounds good. I will revisit the Power/Battery spec and see what future sensors might require dependent behavior. Since you're doing some great job with upd(4), here's another idea. Did you consider updating the values of the sensors via an asynchronous function and a callback? This way the thread does not need to way for the previous queries. That would involve adding a new function, let's say uhidev_get_report_async() that takes a custom callback since there's no such API for the moment. This sounds like a good idea, but batched report_ID values can produce redundant USB queries or will require additional logic/caching to minimize this redundancy. I'm new to all this, and am not sure how to decide which trade-offs to make. Thanks. --david
Re: Better/more upd(4) timedelta sensors
On Fri, Dec 19, 2014 at 7:17 AM, Martin Pieuchot mpieuc...@nolizard.org wrote: Hello David, On 18/12/14(Thu) 00:45, David Higgs wrote: While my device does not seem to provide AtRateTimeToFull or AtRateTimeToEmpty, it does have RunTimeToEmpty. Then I found that SENSOR_TIMEDELTA values are in nanoseconds and that scaling for them was never implemented correctly. I am confused by the spec [1], though; see 4.2.5 - Battery Measures. The reported values are supposedly in minutes but hid_info.unit is 0x1001 (seconds) and observation of RunTimeToEmpty appears to agree. I don't see any other drivers in the tree that pay attention to unit or unit_exponent fields, and don't want to go down a rabbit hole if there's no interest. As usual, feedback is welcome. Table 1 of section 3.2.3 indicates that the physical unit for time reports are in seconds. So I guess it is a mistake in the document. I like your diff but I'd prefer to see the scaling done based on the unit rather than the name. Why not adding new defines for the various Physical Unit defined in the table and do the switch on hitem.unit? And what about the exponent? Thanks for looking at this. I've rethought my approach to upd(4) a bit, and will be addressing bugs prior to adding new sensors. There will be a diff to address units in a more coherent fashion soon. --david
Fix upd(4) sensor refresh
Split upd_update_sensors() behavior to gather all values prior to updating
sensor contents.
Because sensors were updated in report_ID order, it could take multiple passes
of upd_refresh() to update some sensors. Specifically, battery-related sensors
“prior” to a change in BatteryPresent would be stale/incorrect until the 2nd
call to upd_refresh(). Also, change a confusing DPRINTF of report_ID from
un-prefixed hex to decimal, to be consistent.
This fix is needed for future improvements, where output or behavior of certain
sensors depends on the current value of more than one report (e.g.
RemainingCapacity should actually depend on CapacityMode).
As always, feedback is welcome.
--david
Index: upd.c
===
RCS file: /cvs/src/sys/dev/usb/upd.c,v
retrieving revision 1.12
diff -u -p -r1.12 upd.c
--- upd.c 11 Dec 2014 18:50:32 - 1.12
+++ upd.c 19 Dec 2014 05:33:14 -
@@ -77,6 +77,8 @@ struct upd_report {
struct upd_sensor {
struct ksensor ksensor;
struct hid_item hitem;
+ int raw_value;
+ int value_read;
int attached;
};
@@ -98,7 +100,8 @@ void upd_attach(struct device *, struct
int upd_detach(struct device *, int);
void upd_refresh(void *);
-void upd_update_sensors(struct upd_softc *, uint8_t *, unsigned int, int);
+void upd_update_values(struct upd_softc *, uint8_t *, unsigned int, int);
+void upd_update_sensors(struct upd_softc *);
void upd_intr(struct uhidev *, void *, uint);
struct upd_usage_entry *upd_lookup_usage_entry(const struct hid_item *);
struct upd_sensor *upd_lookup_sensor(struct upd_softc *, int, int);
@@ -275,7 +278,7 @@ upd_refresh(void *arg)
UHID_FEATURE_REPORT, repid, buf, report-size);
if (actlen == -1) {
- DPRINTF((upd: failed to get report id=%02x\n, repid));
+ DPRINTF((upd: failed to get report id=%d\n, repid));
continue;
}
@@ -283,8 +286,10 @@ upd_refresh(void *arg)
if (actlen report-size)
report-size = actlen;
- upd_update_sensors(sc, buf, report-size, repid);
+ upd_update_values(sc, buf, report-size, repid);
}
+
+ upd_update_sensors(sc);
}
struct upd_usage_entry *
@@ -318,33 +323,48 @@ upd_lookup_sensor(struct upd_softc *sc,
}
void
-upd_update_sensors(struct upd_softc *sc, uint8_t *buf, unsigned int len,
+upd_update_values(struct upd_softc *sc, uint8_t *buf, unsigned int len,
int repid)
{
struct upd_sensor *sensor;
- ulong hdata, batpres;
- ulong adjust;
int i;
+ for (i = 0; i sc-sc_num_sensors; i++) {
+ sensor = sc-sc_sensors[i];
+ if (sensor-hitem.report_ID == repid sensor-attached) {
+ sensor-value_read = 1;
+ sensor-raw_value =
+ hid_get_data(buf, len, sensor-hitem.loc);
+ DPRINTF((%s: repid=%d, %s value=%d\n,
+ sc-sc_sensordev.xname, repid,
+ sensor-ksensor.desc, sensor-raw_value));
+ }
+ }
+}
+
+void
+upd_update_sensors(struct upd_softc *sc)
+{
+ struct upd_sensor *sensor;
+ ulong adjust;
+ int i, batpres = 0;
+
sensor = upd_lookup_sensor(sc, HUP_BATTERY, HUB_BATTERY_PRESENT);
- batpres = sensor ? sensor-ksensor.value : -1;
+ if (sensor sensor-value_read)
+ batpres = sensor-raw_value;
for (i = 0; i sc-sc_num_sensors; i++) {
sensor = sc-sc_sensors[i];
- if (!(sensor-hitem.report_ID == repid sensor-attached))
+ sensor-ksensor.flags |= SENSOR_FINVALID;
+ sensor-ksensor.status = SENSOR_S_UNKNOWN;
+ if (!sensor-value_read)
continue;
- /* invalidate battery dependent sensors */
+ /* ignore battery sensors when no battery present */
if (HID_GET_USAGE_PAGE(sensor-hitem.usage) == HUP_BATTERY
- batpres = 0) {
- /* exception to the battery sensor itself */
- if (HID_GET_USAGE(sensor-hitem.usage) !=
- HUB_BATTERY_PRESENT) {
- sensor-ksensor.status = SENSOR_S_UNKNOWN;
- sensor-ksensor.flags |= SENSOR_FINVALID;
- continue;
- }
- }
+ HID_GET_USAGE(sensor-hitem.usage) != HUB_BATTERY_PRESENT
+ !batpres)
+ continue;
switch
Unit descriptions in sensors.h
For consistency.
--david
Index: sensors.h
===
RCS file: /cvs/src/sys/sys/sensors.h,v
retrieving revision 1.33
diff -u -p -r1.33 sensors.h
--- sensors.h 4 Nov 2013 02:41:49 - 1.33
+++ sensors.h 18 Dec 2014 18:16:07 -
@@ -38,11 +38,11 @@ enum sensor_type {
SENSOR_OHMS,/* resistance */
SENSOR_WATTS, /* power (muW) */
SENSOR_AMPS,/* current (muA) */
- SENSOR_WATTHOUR,/* power capacity */
- SENSOR_AMPHOUR, /* power capacity */
+ SENSOR_WATTHOUR,/* power capacity (muWh) */
+ SENSOR_AMPHOUR, /* power capacity (muWh) */
SENSOR_INDICATOR, /* boolean indicator */
SENSOR_INTEGER, /* generic integer value */
- SENSOR_PERCENT, /* percent */
+ SENSOR_PERCENT, /* percent (m%) */
SENSOR_LUX, /* illuminance (mulx) */
SENSOR_DRIVE, /* disk */
SENSOR_TIMEDELTA, /* system time error (nSec) */
Re: Unit descriptions in sensors.h
Bah. One of those should be (muAh). --david
Re: Unit descriptions in sensors.h
On Dec 18, 2014, at 1:48 PM, David Higgs hig...@gmail.com wrote:
Bah. One of those should be (muAh).
Take two.
--david
Index: sensors.h
===
RCS file: /cvs/src/sys/sys/sensors.h,v
retrieving revision 1.33
diff -u -p -r1.33 sensors.h
--- sensors.h 4 Nov 2013 02:41:49 - 1.33
+++ sensors.h 18 Dec 2014 19:36:51 -
@@ -38,11 +38,11 @@ enum sensor_type {
SENSOR_OHMS,/* resistance */
SENSOR_WATTS, /* power (muW) */
SENSOR_AMPS,/* current (muA) */
- SENSOR_WATTHOUR,/* power capacity */
- SENSOR_AMPHOUR, /* power capacity */
+ SENSOR_WATTHOUR,/* power capacity (muWh) */
+ SENSOR_AMPHOUR, /* power capacity (muAh) */
SENSOR_INDICATOR, /* boolean indicator */
SENSOR_INTEGER, /* generic integer value */
- SENSOR_PERCENT, /* percent */
+ SENSOR_PERCENT, /* percent (m%) */
SENSOR_LUX, /* illuminance (mulx) */
SENSOR_DRIVE, /* disk */
SENSOR_TIMEDELTA, /* system time error (nSec) */
Better/more upd(4) timedelta sensors
While my device does not seem to provide AtRateTimeToFull or AtRateTimeToEmpty,
it does have RunTimeToEmpty. Then I found that SENSOR_TIMEDELTA values are in
nanoseconds and that scaling for them was never implemented correctly.
I am confused by the spec [1], though; see 4.2.5 - Battery Measures. The
reported values are supposedly in minutes but hid_info.unit is 0x1001 (seconds)
and observation of RunTimeToEmpty appears to agree. I don’t see any other
drivers in the tree that pay attention to unit or unit_exponent fields, and
don’t want to go down a rabbit hole if there’s no interest.
As usual, feedback is welcome.
[1] http://www.usb.org/developers/hidpage/pdcv10.pdf
--david
Index: upd.c
===
RCS file: /cvs/src/sys/dev/usb/upd.c,v
retrieving revision 1.12
diff -u -p -r1.12 upd.c
--- upd.c 11 Dec 2014 18:50:32 - 1.12
+++ upd.c 18 Dec 2014 05:02:30 -
@@ -66,7 +66,11 @@ static struct upd_usage_entry upd_usage_
{ HUP_BATTERY, HUB_AC_PRESENT,
SENSOR_INDICATOR,ACPresent },
{ HUP_BATTERY, HUB_ATRATE_TIMETOFULL,
- SENSOR_TIMEDELTA,AtRateTimeToFull }
+ SENSOR_TIMEDELTA,AtRateTimeToFull },
+ { HUP_BATTERY, HUB_ATRATE_TIMETOEMPTY,
+ SENSOR_TIMEDELTA,AtRateTimeToEmpty },
+ { HUP_BATTERY, HUB_RUNTIMETO_EMPTY,
+ SENSOR_TIMEDELTA,RunTimeToEmpty },
};
struct upd_report {
@@ -322,9 +326,9 @@ upd_update_sensors(struct upd_softc *sc,
int repid)
{
struct upd_sensor *sensor;
- ulong hdata, batpres;
- ulong adjust;
- int i;
+ int64_t adjust;
+ ulong batpres;
+ int hdata, i;
sensor = upd_lookup_sensor(sc, HUP_BATTERY, HUB_BATTERY_PRESENT);
batpres = sensor ? sensor-ksensor.value : -1;
@@ -353,6 +357,11 @@ upd_update_sensors(struct upd_softc *sc,
case HUB_FULLCHARGE_CAPACITY:
adjust = 1000; /* scale adjust */
break;
+ case HUB_ATRATE_TIMETOFULL:
+ case HUB_ATRATE_TIMETOEMPTY:
+ case HUB_RUNTIMETO_EMPTY:
+ adjust = 10LL; /* XXX not minutes? */
+ break;
default:
adjust = 1; /* no scale adjust */
break;
@@ -363,7 +372,7 @@ upd_update_sensors(struct upd_softc *sc,
sensor-ksensor.value = hdata * adjust;
sensor-ksensor.status = SENSOR_S_OK;
sensor-ksensor.flags = ~SENSOR_FINVALID;
- DPRINTF((%s: hidget data: %lu\n,
+ DPRINTF((%s: hidget data: %d\n,
sc-sc_sensordev.xname, hdata));
}
}
Re: upd(4) buggy firmware
On Thu, Dec 11, 2014 at 6:22 AM, Martin Pieuchot mpieuc...@nolizard.org wrote: Thanks for all your comments. I though a bit more about this change and decided to: - Simply return the number of bytes written/read upon success and -1 otherwise (à la read/write). This allows us to remove some usages of usbd_status when we are not directly manipulating a xfer object. - Always consider a short transfer for SET requests as failures. So the diff below includes your changes to upd(4) that I'll commit separately if you're ok. Concerning your remark about the USB_SET_IMMED ioctl(2), it doesn't really matter, this interface is not used and should be removed. Aside from what kettenis@ already saw, I also notice that uhidev_use_rdesc and ukbd_set_leds don't check their set_report calls, but that was the case in the previous code too. Other than that, the diff looks good and runs my UPS fine. Commit in whatever chunks you deem appropriate. Thanks. --david
upd(4) status
Now that my upd(4) is working (thanks to all involved), I’m looking to improve
behavior a bit. Let’s add some state transitions to the sensors.
Feedback is welcome.
--david
## before
[vm@vm usb]$ sysctl hw.sensors.upd0
hw.sensors.upd0.indicator0=Off (Charging), OK
hw.sensors.upd0.indicator1=Off (Discharging), OK
hw.sensors.upd0.indicator2=On (ACPresent), OK
hw.sensors.upd0.indicator3=Off (ShutdownImminent), OK
hw.sensors.upd0.indicator4=On (BatteryPresent), OK
hw.sensors.upd0.percent0=100.00% (RemainingCapacity), OK
hw.sensors.upd0.percent1=100.00% (FullChargeCapacity), OK
## AC power loss
[vm@vm usb]$ sysctl hw.sensors.upd0
hw.sensors.upd0.indicator0=Off (Charging), OK
hw.sensors.upd0.indicator1=On (Discharging), WARNING
hw.sensors.upd0.indicator2=Off (ACPresent), WARNING
hw.sensors.upd0.indicator3=Off (ShutdownImminent), OK
hw.sensors.upd0.indicator4=On (BatteryPresent), OK
hw.sensors.upd0.percent0=95.00% (RemainingCapacity), OK
hw.sensors.upd0.percent1=100.00% (FullChargeCapacity), OK
## AC power restored
[vm@vm usb]$ sysctl hw.sensors.upd0
hw.sensors.upd0.indicator0=On (Charging), WARNING
hw.sensors.upd0.indicator1=Off (Discharging), OK
hw.sensors.upd0.indicator2=On (ACPresent), OK
hw.sensors.upd0.indicator3=Off (ShutdownImminent), OK
hw.sensors.upd0.indicator4=On (BatteryPresent), OK
hw.sensors.upd0.percent0=97.00% (RemainingCapacity), OK
hw.sensors.upd0.percent1=100.00% (FullChargeCapacity), OK
Index: upd.c
===
RCS file: /cvs/src/sys/dev/usb/upd.c,v
retrieving revision 1.12
diff -u -p -r1.12 upd.c
--- upd.c 11 Dec 2014 18:50:32 - 1.12
+++ upd.c 12 Dec 2014 00:32:09 -
@@ -324,6 +324,7 @@ upd_update_sensors(struct upd_softc *sc,
struct upd_sensor *sensor;
ulong hdata, batpres;
ulong adjust;
+ enum sensor_status status;
int i;
sensor = upd_lookup_sensor(sc, HUP_BATTERY, HUB_BATTERY_PRESENT);
@@ -346,6 +347,10 @@ upd_update_sensors(struct upd_softc *sc,
}
}
+ adjust = 1;
+ status = SENSOR_S_OK;
+ hdata = hid_get_data(buf, len, sensor-hitem.loc);
+
switch (HID_GET_USAGE(sensor-hitem.usage)) {
case HUB_REL_STATEOF_CHARGE:
case HUB_ABS_STATEOF_CHARGE:
@@ -353,15 +358,28 @@ upd_update_sensors(struct upd_softc *sc,
case HUB_FULLCHARGE_CAPACITY:
adjust = 1000; /* scale adjust */
break;
+ case HUB_CHARGING:
+ case HUB_DISCHARGING:
+ if (hdata)
+ status = SENSOR_S_WARN;
+ break;
+ case HUB_BATTERY_PRESENT:
+ if (!hdata)
+ status = SENSOR_S_CRIT;
+ break;
+ case HUP_SHUTDOWN_IMMINENT:
+ if (hdata)
+ status = SENSOR_S_CRIT;
+ break;
+ case HUB_AC_PRESENT:
+ if (!hdata)
+ status = SENSOR_S_WARN;
default:
- adjust = 1; /* no scale adjust */
break;
}
- hdata = hid_get_data(buf, len, sensor-hitem.loc);
-
sensor-ksensor.value = hdata * adjust;
- sensor-ksensor.status = SENSOR_S_OK;
+ sensor-ksensor.status = status;
sensor-ksensor.flags = ~SENSOR_FINVALID;
DPRINTF((%s: hidget data: %lu\n,
sc-sc_sensordev.xname, hdata));
Improve upd(4) battery present check
First, I think the BatteryPresent check has a signedness problem. Second, I
think that it should check for sensor validity too, so it doesn’t use stale
values if BatteryPresent somehow goes straight from present to invalid.
This diff should fix both things, in theory. Compiled and running without
adverse effects. I did not pull apart my UPS while it’s actively running to
confirm, though.
--david
Index: upd.c
===
RCS file: /cvs/src/sys/dev/usb/upd.c,v
retrieving revision 1.12
diff -u -p -r1.12 upd.c
--- upd.c 11 Dec 2014 18:50:32 - 1.12
+++ upd.c 12 Dec 2014 02:11:09 -
@@ -322,12 +322,14 @@ upd_update_sensors(struct upd_softc *sc,
int repid)
{
struct upd_sensor *sensor;
- ulong hdata, batpres;
+ ulong hdata;
ulong adjust;
- int i;
+ int i, batpres = 0;
sensor = upd_lookup_sensor(sc, HUP_BATTERY, HUB_BATTERY_PRESENT);
- batpres = sensor ? sensor-ksensor.value : -1;
+ if (sensor !(sensor-ksensor.flags SENSOR_FINVALID)
+ sensor-ksensor.value)
+ batpres = 1;
for (i = 0; i sc-sc_num_sensors; i++) {
sensor = sc-sc_sensors[i];
@@ -336,7 +338,7 @@ upd_update_sensors(struct upd_softc *sc,
/* invalidate battery dependent sensors */
if (HID_GET_USAGE_PAGE(sensor-hitem.usage) == HUP_BATTERY
- batpres = 0) {
+ !batpres) {
/* exception to the battery sensor itself */
if (HID_GET_USAGE(sensor-hitem.usage) !=
HUB_BATTERY_PRESENT) {
vic0: no link on recent i386 /bsd.rd
Working and non-working dmesgs from the same VM below. Let me know if any more info is needed. If this is already known, sorry for the noise. --david # does not work OpenBSD 5.6-current (RAMDISK_CD) #555: Tue Dec 9 00:50:21 MST 2014 dera...@i386.openbsd.org:/usr/src/sys/arch/i386/compile/RAMDISK_CD cpu0: Intel(R) Core(TM)2 Duo CPU T7700 @ 2.40GHz (GenuineIntel 686-class) 2.42 GHz cpu0: FPU,V86,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,NXE,LONG,SSE3,SSSE3,CX16,LAHF,PERF,ITSC real mem = 267907072 (255MB) avail mem = 255856640 (244MB) mainbus0 at root bios0 at mainbus0: AT/286+ BIOS, date 09/20/12, BIOS32 rev. 0 @ 0xfd780, SMBIOS rev. 2.4 @ 0xe0010 (364 entries) bios0: vendor Phoenix Technologies LTD version 6.00 date 09/20/2012 bios0: VMware, Inc. VMware Virtual Platform acpi0 at bios0: rev 2 acpi0: sleep states S0 S1 S4 S5 acpi0: tables DSDT FACP BOOT APIC MCFG SRAT HPET WAET acpimadt0 at acpi0 addr 0xfee0: PC-AT compat cpu0 at mainbus0: apid 0 (boot processor) cpu0: apic clock running at 65MHz ioapic0 at mainbus0: apid 1 pa 0xfec0, version 11, 24 pins acpiprt0 at acpi0: bus 0 (PCI0) bios0: ROM list: 0xc/0x8000 0xc8000/0x1000 0xdc000/0x4000! 0xe/0x8000! pci0 at mainbus0 bus 0: configuration mode 1 (bios) pchb0 at pci0 dev 0 function 0 Intel 82443BX AGP rev 0x01 ppb0 at pci0 dev 1 function 0 Intel 82443BX AGP rev 0x01 pci1 at ppb0 bus 1 pcib0 at pci0 dev 7 function 0 Intel 82371AB PIIX4 ISA rev 0x08 pciide0 at pci0 dev 7 function 1 Intel 82371AB IDE rev 0x01: DMA, channel 0 configured to compatibility, channel 1 configured to compatibility wd0 at pciide0 channel 0 drive 0: VMware Virtual IDE Hard Drive wd0: 64-sector PIO, LBA, 8192MB, 16777216 sectors wd0(pciide0:0:0): using PIO mode 4, Ultra-DMA mode 2 pciide0: channel 1 disabled (no drives) Intel 82371AB Power rev 0x08 at pci0 dev 7 function 3 not configured VMware VMCI rev 0x10 at pci0 dev 7 function 7 not configured vga1 at pci0 dev 15 function 0 VMware SVGA II rev 0x00 vga1: aperture needed wsdisplay0 at vga1 mux 1: console (80x25, vt100 emulation) ppb1 at pci0 dev 17 function 0 VMware PCI rev 0x02 pci2 at ppb1 bus 2 vic0 at pci2 dev 0 function 0 AMD 79c970 PCnet-PCI rev 0x10: apic 1 int 18, address 00:0c:29:23:6c:31 ppb2 at pci0 dev 21 function 0 VMware PCIE rev 0x01 pci3 at ppb2 bus 3 ppb3 at pci0 dev 21 function 1 VMware PCIE rev 0x01 pci4 at ppb3 bus 4 ppb4 at pci0 dev 21 function 2 VMware PCIE rev 0x01 pci5 at ppb4 bus 5 ppb5 at pci0 dev 21 function 3 VMware PCIE rev 0x01 pci6 at ppb5 bus 6 ppb6 at pci0 dev 21 function 4 VMware PCIE rev 0x01 pci7 at ppb6 bus 7 ppb7 at pci0 dev 21 function 5 VMware PCIE rev 0x01 pci8 at ppb7 bus 8 ppb8 at pci0 dev 21 function 6 VMware PCIE rev 0x01 pci9 at ppb8 bus 9 ppb9 at pci0 dev 21 function 7 VMware PCIE rev 0x01 pci10 at ppb9 bus 10 ppb10 at pci0 dev 22 function 0 VMware PCIE rev 0x01 pci11 at ppb10 bus 11 ppb11 at pci0 dev 22 function 1 VMware PCIE rev 0x01 pci12 at ppb11 bus 12 ppb12 at pci0 dev 22 function 2 VMware PCIE rev 0x01 pci13 at ppb12 bus 13 ppb13 at pci0 dev 22 function 3 VMware PCIE rev 0x01 pci14 at ppb13 bus 14 ppb14 at pci0 dev 22 function 4 VMware PCIE rev 0x01 pci15 at ppb14 bus 15 ppb15 at pci0 dev 22 function 5 VMware PCIE rev 0x01 pci16 at ppb15 bus 16 ppb16 at pci0 dev 22 function 6 VMware PCIE rev 0x01 pci17 at ppb16 bus 17 ppb17 at pci0 dev 22 function 7 VMware PCIE rev 0x01 pci18 at ppb17 bus 18 ppb18 at pci0 dev 23 function 0 VMware PCIE rev 0x01 pci19 at ppb18 bus 19 ppb19 at pci0 dev 23 function 1 VMware PCIE rev 0x01 pci20 at ppb19 bus 20 ppb20 at pci0 dev 23 function 2 VMware PCIE rev 0x01 pci21 at ppb20 bus 21 ppb21 at pci0 dev 23 function 3 VMware PCIE rev 0x01 pci22 at ppb21 bus 22 ppb22 at pci0 dev 23 function 4 VMware PCIE rev 0x01 pci23 at ppb22 bus 23 ppb23 at pci0 dev 23 function 5 VMware PCIE rev 0x01 pci24 at ppb23 bus 24 ppb24 at pci0 dev 23 function 6 VMware PCIE rev 0x01 pci25 at ppb24 bus 25 ppb25 at pci0 dev 23 function 7 VMware PCIE rev 0x01 pci26 at ppb25 bus 26 ppb26 at pci0 dev 24 function 0 VMware PCIE rev 0x01 pci27 at ppb26 bus 27 ppb27 at pci0 dev 24 function 1 VMware PCIE rev 0x01 pci28 at ppb27 bus 28 ppb28 at pci0 dev 24 function 2 VMware PCIE rev 0x01 pci29 at ppb28 bus 29 ppb29 at pci0 dev 24 function 3 VMware PCIE rev 0x01 pci30 at ppb29 bus 30 ppb30 at pci0 dev 24 function 4 VMware PCIE rev 0x01 pci31 at ppb30 bus 31 ppb31 at pci0 dev 24 function 5 VMware PCIE rev 0x01 pci32 at ppb31 bus 32 ppb32 at pci0 dev 24 function 6 VMware PCIE rev 0x01 pci33 at ppb32 bus 33 ppb33 at pci0 dev 24 function 7 VMware PCIE rev 0x01 pci34 at ppb33 bus 34 isa0 at pcib0 isadma0 at isa0 fdc0 at isa0 port 0x3f0/6 irq 6 drq 2 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 pckbd0 at pckbc0 (kbd slot) pckbc0: using irq 1 for kbd slot wskbd0 at pckbd0: console keyboard, using wsdisplay0 npx0 at isa0
Re: sensorsd.conf(5) indicator parsing
On Tue, Dec 9, 2014 at 5:02 AM, Stuart Henderson st...@openbsd.org wrote: On 2014/12/08 15:04, David Higgs wrote: As per an earlier thread on misc@, this fixes sensorsd.conf(5) parsing of SENSOR_INDICATOR values. Since parsing as integers was both undocumented and confusing, it is no longer supported. Also, bail on error if the high/low values don't create a valid range. Low/high transitions don't make sense for these types of sensor anyway. That was a quick hack because the indicator sensors don't function as status sensors (but I didn't get as far as working out why). This mimics existing behavior, but still isn't very intuitive. I.e. low=Off:high=On will always be within user limits. Should indicators limits behave differently? Feedback is welcome. Specific feedback on the diff, I'm not keen on the case sensitive comparison (so at least use strcasecmp). But it feels to me like if we're changing things in this area we should fix it properly rather than continue the hack but using strings instead of integers. In my opinion the proper fix would be to treat these sensors as status sensors. I'm looking into adding some status changes to upd(4), but it probably isn't safe to assume that all devices provide useful status indicators. I don't think all of them have an obvious mapping to status and are best left as informational, but sensorsd(8) should have a way to add user limits / triggers regardless. I'll look into this, thanks. --david
last(1) crash fix
I was playing with afl a few weeks ago and found this. I believe it is
triggered by non-sequential timestamp records, but I didn’t dig into it or run
afl for particularly long. The patch below fixed all the crashes afl had found
up to that point.
The string used doesn’t matter, ‘crmsg’ just needs to be initialized in case
there isn’t a ‘~’ entry prior to needing to use it.
Thanks.
--david
Index: last.c
===
RCS file: /cvs/src/usr.bin/last/last.c,v
retrieving revision 1.43
diff -u -p -r1.43 last.c
--- last.c 26 Nov 2014 18:34:51 - 1.43
+++ last.c 27 Nov 2014 14:36:45 -
@@ -241,7 +241,7 @@ wtmp(void)
{
time_t delta, total = 0;
int timesize, wfd, snapfound = 0;
- char*ct, *crmsg;
+ char*ct, *crmsg = invalid;
struct utmp *bp;
struct stat stb;
ssize_t bytes;
Re: sensorsd.conf(5) indicator parsing
On Tue, Dec 9, 2014 at 9:40 AM, Stuart Henderson st...@openbsd.org wrote: On 2014/12/09 09:36, David Higgs wrote: On Tue, Dec 9, 2014 at 5:02 AM, Stuart Henderson st...@openbsd.org wrote: On 2014/12/08 15:04, David Higgs wrote: As per an earlier thread on misc@, this fixes sensorsd.conf(5) parsing of SENSOR_INDICATOR values. Since parsing as integers was both undocumented and confusing, it is no longer supported. Also, bail on error if the high/low values don't create a valid range. Low/high transitions don't make sense for these types of sensor anyway. That was a quick hack because the indicator sensors don't function as status sensors (but I didn't get as far as working out why). This mimics existing behavior, but still isn't very intuitive. I.e. low=Off:high=On will always be within user limits. Should indicators limits behave differently? Feedback is welcome. Specific feedback on the diff, I'm not keen on the case sensitive comparison (so at least use strcasecmp). But it feels to me like if we're changing things in this area we should fix it properly rather than continue the hack but using strings instead of integers. In my opinion the proper fix would be to treat these sensors as status sensors. I'm looking into adding some status changes to upd(4), but it probably isn't safe to assume that all devices provide useful status indicators. I don't think all of them have an obvious mapping to status and are best left as informational, but sensorsd(8) should have a way to add user limits / triggers regardless. I'll look into this, thanks. I was thinking more that it might be better for sensorsd internally to treat state transitions of indicator sensors like it treats status changes, rather than change how the sensors themselves report. What do you think? Yes, that makes a lot of sense. I still think that some of them should have an associated status for reporting purposes (AC power loss or mfi(4) drive status), but indicators could automatically trigger on value changes. User-specified high/low settings will be ignored or rejected. The %l (above / below / within / etc) output may need rethinking... --david
Re: vic0: no link on recent i386 /bsd.rd
On Tue, Dec 9, 2014 at 9:19 AM, Mark Kettenis mark.kette...@xs4all.nl wrote: Date: Tue, 9 Dec 2014 14:12:20 + From: Stuart Henderson st...@openbsd.org On 2014/12/09 08:37, David Higgs wrote: Working and non-working dmesgs from the same VM below. Let me know if any more info is needed. If this is already known, sorry for the noise. gonzalo@ has seen similar on vr(4) starting recently, but the timing doesn't quite match yours. # does not work OpenBSD 5.6-current (RAMDISK_CD) #555: Tue Dec 9 00:50:21 MST 2014 # works fine OpenBSD 5.6-current (GENERIC) #574: Mon Dec 8 15:39:56 MST 2014 His old (working) kernel was i386 #507 snapshot (Nov 13) and he's seeing problems in #572 (Dec 6 or 7th) which is a bit earlier than yours. Might be a different cause, of course... Pretty sure this one can be blamed on krw's recent changes to dhclient. For the record, same problem with em(4) on my amd64 VM. Fresh bsd.rd, amd64 #613 snapshot (Dec 9), using the (U)pgrade option. My previously working kernel is amd64 #597 (Dec 1).
Re: upd(4) buggy firmware
On Dec 8, 2014, at 6:07 PM, Martin Pieuchot mpieuc...@nolizard.org wrote: On 08/12/14(Mon) 09:35, David Higgs wrote: On Mon, Dec 8, 2014 at 9:29 AM, Martin Pieuchot mpieuc...@nolizard.org wrote: [...] Now I'd like to finish the transition that started with the import of upd(4) and move away from the actual 1 reportID = 1 driver model. Because in the end they are all sharing the same USB device actually represented by an uhidev(4) instance. So I'll ride the refactoring needed by your buggy firmware to also change that. Since all the uhidev_*_report() will be modified, we can also pass a pointer to the uhidev(4) device instead of one of its children. Is it clearer? Sounds great to me. As promised, here's a second and last diff that changes the various uhidev_*_report() functions to provide the transfered length to the caller if requested. The *get* variant now also handles the first byte required for non-0 reportID. And last but not least, it includes the hack from apcupsd to deal with buggy firmwares like yours. Could you try it and let me know how it goes? Seems to work just fine on my -current VM after adding the diff below (I didn’t try without). See my previous email in this thread for other comments re: your diff. hw.sensors.upd0.indicator0=Off (Charging), OK hw.sensors.upd0.indicator1=Off (Discharging), OK hw.sensors.upd0.indicator2=On (ACPresent), OK hw.sensors.upd0.indicator3=Off (ShutdownImminent), OK hw.sensors.upd0.indicator4=On (BatteryPresent), OK hw.sensors.upd0.percent0=100.00% (RemainingCapacity), OK hw.sensors.upd0.percent1=100.00% (FullChargeCapacity), OK --david Index: upd.c === RCS file: /cvs/src/sys/dev/usb/upd.c,v retrieving revision 1.10 diff -u -p -r1.10 upd.c --- upd.c 12 Jul 2014 18:48:52 - 1.10 +++ upd.c 9 Dec 2014 15:39:20 - @@ -358,8 +360,7 @@ upd_update_sensors(struct upd_softc *sc, break; } - /* XXX first byte which is the report id */ - hdata = hid_get_data(buf + 1, len, sensor-hitem.loc); + hdata = hid_get_data(buf, len, sensor-hitem.loc); sensor-ksensor.value = hdata * adjust; sensor-ksensor.status = SENSOR_S_OK;
Re: sensorsd.conf(5) indicator parsing
On Dec 9, 2014, at 9:53 AM, David Higgs hig...@gmail.com wrote:
On Tue, Dec 9, 2014 at 9:40 AM, Stuart Henderson st...@openbsd.org wrote:
I was thinking more that it might be better for sensorsd internally
to treat state transitions of indicator sensors like it treats
status changes, rather than change how the sensors themselves report.
What do you think?
Yes, that makes a lot of sense. I still think that some of them
should have an associated status for reporting purposes (AC power loss
or mfi(4) drive status), but indicators could automatically trigger on
value changes. User-specified high/low settings will be ignored or
rejected. The %l (above / below / within / etc) output may need
rethinking…
Something like the following, maybe?
Add a new ‘rvalues’ capability for sensorsd.conf(5) to report value changes.
While most useful for indicator and drive sensors, it can provide granular
output for *any* sensor that changes slowly enough for sensorsd(8) debouncing
behavior.
Also, ignore user-specified limits on indicator and drive sensors. These types
of sensors are not actually intervals; using them as such was confusing,
error-prone, and undocumented. Users should specify ‘rvalues’ with an optional
‘command=’ to implement more useful behavior.
--david
Index: sensorsd.c
===
RCS file: /cvs/src/usr.sbin/sensorsd/sensorsd.c,v
retrieving revision 1.53
diff -u -p -r1.53 sensorsd.c
--- sensorsd.c 29 Jun 2014 00:58:45 - 1.53
+++ sensorsd.c 9 Dec 2014 23:53:34 -
@@ -50,20 +50,26 @@ struct limits_t {
enum sensor_typetype; /* sensor type */
int numt; /* sensor number */
int64_t last_val;
+ int64_t last_val2;
int64_t lower; /* lower limit */
int64_t upper; /* upper limit */
char*command; /* failure command */
time_t astatus_changed;
+ time_t value_changed;
time_t ustatus_changed;
enum sensor_status astatus;/* last automatic status */
enum sensor_status astatus2;
+ int last_valid; /* last value validity */
+ int last_valid2;
enum sensorsd_s_status ustatus;/* last user-limit status */
enum sensorsd_s_status ustatus2;
int acount; /* stat change counter */
+ int vcount; /* value change counter */
int ucount; /* stat change counter */
u_int8_tflags; /* sensorsd limit flags */
#define SENSORSD_L_USERLIMIT 0x0001 /* user specified limit */
#define SENSORSD_L_ISTATUS 0x0002 /* ignore automatic status */
+#define SENSORSD_L_RVALUES 0x0004 /* report value changes */
};
struct sdlim_t {
@@ -337,6 +343,7 @@ check_sdlim(struct sdlim_t *sdlim, time_
TAILQ_FOREACH(limit, sdlim-limits, entries) {
if ((limit-flags SENSORSD_L_ISTATUS)
+ !(limit-flags SENSORSD_L_RVALUES)
!(limit-flags SENSORSD_L_USERLIMIT))
continue;
@@ -361,6 +368,28 @@ check_sdlim(struct sdlim_t *sdlim, time_
}
}
+ if (limit-flags SENSORSD_L_RVALUES) {
+ int newvalid = !(sensor.flags SENSOR_FINVALID);
+
+ if (limit-last_val != sensor.value ||
+ limit-last_valid != newvalid) {
+ if (limit-last_val2 != sensor.value ||
+ limit-last_valid2 != newvalid) {
+ limit-last_val2 = sensor.value;
+ limit-last_valid2 = newvalid;
+ limit-vcount = 0;
+ } else if (++limit-vcount = 3) {
+ limit-last_val2 =
+ limit-last_val = sensor.value;
+ limit-last_valid2 =
+ limit-last_valid = newvalid;
+ limit-astatus2 =
+ limit-astatus = sensor.status;
+ limit-value_changed = this_check;
+ }
+ }
+ }
+
if (limit-flags SENSORSD_L_USERLIMIT) {
enum sensorsd_s_status newustatus;
@@ -422,10 +451,12 @@ report_sdlim(struct sdlim_t *sdlim, time
TAILQ_FOREACH(limit, sdlim-limits, entries
Re: upd(4) buggy firmware
On Sat, Dec 6, 2014 at 8:57 AM, Martin Pieuchot mpieuc...@nolizard.org wrote: The ohci(4) diff is almost fine, USBD_SHORT_XFER should only be set in usbd_transfer_complete() so the HCD should only set the status to USBD_NORMAL_COMPLETION, see below. Concerning your broken firmware, what we should do is change the uhidev_*_report() function to somehow return the number of transferred bytes (actlen). Since this involves calling usbd_do_request_flag() and update all the consumers of this API, here's a first step. Could you test the diff below and tell me if it still works for you? You'll still need your upd(4) diff. If that works, I'll commit it and send a second diff to deal with your broken firmware. Seems to work fine when patched against -stable. (Sorry, I'd rather not update this box to -current, unless necessary for some later diff). Since uhidev_set_report didn't change, why did wacom behavior need tweaking? I don't own this peripheral... --david
Re: upd(4) buggy firmware
On Mon, Dec 8, 2014 at 9:29 AM, Martin Pieuchot mpieuc...@nolizard.org wrote: On 08/12/14(Mon) 09:02, David Higgs wrote: On Sat, Dec 6, 2014 at 8:57 AM, Martin Pieuchot mpieuc...@nolizard.org wrote: The ohci(4) diff is almost fine, USBD_SHORT_XFER should only be set in usbd_transfer_complete() so the HCD should only set the status to USBD_NORMAL_COMPLETION, see below. Concerning your broken firmware, what we should do is change the uhidev_*_report() function to somehow return the number of transferred bytes (actlen). Since this involves calling usbd_do_request_flag() and update all the consumers of this API, here's a first step. Could you test the diff below and tell me if it still works for you? You'll still need your upd(4) diff. If that works, I'll commit it and send a second diff to deal with your broken firmware. Seems to work fine when patched against -stable. (Sorry, I'd rather not update this box to -current, unless necessary for some later diff). Stable should be fine. Does this include the ohci(4) diff? Yes, I replaced my ohci(4) tweak with your full diff, while keeping my changes to upd(4). Since uhidev_set_report didn't change, why did wacom behavior need tweaking? I don't own this peripheral... Because it was using usb_set_report() instead of uhidev_set_report() and doing the reportID dance by itself. Whoops, you're right. I missed that there was a different function prefix. Now I'd like to finish the transition that started with the import of upd(4) and move away from the actual 1 reportID = 1 driver model. Because in the end they are all sharing the same USB device actually represented by an uhidev(4) instance. So I'll ride the refactoring needed by your buggy firmware to also change that. Since all the uhidev_*_report() will be modified, we can also pass a pointer to the uhidev(4) device instead of one of its children. Is it clearer? Sounds great to me. --david
sensorsd.conf(5) indicator parsing
As per an earlier thread on misc@, this fixes sensorsd.conf(5) parsing of
SENSOR_INDICATOR values. Since parsing as integers was both undocumented and
confusing, it is no longer supported. Also, bail on error if the high/low
values don’t create a valid range.
This mimics existing behavior, but still isn't very intuitive. I.e.
“low=Off:high=On” will always be “within” user limits. Should indicators
limits behave differently?
Feedback is welcome.
--david
Index: sensorsd.c
===
RCS file: /cvs/src/usr.sbin/sensorsd/sensorsd.c,v
retrieving revision 1.53
diff -u -p -r1.53 sensorsd.c
--- sensorsd.c 29 Jun 2014 00:58:45 - 1.53
+++ sensorsd.c 8 Dec 2014 19:45:45 -
@@ -723,6 +723,8 @@ parse_config_sdlim(struct sdlim_t *sdlim
if (cgetstr(buf, high, ebuf) 0)
ebuf = NULL;
p-upper = get_val(ebuf, 1, p-type);
+ if (p-lower p-upper)
+ errx(1, %s: incorrect sensor range, node);
if (cgetstr(buf, command, ebuf) 0)
ebuf = NULL;
if (ebuf)
@@ -749,7 +751,7 @@ get_val(char *buf, int upper, enum senso
}
val = strtod(buf, p);
- if (buf == p)
+ if (buf == p type != SENSOR_INDICATOR)
err(1, incorrect value: %s, buf);
switch(type) {
@@ -780,6 +782,13 @@ get_val(char *buf, int upper, enum senso
rval = val * 1000.0;
break;
case SENSOR_INDICATOR:
+ if (strcmp(buf, On) == 0)
+ rval = 1;
+ else if (strcmp(buf, Off) == 0)
+ rval = 0;
+ else
+ errx(1, incorrect value: %s, buf);
+ break;
case SENSOR_INTEGER:
case SENSOR_DRIVE:
case SENSOR_ANGLE:
Re: upd(4) buggy firmware
On Mon, Dec 8, 2014 at 6:07 PM, Martin Pieuchot mpieuc...@nolizard.org wrote:
On 08/12/14(Mon) 09:35, David Higgs wrote:
On Mon, Dec 8, 2014 at 9:29 AM, Martin Pieuchot mpieuc...@nolizard.org
wrote:
[...]
Now I'd like to finish the transition that started with the import of
upd(4) and move away from the actual 1 reportID = 1 driver model.
Because in the end they are all sharing the same USB device actually
represented by an uhidev(4) instance. So I'll ride the refactoring
needed by your buggy firmware to also change that. Since all the
uhidev_*_report() will be modified, we can also pass a pointer to
the uhidev(4) device instead of one of its children. Is it clearer?
Sounds great to me.
As promised, here's a second and last diff that changes the various
uhidev_*_report() functions to provide the transfered length to the
caller if requested.
The *get* variant now also handles the first byte required for non-0
reportID.
And last but not least, it includes the hack from apcupsd to deal with
buggy firmwares like yours.
Could you try it and let me know how it goes?
This diff doesn't apply cleanly against -stable but I believe it
should be safe to temporarily connect my UPS back to my -current VM
and test there, since it doesn't appear to touch ohci/ehci-specific
code paths. Let me know if I'm wrong about this.
From reading the diff, though, I had a few questions and found at
least one problem. See comments inline, possibly mangled by gmail.
I'll try to provide results tomorrow morning.
Thanks.
Index: ucycom.c
===
RCS file: /cvs/src/sys/dev/usb/ucycom.c,v
retrieving revision 1.29
diff -u -p -r1.29 ucycom.c
--- ucycom.c12 Jul 2014 20:26:33 - 1.29
+++ ucycom.c8 Dec 2014 22:54:53 -
@@ -453,8 +453,8 @@ ucycom_param(void *addr, int portno, str
report[2] = (baud 16) 0xff;
report[3] = (baud 24) 0xff;
report[4] = cfg;
- err = uhidev_set_report(sc-sc_hdev, UHID_FEATURE_REPORT,
- sc-sc_hdev.sc_report_id, report, sc-sc_flen);
+ err = uhidev_set_report(sc-sc_hdev.sc_parent, UHID_FEATURE_REPORT,
+ sc-sc_hdev.sc_report_id, report, sc-sc_flen, NULL);
if (err != 0) {
DPRINTF((ucycom_param: uhidev_set_report %d %s\n,
err, usbd_errstr(err)));
@@ -553,11 +553,11 @@ ucycom_set(void *addr, int portno, int r
void
ucycom_get_cfg(struct ucycom_softc *sc)
{
- int err, cfg, baud;
+ int cfg, baud;
uint8_t report[5];
- err = uhidev_get_report(sc-sc_hdev, UHID_FEATURE_REPORT,
- sc-sc_hdev.sc_report_id, report, sc-sc_flen);
+ uhidev_get_report(sc-sc_hdev.sc_parent, UHID_FEATURE_REPORT,
+ sc-sc_hdev.sc_report_id, report, sc-sc_flen, NULL);
cfg = report[4];
baud = (report[3] 24) + (report[2] 16) + (report[1] 8) +
report[0];
DPRINTF((ucycom_configure: device reports %d baud, %d-%c-%d (%d)\n,
baud,
Index: ugold.c
===
RCS file: /cvs/src/sys/dev/usb/ugold.c,v
retrieving revision 1.6
diff -u -p -r1.6 ugold.c
--- ugold.c 29 Apr 2014 12:53:33 - 1.6
+++ ugold.c 8 Dec 2014 22:54:53 -
@@ -260,6 +260,6 @@ ugold_refresh(void *arg)
int
ugold_issue_cmd(struct ugold_softc *sc, uint8_t *cmd, int len)
{
- return uhidev_set_report_async(sc-sc_hdev, UHID_OUTPUT_REPORT,
- sc-sc_hdev.sc_report_id, cmd, len);
+ return uhidev_set_report_async(sc-sc_hdev.sc_parent,
+ UHID_OUTPUT_REPORT, sc-sc_hdev.sc_report_id, cmd, len, NULL);
}
Index: uhid.c
===
RCS file: /cvs/src/sys/dev/usb/uhid.c,v
retrieving revision 1.58
diff -u -p -r1.58 uhid.c
--- uhid.c 12 Jul 2014 18:48:52 - 1.58
+++ uhid.c 8 Dec 2014 22:54:53 -
@@ -259,7 +259,6 @@ uhid_do_read(struct uhid_softc *sc, stru
{
int s;
int error = 0;
- int extra;
size_t length;
u_char buffer[UHID_CHUNK];
usbd_status err;
@@ -267,13 +266,12 @@ uhid_do_read(struct uhid_softc *sc, stru
DPRINTFN(1, (uhidread\n));
if (sc-sc_state UHID_IMMED) {
DPRINTFN(1, (uhidread immed\n));
- extra = sc-sc_hdev.sc_report_id != 0;
- err = uhidev_get_report(sc-sc_hdev, UHID_INPUT_REPORT,
- sc-sc_hdev.sc_report_id, buffer,
- sc-sc_hdev.sc_isize + extra);
+ err = uhidev_get_report(sc-sc_hdev.sc_parent,
+ UHID_INPUT_REPORT, sc-sc_hdev.sc_report_id, buffer,
+ sc-sc_hdev.sc_isize, NULL);
if (err)
return (EIO);
- return (uiomove(buffer+extra, sc-sc_hdev.sc_isize, uio));
+ return (uiomove(buffer, sc
Re: upd(4) buggy firmware
On Dec 4, 2014, at 4:30 PM, David Higgs hig...@gmail.com wrote: I am trying to figure out how to handle the buggy USB firmware in my UPS (see misc@ thread from last week). With some kernel debug enabled, I see usb_transfer_complete: short transfer 35 messages. Since the BatteryPresent sensor could not be read, all battery-related sensors were forced into UNKNOWN state. It appears that apcupsd adjusts the expected HID report length as soon as a bogus request is detected: http://sourceforge.net/p/apcupsd/svn/2273/tree/branches/Branch-3_14/src/drivers/usb/generic/generic-usb.c#l254 The patch below handles my particular case but is probably too generic/sloppy. Is there any interest in workarounds, either like this or something less terrible? Below is an updated diff against -current, and dmesg when applied to -stable (no change). On my non-virtual hardware, upd(4) connects via ohci(4), which didn’t report USBD_SHORT_XFER. Also, I fixed a format string warning when UPD_DEBUG was enabled. Please let me know if my diffs are still being mangled, so I can resend as attachments and keep searching for a solution. Feedback is welcome. --david OpenBSD 5.6-stable (GENERIC) #8: Fri Dec 5 13:25:33 EST 2014 vm@vm.localdomain:/home/vm/src/sys/arch/i386/compile/GENERIC cpu0: Geode(TM) Integrated Processor by AMD PCS (AuthenticAMD 586-class) 499 MHz cpu0: FPU,DE,PSE,TSC,MSR,CX8,SEP,PGE,CMOV,CFLUSH,MMX,MMXX,3DNOW2,3DNOW real mem = 267943936 (255MB) avail mem = 251113472 (239MB) mpath0 at root scsibus0 at mpath0: 256 targets mainbus0 at root bios0 at mainbus0: AT/286+ BIOS, date 11/05/08, BIOS32 rev. 0 @ 0xfd088 pcibios0 at bios0: rev 2.1 @ 0xf/0x1 pcibios0: pcibios_get_intr_routing - function not supported pcibios0: PCI IRQ Routing information unavailable. pcibios0: PCI bus #0 is the last bus bios0: ROM list: 0xe/0xa800 cpu0 at mainbus0: (uniprocessor) mtrr: K6-family MTRR support (2 registers) pci0 at mainbus0 bus 0: configuration mode 1 (bios) pchb0 at pci0 dev 1 function 0 AMD Geode LX rev 0x33 glxsb0 at pci0 dev 1 function 2 AMD Geode LX Crypto rev 0x00: RNG AES vr0 at pci0 dev 9 function 0 VIA VT6105M RhineIII rev 0x96: irq 10, address 00:0d:b9:1e:60:7c ukphy0 at vr0 phy 1: Generic IEEE 802.3u media interface, rev. 3: OUI 0x004063, model 0x0034 vr1 at pci0 dev 10 function 0 VIA VT6105M RhineIII rev 0x96: irq 11, address 00:0d:b9:1e:60:7d ukphy1 at vr1 phy 1: Generic IEEE 802.3u media interface, rev. 3: OUI 0x004063, model 0x0034 vr2 at pci0 dev 11 function 0 VIA VT6105M RhineIII rev 0x96: irq 15, address 00:0d:b9:1e:60:7e ukphy2 at vr2 phy 1: Generic IEEE 802.3u media interface, rev. 3: OUI 0x004063, model 0x0034 glxpcib0 at pci0 dev 15 function 0 AMD CS5536 ISA rev 0x03: rev 3, 32-bit 3579545Hz timer, watchdog, gpio, i2c gpio0 at glxpcib0: 32 pins iic0 at glxpcib0 maxtmp0 at iic0 addr 0x4c: lm86 pciide0 at pci0 dev 15 function 2 AMD CS5536 IDE rev 0x01: DMA, channel 0 wired to compatibility, channel 1 wired to compatibility wd0 at pciide0 channel 0 drive 0: TS16GCF133 wd0: 1-sector PIO, LBA, 15296MB, 31326208 sectors wd0(pciide0:0:0): using PIO mode 4, Ultra-DMA mode 2 pciide0: channel 1 ignored (disabled) ohci0 at pci0 dev 15 function 4 AMD CS5536 USB rev 0x02: irq 12, version 1.0, legacy support ehci0 at pci0 dev 15 function 5 AMD CS5536 USB rev 0x02: irq 12 usb0 at ehci0: USB revision 2.0 uhub0 at usb0 AMD EHCI root hub rev 2.00/1.00 addr 1 isa0 at glxpcib0 isadma0 at isa0 com0 at isa0 port 0x3f8/8 irq 4: ns16550a, 16 byte fifo com0: console com1 at isa0 port 0x2f8/8 irq 3: ns16550a, 16 byte fifo pcppi0 at isa0 port 0x61 spkr0 at pcppi0 npx0 at isa0 port 0xf0/16: reported by CPUID; using exception 16 usb1 at ohci0: USB revision 1.0 uhub1 at usb1 AMD OHCI root hub rev 1.00/1.00 addr 1 nvram: invalid checksum uhidev0 at uhub1 port 2 configuration 1 interface 0 American Power Conversion Back-UPS ES 750 FW:841.I3 .D USB FW:I3 rev 1.10/1.01 addr 2 uhidev0: iclass 3/0, 146 report ids upd0 at uhidev0 vscsi0 at root scsibus1 at vscsi0: 256 targets softraid0 at root scsibus2 at softraid0: 256 targets root on wd0a (295fd65258ac0a48.a) swap on wd0b dump on wd0b clock: unknown CMOS layout Index: ohci.c === RCS file: /cvs/src/sys/dev/usb/ohci.c,v retrieving revision 1.140 diff -u -p -r1.140 ohci.c --- ohci.c 5 Oct 2014 08:40:29 - 1.140 +++ ohci.c 5 Dec 2014 18:45:40 - @@ -1311,6 +1311,8 @@ ohci_softintr(void *v) if (cc == OHCI_CC_STALL) xfer-status = USBD_STALLED; + else if (cc == OHCI_CC_DATA_UNDERRUN) + xfer-status = USBD_SHORT_XFER; else xfer-status = USBD_IOERROR; s = splusb(); Index: upd.c === RCS file: /cvs/src/sys
upd(4) buggy firmware
I am trying to figure out how to handle the buggy USB firmware in my UPS (see misc@ thread from last week). With some kernel debug enabled, I see usb_transfer_complete: short transfer 35 messages. Since the BatteryPresent sensor could not be read, all battery-related sensors were forced into UNKNOWN state. It appears that apcupsd adjusts the expected HID report length as soon as a bogus request is detected: http://sourceforge.net/p/apcupsd/svn/2273/tree/branches/Branch-3_14/src/drivers/usb/generic/generic-usb.c#l254 The patch below handles my particular case but is probably too generic/sloppy. Is there any interest in workarounds, either like this or something less terrible? Thanks. --david OpenBSD 5.6-current (GENERIC) #0: Thu Dec 4 16:24:54 EST 2014 vm@vm.localdomain:/home/vm/src/sys/arch/i386/compile/GENERIC cpu0: Intel(R) Core(TM)2 Duo CPU T7700 @ 2.40GHz (GenuineIntel 686-class) 2.40 GHz cpu0: FPU,V86,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,NXE,LONG,SSE3,SSSE3,CX16,LAHF,PERF,ITSC real mem = 267866112 (255MB) avail mem = 251166720 (239MB) mpath0 at root scsibus0 at mpath0: 256 targets mainbus0 at root bios0 at mainbus0: AT/286+ BIOS, date 09/20/12, BIOS32 rev. 0 @ 0xfd780, SMBIOS rev. 2.4 @ 0xe0010 (364 entries) bios0: vendor Phoenix Technologies LTD version 6.00 date 09/20/2012 bios0: VMware, Inc. VMware Virtual Platform acpi0 at bios0: rev 2 acpi0: sleep states S0 S1 S4 S5 acpi0: tables DSDT FACP BOOT APIC MCFG SRAT HPET WAET acpi0: wakeup devices PCI0(S3) USB_(S1) P2P0(S3) S1F0(S3) S2F0(S3) S3F0(S3) S4F0(S3) S5F0(S3) S6F0(S3) S7F0(S3) S8F0(S3) S9F0(S3) Z00Q(S3) Z00R(S3) Z00S(S3) Z00T(S3) [...] acpitimer0 at acpi0: 3579545 Hz, 24 bits acpimadt0 at acpi0 addr 0xfee0: PC-AT compat cpu0 at mainbus0: apid 0 (boot processor) mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges cpu0: apic clock running at 65MHz ioapic0 at mainbus0: apid 1 pa 0xfec0, version 11, 24 pins acpimcfg0 at acpi0 addr 0xe000, bus 0-255 acpihpet0 at acpi0: 14318179 Hz acpiprt0 at acpi0: bus 0 (PCI0) acpicpu0 at acpi0 acpibat0 at acpi0: BAT1 not present acpibat1 at acpi0: BAT2 not present acpiac0 at acpi0: AC unit online acpibtn0 at acpi0: SLPB acpibtn1 at acpi0: LID_ bios0: ROM list: 0xc/0x8000 0xc8000/0x1000 0xdc000/0x4000! 0xe/0x8000! vmt0 at mainbus0 pci0 at mainbus0 bus 0: configuration mode 1 (bios) pchb0 at pci0 dev 0 function 0 Intel 82443BX AGP rev 0x01 ppb0 at pci0 dev 1 function 0 Intel 82443BX AGP rev 0x01 pci1 at ppb0 bus 1 piixpcib0 at pci0 dev 7 function 0 Intel 82371AB PIIX4 ISA rev 0x08 pciide0 at pci0 dev 7 function 1 Intel 82371AB IDE rev 0x01: DMA, channel 0 configured to compatibility, channel 1 configured to compatibility wd0 at pciide0 channel 0 drive 0: VMware Virtual IDE Hard Drive wd0: 64-sector PIO, LBA, 8192MB, 16777216 sectors wd0(pciide0:0:0): using PIO mode 4, Ultra-DMA mode 2 pciide0: channel 1 disabled (no drives) piixpm0 at pci0 dev 7 function 3 Intel 82371AB Power rev 0x08: SMBus disabled VMware VMCI rev 0x10 at pci0 dev 7 function 7 not configured vga1 at pci0 dev 15 function 0 VMware SVGA II rev 0x00 wsdisplay0 at vga1 mux 1: console (80x25, vt100 emulation) wsdisplay0: screen 1-5 added (80x25, vt100 emulation) ppb1 at pci0 dev 17 function 0 VMware PCI rev 0x02 pci2 at ppb1 bus 2 vic0 at pci2 dev 0 function 0 AMD 79c970 PCnet-PCI rev 0x10: apic 1 int 18, address 00:0c:29:23:6c:31 uhci0 at pci2 dev 2 function 0 VMware UHCI rev 0x00: apic 1 int 16 ehci0 at pci2 dev 3 function 0 VMware EHCI rev 0x00: apic 1 int 17 usb0 at ehci0: USB revision 2.0 uhub0 at usb0 VMware EHCI root hub rev 2.00/1.00 addr 1 usb1 at uhci0: USB revision 1.0 uhub1 at usb1 VMware UHCI root hub rev 1.00/1.00 addr 1 ppb2 at pci0 dev 21 function 0 VMware PCIE rev 0x01 pci3 at ppb2 bus 3 ppb3 at pci0 dev 21 function 1 VMware PCIE rev 0x01 pci4 at ppb3 bus 4 ppb4 at pci0 dev 21 function 2 VMware PCIE rev 0x01 pci5 at ppb4 bus 5 ppb5 at pci0 dev 21 function 3 VMware PCIE rev 0x01 pci6 at ppb5 bus 6 ppb6 at pci0 dev 21 function 4 VMware PCIE rev 0x01 pci7 at ppb6 bus 7 ppb7 at pci0 dev 21 function 5 VMware PCIE rev 0x01 pci8 at ppb7 bus 8 ppb8 at pci0 dev 21 function 6 VMware PCIE rev 0x01 pci9 at ppb8 bus 9 ppb9 at pci0 dev 21 function 7 VMware PCIE rev 0x01 pci10 at ppb9 bus 10 ppb10 at pci0 dev 22 function 0 VMware PCIE rev 0x01 pci11 at ppb10 bus 11 ppb11 at pci0 dev 22 function 1 VMware PCIE rev 0x01 pci12 at ppb11 bus 12 ppb12 at pci0 dev 22 function 2 VMware PCIE rev 0x01 pci13 at ppb12 bus 13 ppb13 at pci0 dev 22 function 3 VMware PCIE rev 0x01 pci14 at ppb13 bus 14 ppb14 at pci0 dev 22 function 4 VMware PCIE rev 0x01 pci15 at ppb14 bus 15 ppb15 at pci0 dev 22 function 5 VMware PCIE rev 0x01 pci16 at ppb15 bus 16 ppb16 at pci0 dev 22 function 6 VMware PCIE rev 0x01 pci17 at ppb16 bus 17 ppb17 at pci0 dev 22 function 7 VMware PCIE rev 0x01 pci18 at ppb17 bus 18 ppb18 at pci0 dev 23 function 0 VMware PCIE rev
Re: sk(4): jumbo mbufs and rxring accounting
On Mon, Aug 18, 2014 at 6:24 PM, David Gwynne da...@gwynne.id.au wrote: On Sun, Jul 13, 2014 at 02:01:15PM +1000, David Gwynne wrote: i think i'll try to find the sk at work and wire it up. its just annoying cos im pretty sure its sr optics with sc connectors. thanks for testing. how's this one? Are either of you (or anyone else) interested in one or both of my twisted-pair sk(4) cards? I've plugged maybe once or twice in the last few years and maybe they can be put to better use elsewhere. I never saw them in want.html and never offered because I figured they fell in the very common PCI category, but contact me off-list if this isn't the case to help me figure out where to ship them to. Full dmesg: http://marc.info/?l=openbsd-techm=139062896512049w=2 skc0 at pci0 dev 16 function 0 3Com 3c940 rev 0x10, Yukon (0x1): irq 9 sk0 at skc0 port A: address 00:0a:5e:5c:50:41 eephy0 at sk0 phy 0: 88E1011 Gigabit PHY, rev. 3 skc1 at pci0 dev 17 function 0 3Com 3c940 rev 0x10, Yukon (0x1): irq 10 sk1 at skc1 port A: address 00:0a:5e:65:ee:47 eephy1 at sk1 phy 0: 88E1011 Gigabit PHY, rev. 3 Thanks. --david
strptime() fix
As of lib/libc/time/strptime.c r1.15, certain conversions will perform
calculations using the provided tm_mday value, which will frequently
produce incorrect values for tm_mday and tm_yday. Apologies in
advance for the mangled patch.
Thanks.
--david
Compare runs of the test program below:
/* strptime_test */
#include stdio.h
#include string.h
#include time.h
int main(int argc, char **argv)
{
struct tm tm;
memset(tm, 0xff, sizeof(tm));
if (argc 2 strptime(argv[1], argv[2], tm) != NULL)
printf((%d,%d)\n, tm.tm_yday, tm.tm_mday);
else
printf(arg/parse error\n);
return 0;
}
# the two examples below should both produce (-1,-1)
./strptime_test '2014-01' '%Y-%m'
./strptime_test '2014-01' '%Y-%d'
--- strptime.c.orig 2014-02-13 13:21:12.0 -0500
+++ strptime.c 2014-02-13 13:45:44.0 -0500
@@ -594,7 +594,8 @@
const int year = tm-tm_year + TM_YEAR_BASE;
const int *mon_lens = mon_lengths[isleap(year)];
if (!(fields FIELD_TM_YDAY)
- (fields (FIELD_TM_MON|FIELD_TM_MDAY))) {
+ (fields (FIELD_TM_MON|FIELD_TM_MDAY)) ==
+ ((FIELD_TM_MON|FIELD_TM_MDAY)) {
tm-tm_yday = tm-tm_mday - 1;
for (i = 0; i tm-tm_mon; i++)
tm-tm_yday += mon_lens[i];
Re: help needed from someone with an sk(4)
On Fri, Jan 24, 2014 at 4:24 AM, Henning Brauer lists-openbsdt...@bsws.de wrote: * Henning Brauer lists-openbsdt...@bsws.de [2014-01-24 05:50]: i need this tested on an sk(4). I don't have that hardware at all. this gets rif od a slight little bit more. Resurrected an old box, kernel compile w/ patch is underway. Should be able to provide feedback tomorrow. I'm kinda new to this - do I need to exercise forwarding, do some speed tests, or is it sufficient to just make sure that host-based usage doesn't break? Snapshot dmesg below. --david OpenBSD 5.5-beta (GENERIC) #239: Fri Jan 24 12:06:50 MST 2014 t...@i386.openbsd.org:/usr/src/sys/arch/i386/compile/GENERIC cpu0: Intel Pentium III (GenuineIntel 686-class, 512KB L2 cache) 599 MHz cpu0: FPU,V86,DE,PSE,TSC,MSR,PAE,MCE,CX8,SEP,MTRR,PGE,MCA,CMOV,PSE36,MMX,FXSR,SSE,PERF real mem = 267927552 (255MB) avail mem = 251641856 (239MB) mainbus0 at root bios0 at mainbus0: AT/286+ BIOS, date 10/13/00, BIOS32 rev. 0 @ 0xfd790, SMBIOS rev. 2.1 @ 0xefa30 (49 entries) bios0: vendor Intel Corp. version A11 date 10/13/2000 bios0: Dell Computer Corporation XPST600 acpi0 at bios0: rev 0 acpi0: sleep states S0 S1 S5 acpi0: tables DSDT FACP acpi0: wakeup devices PCI0(S1) USB0(S1) UAR1(S1) KBC_(S1) MICE(S1) acpitimer0 at acpi0: 3579545 Hz, 24 bits acpiprt0 at acpi0: bus 0 (PCI0) acpicpu0 at acpi0: C3, C2 acpipwrres0 at acpi0: PFAN, resource for FAN1 bios0: ROM list: 0xc/0xb800 0xcb800/0x800 0xcc000/0x800 0xe/0x4000! 0xe4000/0xc000 cpu0 at mainbus0: (uniprocessor) mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges pci0 at mainbus0 bus 0: configuration mode 1 (bios) pchb0 at pci0 dev 0 function 0 Intel 82443BX AGP rev 0x03 intelagp0 at pchb0 agp0 at intelagp0: aperture at 0xe000, size 0x1000 ppb0 at pci0 dev 1 function 0 Intel 82443BX AGP rev 0x03 pci1 at ppb0 bus 1 vga1 at pci1 dev 0 function 0 NVIDIA GeForce3 rev 0xa3 wsdisplay0 at vga1 mux 1: console (80x25, vt100 emulation) wsdisplay0: screen 1-5 added (80x25, vt100 emulation) piixpcib0 at pci0 dev 7 function 0 Intel 82371AB PIIX4 ISA rev 0x02 pciide0 at pci0 dev 7 function 1 Intel 82371AB IDE rev 0x01: DMA, channel 0 wired to compatibility, channel 1 wired to compatibility wd0 at pciide0 channel 0 drive 0: Maxtor 52049H3 wd0: 16-sector PIO, LBA, 19473MB, 39882528 sectors wd0(pciide0:0:0): using PIO mode 4, Ultra-DMA mode 2 pciide0: channel 1 ignored (disabled) uhci0 at pci0 dev 7 function 2 Intel 82371AB USB rev 0x01: irq 9 piixpm0 at pci0 dev 7 function 3 Intel 82371AB Power rev 0x02: SMI iic0 at piixpm0 spdmem0 at iic0 addr 0x50: 128MB SDRAM ECC PC100CL3 spdmem1 at iic0 addr 0x51: 128MB SDRAM ECC PC100CL3 ral0 at pci0 dev 14 function 0 Ralink RT2561S rev 0x00: irq 3, address 00:1d:7d:34:0e:ec ral0: MAC/BBP RT2561C, RF RT2527 skc0 at pci0 dev 16 function 0 3Com 3c940 rev 0x10, Yukon (0x1): irq 9 sk0 at skc0 port A: address 00:0a:5e:5c:50:41 eephy0 at sk0 phy 0: 88E1011 Gigabit PHY, rev. 3 skc1 at pci0 dev 17 function 0 3Com 3c940 rev 0x10, Yukon (0x1): irq 10 sk1 at skc1 port A: address 00:0a:5e:65:ee:47 eephy1 at sk1 phy 0: 88E1011 Gigabit PHY, rev. 3 isa0 at piixpcib0 isadma0 at isa0 com0 at isa0 port 0x3f8/8 irq 4: ns16550a, 16 byte fifo com0: console pckbc0 at isa0 port 0x60/5 pckbd0 at pckbc0 (kbd slot) pckbc0: using irq 1 for kbd slot wskbd0 at pckbd0: console keyboard, using wsdisplay0 pcppi0 at isa0 port 0x61 spkr0 at pcppi0 lpt0 at isa0 port 0x378/4 irq 7 npx0 at isa0 port 0xf0/16: reported by CPUID; using exception 16 fdc0 at isa0 port 0x3f0/6 irq 6 drq 2 usb0 at uhci0: USB revision 1.0 uhub0 at usb0 Intel UHCI root hub rev 1.00/1.00 addr 1 vscsi0 at root scsibus0 at vscsi0: 256 targets softraid0 at root scsibus1 at softraid0: 256 targets root on wd0a (ffd0e9298ff269a9.a) swap on wd0b dump on wd0b
ksh globbing vs. trailing backslash [patch]
Long time user, first time patch submission. I can send this again after tree
lock, but feedback is welcome in the meantime.
I started trying to fix PR 6006 and felt like there was progress being made
for a while. I was pretty sure that x_file_glob function shouldn't have
stripped backslashes before lexing, which caused the unescaped backtick to
throw the error. However, the code that checked for globbing failure would
then have to be adjusted, and I couldn't find logic to consistently predict
what expand() would return on such failure. Getting that wrong caused tab
completion to incorrectly escape the input buffer.
Long story short I didn't fix the PR but found a minor nit that seemed safe to
fix: don't allow add_glob to process strings with a trailing backslash.
# patched code sends a bell instead of displaying matches
# unpatched code displays as below
[vm@vm obj]$ cd c_\TAB
c_ksh.o c_sh.o c_test.o c_ulimit.o
Thanks.
--david
Index: edit.c
===
RCS file: /cvs/src/bin/ksh/edit.c,v
retrieving revision 1.34
diff -u -r1.34 edit.c
--- edit.c 20 May 2010 01:13:07 - 1.34
+++ edit.c 26 Feb 2011 01:34:36 -
@@ -355,7 +355,10 @@
if (slen 0)
return 0;
- toglob = add_glob(str, slen);
+ if ((toglob = add_glob(str, slen)) == NULL) {
+ *wordsp = NULL;
+ return 0;
+ }
/* remove all escaping backward slashes */
escaping = 0;
@@ -455,7 +458,10 @@
if (slen 0)
return 0;
- toglob = add_glob(str, slen);
+ if ((toglob = add_glob(str, slen)) == NULL) {
+ *wordsp = NULL;
+ return 0;
+ }
/* Convert foo* (toglob) to a pattern for future use */
pat = evalstr(toglob, DOPAT|DOTILDE);
@@ -646,6 +652,10 @@
for (s = toglob; *s; s++) {
if (*s == '\\' s[1])
s++;
+ else if (*s == '\\' !s[1]) {
+ afree(toglob, ATEMP);
+ return NULL;
+ }
else if (*s == '*' || *s == '[' || *s == '?' || *s == '$' ||
(s[1] == '(' /*)*/ strchr(+@!, *s)))
break;
