Date: Fri, 06 Feb 2015 10:51:08 -0500 From: John Baldwin <jhb@freebsd.org> To: NGie Cooper <yaneurabeya@gmail.com> Cc: "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, Navdeep Parhar <np@freebsd.org> Subject: Re: svn commit: r278303 - in head/sys: dev/cxgbe modules/cxgbe modules/cxgbe/if_cxl Message-ID: <2608873.KEiTQF7Lcd@ralph.baldwin.cx> In-Reply-To: <CAGHfRMCfWoc760X4B%2BWXrGjdUhtXch0VTsv8Ek3J5rY_zeTsyg@mail.gmail.com> References: <201502060110.t161A58m067355@svn.freebsd.org> <CAGHfRMCfWoc760X4B%2BWXrGjdUhtXch0VTsv8Ek3J5rY_zeTsyg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday, February 05, 2015 05:33:36 PM NGie Cooper wrote: > On Thu, Feb 5, 2015 at 5:10 PM, Navdeep Parhar <np@freebsd.org> wrote: > > Author: np > > Date: Fri Feb 6 01:10:04 2015 > > New Revision: 278303 > > URL: https://svnweb.freebsd.org/changeset/base/278303 > > > > Log: > > cxgbe(4): Add a minimal if_cxl module that pulls in the real driver as > > a dependency. This ensures "ifconfig cxl<n> ..." does the right thing > > even when it's run with no driver loaded. > > > > if_cxl.ko is the tiniest module in /boot/kernel. > > A couple things: > > 1. cxl(4) doesn't have a manpage: > > $ man 4 cxl > No manual entry for cxl > $ man 4 if_cxl > No manual entry for if_cxl An MLINK (along with updating cxgbe.4 to reference both names) should suffice for that. > 2. This could have been done with hardlinks to avoid creating an > additional driver (and on the plus side it saves disk space): > > $ git diff Makefile > diff --git a/sys/modules/cxgbe/if_cxgbe/Makefile > b/sys/modules/cxgbe/if_cxgbe/Makefile > index 32347f4..b00ee0a 100644 > --- a/sys/modules/cxgbe/if_cxgbe/Makefile > +++ b/sys/modules/cxgbe/if_cxgbe/Makefile > @@ -25,5 +25,7 @@ SRCS+= t4_tracer.c > > CFLAGS+= -I${CXGBE} > > +LINKS+= ${KMOD}.ko if_cxl.ko > + > .include <bsd.kmod.mk> > CFLAGS+= ${GCC_MS_EXTENSIONS} > > This is basically what I'm going to set out and do to fix this PR: > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=186449 (it's another > item that irritates me with misnamed drivers, like ixgbe.ko used to > be...). > > Using hardlinks instead of renaming the driver is preferred because it > makes MFCing possible without breaking loader.conf for sysadmins. In > general, major version steps (CURRENT) should use if_<foo> > consistently. Hardlinks won't work alone. You need an actual module named 'cxl' or 'if_cxl' (with an optional bus prefix) for ifmaybeload() to not try a spurious kldload as it depends on module names, not kld names (it depends on the kldname once it goes to load something, but uses the module name to decide whether or not it needs to try to load something). You can either stuff that module into if_cxgbe.c and use a hard link or create a dummy module. Also, the code for this in ifconfig seems to be using strlcpy to complicated. It also doesn't allow for numbers anywhere in a driver name (like in the original Intel 40GB driver name) since it stops at the first digit instead of walking backwards to find the first non-digit. I didn't try to fix the latter, but this seems more sane: Index: ifconfig.c =================================================================== --- ifconfig.c (revision 278285) +++ ifconfig.c (working copy) @@ -1261,7 +1261,6 @@ print_vhid(const struct ifaddrs *ifa, const char * void ifmaybeload(const char *name) { -#define MOD_PREFIX_LEN 3 /* "if_" */ struct module_stat mstat; int fileid, modid; char ifkind[IFNAMSIZ + MOD_PREFIX_LEN], ifname[IFNAMSIZ], *dp; @@ -1280,9 +1279,8 @@ ifmaybeload(const char *name) } /* turn interface and unit into module name */ - strcpy(ifkind, "if_"); - strlcpy(ifkind + MOD_PREFIX_LEN, ifname, - sizeof(ifkind) - MOD_PREFIX_LEN); + strlcpy(ifkind, "if_", sizeof(ifkind)); + strlcat(ifkind, ifname, sizeof(ifkind)); /* scan files in kernel */ mstat.version = sizeof(struct module_stat); @@ -1299,8 +1297,8 @@ ifmaybeload(const char *name) cp = mstat.name; } /* already loaded? */ - if (strncmp(ifname, cp, strlen(ifname) + 1) == 0 || - strncmp(ifkind, cp, strlen(ifkind) + 1) == 0) + if (strcmp(ifname, cp) == 0 || + strcmp(ifkind, cp) == 0) return; } } -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2608873.KEiTQF7Lcd>