Re: git: c5312bd79e66 - main - cam: Move bus_dmamap_load_ccb into cam.c.

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,
>>
>