Date: Tue, 18 Jul 2023 20:42:36 -0600 From: Warner Losh <imp@bsdimp.com> To: John Baldwin <jhb@freebsd.org> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: c5312bd79e66 - main - cam: Move bus_dmamap_load_ccb into cam.c. Message-ID: <CANCZdfpJuh4TE0z0wBmu6ReOnCFV%2BBkcSs7of=VLGUBXhPHMfA@mail.gmail.com> In-Reply-To: <CANCZdfotvSACUJshOLDR-qd97z1BTwgeYqtYrFYW6-_wSzL1CQ@mail.gmail.com> References: <202307190120.36J1K1mQ011397@gitrepo.freebsd.org> <CANCZdfotvSACUJshOLDR-qd97z1BTwgeYqtYrFYW6-_wSzL1CQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <imp@bsdimp.com> 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 <jhb@freebsd.org> 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 <jhb@FreeBSD.org> >> AuthorDate: 2023-07-19 01:19:27 +0000 >> Commit: John Baldwin <jhb@FreeBSD.org> >> 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 <sys/libkern.h> >> +#include <machine/bus.h> >> #include <cam/cam_queue.h> >> #include <cam/cam_xpt.h> >> >> @@ -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 <div dir=3D"ltr"><div dir=3D"ltr"><div>Sorry for the top post. Two ways for= ward that I see:</div><div><br></div><div>Two proposed fixes<br><br><a href= =3D"https://reviews.freebsd.org/D41077">https://reviews.freebsd.org/D41077<= /a> which makes loading the dma for a nvme request a function pointer (and = keeps all ccb knowledge in nvme_sim.c)<br><a href=3D"https://reviews.freebs= d.org/D41078">https://reviews.freebsd.org/D41078</a>; which moves renames me= mdesc_ccb to cam_memdesc_ccb and moves it to cam_cch.h</div><div><br></div>= <div>'78 is ready to commit as is. I think it's fine, but don't= like the dependency graph.</div><div>'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.</div><div><br></div><div>Warner</div><div><br></div><div>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.<br></div><div><br></= div></div><br><div class=3D"gmail_quote"><div dir=3D"ltr" class=3D"gmail_at= tr">On Tue, Jul 18, 2023 at 7:44=E2=80=AFPM Warner Losh <<a href=3D"mail= to:imp@bsdimp.com">imp@bsdimp.com</a>> wrote:<br></div><blockquote class= =3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rg= b(204,204,204);padding-left:1ex"><div dir=3D"ltr"><div dir=3D"ltr"><div>As = predicted in the review, this is broken:</div><div><br></div><div>-- comman= d output --<br>linking kernel.full<br>ld: error: undefined symbol: bus_dmam= ap_load_ccb<br>>>> referenced by nvme_qpair.c:1209 (/usr/home/imp/= git/freebsd/src/sys/dev/nvme/nvme_qpair.c:1209)<br>>>> =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 nvme_qpair.o:(_nvme_qpair_submit_req= uest)<br>_</div><div>from using sys/amd64/conf/EX <br>include MINIMAL<br>de= vice nvme<br>device nvd<br></div></div><br><div class=3D"gmail_quote"><div = class=3D"gmail_attr">This has to be in the header file. The MODULE_DEPENDS = stuff doesn't pull anything in for the</div><div class=3D"gmail_attr">s= tatic kernel case.</div><div class=3D"gmail_attr"><br></div><div class=3D"g= mail_attr">Please, lets' do this in the header with a static inline lik= e I suggested to get around this issue.</div><div class=3D"gmail_attr"><br>= </div><div class=3D"gmail_attr">Warner<br></div><div dir=3D"ltr" class=3D"g= mail_attr"><br></div><div dir=3D"ltr" class=3D"gmail_attr">On Tue, Jul 18, = 2023 at 7:20=E2=80=AFPM John Baldwin <<a href=3D"mailto:jhb@freebsd.org"= target=3D"_blank">jhb@freebsd.org</a>> wrote:<br></div><blockquote clas= s=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid r= gb(204,204,204);padding-left:1ex">The branch main has been updated by jhb:<= br> <br> URL: <a href=3D"https://cgit.FreeBSD.org/src/commit/?id=3Dc5312bd79e66ce8ef= 50655ce7f3eca06d6b6e24f" rel=3D"noreferrer" target=3D"_blank">https://cgit.= FreeBSD.org/src/commit/?id=3Dc5312bd79e66ce8ef50655ce7f3eca06d6b6e24f</a><b= r> <br> commit c5312bd79e66ce8ef50655ce7f3eca06d6b6e24f<br> Author:=C2=A0 =C2=A0 =C2=A0John Baldwin <jhb@FreeBSD.org><br> AuthorDate: 2023-07-19 01:19:27 +0000<br> Commit:=C2=A0 =C2=A0 =C2=A0John Baldwin <jhb@FreeBSD.org><br> CommitDate: 2023-07-19 01:19:27 +0000<br> <br> =C2=A0 =C2=A0 cam: Move bus_dmamap_load_ccb into cam.c.<br> <br> =C2=A0 =C2=A0 This routine is specific to CAM and no longer assumes any int= ernal<br> =C2=A0 =C2=A0 bus_dma knowledge as it is simple wrapper around bus_dmamap_l= oad_mem.<br> <br> =C2=A0 =C2=A0 Fixes:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 60381fd1ee86 memdesc= : Retire MEMDESC_CCB.<br> =C2=A0 =C2=A0 Reviewed by:=C2=A0 =C2=A0 kib<br> =C2=A0 =C2=A0 Differential Revision:=C2=A0 <a href=3D"https://reviews.freeb= sd.org/D41058" rel=3D"noreferrer" target=3D"_blank">https://reviews.freebsd= .org/D41058</a><br> ---<br> =C2=A0sys/cam/cam.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 20 ++++++++++= ++++++++++<br> =C2=A0sys/kern/subr_bus_dma.c | 19 -------------------<br> =C2=A02 files changed, 20 insertions(+), 19 deletions(-)<br> <br> diff --git a/sys/cam/cam.c b/sys/cam/cam.c<br> index ce7dc81b3495..7d9d8602d009 100644<br> --- a/sys/cam/cam.c<br> +++ b/sys/cam/cam.c<br> @@ -52,6 +52,7 @@ __FBSDID("$FreeBSD$");<br> <br> =C2=A0#ifdef _KERNEL<br> =C2=A0#include <sys/libkern.h><br> +#include <machine/bus.h><br> =C2=A0#include <cam/cam_queue.h><br> =C2=A0#include <cam/cam_xpt.h><br> <br> @@ -642,4 +643,23 @@ memdesc_ccb(union ccb *ccb)<br> =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);<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 }<br> =C2=A0}<br> +<br> +int<br> +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,<br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0int f= lags)<br> +{<br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0struct ccb_hdr *ccb_h;<br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0struct memdesc mem;<br> +<br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0ccb_h =3D &ccb->ccb_h;<br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0if ((ccb_h->flags & CAM_DIR_MASK) =3D=3D= CAM_DIR_NONE) {<br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0callback(callback_a= rg, NULL, 0, 0);<br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return (0);<br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0}<br> +<br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0mem =3D memdesc_ccb(ccb);<br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0return (bus_dmamap_load_mem(dmat, map, &mem= , callback, callback_arg,<br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0flags));<br> +}<br> =C2=A0#endif<br> diff --git a/sys/kern/subr_bus_dma.c b/sys/kern/subr_bus_dma.c<br> index da7a2ee4cdc9..683b41d0047c 100644<br> --- a/sys/kern/subr_bus_dma.c<br> +++ b/sys/kern/subr_bus_dma.c<br> @@ -449,25 +449,6 @@ bus_dmamap_load_uio(bus_dma_tag_t dmat, bus_dmamap_t m= ap, struct uio *uio,<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 return (error);<br> =C2=A0}<br> <br> -int<br> -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,<br> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0int f= lags)<br> -{<br> -=C2=A0 =C2=A0 =C2=A0 =C2=A0struct ccb_hdr *ccb_h;<br> -=C2=A0 =C2=A0 =C2=A0 =C2=A0struct memdesc mem;<br> -<br> -=C2=A0 =C2=A0 =C2=A0 =C2=A0ccb_h =3D &ccb->ccb_h;<br> -=C2=A0 =C2=A0 =C2=A0 =C2=A0if ((ccb_h->flags & CAM_DIR_MASK) =3D=3D= CAM_DIR_NONE) {<br> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0callback(callback_a= rg, NULL, 0, 0);<br> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return (0);<br> -=C2=A0 =C2=A0 =C2=A0 =C2=A0}<br> -<br> -=C2=A0 =C2=A0 =C2=A0 =C2=A0mem =3D memdesc_ccb(ccb);<br> -=C2=A0 =C2=A0 =C2=A0 =C2=A0return (bus_dmamap_load_mem(dmat, map, &mem= , callback, callback_arg,<br> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0flags));<br> -}<br> -<br> =C2=A0int<br> =C2=A0bus_dmamap_load_bio(bus_dma_tag_t dmat, bus_dmamap_t map, struct bio = *bio,<br> =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,<br> </blockquote></div></div> </blockquote></div></div> --0000000000009cb26d0600cdfc44--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfpJuh4TE0z0wBmu6ReOnCFV%2BBkcSs7of=VLGUBXhPHMfA>