Page MenuHomeFreeBSD

Remove unnecessary variable for NVM checksum

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


Group Reviewers
Intel Networking

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

Diff Detail

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

Event Timeline retitled this revision from to Remove unnecessary variable for NVM checksum. updated this object. edited the test plan for this revision. (Show Details) 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


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.