Add extended media types to if_media.h and ifconfig

[erj]

Summary

Mailing list archive link: https://lists.freebsd.org/pipermail/freebsd-arch/2015-February/016756.html

This patch (minus new non-V.fast media types) was originally created by Mike Karels <[e-mail]>.

In the ifmedia word (used to represent various media types that a network adapter will use), the Ethernet media type was running out of room for new subtypes (e.g. 10GBaseKR). So in this patch, the previously unused RFU bit is now the "extended media type" bit, which can be used to double the number of available subtypes for use for code that checks for it. This solution should be backwards compatible with drivers/utilities that don't know what the bit is used for, meaning that nothing has to change if it doesn't want to properly display or use the new media types.

[glebius]

We can't and don't plan to preserve the driver KPI for the 11 branch. We
actually plan to change it very much, in sake of keeping it stable there
on. Please see on what is going on in the projects/ifnet branch.

So, I'd suggest to introduce new 'struct ifmedia' with enough space,
and of course put extra space in there. Give a new value to SIOCGIFMEDIA.
Write a new clear code to handle it, without any extended bit tricks.

For the sake of userland API, save old current 'struct ifmedia' as
'struct oifmedia', and take old value of ioctl to OSIOCIGIFMEDIA.
Keep current code for old APU, only renaming the struct and ioctl
value.

[glebius]

Is it possible to add Mike Karels to this discussion?

[gnn]

BTW Mike Karels was in favor of this in an email thread. He's not yet on phabricator but I'll ask him here as well.

[• karels]

In D1965#11, @gnn wrote:

BTW Mike Karels was in favor of this in an email thread. He's not yet on phabricator but I'll ask him here as well.

Agreed, with minor exceptions. Given the ixl changes, I don't think the vtnet example is worth keeping, and I think that the VFAST/V.fast entry should be removed and other subtypes moved down.

[• hselasky]

sys/net/if_media.h

192

Style nit. <tab> after define.

[erj]

Updated patch from Mike Karels (@mike-karels.net). I hope I got it merged in correctly.

[• karels]

A few notes on the last update:

• The encoding is now based on hps' idea and patch to use most of the unused type-specific option bits. I added some macros and comments. The additional types include in his last update are included here (even 100_T).

• I also changed the mechanism to avoid shifting and masking, which messed up setting the extended values. Sets via ifconfig now work.

• I retained the value of IFM_OTHER, and the compatibility code that returns IFM_OTHER for subtypes that can't be expressed in the old encoding when using SIOCGIFMEDIA. As before, there is SIOCGIFXMEDIA to get the new value. This is more compatible with old binaries.

It looks like net/if_media.c hasn't been updated. The version in -current has changed since the review started, and I removed the DEBUG definition.

Should __FreeBSD_version be updated when this goes in?

sys/net/if_media.h

192

fwiw, every define in the original file is followed by a tab.

If you are talking about line 178, it should be removed.

[• hselasky]

Hi,

Looks good to me. Did you include all the additional media types that was added in D2056?

--HPS

[• karels]

Yes, I pulled in all the types from the latest in D2056 except VFAST.

[glebius]

So, how many extra types do we got after Hans suggestion? For how long would that be enough?

[• hselasky]

Thank you!

sys/net/if_media.h

137

Maybe the _X() macro should be named _XSUBTYPE(), because _X() is too generic and might conflict with existing applications!

[• hselasky]

Gleb: I think we added 57 types in the past, the answer would be 512/57*(2015-1995) ~ 180 years, starting 1995.

I think the string conversion in the end has to be done by the kernel or we need to throw out types no longer in use, like keep the list below 511 types total at any time. We simply can't just accumulate and accumulate. It's like with PCI. Will they stop making PCI devices once the PCI-IDs are all allocated?

--HPS

[• karels]

Hmm, looks like if_media.h isn't right. Eric, did you apply the new patch to the previous version? It was meant to apply to the base version. It is currently only using one extra bit for extended type. Check the whole file, it should be right.

About _X: I meant that as a shorthand within this file. It should be undefed at the end of the file.

[erj]

@mike-karels.net, it does look like I applied it over the past version. This one should be correct.

Though, you mentioned _X() was supposed to be undefined at the end of the file, and I don't see that happening in this patched version.

[• karels]

I meant that _X should be undefed, not that it was. I forgot to do it. If there are no other changes, could you add that? Thanks.

[• karels]

This now requires re-review; reviewers, could you take a look?

Thanks,
Mike

[glebius]

As I already said, I prefer to make new ioctl use new improved structure (see D2008). But looks like majority disagrees, so I need to abide to it. I hope that authors of the patch will maintain this part of the network stack in future. With the new complex logic this part is less comprehendable by a newcomer than it was before.

JFYI, what I plan to do in projects/ifnet is to change the way how drivers allocate supported medias. The drivers would have a static array of supported medias, so that there will be no need to malloc(9). Since different NICs under one driver can support different set of medias, at attach time they will supply a bitset enabling certain medias from the drivers array.

Since this is a kernel API/ABI change (as the whole projects/ifnet branch is!), then I would like also to improve the media description, while here. We can supply as many bytes as needed into the new media description. So, drivers can stop pushing all info into one 'int'. This would allow to shift all the complex logic of D1965 into the ioctl layer. If you volunteer to help me with that, that will be appreciated.

[erj]

In D1965#36, @mike-karels.net wrote:

I meant that _X should be undefed, not that it was. I forgot to do it. If there are no other changes, could you add that? Thanks.

I tried compiling with _X #undef'd at the end of the file, but I got this error:

--- all_subdir_etherswitchcfg ---
/usr/home/erj/fbsd/head/sbin/etherswitchcfg/ifmedia.c:364:5: error: implicit declaration of function '_X' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
    IFM_SUBTYPE_ETHERNET_DESCRIPTIONS;
    ^
--- bin.all__D ---
--- all_subdir_sh ---
chmod +x functional_test.tmp
--- lib.all__D ---
c++   -pg  -O2 -pipe -I/usr/home/erj/fbsd/head/lib/libc++/../../contrib/libc++/include -I/usr/home/erj/fbsd/head/lib/libc++/../../contrib/libcxxrt -nostdlib -DLIBCXXRT -fstack-protector -Wno-empty-body -Wno-string-plus-int -Wno-unused-const-variable -Wno-tautological-compare -Wno-unused-value -Wno-parentheses-equality -Wno-unused-function -Wno-enum-conversion -Wno-unused-local-typedef -Wno-switch -Wno-switch-enum -Wno-knr-promoted-parameter -Wno-parentheses -Qunused-arguments  -std=c++11 -Wno-c++11-extensions -c /usr/home/erj/fbsd/head/lib/libc++/../../contrib/libc++/src/hash.cpp -o hash.po
--- usr.bin.all__D ---
--- all_subdir_basename ---
chmod +x basename_test.tmp
--- sbin.all__D ---
/usr/home/erj/fbsd/head/sbin/etherswitchcfg/../../sys/net/if_media.h:424:4: note: expanded from macro 'IFM_SUBTYPE_ETHERNET_DESCRIPTIONS'
        { IFM_10G_KX4,  "10GBase-KX4" },                                \
          ^
/usr/home/erj/fbsd/head/sbin/etherswitchcfg/../../sys/net/if_media.h:173:21: note: expanded from macro 'IFM_10G_KX4'
#define IFM_10G_KX4     _X(32)          /* 10GBase-KX4 backplane */
                        ^
/usr/home/erj/fbsd/head/sbin/etherswitchcfg/ifmedia.c:364:5: error: initializer element is not a compile-time constant
    IFM_SUBTYPE_ETHERNET_DESCRIPTIONS;
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/home/erj/fbsd/head/sbin/etherswitchcfg/../../sys/net/if_media.h:424:4: note: expanded from macro 'IFM_SUBTYPE_ETHERNET_DESCRIPTIONS'
        { IFM_10G_KX4,  "10GBase-KX4" },                                \
          ^~~~~~~~~~~
/usr/home/erj/fbsd/head/sbin/etherswitchcfg/../../sys/net/if_media.h:173:21: note: expanded from macro 'IFM_10G_KX4'
#define IFM_10G_KX4     _X(32)          /* 10GBase-KX4 backplane */

I don't know if there's a way to get _X() macro evaluated before ifmedia.c is compiled, or if I should not undefine _X.

[• karels]

You are right, it doesn't work. I switched to IFM_X. See ftp.karels.net/outgoing/if_media.h.

[erj]

Updated if_media.h to replace _X() with IFM_X(), from @mike-karels.net

[• karels]

Following up on Glebius' comment: I think this interface is fine for the short term, including MFC. I think it is quite reasonable to come up with an improved interface, both ioctl and KPI, for 11. I don't think more bits for existing fields is a big enough motivation; I think the interface needs a fair amount of thought and rework. I'm willing to assist in this, but we also need input from driver authors.

[erj]

Can I assume no one else is going to update their approval because they implicitly approve this current version?

[gnn]

Do it.

[erj]

Commiitted as r281236 (rS281236).

Now to work on updating D1966...