Looks ok, thanks.

It can be merged there (MFC'd) as well, after a period of a minimum of 3 days. There is no need to submit a separate patch.

Will the fix be provided as a firmware upgrade or how?
I could commit this change if you don't have someone else around that can do that for you.

Wouldn't it be easier to always perform the complete phy reset if this is a known hardware issue? Or maye perform it automatically after a normal reset does not work?
I'm afraid that adding an obscure sysctl (obscure for anyone except for the axgbe developers) would be of little use. Unaware users wouldn't have a clue about what to do...
just my 2 cents

Shall I commit this change?

Looks good to me.

I added some comments, mostly about readability.
If you tested all the valid combinations of sph_enable and netmap/non-netmap I don't have any objection in this being merged.

Il giorno dom 10 gen 2021 alle ore 13:20 rajesh1.kumar_amd.com (Rajesh
Kumar) <phabric-noreply@freebsd.org> ha scritto:

Thanks to both for the clarification.

Il giorno ven 8 gen 2021 alle ore 10:37 rajesh1.kumar_amd.com (Rajesh
Kumar) <phabric-noreply@freebsd.org> ha scritto:

In D27797#626328, @rajesh1.kumar_amd.com wrote:

Are both netmap_test and sph_enable actually necessary? Would it make sense to have only sph_enable, and set isc_nfl = 1 when sph_enable = 0 and isc_nfl = 2 when sph_enable = 1?
If not, I think netmap_test should be called something like single_free_list

I agree, both sysctl variables now depend on each other. I will create a change and submit it for review here once I find the time.

@stephan.dewt_yahoo.co.uk, I have already made the changes for this and about to submit it.

@vmaffione, I feel it's better to retain both the variables, since it will give some flexibility to the user to try disabling split header even with non-netmap cases if needed. But as you suggested, I have modified the "netmap_test" variable to "single_fl".

But if you people see no usage for the same, we shall remov it.

Are both netmap_test and sph_enable actually necessary? Would it make sense to have only sph_enable, and set isc_nfl = 1 when sph_enable = 0 and isc_nfl = 2 when sph_enable = 1?
If not, I think netmap_test should be called something like single_free_list

Il giorno mer 6 gen 2021 alle ore 20:16 rajesh1.kumar_amd.com (Rajesh
Kumar) <phabric-noreply@freebsd.org> ha scritto:

Il giorno mer 6 gen 2021 alle ore 20:01 rajesh1.kumar_amd.com (Rajesh
Kumar) <phabric-noreply@freebsd.org> ha scritto:

@rajesh1.kumar_amd.com
Thanks for testing, I'll push this right away.

Good catch, thanks.
I think the existing line 113 (copying the NS_MOREFRAG flag) was never tested, because it copies the flag the other way around.

Il giorno mar 5 gen 2021 alle ore 07:55 rajesh1.kumar_amd.com (Rajesh
Kumar) <phabric-noreply@freebsd.org> ha scritto:

Thanks.
I tested it on my side on em(3) and vmx(3) in a VM to look for regressions.
What kind of tests have you done (I guess you are using axgbe)?
Have you tried the pkt-gen -F and -M options on the transmitter side (pkt-gen supports multifrag), and see if a pkt-gen receiver receives the correct traffic?
Do you want me to commit this change?

LGTM
Thanks!

I'd say let's go step by step.
First we should merge this patch, which makes sense as is (unless @grehan has some more observations)?
Then we can talk about the other issue in a separate review.

I agree with @grehan , but see my comments.

Not familiar with the code, but it looks ok to me.

Although the change seems legit, it has been reported to cause hangs with netmap pkt-gen.
See:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=248652 (from comment 29)
Any idea why this may be the case?
In stable/11, the same logic (two report flags set for every ring) does not cause hangs.

Load the correct patch.

Added assert in iflib_fl_refill().
Improved comment.
Removed assert() at first refill.