riscv64: enable FIDO/U2F devices (Yubikeys)
Hello, The following diff enables FIDO/U2F keys on riscv64. I've tested this with a Yubikey 5 Nano and was able to successfully generate an ed25519-sk key pair with ssh-keygen (which is my usecase): icarus$ uname -a OpenBSD icarus.fagg.id.au 7.0 GENERIC.MP#1 riscv64 icarus$ ssh-keygen -t ed25519-sk Generating public/private ed25519-sk key pair. You may need to touch your authenticator to authorize key generation. Enter file in which to save the key (/home/fagg/.ssh/id_ed25519_sk): Enter passphrase (empty for no passphrase): Enter same passphrase again: Your identification has been saved in /home/fagg/.ssh/id_ed25519_sk Your public key has been saved in /home/fagg/.ssh/id_ed25519_sk.pub If there's anything else I should test please let me know. Thanks, Ash Index: sys/arch/riscv64/conf/GENERIC === RCS file: /cvs/src/sys/arch/riscv64/conf/GENERIC,v retrieving revision 1.29 diff -u -p -u -p -r1.29 GENERIC --- sys/arch/riscv64/conf/GENERIC 2 Sep 2021 10:11:21 - 1.29 +++ sys/arch/riscv64/conf/GENERIC 30 Nov 2021 01:58:44 - @@ -133,6 +133,8 @@ uwacom* at uhidev? # USB Wacom tablet wsmouse* at uwacom? mux 0 ukbd* at uhidev? wskbd* at ukbd? mux 1 +uhid* at uhidev? # USB generic HID support +fido* at uhidev? # FIDO/U2F security key support udl* at uhub?# DisplayLink USB displays wsdisplay* at udl? umass* at uhub? Index: sys/arch/riscv64/riscv64/conf.c === RCS file: /cvs/src/sys/arch/riscv64/riscv64/conf.c,v retrieving revision 1.12 diff -u -p -u -p -r1.12 conf.c --- sys/arch/riscv64/riscv64/conf.c 11 Nov 2021 10:03:09 - 1.12 +++ sys/arch/riscv64/riscv64/conf.c 30 Nov 2021 01:58:44 - @@ -91,6 +91,8 @@ cdev_decl(lpt); #include "radio.h" #include "drm.h" cdev_decl(drm); +#include "uhid.h" +#include "fido.h" #include "wsdisplay.h" #include "wskbd.h" @@ -178,7 +180,7 @@ struct cdevsw cdevsw[] = cdev_notdef(), /* 60: i4b phone device */ /* End of reserved slots for isdn4bsd. */ cdev_notdef(), /* 61: USB controller */ - cdev_notdef(), /* 62: USB generic HID */ + cdev_usbdev_init(NUHID,uhid), /* 62: USB generic HID */ cdev_notdef(), /* 63: USB generic driver */ cdev_notdef(), /* 64: USB printers */ cdev_notdef(), /* 65: urio */ @@ -219,7 +221,7 @@ struct cdevsw cdevsw[] = cdev_notdef(), /* 95 */ cdev_ipmi_init(NIPMI,ipmi), /* 96: ipmi */ cdev_notdef(), /* 97: was switch(4) */ - cdev_notdef(), /* 98: FIDO/U2F security key */ + cdev_fido_init(NFIDO,fido), /* 98: FIDO/U2F security key */ cdev_pppx_init(NPPPX,pppac),/* 99: PPP Access Concentrator */ cdev_notdef(), /* 100: USB joystick/gamecontroller */ };
Re: ahci(4): Add support for JMicron JMB585 chipset
Stuart Henderson writes: > That FreeBSD commit prevents using their "hw.ahci.force" tunable on the > device, it's used for attaching as AHCI to certain known chips even if > they're set in legacy IDE mode. > > Does it work to just add the vid/pid to the ahci_devices[] array > without a specific attach function? (like PCI_PRODUCT_ASMEDIA_ASM1061_SATA). After a little discussion off-list with Jonathan Matthew, I think what I've experienced here is actually an SSD that is on it's way to the great silicon graveyard. What I was experiencing was brief periods (sometimes a couple of seconds) of what seemed like I/O related pauses when trying to access files from that disk (the machine remained responsive the whole time to ssh, tmux etc but whatever process was trying to access that disk would hiccup briefly). However, I experienced it *only* with one particular drive (and it's quite an older one) - other drives did not do this whether regardless of NCQ being on or off. I had not experienced this with that drive in other systems previously. But after doing some testing again today I've now experienced the same hang on a completely different machine with a completely different AHCI controller with the drive getting somewhat warm to the touch when idle - so given that the SSD in question is ~7 years old, I'm going to put it out to pasture. I selected NCQ because IIRC it's the first feature knockout mask listed in the header - it just happened to seem like it worked. Me Googling and misreading the FreeBSD commit wasn't the trigger for selecting NCQ anyway - I was just looking for confirmation I wasn't going crazy but apparently I am. :-) Based on that I *think* attaching it just like the ASMedia device will work, but let me test that a little more thoroughly before I send an updated diff. Thanks for the review and sorry for the confusion, Ash
iwm(4): enable on riscv64
The following diffs adds iwm(4) to the riscv64 kernel config. I tested this with the following device: iwm0 at pci5 dev 0 function 0 "Intel Dual Band Wireless-AC 9260" rev 0x29, intx icarus$ ifconfig iwm0 iwm0: flags=808843 mtu 1500 lladdr bc:54:2f:cb:3b:21 index 2 priority 4 llprio 3 groups: wlan egress media: IEEE802.11 autoselect (HT-MCS0 mode 11n) status: active ieee80211: nwid chan 36 bssid 18:4b:0d:17:c0:ac 100% wpakey wpaprotos wpa2 wpaakms psk wpaciphers ccmp wpagroupcipher ccmp inet 192.168.1.76 netmask 0xff00 broadcast 192.168.1.255 Patch and dmesg to follow. fw_update went fine and after enabling debugging on the driver it doesn't seem to be throwing firmware errors or anything like that. This is my only iwm(4) compatible device unfortunately so I can't test with any others. This seems to be happily passing packets. Index: sys/arch/riscv64/conf/GENERIC === RCS file: /cvs/src/sys/arch/riscv64/conf/GENERIC,v retrieving revision 1.26 diff -u -p -u -p -r1.26 GENERIC --- sys/arch/riscv64/conf/GENERIC 12 Jul 2021 19:11:42 - 1.26 +++ sys/arch/riscv64/conf/GENERIC 23 Jul 2021 20:26:25 - @@ -95,6 +95,9 @@ em* at pci? # Intel Pro/1000 Ethernet bge* at pci? # Broadcom BCM57xx (aka Tigon3) oce* at pci? # Emulex OneConnect 10Gb ethernet +# Wireless network cards +iwm* at pci? # Intel WiFi Link 7xxx + nvme* at pci? # NVMe controllers ahci* at pci? # AHCI SATA controllers dmesg: OpenBSD 6.9-current (GENERIC) #6: Fri Jul 23 16:16:13 EDT 2021 f...@icarus.fagg.id.au:/usr/src/sys/arch/riscv64/compile/GENERIC real mem = 17179869184 (16384MB) avail mem = 16416555008 (15656MB) random: good seed from bootblocks mainbus0 at root: SiFive HiFive Unmatched A00 cpu0 at mainbus0: SiFive U7 imp 20181004 rv64imafdc intc0 at cpu0 cpu0: 32KB 64b/line 128-way L1 I-cache, 32KB 64b/line 64-way L1 D-cache cpu0: 2048KB 64b/line 2048-way L2 cache "fit-images" at mainbus0 not configured simplebus0 at mainbus0: "soc" plic0 at simplebus0 sfclock0 at simplebus0 sfuart0 at simplebus0: console sfuart1 at simplebus0 ociic0 at simplebus0 iic0 at ociic0 titmp0 at iic0 addr 0x4c dapmic0 at iic0 addr 0x58 "spi" at simplebus0 not configured "spi" at simplebus0 not configured cad0 at simplebus0: rev 0x10070109, address 70:b3:d5:92:f9:7b ukphy0 at cad0 phy 0: Generic IEEE 802.3u media interface, rev. 2: OUI 0x0001c1, model 0x0037 "pwm" at simplebus0 not configured "pwm" at simplebus0 not configured "cache-controller" at simplebus0 not configured "gpio" at simplebus0 not configured dwpcie0 at simplebus0 "clint" at simplebus0 not configured "dmc" at simplebus0 not configured pci0 at dwpcie0 ppb0 at pci0 dev 0 function 0 "SiFive PCIe" rev 0x00 pci1 at ppb0 bus 1 ppb1 at pci1 dev 0 function 0 "ASMedia ASM2824" rev 0x01 pci2 at ppb1 bus 2 ppb2 at pci2 dev 0 function 0 "ASMedia ASM2824" rev 0x01: intx pci3 at ppb2 bus 3 ppb3 at pci2 dev 2 function 0 "ASMedia ASM2824" rev 0x01: intx pci4 at ppb3 bus 4 xhci0 at pci4 dev 0 function 0 "ASMedia ASM1042A xHCI" rev 0x00: intx, xHCI 1.0 usb0 at xhci0: USB revision 3.0 uhub0 at usb0 configuration 1 interface 0 "ASMedia xHCI root hub" rev 3.00/1.00 addr 1 ppb4 at pci2 dev 3 function 0 "ASMedia ASM2824" rev 0x01: intx pci5 at ppb4 bus 5 iwm0 at pci5 dev 0 function 0 "Intel Dual Band Wireless-AC 9260" rev 0x29, intx ppb5 at pci2 dev 4 function 0 "ASMedia ASM2824" rev 0x01: intx pci6 at ppb5 bus 6 nvme0 at pci6 dev 0 function 0 vendor "Silicon Motion", unknown product 0x2263 rev 0x03: intx, NVMe 1.3 nvme0: Inland NVMe SSD 256GB, firmware T0918A0L, serial IBSCMC210625600201 scsibus0 at nvme0: 2 targets, initiator 0 sd0 at scsibus0 targ 1 lun 0: sd0: 244198MB, 512 bytes/sector, 500118192 sectors ppb6 at pci2 dev 8 function 0 "ASMedia ASM2824" rev 0x01: intx pci7 at ppb6 bus 7 "hfclk" at mainbus0 not configured "rtcclk" at mainbus0 not configured "gpio-poweroff" at mainbus0 not configured uhub1 at uhub0 port 2 configuration 1 interface 0 "ASMedia AS2107" rev 3.00/0.01 addr 2 uhub2 at uhub0 port 4 configuration 1 interface 0 "ASMedia AS2107" rev 2.10/0.01 addr 3 "vendor 0x8087 product 0x0025" rev 2.00/0.02 addr 4 at uhub2 port 4 not configured vscsi0 at root scsibus1 at vscsi0: 256 targets softraid0 at root scsibus2 at softraid0: 256 targets root on sd0a (298fb3493efcd19a.a) swap on sd0b dump on sd0b iwm0: hw rev 0x320, fw ver 46.6b541b68.0, address bc:54:2f:cb:3b:21
ahci(4): Add support for JMicron JMB585 chipset
I have two devices here based on the JMicron JMB585 chipset. This diff
adds the required pcidev IDs and sets disables native command queuing in
the driver. FreeBSD does something similar for this device:
https://github.com/freebsd/freebsd-src/commit/16b766eed443043f4216d50e40ba283e74f992c2
I've tested both devices on amd64 and riscv64 and it seems to be working well.
On my amd64 machine dmesg shows:
ahci0 at pci1 dev 0 function 0 "JMicron JMB585 SATA/AHCI" rev 0x00: msi,
AHCI 1.3.1
Thanks,
Ash
Index: sys/dev/pci/pcidevs
===
RCS file: /cvs/src/sys/dev/pci/pcidevs,v
retrieving revision 1.1975
diff -u -p -u -p -r1.1975 pcidevs
--- sys/dev/pci/pcidevs 18 Jul 2021 05:02:08 - 1.1975
+++ sys/dev/pci/pcidevs 23 Jul 2021 01:27:53 -
@@ -6400,6 +6400,7 @@ product ITT ITT3204 0x0002 ITT3204 MPEG
/* JMicron */
product JMICRON JMC250 0x0250 JMC250
product JMICRON JMC260 0x0260 JMC260
+product JMICRON JMB585 0x0585 JMB585 SATA/AHCI
product JMICRON JMB360 0x2360 JMB360 SATA
product JMICRON JMB361 0x2361 JMB361 IDE/SATA
product JMICRON JMB362 0x2362 JMB362 SATA
Index: sys/dev/pci/ahci_pci.c
===
RCS file: /cvs/src/sys/dev/pci/ahci_pci.c,v
retrieving revision 1.15
diff -u -p -u -p -r1.15 ahci_pci.c
--- sys/dev/pci/ahci_pci.c 3 Aug 2018 22:18:13 - 1.15
+++ sys/dev/pci/ahci_pci.c 23 Jul 2021 01:28:02 -
@@ -76,6 +76,8 @@ int ahci_amd_hudson2_attach(struct ahc
struct pci_attach_args *);
intahci_intel_attach(struct ahci_softc *,
struct pci_attach_args *);
+intahci_jmicron_jmb58x_attach(struct ahci_softc *,
+ struct pci_attach_args *);
intahci_samsung_attach(struct ahci_softc *,
struct pci_attach_args *);
@@ -147,6 +149,9 @@ static const struct ahci_device ahci_dev
{ PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_EP80579_AHCI,
NULL, ahci_intel_attach },
+ { PCI_VENDOR_JMICRON, PCI_PRODUCT_JMICRON_JMB585,
+ NULL, ahci_jmicron_jmb58x_attach },
+
{ PCI_VENDOR_SAMSUNG2, PCI_PRODUCT_SAMSUNG2_S4LN053X01,
NULL, ahci_samsung_attach },
{ PCI_VENDOR_SAMSUNG2, PCI_PRODUCT_SAMSUNG2_XP941,
@@ -288,6 +293,14 @@ ahci_samsung_attach(struct ahci_softc *s
* https://bugzilla.kernel.org/show_bug.cgi?id=89171
*/
sc->sc_flags |= AHCI_F_NO_MSI;
+
+ return (0);
+}
+
+int
+ahci_jmicron_jmb58x_attach(struct ahci_softc *sc, struct pci_attach_args *pa)
+{
+ sc->sc_flags |= AHCI_F_NO_NCQ;
return (0);
}
Re: ix(4)/riscv64: Make ix(4) work when MSI-X interrupts aren't available
Jonathan Matthew writes: > Yes, on second look that makes sense. Here's a better diff with that > change, and that also doesn't break arches without __HAVE_PCI_MSIX. > ok? Jonathan/Mark, Thank you for your help here. Just confirming that does indeed work for me. Any chance the other part of my diff could go in as well? Thanks, Ash Index: sys/arch/riscv64/conf/GENERIC === RCS file: /cvs/src/sys/arch/riscv64/conf/GENERIC,v retrieving revision 1.26 diff -u -p -u -p -r1.26 GENERIC --- sys/arch/riscv64/conf/GENERIC 12 Jul 2021 19:11:42 - 1.26 +++ sys/arch/riscv64/conf/GENERIC 19 Jul 2021 22:53:38 - @@ -94,6 +94,7 @@ wsdisplay*at radeondrm? em*at pci? # Intel Pro/1000 Ethernet bge* at pci? # Broadcom BCM57xx (aka Tigon3) oce* at pci? # Emulex OneConnect 10Gb ethernet +ix*at pci? # Intel 82598EB 10Gb ethernet nvme* at pci? # NVMe controllers ahci* at pci? # AHCI SATA controllers
Re: vscsi/iscsid: wait for scsi_probe to complete after connections are established
Friendly ping.
Ashton Fagg writes:
> I have been working on fixing an issue (which was partially fixed by a
> diff I sent in earlier this year) with iscsid. With iscsi disks in
> /etc/fstab, sometimes the devices aren't fully up and ready before fsck
> tries to run - causing the machine to go into single user mode on boot.
>
> The diff that was merged earlier in the year added a poll routine which
> monitors for connection success before letting iscsictl return. This
> fixed the issue in some cases, however it's still only half the fix. The
> principled fix is to, additionally, wait until the scsi_probe calls are
> complete - at which point we can reasonably assume the disk device are
> ready for use. This requires some work to the vscsi driver to make this
> happen, as well as changes to both iscsid and iscsictl. The diffs
> attached here are a full implementation of this.
>
> I was still encountering this issue on one of my machines (much slower
> than my normal machine), where the connections would be up but the
> scsi_probe calls would not have completed - even with my earlier diff
> this would still cause the machine to go to single user mode. However,
> this indeed fixes that problem completely and I've been running it for a
> couple of weeks with no problems.
>
> The diffs are designed to be applied in the order they appear. In
> summary, the proposed changes are:
>
> (1) taskq.diff adds a taskq_empty function, which lets us check to see
> if a taskq is, well, empty. This is used in (2). Updates the man pages
> for taskq/task_add to reflect this.
>
> (2) vscsi.diff adds an ioctl to the vscsi driver which lets us monitor
> for device event queue completion. To aid in this it also separates
> calls to scsi_probe and scsi_detach to a dedicated taskq, rather than
> using systq. Updates the man pages for vscsi to reflect this.
>
> (3) iscsid.diff does the plumbing to actually call the new ioctl and
> provide that information back to iscsictl during status poll.
>
> (4) iscsictl.diff integrates the device queue information into the
> polling routine. Updates the man page for iscsictl to describe the new
> behavior.
>
> Based on commmit messages around vscsi it seems there was some plan to
> do this quite some years ago but it's hard for me to know what the
> proposed method was (though I assume what was envisaged might be similar
> to something like this).
>
> Feedback sought and greatly welcomed. I am basically certain there are
> ways this can be improved.
>
> Thanks,
>
> Ash
Index: share/man/man9/task_add.9
===
RCS file: /cvs/src/share/man/man9/task_add.9,v
retrieving revision 1.22
diff -u -p -u -p -r1.22 task_add.9
--- share/man/man9/task_add.9 8 Jun 2020 00:29:51 - 1.22
+++ share/man/man9/task_add.9 13 Jul 2021 23:45:59 -
@@ -20,6 +20,7 @@
.Sh NAME
.Nm taskq_create ,
.Nm taskq_destroy ,
+.Nm taskq_empty ,
.Nm taskq_barrier ,
.Nm taskq_del_barrier ,
.Nm task_set ,
@@ -43,6 +44,8 @@
.Fn taskq_barrier "struct taskq *tq"
.Ft void
.Fn taskq_del_barrier "struct taskq *tq" "struct task *t"
+.Ft int
+.Fn taskq_empty "struct taskq *tq"
.Ft void
.Fn task_set "struct task *t" "void (*fn)(void *)" "void *arg"
.Ft int
@@ -86,6 +89,9 @@ argument:
The threads servicing the taskq will be run without the kernel big lock.
.El
.Pp
+.Fn taskq_empty
+indicates whether there are any queued tasks.
+.Pp
.Fn taskq_destroy
causes the resources associated with a previously created taskq to be freed.
It will wait till all the tasks in the work queue are completed before
@@ -220,6 +226,9 @@ or 0 if the task was not already on the
.Fn task_pending
will return non-zero if the task is queued to run, or 0 if the task
is not queued.
+.Pp
+.Fn taskq_empty
+will return 1 if there are no tasks queued, or 0 otherwise.
.Sh SEE ALSO
.Xr autoconf 9 ,
.Xr spl 9
Index: sys/kern/kern_task.c
===
RCS file: /cvs/src/sys/kern/kern_task.c,v
retrieving revision 1.31
diff -u -p -u -p -r1.31 kern_task.c
--- sys/kern/kern_task.c 1 Aug 2020 08:40:20 - 1.31
+++ sys/kern/kern_task.c 13 Jul 2021 23:46:00 -
@@ -394,6 +394,18 @@ task_del(struct taskq *tq, struct task *
}
int
+taskq_empty(struct taskq *tq)
+{
+ int rv;
+
+ mtx_enter(&tq->tq_mtx);
+ rv = TAILQ_EMPTY(&tq->tq_worklist);
+ mtx_leave(&tq->tq_mtx);
+
+ return (rv);
+}
+
+int
taskq_next_work(struct taskq *tq, struct task *work)
{
struct task *next;
Index: sys/sys/task.h
===
RCS file: /cvs/src/sys/sys/task.h,v
retrieving revision 1.18
diff -u -p -u -p -r1.18 task.h
--- sys/sys/task.h 1 Aug 2020 08:
iwx(4): Whitespace fix
Found this while poking around - an extra newline in if_iwx.c.
Index: sys/dev/pci/if_iwx.c
===
RCS file: /cvs/src/sys/dev/pci/if_iwx.c,v
retrieving revision 1.69
diff -u -p -u -p -r1.69 if_iwx.c
--- sys/dev/pci/if_iwx.c 18 Jul 2021 13:07:13 - 1.69
+++ sys/dev/pci/if_iwx.c 20 Jul 2021 01:17:35 -
@@ -502,7 +502,6 @@ iwx_is_mimo_mcs(int mcs)
{
int ridx = iwx_mcs2ridx[mcs];
return iwx_is_mimo_ht_plcp(iwx_rates[ridx].ht_plcp);
-
}
int
ix(4)/riscv64: Make ix(4) work when MSI-X interrupts aren't available
I have an Intel 82599 10 gigabit ethernet card I wanted to get working
on my SiFive Unmatched board.
I found the ix(4) driver has some weirdness around MSI-X
interrupts. While the driver supports operating both with and without
MSI-X support, it's hard-coded via a flag rather than dynamically checking
if it's available. If the flag is set (which it always is right now),
but MSI-X isn't available, the driver will throw an error and the device
won't work:
ix0 at pci7 dev 0 function 0 "Intel 82599" rev 0x01ixgbe_allocate_msix:
pci_intr_map_msix vec 0 failed
The root cause is this call failing in if_ix.c:
if (pci_intr_map_msix(pa, i, &ih)) {
printf("ixgbe_allocate_msix: "
"pci_intr_map_msix vec %d failed\n", i);
error = ENOMEM;
goto fail;
}
Because in _pci_intr_map_msix (in sys/arch/riscv64/dev/pci_machdep.c):
if ((pa->pa_flags & PCI_FLAGS_MSI_ENABLED) == 0 ||
pci_get_capability(pc, tag, PCI_CAP_MSI, NULL, NULL) == 0)
return -1;
The PCI attach flags would not have PCI_FLAGS_MSI_ENABLED set.
The following diff remedies that by checking if PCI_FLAGS_MSI_ENABLED is
actually set, rather than just trying and failing because the hard-coded
flag says so. It also enables ix(4) in the kernel config for
riscv64. Effectively, the driver will now only try to use MSI-X if the
machine is advertising it to be available.
I've tested this on riscv64 (obviously) and an amd64 machine with the
same model card.
On riscv64 the device shows up as:
ix0 at pci7 dev 0 function 0 "Intel 82599" rev 0x01: intx, address
90:e2:ba:29:18:72
And seems to be happily passing packets.
On amd64, the device still shows up as using MSI-X as it did before:
ix0 at pci2 dev 0 function 0 "Intel 82599" rev 0x01, msix, 8 queues,
address 90:e2:ba:29:76:a0
I notice in some of the other drivers for Intel ethernet chipsets, this
same flag exists. I don't know if it's preferable to make this same
change to them nor do I have the hardware to test such a change, but if
this is something that's of interest I'd be happy to do it.
Thanks!
Index: sys/arch/riscv64/conf/GENERIC
===
RCS file: /cvs/src/sys/arch/riscv64/conf/GENERIC,v
retrieving revision 1.26
diff -u -p -u -p -r1.26 GENERIC
--- sys/arch/riscv64/conf/GENERIC 12 Jul 2021 19:11:42 - 1.26
+++ sys/arch/riscv64/conf/GENERIC 19 Jul 2021 22:53:38 -
@@ -94,6 +94,7 @@ wsdisplay* at radeondrm?
em* at pci? # Intel Pro/1000 Ethernet
bge* at pci? # Broadcom BCM57xx (aka Tigon3)
oce* at pci? # Emulex OneConnect 10Gb ethernet
+ix* at pci? # Intel 82598EB 10Gb ethernet
nvme* at pci? # NVMe controllers
ahci* at pci? # AHCI SATA controllers
Index: sys/dev/pci/if_ix.c
===
RCS file: /cvs/src/sys/dev/pci/if_ix.c,v
retrieving revision 1.178
diff -u -p -u -p -r1.178 if_ix.c
--- sys/dev/pci/if_ix.c 22 Dec 2020 23:25:37 - 1.178
+++ sys/dev/pci/if_ix.c 19 Jul 2021 22:53:38 -
@@ -202,7 +202,6 @@ struct cfattach ix_ca = {
};
int ixgbe_smart_speed = ixgbe_smart_speed_on;
-int ixgbe_enable_msix = 1;
/*
* Device identification routine
@@ -1780,7 +1779,7 @@ ixgbe_setup_msix(struct ix_softc *sc)
int nmsix;
unsigned int maxq;
- if (!ixgbe_enable_msix)
+ if (!(pa->pa_flags & PCI_FLAGS_MSI_ENABLED))
return;
nmsix = pci_intr_msix_count(pa->pa_pc, pa->pa_tag);
Re: vscsi/iscsid: wait for scsi_probe to complete after connections are established
Friendly weekly ping. Patches reattached.
- ajf
Ashton Fagg writes:
> I have been working on fixing an issue (which was partially fixed by a
> diff I sent in earlier this year) with iscsid. With iscsi disks in
> /etc/fstab, sometimes the devices aren't fully up and ready before fsck
> tries to run - causing the machine to go into single user mode on boot.
>
> The diff that was merged earlier in the year added a poll routine which
> monitors for connection success before letting iscsictl return. This
> fixed the issue in some cases, however it's still only half the fix. The
> principled fix is to, additionally, wait until the scsi_probe calls are
> complete - at which point we can reasonably assume the disk device are
> ready for use. This requires some work to the vscsi driver to make this
> happen, as well as changes to both iscsid and iscsictl. The diffs
> attached here are a full implementation of this.
>
> I was still encountering this issue on one of my machines (much slower
> than my normal machine), where the connections would be up but the
> scsi_probe calls would not have completed - even with my earlier diff
> this would still cause the machine to go to single user mode. However,
> this indeed fixes that problem completely and I've been running it for a
> couple of weeks with no problems.
>
> The diffs are designed to be applied in the order they appear. In
> summary, the proposed changes are:
>
> (1) taskq.diff adds a taskq_empty function, which lets us check to see
> if a taskq is, well, empty. This is used in (2). Updates the man pages
> for taskq/task_add to reflect this.
>
> (2) vscsi.diff adds an ioctl to the vscsi driver which lets us monitor
> for device event queue completion. To aid in this it also separates
> calls to scsi_probe and scsi_detach to a dedicated taskq, rather than
> using systq. Updates the man pages for vscsi to reflect this.
>
> (3) iscsid.diff does the plumbing to actually call the new ioctl and
> provide that information back to iscsictl during status poll.
>
> (4) iscsictl.diff integrates the device queue information into the
> polling routine. Updates the man page for iscsictl to describe the new
> behavior.
>
> Based on commmit messages around vscsi it seems there was some plan to
> do this quite some years ago but it's hard for me to know what the
> proposed method was (though I assume what was envisaged might be similar
> to something like this).
>
> Feedback sought and greatly welcomed. I am basically certain there are
> ways this can be improved.
>
> Thanks,
>
> Ash
Index: share/man/man9/task_add.9
===
RCS file: /cvs/src/share/man/man9/task_add.9,v
retrieving revision 1.22
diff -u -p -u -p -r1.22 task_add.9
--- share/man/man9/task_add.9 8 Jun 2020 00:29:51 - 1.22
+++ share/man/man9/task_add.9 13 Jul 2021 23:45:59 -
@@ -20,6 +20,7 @@
.Sh NAME
.Nm taskq_create ,
.Nm taskq_destroy ,
+.Nm taskq_empty ,
.Nm taskq_barrier ,
.Nm taskq_del_barrier ,
.Nm task_set ,
@@ -43,6 +44,8 @@
.Fn taskq_barrier "struct taskq *tq"
.Ft void
.Fn taskq_del_barrier "struct taskq *tq" "struct task *t"
+.Ft int
+.Fn taskq_empty "struct taskq *tq"
.Ft void
.Fn task_set "struct task *t" "void (*fn)(void *)" "void *arg"
.Ft int
@@ -86,6 +89,9 @@ argument:
The threads servicing the taskq will be run without the kernel big lock.
.El
.Pp
+.Fn taskq_empty
+indicates whether there are any queued tasks.
+.Pp
.Fn taskq_destroy
causes the resources associated with a previously created taskq to be freed.
It will wait till all the tasks in the work queue are completed before
@@ -220,6 +226,9 @@ or 0 if the task was not already on the
.Fn task_pending
will return non-zero if the task is queued to run, or 0 if the task
is not queued.
+.Pp
+.Fn taskq_empty
+will return 1 if there are no tasks queued, or 0 otherwise.
.Sh SEE ALSO
.Xr autoconf 9 ,
.Xr spl 9
Index: sys/kern/kern_task.c
===
RCS file: /cvs/src/sys/kern/kern_task.c,v
retrieving revision 1.31
diff -u -p -u -p -r1.31 kern_task.c
--- sys/kern/kern_task.c 1 Aug 2020 08:40:20 - 1.31
+++ sys/kern/kern_task.c 13 Jul 2021 23:46:00 -
@@ -394,6 +394,18 @@ task_del(struct taskq *tq, struct task *
}
int
+taskq_empty(struct taskq *tq)
+{
+ int rv;
+
+ mtx_enter(&tq->tq_mtx);
+ rv = TAILQ_EMPTY(&tq->tq_worklist);
+ mtx_leave(&tq->tq_mtx);
+
+ return (rv);
+}
+
+int
taskq_next_work(struct taskq *tq, struct task *work)
{
struct task *next;
Index: sys/sys/task.h
===
RCS file: /cvs/src/sys/sys/task.h,v
retrieving revision 1.18
diff -u -p -u -p -r1.18
Re: Cleanup of err(1, "unveil") pattern
Friendly weekly ping.
Ashton Fagg writes:
> Original thread and discussion here:
>
> https://marc.info/?l=openbsd-tech&m=162000231914017&w=2
>
> I started this a couple of months ago but realized I never actually
> finished this and submitted the full diff. So here it is, for whole
> src tree.
>
> I've run this through a `make build` here locally to ensure it builds -
> there's no behavioral changes anywhere, only the formatting of the error
> strings. Hopefully I've not missed any.
>
> If it's preferred for me to break the patch up into smaller pieces,
> that's fine - just let me know. I figured it's probably fine to apply
> all at once since it's related but I could be wrong.
>
> Perhaps another question is whether it's worth documenting this
> convention somewhere - if that's of interest also I'm happy to take a
> stab at that.
Index: bin/ps/ps.c
===
RCS file: /cvs/src/bin/ps/ps.c,v
retrieving revision 1.76
diff -u -p -u -p -r1.76 ps.c
--- bin/ps/ps.c 16 Dec 2019 19:21:16 - 1.76
+++ bin/ps/ps.c 2 Jul 2021 20:40:08 -
@@ -276,18 +276,18 @@ main(int argc, char *argv[])
errx(1, "%s", errbuf);
if (unveil(_PATH_DEVDB, "r") == -1 && errno != ENOENT)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_DEVDB);
if (unveil(_PATH_DEV, "r") == -1 && errno != ENOENT)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_DEV);
if (swapf)
if (unveil(swapf, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", swapf);
if (nlistf)
if (unveil(nlistf, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", nlistf);
if (memf)
if (unveil(memf, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", memf);
if (pledge("stdio rpath getpw ps", NULL) == -1)
err(1, "pledge");
Index: games/tetris/tetris.c
===
RCS file: /cvs/src/games/tetris/tetris.c,v
retrieving revision 1.34
diff -u -p -u -p -r1.34 tetris.c
--- games/tetris/tetris.c 18 May 2019 19:38:25 - 1.34
+++ games/tetris/tetris.c 2 Jul 2021 20:40:09 -
@@ -234,7 +234,7 @@ main(int argc, char *argv[])
scr_init();
if (unveil(scorepath, "rwc") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", scorepath);
if (pledge("stdio rpath wpath cpath tty", NULL) == -1)
err(1, "pledge");
Index: libexec/comsat/comsat.c
===
RCS file: /cvs/src/libexec/comsat/comsat.c,v
retrieving revision 1.49
diff -u -p -u -p -r1.49 comsat.c
--- libexec/comsat/comsat.c 24 Sep 2018 22:56:54 - 1.49
+++ libexec/comsat/comsat.c 2 Jul 2021 20:40:17 -
@@ -92,13 +92,13 @@ main(int argc, char *argv[])
}
if (unveil(_PATH_MAILDIR, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_MAILDIR);
if (unveil(_PATH_UTMP, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_UTMP);
if (unveil("/tmp", "w") == -1)
- err(1, "unveil");
+ err(1, "unveil /tmp");
if (unveil(_PATH_DEV, "rw") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_DEV);
if (pledge("stdio rpath wpath proc tty", NULL) == -1)
err(1, "pledge");
Index: libexec/fingerd/fingerd.c
===
RCS file: /cvs/src/libexec/fingerd/fingerd.c,v
retrieving revision 1.41
diff -u -p -u -p -r1.41 fingerd.c
--- libexec/fingerd/fingerd.c 28 Jun 2019 13:32:53 - 1.41
+++ libexec/fingerd/fingerd.c 2 Jul 2021 20:40:17 -
@@ -109,7 +109,7 @@ main(int argc, char *argv[])
}
if (unveil(prog, "x") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", prog);
if (pledge("stdio inet dns proc exec", NULL) == -1)
err(1, "pledge");
Index: libexec/lockspool/lockspool.c
===
RCS file: /cvs/src/libexec/lockspool/lockspool.c,v
retrieving revision 1.21
diff -u -p -u -p -r1.21 lockspool.c
--- libexec/lockspool/lockspool.c 9 Feb 2020 14:59:20 - 1.21
+++ libexec/lockspool/lockspool.c 2 Jul 2021 20:40:17 -
@@ -54,7 +54,7 @@ main(int argc, char *argv[])
int holdfd;
if (unveil(_PATH_MAILDIR, "rwc") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_MAILDIR);
if (pledge("stdio rpath wpath getpw cpath fattr", NULL) == -1)
err(1, "pledge");
Index: libexec/spamlogd/spamlogd.c
==
Re: vscsi/iscsid: wait for scsi_probe to complete after connections are established
Ping again. Diffs re-attached below.
Ashton Fagg writes:
> Friendly ping, really hoping someone can take a look. Diffs re-attached.
>
> Thanks,
>
> Ash
>
> Ashton Fagg writes:
>
>> Updated diffs attached.
>>
>> - I read style(9) a little more closely and worked out I had some
>> issues, so I corrected those.
>>
>> - Revisited the wording in my proposed documentation to make things a
>> clearer.
>>
>> - My mandoc changes were not lint clean - also fixed.
>>
>> No functional changes to the original diffs.
>>
>> Thanks,
>> ajf
>>
>> Ashton Fagg writes:
>>
>>> I have been working on fixing an issue (which was partially fixed by a
>>> diff I sent in earlier this year) with iscsid. With iscsi disks in
>>> /etc/fstab, sometimes the devices aren't fully up and ready before fsck
>>> tries to run - causing the machine to go into single user mode on boot.
>>>
>>> The diff that was merged earlier in the year added a poll routine which
>>> monitors for connection success before letting iscsictl return. This
>>> fixed the issue in some cases, however it's still only half the fix. The
>>> principled fix is to, additionally, wait until the scsi_probe calls are
>>> complete - at which point we can reasonably assume the disk device are
>>> ready for use. This requires some work to the vscsi driver to make this
>>> happen, as well as changes to both iscsid and iscsictl. The diffs
>>> attached here are a full implementation of this.
>>>
>>> I was still encountering this issue on one of my machines (much slower
>>> than my normal machine), where the connections would be up but the
>>> scsi_probe calls would not have completed - even with my earlier diff
>>> this would still cause the machine to go to single user mode. However,
>>> this indeed fixes that problem completely and I've been running it for a
>>> couple of weeks with no problems.
>>>
>>> The diffs are designed to be applied in the order they appear. In
>>> summary, the proposed changes are:
>>>
>>> (1) taskq.diff adds a taskq_empty function, which lets us check to see
>>> if a taskq is, well, empty. This is used in (2). Updates the man pages
>>> for taskq/task_add to reflect this.
>>>
>>> (2) vscsi.diff adds an ioctl to the vscsi driver which lets us monitor
>>> for device event queue completion. To aid in this it also separates
>>> calls to scsi_probe and scsi_detach to a dedicated taskq, rather than
>>> using systq. Updates the man pages for vscsi to reflect this.
>>>
>>> (3) iscsid.diff does the plumbing to actually call the new ioctl and
>>> provide that information back to iscsictl during status poll.
>>>
>>> (4) iscsictl.diff integrates the device queue information into the
>>> polling routine. Updates the man page for iscsictl to describe the new
>>> behavior.
>>>
>>> Based on commmit messages around vscsi it seems there was some plan to
>>> do this quite some years ago but it's hard for me to know what the
>>> proposed method was (though I assume what was envisaged might be similar
>>> to something like this).
>>>
>>> Feedback sought and greatly welcomed. I am basically certain there are
>>> ways this can be improved.
Index: sys/kern/kern_task.c
===
RCS file: /cvs/src/sys/kern/kern_task.c,v
retrieving revision 1.31
diff -u -p -u -p -r1.31 kern_task.c
--- sys/kern/kern_task.c 1 Aug 2020 08:40:20 - 1.31
+++ sys/kern/kern_task.c 20 Jun 2021 22:05:13 -
@@ -394,6 +394,18 @@ task_del(struct taskq *tq, struct task *
}
int
+taskq_empty(struct taskq *tq)
+{
+ int rv;
+
+ mtx_enter(&tq->tq_mtx);
+ rv = TAILQ_EMPTY(&tq->tq_worklist);
+ mtx_leave(&tq->tq_mtx);
+
+ return (rv);
+}
+
+int
taskq_next_work(struct taskq *tq, struct task *work)
{
struct task *next;
Index: share/man/man9/task_add.9
===
RCS file: /cvs/src/share/man/man9/task_add.9,v
retrieving revision 1.22
diff -u -p -u -p -r1.22 task_add.9
--- share/man/man9/task_add.9 8 Jun 2020 00:29:51 - 1.22
+++ share/man/man9/task_add.9 20 Jun 2021 22:05:30 -
@@ -20,6 +20,7 @@
.Sh NAME
.Nm taskq_create ,
.Nm taskq_destroy ,
+.Nm taskq_empty ,
.Nm taskq_barrier ,
.Nm taskq_del_barrier ,
.Nm task_set ,
@@ -43,6 +44,8 @@
.Fn taskq_barrier "struct taskq *tq"
.Ft void
.Fn taskq_del_barrier "struct taskq *tq" "struct task *t"
+.Ft int
+.Fn
Cleanup of err(1, "unveil") pattern
Original thread and discussion here:
https://marc.info/?l=openbsd-tech&m=162000231914017&w=2
I started this a couple of months ago but realized I never actually
finished this and submitted the full diff. So here it is, for whole
src tree.
I've run this through a `make build` here locally to ensure it builds -
there's no behavioral changes anywhere, only the formatting of the error
strings. Hopefully I've not missed any.
If it's preferred for me to break the patch up into smaller pieces,
that's fine - just let me know. I figured it's probably fine to apply
all at once since it's related but I could be wrong.
Perhaps another question is whether it's worth documenting this
convention somewhere - if that's of interest also I'm happy to take a
stab at that.
Index: bin/ps/ps.c
===
RCS file: /cvs/src/bin/ps/ps.c,v
retrieving revision 1.76
diff -u -p -u -p -r1.76 ps.c
--- bin/ps/ps.c 16 Dec 2019 19:21:16 - 1.76
+++ bin/ps/ps.c 2 Jul 2021 20:40:08 -
@@ -276,18 +276,18 @@ main(int argc, char *argv[])
errx(1, "%s", errbuf);
if (unveil(_PATH_DEVDB, "r") == -1 && errno != ENOENT)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_DEVDB);
if (unveil(_PATH_DEV, "r") == -1 && errno != ENOENT)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_DEV);
if (swapf)
if (unveil(swapf, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", swapf);
if (nlistf)
if (unveil(nlistf, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", nlistf);
if (memf)
if (unveil(memf, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", memf);
if (pledge("stdio rpath getpw ps", NULL) == -1)
err(1, "pledge");
Index: games/tetris/tetris.c
===
RCS file: /cvs/src/games/tetris/tetris.c,v
retrieving revision 1.34
diff -u -p -u -p -r1.34 tetris.c
--- games/tetris/tetris.c 18 May 2019 19:38:25 - 1.34
+++ games/tetris/tetris.c 2 Jul 2021 20:40:09 -
@@ -234,7 +234,7 @@ main(int argc, char *argv[])
scr_init();
if (unveil(scorepath, "rwc") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", scorepath);
if (pledge("stdio rpath wpath cpath tty", NULL) == -1)
err(1, "pledge");
Index: libexec/comsat/comsat.c
===
RCS file: /cvs/src/libexec/comsat/comsat.c,v
retrieving revision 1.49
diff -u -p -u -p -r1.49 comsat.c
--- libexec/comsat/comsat.c 24 Sep 2018 22:56:54 - 1.49
+++ libexec/comsat/comsat.c 2 Jul 2021 20:40:17 -
@@ -92,13 +92,13 @@ main(int argc, char *argv[])
}
if (unveil(_PATH_MAILDIR, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_MAILDIR);
if (unveil(_PATH_UTMP, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_UTMP);
if (unveil("/tmp", "w") == -1)
- err(1, "unveil");
+ err(1, "unveil /tmp");
if (unveil(_PATH_DEV, "rw") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_DEV);
if (pledge("stdio rpath wpath proc tty", NULL) == -1)
err(1, "pledge");
Index: libexec/fingerd/fingerd.c
===
RCS file: /cvs/src/libexec/fingerd/fingerd.c,v
retrieving revision 1.41
diff -u -p -u -p -r1.41 fingerd.c
--- libexec/fingerd/fingerd.c 28 Jun 2019 13:32:53 - 1.41
+++ libexec/fingerd/fingerd.c 2 Jul 2021 20:40:17 -
@@ -109,7 +109,7 @@ main(int argc, char *argv[])
}
if (unveil(prog, "x") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", prog);
if (pledge("stdio inet dns proc exec", NULL) == -1)
err(1, "pledge");
Index: libexec/lockspool/lockspool.c
===
RCS file: /cvs/src/libexec/lockspool/lockspool.c,v
retrieving revision 1.21
diff -u -p -u -p -r1.21 lockspool.c
--- libexec/lockspool/lockspool.c 9 Feb 2020 14:59:20 - 1.21
+++ libexec/lockspool/lockspool.c 2 Jul 2021 20:40:17 -
@@ -54,7 +54,7 @@ main(int argc, char *argv[])
int holdfd;
if (unveil(_PATH_MAILDIR, "rwc") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_MAILDIR);
if (pledge("stdio rpath wpath getpw cpath fattr", NULL) == -1)
err(1, "pledge");
Index: libexec/spamlogd/spamlogd.c
===
RCS file: /cvs/src/libexec/spamlogd/spamlogd.c,v
retrieving revision 1.31
diff -u -p -u -p -r1.31 spamlogd.c
--- libexec/spamlogd/spamlogd.c 25 Jul 2019 17:32:33 - 1.31
+++ libexec/spamlogd/spamlogd.c 2 Jul 2021 20:40:17 -
@@ -463,7 +463,7 @@ main(int argc, char **argv)
}
if (unveil(PATH_SPAMD_DB, "rw") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", PATH_SPAMD_DB);
if (syncsend) {
if (pledge("stdio rpath wpath inet flock", NULL) == -1)
err(1, "pledge");
Index: regress/sbin/ifconfig/ifaddr.c
===
RCS file: /cvs/src/regress/sbin/ifconfig/ifaddr.c,v
retrieving revision 1.5
diff -u -p -u -p -r
Re: vscsi/iscsid: wait for scsi_probe to complete after connections are established
Friendly ping, really hoping someone can take a look. Diffs re-attached.
Thanks,
Ash
Ashton Fagg writes:
> Updated diffs attached.
>
> - I read style(9) a little more closely and worked out I had some
> issues, so I corrected those.
>
> - Revisited the wording in my proposed documentation to make things a
> clearer.
>
> - My mandoc changes were not lint clean - also fixed.
>
> No functional changes to the original diffs.
>
> Thanks,
> ajf
>
> Ashton Fagg writes:
>
>> I have been working on fixing an issue (which was partially fixed by a
>> diff I sent in earlier this year) with iscsid. With iscsi disks in
>> /etc/fstab, sometimes the devices aren't fully up and ready before fsck
>> tries to run - causing the machine to go into single user mode on boot.
>>
>> The diff that was merged earlier in the year added a poll routine which
>> monitors for connection success before letting iscsictl return. This
>> fixed the issue in some cases, however it's still only half the fix. The
>> principled fix is to, additionally, wait until the scsi_probe calls are
>> complete - at which point we can reasonably assume the disk device are
>> ready for use. This requires some work to the vscsi driver to make this
>> happen, as well as changes to both iscsid and iscsictl. The diffs
>> attached here are a full implementation of this.
>>
>> I was still encountering this issue on one of my machines (much slower
>> than my normal machine), where the connections would be up but the
>> scsi_probe calls would not have completed - even with my earlier diff
>> this would still cause the machine to go to single user mode. However,
>> this indeed fixes that problem completely and I've been running it for a
>> couple of weeks with no problems.
>>
>> The diffs are designed to be applied in the order they appear. In
>> summary, the proposed changes are:
>>
>> (1) taskq.diff adds a taskq_empty function, which lets us check to see
>> if a taskq is, well, empty. This is used in (2). Updates the man pages
>> for taskq/task_add to reflect this.
>>
>> (2) vscsi.diff adds an ioctl to the vscsi driver which lets us monitor
>> for device event queue completion. To aid in this it also separates
>> calls to scsi_probe and scsi_detach to a dedicated taskq, rather than
>> using systq. Updates the man pages for vscsi to reflect this.
>>
>> (3) iscsid.diff does the plumbing to actually call the new ioctl and
>> provide that information back to iscsictl during status poll.
>>
>> (4) iscsictl.diff integrates the device queue information into the
>> polling routine. Updates the man page for iscsictl to describe the new
>> behavior.
>>
>> Based on commmit messages around vscsi it seems there was some plan to
>> do this quite some years ago but it's hard for me to know what the
>> proposed method was (though I assume what was envisaged might be similar
>> to something like this).
>>
>> Feedback sought and greatly welcomed. I am basically certain there are
>> ways this can be improved.
Index: sys/kern/kern_task.c
===
RCS file: /cvs/src/sys/kern/kern_task.c,v
retrieving revision 1.31
diff -u -p -u -p -r1.31 kern_task.c
--- sys/kern/kern_task.c 1 Aug 2020 08:40:20 - 1.31
+++ sys/kern/kern_task.c 20 Jun 2021 22:05:13 -
@@ -394,6 +394,18 @@ task_del(struct taskq *tq, struct task *
}
int
+taskq_empty(struct taskq *tq)
+{
+ int rv;
+
+ mtx_enter(&tq->tq_mtx);
+ rv = TAILQ_EMPTY(&tq->tq_worklist);
+ mtx_leave(&tq->tq_mtx);
+
+ return (rv);
+}
+
+int
taskq_next_work(struct taskq *tq, struct task *work)
{
struct task *next;
Index: share/man/man9/task_add.9
===
RCS file: /cvs/src/share/man/man9/task_add.9,v
retrieving revision 1.22
diff -u -p -u -p -r1.22 task_add.9
--- share/man/man9/task_add.9 8 Jun 2020 00:29:51 - 1.22
+++ share/man/man9/task_add.9 20 Jun 2021 22:05:30 -
@@ -20,6 +20,7 @@
.Sh NAME
.Nm taskq_create ,
.Nm taskq_destroy ,
+.Nm taskq_empty ,
.Nm taskq_barrier ,
.Nm taskq_del_barrier ,
.Nm task_set ,
@@ -43,6 +44,8 @@
.Fn taskq_barrier "struct taskq *tq"
.Ft void
.Fn taskq_del_barrier "struct taskq *tq" "struct task *t"
+.Ft int
+.Fn taskq_empty "struct taskq *tq"
.Ft void
.Fn task_set "struct task *t" "void (*fn)(void *)" "void *arg"
.Ft int
@@ -86,6 +89,9 @@ argument:
The threads servicing the taskq will be run without the kernel big lock.
.El
.Pp
+.Fn taskq_empty
+indicates whether there are any queued tasks.
+.Pp
.Fn taskq_destroy
Whitespace diff for sys/arch/amd64/amd64/identcpu.c
Found these while tinkering with my CPU scaling issue the other day. Two
instances of trailing white-space.
Index: sys/arch/amd64/amd64/identcpu.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/identcpu.c,v
retrieving revision 1.118
diff -u -p -u -p -r1.118 identcpu.c
--- sys/arch/amd64/amd64/identcpu.c 31 Dec 2020 06:22:33 - 1.118
+++ sys/arch/amd64/amd64/identcpu.c 24 Jun 2021 15:42:05 -
@@ -941,7 +941,7 @@ cpu_check_vmm_cap(struct cpu_info *ci)
/* VM Functions available? */
if (msr & (IA32_VMX_ENABLE_VM_FUNCTIONS) << 32) {
ci->ci_vmm_cap.vcc_vmx.vmx_vm_func =
-rdmsr(IA32_VMX_VMFUNC);
+rdmsr(IA32_VMX_VMFUNC);
}
}
}
@@ -1025,7 +1025,7 @@ cpu_check_vmm_cap(struct cpu_info *ci)
* hardware (RDCL_NO), or we may be nested in an VMM that
* is doing flushes (SKIP_L1DFL_VMENTRY) using the MSR.
* In either case no mitigation at all is necessary.
- */
+ */
if (ci->ci_feature_sefflags_edx & SEFF0EDX_ARCH_CAP) {
msr = rdmsr(MSR_ARCH_CAPABILITIES);
if ((msr & ARCH_CAPABILITIES_RDCL_NO) ||
Re: Z590 chipset + i7 10700 CPU - slowness, sysctl hw.cpuspeed/setperf weirdness
Mark Kettenis writes:
> Sounds like Intel changed the way CPU frequency scaling is implemented
> on these new CPUs. Somebody will have to look into this, but many
> OpenBSD developers prefer to buy machines with AMD CPU which is
> probably why this hasn't happened yet.
For those joining us on tech@, original bug report:
https://marc.info/?l=openbsd-bugs&m=162424580212241&w=2
I managed to work out what's happening here. There's a feature called
Intel Hardware P-States (HWP), which is known by the marketing gimmick
name of "Intel SpeedShift". This is apparently something distinct from
SpeedStep.
This is nothing new - it's been around since Skylake apparently. But, my
research indicates it is usually disabled by default. For whatever
reason - whether it be the fact my chipset is very very new, or just by
pure luck - my BIOS *enables* it by default.
Despite the fact there's a separate setting for SpeedStep, if SpeedShift
is enabled, according to Intel's documentation, the chip will no longer
repond to the usual scaling mechanisms (i.e. SpeedStep). So one solution
to this is just to turn it off in the BIOS - this works for me, even
once I re-enabled SpeedStep and C-States, leaving SpeedShift disabled
was enough to get good performance.
The issue is while you can detect that SpeedShift is on at
run-time, you can't turn it off without a reset. The Intel documentation
however suggests a workaround, which is implemented in the patch below.
https://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-vol-3b-part-2-manual.pdf
Seconds 14.4 has all the necessary info.
The workaround consists of this:
- Detect if HWP is enabled and set a flag indicating the package
supports it.
- If it is, don't enable SpeedStep (since it won't work anyway).
Then, for each core:
- Ask what the "maximum performance" is by querying the capabilities
register.
- Request that the minimum and maximum performance be set to this and
call it a day.
(there is an MSR defined in the manual for doing this step for the
*whole* package, but the manual indicates it's not always available - I
tried it on my hardware with no success)
This is kinda ugly as it basically runs the CPU flat-out as if no
scaling was enabled. Unfortunately, *not* doing the second step results
in very poor performance as I was experiencing before. After I found
this I wondered if just disabling the SpeedStep driver was sufficient -
apparently not.
Interestingly, my compile test went from 2min15sec with SpeedShift
disabled in the BIOS, to 1min30sec with this patch (compared to the
7mins in my initial report where scaling wasn't working properly).
I have no idea if anyone will be interested in committing this patch, I
just wanted to share what I learned. The ultimate solution is probably
to either write a driver for this or import the one that's in FreeBSD to
replace SpeedStep for CPUs where HWP is enabled. But I'm probably going
to just disable SpeedShift in the BIOS since that does the job. :-)
Thanks,
-ajf
Index: sys/arch/amd64//amd64/identcpu.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/identcpu.c,v
retrieving revision 1.118
diff -u -p -u -p -r1.118 identcpu.c
--- sys/arch/amd64//amd64/identcpu.c 31 Dec 2020 06:22:33 - 1.118
+++ sys/arch/amd64//amd64/identcpu.c 23 Jun 2021 02:58:59 -
@@ -64,6 +64,7 @@ int amd64_has_aesni;
#endif
int has_rdrand;
int has_rdseed;
+int intel_has_hwp;
const struct {
u_int32_t bit;
@@ -214,6 +215,7 @@ const struct {
}, cpu_tpm_eaxfeatures[] = {
{ TPM_SENSOR, "SENSOR" },
{ TPM_ARAT, "ARAT" },
+ { TPM_HWP, "HWP" },
}, cpu_cpuid_perf_eax[] = {
{ CPUIDEAX_VERID, "PERF" },
}, cpu_cpuid_apmi_edx[] = {
@@ -699,7 +701,24 @@ identifycpu(struct cpu_info *ci)
setperf_setup = k1x_init;
}
- if (cpu_ecxfeature & CPUIDECX_EST)
+ /*
+ * Intel HWP/"SpeedShift" - if enabled, CPU scaling won't work right.
+ * This detects that the package has this capability, but needs to be
+ * configured per core.
+ */
+ intel_has_hwp = 0;
+ if (!strcmp(cpu_vendor, "GenuineIntel") && (ci->ci_feature_tpmflags & TPM_HWP))
+ {
+ uint64_t msr_hwp;
+
+ msr_hwp = rdmsr(IA32_PM_ENABLE);
+ if (msr_hwp & IA32_HWP_ENABLE) {
+printf("SpeedShift detected. SpeedStep will not be enabled, and CPU scaling won't work.\n");
+intel_has_hwp = 1;
+ }
+ }
+
+ if ((cpu_ecxfeature & CPUIDECX_EST) && !intel_has_hwp)
setperf_setup = est_init;
#endif
@@ -729,6 +748,25 @@ identifycpu(struct cpu_info *ci)
sensor_task_register(ci, intelcore_update_sensor, 5);
sensor_attach(&ci->ci_sensordev, &ci->ci_sensor);
sensordev_install(&ci->ci_sensordev);
+ }
+
+ if (intel_has_hwp)
+ {
+ uint64_t msr_cap, msr_req, max_perf;
+
+ msr_cap = rdmsr(IA32_HWP_CAPABILITIES);
+
+ /* Lowest byte is the "highest performance" capability */
+ max_perf = msr_cap & 0xFF;
+
+ /*
+ * Least-signif
Re: vscsi/iscsid: wait for scsi_probe to complete after connections are established
Updated diffs attached.
- I read style(9) a little more closely and worked out I had some
issues, so I corrected those.
- Revisited the wording in my proposed documentation to make things a
clearer.
- My mandoc changes were not lint clean - also fixed.
No functional changes to the original diffs.
Thanks,
ajf
Ashton Fagg writes:
> I have been working on fixing an issue (which was partially fixed by a
> diff I sent in earlier this year) with iscsid. With iscsi disks in
> /etc/fstab, sometimes the devices aren't fully up and ready before fsck
> tries to run - causing the machine to go into single user mode on boot.
>
> The diff that was merged earlier in the year added a poll routine which
> monitors for connection success before letting iscsictl return. This
> fixed the issue in some cases, however it's still only half the fix. The
> principled fix is to, additionally, wait until the scsi_probe calls are
> complete - at which point we can reasonably assume the disk device are
> ready for use. This requires some work to the vscsi driver to make this
> happen, as well as changes to both iscsid and iscsictl. The diffs
> attached here are a full implementation of this.
>
> I was still encountering this issue on one of my machines (much slower
> than my normal machine), where the connections would be up but the
> scsi_probe calls would not have completed - even with my earlier diff
> this would still cause the machine to go to single user mode. However,
> this indeed fixes that problem completely and I've been running it for a
> couple of weeks with no problems.
>
> The diffs are designed to be applied in the order they appear. In
> summary, the proposed changes are:
>
> (1) taskq.diff adds a taskq_empty function, which lets us check to see
> if a taskq is, well, empty. This is used in (2). Updates the man pages
> for taskq/task_add to reflect this.
>
> (2) vscsi.diff adds an ioctl to the vscsi driver which lets us monitor
> for device event queue completion. To aid in this it also separates
> calls to scsi_probe and scsi_detach to a dedicated taskq, rather than
> using systq. Updates the man pages for vscsi to reflect this.
>
> (3) iscsid.diff does the plumbing to actually call the new ioctl and
> provide that information back to iscsictl during status poll.
>
> (4) iscsictl.diff integrates the device queue information into the
> polling routine. Updates the man page for iscsictl to describe the new
> behavior.
>
> Based on commmit messages around vscsi it seems there was some plan to
> do this quite some years ago but it's hard for me to know what the
> proposed method was (though I assume what was envisaged might be similar
> to something like this).
>
> Feedback sought and greatly welcomed. I am basically certain there are
> ways this can be improved.
Index: sys/kern/kern_task.c
===
RCS file: /cvs/src/sys/kern/kern_task.c,v
retrieving revision 1.31
diff -u -p -u -p -r1.31 kern_task.c
--- sys/kern/kern_task.c 1 Aug 2020 08:40:20 - 1.31
+++ sys/kern/kern_task.c 20 Jun 2021 22:05:13 -
@@ -394,6 +394,18 @@ task_del(struct taskq *tq, struct task *
}
int
+taskq_empty(struct taskq *tq)
+{
+ int rv;
+
+ mtx_enter(&tq->tq_mtx);
+ rv = TAILQ_EMPTY(&tq->tq_worklist);
+ mtx_leave(&tq->tq_mtx);
+
+ return (rv);
+}
+
+int
taskq_next_work(struct taskq *tq, struct task *work)
{
struct task *next;
Index: share/man/man9/task_add.9
===
RCS file: /cvs/src/share/man/man9/task_add.9,v
retrieving revision 1.22
diff -u -p -u -p -r1.22 task_add.9
--- share/man/man9/task_add.9 8 Jun 2020 00:29:51 - 1.22
+++ share/man/man9/task_add.9 20 Jun 2021 22:05:30 -
@@ -20,6 +20,7 @@
.Sh NAME
.Nm taskq_create ,
.Nm taskq_destroy ,
+.Nm taskq_empty ,
.Nm taskq_barrier ,
.Nm taskq_del_barrier ,
.Nm task_set ,
@@ -43,6 +44,8 @@
.Fn taskq_barrier "struct taskq *tq"
.Ft void
.Fn taskq_del_barrier "struct taskq *tq" "struct task *t"
+.Ft int
+.Fn taskq_empty "struct taskq *tq"
.Ft void
.Fn task_set "struct task *t" "void (*fn)(void *)" "void *arg"
.Ft int
@@ -86,6 +89,9 @@ argument:
The threads servicing the taskq will be run without the kernel big lock.
.El
.Pp
+.Fn taskq_empty
+indicates whether there are any queued tasks.
+.Pp
.Fn taskq_destroy
causes the resources associated with a previously created taskq to be freed.
It will wait till all the tasks in the work queue are completed before
@@ -220,6 +226,9 @@ or 0 if the task was not already on the
.Fn task_pending
will return non-zero if the task is queued to run, or 0 if the task
is not queued.
+.Pp
+.Fn taskq_empty
+will return 1 if there are no tasks
Re: vscsi/iscsid: wait for scsi_probe to complete after connections are established
A friendly weekly ping.
Thanks,
Ash
Ashton Fagg writes:
> I have been working on fixing an issue (which was partially fixed by a
> diff I sent in earlier this year) with iscsid. With iscsi disks in
> /etc/fstab, sometimes the devices aren't fully up and ready before fsck
> tries to run - causing the machine to go into single user mode on boot.
>
> The diff that was merged earlier in the year added a poll routine which
> monitors for connection success before letting iscsictl return. This
> fixed the issue in some cases, however it's still only half the fix. The
> principled fix is to, additionally, wait until the scsi_probe calls are
> complete - at which point we can reasonably assume the disk device are
> ready for use. This requires some work to the vscsi driver to make this
> happen, as well as changes to both iscsid and iscsictl. The diffs
> attached here are a full implementation of this.
>
> I was still encountering this issue on one of my machines (much slower
> than my normal machine), where the connections would be up but the
> scsi_probe calls would not have completed - even with my earlier diff
> this would still cause the machine to go to single user mode. However,
> this indeed fixes that problem completely and I've been running it for a
> couple of weeks with no problems.
>
> The diffs are designed to be applied in the order they appear. In
> summary, the proposed changes are:
>
> (1) taskq.diff adds a taskq_empty function, which lets us check to see
> if a taskq is, well, empty. This is used in (2). Updates the man pages
> for taskq/task_add to reflect this.
>
> (2) vscsi.diff adds an ioctl to the vscsi driver which lets us monitor
> for device event queue completion. To aid in this it also separates
> calls to scsi_probe and scsi_detach to a dedicated taskq, rather than
> using systq. Updates the man pages for vscsi to reflect this.
>
> (3) iscsid.diff does the plumbing to actually call the new ioctl and
> provide that information back to iscsictl during status poll.
>
> (4) iscsictl.diff integrates the device queue information into the
> polling routine. Updates the man page for iscsictl to describe the new
> behavior.
>
> Based on commmit messages around vscsi it seems there was some plan to
> do this quite some years ago but it's hard for me to know what the
> proposed method was (though I assume what was envisaged might be similar
> to something like this).
>
> Feedback sought and greatly welcomed. I am basically certain there are
> ways this can be improved.
>
> Thanks,
>
> Ash
Index: sys/sys/task.h
===
RCS file: /cvs/src/sys/sys/task.h,v
retrieving revision 1.18
diff -u -p -u -p -r1.18 task.h
--- sys/sys/task.h 1 Aug 2020 08:40:20 - 1.18
+++ sys/sys/task.h 8 Jun 2021 23:42:00 -
@@ -51,6 +51,8 @@ void taskq_barrier(struct taskq *);
void taskq_del_barrier(struct taskq *, struct task *);
+int taskq_empty(struct taskq *);
+
void task_set(struct task *, void (*)(void *), void *);
int task_add(struct taskq *, struct task *);
int task_del(struct taskq *, struct task *);
Index: sys/kern/kern_task.c
===
RCS file: /cvs/src/sys/kern/kern_task.c,v
retrieving revision 1.31
diff -u -p -u -p -r1.31 kern_task.c
--- sys/kern/kern_task.c 1 Aug 2020 08:40:20 - 1.31
+++ sys/kern/kern_task.c 8 Jun 2021 23:42:16 -
@@ -394,6 +394,18 @@ task_del(struct taskq *tq, struct task *
}
int
+taskq_empty(struct taskq *tq)
+{
+ int rv;
+
+ mtx_enter(&tq->tq_mtx);
+ rv = TAILQ_EMPTY(&tq->tq_worklist);
+ mtx_leave(&tq->tq_mtx);
+
+ return (rv);
+}
+
+int
taskq_next_work(struct taskq *tq, struct task *work)
{
struct task *next;
Index: share/man/man9/task_add.9
===
RCS file: /cvs/src/share/man/man9/task_add.9,v
retrieving revision 1.22
diff -u -p -u -p -r1.22 task_add.9
--- share/man/man9/task_add.9 8 Jun 2020 00:29:51 - 1.22
+++ share/man/man9/task_add.9 8 Jun 2021 23:42:29 -
@@ -20,6 +20,7 @@
.Sh NAME
.Nm taskq_create ,
.Nm taskq_destroy ,
+.Nm taskq_empty ,
.Nm taskq_barrier ,
.Nm taskq_del_barrier ,
.Nm task_set ,
@@ -43,6 +44,8 @@
.Fn taskq_barrier "struct taskq *tq"
.Ft void
.Fn taskq_del_barrier "struct taskq *tq" "struct task *t"
+.Ft int
+.Fn taskq_empty "struct taskq *tq"
.Ft void
.Fn task_set "struct task *t" "void (*fn)(void *)" "void *arg"
.Ft int
@@ -86,6 +89,9 @@ argument:
The threads servicing the taskq will be run without the kernel big lock.
.El
.Pp
+.Fn taskq_empty
+indicates whether there are any queued tasks.
+.Pp
.Fn taskq_destroy
causes the resources associated with a pr
Re: pcidevs + azalia: patch for new intel audio
Jonathan Gray writes: > I can't find 0xf0c8 in the datasheets either. > > I've committed this but moved the added line in pcidevs to maintain > ordering by device id. Thanks for the patch. Great, thank you!
Re: pcidevs + azalia: patch for new intel audio
Friendly ping.
Ashton Fagg writes:
> My new Intel Z590-based machine seems to have some different kind of
> Intel audio device onboard.
>
> I couldn't find very much online about it (all the usual pci id
> databases don't seem to have it yet). The only really useful thing I
> found was this:
>
> https://github.com/torvalds/linux/commit/f84d3a1ec375e46a55cc3ba85c04272b24bd3921#diff-bfe681fff464a07274400d493ba696cc6e10649a993ae7c1cfc1c29a106feda0
>
> This doesn't give much info but seems to indicate that it's a variant of
> some existing chip.
>
> I gave this the not very descriptive name of
> "PCI_PRODUCT_INTEL_500SERIES_HDA_2", since that's about all I could come
> up with, since I'm assuming it's a variant of
> "PCI_PRODUCT_INTEL_500SERIES_HDA".
>
> Patch which defines device in pcidevs and tells azalia how to
> configure it is attached. Playback was the only thing I could readily
> test and that's working - so my itch here has been scratched.
>
> I've also attached a dmesg output and a pcidump output (dmesg has also
> been sent to dmesg@).
>
> Feedback greatly welcomed.
>
> Before:
>
> azalia0 at pci0 dev 31 function 3 vendor "Intel", unknown product 0xf0c8 rev
> 0x11: msi
> azalia0: codecs: Realtek/0x0897, 0x/0x, using Realtek/0x0897
> audio0 at azalia0
>
> After:
>
> azalia0 at pci0 dev 31 function 3 "Intel 500 Series HD Audio" rev 0x11: msi
> azalia0: codecs: Realtek/0x0897, 0x/0x, using Realtek/0x0897
> audio0 at azalia0
Index: sys/dev/pci/azalia.c
===
RCS file: /cvs/src/sys/dev/pci/azalia.c,v
retrieving revision 1.262
diff -u -p -u -p -r1.262 azalia.c
--- sys/dev/pci/azalia.c 30 May 2021 02:54:36 - 1.262
+++ sys/dev/pci/azalia.c 1 Jun 2021 01:20:42 -
@@ -470,6 +470,7 @@ azalia_configure_pci(azalia_t *az)
case PCI_PRODUCT_INTEL_400SERIES_LP_HDA:
case PCI_PRODUCT_INTEL_495SERIES_LP_HDA:
case PCI_PRODUCT_INTEL_500SERIES_HDA:
+ case PCI_PRODUCT_INTEL_500SERIES_HDA_2:
case PCI_PRODUCT_INTEL_500SERIES_LP_HDA:
case PCI_PRODUCT_INTEL_C600_HDA:
case PCI_PRODUCT_INTEL_C610_HDA_1:
Index: sys/dev/pci/pcidevs
===
RCS file: /cvs/src/sys/dev/pci/pcidevs,v
retrieving revision 1.1970
diff -u -p -u -p -r1.1970 pcidevs
--- sys/dev/pci/pcidevs 19 May 2021 05:20:48 - 1.1970
+++ sys/dev/pci/pcidevs 1 Jun 2021 01:20:42 -
@@ -5371,6 +5371,7 @@ product INTEL 500SERIES_PCIE_22 0x43c5 5
product INTEL 500SERIES_PCIE_23 0x43c6 500 Series PCIE
product INTEL 500SERIES_PCIE_24 0x43c7 500 Series PCIE
product INTEL 500SERIES_HDA 0x43c8 500 Series HD Audio
+product INTEL 500SERIES_HDA_2 0xf0c8 500 Series HD Audio
product INTEL 500SERIES_THC_0 0x43d0 500 Series THC
product INTEL 500SERIES_THC_1 0x43d1 500 Series THC
product INTEL 500SERIES_AHCI_1 0x43d2 500 Series AHCI
Index: sys/dev/pci/pcidevs.h
===
RCS file: /cvs/src/sys/dev/pci/pcidevs.h,v
retrieving revision 1.1964
diff -u -p -u -p -r1.1964 pcidevs.h
--- sys/dev/pci/pcidevs.h 19 May 2021 05:21:24 - 1.1964
+++ sys/dev/pci/pcidevs.h 1 Jun 2021 01:20:42 -
@@ -5376,6 +5376,7 @@
#define PCI_PRODUCT_INTEL_500SERIES_PCIE_23 0x43c6 /* 500 Series PCIE */
#define PCI_PRODUCT_INTEL_500SERIES_PCIE_24 0x43c7 /* 500 Series PCIE */
#define PCI_PRODUCT_INTEL_500SERIES_HDA 0x43c8 /* 500 Series HD Audio */
+#define PCI_PRODUCT_INTEL_500SERIES_HDA_2 0xf0c8 /* 500 Series HD Audio */
#define PCI_PRODUCT_INTEL_500SERIES_THC_0 0x43d0 /* 500 Series THC */
#define PCI_PRODUCT_INTEL_500SERIES_THC_1 0x43d1 /* 500 Series THC */
#define PCI_PRODUCT_INTEL_500SERIES_AHCI_1 0x43d2 /* 500 Series AHCI */
Index: sys/dev/pci/pcidevs_data.h
===
RCS file: /cvs/src/sys/dev/pci/pcidevs_data.h,v
retrieving revision 1.1959
diff -u -p -u -p -r1.1959 pcidevs_data.h
--- sys/dev/pci/pcidevs_data.h 19 May 2021 05:21:24 - 1.1959
+++ sys/dev/pci/pcidevs_data.h 1 Jun 2021 01:20:43 -
@@ -18912,6 +18912,10 @@ static const struct pci_known_product pc
"500 Series HD Audio",
},
{
+ PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_500SERIES_HDA_2,
+ "500 Series HD Audio",
+ },
+ {
PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_500SERIES_THC_0,
"500 Series THC",
},
vscsi/iscsid: wait for scsi_probe to complete after connections are established
I have been working on fixing an issue (which was partially fixed by a
diff I sent in earlier this year) with iscsid. With iscsi disks in
/etc/fstab, sometimes the devices aren't fully up and ready before fsck
tries to run - causing the machine to go into single user mode on boot.
The diff that was merged earlier in the year added a poll routine which
monitors for connection success before letting iscsictl return. This
fixed the issue in some cases, however it's still only half the fix. The
principled fix is to, additionally, wait until the scsi_probe calls are
complete - at which point we can reasonably assume the disk device are
ready for use. This requires some work to the vscsi driver to make this
happen, as well as changes to both iscsid and iscsictl. The diffs
attached here are a full implementation of this.
I was still encountering this issue on one of my machines (much slower
than my normal machine), where the connections would be up but the
scsi_probe calls would not have completed - even with my earlier diff
this would still cause the machine to go to single user mode. However,
this indeed fixes that problem completely and I've been running it for a
couple of weeks with no problems.
The diffs are designed to be applied in the order they appear. In
summary, the proposed changes are:
(1) taskq.diff adds a taskq_empty function, which lets us check to see
if a taskq is, well, empty. This is used in (2). Updates the man pages
for taskq/task_add to reflect this.
(2) vscsi.diff adds an ioctl to the vscsi driver which lets us monitor
for device event queue completion. To aid in this it also separates
calls to scsi_probe and scsi_detach to a dedicated taskq, rather than
using systq. Updates the man pages for vscsi to reflect this.
(3) iscsid.diff does the plumbing to actually call the new ioctl and
provide that information back to iscsictl during status poll.
(4) iscsictl.diff integrates the device queue information into the
polling routine. Updates the man page for iscsictl to describe the new
behavior.
Based on commmit messages around vscsi it seems there was some plan to
do this quite some years ago but it's hard for me to know what the
proposed method was (though I assume what was envisaged might be similar
to something like this).
Feedback sought and greatly welcomed. I am basically certain there are
ways this can be improved.
Thanks,
Ash
Index: sys/sys/task.h
===
RCS file: /cvs/src/sys/sys/task.h,v
retrieving revision 1.18
diff -u -p -u -p -r1.18 task.h
--- sys/sys/task.h 1 Aug 2020 08:40:20 - 1.18
+++ sys/sys/task.h 8 Jun 2021 23:42:00 -
@@ -51,6 +51,8 @@ void taskq_barrier(struct taskq *);
void taskq_del_barrier(struct taskq *, struct task *);
+int taskq_empty(struct taskq *);
+
void task_set(struct task *, void (*)(void *), void *);
int task_add(struct taskq *, struct task *);
int task_del(struct taskq *, struct task *);
Index: sys/kern/kern_task.c
===
RCS file: /cvs/src/sys/kern/kern_task.c,v
retrieving revision 1.31
diff -u -p -u -p -r1.31 kern_task.c
--- sys/kern/kern_task.c 1 Aug 2020 08:40:20 - 1.31
+++ sys/kern/kern_task.c 8 Jun 2021 23:42:16 -
@@ -394,6 +394,18 @@ task_del(struct taskq *tq, struct task *
}
int
+taskq_empty(struct taskq *tq)
+{
+ int rv;
+
+ mtx_enter(&tq->tq_mtx);
+ rv = TAILQ_EMPTY(&tq->tq_worklist);
+ mtx_leave(&tq->tq_mtx);
+
+ return (rv);
+}
+
+int
taskq_next_work(struct taskq *tq, struct task *work)
{
struct task *next;
Index: share/man/man9/task_add.9
===
RCS file: /cvs/src/share/man/man9/task_add.9,v
retrieving revision 1.22
diff -u -p -u -p -r1.22 task_add.9
--- share/man/man9/task_add.9 8 Jun 2020 00:29:51 - 1.22
+++ share/man/man9/task_add.9 8 Jun 2021 23:42:29 -
@@ -20,6 +20,7 @@
.Sh NAME
.Nm taskq_create ,
.Nm taskq_destroy ,
+.Nm taskq_empty ,
.Nm taskq_barrier ,
.Nm taskq_del_barrier ,
.Nm task_set ,
@@ -43,6 +44,8 @@
.Fn taskq_barrier "struct taskq *tq"
.Ft void
.Fn taskq_del_barrier "struct taskq *tq" "struct task *t"
+.Ft int
+.Fn taskq_empty "struct taskq *tq"
.Ft void
.Fn task_set "struct task *t" "void (*fn)(void *)" "void *arg"
.Ft int
@@ -86,6 +89,9 @@ argument:
The threads servicing the taskq will be run without the kernel big lock.
.El
.Pp
+.Fn taskq_empty
+indicates whether there are any queued tasks.
+.Pp
.Fn taskq_destroy
causes the resources associated with a previously created taskq to be freed.
It will wait till all the tasks in the work queue are completed before
@@ -220,6 +226,11 @@ or 0 if the task was not already on the
.Fn task_pending
will return non-zero if the task is queued to run, or 0 if the task
is not queued.
+.Pp
+.Fn taskq_empty
+will return 1 if there are no tasks queued, or 0 if there is at least
+one task queued.
+
.Sh SEE ALSO
.Xr autoconf 9 ,
.Xr
pcidevs + azalia: patch for new intel audio
My new Intel Z590-based machine seems to have some different kind of
Intel audio device onboard.
I couldn't find very much online about it (all the usual pci id
databases don't seem to have it yet). The only really useful thing I
found was this:
https://github.com/torvalds/linux/commit/f84d3a1ec375e46a55cc3ba85c04272b24bd3921#diff-bfe681fff464a07274400d493ba696cc6e10649a993ae7c1cfc1c29a106feda0
This doesn't give much info but seems to indicate that it's a variant of
some existing chip.
I gave this the not very descriptive name of
"PCI_PRODUCT_INTEL_500SERIES_HDA_2", since that's about all I could come
up with, since I'm assuming it's a variant of
"PCI_PRODUCT_INTEL_500SERIES_HDA".
Patch which defines device in pcidevs and tells azalia how to
configure it is attached. Playback was the only thing I could readily
test and that's working - so my itch here has been scratched.
I've also attached a dmesg output and a pcidump output (dmesg has also
been sent to dmesg@).
Feedback greatly welcomed.
Before:
azalia0 at pci0 dev 31 function 3 vendor "Intel", unknown product 0xf0c8 rev
0x11: msi
azalia0: codecs: Realtek/0x0897, 0x/0x, using Realtek/0x0897
audio0 at azalia0
After:
azalia0 at pci0 dev 31 function 3 "Intel 500 Series HD Audio" rev 0x11: msi
azalia0: codecs: Realtek/0x0897, 0x/0x, using Realtek/0x0897
audio0 at azalia0
Index: sys/dev/pci/azalia.c
===
RCS file: /cvs/src/sys/dev/pci/azalia.c,v
retrieving revision 1.262
diff -u -p -u -p -r1.262 azalia.c
--- sys/dev/pci/azalia.c 30 May 2021 02:54:36 - 1.262
+++ sys/dev/pci/azalia.c 1 Jun 2021 01:20:42 -
@@ -470,6 +470,7 @@ azalia_configure_pci(azalia_t *az)
case PCI_PRODUCT_INTEL_400SERIES_LP_HDA:
case PCI_PRODUCT_INTEL_495SERIES_LP_HDA:
case PCI_PRODUCT_INTEL_500SERIES_HDA:
+ case PCI_PRODUCT_INTEL_500SERIES_HDA_2:
case PCI_PRODUCT_INTEL_500SERIES_LP_HDA:
case PCI_PRODUCT_INTEL_C600_HDA:
case PCI_PRODUCT_INTEL_C610_HDA_1:
Index: sys/dev/pci/pcidevs
===
RCS file: /cvs/src/sys/dev/pci/pcidevs,v
retrieving revision 1.1970
diff -u -p -u -p -r1.1970 pcidevs
--- sys/dev/pci/pcidevs 19 May 2021 05:20:48 - 1.1970
+++ sys/dev/pci/pcidevs 1 Jun 2021 01:20:42 -
@@ -5371,6 +5371,7 @@ product INTEL 500SERIES_PCIE_22 0x43c5 5
product INTEL 500SERIES_PCIE_23 0x43c6 500 Series PCIE
product INTEL 500SERIES_PCIE_24 0x43c7 500 Series PCIE
product INTEL 500SERIES_HDA 0x43c8 500 Series HD Audio
+product INTEL 500SERIES_HDA_2 0xf0c8 500 Series HD Audio
product INTEL 500SERIES_THC_0 0x43d0 500 Series THC
product INTEL 500SERIES_THC_1 0x43d1 500 Series THC
product INTEL 500SERIES_AHCI_1 0x43d2 500 Series AHCI
Index: sys/dev/pci/pcidevs.h
===
RCS file: /cvs/src/sys/dev/pci/pcidevs.h,v
retrieving revision 1.1964
diff -u -p -u -p -r1.1964 pcidevs.h
--- sys/dev/pci/pcidevs.h 19 May 2021 05:21:24 - 1.1964
+++ sys/dev/pci/pcidevs.h 1 Jun 2021 01:20:42 -
@@ -5376,6 +5376,7 @@
#define PCI_PRODUCT_INTEL_500SERIES_PCIE_23 0x43c6 /* 500 Series PCIE */
#define PCI_PRODUCT_INTEL_500SERIES_PCIE_24 0x43c7 /* 500 Series PCIE */
#define PCI_PRODUCT_INTEL_500SERIES_HDA 0x43c8 /* 500 Series HD Audio */
+#define PCI_PRODUCT_INTEL_500SERIES_HDA_2 0xf0c8 /* 500 Series HD Audio */
#define PCI_PRODUCT_INTEL_500SERIES_THC_0 0x43d0 /* 500 Series THC */
#define PCI_PRODUCT_INTEL_500SERIES_THC_1 0x43d1 /* 500 Series THC */
#define PCI_PRODUCT_INTEL_500SERIES_AHCI_1 0x43d2 /* 500 Series AHCI */
Index: sys/dev/pci/pcidevs_data.h
===
RCS file: /cvs/src/sys/dev/pci/pcidevs_data.h,v
retrieving revision 1.1959
diff -u -p -u -p -r1.1959 pcidevs_data.h
--- sys/dev/pci/pcidevs_data.h 19 May 2021 05:21:24 - 1.1959
+++ sys/dev/pci/pcidevs_data.h 1 Jun 2021 01:20:43 -
@@ -18912,6 +18912,10 @@ static const struct pci_known_product pc
"500 Series HD Audio",
},
{
+ PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_500SERIES_HDA_2,
+ "500 Series HD Audio",
+ },
+ {
PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_500SERIES_THC_0,
"500 Series THC",
},
OpenBSD 6.9-current (GENERIC.MP) #7: Mon May 31 21:13:25 EDT 2021
f...@elara.fagg.id.au:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 42702798848 (40724MB)
avail mem = 41393143808 (39475MB)
random: good seed from bootblocks
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 3.3 @ 0x99d71000 (109 entries)
bios0: vendor American Megatrends Inc. version "0405" date 01/14/2021
bios0: ASUS PRIME Z590-V
acpi0 at bios0: ACPI 6.2
acpi0: sleep states S0 S3 S4 S5
acpi0: tables DSDT FACP MCFG FIDT FPDT SSDT SSDT SSDT HPET APIC SSDT SSDT NHLT
SSDT LPIT SSDT SSDT DBGP DBG2 SSDT SSDT WPBT PTDT WSMT
acpi0: wakeup devices PEGP(S4) PEGP(S4) PEGP(S4) PEGP(S4) RP09(S4) PXSX(S4)
RP10(S4) PXSX(S4)
Re: nvme: add timeout to nvme_poll() loop
Ping.
Ashton Fagg writes:
> I noticed this when looking through the nvme.c code the other day.
>
> Currently, nvme_poll() has a loop like this:
>
> while (!ISSET(state.c.flags, htole16(NVME_CQE_PHASE))) {
> if (nvme_q_complete(sc, q) == 0)
> delay(10);
>
> /* XXX no timeout? */
> }
>
> The comment is indicative - there probably should be a timeout here in
> case things are terribly wrong and the queue never gets fully processed.
>
> NetBSD appears to have a similar construct in their version of the
> driver:
>
> https://github.com/NetBSD/src/blob/trunk/sys/dev/ic/nvme.c
>
> The following diff adds a counter to fail the poll after 10 seconds of
> spinning. I've been torturing my test machine (which has an nvme boot
> drive) with this applied - no issues so far.
>
> AFAICT, returning anything non-zero should be ok as it looks like
> nothing specifically checks the return code for anything other being
> zero. But, I may be wrong and perhaps one of the various NVME_CQE_*
> masks is more suitable.
>
> Thanks.
Index: sys/dev/ic/nvme.c
===
RCS file: /cvs/src/sys/dev/ic/nvme.c,v
retrieving revision 1.91
diff -u -p -u -p -r1.91 nvme.c
--- sys/dev/ic/nvme.c 25 Feb 2021 07:30:36 - 1.91
+++ sys/dev/ic/nvme.c 19 May 2021 01:15:28 -
@@ -117,6 +117,16 @@ void nvme_scsi_inquiry(struct scsi_xfer
void nvme_scsi_capacity16(struct scsi_xfer *);
void nvme_scsi_capacity(struct scsi_xfer *);
+/* Here, we define a delay of 10 microseconds between poll attempts.
+ * And, we allow 1e6 attempts. This corresponds to a ~10 second wait. */
+#define NVME_POLL_DELAY_USECS 10
+#define NVME_POLL_MAX_ATTEMPTS 1E6
+
+/* Here, we define a delay of 10 microseconds between poll attempts.
+ * And, we allow 1e6 attempts. This corresponds to a ~10 second wait. */
+#define NVME_POLL_DELAY_USECS 10
+#define NVME_POLL_MAX_ATTEMPTS 1E6
+
#define nvme_read4(_s, _r) \
bus_space_read_4((_s)->sc_iot, (_s)->sc_ioh, (_r))
#define nvme_write4(_s, _r, _v) \
@@ -918,6 +928,7 @@ nvme_poll(struct nvme_softc *sc, struct
void (*done)(struct nvme_softc *, struct nvme_ccb *, struct nvme_cqe *);
void *cookie;
u_int16_t flags;
+ int tries = 0;
memset(&state, 0, sizeof(state));
(*fill)(sc, ccb, &state.s);
@@ -931,9 +942,13 @@ nvme_poll(struct nvme_softc *sc, struct
nvme_q_submit(sc, q, ccb, nvme_poll_fill);
while (!ISSET(state.c.flags, htole16(NVME_CQE_PHASE))) {
if (nvme_q_complete(sc, q) == 0)
- delay(10);
+ delay(NVME_POLL_DELAY_USECS);
- /* XXX no timeout? */
+ if (++tries >= NVME_POLL_MAX_ATTEMPTS) {
+ /* Something appears to be quite wrong... */
+ printf("%s: poll timeout.\n", DEVNAME(sc));
+ return (1);
+ }
}
ccb->ccb_cookie = cookie;
Whitespace fix in sys/scsi/scsi_base.c
Found this while poking around. diff --git a/sys/scsi/scsi_base.c b/sys/scsi/scsi_base.c index 2ba6a702fbc..6769449e2fa 100644 --- a/sys/scsi/scsi_base.c +++ b/sys/scsi/scsi_base.c @@ -478,7 +478,7 @@ scsi_io_get(struct scsi_iopool *iopl, int flags) return NULL; /* otherwise sleep until we get one */ -scsi_ioh_set(&ioh, iopl, scsi_io_get_done, &m); + scsi_ioh_set(&ioh, iopl, scsi_io_get_done, &m); scsi_ioh_add(&ioh); scsi_move(&m);
Re: [Diff] Implement multiple device cloning for hotplug
joshua stein writes: > I'm glad I could inspire you to repost the work I already did years > ago. I'm not sure if you're being sarcastic. > But either way, if a driver is causing a panic because it responds > before it is ready, the same thing would happen without hotplug if > it was communicated with at the wrong time (like some script running > in a loop). Those drivers should just not respond to ioctls or > whatever they are doing before they are ready rather than just > hoping that no one sees their attachment too early. So the issue I have with iscsid is actually different to this because it's entirely in userland. To fix it in a robust way, you need to know when a device has attached and if it's ready or not, but there's extra requirements for iscsid that would require you to match those /dev/sd* devices back to the configured targets. At a minimum, we would need to figure out (after device attachment): a) which scsibus it has shown up on b) if the scsibus it has appeared on is indeed the one you'd expect it to be on (/dev/vscsi0 will have the same scsibus as devices initiated from iscsid, as far as I can tell) c) does the target/lun info match an initiated target with a successful session established. d) then indicate whether the disk device is actually ready There's no panic in this case, the "iscsictl reload" call in the init script returns before the devices are up and ready since we're only looking at connection completion right now. Only returning once we know there's a ready device corresponding to every connected target we expect to have one avoids the problem where fsck tries to do it's thing on devices that aren't ready. Matching back to the iscsi targets avoids some brittleness and edge cases that could be encountered if we were just looking for generic scsi disk attachments/readiness. c) is fairly easy with the diff that I submitted (and was merged) a while ago, since there is book-keeping code inside iscsid to work this out that would be very easy to extend. So the hotplug idea would solve part of this, but not all of it. There's still parts of this that are not easy and aren't clear to me how to fully resolve.
Re: nvme: add timeout to nvme_poll() loop
Ping?
Ashton Fagg writes:
> I noticed this when looking through the nvme.c code the other day.
>
> Currently, nvme_poll() has a loop like this:
>
> while (!ISSET(state.c.flags, htole16(NVME_CQE_PHASE))) {
> if (nvme_q_complete(sc, q) == 0)
> delay(10);
>
> /* XXX no timeout? */
> }
>
> The comment is indicative - there probably should be a timeout here in
> case things are terribly wrong and the queue never gets fully processed.
>
> NetBSD appears to have a similar construct in their version of the
> driver:
>
> https://github.com/NetBSD/src/blob/trunk/sys/dev/ic/nvme.c
>
> The following diff adds a counter to fail the poll after 10 seconds of
> spinning. I've been torturing my test machine (which has an nvme boot
> drive) with this applied - no issues so far.
>
> AFAICT, returning anything non-zero should be ok as it looks like
> nothing specifically checks the return code for anything other being
> zero. But, I may be wrong and perhaps one of the various NVME_CQE_*
> masks is more suitable.
>
> Thanks.
diff --git a/sys/dev/ic/nvme.c b/sys/dev/ic/nvme.c
index 9a79c8b1bfe..4e112f482ee 100644
--- a/sys/dev/ic/nvme.c
+++ b/sys/dev/ic/nvme.c
@@ -117,6 +117,11 @@ void nvme_scsi_inquiry(struct scsi_xfer *);
void nvme_scsi_capacity16(struct scsi_xfer *);
void nvme_scsi_capacity(struct scsi_xfer *);
+/* Here, we define a delay of 10 microseconds between poll attempts.
+ * And, we allow 1e6 attempts. This corresponds to a ~10 second wait. */
+#define NVME_POLL_DELAY_USECS 10
+#define NVME_POLL_MAX_ATTEMPTS 1E6
+
#define nvme_read4(_s, _r) \
bus_space_read_4((_s)->sc_iot, (_s)->sc_ioh, (_r))
#define nvme_write4(_s, _r, _v) \
@@ -918,6 +923,7 @@ nvme_poll(struct nvme_softc *sc, struct nvme_queue *q, struct nvme_ccb *ccb,
void (*done)(struct nvme_softc *, struct nvme_ccb *, struct nvme_cqe *);
void *cookie;
u_int16_t flags;
+ int tries = 0;
memset(&state, 0, sizeof(state));
(*fill)(sc, ccb, &state.s);
@@ -931,9 +937,13 @@ nvme_poll(struct nvme_softc *sc, struct nvme_queue *q, struct nvme_ccb *ccb,
nvme_q_submit(sc, q, ccb, nvme_poll_fill);
while (!ISSET(state.c.flags, htole16(NVME_CQE_PHASE))) {
if (nvme_q_complete(sc, q) == 0)
- delay(10);
+ delay(NVME_POLL_DELAY_USECS);
- /* XXX no timeout? */
+ if (++tries >= NVME_POLL_MAX_ATTEMPTS) {
+ /* Something appears to be quite wrong... */
+ printf("%s: poll timeout.\n", DEVNAME(sc));
+ return (1);
+ }
}
ccb->ccb_cookie = cookie;
Re: [Diff] Implement multiple device cloning for hotplug
"Theo de Raadt" writes: > We don't need a new subsystem. > > We just the operation changed, so it sends the event when the device is ready, > rather than before it is ready. Ok fair enough. More curiosity than anything on my part. I need to find a way to make this information available for iscsid irrespective of the broader issue. I think it might be not "as simple" as just waiting for an attach and subsequent ready event to show up for the iscsi use case. I have some ideas, I'll come back with a diff rather than waste everyone's time on this. But happy to discuss privately if anyone is interested.
Re: [Diff] Implement multiple device cloning for hotplug
On May 11, 2021, at 22:53, Theo de Raadt wrote: > This is because the design of hotplug is completely broken. Thank you, Theo, for the comprehensive reply. This was just the sort of feedback I’d hoped to get. Ok so we have established this is a terrible idea because of the way hotplug works. :) Let’s say someone was interested in solving* this. And let’s assume there was some infrastructure that you signify attach, detach of devices along with basic state (ready, not ready). (* - where solved is some value of “better than what’s there now”) […snip…] > Unfortunately there is no instrumentation in drivers or subsystems to > capture "ready to do work", and create the events at that point instead. > hotplug is hooked into subr_autoconf.c, this code is for handling the > device heirarchy, but it is not involved in operation, and thus unaware > of "readiness". Fixing this would require a subsystem by subsystem study, > figuring out where the glue should exist, and creating the events at > the CORRECT time. Would it make more sense to have a general solution which aggregates all the information from every device? (Maybe something like /dev/devstate, I dunno, names are hard). I could see that maybe this could be a side channel, though, if it’s somehow useful to know the state of a certain device (though no more so than /dev/hotplug already is admittedly). You’d also be getting a firehose of everything even though you might be only interested in disk devices, for example. Or, have something per subsystem? (Infrastructure could be shared, i guess). So for scsi, you might have something like “/dev/scsistate” which provides information to userland stuff on everything that’s under the scsi umbrella. This feels more friendly in that you would unveil only a particular subsystems monitor device, limiting the firehose and also the potential side channel. You might also want to attach more specific information to the message (for scsi, maybe attach LUN and target info in addition to the device name and type). But you pay for that in extra queues, devices etc. Any thoughts? I would be willing to try and develop something, it’s interesting to me and it will hopefully solve my iSCSI mount issues once and for all. :)
[Diff] Implement multiple device cloning for hotplug
Attached is a diff that implements device cloning to allow the
/dev/hotplug device to be cloned (to allow multiple concurrent readers).
Rationale:
Currently, iscsid/iscsictl cannot see when the connections it initiates
results in a device (disk) being attached. Recently, Claudio Jeker
committed a diff I wrote to block "iscsictl reload" until all
connections have been finalized (either connecting successfully, or
notified of failure). This fixes an issue I encountered where iscsi
mounts in /etc/fstab would not be ready at boot time, and would result
in the machine going into single user mode when fsck would try to access
them.
The committed diff fixes my original problem. However, I've now
encountered another one. On one of my machines (far slower than the
first one), the devices themselves are much slower to attach - this
leads to the same race condition as before, as "iscsictl reload" is only
aware that the connections have been initiated, not whether the drives
themselves are actually attached.
What I am hoping here is that I can use hotplug to monitor for device
attach events, and match those to the targets we expect to be attaching
from iscsi. Obviously, we can't just use the hotplug device as is
because potentialy we'd be competing with hotplugd.
I am wondering if this is the right solution...some quick thoughts i'd
love to hear feedback on:
1. Kind of feels like I'm conflating what hotplug is for - this isn't
really "hotplugging" per se. This specific use-case is assuming the
volumes are listed in /etc/fstab.
2. Based on 1, would it be more appropriate to create a separate device
with it's own semantics (potentially very different from /dev/hotplug)
that lets us do this...
Inspiration for this diff was drawn from Joshua Stein [1], seeminly he
had a use-case that was somewhat similar to mine at one point but it
doesn't look like this was ever committed. Nor can I find any discussion
on it.
[1]: https://jcs.org/2018/08/31/surface_go
Feedback greatly welcomed.
Thanks,
Ash
diff --git a/sys/dev/hotplug.c b/sys/dev/hotplug.c
index e1e7bad95c9..522939f7f7a 100644
--- a/sys/dev/hotplug.c
+++ b/sys/dev/hotplug.c
@@ -25,15 +25,24 @@
#include
#include
#include
+#include
#include
#include
#define HOTPLUG_MAXEVENTS 64
-static int opened;
+struct hotplug_dev {
+ int unit;
+ int evqueue_head;
+ struct selinfo sel_info;
+
+ LIST_ENTRY(hotplug_dev) dev_list;
+};
+
+LIST_HEAD(, hotplug_dev) hotplug_dev_list;
+static int evqueue_head;
static struct hotplug_event evqueue[HOTPLUG_MAXEVENTS];
-static int evqueue_head, evqueue_tail, evqueue_count;
-static struct selinfo hotplug_sel;
+
void filt_hotplugrdetach(struct knote *);
int filt_hotplugread(struct knote *, long);
@@ -45,21 +54,40 @@ const struct filterops hotplugread_filtops = {
.f_event = filt_hotplugread,
};
+#define EVQUEUE_PREV(p) (p == 0 ? HOTPLUG_MAXEVENTS : p - 1)
#define EVQUEUE_NEXT(p) (p == HOTPLUG_MAXEVENTS - 1 ? 0 : p + 1)
+#define EMPTY_EVENT(ev) (ev.he_type == HOTPLUG_EMPTY)
+#define EVENT_IS_HEAD(p) (p == evqueue_head)
+#define HEAD_EVENT evqueue[evqueue_head]
int hotplug_put_event(struct hotplug_event *);
-int hotplug_get_event(struct hotplug_event *);
+int hotplug_get_event(struct hotplug_dev *, struct hotplug_event *);
void hotplugattach(int);
+struct hotplug_dev * hotplug_device_lookup(int);
void
hotplugattach(int count)
{
- opened = 0;
- evqueue_head = 0;
- evqueue_tail = 0;
- evqueue_count = 0;
+ int i;
+
+ for (i = 0; i < HOTPLUG_MAXEVENTS; ++i)
+ evqueue[i].he_type = HOTPLUG_EMPTY;
+
+ LIST_INIT(&hotplug_dev_list);
+}
+
+struct hotplug_dev *
+hotplug_device_lookup(int unit)
+{
+ struct hotplug_dev *d;
+
+ LIST_FOREACH(d, &hotplug_dev_list, dev_list)
+ if (d->unit == unit)
+ return d;
+
+ return (NULL);
}
void
@@ -87,59 +115,79 @@ hotplug_device_detach(enum devclass class, char *name)
int
hotplug_put_event(struct hotplug_event *he)
{
- if (evqueue_count == HOTPLUG_MAXEVENTS && opened) {
- printf("hotplug: event lost, queue full\n");
- return (1);
- }
+ struct hotplug_dev *d;
- evqueue[evqueue_head] = *he;
+ HEAD_EVENT = *he;
evqueue_head = EVQUEUE_NEXT(evqueue_head);
- if (evqueue_count == HOTPLUG_MAXEVENTS)
- evqueue_tail = EVQUEUE_NEXT(evqueue_tail);
- else
- evqueue_count++;
+ HEAD_EVENT.he_type = HOTPLUG_EMPTY;
+
+ LIST_FOREACH(d, &hotplug_dev_list, dev_list) {
+ if (EVENT_IS_HEAD(d->evqueue_head))
+ d->evqueue_head = EVQUEUE_NEXT(evqueue_head);
+ selwakeup(&d->sel_info);
+ }
+
+
wakeup(&evqueue);
- selwakeup(&hotplug_sel);
return (0);
}
int
-hotplug_get_event(struct hotplug_event *he)
+hotplug_get_event(struct hotplug_dev *d, struct hotplug_event *he)
{
- int s;
+ int s = splbio();
- if (evqueue_count == 0)
- return (1);
+ if (!EMPTY_EVENT(evqueue[d->evqueue_head])) {
+ *he = evqueue[d->evqueue_head];
+ d->evqueue_head = EVQUEUE_NEXT(d->evqueue_head);
+ splx(s);
+ return (0);
+ }
- s = splbio();
- *he = evqueue[evqueue_tail];
- evqueue_tail
nvme: add timeout to nvme_poll() loop
I noticed this when looking through the nvme.c code the other day.
Currently, nvme_poll() has a loop like this:
while (!ISSET(state.c.flags, htole16(NVME_CQE_PHASE))) {
if (nvme_q_complete(sc, q) == 0)
delay(10);
/* XXX no timeout? */
}
The comment is indicative - there probably should be a timeout here in
case things are terribly wrong and the queue never gets fully processed.
NetBSD appears to have a similar construct in their version of the
driver:
https://github.com/NetBSD/src/blob/trunk/sys/dev/ic/nvme.c
The following diff adds a counter to fail the poll after 10 seconds of
spinning. I've been torturing my test machine (which has an nvme boot
drive) with this applied - no issues so far.
AFAICT, returning anything non-zero should be ok as it looks like
nothing specifically checks the return code for anything other being
zero. But, I may be wrong and perhaps one of the various NVME_CQE_*
masks is more suitable.
Thanks.
diff --git a/sys/dev/ic/nvme.c b/sys/dev/ic/nvme.c
index 9a79c8b1bfe..4e112f482ee 100644
--- a/sys/dev/ic/nvme.c
+++ b/sys/dev/ic/nvme.c
@@ -117,6 +117,11 @@ void nvme_scsi_inquiry(struct scsi_xfer *);
void nvme_scsi_capacity16(struct scsi_xfer *);
void nvme_scsi_capacity(struct scsi_xfer *);
+/* Here, we define a delay of 10 microseconds between poll attempts.
+ * And, we allow 1e6 attempts. This corresponds to a ~10 second wait. */
+#define NVME_POLL_DELAY_USECS 10
+#define NVME_POLL_MAX_ATTEMPTS 1E6
+
#define nvme_read4(_s, _r) \
bus_space_read_4((_s)->sc_iot, (_s)->sc_ioh, (_r))
#define nvme_write4(_s, _r, _v) \
@@ -918,6 +923,7 @@ nvme_poll(struct nvme_softc *sc, struct nvme_queue *q, struct nvme_ccb *ccb,
void (*done)(struct nvme_softc *, struct nvme_ccb *, struct nvme_cqe *);
void *cookie;
u_int16_t flags;
+ int tries = 0;
memset(&state, 0, sizeof(state));
(*fill)(sc, ccb, &state.s);
@@ -931,9 +937,13 @@ nvme_poll(struct nvme_softc *sc, struct nvme_queue *q, struct nvme_ccb *ccb,
nvme_q_submit(sc, q, ccb, nvme_poll_fill);
while (!ISSET(state.c.flags, htole16(NVME_CQE_PHASE))) {
if (nvme_q_complete(sc, q) == 0)
- delay(10);
+ delay(NVME_POLL_DELAY_USECS);
- /* XXX no timeout? */
+ if (++tries >= NVME_POLL_MAX_ATTEMPTS) {
+ /* Something appears to be quite wrong... */
+ printf("%s: poll timeout.\n", DEVNAME(sc));
+ return (1);
+ }
}
ccb->ccb_cookie = cookie;
Cleanup of err(1, "unveil") pattern: usr.bin
Ashton Fagg writes:
> Ok splendid. I've regenerated these, this time including dhcpleased and
> slaacd since Florian requested I do this in private mail.
>
> I went for err(1, "unveil %s", path) per Theo's suggestion - nice and
> clear. This is now everything in sbin, bin and games. usr/{bin, sbin}
> looks like a bigger job but I'll get to it this week probably.
Patches attached for all utilities in usr.bin. There's a couple of these
that also fix some whitespace problems (trailing tabs for example).
All that leaves now is usr.sbin, which I will get to tomorrow most likely.
diff --git a/usr.bin/audioctl/audioctl.c b/usr.bin/audioctl/audioctl.c
index ec2c1927695..b547c618b16 100644
--- a/usr.bin/audioctl/audioctl.c
+++ b/usr.bin/audioctl/audioctl.c
@@ -285,7 +285,7 @@ main(int argc, char **argv)
argv += optind;
if (unveil(path, "w") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", path);
if (unveil(NULL, NULL) == -1)
err(1, "unveil");
@@ -296,5 +296,5 @@ main(int argc, char **argv)
audio_main(argc, argv);
close(fd);
- return 0;
+ return 0;
}
diff --git a/usr.bin/biff/biff.c b/usr.bin/biff/biff.c
index f8c102f5dfc..565b8ed9177 100644
--- a/usr.bin/biff/biff.c
+++ b/usr.bin/biff/biff.c
@@ -62,7 +62,7 @@ main(int argc, char *argv[])
err(2, "tty");
if (unveil(name, "rw") == -1)
- err(2, "unveil");
+ err(2, "unveil %s", name);
if (pledge("stdio rpath fattr", NULL) == -1)
err(2, "pledge");
diff --git a/usr.bin/chpass/chpass.c b/usr.bin/chpass/chpass.c
index f20b7f18b9b..d92a1d4c1e2 100644
--- a/usr.bin/chpass/chpass.c
+++ b/usr.bin/chpass/chpass.c
@@ -137,11 +137,11 @@ main(int argc, char *argv[])
display(tempname, dfd, pw);
if (unveil(_PATH_BSHELL, "x") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_BSHELL);
if (unveil(_PATH_SHELLS, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_SHELLS);
if (unveil(tempname, "rc") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", tempname);
if (pledge("stdio rpath wpath cpath id proc exec unveil",
NULL) == -1)
err(1, "pledge");
@@ -165,7 +165,7 @@ main(int argc, char *argv[])
if (op == NEWSH) {
if (unveil(_PATH_SHELLS, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_SHELLS);
if (pledge("stdio rpath wpath cpath id proc exec unveil",
NULL) == -1)
err(1, "pledge");
@@ -184,11 +184,11 @@ main(int argc, char *argv[])
sigprocmask(SIG_BLOCK, &fullset, NULL);
if (unveil(_PATH_MASTERPASSWD_LOCK, "rwc") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_MASTERPASSWD_LOCK);
if (unveil(_PATH_MASTERPASSWD, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_MASTERPASSWD);
if (unveil(_PATH_PWD_MKDB, "x") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_PWD_MKDB);
if (pledge("stdio rpath wpath cpath proc exec", NULL) == -1)
err(1, "pledge");
diff --git a/usr.bin/ctfconv/ctfconv.c b/usr.bin/ctfconv/ctfconv.c
index f47af91541d..b86e89d33e6 100644
--- a/usr.bin/ctfconv/ctfconv.c
+++ b/usr.bin/ctfconv/ctfconv.c
@@ -128,11 +128,11 @@ main(int argc, char *argv[])
filename = *argv;
if (unveil(filename, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", filename);
if (outfile != NULL) {
if (unveil(outfile, "wc") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", outfile);
}
if (pledge("stdio rpath wpath cpath", NULL) == -1)
diff --git a/usr.bin/doas/doas.c b/usr.bin/doas/doas.c
index be05be3a968..5940402e27e 100644
--- a/usr.bin/doas/doas.c
+++ b/usr.bin/doas/doas.c
@@ -416,9 +416,10 @@ main(int argc, char **argv)
if (formerpath == NULL)
formerpath = "";
- if (unveil(_PATH_LOGIN_CONF, "r") == -1 ||
- unveil(_PATH_LOGIN_CONF ".db", "r") == -1)
- err(1, "unveil");
+ if (unveil(_PATH_LOGIN_CONF, "r") == -1)
+ err(1, "unveil %s", _PATH_LOGIN_CONF);
+ if (unveil(_PATH_LOGIN_CONF ".db", "r") == -1)
+ err(1, "unveil %s.db", _PATH_LOGIN_CONF);
if (rule->cmd) {
if (setenv("PATH", safepath, 1) == -1)
err(1, "failed to set PATH '%s'", safepath);
diff --git a/usr.bin/encrypt/encrypt.c b/usr.bin/encrypt/encrypt.c
index 01e96edd9f8..bbc41011783 100644
--- a/usr.bin/encrypt/encrypt.c
+++ b/usr.bin/encrypt/encrypt.c
@@ -95,9 +95,10 @@ main(int argc, char **argv)
char *extra = NULL; /* Store login class or number of rounds */
const char *errstr;
- if (unveil(_PATH_LOGIN_
Re: Cleanup of err(1, "unveil") pattern: bin, games, sbin
"Theo de Raadt" writes:
> No, it is either:
>
> err(1, "unveil %s", path)
>
> or
>
> err(1, "unveil: %s", path)
>
> I remain undecided between those two, i don't particularily like two :: in
> a error message.
Ok splendid. I've regenerated these, this time including dhcpleased and
slaacd since Florian requested I do this in private mail.
I went for err(1, "unveil %s", path) per Theo's suggestion - nice and
clear. This is now everything in sbin, bin and games. usr/{bin, sbin}
looks like a bigger job but I'll get to it this week probably.
diff --git a/sbin/dhclient/dhclient.c b/sbin/dhclient/dhclient.c
index 993c829f2d2..4273a26fbc9 100644
--- a/sbin/dhclient/dhclient.c
+++ b/sbin/dhclient/dhclient.c
@@ -2334,9 +2334,9 @@ fork_privchld(struct interface_info *ifi, int fd, int fd2)
fatal("socket(AF_ROUTE, SOCK_RAW)");
if (unveil(_PATH_RESCONF, "wc") == -1)
- fatal("unveil");
+ fatal("unveil %s", _PATH_RESCONF);
if (unveil("/etc/resolv.conf.tail", "r") == -1)
- fatal("unveil");
+ fatal("unveil /etc/resolve.conf.tail");
if (unveil(NULL, NULL) == -1)
fatal("unveil");
diff --git a/sbin/fsck_ffs/setup.c b/sbin/fsck_ffs/setup.c
index e9c922d7c37..e3706d7b759 100644
--- a/sbin/fsck_ffs/setup.c
+++ b/sbin/fsck_ffs/setup.c
@@ -105,7 +105,7 @@ setup(char *dev, int isfsdb)
if (isfsdb || !hotroot()) {
if (unveil("/dev", "rw") == -1)
-err(1, "unveil");
+err(1, "unveil /dev");
if (pledge("stdio rpath wpath getpw tty disklabel",
NULL) == -1)
err(1, "pledge");
diff --git a/sbin/fsck_msdos/check.c b/sbin/fsck_msdos/check.c
index 4a2f07f1131..b011cd7dca6 100644
--- a/sbin/fsck_msdos/check.c
+++ b/sbin/fsck_msdos/check.c
@@ -55,7 +55,7 @@ checkfilesys(const char *fname)
int mod = 0;
if (unveil("/dev", "rw") == -1)
- err(1, "unveil");
+ err(1, "unveil /dev");
rdonly = alwaysno;
diff --git a/sbin/fsck/fsck.c b/sbin/fsck/fsck.c
index 09475f346d3..0c8efa626a2 100644
--- a/sbin/fsck/fsck.c
+++ b/sbin/fsck/fsck.c
@@ -110,11 +110,11 @@ main(int argc, char *argv[])
checkroot();
if (unveil("/dev", "rw") == -1)
- err(1, "unveil");
+ err(1, "unveil /dev");
if (unveil(_PATH_FSTAB, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_FSTAB);
if (unveil("/sbin", "x") == -1)
- err(1, "unveil");
+ err(1, "unveil /sbin");
if (pledge("stdio rpath wpath disklabel proc exec", NULL) == -1)
err(1, "pledge");
diff --git a/sbin/ifconfig/ifconfig.c b/sbin/ifconfig/ifconfig.c
index c527dadadaf..1681702f9bc 100644
--- a/sbin/ifconfig/ifconfig.c
+++ b/sbin/ifconfig/ifconfig.c
@@ -773,7 +773,7 @@ main(int argc, char *argv[])
if (argc < 2) {
/* no filesystem visibility */
if (unveil("/", "") == -1)
- err(1, "unveil");
+ err(1, "unveil /");
if (unveil(NULL, NULL) == -1)
err(1, "unveil");
aflag = 1;
@@ -827,11 +827,11 @@ main(int argc, char *argv[])
if (!found_rulefile) {
if (unveil(_PATH_RESCONF, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_RESCONF);
if (unveil(_PATH_HOSTS, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_HOSTS);
if (unveil(_PATH_SERVICES, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_SERVICES);
if (unveil(NULL, NULL) == -1)
err(1, "unveil");
}
diff --git a/sbin/nologin/nologin.c b/sbin/nologin/nologin.c
index 88bdd5f6fd7..c60257da517 100644
--- a/sbin/nologin/nologin.c
+++ b/sbin/nologin/nologin.c
@@ -47,7 +47,7 @@ main(int argc, char *argv[])
char nbuf[BUFSIZ];
if (unveil(_PATH_NOLOGIN_TXT, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_NOLOGIN_TXT);
if (pledge("stdio rpath", NULL) == -1)
err(1, "pledge");
diff --git a/sbin/pflogd/privsep.c b/sbin/pflogd/privsep.c
index a1c109005cf..805b460ffce 100644
--- a/sbin/pflogd/privsep.c
+++ b/sbin/pflogd/privsep.c
@@ -134,15 +134,15 @@ priv_init(int Pflag, int argc, char *argv[])
setproctitle("[priv]");
if (unveil(_PATH_RESCONF, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_RESCONF);
if (unveil(_PATH_HOSTS, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_HOSTS);
if (unveil(_PATH_SERVICES, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_SERVICES);
if (unveil("/dev/bpf", "r") == -1)
- err(1, "unveil");
+ err(1, "unveil /dev/bpf");
if (unveil(filename, "rwc") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", filename);
if (unveil(NULL, NULL) == -1)
err(1, "unveil");
diff --git a/sbin/ping/ping.c b/sbin/ping/ping.c
index f7c3c101b25..0693f804f81 100644
--- a/sbin/ping/ping.c
+++ b/sbin/ping/ping.c
@@ -266,7 +266,7 @@ main(int argc, char *argv[])
/* Cannot pledge due to special setsockopt()s below */
if (unveil("/", "r") == -1)
- err(1, "unveil");
+ err(1, "unveil /");
if (unveil(NULL, NULL) == -1)
err(1, "unveil");
diff --git a/bin/ps/ps.c b/bin/ps/ps.c
index 84be7afe802..7df4b0e17cd 100644
--- a/bin/ps/ps.c
+++ b/bin/ps/ps.c
@@ -276,18 +276,18 @@ main(int argc, char *argv[])
errx(1, "%s", errbu
Re: Cleanup of err(1, "unveil") pattern: bin, games, sbin
On Mon, 3 May 2021 at 10:17, Theo de Raadt wrote: > So if the messages were just 'unveil %s: error' or 'unveil: %s: error' > I would be thrilled, as this allows users to realize why the program is > not working right. Florian/Theo, Thanks for the reviews. So it sounds like err(1, "unveil: %s error", some_path); is the way to go - with the exception of unveil(NULL, NULL) as discussed since that's not ambiguous as there's no path involved. Seems like slaacd and dhcpleased should also be included in this as well for consistency's sake - I will update these diffs and continue with /usr/bin and /usr/sbin in the coming days. Cheers, Ash
Cleanup of err(1, "unveil") pattern: xenocara
Ashton Fagg writes:
> Hi all,
>
> I saw a discussion on here a while ago about the use of patterns like:
>
> if (unveil(some_path, "r") == -1)
>err(1, "unveil");
>
> And why that's maybe not preferable for debugging and troubleshooting
> purposes for programs which have multiple unveil calls (which happens
> fairly often).
>
> Original message here: https://marc.info/?l=openbsd-tech&m=161470144611031&w=2
Patch attached which cleans up the single occurrence of this in xenocara.
Thanks.
diff --git a/xserver/os/privsep.c b/xserver/os/privsep.c
index bbe9222c8..baba33e03 100644
--- a/xserver/os/privsep.c
+++ b/xserver/os/privsep.c
@@ -287,7 +287,7 @@ priv_init(uid_t uid, gid_t gid)
for (dev = allowed_devices; dev->name != NULL; dev++) {
if (unveil(dev->name, "rw") == -1 && errno != ENOENT)
- err(1, "unveil");
+ err(1, "unveil %s", dev->name);
}
if (pledge("stdio rpath wpath sendfd proc", NULL) == -1)
err(1, "pledge");
Re: Cleanup of err(1, "unveil") pattern: bin, games, sbin
"Theo de Raadt" writes:
> Showing the symbolic name is not doing anywhere else in the tree.
>
> Most likely they should be
>
> err(1, "unveil: %s", path);
Per Theo's advice, updated diffs are attached.
diff --git a/sbin/dhclient/dhclient.c b/sbin/dhclient/dhclient.c
index 993c829f2d2..ba88d9f5f67 100644
--- a/sbin/dhclient/dhclient.c
+++ b/sbin/dhclient/dhclient.c
@@ -2334,11 +2334,11 @@ fork_privchld(struct interface_info *ifi, int fd, int fd2)
fatal("socket(AF_ROUTE, SOCK_RAW)");
if (unveil(_PATH_RESCONF, "wc") == -1)
- fatal("unveil");
+ fatal("unveil %s", _PATH_RESCONF);
if (unveil("/etc/resolv.conf.tail", "r") == -1)
- fatal("unveil");
+ fatal("unveil /etc/resolve.conf.tail");
if (unveil(NULL, NULL) == -1)
- fatal("unveil");
+ fatal("unveil(NULL,NULL)");
while (quit == 0) {
pfd[0].fd = priv_ibuf->fd;
diff --git a/sbin/fsck_ffs/setup.c b/sbin/fsck_ffs/setup.c
index e9c922d7c37..e3706d7b759 100644
--- a/sbin/fsck_ffs/setup.c
+++ b/sbin/fsck_ffs/setup.c
@@ -105,7 +105,7 @@ setup(char *dev, int isfsdb)
if (isfsdb || !hotroot()) {
if (unveil("/dev", "rw") == -1)
-err(1, "unveil");
+err(1, "unveil /dev");
if (pledge("stdio rpath wpath getpw tty disklabel",
NULL) == -1)
err(1, "pledge");
diff --git a/sbin/fsck_msdos/check.c b/sbin/fsck_msdos/check.c
index 4a2f07f1131..b011cd7dca6 100644
--- a/sbin/fsck_msdos/check.c
+++ b/sbin/fsck_msdos/check.c
@@ -55,7 +55,7 @@ checkfilesys(const char *fname)
int mod = 0;
if (unveil("/dev", "rw") == -1)
- err(1, "unveil");
+ err(1, "unveil /dev");
rdonly = alwaysno;
diff --git a/sbin/fsck/fsck.c b/sbin/fsck/fsck.c
index 09475f346d3..ce3826885c4 100644
--- a/sbin/fsck/fsck.c
+++ b/sbin/fsck/fsck.c
@@ -110,11 +110,11 @@ main(int argc, char *argv[])
checkroot();
if (unveil("/dev", "rw") == -1)
- err(1, "unveil");
+ err(1, "unveil /dev");
if (unveil(_PATH_FSTAB, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_FSTAB);
if (unveil("/sbin", "x") == -1)
- err(1, "unveil");
+ err(1, "unveil /sbin");
if (pledge("stdio rpath wpath disklabel proc exec", NULL) == -1)
err(1, "pledge");
diff --git a/sbin/nologin/nologin.c b/sbin/nologin/nologin.c
index 88bdd5f6fd7..7eb39266c56 100644
--- a/sbin/nologin/nologin.c
+++ b/sbin/nologin/nologin.c
@@ -47,7 +47,7 @@ main(int argc, char *argv[])
char nbuf[BUFSIZ];
if (unveil(_PATH_NOLOGIN_TXT, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_NOLOGIN_TXT);
if (pledge("stdio rpath", NULL) == -1)
err(1, "pledge");
diff --git a/sbin/pflogd/privsep.c b/sbin/pflogd/privsep.c
index a1c109005cf..fa0ec37ae6a 100644
--- a/sbin/pflogd/privsep.c
+++ b/sbin/pflogd/privsep.c
@@ -134,17 +134,17 @@ priv_init(int Pflag, int argc, char *argv[])
setproctitle("[priv]");
if (unveil(_PATH_RESCONF, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_RESCONF);
if (unveil(_PATH_HOSTS, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_HOSTS);
if (unveil(_PATH_SERVICES, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_SERVICES);
if (unveil("/dev/bpf", "r") == -1)
- err(1, "unveil");
+ err(1, "unveil /dev/bpf");
if (unveil(filename, "rwc") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", filename);
if (unveil(NULL, NULL) == -1)
- err(1, "unveil");
+ err(1, "unveil(NULL,NULL)");
#if 0
/* This needs to do bpf ioctl */
diff --git a/sbin/ping/ping.c b/sbin/ping/ping.c
index f7c3c101b25..63585019da2 100644
--- a/sbin/ping/ping.c
+++ b/sbin/ping/ping.c
@@ -266,9 +266,9 @@ main(int argc, char *argv[])
/* Cannot pledge due to special setsockopt()s below */
if (unveil("/", "r") == -1)
- err(1, "unveil");
+ err(1, "unveil /");
if (unveil(NULL, NULL) == -1)
- err(1, "unveil");
+ err(1, "unveil(NULL,NULL)");
if (strcmp("ping6", __progname) == 0) {
v6flag = 1;
diff --git a/bin/ps/ps.c b/bin/ps/ps.c
index 84be7afe802..3b6593a46a1 100644
--- a/bin/ps/ps.c
+++ b/bin/ps/ps.c
@@ -276,18 +276,18 @@ main(int argc, char *argv[])
errx(1, "%s", errbuf);
if (unveil(_PATH_DEVDB, "r") == -1 && errno != ENOENT)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_DEVDB);
if (unveil(_PATH_DEV, "r") == -1 && errno != ENOENT)
- err(1, "unveil");
+ err(1, "unveil %s", _PATH_DEV);
if (swapf)
if (unveil(swapf, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", swapf);
if (nlistf)
if (unveil(nlistf, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", nlistf);
if (memf)
if (unveil(memf, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil %s", memf);
if (pledge("stdio rpath getpw ps", NULL) == -1)
err(1, "pledge");
diff --git a/sbin/shutdown/shutdown.c b/sbin/shutdown/shutdown.c
index d28eb676172..94c323c25dc 100644
--- a/sbin/shutdown/shutdown.c
+++ b/sbin/shutdown/shutdown.c
@@ -166,24 +166,24 @@ main(int argc, char *argv[])
}
if (unveil(_PATH_CONSOLE, "rw") == -1)
- err(1, "unveil");
+ err(1, "unveil %s" _PATH_CONSOLE);
if (unveil(
Re: Cleanup of err(1, "unveil") pattern: bin, games, sbin
Thanks for the review - wasn't sure if that was useful. Will
regenerate without the symbolic names.
On Sun, 2 May 2021 at 20:37, Theo de Raadt wrote:
>
> Showing the symbolic name is not doing anywhere else in the tree.
>
> Most likely they should be
>
>err(1, "unveil: %s", path);
>
>
> Ashton Fagg wrote:
>
> > Ashton Fagg writes:
> >
> > > Hi all,
> > >
> > > I saw a discussion on here a while ago about the use of patterns like:
> > >
> > > if (unveil(some_path, "r") == -1)
> > >err(1, "unveil");
> > >
> > > And why that's maybe not preferable for debugging and troubleshooting
> > > purposes for programs which have multiple unveil calls (which happens
> > > fairly often).
> > >
> > > Original message here:
> > > https://marc.info/?l=openbsd-tech&m=161470144611031&w=2
> >
> > I decided just to go ahead and do this since I noticed there's some
> > newer stuff not following this pattern (dhcpleased for example). Here are
> > patches for:
> >
> > games/tetris
> > bin/ps
> > sbin/dhclient
> > sbin/fsck
> > sbin/fsck_msdos
> > sbin/fsck_ffs
> > sbin/nologin
> > sbin/pflogd
> > sbin/ping
> > sbin/shutdown
> > sbin/sysctl
> > sbin/unwind
> >
> > I'll tackle /usr/bin and /usr/sbin another time.
> >
> > diff --git a/bin/ps/ps.c b/bin/ps/ps.c
> > index 84be7afe802..3b6593a46a1 100644
> > --- a/bin/ps/ps.c
> > +++ b/bin/ps/ps.c
> > @@ -276,18 +276,18 @@ main(int argc, char *argv[])
> > errx(1, "%s", errbuf);
> >
> > if (unveil(_PATH_DEVDB, "r") == -1 && errno != ENOENT)
> > - err(1, "unveil");
> > + err(1, "unveil: _PATH_DEVDB -> %s", _PATH_DEVDB);
> > if (unveil(_PATH_DEV, "r") == -1 && errno != ENOENT)
> > - err(1, "unveil");
> > + err(1, "unveil: _PATH_DEV -> %s", _PATH_DEV);
> > if (swapf)
> > if (unveil(swapf, "r") == -1)
> > - err(1, "unveil");
> > + err(1, "unveil: swapf -> %s", swapf);
> > if (nlistf)
> > if (unveil(nlistf, "r") == -1)
> > - err(1, "unveil");
> > + err(1, "unveil: nlistf -> %s", nlistf);
> > if (memf)
> > if (unveil(memf, "r") == -1)
> > - err(1, "unveil");
> > + err(1, "unveil: memf -> %s", memf);
> > if (pledge("stdio rpath getpw ps", NULL) == -1)
> > err(1, "pledge");
> >
> > diff --git a/games/tetris/tetris.c b/games/tetris/tetris.c
> > index 69f4532a4ac..fdb7e7d2d40 100644
> > --- a/games/tetris/tetris.c
> > +++ b/games/tetris/tetris.c
> > @@ -234,7 +234,7 @@ main(int argc, char *argv[])
> > scr_init();
> >
> > if (unveil(scorepath, "rwc") == -1)
> > - err(1, "unveil");
> > + err(1, "unveil: scorepath -> %s", scorepath);
> >
> > if (pledge("stdio rpath wpath cpath tty", NULL) == -1)
> > err(1, "pledge");
> > diff --git a/sbin/dhclient/dhclient.c b/sbin/dhclient/dhclient.c
> > index 993c829f2d2..ba88d9f5f67 100644
> > --- a/sbin/dhclient/dhclient.c
> > +++ b/sbin/dhclient/dhclient.c
> > @@ -2334,11 +2334,11 @@ fork_privchld(struct interface_info *ifi, int fd,
> > int fd2)
> > fatal("socket(AF_ROUTE, SOCK_RAW)");
> >
> > if (unveil(_PATH_RESCONF, "wc") == -1)
> > - fatal("unveil");
> > + fatal("unveil: _PATH_RESCONF -> %s", _PATH_RESCONF);
> > if (unveil("/etc/resolv.conf.tail", "r") == -1)
> > - fatal("unveil");
> > + fatal("unveil: /etc/resolve.conf.tail");
> > if (unveil(NULL, NULL) == -1)
> > - fatal("unveil");
> > + fatal("unveil(NULL,NULL)");
> >
> > while (quit == 0) {
> > pfd[0].fd = priv_ibuf->fd;
> > diff --git a/sbin/fsck/fsck.c b/sbin/fsck/fsck.c
> > index 09475f346d3..ce3826885c4 100644
> > --- a/sbin/fsck/fsck.c
> > +++ b/sbin/
Cleanup of err(1, "unveil") pattern: bin, games, sbin
Ashton Fagg writes:
> Hi all,
>
> I saw a discussion on here a while ago about the use of patterns like:
>
> if (unveil(some_path, "r") == -1)
>err(1, "unveil");
>
> And why that's maybe not preferable for debugging and troubleshooting
> purposes for programs which have multiple unveil calls (which happens
> fairly often).
>
> Original message here: https://marc.info/?l=openbsd-tech&m=161470144611031&w=2
I decided just to go ahead and do this since I noticed there's some
newer stuff not following this pattern (dhcpleased for example). Here are
patches for:
games/tetris
bin/ps
sbin/dhclient
sbin/fsck
sbin/fsck_msdos
sbin/fsck_ffs
sbin/nologin
sbin/pflogd
sbin/ping
sbin/shutdown
sbin/sysctl
sbin/unwind
I'll tackle /usr/bin and /usr/sbin another time.
diff --git a/bin/ps/ps.c b/bin/ps/ps.c
index 84be7afe802..3b6593a46a1 100644
--- a/bin/ps/ps.c
+++ b/bin/ps/ps.c
@@ -276,18 +276,18 @@ main(int argc, char *argv[])
errx(1, "%s", errbuf);
if (unveil(_PATH_DEVDB, "r") == -1 && errno != ENOENT)
- err(1, "unveil");
+ err(1, "unveil: _PATH_DEVDB -> %s", _PATH_DEVDB);
if (unveil(_PATH_DEV, "r") == -1 && errno != ENOENT)
- err(1, "unveil");
+ err(1, "unveil: _PATH_DEV -> %s", _PATH_DEV);
if (swapf)
if (unveil(swapf, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil: swapf -> %s", swapf);
if (nlistf)
if (unveil(nlistf, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil: nlistf -> %s", nlistf);
if (memf)
if (unveil(memf, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil: memf -> %s", memf);
if (pledge("stdio rpath getpw ps", NULL) == -1)
err(1, "pledge");
diff --git a/games/tetris/tetris.c b/games/tetris/tetris.c
index 69f4532a4ac..fdb7e7d2d40 100644
--- a/games/tetris/tetris.c
+++ b/games/tetris/tetris.c
@@ -234,7 +234,7 @@ main(int argc, char *argv[])
scr_init();
if (unveil(scorepath, "rwc") == -1)
- err(1, "unveil");
+ err(1, "unveil: scorepath -> %s", scorepath);
if (pledge("stdio rpath wpath cpath tty", NULL) == -1)
err(1, "pledge");
diff --git a/sbin/dhclient/dhclient.c b/sbin/dhclient/dhclient.c
index 993c829f2d2..ba88d9f5f67 100644
--- a/sbin/dhclient/dhclient.c
+++ b/sbin/dhclient/dhclient.c
@@ -2334,11 +2334,11 @@ fork_privchld(struct interface_info *ifi, int fd, int fd2)
fatal("socket(AF_ROUTE, SOCK_RAW)");
if (unveil(_PATH_RESCONF, "wc") == -1)
- fatal("unveil");
+ fatal("unveil: _PATH_RESCONF -> %s", _PATH_RESCONF);
if (unveil("/etc/resolv.conf.tail", "r") == -1)
- fatal("unveil");
+ fatal("unveil: /etc/resolve.conf.tail");
if (unveil(NULL, NULL) == -1)
- fatal("unveil");
+ fatal("unveil(NULL,NULL)");
while (quit == 0) {
pfd[0].fd = priv_ibuf->fd;
diff --git a/sbin/fsck/fsck.c b/sbin/fsck/fsck.c
index 09475f346d3..ce3826885c4 100644
--- a/sbin/fsck/fsck.c
+++ b/sbin/fsck/fsck.c
@@ -110,11 +110,11 @@ main(int argc, char *argv[])
checkroot();
if (unveil("/dev", "rw") == -1)
- err(1, "unveil");
+ err(1, "unveil: /dev");
if (unveil(_PATH_FSTAB, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil: _PATH_FSTAB -> %s", _PATH_FSTAB);
if (unveil("/sbin", "x") == -1)
- err(1, "unveil");
+ err(1, "unveil /sbin");
if (pledge("stdio rpath wpath disklabel proc exec", NULL) == -1)
err(1, "pledge");
diff --git a/sbin/fsck_msdos/check.c b/sbin/fsck_msdos/check.c
index 4a2f07f1131..b011cd7dca6 100644
--- a/sbin/fsck_msdos/check.c
+++ b/sbin/fsck_msdos/check.c
@@ -55,7 +55,7 @@ checkfilesys(const char *fname)
int mod = 0;
if (unveil("/dev", "rw") == -1)
- err(1, "unveil");
+ err(1, "unveil /dev");
rdonly = alwaysno;
diff --git a/sbin/fsck_ffs/setup.c b/sbin/fsck_ffs/setup.c
index e9c922d7c37..e3706d7b759 100644
--- a/sbin/fsck_ffs/setup.c
+++ b/sbin/fsck_ffs/setup.c
@@ -105,7 +105,7 @@ setup(char *dev, int isfsdb)
if (isfsdb || !hotroot()) {
if (unveil("/dev", "rw") == -1)
-err(1, "unveil");
+err(1, "unveil /dev");
if (pledge("stdio rpath wpath getpw tty disklabel",
NULL) == -1)
err(1, "pledge");
diff --git a/sbin/nologin/nologin.c b/sbin/nologin/nologin.c
index 88bdd5f6fd7..7eb39266c56 100644
--- a/sbin/nologin/nologin.c
+++ b/sbin/nologin/nologin.c
@@ -47,7 +47,7 @@ main(int argc, char *argv[])
char nbuf[BUFSIZ];
if (unveil(_PATH_NOLOGIN_TXT, "r") == -1)
- err(1, &quo
Fix compiler warning from sysctl.c
Fixes the following warning:
sysctl.c:835:18: warning: format specifies type 'char *' but the argument has
type 'void *' [-Wformat]
string, newval);
^~
sysctl.c:840:18: warning: format specifies type 'char *' but the argument has
type 'void *' [-Wformat]
string, newval);
Appears as though this was fixed in lines above these two instances, but
these were still not properly cast.
diff --git a/sbin/sysctl/sysctl.c b/sbin/sysctl/sysctl.c
index 5e9e562d308..a03220d7d56 100644
--- a/sbin/sysctl/sysctl.c
+++ b/sbin/sysctl/sysctl.c
@@ -832,12 +832,12 @@ parse(char *string, int flags)
newval);
if (len == -1) {
warnx("%s: hex string %s: invalid",
- string, newval);
+ string, (char *)newval);
return;
}
if (len > sizeof(hex)) {
warnx("%s: hex string %s: too long",
- string, newval);
+ string, (char *)newval);
return;
}
Cleanup of err(1, "unveil") pattern? Diff for ps.c.
Hi all,
I saw a discussion on here a while ago about the use of patterns like:
if (unveil(some_path, "r") == -1)
err(1, "unveil");
And why that's maybe not preferable for debugging and troubleshooting
purposes for programs which have multiple unveil calls (which happens
fairly often).
Original message here: https://marc.info/?l=openbsd-tech&m=161470144611031&w=2
I would like to try and clean this up if I can. I did the first program
that came up from my grep of the tree - bin/ps/ps.c - as an example to
accompany this mail.
Would this work be of interest if I were to undertake it broadly
across the tree?
2. If so, are there any suggestions/strong feelings with the formatting
of the messages. My included diff shows one possible suggestion.
Comments/suggestions welcome.
Thanks,
Ash
diff --git a/bin/ps/ps.c b/bin/ps/ps.c
index 84be7afe802..3b6593a46a1 100644
--- a/bin/ps/ps.c
+++ b/bin/ps/ps.c
@@ -276,18 +276,18 @@ main(int argc, char *argv[])
errx(1, "%s", errbuf);
if (unveil(_PATH_DEVDB, "r") == -1 && errno != ENOENT)
- err(1, "unveil");
+ err(1, "unveil: _PATH_DEVDB -> %s", _PATH_DEVDB);
if (unveil(_PATH_DEV, "r") == -1 && errno != ENOENT)
- err(1, "unveil");
+ err(1, "unveil: _PATH_DEV -> %s", _PATH_DEV);
if (swapf)
if (unveil(swapf, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil: swapf -> %s", swapf);
if (nlistf)
if (unveil(nlistf, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil: nlistf -> %s", nlistf);
if (memf)
if (unveil(memf, "r") == -1)
- err(1, "unveil");
+ err(1, "unveil: memf -> %s", memf);
if (pledge("stdio rpath getpw ps", NULL) == -1)
err(1, "pledge");
iscsictl.8: Update man page to reflect new behavior of reload command
Hi all, Attached is a diff which update iscsictl.8 to reflect the recent change in behavior of iscsictl's reload command. Thanks, Ash diff --git a/usr.sbin/iscsictl/iscsictl.8 b/usr.sbin/iscsictl/iscsictl.8 index 5886a0f8f1b..1d27978eac5 100644 --- a/usr.sbin/iscsictl/iscsictl.8 +++ b/usr.sbin/iscsictl/iscsictl.8 @@ -47,7 +47,9 @@ to communicate with The following commands are available: .Bl -tag -width Ds .It Cm reload -Reload the configuration file. +Reload the configuration file and wait to return until iscsid reports all +connections have completed (successfully or otherwise), or for up to 10 +seconds. .It Cm show Op Cm summary Show a list of all configured sessions. .It Cm show Cm vscsi stats
Re: iscsid/iscsictl: Introduce poll-and-wait delay during reload
Claudio Jeker writes: > Hi Ashton, > > I adjusted your diff a bit (mainly cleanup of spacing and other style > changes). Please have a look. I think this version should be > committed. > > Cheers Hi Claudio, Apologies for the late reply. Thank you for the review and I'm very pleased this has been committed. - ajf
Re: iscsid/iscsictl: Introduce poll-and-wait delay during reload
Hello.
Pinging on this one hoping to get some feedback. I've reattached the
diff below.
Thanks.
Ashton Fagg writes:
> Hello tech,
>
> Recently I encountered a problem with automounting iscsi volumes at boot
> time. This came down to a timing issue, where iscsictl reload was
> returning before the volumes were attached - causing the machine to
> enter single user mode when it would try to fsck the iscsi volumes.
>
> See this thread:
>
> https://marc.info/?l=openbsd-bugs&m=161404736018432&w=2
>
> Adding "&& sleep 5" to /etc/rc.d/iscsid did mitigate this, however
> that's both hacky and precarious.
>
> After some discussion with claudio@, the principled solution is to make
> iscsictl wait until the sessions are up and the devices are attached
> before returning. Since "iscsictl reload" is called in the rc script,
> this would halt the boot process long enough for all the volumes to be
> available before progressing - and thus, eliminating my problem.
>
> The following diff is an attempt at that, with some caveats. It does the
> following things:
>
> - adds a control command to iscsid to allow for polling of session and
> connection status during iscsictl reload.
>
> - book-keeping code for working out if there are sessions still
> initializing
>
> - some poll-wait mechanisms in iscsictl, which (a) waits for iscsid to
> tell it everything is up and running or (b) returns after 10 poll
> attempts 1 second apart (thus, the max delay here is currently 10
> seconds).
>
> I have confirmed that the current implementation does indeed solve my
> problem, however there's a couple of things I still question:
>
> 1. In the poll_and_wait() function in iscsictl, you'll notice there's an
> extra sleep with the comment that it is there to give time for the
> devices to attach. In my case, the extra second was needed for both my
> devices to attach. without the extra sleep, the first device would
> attach successfully while iscsictl was still waiting, but the second
> still had not fully attached even though my book-keeping code said that
> nothing further was in flight. I question whether there's a better way
> to do this and welcome suggestions about that.
>
> 2. It's probably too chatty. Is it also perhaps better to have a
> separate command that does this, rather than hijacking "reload"?
>
> 3. I don't know whether the book-keeping code is entirely
> reasonable. I've tried to capture every state, but I do wonder if there
> are cases where things slip through the cracks.
>
> For context, here is the machine booting (and hitting single user mode)
> with the standard iscsid:
>
> https://www.youtube.com/watch?v=F09PaT-8IJU&feature=youtu.be
>
> Whereas, here is the same machine configured identically with my iscsid
> changes applied:
>
> https://www.youtube.com/watch?v=TZzmQBVDRxo&feature=youtu.be
>
> The delay appears to be somewhere around 2-3 seconds, which is less than
> the full 10 allowable by the poll_and_wait() loop - so it does appear to
> be detecting the status correctly (at least in this configuration).
>
> Anyhow, I'd love to hear some comments and suggestions. Please note this
> is my first submission outside of ports that's bigger than a 1 or 2
> liner, and it's been a while since I've written C (I write C++ at my day
> job, and it's not systems programming...), so hopefully I haven't done
> anything too silly.
>
> Thanks,
diff --git a/usr.sbin/iscsictl/iscsictl.c b/usr.sbin/iscsictl/iscsictl.c
index 77f9c74abde..46404b5512b 100644
--- a/usr.sbin/iscsictl/iscsictl.c
+++ b/usr.sbin/iscsictl/iscsictl.c
@@ -40,6 +40,10 @@ struct pdu *ctl_getpdu(char *, size_t);
int ctl_sendpdu(int, struct pdu *);
void show_config(struct ctrlmsghdr *, struct pdu *);
void show_vscsi_stats(struct ctrlmsghdr *, struct pdu *);
+void poll_and_wait(void);
+void poll_session_status(void);
+void register_poll(struct ctrlmsghdr *, struct pdu *);
+void poll_print(struct session_poll *);
char cbuf[CONTROL_READ_SIZE];
@@ -48,6 +52,12 @@ struct control {
int fd;
} control;
+
+struct session_poll poll_result; /* Poll result */
+
+#define POLL_DELAY 1 /* Delay between poll attempts (seconds) */
+#define POLL_ATTEMPTS 10/* Number of poll attempts before returning */
+
__dead void
usage(void)
{
@@ -68,7 +78,7 @@ main (int argc, char* argv[])
char *sockname = ISCSID_CONTROL;
struct session_ctlcfg *s;
struct iscsi_config *cf;
- int ch, val = 0;
+ int ch, poll = 0, val = 0;
/* check flags */
while ((ch = getopt(argc, argv, "f:s:")) != -1) {
@@ -135,6 +145,9 @@ main (int
iscsid/iscsictl: Introduce poll-and-wait delay during reload
t session *);
+voidpoll_finalize(struct session_poll *);
+voidpoll_print(struct session_poll *);
+
/* logmsg.c */
void log_hexdump(void *, size_t);
void log_pdu(struct pdu *, int);
diff --git a/usr.sbin/iscsid/poll.c b/usr.sbin/iscsid/poll.c
new file mode 100644
index 000..8eb08107f39
--- /dev/null
+++ b/usr.sbin/iscsid/poll.c
@@ -0,0 +1,100 @@
+/*
+ * Copyright (c) 2021 Dr Ashton Fagg
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+
+#include "iscsid.h"
+#include "log.h"
+
+
+void
+poll_init(struct session_poll *p)
+{
+ p->session_count = 0;
+ p->session_init_count= 0;
+ p->session_running_count = 0;
+ p->conn_logged_in_count = 0;
+ p->conn_failed_count = 0;
+ p->conn_waiting_count= 0;
+ p->sess_conn_status = 0;
+}
+
+void
+poll_session(struct session_poll *p, struct session *s)
+{
+ if (!s)
+ fatal("poll_session failed: invalid session");
+
+ ++p->session_count;
+
+ /* If SESS_RUNNING, this determines the session has either
+ been brought up successfully, or has failed. */
+ if (s->state & SESS_RUNNING) {
+ ++p->session_running_count;
+
+ /* Next, check what state the connections are in. */
+ struct connection *c;
+ int is_logged_in = 0;
+ int is_failed = 0;
+ int is_waiting = 0;
+
+ TAILQ_FOREACH(c, &s->connections, entry) {
+ if (c->state & CONN_LOGGED_IN)
+++is_logged_in;
+ else if (c->state & CONN_FAILED)
+++is_failed;
+ else if (c->state & CONN_NEVER_LOGGED_IN)
+++is_waiting;
+
+ }
+
+ /* Potentially, we have multiple connections for each session.
+ We handle this by saying that a single connection logging in takes
+ precedent over a failed connection. We only say the session login has
+ failed if none of the connections have logged in and nothing is in
+ flight. */
+
+ if (is_logged_in)
+ ++p->conn_logged_in_count;
+ if (is_failed)
+ ++p->conn_failed_count;
+ if (is_waiting)
+ ++p->conn_waiting_count;
+
+ /* Otherwise, it is in SESS_INIT. These need to be waited on. */
+ } else if (s->state & SESS_INIT)
+ ++p->session_init_count;
+ else
+ fatal("poll_session: unknown state.");
+}
+
+void
+poll_finalize(struct session_poll *p)
+{
+ /* total number of sessions that have finished initializing (and ended
+ sucessfully or otherwise). */
+ int num_accounted_conn = p->conn_logged_in_count + p->conn_failed_count;
+
+ /* Set status flag to signal iscsictl to stop polling */
+ p->sess_conn_status = p->session_count == num_accounted_conn;
+}
Re: [Patch] Make Azalia recognize audio chipset for Thinkpad T14s
Jonathan Gray writes: > pcidevs.h is a generated file. After pcidevs is modified 'make' is run > in sys/dev/pci then pcidevs.h and pcidevs_data.h are created based on > that. In this case it is already there though so you don't need to > change it. Thanks for the review. Updated patch attached. I've rebuilt and tested it to ensure it works. diff --git a/sys/dev/pci/azalia.c b/sys/dev/pci/azalia.c index 36e8ae36d27..65730dcfaf2 100644 --- a/sys/dev/pci/azalia.c +++ b/sys/dev/pci/azalia.c @@ -387,6 +387,7 @@ azalia_configure_pci(azalia_t *az) case PCI_PRODUCT_ATI_SB450_HDA: case PCI_PRODUCT_ATI_SBX00_HDA: case PCI_PRODUCT_AMD_HUDSON2_HDA: + case PCI_PRODUCT_ATI_RENOIR_HDA: reg = azalia_pci_read(az->pc, az->tag, ATI_PCIE_SNOOP_REG); reg &= ATI_PCIE_SNOOP_MASK; reg |= ATI_PCIE_SNOOP_ENABLE; diff --git a/sys/dev/pci/azalia_codec.c b/sys/dev/pci/azalia_codec.c index 33833d2f8ea..9a1e000e5f3 100644 --- a/sys/dev/pci/azalia_codec.c +++ b/sys/dev/pci/azalia_codec.c @@ -83,6 +83,10 @@ azalia_codec_init_vtbl(codec_t *this) this->name = "Realtek ALC221"; this->qrks |= AZ_QRK_WID_CDIN_1C | AZ_QRK_WID_BEEP_1D; break; + case 0x10ec0257: + this->name = "Realtek ALC257"; + this->qrks |= AZ_QRK_WID_CDIN_1C | AZ_QRK_WID_BEEP_1D; + break; case 0x10ec0260: this->name = "Realtek ALC260"; if (this->subid == 0x008f1025) dmesg: OpenBSD 6.8-current (GENERIC.MP) #13: Wed Oct 21 19:24:22 EDT 2020 f...@moon.fagg.id.au:/usr/src/sys/arch/amd64/compile/GENERIC.MP real mem = 16332681216 (15576MB) avail mem = 15822606336 (15089MB) random: good seed from bootblocks mpath0 at root scsibus0 at mpath0: 256 targets mainbus0 at root bios0 at mainbus0: SMBIOS rev. 3.2 @ 0xbf912000 (68 entries) bios0: vendor LENOVO version "R1CET36W(1.05 )" date 06/11/2020 bios0: LENOVO 20UH000CUS acpi0 at bios0: ACPI 6.3 acpi0: sleep states S0 S3 S4 S5 acpi0: tables DSDT FACP SSDT SSDT SSDT IVRS SSDT SSDT TPM2 SSDT MSDM BATB HPET APIC MCFG SBST WSMT VFCT SSDT CRAT CDIT FPDT SSDT SSDT SSDT UEFI SSDT SSDT BGRT acpi0: wakeup devices GPP0(S3) RESA(S3) GPP4(S4) GPP5(S3) L850(S3) GPP6(S3) GPP7(S3) GP17(S3) XHC0(S3) XHC1(S3) LID_(S4) SLPB(S3) acpitimer0 at acpi0: 3579545 Hz, 32 bits acpihpet0 at acpi0: 14318180 Hz acpimadt0 at acpi0 addr 0xfee0: PC-AT compat cpu0 at mainbus0: apid 0 (boot processor) cpu0: AMD Ryzen 7 PRO 4750U with Radeon Graphics, 1697.03 MHz, 17-60-01 cpu0: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,PCLMUL,MWAIT,SSSE3,FMA3,CX16,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,MASSE,3DNOWP,OSVW,IBS,SKINIT,TCE,TOPEXT,CPCTR,DBKP,PCTRL3,MWAITX,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,PQM,RDSEED,ADX,SMAP,CLFLUSHOPT,CLWB,SHA,UMIP,IBPB,IBRS,STIBP,SSBD,XSAVEOPT,XSAVEC,XGETBV1,XSAVES cpu0: 32KB 64b/line 8-way I-cache, 32KB 64b/line 8-way D-cache, 512KB 64b/line 8-way L2 cache, 8MB 64b/line disabled L3 cache cpu0: ITLB 64 4KB entries fully associative, 64 4MB entries fully associative cpu0: DTLB 64 4KB entries fully associative, 64 4MB entries fully associative cpu0: smt 0, core 0, package 0 mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges cpu0: apic clock running at 99MHz cpu0: mwait min=64, max=64, C-substates=1.1, IBE cpu1 at mainbus0: apid 1 (application processor) cpu1: AMD Ryzen 7 PRO 4750U with Radeon Graphics, 1696.82 MHz, 17-60-01 cpu1: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,PCLMUL,MWAIT,SSSE3,FMA3,CX16,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,MASSE,3DNOWP,OSVW,IBS,SKINIT,TCE,TOPEXT,CPCTR,DBKP,PCTRL3,MWAITX,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,PQM,RDSEED,ADX,SMAP,CLFLUSHOPT,CLWB,SHA,UMIP,IBPB,IBRS,STIBP,SSBD,XSAVEOPT,XSAVEC,XGETBV1,XSAVES cpu1: 32KB 64b/line 8-way I-cache, 32KB 64b/line 8-way D-cache, 512KB 64b/line 8-way L2 cache, 8MB 64b/line disabled L3 cache cpu1: ITLB 64 4KB entries fully associative, 64 4MB entries fully associative cpu1: DTLB 64 4KB entries fully associative, 64 4MB entries fully associative cpu1: disabling user TSC (skew=-392393047) cpu1: smt 1, core 0, package 0 cpu2 at mainbus0: apid 2 (application processor) cpu2: AMD Ryzen 7 PRO 4750U with Radeon Graphics, 1696.81 MHz, 17-60-01 cpu2: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,PCLMUL,MWAIT,SSSE3,FMA3,CX16,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,MASSE,3DNOWP,OSVW,IBS,SKINIT,TCE,TOPEXT,CPCTR,DBKP,PCTRL3,MWAITX,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,PQM,RDSEED,ADX,SMAP,CLFLUSHOPT,CLWB,SHA,UMIP,IBPB,IBRS,STIBP,SSBD,XSAVEOPT,XSAVEC,XGETBV1,XSAVES cpu2: 32KB 64b/line 8-way I-cache, 32KB 64b/line 8-way D-cache, 512KB 64b/line 8-way L2 cache, 8MB 64b/line disabled L3 cache cpu2: ITLB 64 4
Re: [Patch] Make Azalia recognize audio chipset for Thinkpad T14s
Ashton Fagg writes: > (My first OpenBSD patch - sorry if it's terrible) > > Attached is a patch to make the audio chipset (Realtek ALC257) in a > Thinkpad T14s get recognized by Azalia. It also sets the usual bits > that get set for most other devices. > > Additionally, this also enabled pci-e snooping for the Renoir HDA > (definition included in pcidevs.h). I am not sure if this is entirely > necessary, I am merely following along with what other patches for > similar things have done. Guidance on that would be appreciated. > > Tested locally, dmesg attached. azalia correctly registers device as > "Realtek ALC257". Someone was kind enough to point out that I had neglected to KNF the patch. Apologies, see attached for a KNF'ed version. diff --git a/sys/dev/pci/azalia.c b/sys/dev/pci/azalia.c index 36e8ae36d27..9c8d8665b7d 100644 --- a/sys/dev/pci/azalia.c +++ b/sys/dev/pci/azalia.c @@ -387,6 +387,7 @@ azalia_configure_pci(azalia_t *az) case PCI_PRODUCT_ATI_SB450_HDA: case PCI_PRODUCT_ATI_SBX00_HDA: case PCI_PRODUCT_AMD_HUDSON2_HDA: + case PCI_PRODUCT_AMD_RENOIR_HDA: reg = azalia_pci_read(az->pc, az->tag, ATI_PCIE_SNOOP_REG); reg &= ATI_PCIE_SNOOP_MASK; reg |= ATI_PCIE_SNOOP_ENABLE; diff --git a/sys/dev/pci/azalia_codec.c b/sys/dev/pci/azalia_codec.c index 33833d2f8ea..9a1e000e5f3 100644 --- a/sys/dev/pci/azalia_codec.c +++ b/sys/dev/pci/azalia_codec.c @@ -83,6 +83,10 @@ azalia_codec_init_vtbl(codec_t *this) this->name = "Realtek ALC221"; this->qrks |= AZ_QRK_WID_CDIN_1C | AZ_QRK_WID_BEEP_1D; break; + case 0x10ec0257: + this->name = "Realtek ALC257"; + this->qrks |= AZ_QRK_WID_CDIN_1C | AZ_QRK_WID_BEEP_1D; + break; case 0x10ec0260: this->name = "Realtek ALC260"; if (this->subid == 0x008f1025) diff --git a/sys/dev/pci/pcidevs.h b/sys/dev/pci/pcidevs.h index e5f74e84555..c09b1a6e521 100644 --- a/sys/dev/pci/pcidevs.h +++ b/sys/dev/pci/pcidevs.h @@ -946,6 +946,7 @@ #define PCI_PRODUCT_AMD_RS780_PCIE_7 0x9608 /* RS780 PCIE */ #define PCI_PRODUCT_AMD_RS780_PCIE_8 0x9609 /* RS780 PCIE */ #define PCI_PRODUCT_AMD_RS780_PCIE_9 0x960b /* RS780 PCIE */ +#define PCI_PRODUCT_AMD_RENOIR_HDA 0x1637 /* Renoir HD Audio */ /* AMI */ #define PCI_PRODUCT_AMI_MEGARAID 0x1960 /* MegaRAID */
[Patch] Make Azalia recognize audio chipset for Thinkpad T14s
(My first OpenBSD patch - sorry if it's terrible) Attached is a patch to make the audio chipset (Realtek ALC257) in a Thinkpad T14s get recognized by Azalia. It also sets the usual bits that get set for most other devices. Additionally, this also enabled pci-e snooping for the Renoir HDA (definition included in pcidevs.h). I am not sure if this is entirely necessary, I am merely following along with what other patches for similar things have done. Guidance on that would be appreciated. Tested locally, dmesg attached. azalia correctly registers device as "Realtek ALC257". diff --git a/sys/dev/pci/azalia.c b/sys/dev/pci/azalia.c index 36e8ae36d27..f821919489e 100644 --- a/sys/dev/pci/azalia.c +++ b/sys/dev/pci/azalia.c @@ -387,6 +387,7 @@ azalia_configure_pci(azalia_t *az) case PCI_PRODUCT_ATI_SB450_HDA: case PCI_PRODUCT_ATI_SBX00_HDA: case PCI_PRODUCT_AMD_HUDSON2_HDA: +case PCI_PRODUCT_AMD_RENOIR_HDA: reg = azalia_pci_read(az->pc, az->tag, ATI_PCIE_SNOOP_REG); reg &= ATI_PCIE_SNOOP_MASK; reg |= ATI_PCIE_SNOOP_ENABLE; diff --git a/sys/dev/pci/azalia_codec.c b/sys/dev/pci/azalia_codec.c index 33833d2f8ea..7edb8dce6be 100644 --- a/sys/dev/pci/azalia_codec.c +++ b/sys/dev/pci/azalia_codec.c @@ -83,6 +83,10 @@ azalia_codec_init_vtbl(codec_t *this) this->name = "Realtek ALC221"; this->qrks |= AZ_QRK_WID_CDIN_1C | AZ_QRK_WID_BEEP_1D; break; +case 0x10ec0257: +this->name = "Realtek ALC257"; +this->qrks |= AZ_QRK_WID_CDIN_1C | AZ_QRK_WID_BEEP_1D; +break; case 0x10ec0260: this->name = "Realtek ALC260"; if (this->subid == 0x008f1025) diff --git a/sys/dev/pci/pcidevs.h b/sys/dev/pci/pcidevs.h index e5f74e84555..b80d2669670 100644 --- a/sys/dev/pci/pcidevs.h +++ b/sys/dev/pci/pcidevs.h @@ -946,6 +946,7 @@ #define PCI_PRODUCT_AMD_RS780_PCIE_7 0x9608 /* RS780 PCIE */ #define PCI_PRODUCT_AMD_RS780_PCIE_8 0x9609 /* RS780 PCIE */ #define PCI_PRODUCT_AMD_RS780_PCIE_9 0x960b /* RS780 PCIE */ +#define PCI_PRODUCT_AMD_RENOIR_HDA 0x1637 /* Renoir HD Audio */ /* AMI */ #define PCI_PRODUCT_AMI_MEGARAID 0x1960 /* MegaRAID */ OpenBSD 6.8-current (GENERIC.MP) #0: Sun Oct 18 12:17:14 EDT 2020 f...@moon.fagg.id.au:/usr/src/sys/arch/amd64/compile/GENERIC.MP real mem = 16332681216 (15576MB) avail mem = 15822606336 (15089MB) random: good seed from bootblocks mpath0 at root scsibus0 at mpath0: 256 targets mainbus0 at root bios0 at mainbus0: SMBIOS rev. 3.2 @ 0xbf912000 (68 entries) bios0: vendor LENOVO version "R1CET36W(1.05 )" date 06/11/2020 bios0: LENOVO 20UH000CUS acpi0 at bios0: ACPI 6.3 acpi0: sleep states S0 S3 S4 S5 acpi0: tables DSDT FACP SSDT SSDT SSDT IVRS SSDT SSDT TPM2 SSDT MSDM BATB HPET APIC MCFG SBST WSMT VFCT SSDT CRAT CDIT FPDT SSDT SSDT SSDT UEFI SSDT SSDT BGRT acpi0: wakeup devices GPP0(S3) RESA(S3) GPP4(S4) GPP5(S3) L850(S3) GPP6(S3) GPP7(S3) GP17(S3) XHC0(S3) XHC1(S3) LID_(S4) SLPB(S3) acpitimer0 at acpi0: 3579545 Hz, 32 bits acpihpet0 at acpi0: 14318180 Hz acpimadt0 at acpi0 addr 0xfee0: PC-AT compat cpu0 at mainbus0: apid 0 (boot processor) cpu0: AMD Ryzen 7 PRO 4750U with Radeon Graphics, 1697.05 MHz, 17-60-01 cpu0: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,PCLMUL,MWAIT,SSSE3,FMA3,CX16,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,MASSE,3DNOWP,OSVW,IBS,SKINIT,TCE,TOPEXT,CPCTR,DBKP,PCTRL3,MWAITX,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,PQM,RDSEED,ADX,SMAP,CLFLUSHOPT,CLWB,SHA,UMIP,IBPB,IBRS,STIBP,SSBD,XSAVEOPT,XSAVEC,XGETBV1,XSAVES cpu0: 32KB 64b/line 8-way I-cache, 32KB 64b/line 8-way D-cache, 512KB 64b/line 8-way L2 cache, 8MB 64b/line disabled L3 cache cpu0: ITLB 64 4KB entries fully associative, 64 4MB entries fully associative cpu0: DTLB 64 4KB entries fully associative, 64 4MB entries fully associative cpu0: smt 0, core 0, package 0 mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges cpu0: apic clock running at 99MHz cpu0: mwait min=64, max=64, C-substates=1.1, IBE cpu1 at mainbus0: apid 1 (application processor) cpu1: AMD Ryzen 7 PRO 4750U with Radeon Graphics, 1696.82 MHz, 17-60-01 cpu1: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,PCLMUL,MWAIT,SSSE3,FMA3,CX16,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,MASSE,3DNOWP,OSVW,IBS,SKINIT,TCE,TOPEXT,CPCTR,DBKP,PCTRL3,MWAITX,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,PQM,RDSEED,ADX,SMAP,CLFLUSHOPT,CLWB,SHA,UMIP,IBPB,IBRS,STIBP,SSBD,XSAVEOPT,XSAVEC,XGETBV1,XSAVES cpu1: 32KB 64b/line 8-way I-cache, 32KB 64b/line 8-way D-cache, 512KB 64b/line 8-way L2 cache, 8MB 64b/line disabled L3 cache cpu1: ITLB 64 4KB entries fully associative, 64 4MB entries fully associative cpu1: DTLB 64 4KB entries fully associative, 64 4MB entries fully associative cpu1: disabling user TSC (skew=-392238127) cpu1:
