Revamp ixgbe into seperate if_ix and if_ixv drivers
ClosedPublic
Actions

Authored by jfv on Jan 21 2015, 10:28 PM.

Details

Reviewers

jhb
marcel
araujo
gnn
rstone

Summary

This delta is very large, but its necessary. In preparation for full SRIOV support for this and the 40G
driver its necessary to seperate them.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

jfv updated this revision to Diff 3299.Jan 21 2015, 10:28 PM

jfv retitled this revision from to Revamp ixgbe into seperate if_ix and if_ixv drivers.

jfv updated this object.

jfv edited the test plan for this revision. (Show Details)

jfv added reviewers: gnn, jhb, rstone, marcel.

emaste added a subscriber: emaste.Jan 22 2015, 1:29 AM

We'll pop this into test and see what happens.

head/sys/conf/NOTES
2104	Do you want to update this comment to reflect 10/40gbe ethernet?
head/sys/conf/files
1768	This requires a kernel options change in sys/conf ? This also requires a kernel config change in GENERIC?
head/sys/dev/ixgbe/if_ixv.c
74	Same as earlier, should this be 10/40GBE?

Can you provide more details on why you need to separate them (and why you need to rename them)? Note that a single kld can contain multiple drivers (like if_lem.c and if_em.c in if_em.ko).

I do think having the kld name match the interface is good (so the ifconfig auto-load magic works), but it's not clear to me why you can't just include the VF driver in the PF driver's module? (And renaming options in a kernel config is pretty disruptive, so there need to be good reasons.. People are probably still somewhat burned over emX devices changing to igbX at one point.)

erj added a subscriber: erj.Jan 28 2015, 5:38 PM

erj added a reviewer: araujo.Jan 28 2015, 7:01 PM

In D1580#6, @jhb wrote:

Can you provide more details on why you need to separate them (and why you need to rename them)? Note that a single kld can contain multiple drivers (like if_lem.c and if_em.c in if_em.ko).

I do think having the kld name match the interface is good (so the ifconfig auto-load magic works), but it's not clear to me why you can't just include the VF driver in the PF driver's module? (And renaming options in a kernel config is pretty disruptive, so there need to be good reasons.. People are probably still somewhat burned over emX devices changing to igbX at one point.)

The big reason for seperating the VF is that once we have the ability to HOST, its essential that the VF driver
does not load.

You mean to avoid having the driver claim the VFs? That is going to be solved differently I think. Ryan's SRIOV changes already allow the driver to be forced to be "ppt" to support pass through, and the devctl tool I'm committing in the next day or so will allow the administrator to change this on the fly. I think those are the better routes to address this.

Hi Eric,

For those two patches below, could you point to me where did you address it?
https://reviews.freebsd.org/D823
https://reviews.freebsd.org/D811

All the rest looks good for me.

@araujo I added inline comments to where I made the changes.

In D1580#11, @araujo wrote:

Hi Eric,

For those two patches below, could you point to me where did you address it?
https://reviews.freebsd.org/D823
https://reviews.freebsd.org/D811

All the rest looks good for me.

head/sys/conf/NOTES
2104	ixl is the one that is for 40GbE products; though that one's comment might need updating because there are X710/10gig products out there.
head/sys/dev/ixgbe/if_ix.c
1490	@araujo -- 1000baseT should be correctly displayed now. D811
1588	@araujo -- This is where you can change the media type to 10GbaseT, or other media types.
2519	@araujo -- This is where 10GBaseT and other media types are added to the ifconfig -m list

Hi,

For those changes I mentioned before:
https://reviews.freebsd.org/D823
https://reviews.freebsd.org/D811

This version does the job.
Thanks Eric,

This revision is now accepted and ready to land.Feb 4 2015, 2:20 AM

In D1580#11, @araujo wrote:

Hi Eric,

For those two patches below, could you point to me where did you address it?
https://reviews.freebsd.org/D823
https://reviews.freebsd.org/D811

All the rest looks good for me.

Hi Eric,

I got approve for D823 and D811 from @jfvogel, I would like to commit it.
Do you have any objection?

Best Regards,

head/sys/dev/ixgbe/if_ix.c
1615	Would it might be 10G instead of 1GB_FULL?
1621	1G instead of 100?

Looks like this can be closed as it was committed?

jfv closed this revision.Mar 27 2015, 7:34 AM

jfv added inline comments.

head/sys/conf/NOTES
2104	What did you mean here Sean, this is not for 40G?
head/sys/conf/files
1768	Oh, you may be right about GENERIC, I'll add that.
head/sys/dev/ixgbe/if_ixv.c
74	Are you confusing this with ixl/ixlv, or did you somehow want the 4x10 skew to be called 40G, I think that would just lead to confusion.