linux-kernel - Re: [PATCH 2/2] staging: slicoss: remove slic_reg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140519195233.GX15585@mwanda>
Date:	Mon, 19 May 2014 22:52:33 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	David Matlack <matlackdavid@...il.com>
Cc:	devel@...verdev.osuosl.org, gregkh@...uxfoundation.org,
	Lior Dotan <liodot@...il.com>, linux-kernel@...r.kernel.org,
	charrer@...critech.com
Subject: Re: [PATCH 2/2] staging: slicoss: remove slic_reg_params struct

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@...cle.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/