Agree that comment should reference NVIDIA but that is fine to do in a non-functional commit

The handbook has been problematic WRT NVIDIA drivers, it duplicates and is often outdated information, so I would support centralizing some of this to the installation and making that a slimline section. In particular I don't think the handbook should elaborate hardware or versions, it should bounce to NVIDIA for hardware support policies and somehow link to the packages in a reasonable way but that may mean an external service like freshports?

I'm not really seeing this as a general improvement

Overall I am fine with it, provided suggestions about the master driver and kmod

Seems like progress to me. Would be good to get a pass from @ivy who did a lot in bridge lately. It would be a good idea to do present quick A/B test to make sure there are no unexpected perf issues. I don't think there is any reason to delay for GSO (unrelated) or a rework flippening offloads which can progress at its own pace.

In D55196#1262270, @gallatin wrote:

In D55196#1262239, @kbowling wrote:

I think it's driver bugs that have flown under the radar until the recent RSS change so I'm not sure this is the place to do this.

In particular to this review, opaque and opaque_hash should both be usable hashes if they are implemented correctly in the driver.

Thinking about this more, I think when we are using the hash for setting flowtype, we should check for valid toeplitz hash types, since we're counting on being able to calculate the same hash in software. So I think this patch is probably the correct place to fix this. I think it would be shorter/cleaner to use M_HASHTYPE_ISHASH(). See https://reviews.freebsd.org/D52989

In D55196#1262240, @tuexen wrote:

If we have some rules regarding how to set the hash type, can we document them somewhere? This way driver writers know what to do and we can inspect drivers and fix them.

I think it's driver bugs that have flown under the radar until the recent RSS change so I'm not sure this is the place to do this.

Thanks. This matches i.e. http://iommu.com/datasheets/ethernet/controllers-nics/intel/e1000/ethernet-controller-i350-datasheet.pdf pg.313-314.

Seems fine, assuming you have check the hw filter table size(s)

ack as long as the other review is ok

Light ack as the two init functions seem fine but I have not looked through the entirety to see if there was some reason for stopping the MAC (rdma?) and you should probably look at all the leafs of qlnx_load especially filter setup.

In D55137#1260494, @cc wrote:

In D55137#1260492, @kbowling wrote:

Looking at the linked change.. em can have two queues in one hw case. Does it matter?

My understanding is that if only one rx queue, RSS is not needed, so no hash value is needed. In a case of two rx queues, does not the hw enable RSS and provide RSS hash values?

qemu can emulate it as 'e1000e' https://github.com/qemu/qemu/blob/master/hw/net/e1000e.c

Looking at the linked change.. em can have two queues in one hw case. Does it matter?

Thanks this seems good to me

Seems fine

@ashafer is this something you can lift into the upstream driver?

Overall looks good to me.

Approving as mentor, wait for @manu to re-review before commiting

Just a cursory look on my end, approving for @kgalazka

I'm fine with this, would recommend awaiting a look from @imp though

In D53128#1213614, @ziaee wrote:

Can you guys update the handbook? Nvidia driver section is in doc/documentation/content/en/books/handbook/x11/. My laptop broke so I can't do it.

I think this would be useful for driver developers, but we'd need to document is suitably so people know to look for it.

Looks good thanks for chasing it down

The deletions seem fine. The convention is probably up to you if you are changing to this.

Holding until Tuesday on this, FreeBSD-ports-kmods still isn't receiving pkgs for nvidia-kmod and @bapt needs some time before looking at it.

There are two possible concerns that come to mind:

overall looks fine

@zlei do you know if all the consumer ifdi_get_counter implementations will still have been initialized correctly, for instance not making invalid references? It seems fine as long as there are no implicit initialization issues that relied on the current behavior.

Committed in 9dbf8452e1fbb8e075b0f8d978fe9be9d0df9355

In D50067#1188547, @kgalazka wrote:

Fixing Eric's comment about codename seemed as a trivial change so I tried to do it without engaging Yogesh and updating the review. I'm sorry, it looks like it was a poor idea. Commit wasn't automatically detected by Phabricator and this review is still open. Is there anything else I should do except for adding the commit with 'Edit Related Objects', and promising to not try to cut the corners in the future?

LGTM, it's out of the way aside from a branch and the change to iflib_encap (is this change strictly necessary for simple or can it be committed separately?).