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
[-- Attachment #1 --] 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 PM 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 PM John Baldwin <jhb@freebsd.org> wrote: > >> The branch main has been updated by jhb: >> >> URL: >> https://cgit.FreeBSD.org/src/commit/?id=c5312bd79e66ce8ef50655ce7f3eca06d6b6e24f >> >> 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 *ccb, >> + bus_dmamap_callback_t *callback, void *callback_arg, >> + int flags) >> +{ >> + struct ccb_hdr *ccb_h; >> + struct memdesc mem; >> + >> + ccb_h = &ccb->ccb_h; >> + if ((ccb_h->flags & CAM_DIR_MASK) == CAM_DIR_NONE) { >> + callback(callback_arg, NULL, 0, 0); >> + return (0); >> + } >> + >> + mem = 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 *ccb, >> - bus_dmamap_callback_t *callback, void *callback_arg, >> - int flags) >> -{ >> - struct ccb_hdr *ccb_h; >> - struct memdesc mem; >> - >> - ccb_h = &ccb->ccb_h; >> - if ((ccb_h->flags & CAM_DIR_MASK) == CAM_DIR_NONE) { >> - callback(callback_arg, NULL, 0, 0); >> - return (0); >> - } >> - >> - mem = 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, >> > [-- Attachment #2 --] <div dir="ltr"><div dir="ltr"><div>Sorry for the top post. Two ways forward that I see:</div><div><br></div><div>Two proposed fixes<br><br><a href="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="https://reviews.freebsd.org/D41078">https://reviews.freebsd.org/D41078</a>; which moves renames memdesc_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 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.</div><div><br></div><div>Warner</div><div><br></div><div>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.<br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jul 18, 2023 at 7:44 PM Warner Losh <<a href="mailto:imp@bsdimp.com">imp@bsdimp.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div>As predicted in the review, this is broken:</div><div><br></div><div>-- command output --<br>linking kernel.full<br>ld: error: undefined symbol: bus_dmamap_load_ccb<br>>>> referenced by nvme_qpair.c:1209 (/usr/home/imp/git/freebsd/src/sys/dev/nvme/nvme_qpair.c:1209)<br>>>> nvme_qpair.o:(_nvme_qpair_submit_request)<br>_</div><div>from using sys/amd64/conf/EX <br>include MINIMAL<br>device nvme<br>device nvd<br></div></div><br><div class="gmail_quote"><div class="gmail_attr">This has to be in the header file. The MODULE_DEPENDS stuff doesn't pull anything in for the</div><div class="gmail_attr">static kernel case.</div><div class="gmail_attr"><br></div><div class="gmail_attr">Please, lets' do this in the header with a static inline like I suggested to get around this issue.</div><div class="gmail_attr"><br></div><div class="gmail_attr">Warner<br></div><div dir="ltr" class="gmail_attr"><br></div><div dir="ltr" class="gmail_attr">On Tue, Jul 18, 2023 at 7:20 PM John Baldwin <<a href="mailto:jhb@freebsd.org" target="_blank">jhb@freebsd.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The branch main has been updated by jhb:<br> <br> URL: <a href="https://cgit.FreeBSD.org/src/commit/?id=c5312bd79e66ce8ef50655ce7f3eca06d6b6e24f" rel="noreferrer" target="_blank">https://cgit.FreeBSD.org/src/commit/?id=c5312bd79e66ce8ef50655ce7f3eca06d6b6e24f</a><br>; <br> commit c5312bd79e66ce8ef50655ce7f3eca06d6b6e24f<br> Author: John Baldwin <jhb@FreeBSD.org><br> AuthorDate: 2023-07-19 01:19:27 +0000<br> Commit: John Baldwin <jhb@FreeBSD.org><br> CommitDate: 2023-07-19 01:19:27 +0000<br> <br> cam: Move bus_dmamap_load_ccb into cam.c.<br> <br> This routine is specific to CAM and no longer assumes any internal<br> bus_dma knowledge as it is simple wrapper around bus_dmamap_load_mem.<br> <br> Fixes: 60381fd1ee86 memdesc: Retire MEMDESC_CCB.<br> Reviewed by: kib<br> Differential Revision: <a href="https://reviews.freebsd.org/D41058" rel="noreferrer" target="_blank">https://reviews.freebsd.org/D41058</a><br>; ---<br> sys/cam/cam.c | 20 ++++++++++++++++++++<br> sys/kern/subr_bus_dma.c | 19 -------------------<br> 2 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> #ifdef _KERNEL<br> #include <sys/libkern.h><br> +#include <machine/bus.h><br> #include <cam/cam_queue.h><br> #include <cam/cam_xpt.h><br> <br> @@ -642,4 +643,23 @@ memdesc_ccb(union ccb *ccb)<br> panic("%s: flags 0x%X unimplemented", __func__, ccb_h->flags);<br> }<br> }<br> +<br> +int<br> +bus_dmamap_load_ccb(bus_dma_tag_t dmat, bus_dmamap_t map, union ccb *ccb,<br> + bus_dmamap_callback_t *callback, void *callback_arg,<br> + int flags)<br> +{<br> + struct ccb_hdr *ccb_h;<br> + struct memdesc mem;<br> +<br> + ccb_h = &ccb->ccb_h;<br> + if ((ccb_h->flags & CAM_DIR_MASK) == CAM_DIR_NONE) {<br> + callback(callback_arg, NULL, 0, 0);<br> + return (0);<br> + }<br> +<br> + mem = memdesc_ccb(ccb);<br> + return (bus_dmamap_load_mem(dmat, map, &mem, callback, callback_arg,<br> + flags));<br> +}<br> #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 map, struct uio *uio,<br> return (error);<br> }<br> <br> -int<br> -bus_dmamap_load_ccb(bus_dma_tag_t dmat, bus_dmamap_t map, union ccb *ccb,<br> - bus_dmamap_callback_t *callback, void *callback_arg,<br> - int flags)<br> -{<br> - struct ccb_hdr *ccb_h;<br> - struct memdesc mem;<br> -<br> - ccb_h = &ccb->ccb_h;<br> - if ((ccb_h->flags & CAM_DIR_MASK) == CAM_DIR_NONE) {<br> - callback(callback_arg, NULL, 0, 0);<br> - return (0);<br> - }<br> -<br> - mem = memdesc_ccb(ccb);<br> - return (bus_dmamap_load_mem(dmat, map, &mem, callback, callback_arg,<br> - flags));<br> -}<br> -<br> int<br> bus_dmamap_load_bio(bus_dma_tag_t dmat, bus_dmamap_t map, struct bio *bio,<br> bus_dmamap_callback_t *callback, void *callback_arg,<br> </blockquote></div></div> </blockquote></div></div>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfpJuh4TE0z0wBmu6ReOnCFV%2BBkcSs7of=VLGUBXhPHMfA>