Page MenuHomeFreeBSD

Remove unnecessary variable for NVM checksum
AbandonedPublic

Authored by cramerj_intel.com on Jun 23 2016, 6:11 PM.

Details

Reviewers
sbruno
Group Reviewers
Intel Networking
Summary

We don't need the checksum itself, just the yay/nay that it was correct.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4322
Build 4367: arc lint + arc unit

Event Timeline

cramerj_intel.com retitled this revision from to Remove unnecessary variable for NVM checksum.
cramerj_intel.com updated this object.
cramerj_intel.com edited the test plan for this revision. (Show Details)
cramerj_intel.com set the repository for this revision to rS FreeBSD src repository - subversion.
sbruno requested changes to this revision.Jun 28 2016, 4:31 PM
sbruno edited edge metadata.

Hrm ... I did a review of the code path here. the csum variable is a holder for the user to examine the checksum from the various functions. It *is* used as far as I can tell if non-NULL.

Is this your understanding as well?

This revision now requires changes to proceed.Jun 28 2016, 4:31 PM

The csum variable holds the checksum, yes. But nothing in attach() uses it afterwards. We only care about the return value of the function. I think the csum variable was added originally because ixgbe_validate_eeprom_checksum(), at that time, required a non-NULL input parameter...maybe? But then over time it was enhanced to allow a NULL parameter for drivers that didn't care about the csum value itself, but just the pass/fail return value of the function. In other words, ixgbe_validate_eeprom_checksum() will only populate "csum" if it's non-NULL. Grep for "checksum_val" to see what I mean.

I don't have a problem leaving the variable in if there was something to use it. Right now, nothing uses it. Is viewing the checksum something users are interested in?

sbruno edited edge metadata.

Its never exposed to userland and its not used in the driver in any code path. I don't know that anyone is using it, and I think its safe to change.

This revision is now accepted and ready to land.Jun 28 2016, 6:22 PM

Jeb:

What is your expectation with this review? Was Eric or I going to do something with it? If its not going to be committed, please abandon/close.

I'm going to abandon/close it today after I ask Eric how to do that. :) This patch will show up again soon, but along with the rest of the changes I'm adding.

Abandoning for now. Will resubmit this patch along with many more changes in a different review.