Bug 4420 - (net tulip) Problem with 6 bit addressing in tulip_read_eeprom()
Summary: (net tulip) Problem with 6 bit addressing in tulip_read_eeprom()
Status: CLOSED CODE_FIX
Alias: None
Product: Drivers
Classification: Unclassified
Component: Network (show other bugs)
Hardware: i386 Linux
: P2 normal
Assignee: Grant Grundler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-03-30 04:01 UTC by Daniel Forsgren
Modified: 2008-09-22 12:07 UTC (History)
4 users (show)

See Also:
Kernel Version: 2.6.11.6
Tree: Mainline
Regression: ---


Attachments
Patch to mask address in tulip_read_eeprom() (510 bytes, patch)
2005-04-11 08:09 UTC, Daniel Forsgren
Details | Diff
Proposed tulip_read_eeprom patch sent to netdev today (1.62 KB, patch)
2008-03-23 22:18 UTC, Grant Grundler
Details | Diff

Description Daniel Forsgren 2005-03-30 04:01:09 UTC
(I posted this bug at http://sourceforge.net/projects/tulip/, before I
realized that that bug tracker was deprecated. So it probably belongs
here instead.)

Problem
=======

There seems to be a problem when running the tulip driver with 6 bit
eeprom address length:

The tulip_read_eeprom() function sets up a read command like:

    int read_cmd = location | (EE_READ_CMD << addr_len);

where location is the offset (within the eeprom) and addr_len is
either 6 or 8 bits. However, with addr_len == 6 and location > 0x3f,
the most significant location bits would overlap with the read command
itself.

Thus, with 6 bit addressing, tulip_read_eeprom() must not be called
with location > 0x3f. Unfortunately, in tulip_core.c, it's called with
location up to sizeof(tp->eeprom)/2, which should be 0x100 if I'm not
mistaken. In the same way de2104x.c uses locations up to
DE_EEPROM_WORDS, which is equal to 256 too.

How-to-fix
==========

Make sure that the location is masked corresponding to add_len in
tulip_read_eeprom().
Comment 1 Andrew Morton 2005-03-30 10:38:50 UTC
Please submit a patch to fix it?
Comment 2 Daniel Forsgren 2005-04-07 03:48:08 UTC
Ok. A patch is on it's way. I just need to do some testing first.
(it's really a one-liner as I described above -  just mask the address before
using it).
Comment 3 Daniel Forsgren 2005-04-11 08:09:56 UTC
Created attachment 4887 [details]
Patch to mask address in tulip_read_eeprom()

Added patch.
Comment 4 Adrian Bunk 2006-01-27 12:31:37 UTC
Jeff, can you review the patch in this bug?
Comment 5 Natalie Protasevich 2007-11-17 23:31:44 UTC
Any update on this please. Daniel, was the problem resolved for you in recent kernels? If not, maybe Grant can now take a look at it and the proposed patch.
Well, if Daniel is not around the bug will have to be closed unfortunately.
Thanks.
Comment 6 Grant Grundler 2007-11-18 10:34:26 UTC
Patch looks good to me and based on previous email, I expect it fixes the problem.
I'll verify against current linus tree and submit to akpm with signed-off-by line.

thanks!
Comment 7 Grant Grundler 2008-03-23 22:16:16 UTC
On second thought, patch isn't ok.
We should NOT read some other "random" register instead of returning an error value. I'll attached a patch that I just submitted upstream that does two things:
1) tulip_read_eeprom will return zero (since we can't return -EINVAL)
   if this is attempted (defensive programming).
2) In tulip_core.c, fix the tulip_read_eeprom caller so they don't
   iterate past addr_len bits and make sure the entire tp->eeprom[]
   array is cleared.

I'll attach the patch I submitted as well to this bug report.
Comment 8 Grant Grundler 2008-03-23 22:18:14 UTC
Created attachment 15410 [details]
Proposed tulip_read_eeprom patch sent to netdev today
Comment 9 Grant Grundler 2008-03-23 22:22:16 UTC
Perhaps this should be a separate bug, but de2104x.c has the same bug since it defines it's own copy of tulip_read_eeprom(). I suspect the right fix there would be to modify de2104x.c to use eeprom.c instead of keeping it's own copy of the same code. But that might not be a trivial change...and I don't have any de2104x cards to test with (I need to double check, but I don't think so offhand).
Comment 10 Alan 2008-09-22 12:07:24 UTC
Verified applied

Note You need to log in before you can comment on or make changes to this bug.