Date: Fri, 6 Jun 2025 15:37:39 -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: 7b3ee39e73af - main - libcam: Include nvme opcode and status code routines from nvme_util.c Message-ID: <CANCZdfoRujBWW8g=HyJ2epS6z7E1Cbv4nwCVO3U-8n-5JGMUxw@mail.gmail.com> In-Reply-To: <346c0310-b9b6-4eb7-ac94-0575a83c3416@FreeBSD.org> References: <202506060132.5561Wakc094185@gitrepo.freebsd.org> <eea5514d-2b3b-4491-bc2a-9bbed5bdacbc@FreeBSD.org> <CANCZdfp6-QVoJRD5JsMML0%2BzdMxSo4Od0U3Zq20nVbn1n0BUQw@mail.gmail.com> <346c0310-b9b6-4eb7-ac94-0575a83c3416@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Jun 6, 2025 at 8:34=E2=80=AFAM John Baldwin <jhb@freebsd.org> wrote= : > > On 6/5/25 22:21, Warner Losh wrote: > > On Thu, Jun 5, 2025 at 7:41=E2=80=AFPM John Baldwin <jhb@freebsd.org> w= rote: > >> > >> On 6/5/25 21:32, John Baldwin wrote: > >>> The branch main has been updated by jhb: > >>> > >>> URL: https://cgit.FreeBSD.org/src/commit/?id=3D7b3ee39e73af36f49f471f= 7900baeb98ac3504d0 > >>> > >>> commit 7b3ee39e73af36f49f471f7900baeb98ac3504d0 > >>> Author: John Baldwin <jhb@FreeBSD.org> > >>> AuthorDate: 2025-06-06 01:28:38 +0000 > >>> Commit: John Baldwin <jhb@FreeBSD.org> > >>> CommitDate: 2025-06-06 01:28:38 +0000 > >>> > >>> libcam: Include nvme opcode and status code routines from nvme_= util.c > >>> > >>> libcam in userspace also includes nvme_all.c which now depends = on > >>> nvme_util.c, so add nvme_util.c to libcam's sources. This requ= ires > >>> exporting the opcode and status code routines in nvme_util.c to > >>> userspace as well as the kernel. In turn, this means nvmecontr= ol now > >>> depends on libsbuf (which is already present in /lib). > >>> > >>> Reported by: viswhin, Jenkins > >>> Fixes: 60159a98a837 ("nvme: Move opcode and status cod= e tables from base CAM to nvme_util.c") > >>> Sponsored by: Chelsio Communications > >> > >> This fixes the build for now (and sorry for breaking it). However, th= is > >> raises a few questions for me at least. Why does libcam include nvme_= all.c > >> at all? We don't document any of the nvme_* functions in cam(3), nor = any > >> of the functions from scsi_all.c, smp_all.c, etc. This seems really o= dd, > >> and it also means that we can add (and remove!) symbols from libcam wi= thout > >> realizing it by changing sys/cam/<proto>/<proto>_all.h which is not ve= ry > >> obvious. > >> > >> It seems to me that we should be more intentional about which symbols = we > >> export from libcam. Switching to symbol versioning (which implicitly > >> enforces hidden visibility on all symbols not explicitly exported) wou= ld > >> keep us from leaking symbols into the ABI of libcam that we don't inte= nd > >> to export. > >> > >> I also wonder if we can remove some of the *_all.c files from libcam > >> entirely? None of the nvme_* ones are used outside of the kernel in t= he > >> base system for example. > > > > No. We can't remove all that. They are used by camcontrol, ddcam and > > others. We use the functions in *_all.h/c functions to populate the > > CCBs to send down into the kernel. I think that we need a different > > take on it. It is, after all, basically designed in FreeBSD 3 > > timeframe when such considerations were the furthest thing from the > > minds of the developers. Times have changed, though, and libcam hasn't > > with it. > > We don't use a single routine from nvme_all.c outside of the kernel. > libcam also doesn't include mmc_all.c. Hmmm, I added it into libcam because we did use one. But maybe we don't need it. It's been a while, and I added it in my experimental tree and push that upstream before I pushed the things that used it, so maybe something got dropped. > > We don't document it very well... Sometimes I've thought we should > > make libcam a private library. But I don't know who all uses it in raw > > mode. I think about a dozen ports use this for things like smart > > reporting... > > I think a decent start would be to maybe be a bit more choosy in what > things have to be exported. Only a few routines in ata_all.c and > smp_all.c appear to be used in camcontrol for example. And it > certainly seems like we could remove nvme_all.c from libcam entirely. I'll give that a short. I was sure that something was using it, but that may have only been in early version(s) of code... I'll look into it. > Using a version map with symbol versioning might be the best way to > enforce that (though more liberal use of #ifdef _KERNEL in *_all.c > would be another route). We definitely will need to bump the > SHLIB_MAJOR for libcam before branching 15 though. Yea. I've been hesitant to clean this up too much. I know $WORK doesn't need this library for anything interesting. But I also think we should check with Netapp and Spectralogic. Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfoRujBWW8g=HyJ2epS6z7E1Cbv4nwCVO3U-8n-5JGMUxw>