From nobody Wed Jul 19 02:42:36 2023 X-Original-To: dev-commits-src-main@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 4R5KqZ4Y8Tz4nsmW for ; Wed, 19 Jul 2023 02:42:50 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-lf1-x130.google.com (mail-lf1-x130.google.com [IPv6:2a00:1450:4864:20::130]) (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 4R5KqY15Cpz3Nfb for ; Wed, 19 Jul 2023 02:42:49 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; dkim=pass header.d=bsdimp-com.20221208.gappssmtp.com header.s=20221208 header.b=kNyltHFh; spf=none (mx1.freebsd.org: domain of wlosh@bsdimp.com has no SPF policy when checking 2a00:1450:4864:20::130) smtp.mailfrom=wlosh@bsdimp.com; dmarc=none Received: by mail-lf1-x130.google.com with SMTP id 2adb3069b0e04-4fdd14c1fbfso2008045e87.1 for ; Tue, 18 Jul 2023 19:42:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20221208.gappssmtp.com; s=20221208; t=1689734567; x=1692326567; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=Yzk2Nf2nbtsTTGPYD5nqapnJVziAE/3S0vvkhWXh0yI=; b=kNyltHFhg5ceYHnLHK96gSMEZ2aqA1dkG41TMP8Wsjr56Psv5KEqLOyGt7OaS3xWeE IOfMmCRnEX6uzi4Y5Vo1bw2F3IUHgX6G/Ca7oqllu2PGJ5sSLlt3OmMp1kw5PBRzvTJp +I0bNFx3p1gUGnLLSf3R5cKmtNzLg0UDGLmhbqEw5jLNMFySHL0lWNcYoOWj2Eh0MCbw sbSYz1ox8q9Dx31XmJw00WYZ9Bb4jqxP3nwsbJAtqg+8I6H6DA6UyoKikGgLKzTwSzCS xekdnibnYDIVukX/FnxioCEBdwTbXTInLg2MU6lJ7rJqMP05TfUzx+it3k9YuwaA/2Es u2jA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689734567; x=1692326567; 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=Yzk2Nf2nbtsTTGPYD5nqapnJVziAE/3S0vvkhWXh0yI=; b=cQS0aWCZi211vgvFHDnvuTGFb/yLnfNi4+VHzFlwXYHbSJyFjSnBtfinYtxaey4wF+ i739gkMpQf3QA88sDJPwPVr0h3TGvgxeogIxlzSiPh3KykvK+E86PkJxjeLwEaZew9dZ sXbv2qgrWd/+Q51hC/z4MXuPWM8e3aoJ6rERiMXLeQkVD03NDJweJxObX4bQm2qiCqQ+ l6Fzp4FzPPvvA0hdBKLuw4IUfTuvmtXITe0uK2EobakKOFtYZpdcLe7Ci1TyyBbBK+bI OPgx0KsSnf1JMfNAsnnoXwz41RO77qDk8eT3/lmhRNpkZ6pvghy5kJLWQzOPSBqudigD OvVw== X-Gm-Message-State: ABy/qLaylmQR8NCVwgplb8CJwME+jgAWN2Q3rHrNXI18ve5xeHRcyWUa AnmaUJCDZUOyEUQ4Uj94oNwez1jQ4BWdOX91n15modXrhhxwLXJBFPE9Vg== X-Google-Smtp-Source: APBJJlFEH4hZFrllTV42JkeOrSwdVJqNPvCLEz/mtGX3S5doeJrZL1lr4Mps7TO048SByd0Q1LwL3h5B7Q74RRBI1jc= X-Received: by 2002:a19:5f11:0:b0:4fd:c771:ed76 with SMTP id t17-20020a195f11000000b004fdc771ed76mr734519lfb.38.1689734566679; Tue, 18 Jul 2023 19:42:46 -0700 (PDT) List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 References: <202307190120.36J1K1mQ011397@gitrepo.freebsd.org> In-Reply-To: From: Warner Losh Date: Tue, 18 Jul 2023 20:42:36 -0600 Message-ID: Subject: Re: git: c5312bd79e66 - main - cam: Move bus_dmamap_load_ccb into cam.c. To: John Baldwin Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: multipart/alternative; boundary="0000000000009cb26d0600cdfc44" X-Spamd-Result: default: False [-2.96 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_SHORT(-0.96)[-0.964]; FORGED_SENDER(0.30)[imp@bsdimp.com,wlosh@bsdimp.com]; R_DKIM_ALLOW(-0.20)[bsdimp-com.20221208.gappssmtp.com:s=20221208]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; R_SPF_NA(0.00)[no SPF record]; MLMMJ_DEST(0.00)[dev-commits-src-main@freebsd.org]; ARC_NA(0.00)[]; RCVD_TLS_LAST(0.00)[]; MIME_TRACE(0.00)[0:+,1:+,2:~]; RCVD_COUNT_TWO(0.00)[2]; RCVD_IN_DNSWL_NONE(0.00)[2a00:1450:4864:20::130:from]; DKIM_TRACE(0.00)[bsdimp-com.20221208.gappssmtp.com:+]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US]; RCPT_COUNT_THREE(0.00)[4]; FROM_HAS_DN(0.00)[]; BLOCKLISTDE_FAIL(0.00)[2a00:1450:4864:20::130:server fail]; TO_DN_SOME(0.00)[]; DMARC_NA(0.00)[bsdimp.com]; PREVIOUSLY_DELIVERED(0.00)[dev-commits-src-main@freebsd.org]; TO_MATCH_ENVRCPT_SOME(0.00)[]; FROM_NEQ_ENVFROM(0.00)[imp@bsdimp.com,wlosh@bsdimp.com] X-Rspamd-Queue-Id: 4R5KqY15Cpz3Nfb X-Spamd-Bar: -- --0000000000009cb26d0600cdfc44 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Sorry for the top post. Two ways forward that I see: Two proposed fixes https://reviews.freebsd.org/D41077 which makes loading the dma for a nvme request a function pointer (and keeps all ccb knowledge in nvme_sim.c) https://reviews.freebsd.org/D41078 which moves renames memdesc_ccb to cam_memdesc_ccb and moves it to cam_cch.h '78 is ready to commit as is. I think it's fine, but don't like the dependency graph. '77 likely needs some polish before it's pushed in, especially with naming. I like its dependency graph, but don't like the extra call through a function pointer for all nvme requests. Warner P.S. Sorry if I sounded too grumpy in other replies... it's been a frustrating day in $REAL_LIFE and I let that infect those replies. On Tue, Jul 18, 2023 at 7:44=E2=80=AFPM Warner Losh wrote: > As predicted in the review, this is broken: > > -- command output -- > linking kernel.full > ld: error: undefined symbol: bus_dmamap_load_ccb > >>> referenced by nvme_qpair.c:1209 > (/usr/home/imp/git/freebsd/src/sys/dev/nvme/nvme_qpair.c:1209) > >>> nvme_qpair.o:(_nvme_qpair_submit_request) > _ > from using sys/amd64/conf/EX > include MINIMAL > device nvme > device nvd > > This has to be in the header file. The MODULE_DEPENDS stuff doesn't pull > anything in for the > static kernel case. > > Please, lets' do this in the header with a static inline like I suggested > to get around this issue. > > Warner > > On Tue, Jul 18, 2023 at 7:20=E2=80=AFPM John Baldwin wr= ote: > >> The branch main has been updated by jhb: >> >> URL: >> https://cgit.FreeBSD.org/src/commit/?id=3Dc5312bd79e66ce8ef50655ce7f3eca= 06d6b6e24f >> >> commit c5312bd79e66ce8ef50655ce7f3eca06d6b6e24f >> Author: John Baldwin >> AuthorDate: 2023-07-19 01:19:27 +0000 >> Commit: John Baldwin >> CommitDate: 2023-07-19 01:19:27 +0000 >> >> cam: Move bus_dmamap_load_ccb into cam.c. >> >> This routine is specific to CAM and no longer assumes any internal >> bus_dma knowledge as it is simple wrapper around bus_dmamap_load_mem= . >> >> Fixes: 60381fd1ee86 memdesc: Retire MEMDESC_CCB. >> Reviewed by: kib >> Differential Revision: https://reviews.freebsd.org/D41058 >> --- >> sys/cam/cam.c | 20 ++++++++++++++++++++ >> sys/kern/subr_bus_dma.c | 19 ------------------- >> 2 files changed, 20 insertions(+), 19 deletions(-) >> >> diff --git a/sys/cam/cam.c b/sys/cam/cam.c >> index ce7dc81b3495..7d9d8602d009 100644 >> --- a/sys/cam/cam.c >> +++ b/sys/cam/cam.c >> @@ -52,6 +52,7 @@ __FBSDID("$FreeBSD$"); >> >> #ifdef _KERNEL >> #include >> +#include >> #include >> #include >> >> @@ -642,4 +643,23 @@ memdesc_ccb(union ccb *ccb) >> panic("%s: flags 0x%X unimplemented", __func__, >> ccb_h->flags); >> } >> } >> + >> +int >> +bus_dmamap_load_ccb(bus_dma_tag_t dmat, bus_dmamap_t map, union ccb *cc= b, >> + bus_dmamap_callback_t *callback, void *callback_arg, >> + int flags) >> +{ >> + struct ccb_hdr *ccb_h; >> + struct memdesc mem; >> + >> + ccb_h =3D &ccb->ccb_h; >> + if ((ccb_h->flags & CAM_DIR_MASK) =3D=3D CAM_DIR_NONE) { >> + callback(callback_arg, NULL, 0, 0); >> + return (0); >> + } >> + >> + mem =3D memdesc_ccb(ccb); >> + return (bus_dmamap_load_mem(dmat, map, &mem, callback, >> callback_arg, >> + flags)); >> +} >> #endif >> diff --git a/sys/kern/subr_bus_dma.c b/sys/kern/subr_bus_dma.c >> index da7a2ee4cdc9..683b41d0047c 100644 >> --- a/sys/kern/subr_bus_dma.c >> +++ b/sys/kern/subr_bus_dma.c >> @@ -449,25 +449,6 @@ bus_dmamap_load_uio(bus_dma_tag_t dmat, bus_dmamap_= t >> map, struct uio *uio, >> return (error); >> } >> >> -int >> -bus_dmamap_load_ccb(bus_dma_tag_t dmat, bus_dmamap_t map, union ccb *cc= b, >> - bus_dmamap_callback_t *callback, void *callback_arg, >> - int flags) >> -{ >> - struct ccb_hdr *ccb_h; >> - struct memdesc mem; >> - >> - ccb_h =3D &ccb->ccb_h; >> - if ((ccb_h->flags & CAM_DIR_MASK) =3D=3D CAM_DIR_NONE) { >> - callback(callback_arg, NULL, 0, 0); >> - return (0); >> - } >> - >> - mem =3D memdesc_ccb(ccb); >> - return (bus_dmamap_load_mem(dmat, map, &mem, callback, >> callback_arg, >> - flags)); >> -} >> - >> int >> bus_dmamap_load_bio(bus_dma_tag_t dmat, bus_dmamap_t map, struct bio >> *bio, >> bus_dmamap_callback_t *callback, void *callback_arg, >> > --0000000000009cb26d0600cdfc44 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Sorry for the top post. Two ways for= ward that I see:


=
'78 is ready to commit as is. I think it's fine, but don't= like the dependency graph.
'77 likely needs some polish befo= re it's pushed in, especially with naming. I like its dependency graph,= but don't like the extra call through a function pointer for all nvme = requests.

Warner

P.S. Sor= ry if I sounded too grumpy in other replies... it's been a frustrating = day in $REAL_LIFE and I let that infect those replies.


On Tue, Jul 18, 2023 at 7:44=E2=80=AFPM Warner Losh <imp@bsdimp.com> wrote:
As = predicted in the review, this is broken:

-- comman= d output --
linking kernel.full
ld: error: undefined symbol: bus_dmam= ap_load_ccb
>>> referenced by nvme_qpair.c:1209 (/usr/home/imp/= git/freebsd/src/sys/dev/nvme/nvme_qpair.c:1209)
>>> =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 nvme_qpair.o:(_nvme_qpair_submit_req= uest)
_
from using sys/amd64/conf/EX
include MINIMAL
de= vice nvme
device nvd

This has to be in the header file. The MODULE_DEPENDS = stuff doesn't pull anything in for the
s= tatic kernel case.

Please, lets' do this in the header with a static inline lik= e I suggested to get around this issue.

=
Warner

On Tue, Jul 18, = 2023 at 7:20=E2=80=AFPM John Baldwin <jhb@freebsd.org> wrote:
The branch main has been updated by jhb:<= br>
URL: https://cgit.= FreeBSD.org/src/commit/?id=3Dc5312bd79e66ce8ef50655ce7f3eca06d6b6e24f
commit c5312bd79e66ce8ef50655ce7f3eca06d6b6e24f
Author:=C2=A0 =C2=A0 =C2=A0John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2023-07-19 01:19:27 +0000
Commit:=C2=A0 =C2=A0 =C2=A0John Baldwin <jhb@FreeBSD.org>
CommitDate: 2023-07-19 01:19:27 +0000

=C2=A0 =C2=A0 cam: Move bus_dmamap_load_ccb into cam.c.

=C2=A0 =C2=A0 This routine is specific to CAM and no longer assumes any int= ernal
=C2=A0 =C2=A0 bus_dma knowledge as it is simple wrapper around bus_dmamap_l= oad_mem.

=C2=A0 =C2=A0 Fixes:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 60381fd1ee86 memdesc= : Retire MEMDESC_CCB.
=C2=A0 =C2=A0 Reviewed by:=C2=A0 =C2=A0 kib
=C2=A0 =C2=A0 Differential Revision:=C2=A0 https://reviews.freebsd= .org/D41058
---
=C2=A0sys/cam/cam.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 20 ++++++++++= ++++++++++
=C2=A0sys/kern/subr_bus_dma.c | 19 -------------------
=C2=A02 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/sys/cam/cam.c b/sys/cam/cam.c
index ce7dc81b3495..7d9d8602d009 100644
--- a/sys/cam/cam.c
+++ b/sys/cam/cam.c
@@ -52,6 +52,7 @@ __FBSDID("$FreeBSD$");

=C2=A0#ifdef _KERNEL
=C2=A0#include <sys/libkern.h>
+#include <machine/bus.h>
=C2=A0#include <cam/cam_queue.h>
=C2=A0#include <cam/cam_xpt.h>

@@ -642,4 +643,23 @@ memdesc_ccb(union ccb *ccb)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 panic("%s: fla= gs 0x%X unimplemented", __func__, ccb_h->flags);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
=C2=A0}
+
+int
+bus_dmamap_load_ccb(bus_dma_tag_t dmat, bus_dmamap_t map, union ccb *ccb,<= br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0bus_d= mamap_callback_t *callback, void *callback_arg,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0int f= lags)
+{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct ccb_hdr *ccb_h;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct memdesc mem;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0ccb_h =3D &ccb->ccb_h;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if ((ccb_h->flags & CAM_DIR_MASK) =3D=3D= CAM_DIR_NONE) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0callback(callback_a= rg, NULL, 0, 0);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return (0);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0}
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0mem =3D memdesc_ccb(ccb);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return (bus_dmamap_load_mem(dmat, map, &mem= , callback, callback_arg,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0flags));
+}
=C2=A0#endif
diff --git a/sys/kern/subr_bus_dma.c b/sys/kern/subr_bus_dma.c
index da7a2ee4cdc9..683b41d0047c 100644
--- a/sys/kern/subr_bus_dma.c
+++ b/sys/kern/subr_bus_dma.c
@@ -449,25 +449,6 @@ bus_dmamap_load_uio(bus_dma_tag_t dmat, bus_dmamap_t m= ap, struct uio *uio,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 return (error);
=C2=A0}

-int
-bus_dmamap_load_ccb(bus_dma_tag_t dmat, bus_dmamap_t map, union ccb *ccb,<= br> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0bus_d= mamap_callback_t *callback, void *callback_arg,
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0int f= lags)
-{
-=C2=A0 =C2=A0 =C2=A0 =C2=A0struct ccb_hdr *ccb_h;
-=C2=A0 =C2=A0 =C2=A0 =C2=A0struct memdesc mem;
-
-=C2=A0 =C2=A0 =C2=A0 =C2=A0ccb_h =3D &ccb->ccb_h;
-=C2=A0 =C2=A0 =C2=A0 =C2=A0if ((ccb_h->flags & CAM_DIR_MASK) =3D=3D= CAM_DIR_NONE) {
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0callback(callback_a= rg, NULL, 0, 0);
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return (0);
-=C2=A0 =C2=A0 =C2=A0 =C2=A0}
-
-=C2=A0 =C2=A0 =C2=A0 =C2=A0mem =3D memdesc_ccb(ccb);
-=C2=A0 =C2=A0 =C2=A0 =C2=A0return (bus_dmamap_load_mem(dmat, map, &mem= , callback, callback_arg,
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0flags));
-}
-
=C2=A0int
=C2=A0bus_dmamap_load_bio(bus_dma_tag_t dmat, bus_dmamap_t map, struct bio = *bio,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 bus_d= mamap_callback_t *callback, void *callback_arg,
--0000000000009cb26d0600cdfc44--