usbd_new_device: consolidate error paths
Hi,
Consolidate error paths in usbd_new_device, shaving of 14 lines.
Comments? OK?
diff --git sys/dev/usb/usb_subr.c sys/dev/usb/usb_subr.c
index a11bc448ece..74fe4c3b4db 100644
--- sys/dev/usb/usb_subr.c
+++ sys/dev/usb/usb_subr.c
@@ -1086,11 +1086,8 @@ usbd_new_device(struct device *parent, struct usbd_bus
*bus, int depth,
/* Establish the default pipe. */
err = usbd_setup_pipe(dev, 0, >def_ep, USBD_DEFAULT_INTERVAL,
>default_pipe);
- if (err) {
- usb_free_device(dev);
- up->device = NULL;
- return (err);
- }
+ if (err)
+ goto fail;
dd = >ddesc;
@@ -1137,12 +1134,8 @@ usbd_new_device(struct device *parent, struct usbd_bus
*bus, int depth,
USB_MAX_IPACKET, dd);
}
- if (err) {
-fail:
- usb_free_device(dev);
- up->device = NULL;
- return (err);
- }
+ if (err)
+ goto fail;
DPRINTF(("%s: adding unit addr=%d, rev=%02x, class=%d, subclass=%d, "
"protocol=%d, maxpacket=%d, len=%d, speed=%d\n", __func__,
@@ -1152,9 +1145,8 @@ fail:
if ((dd->bDescriptorType != UDESC_DEVICE) ||
(dd->bLength < USB_DEVICE_DESCRIPTOR_SIZE)) {
- usb_free_device(dev);
- up->device = NULL;
- return (USBD_INVAL);
+ err = USBD_INVAL;
+ goto fail;
}
mps = dd->bMaxPacketSize;
@@ -1168,9 +1160,8 @@ fail:
if (mps != mps0) {
if ((speed == USB_SPEED_LOW) ||
(mps != 8 && mps != 16 && mps != 32 && mps != 64)) {
- usb_free_device(dev);
- up->device = NULL;
- return (USBD_INVAL);
+ err = USBD_INVAL;
+ goto fail;
}
USETW(dev->def_ep_desc.wMaxPacketSize, mps);
}
@@ -1179,9 +1170,8 @@ fail:
/* Set the address if the HC didn't do it already. */
if (bus->methods->dev_setaddr != NULL &&
bus->methods->dev_setaddr(dev, addr)) {
- usb_free_device(dev);
- up->device = NULL;
- return (USBD_SET_ADDR_FAILED);
+ err = USBD_SET_ADDR_FAILED;
+ goto fail;
}
/* Wait for device to settle before reloading the descriptor. */
@@ -1194,11 +1184,8 @@ fail:
dev->address = addr;
err = usbd_reload_device_desc(dev);
- if (err) {
- usb_free_device(dev);
- up->device = NULL;
- return (err);
- }
+ if (err)
+ goto fail;
/* send disown request to handover 2.0 to 1.1. */
if (dev->quirks->uq_flags & UQ_EHCI_NEEDTO_DISOWN) {
@@ -1222,22 +1209,21 @@ fail:
/* Get device info and cache it */
err = usbd_cache_devinfo(dev);
- if (err) {
- usb_free_device(dev);
- up->device = NULL;
- return (err);
- }
+ if (err)
+ goto fail;
bus->devices[addr] = dev;
err = usbd_probe_and_attach(parent, dev, port, addr);
- if (err) {
- usb_free_device(dev);
- up->device = NULL;
- return (err);
- }
+ if (err)
+ goto fail;
return (USBD_NORMAL_COMPLETION);
+
+fail:
+ usb_free_device(dev);
+ up->device = NULL;
+ return (err);
}
usbd_status
Re: Please test: UVM fault unlocking (aka vmobjlock)
Hi, I've tested the second diff (containing the bug fix and I get a lock order reversal on bootup. On the first patch I was getting the same lock order reversal while using X and sometimes (only sometimes) would crash X. witness: lock order reversal: 1st 0xfd83fef05248 uobjlk (>vmobjlock) 2nd 0x8119f700 objmm (>mm.lock) lock order ">mm.lock"(rwlock) -> ">vmobjlock"(rwlock) first seen at: #0 rw_enter+0x65 #1 uvm_obj_wire+0x46 #2 shmem_get_pages+0xaf #3 __i915_gem_object_get_pages+0x9d #4 i915_vma_pin_ww+0x49b #5 i915_ggtt_pin+0x61 #6 intel_execlists_submission_setup+0x3b2 #7 intel_engines_init+0x2ff #8 intel_gt_init+0x133 #9 i915_gem_init+0xa3 #10 i915_driver_probe+0x75a #11 inteldrm_attachhook+0x45 #12 config_process_deferred_mountroot+0x6b #13 main+0x743 lock order ">vmobjlock"(rwlock) -> ">mm.lock"(rwlock) first seen at: #0 rw_enter+0x65 #1 __i915_gem_object_get_pages+0x30 #2 i915_gem_fault+0x1aa #3 drm_fault+0x156 #4 uvm_fault+0x179 #5 upageflttrap+0x62 #6 usertrap+0x18b #7 recall_trap+0x8 Dmesg: OpenBSD 7.0-current (GENERIC.MP) #3: Thu Dec 2 18:38:43 GMT 2021 pertho@pertho.local:/usr/src/sys/arch/amd64/compile/GENERIC.MP real mem = 17040445440 (16251MB) avail mem = 16376905728 (15618MB) random: good seed from bootblocks mpath0 at root scsibus0 at mpath0: 256 targets mainbus0 at root bios0 at mainbus0: SMBIOS rev. 3.0 @ 0x7b288000 (41 entries) bios0: vendor American Megatrends Inc. version "1.05.07" date 09/29/2017 bios0: PC Specialist LTD N13xWU acpi0 at bios0: ACPI 6.0 acpi0: sleep states S0 S3 S4 S5 acpi0: tables DSDT FACP APIC FPDT FIDT MCFG SSDT SSDT HPET UEFI SSDT SSDT SSDT DBGP DBG2 DMAR BGRT ASF! WSMT acpi0: wakeup devices RP17(S4) PXSX(S4) RP18(S4) PXSX(S4) RP19(S4) PXSX(S4) RP20(S4) PXSX(S4) RP21(S4) PXSX(S4) RP22(S4) PXSX(S4) RP23(S4) PXSX(S4) RP24(S4) PXSX(S4) [...] acpitimer0 at acpi0: 3579545 Hz, 24 bits acpimadt0 at acpi0 addr 0xfee0: PC-AT compat cpu0 at mainbus0: apid 0 (boot processor) cpu0: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz, 1696.05 MHz, 06-8e-0a cpu0: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,TSC_ADJUST,SGX,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,MPX,RDSEED,ADX,SMAP,CLFLUSHOPT,PT,SRBDS_CTRL,MD_CLEAR,TSXFA,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,XSAVEC,XGETBV1,XSAVES,MELTDOWN cpu0: 256KB 64b/line 8-way L2 cache cpu0: smt 0, core 0, package 0 mtrr: Pentium Pro MTRR support, 10 var ranges, 88 fixed ranges cpu0: apic clock running at 24MHz cpu0: mwait min=64, max=64, C-substates=0.2.1.2.4.1.1.1, IBE cpu1 at mainbus0: apid 2 (application processor) cpu1: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz, 1696.05 MHz, 06-8e-0a cpu1: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,TSC_ADJUST,SGX,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,MPX,RDSEED,ADX,SMAP,CLFLUSHOPT,PT,SRBDS_CTRL,MD_CLEAR,TSXFA,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,XSAVEC,XGETBV1,XSAVES,MELTDOWN cpu1: 256KB 64b/line 8-way L2 cache cpu1: smt 0, core 1, package 0 cpu2 at mainbus0: apid 4 (application processor) cpu2: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz, 1696.05 MHz, 06-8e-0a cpu2: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,TSC_ADJUST,SGX,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,MPX,RDSEED,ADX,SMAP,CLFLUSHOPT,PT,SRBDS_CTRL,MD_CLEAR,TSXFA,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,XSAVEC,XGETBV1,XSAVES,MELTDOWN cpu2: 256KB 64b/line 8-way L2 cache cpu2: smt 0, core 2, package 0 cpu3 at mainbus0: apid 6 (application processor) cpu3: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz, 1696.05 MHz, 06-8e-0a cpu3: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,TSC_ADJUST,SGX,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,MPX,RDSEED,ADX,SMAP,CLFLUSHOPT,PT,SRBDS_CTRL,MD_CLEAR,TSXFA,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,XSAVEC,XGETBV1,XSAVES,MELTDOWN cpu3: 256KB 64b/line 8-way L2 cache cpu3: smt 0, core 3, package 0 cpu4 at mainbus0: apid 1 (application processor) cpu4: Intel(R) Core(TM)
Re: Rework UNIX sockets locking to be fine grained
Thanks! Bulk build make us sure all packages have been built with new headers. > On 2 Dec 2021, at 16:26, Christian Weisgerber wrote: > > Vitaliy Makkoveev: > >> Include missing "sys/refcnt.h" header to unpcb.h to fix libkvm and >> netstat(1) build. No functional changes. > > I ran an amd64 package bulk build with this, four ncpu=4 machines. > No problems. > > (Other than for dpb's ssh connection multiplexing, I don't think > Unix domain sockets or FIFOs are really used by this workload, > though.) > > -- > Christian "naddy" Weisgerber na...@mips.inka.de
Re: bsd.upgrade fails `umount /mnt` with a single partition disk.
This reads correct.
OK florian
On 2021-12-02 00:01 -07, "Theo de Raadt" wrote:
> Yuichiro NAITO wrote:
>
>> bsd.upgrade fails and shows following messages with a single partition disk.
>>
>> ```
>> Force checking of clean non-root filesystems? [no] no
>> umount: /mnt: Device busy
>> Can't umount sd0a!
>
> OK, we figured out what is going on. During upgrade, the async random
> seed feeding code has vnodes open on the bsd.rd ramdisk root filesystem
> (mounted as /), and also on the real root (mounted as /mnt, to collect
> fstab information). The umount honestly can fail to flush all the vnodes,
> I suspect because the random seed generation processes have vnodes on both
> filesystems. The EBUSY is true.
>
> The install script can umount /mnt earlier, in fact immediately after it
> has collected fstab information, and this is before the cgi fetching and
> random feeding.
>
> Yuichiro built a reliable & quick test case, and with this diff (based
> upon an earlier proposal from him), doesn't see the problem anymore.
>
> Index: distrib/miniroot/install.sub
> ===
> RCS file: /cvs/src/distrib/miniroot/install.sub,v
> retrieving revision 1.1184
> diff -u -p -u -r1.1184 install.sub
> --- distrib/miniroot/install.sub 2 Nov 2021 16:54:01 - 1.1184
> +++ distrib/miniroot/install.sub 2 Dec 2021 05:19:45 -
> @@ -3258,17 +3258,19 @@ do_upgrade() {
> # Configure the network.
> enable_network
>
> - # Fetch list of mirror servers and installer choices from previous runs.
> - start_cgiinfo
> -
> # Create a skeletal /etc/fstab which is usable for the upgrade process.
> munge_fstab
>
> + # Do not need to look in /mnt anymore
> + umount /mnt || { echo "Can't umount $ROOTDEV!"; exit; }
> +
> + # Fetch list of mirror servers and installer choices from previous runs.
> + start_cgiinfo
> +
> # fsck -p non-root filesystems in /etc/fstab.
> check_fs
>
> # Mount filesystems in /etc/fstab.
> - umount /mnt || { echo "Can't umount $ROOTDEV!"; exit; }
> mount_fs
>
> rm -f /mnt/bsd.upgrade /mnt/auto_upgrade.conf
>
--
I'm not entirely sure you are real.
Re: Please test: UVM fault unlocking (aka vmobjlock)
On Mon, Nov 29, 2021 at 10:50:32PM +0100, Martin Pieuchot wrote: > On 24/11/21(Wed) 11:16, Martin Pieuchot wrote: > > Diff below unlock the bottom part of the UVM fault handler. I'm > > interested in squashing the remaining bugs. Please test with your usual > > setup & report back. > > Thanks to all the testers, here's a new version that includes a bug fix. > > Tests on !x86 architectures are much appreciated! GENERIC.MP build becomes noticably faster on a sparc64 T4-2 LDOM: This diff survived a full regress as well as all kernel builds. I've run regress and builds in parallel each, i.e. one `make' per regress/ and sparc64/compile/ subdirectory.
Re: Please test: UVM fault unlocking (aka vmobjlock)
On Mon, Nov 29, 2021 at 10:50:32PM +0100, Martin Pieuchot wrote: > On 24/11/21(Wed) 11:16, Martin Pieuchot wrote: > > Diff below unlock the bottom part of the UVM fault handler. I'm > > interested in squashing the remaining bugs. Please test with your usual > > setup & report back. > > Thanks to all the testers, here's a new version that includes a bug fix. I removed the witness part and ran it through my perfomamnce tests. 4 core machine http://bluhm.genua.de/perform/results/2021-12-01T23:10:12Z/perform.html 8 core, 2 socket machine http://bluhm.genua.de/perform/results/2021-12-01T09:53:27Z/perform.html The best improvement is make -j8 bsd on the 8 core machine, 22% system time, 5% real time decrease. I see slowdown in network performance, but that may be within the variation I always see when kernel layout changes. Here you can see best how kernel lock goes away in usertrap. http://bluhm.genua.de/perform/results/2021-12-01T09:53:27Z/2021-12-01T00%3A00%3A00Z/btrace/time_-lp_make_-CGENERIC.MP_-j8_-s-btrace-kstack.0.svg http://bluhm.genua.de/perform/results/2021-12-01T09:53:27Z/patch-mpi-uvm-unlock-nowitness.0/btrace/time_-lp_make_-CGENERIC.MP_-j8_-s-btrace-kstack.0.svg On 4 core machine system time during kernel build is reduced by 6%. bluhm
Re: Rework UNIX sockets locking to be fine grained
Vitaliy Makkoveev: > Include missing "sys/refcnt.h" header to unpcb.h to fix libkvm and > netstat(1) build. No functional changes. I ran an amd64 package bulk build with this, four ncpu=4 machines. No problems. (Other than for dpb's ssh connection multiplexing, I don't think Unix domain sockets or FIFOs are really used by this workload, though.) -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: sppp(4)/pppoe(4) - avoid endless loop in remote ip negotiation
On Fri, Nov 26, 2021 at 01:35:14PM +0100, Krzysztof Kanas wrote: > Hi. When remote side in sppp doesn't reply for to PPP IPCP IP-Address > sppp will try to negotiate remote IP in endless loop. Instead use > 10.64.64.1 + if_index as remote IP. > Okay, but why it doesn't reply? I don't use anymore PPPoE, as none of the machines connected to the internet use that mechanism, but from my very old backups I see I used in the past: # /etc/hostname.pppoe0 inet 0.0.0.0 255.255.255.255 NONE pppoedev vr0 authproto chap \ authname 'some-customer-id-here' authkey 'secret-key' \ description "pppoe uplink" up dest 0.0.0.1 !route -nq add default -ifp \$if 0.0.0.1 I don't remember any issues with above setup and that OpenBSD machine was running -current and was rebooted many times during its 6+ years of service. Also machine faced many power outages and on a fresh bootup I don't remember it not able to connect, that I needed to run a custom kernel or do some extensive troubleshooting. Looking again at pppoe(4) manual page I copied above config directly from there. What problem are you trying to solve here is not clear to me. -- Regards, Mikolaj
Re: sppp(4)/pppoe(4) - avoid endless loop in remote ip negotiation
On 2021/12/01 20:58, Krzysztof Kanas wrote: > On 2021-12-01 10:21, Klemens Nanni wrote: > > On Fri, Nov 26, 2021 at 01:35:14PM +0100, Krzysztof Kanas wrote: > > > Hi. When remote side in sppp doesn't reply for to PPP IPCP IP-Address > > > sppp will try to negotiate remote IP in endless loop. Instead use > > > 10.64.64.1 + if_index as remote IP. > > > > Why add some arbitrary RFC 1918 IP if negotiation fails? That's not > > what users request or expect. > > > > I agree that is unexpected but I couldn't find any info in RFC's what to > do if remote side don't reply. I am not familiar enough with PPP/SPP, > so I tried simples thing I could think of, which is to assign RFC 1918 > IP address. IMHO: either keep waiting (the other side may complete IPCP later), or after X retries then drop the session and reattempt negotiation from scratch. A session that is up and stuck with a bogus IP is probably worse than a session that keeps trying to negotiate an address isn't it? How are you configuring the interface? Due to the race condition with "auto up" behaviour on the interface, I would recommend something like this in hostname.pppoeX, you need to have the dest address set to 0.0.0.1 *before* the interface comes up and if you follow the "clean" example in the manpage that doesn't always work inet 0.0.0.0 255.255.255.255 0.0.0.1 pppoedev em1 authproto chap authname "xyz@foo" authkey "fnord" up This abuses the fact that the dest address in stored in the same variable as the broadcast address on an ethernet interface (my diff to change the example in the manual previously was rejected due to this) but it works ok everywhere I've tried it and "dest" on a separate line doesn't.
Re: ipsec useless inner header
On Thu, Dec 02, 2021 at 01:24:36AM +0100, Alexander Bluhm wrote: > ipsec_common_input_cb() extracts the inner IP header of IPsec > tunnels. It is never used, so this is useless code. OK kn
