e1000: fix interface capabilities management

[vmaffione]

Summary

The e1000 drivers (em, lem, igb) are currently looking at the
iflib copies of the capabilities bitvectors (scctx->isc_capabilities
and scctx->isc_capenable) rather than the ifnet ones
(ifp->if_capabilities and ifp->if_capenable). However, the latter
are the ones that are actually updated by ifconfig and that should
be used by the drivers during interface operation. The former are
set by the driver on interface attach (for iflib internal use)
and should not be used anymore by the driver.
This patch fixes the e1000 driver to use the correct bitvectors.

[markj]

Could you please upload a diff with context?

[vmaffione]

Added diff context.

[markj]

Doesn't this change need to go further? I see some uses of isc_capenable in the data path, e.g., in igb_isc_rxd_pkt_get(), why do we not go to the ifp there? I'd guess it's to avoid extra memory accesses, but we do query the ifp capenable flags in some places.

I also wonder why exactly iflib maintains its own capenable bitset, and why SIOCSIFCAP doesn't modify it.

sys/dev/e1000/if_em.c

929

Stray newline.

[vmaffione]

Indeed. I had put the changes to igb in a separate review: https://reviews.freebsd.org/D33156

Regarding the iflib private copies of capabilities (isc_capenable and isc_capabilities), it's also a bit unclear to me why those are necessary. It looks they're mostly used to pass the info from the driver to iflib, so that iflib can call if_setcapabilities() and if_setcapenable().
I guess SIOCSIFCAP could update them to align to if_capenable and if_capabilities. That could be useful to avoid an additional indirection in the drivers.

[markj]

In D33154#751282, @vmaffione wrote:

Indeed. I had put the changes to igb in a separate review: https://reviews.freebsd.org/D33156

Ah, sorry I missed it.

Regarding the iflib private copies of capabilities (isc_capenable and isc_capabilities), it's also a bit unclear to me why those are necessary. It looks they're mostly used to pass the info from the driver to iflib, so that iflib can call if_setcapabilities() and if_setcapenable().
I guess SIOCSIFCAP could update them to align to if_capenable and if_capabilities. That could be useful to avoid an additional indirection in the drivers.

I suspect that isc_capenable should just go away. This change looks like a good step forward though.

[vmaffione]

sys/dev/e1000/if_em.c

929

Thanks, I'll apply this before committing