Date: Mon, 17 Jul 2023 11:52:07 -0600 From: Warner Losh <imp@bsdimp.com> To: John Baldwin <jhb@freebsd.org> Cc: Konstantin Belousov <kostikbel@gmail.com>, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 60381fd1ee86 - main - memdesc: Retire MEMDESC_CCB. Message-ID: <CANCZdfrr3pPhkbueJyHLYA0FhkKXQefSNx=QLOUjxNPh4FR1Aw@mail.gmail.com> In-Reply-To: <17cd00bb-c6f2-5afa-a1ac-b7e14c3c758e@FreeBSD.org> References: <202307141841.36EIf3f0019403@gitrepo.freebsd.org> <ZLVizUl1Xyo1AQmy@kib.kiev.ua> <65d7d8d8-9f98-abd2-1ce3-ae3a2d3bf111@FreeBSD.org> <CANCZdfoHid=H1Ys_4XTJPfCaifevSoW92nGYXU3Ot=mTT13T%2Bg@mail.gmail.com> <17cd00bb-c6f2-5afa-a1ac-b7e14c3c758e@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--000000000000aefcf40600b2750f Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Jul 17, 2023 at 11:45=E2=80=AFAM John Baldwin <jhb@freebsd.org> wro= te: > On 7/17/23 10:29 AM, Warner Losh wrote: > > On Mon, Jul 17, 2023 at 11:15=E2=80=AFAM 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=3D60381fd1ee8668ea1e4676a6128883d= 987cab858 > >>>> > >>>> 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 =3D 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 versio= n > 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 =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)); > } > 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 --000000000000aefcf40600b2750f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">= <div dir=3D"ltr" class=3D"gmail_attr">On Mon, Jul 17, 2023 at 11:45=E2=80= =AFAM John Baldwin <<a href=3D"mailto:jhb@freebsd.org">jhb@freebsd.org</= a>> wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0p= x 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On= 7/17/23 10:29 AM, Warner Losh wrote:<br> > On Mon, Jul 17, 2023 at 11:15=E2=80=AFAM John Baldwin <<a href=3D"m= ailto:jhb@freebsd.org" target=3D"_blank">jhb@freebsd.org</a>> wrote:<br> > <br> >> On 7/17/23 8:48 AM, Konstantin Belousov wrote:<br> >>> On Fri, Jul 14, 2023 at 06:41:03PM +0000, John Baldwin wrote:<= br> >>>> The branch main has been updated by jhb:<br> >>>><br> >>>> URL:<br> >> <a href=3D"https://cgit.FreeBSD.org/src/commit/?id=3D60381fd1ee866= 8ea1e4676a6128883d987cab858" rel=3D"noreferrer" target=3D"_blank">https://c= git.FreeBSD.org/src/commit/?id=3D60381fd1ee8668ea1e4676a6128883d987cab858</= a><br> >>>><br> >>>> commit 60381fd1ee8668ea1e4676a6128883d987cab858<br> >>>> Author:=C2=A0 =C2=A0 =C2=A0John Baldwin <jhb@FreeBSD.or= g><br> >>>> AuthorDate: 2023-07-14 18:30:31 +0000<br> >>>> Commit:=C2=A0 =C2=A0 =C2=A0John Baldwin <jhb@FreeBSD.or= g><br> >>>> CommitDate: 2023-07-14 18:32:16 +0000<br> >>>><br> >>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0memdesc: Retire MEMDESC_CCB.<br> >>>><br> >>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0Instead, change memdesc_ccb to e= xamine the CCB and return a<br> >> memdesc of<br> >>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0a more generic type describing t= he data buffer.<br> >>><br> >>>> diff --git a/sys/kern/subr_bus_dma.c b/sys/kern/subr_bus_d= ma.c<br> >>>> index 65a08aeba17c..bfaad30b37d3 100644<br> >>>> --- a/sys/kern/subr_bus_dma.c<br> >>>> +++ b/sys/kern/subr_bus_dma.c<br> >>>> @@ -304,94 +304,6 @@ bus_dmamap_load_ma_triv(bus_dma_tag_t= dmat,<br> >> bus_dmamap_t map,<br> >>>> @@ -566,49 +478,18 @@ bus_dmamap_load_ccb(bus_dma_tag_t dm= at,<br> >> bus_dmamap_t map, union ccb *ccb,<br> >>>> +=C2=A0 =C2=A0 mem =3D memdesc_ccb(ccb);<br> >>>> +=C2=A0 =C2=A0 return (bus_dmamap_load_mem(dmat, map, &= ;mem, callback,<br> >> callback_arg,<br> >>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 flags));<br> >>>>=C2=A0 =C2=A0 }<br> >>> This makes kernel not linkable if CAM is not included into it.= <br> >><br> >> Hmmm, ok.=C2=A0 I can either move the memdesc_ccb routine into sys= /kern<br> >> somewhere<br> >> (like the kern_memdesc.c file in my other pending review), or we c= an #ifdef<br> >> this function.=C2=A0 It probably doesn't make sense to have a<= br> >> bus_dmamap_load_ccb<br> >> if you don't have CAM, so I think I prefer the second option.<= br> >><br> > <br> > MINIMAL doesn't have CAM configured, but it is loadable as a modul= e.<br> > <br> > I'd think we'd want a dummy one fo these with weak symbol bind= ing and have<br> > the actual<br> > one live in cam somewhere that overrides this=C2=A0 symbol.<br> > <br> > I just hit this in building MINIMAL for other reasons....<br> <br> Yeah, I was testing MINIMAL locally (which is still broken due to recent<br= > TCP commits) to try out possible fixes.=C2=A0 One could possibly move<br> bus_dmamap_load_ccb into cam.ko entirely?=C2=A0 Especially this current ver= sion<br> doesn't really have any bus_dma-specific knowledge anymore but is just = a<br> thin wrapper around bus_dmamap_load_mem:<br> <br> int<br> bus_dmamap_load_ccb(bus_dma_tag_t dmat, bus_dmamap_t map, union ccb *ccb,<b= r> =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> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 int f= lags)<br> {<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct ccb_hdr *ccb_h;<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct memdesc mem;<br> <br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 ccb_h =3D &ccb->ccb_h;<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ((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=A0 callback(callback_a= rg, NULL, 0, 0);<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return (0);<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 }<br> <br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 mem =3D memdesc_ccb(ccb);<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 return (bus_dmamap_load_mem(dmat, map, &mem= , callback, callback_arg,<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 flags));<br> }<br></blockquote><div><br></div><div>And who calls bus_dmamap_load_ccb? If= this were entirely in cam.ko, then callers of it would<br></div><div>need = to resolve it. sys/dev/nvme/nvme_qpair.c is not otherwise dependent on cam,= and would</div><div>be unresolved for a minimal=C2=A0+ nvme=C2=A0+ nvd ker= nel.</div><div><br></div><div>Warner</div></div></div> --000000000000aefcf40600b2750f--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfrr3pPhkbueJyHLYA0FhkKXQefSNx=QLOUjxNPh4FR1Aw>