Re: [PATCH 2/2] staging: slicoss: remove slic_reg_params struct

[Dan Carpenter <dan.carpenter@oracle.com>]

On Mon, May 19, 2014 at 08:57:49AM -0700, David Matlack wrote:
> On Mon, May 19, 2014 at 2:21 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > Looking at the patch, this looks like a bugfix but the changelog doesn't
> > give any clues.
> 
> Yeah this isn't a bug fix.
> 
> The only member of struct slic_reg_params that was in use was fail_on_bad_eeprom
> (implicitly set to zero by alloc_etherdev -> kzalloc). Since the previous patch
> in this series fixes the eeprom checksum, we can remove this struct entirely.
> 
> If we do want the feature of ignoring a corrupt/bad eeprom, a module param
> would work better anyway.

I think we are talking at cross purposes.

-               if ((!card->config.EepromValid) &&
-                   (adapter->reg_params.fail_on_bad_eeprom)) {
+               if (!card->config.EepromValid) {
                        slic_reg64_write(adapter, &slic_regs->slic_isp, 0,

In the original code then this if condition is never true because
->fail_on_bad_eeprom is zero.

You are saying that this condition is still not true because
->EepromValid is true now you fixed the checksum code.  I am saying
that *sometimes* it *could* be true if the eeprom is corrupt.

You have to understand that I review a lot of staging patches every day.
Most patches try to remove struct members but the code should still work
exactly as it did before (a clean up).  Some patches remove struct
members and the behavior changes.  Hopefully it's deliberate and the
changelog mentions that it is a bug fix.  If it changes the run time,
and not deliberately then that's a bug.

This code, your code, is a bugfix.  Thanks.  :)  Next time mention then
that you fixing stuff in changelog.

regards,
dan carpenter