From nobody Tue Dec 06 16:09:36 2022 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4NRQNX4xZXz4k1wP for ; Tue, 6 Dec 2022 16:09:48 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-ej1-x62f.google.com (mail-ej1-x62f.google.com [IPv6:2a00:1450:4864:20::62f]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4NRQNX34fdz43tD for ; Tue, 6 Dec 2022 16:09:48 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-ej1-x62f.google.com with SMTP id gh17so7371341ejb.6 for ; Tue, 06 Dec 2022 08:09:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20210112.gappssmtp.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=Iag/yyC/xN7WqkwSyScuCPjo9sqBJejuRUM//LWxkEs=; b=1jPbyV6RW3ehnHnPbH4JVJ1pNk3aUkXQpKNQPiDdSDs+Tp4gkxNPqB0YJjOxzHeRa6 ZmCMnLKP1DybDYzNkbbYVU3wpOchXTea6kiZO4f/grW9JJ6/BBBOGwErJF832n5/Xa0X dvL8fudP7/5ZuxozyH7dyne38tZTAyKZ3UgvOD53yGc75arcnb1+ltUFukLTGx/qszVa GVTmIMlYZmddr8dUOqunVFE15HDIU+qU+tXTvq+/Jsfx1XHwLa43fVtqoGnFIVgR6yvo MeczeAVQ3akBb7+c/lCntViMrc6QK9+vzOqr4WeO8NeaCmGAzJIJrm/CfbIdr+I0DvVW lYZg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Iag/yyC/xN7WqkwSyScuCPjo9sqBJejuRUM//LWxkEs=; b=jNVmCPWjFuFKWjkq9CwLd/kg5ikmiwsqFD5Jc7k4bFK+oAyV8oPAN0nT80eQPLfuf3 gq8MaHzjkFE6Oa6YZcpBtIjWe+E8tKo6ac6sjrtxPl5LW5uUU/S3nxrn4Vb90xzm+RHm /3R7b2GNbvtxNmdgWlDHjK9XCPwHvUVX7aesmNn1Qg7AWOipf5Ei6XdTtKAHJZql1VrX +4+OeBGrSnKbSNFXZ0xWdAFZRD2Q9S6bL+kv9if736fNJG6G3WNibEQbNFsy3iuqSYi5 YXMwGmwyagUFw5RKwstYsC0nomuP4+mFaxYOQ5x0cXydEkdoQ7uqznI+QCOJ1urtoTdx mqOA== X-Gm-Message-State: ANoB5pkgH4R9Lf51Ole8ZMgxotNac7Z4icOGqb991BbBG2Ub30n2h29N EWCsyTVYjQySAJNpgailpYYZqI2ToCmzB4c+vE8k/w== X-Google-Smtp-Source: AA0mqf5p+0HWkvY5qdbY8tKsaBJZI+W0Tiv6Uq+Zu6V4qv/qJF+ZaJkJldc/QGDuinhCLv1chS4fVYpu9XmKssW4OWg= X-Received: by 2002:a17:906:c2d3:b0:7c1:535:f2fb with SMTP id ch19-20020a170906c2d300b007c10535f2fbmr4640828ejb.252.1670342986994; Tue, 06 Dec 2022 08:09:46 -0800 (PST) List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 References: <202212060209.2B629pnu053879@gitrepo.freebsd.org> In-Reply-To: From: Warner Losh Date: Tue, 6 Dec 2022 09:09:36 -0700 Message-ID: Subject: Re: git: 3cf97e91fac5 - main - Revert "newbus: Change attach failure behavior" To: Hans Petter Selasky Cc: Warner Losh , src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: multipart/alternative; boundary="00000000000064aa5a05ef2b0876" X-Rspamd-Queue-Id: 4NRQNX34fdz43tD X-Spamd-Bar: ---- X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US] X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-ThisMailContainsUnwantedMimeParts: N --00000000000064aa5a05ef2b0876 Content-Type: text/plain; charset="UTF-8" On Tue, Dec 6, 2022 at 3:57 AM Hans Petter Selasky wrote: > On 12/6/22 03:09, Warner Losh wrote: > > The branch main has been updated by imp: > > > > URL: > https://cgit.FreeBSD.org/src/commit/?id=3cf97e91fac5f53fc0375bc816cc541a8864ffc4 > > > > commit 3cf97e91fac5f53fc0375bc816cc541a8864ffc4 > > Author: Warner Losh > > AuthorDate: 2022-12-05 23:57:58 +0000 > > Commit: Warner Losh > > CommitDate: 2022-12-06 00:00:26 +0000 > > > > Revert "newbus: Change attach failure behavior" > > > > This reverts commit 68c3f0302106643207dcdfe3b414810e245228e5. There > are > > some weird crashes when KVMs switch caused by this, so revert this > > commit until they are sorted out. > > > > Reported by: cy@ > > Sponsored by: Netflix > > --- > > UPDATING | 2 ++ > > sys/kern/subr_bus.c | 2 +- > > 2 files changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/UPDATING b/UPDATING > > index 099066031b8e..001ec9f6de3a 100644 > > --- a/UPDATING > > +++ b/UPDATING > > @@ -43,6 +43,8 @@ NOTE TO PEOPLE WHO THINK THAT FreeBSD 14.x IS SLOW: > > needs to use devctl to re-enable the device, and reprobe it (or set > > the sysctl/tunable hw.bus.disable_failed_devices=false). > > > > + NOTE: This was reverted 20221205 due to unexpected compatibility > issues > > + > > 20221122: > > pf no longer accepts 'scrub fragment crop' or 'scrub fragment > drop-ovl'. > > These configurations are no longer automatically reinterpreted as > > diff --git a/sys/kern/subr_bus.c b/sys/kern/subr_bus.c > > index 6a5ec4efc38d..b9615b033007 100644 > > --- a/sys/kern/subr_bus.c > > +++ b/sys/kern/subr_bus.c > > @@ -69,7 +69,7 @@ SYSCTL_NODE(_hw, OID_AUTO, bus, CTLFLAG_RW | > CTLFLAG_MPSAFE, NULL, > > SYSCTL_ROOT_NODE(OID_AUTO, dev, CTLFLAG_RW | CTLFLAG_MPSAFE, NULL, > > NULL); > > > > -static bool disable_failed_devs = true; > > +static bool disable_failed_devs = false; > > SYSCTL_BOOL(_hw_bus, OID_AUTO, disable_failed_devices, CTLFLAG_RWTUN, > &disable_failed_devs, > > 0, "Do not retry attaching devices that return an error from > DEVICE_ATTACH the first time"); > > > > Thinking about it, this flag shouldn't be set for USB devices and HUBS > and such. Probably only makes sense for PCI devices, though there is > something called thunderbolt too, which may fail during probe/attach, > because the user yanked the device. > I think it makes perfect sense for all devices everywhere. When a device goes away like you say, it's device_t will be gone soonish and this flag will clear if it is reinserted in the future. The bus will get a signal for that yanking and will remove the device_t (now maybe we have a bug in device deletion when that happens, which is what I suspected when I saw this and a couple other tracebacks). > Regarding the assert in the USB stack, maybe the state was not correctly > set on the device_t ? > It's unclear to me. Newbus doesn't guarantee certain states to the bus drivers, so maybe the assert in the USB stack is incorrectly strict on what states it assumes the device is in? I'm unsure. I haven't looked deeply enough to know what exactly is going on. Since there were problems and I didn't have time to do the proper deep dive, I just reverted for now and will revisit when I have the time. Warner --00000000000064aa5a05ef2b0876 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Tue, Dec 6, 2022 at 3:57 AM Hans P= etter Selasky <hps@selasky.org>= ; wrote:
On 12/6= /22 03:09, Warner Losh wrote:
> The branch main has been updated by imp:
>
> URL: https://= cgit.FreeBSD.org/src/commit/?id=3D3cf97e91fac5f53fc0375bc816cc541a8864ffc4<= /a>
>
> commit 3cf97e91fac5f53fc0375bc816cc541a8864ffc4
> Author:=C2=A0 =C2=A0 =C2=A0Warner Losh <imp@FreeBSD.org>
> AuthorDate: 2022-12-05 23:57:58 +0000
> Commit:=C2=A0 =C2=A0 =C2=A0Warner Losh <imp@FreeBSD.org>
> CommitDate: 2022-12-06 00:00:26 +0000
>
>=C2=A0 =C2=A0 =C2=A0 Revert "newbus: Change attach failure behavio= r"
>=C2=A0 =C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0 This reverts commit 68c3f0302106643207dcdfe3b41481= 0e245228e5. There are
>=C2=A0 =C2=A0 =C2=A0 some weird crashes when KVMs switch caused by this= , so revert this
>=C2=A0 =C2=A0 =C2=A0 commit until they are sorted out.
>=C2=A0 =C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0 Reported by:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 cy@
>=C2=A0 =C2=A0 =C2=A0 Sponsored by:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0Netflix
> ---
>=C2=A0 =C2=A0UPDATING=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | 2 ++ >=C2=A0 =C2=A0sys/kern/subr_bus.c | 2 +-
>=C2=A0 =C2=A02 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/UPDATING b/UPDATING
> index 099066031b8e..001ec9f6de3a 100644
> --- a/UPDATING
> +++ b/UPDATING
> @@ -43,6 +43,8 @@ NOTE TO PEOPLE WHO THINK THAT FreeBSD 14.x IS SLOW:<= br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0needs to use devctl to re-enable the device,= and reprobe it (or set
>=C2=A0 =C2=A0 =C2=A0 =C2=A0the sysctl/tunable hw.bus.disable_failed_dev= ices=3Dfalse).
>=C2=A0 =C2=A0
> +=C2=A0 =C2=A0 =C2=A0NOTE: This was reverted 20221205 due to unexpecte= d compatibility issues
> +
>=C2=A0 =C2=A020221122:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0pf no longer accepts 'scrub fragment cro= p' or 'scrub fragment drop-ovl'.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0These configurations are no longer automatic= ally reinterpreted as
> diff --git a/sys/kern/subr_bus.c b/sys/kern/subr_bus.c
> index 6a5ec4efc38d..b9615b033007 100644
> --- a/sys/kern/subr_bus.c
> +++ b/sys/kern/subr_bus.c
> @@ -69,7 +69,7 @@ SYSCTL_NODE(_hw, OID_AUTO, bus, CTLFLAG_RW | CTLFLAG= _MPSAFE, NULL,
>=C2=A0 =C2=A0SYSCTL_ROOT_NODE(OID_AUTO, dev, CTLFLAG_RW | CTLFLAG_MPSAF= E, NULL,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0NULL);
>=C2=A0 =C2=A0
> -static bool disable_failed_devs =3D true;
> +static bool disable_failed_devs =3D false;
>=C2=A0 =C2=A0SYSCTL_BOOL(_hw_bus, OID_AUTO, disable_failed_devices, CTL= FLAG_RWTUN, &disable_failed_devs,
>=C2=A0 =C2=A0 =C2=A0 =C2=A00, "Do not retry attaching devices that= return an error from DEVICE_ATTACH the first time");
>=C2=A0 =C2=A0

Thinking about it, this flag shouldn't be set for USB devices and HUBS =
and such. Probably only makes sense for PCI devices, though there is
something called thunderbolt too, which may fail during probe/attach,
because the user yanked the device.

I t= hink it makes perfect sense for all devices everywhere. When a device goes<= /div>
away like you say, it's device_t will be gone soonish and thi= s flag will clear if
it is reinserted in the future. The bus will= get a signal for that yanking and will
remove the device_t (now = maybe we have a bug in device deletion when that
happens, which i= s what I suspected when I saw this and a couple other
tracebacks)= .
=C2=A0
Regarding the assert in the USB stack, maybe the state was not correctly set on the device_t ?

It's unclear = to me. Newbus doesn't guarantee certain states to the bus drivers, so
maybe the assert in the USB stack is incorrectly strict on what st= ates
it assumes the device is in? I'm unsure. I haven't l= ooked deeply enough to know
what exactly is going on. Since there= were problems and I didn't have time to do
the proper deep d= ive, I just reverted for now and will revisit when I have the time.

Warner
--00000000000064aa5a05ef2b0876--