Date: Thu, 19 Feb 2015 13:52:19 -0800 From: Eric Joyner <ricera10@gmail.com> To: Adrian Chadd <adrian@freebsd.org> Cc: "freebsd-net@freebsd.org" <freebsd-net@freebsd.org>, George Neville-Neil <gnn@neville-neil.com>, mike@karels.net, "freebsd-arch@freebsd.org" <freebsd-arch@freebsd.org> Subject: Re: Adding new media types to if_media.h Message-ID: <CA%2Bb0zg_yDLhBmA-FkMTHrYyhOYfzh-6jVJ54GJ_vzwvSXYS2Xg@mail.gmail.com> In-Reply-To: <CAJ-VmomXdvW4YsneqiRAqO0EV19uOp=OyOLH7rAqMh3EPp=uGg@mail.gmail.com> References: <BC6AB805-B699-438E-9EC6-015C005D6B1D@neville-neil.com> <201502170150.t1H1ouxM020621@mail.karels.net> <CAJ-VmomXdvW4YsneqiRAqO0EV19uOp=OyOLH7rAqMh3EPp=uGg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
It does look good! We already have at least a half-dozen new media types to add. --- - Eric Joyner On Tue, Feb 17, 2015 at 9:26 AM, Adrian Chadd <adrian@freebsd.org> wrote: > Looks good to me. > > Thanks for doing this! > > > -a > > > On 16 February 2015 at 17:50, Mike Karels <mike@karels.net> wrote: > > On Feb 9, gnn wrote: > > > >> On 8 Feb 2015, at 22:41, Mike Karels wrote: > > > >> > Sorry to reply to a thread after such a long delay, but I think it is > >> > unresolved, and needs more discussion. I'd like to elaborate a bit on > >> > my goals and proposal. I believe Adrian has newer thoughts than have > >> > been > >> > circulated here as well. > >> > > >> > The last message(s) have gone to freebsd-arch and freebsd-net. If > >> > someone > >> > wants to pick one, we could consolidate, but this seems relevant to > >> > both. > >> > > >> > I'm going to top-post to try to summarize and extend the discussion, > >> > but the > >> > preceding emails follow for reference. > >> > > >> > To recap: the existing if_media interface is running out of steam, at > >> > least > >> > in that the "Media variant" field, with 5 bits, is going to be > >> > insufficient > >> > to express existing 40 Gb/s variants. The if_media media type is a > >> > 32-bit > >> > int with a bunch of sub-fields for type (e.g. Ethernet), > >> > subtype/variant > >> > (e.g. 10baseT, 10base5, 1000baseT, etc), flags, and some MII-related > >> > fields. > >> > > >> > I made a proposal to extend the interface in a small way, specifically > >> > to > >> > replace the "media word" with a 64-bit int that is mostly the same, > >> > but > >> > has a new, larger variant/subtype field. The main reason for this > >> > proposal > >> > is to maintain the driver KPI (glimpse showed me 240 inclusions of > >> > if_media.h > >> > in the kernel in 8.2). That interface includes an initialization > >> > using a > >> > scalar value of fields ORed with each other. It would also be easy to > >> > preserve a 32-bit user-level API/ABI that can express most of the > >> > current > >> > state, with a subtype/variant field value reserved for "other" (there > >> > is > >> > already one for "unknown", but that is not quite the same). fwiw, I > >> > found 45 references to this user-level API in our tree, including both > >> > base and "ports"-type software, which includes libpcap, snmpd, > >> > dhclient, > >> > quagga, xorp, atm, devd, and rtsold, which argues for a > >> > backward-compatible > >> > API/ABI as well as a more-complete current interface for ifconfig at > >> > least. > >> > > >> > More generally, I see two problems with the existing if_media > >> > interface: > >> > > >> > 1. It doesn't have enough bits for all the fields, in particular, > >> > variant/ > >> > subtype for Ethernet. That is the immediate issue. > >> > > >> > 2. The interface is not sufficiently generic; it was designed around > >> > Ethernet > >> > including MII, token ring, FDDI, and a few other interface types. > >> > Some of > >> > the fields like "instance" are primarily for MII as far as I know, and > >> > are > >> > basically unused. It is definitely not sufficient for 802.11, which > >> > has > >> > rolled its own interfaces. > >> > > >> > To solve the second problem, I think the right approach would be to > >> > reduce > >> > this interface to a truly generic one, such as media type (e.g. > >> > Ethernet), > >> > generic flags, and perhaps generic status. Then there should be a > >> > separate > >> > media-specific interface for each type, such as Ethernet and 802.11. > >> > To a > >> > small extent, we already have that. Solving the second, more general > >> > problem, > >> > requires a whole new driver KPI that will require surgery to every > >> > driver, > >> > which is not an exercise that I would consider. > >> > > >> > Using a separate int for each existing field, as proposed, would break > >> > the > >> > driver KPI, but would not really make the interface generic. Trying > >> > to > >> > make a single interface with the union of all network interface > >> > requirements > >> > seems like a bad idea to me (we failed last time; the "we" is BSDi, > >> > where > >> > I was the architect when this interface was first designed). (No, I > >> > didn't > >> > design this interface.) > >> > > >> > Solving the first problem only, I think it is preferable to preserve a > >> > compatible driver KPI, which means using a scalar value encoding what > >> > is > >> > necessary. Although that interface is rather Ethernet-centric, that > >> > is > >> > really what it is used for. > >> > > >> > An additional, selfish goal is to make it easy to back-port drivers > >> > using > >> > the new interface to older versions (which I am quite likely to do). > >> > Preserving the KPI and general user API will be highly useful there. > >> > I'd be likely to do a 11-style version of ifconfig personally, but it > >> > might not be difficult to do in a more general way. > >> > > >> > I am willing to do a prototype for -current for evaluation. > >> > > >> > Comments, alternatives, ? > > > >> I agree with your statements above and I'd like to see the prototype. > > > > Well, I developed the prototype as I had planned, using a 64-bit media > > word, and found that I got about 100 files in GENERIC that didn't > compile; > > they attempted to store "media words" in an int. My kingdom for a > typedef. > > That didn't meet my goal of KPI compatibility, so I went to Plan B. > > > > Plan B is to steal an unused bit (RFU) to indicate an "extended" media > > type. I then used the variant/subtype field to store the extended type. > > Effectively, the previously unused bit doubles the effective size of the > > subtype field. Given that the previous 5-bit field lasted us 18 years, > > I figured that doubling it would last a while. I also changed the > > SIOGGIFMEDIA ioctl, splitting it for binary compatibility; extended > > types are all mapped to IFM_OTHER (31) using the old interface, but > > are visible using the new one. > > > > With these changes, I modified one driver (vtnet) to use an extended > type, > > and the rest of GENERIC is happy. The changes to ifconfig are also > fairly > > small. The patch is appended, where email programs will screw it up, > > or at ftp://ftp.karels.net/outgoing/if_media.patch. > > > > The VFAST subtype is a throw-away for testing. > > > > This seems like a reasonably pragmatic change to support the new 40 Gb/s > > media types until someone wants to design an improved but non-backward- > > compatible interface. I think it meets the goal of suitability for > > back-porting; it could be MFCed. > > > > Mike > > > > Index: sys/net/if_media.h > > =================================================================== > > --- sys/net/if_media.h (revision 278804) > > +++ sys/net/if_media.h (working copy) > > @@ -120,15 +120,29 @@ > > * 5-7 Media type > > * 8-15 Type specific options > > * 16-18 Mode (for multi-mode devices) > > - * 19 RFU > > + * 19 "extended" bit for media variant > > * 20-27 Shared (global) options > > * 28-31 Instance > > */ > > > > /* > > + * As we have used all of the original values for the media variant > (subtype) > > + * for Ethernet, extended subtypes have been added, marked with > XSUBTYPE, > > + * which is effectively the "high bit" of the media variant (subtype) > field. > > + * IFM_OTHER (the highest basic type) is reserved to indicate use of an > > + * extended type when using an old SIOCGIFMEDIA operation. This is true > > + * for all media types, not just Ethernet. > > + */ > > +#define XSUBTYPE 0x80000 /* extended variant high > bit */ > > +#define _X(var) ((var) | XSUBTYPE) /* extended > variant */ > > +#define IFM_OTHER 31 /* Other: some > extended type */ > > +#define OMEDIA(var) (((var) & XSUBTYPE) ? IFM_OTHER : (var)) > > + > > +/* > > * Ethernet > > */ > > #define IFM_ETHER 0x00000020 > > +/* NB: 0,1,2 are auto, manual, none defined below */ > > #define IFM_10_T 3 /* 10BaseT - RJ45 */ > > #define IFM_10_2 4 /* 10Base2 - Thinnet */ > > #define IFM_10_5 5 /* 10Base5 - AUI */ > > @@ -156,11 +170,17 @@ > > #define IFM_40G_CR4 27 /* 40GBase-CR4 */ > > #define IFM_40G_SR4 28 /* 40GBase-SR4 */ > > #define IFM_40G_LR4 29 /* 40GBase-LR4 */ > > +#define IFM_AVAIL30 30 /* available */ > > +/* #define IFM_OTHER 31 Other: some extended type */ > > +/* note 31 is the max! */ > > + > > +/* Extended variants/subtypes */ > > +#define IFM_VFAST _X(0) /* test "V.fast" */ > > +/* note _X(31) is the max! */ > > /* > > * Please update ieee8023ad_lacp.c:lacp_compose_key() > > * after adding new Ethernet media types. > > */ > > -/* note 31 is the max! */ > > > > #define IFM_ETH_MASTER 0x00000100 /* master mode > (1000baseT) */ > > #define IFM_ETH_RXPAUSE 0x00000200 /* receive PAUSE frames > */ > > @@ -170,6 +190,7 @@ > > * Token ring > > */ > > #define IFM_TOKEN 0x00000040 > > +/* NB: 0,1,2 are auto, manual, none defined below */ > > #define IFM_TOK_STP4 3 /* Shielded twisted pair > 4m - DB9 */ > > #define IFM_TOK_STP16 4 /* Shielded twisted pair > 16m - DB9 */ > > #define IFM_TOK_UTP4 5 /* Unshielded twisted > pair 4m - RJ45 */ > > @@ -187,6 +208,7 @@ > > * FDDI > > */ > > #define IFM_FDDI 0x00000060 > > +/* NB: 0,1,2 are auto, manual, none defined below */ > > #define IFM_FDDI_SMF 3 /* Single-mode fiber */ > > #define IFM_FDDI_MMF 4 /* Multi-mode fiber */ > > #define IFM_FDDI_UTP 5 /* CDDI / UTP */ > > @@ -220,6 +242,7 @@ > > #define IFM_IEEE80211_OFDM27 23 /* OFDM 27Mbps */ > > /* NB: not enough bits to express MCS fully */ > > #define IFM_IEEE80211_MCS 24 /* HT MCS rate */ > > +/* #define IFM_OTHER 31 Other: some extended type */ > > > > #define IFM_IEEE80211_ADHOC 0x00000100 /* Operate in > Adhoc mode */ > > #define IFM_IEEE80211_HOSTAP 0x00000200 /* Operate in > Host AP mode */ > > @@ -241,6 +264,7 @@ > > * ATM > > */ > > #define IFM_ATM 0x000000a0 > > +/* NB: 0,1,2 are auto, manual, none defined below */ > > #define IFM_ATM_UNKNOWN 3 > > #define IFM_ATM_UTP_25 4 > > #define IFM_ATM_TAXI_100 5 > > @@ -277,7 +301,7 @@ > > * Masks > > */ > > #define IFM_NMASK 0x000000e0 /* Network type */ > > -#define IFM_TMASK 0x0000001f /* Media sub-type */ > > +#define IFM_TMASK 0x0008001f /* Media sub-type */ > > #define IFM_IMASK 0xf0000000 /* Instance */ > > #define IFM_ISHIFT 28 /* Instance shift */ > > #define IFM_OMASK 0x0000ff00 /* Type specific options > */ > > @@ -372,6 +396,7 @@ > > { IFM_40G_CR4, "40Gbase-CR4" }, \ > > { IFM_40G_SR4, "40Gbase-SR4" }, \ > > { IFM_40G_LR4, "40Gbase-LR4" }, \ > > + { IFM_VFAST, "V.fast" }, \ > > { 0, NULL }, \ > > } > > > > @@ -603,6 +628,7 @@ > > { IFM_AUTO, "autoselect" }, \ > > { IFM_MANUAL, "manual" }, \ > > { IFM_NONE, "none" }, \ > > + { IFM_OTHER, "other" }, \ > > { 0, NULL }, \ > > } > > > > @@ -673,6 +699,7 @@ > > { IFM_ETHER | IFM_40G_CR4, IF_Gbps(40ULL) }, \ > > { IFM_ETHER | IFM_40G_SR4, IF_Gbps(40ULL) }, \ > > { IFM_ETHER | IFM_40G_LR4, IF_Gbps(40ULL) }, \ > > + { IFM_ETHER | IFM_VFAST, IF_Gbps(40ULL) }, \ > > \ > > { IFM_TOKEN | IFM_TOK_STP4, IF_Mbps(4) }, \ > > { IFM_TOKEN | IFM_TOK_STP16, IF_Mbps(16) }, \ > > Index: sys/sys/sockio.h > > =================================================================== > > --- sys/sys/sockio.h (revision 278810) > > +++ sys/sys/sockio.h (working copy) > > @@ -128,5 +128,6 @@ > > #define SIOCGIFGROUP _IOWR('i', 136, struct ifgroupreq) /* > get ifgroups */ > > #define SIOCDIFGROUP _IOW('i', 137, struct ifgroupreq) /* > delete ifgroup */ > > #define SIOCGIFGMEMB _IOWR('i', 138, struct ifgroupreq) /* > get members */ > > +#define SIOCGIFXMEDIA _IOWR('i', 139, struct ifmediareq) /* > get net xmedia */ > > > > #endif /* !_SYS_SOCKIO_H_ */ > > Index: sys/net/if.c > > =================================================================== > > --- sys/net/if.c (revision 278749) > > +++ sys/net/if.c (working copy) > > @@ -2561,6 +2561,7 @@ > > case SIOCGIFPSRCADDR: > > case SIOCGIFPDSTADDR: > > case SIOCGIFMEDIA: > > + case SIOCGIFXMEDIA: > > case SIOCGIFGENERIC: > > if (ifp->if_ioctl == NULL) > > return (EOPNOTSUPP); > > Index: sys/net/if_media.c > > =================================================================== > > --- sys/net/if_media.c (revision 278804) > > +++ sys/net/if_media.c (working copy) > > @@ -67,7 +67,9 @@ > > static struct ifmedia_entry *ifmedia_match(struct ifmedia *ifm, > > int flags, int mask); > > > > +#define IFMEDIA_DEBUG > > #ifdef IFMEDIA_DEBUG > > +#include <net/if_var.h> > > int ifmedia_debug = 0; > > SYSCTL_INT(_debug, OID_AUTO, ifmedia, CTLFLAG_RW, &ifmedia_debug, > > 0, "if_media debugging msgs"); > > @@ -271,6 +273,7 @@ > > * Get list of available media and current media on interface. > > */ > > case SIOCGIFMEDIA: > > + case SIOCGIFXMEDIA: > > { > > struct ifmedia_entry *ep; > > int *kptr, count; > > @@ -278,8 +281,13 @@ > > > > kptr = NULL; /* XXX gcc */ > > > > - ifmr->ifm_active = ifmr->ifm_current = ifm->ifm_cur ? > > - ifm->ifm_cur->ifm_media : IFM_NONE; > > + if (cmd == SIOCGIFMEDIA) { > > + ifmr->ifm_active = ifmr->ifm_current = > ifm->ifm_cur ? > > + OMEDIA(ifm->ifm_cur->ifm_media) : IFM_NONE; > > + } else { > > + ifmr->ifm_active = ifmr->ifm_current = > ifm->ifm_cur ? > > + ifm->ifm_cur->ifm_media : IFM_NONE; > > + } > > ifmr->ifm_mask = ifm->ifm_mask; > > ifmr->ifm_status = 0; > > (*ifm->ifm_status)(ifp, ifmr); > > @@ -317,7 +325,10 @@ > > ep = LIST_FIRST(&ifm->ifm_list); > > for (; ep != NULL && count < ifmr->ifm_count; > > ep = LIST_NEXT(ep, ifm_list), count++) > > - kptr[count] = ep->ifm_media; > > + if (cmd == SIOCGIFMEDIA) > > + kptr[count] = > OMEDIA(ep->ifm_media); > > + else > > + kptr[count] = ep->ifm_media; > > > > if (ep != NULL) > > error = E2BIG; /* oops! */ > > @@ -505,7 +516,7 @@ > > printf("<unknown type>\n"); > > return; > > } > > - printf(desc->ifmt_string); > > + printf("%s", desc->ifmt_string); > > > > /* Any mode. */ > > for (desc = ttos->modes; desc && desc->ifmt_string != NULL; > desc++) > > > > Index: sys/dev/virtio/network/if_vtnet.c > > =================================================================== > > --- sys/dev/virtio/network/if_vtnet.c (revision 278749) > > +++ sys/dev/virtio/network/if_vtnet.c (working copy) > > @@ -938,6 +938,7 @@ > > ifmedia_init(&sc->vtnet_media, IFM_IMASK, vtnet_ifmedia_upd, > > vtnet_ifmedia_sts); > > ifmedia_add(&sc->vtnet_media, VTNET_MEDIATYPE, 0, NULL); > > + ifmedia_add(&sc->vtnet_media, IFM_ETHER | IFM_VFAST, 0, NULL); > > ifmedia_set(&sc->vtnet_media, VTNET_MEDIATYPE); > > > > /* Read (or generate) the MAC address for the adapter. */ > > @@ -1103,6 +1104,7 @@ > > > > case SIOCSIFMEDIA: > > case SIOCGIFMEDIA: > > + case SIOCGIFXMEDIA: > > error = ifmedia_ioctl(ifp, ifr, &sc->vtnet_media, cmd); > > break; > > Index: sbin/ifconfig/ifmedia.c > > =================================================================== > > --- sbin/ifconfig/ifmedia.c (revision 278749) > > +++ sbin/ifconfig/ifmedia.c (working copy) > > @@ -109,11 +109,17 @@ > > { > > struct ifmediareq ifmr; > > int *media_list, i; > > + int xmedia = 1; > > > > (void) memset(&ifmr, 0, sizeof(ifmr)); > > (void) strncpy(ifmr.ifm_name, name, sizeof(ifmr.ifm_name)); > > > > - if (ioctl(s, SIOCGIFMEDIA, (caddr_t)&ifmr) < 0) { > > + /* > > + * Check if interface supports extended media types. > > + */ > > + if (ioctl(s, SIOCGIFXMEDIA, (caddr_t)&ifmr) < 0) > > + xmedia = 0; > > + if (xmedia == 0 && ioctl(s, SIOCGIFMEDIA, (caddr_t)&ifmr) < 0) { > > /* > > * Interface doesn't support SIOC{G,S}IFMEDIA. > > */ > > @@ -130,8 +136,13 @@ > > err(1, "malloc"); > > ifmr.ifm_ulist = media_list; > > > > - if (ioctl(s, SIOCGIFMEDIA, (caddr_t)&ifmr) < 0) > > - err(1, "SIOCGIFMEDIA"); > > + if (xmedia) { > > + if (ioctl(s, SIOCGIFXMEDIA, (caddr_t)&ifmr) < 0) > > + err(1, "SIOCGIFXMEDIA"); > > + } else { > > + if (ioctl(s, SIOCGIFMEDIA, (caddr_t)&ifmr) < 0) > > + err(1, "SIOCGIFMEDIA"); > > + } > > > > printf("\tmedia: "); > > print_media_word(ifmr.ifm_current, 1); > > @@ -194,6 +205,7 @@ > > { > > static struct ifmediareq *ifmr = NULL; > > int *mwords; > > + int xmedia = 1; > > > > if (ifmr == NULL) { > > ifmr = (struct ifmediareq *)malloc(sizeof(struct > ifmediareq)); > > @@ -213,7 +225,10 @@ > > * the current media type and the top-level type. > > */ > > > > - if (ioctl(s, SIOCGIFMEDIA, (caddr_t)ifmr) < 0) { > > + if (ioctl(s, SIOCGIFXMEDIA, (caddr_t)ifmr) < 0) { > > + xmedia = 0; > > + } > > + if (xmedia == 0 && ioctl(s, SIOCGIFMEDIA, (caddr_t)ifmr) > < 0) { > > err(1, "SIOCGIFMEDIA"); > > } > > > > @@ -225,8 +240,13 @@ > > err(1, "malloc"); > > > > ifmr->ifm_ulist = mwords; > > - if (ioctl(s, SIOCGIFMEDIA, (caddr_t)ifmr) < 0) > > - err(1, "SIOCGIFMEDIA"); > > + if (xmedia) { > > + if (ioctl(s, SIOCGIFXMEDIA, (caddr_t)ifmr) < 0) > > + err(1, "SIOCGIFXMEDIA"); > > + } else { > > + if (ioctl(s, SIOCGIFMEDIA, (caddr_t)ifmr) < 0) > > + err(1, "SIOCGIFMEDIA"); > > + } > > } > > > > return ifmr; > > _______________________________________________ > > freebsd-net@freebsd.org mailing list > > http://lists.freebsd.org/mailman/listinfo/freebsd-net > > To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org" > _______________________________________________ > freebsd-arch@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-arch > To unsubscribe, send any mail to "freebsd-arch-unsubscribe@freebsd.org" >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CA%2Bb0zg_yDLhBmA-FkMTHrYyhOYfzh-6jVJ54GJ_vzwvSXYS2Xg>