Re: git: 7b3ee39e73af - main - libcam: Include nvme opcode and status code routines from nvme_util.c

On Fri, Jun 6, 2025 at 8:34 AM John Baldwin <jhb@freebsd.org> wrote:
>
> On 6/5/25 22:21, Warner Losh wrote:
> > On Thu, Jun 5, 2025 at 7:41 PM John Baldwin <jhb@freebsd.org> wrote:
> >>
> >> 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=7b3ee39e73af36f49f471f7900baeb98ac3504d0
> >>>
> >>> 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 requires
> >>>       exporting the opcode and status code routines in nvme_util.c to
> >>>       userspace as well as the kernel.  In turn, this means nvmecontrol now
> >>>       depends on libsbuf (which is already present in /lib).
> >>>
> >>>       Reported by:    viswhin, Jenkins
> >>>       Fixes:          60159a98a837 ("nvme: Move opcode and status code tables from base CAM to nvme_util.c")
> >>>       Sponsored by:   Chelsio Communications
> >>
> >> This fixes the build for now (and sorry for breaking it).  However, this
> >> 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 odd,
> >> and it also means that we can add (and remove!) symbols from libcam without
> >> realizing it by changing sys/cam/<proto>/<proto>_all.h which is not very
> >> 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) would
> >> keep us from leaking symbols into the ABI of libcam that we don't intend
> >> 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 the
> >> 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