Re: git: 60381fd1ee86 - main - memdesc: Retire MEMDESC_CCB.

On Mon, Jul 17, 2023 at 11:45 AM John Baldwin <jhb@freebsd.org> wrote:

> On 7/17/23 10:29 AM, Warner Losh wrote:
> > On Mon, Jul 17, 2023 at 11:15 AM John Baldwin <jhb@freebsd.org> wrote:
> >
> >> On 7/17/23 8:48 AM, Konstantin Belousov wrote:
> >>> On Fri, Jul 14, 2023 at 06:41:03PM +0000, John Baldwin wrote:
> >>>> The branch main has been updated by jhb:
> >>>>
> >>>> URL:
> >>
> https://cgit.FreeBSD.org/src/commit/?id=60381fd1ee8668ea1e4676a6128883d987cab858
> >>>>
> >>>> commit 60381fd1ee8668ea1e4676a6128883d987cab858
> >>>> Author:     John Baldwin <jhb@FreeBSD.org>
> >>>> AuthorDate: 2023-07-14 18:30:31 +0000
> >>>> Commit:     John Baldwin <jhb@FreeBSD.org>
> >>>> CommitDate: 2023-07-14 18:32:16 +0000
> >>>>
> >>>>       memdesc: Retire MEMDESC_CCB.
> >>>>
> >>>>       Instead, change memdesc_ccb to examine the CCB and return a
> >> memdesc of
> >>>>       a more generic type describing the data buffer.
> >>>
> >>>> diff --git a/sys/kern/subr_bus_dma.c b/sys/kern/subr_bus_dma.c
> >>>> index 65a08aeba17c..bfaad30b37d3 100644
> >>>> --- a/sys/kern/subr_bus_dma.c
> >>>> +++ b/sys/kern/subr_bus_dma.c
> >>>> @@ -304,94 +304,6 @@ bus_dmamap_load_ma_triv(bus_dma_tag_t dmat,
> >> bus_dmamap_t map,
> >>>> @@ -566,49 +478,18 @@ bus_dmamap_load_ccb(bus_dma_tag_t dmat,
> >> bus_dmamap_t map, union ccb *ccb,
> >>>> +    mem = memdesc_ccb(ccb);
> >>>> +    return (bus_dmamap_load_mem(dmat, map, &mem, callback,
> >> callback_arg,
> >>>> +        flags));
> >>>>    }
> >>> This makes kernel not linkable if CAM is not included into it.
> >>
> >> Hmmm, ok.  I can either move the memdesc_ccb routine into sys/kern
> >> somewhere
> >> (like the kern_memdesc.c file in my other pending review), or we can
> #ifdef
> >> this function.  It probably doesn't make sense to have a
> >> bus_dmamap_load_ccb
> >> if you don't have CAM, so I think I prefer the second option.
> >>
> >
> > MINIMAL doesn't have CAM configured, but it is loadable as a module.
> >
> > I'd think we'd want a dummy one fo these with weak symbol binding and
> have
> > the actual
> > one live in cam somewhere that overrides this  symbol.
> >
> > I just hit this in building MINIMAL for other reasons....
>
> Yeah, I was testing MINIMAL locally (which is still broken due to recent
> TCP commits) to try out possible fixes.  One could possibly move
> bus_dmamap_load_ccb into cam.ko entirely?  Especially this current version
> doesn't really have any bus_dma-specific knowledge anymore but is just a
> thin wrapper around bus_dmamap_load_mem:
>
> 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));
> }
>

And who calls bus_dmamap_load_ccb? If this were entirely in cam.ko, then
callers of it would
need to resolve it. sys/dev/nvme/nvme_qpair.c is not otherwise dependent on
cam, and would
be unresolved for a minimal + nvme + nvd kernel.

Warner