Bug 4420

Summary:	(net tulip) Problem with 6 bit addressing in tulip_read_eeprom()
Product:	Drivers	Reporter:	Daniel Forsgren (daniel.forsgren)
Component:	Network	Assignee:	Grant Grundler (grundler)
Status:	CLOSED CODE_FIX
Severity:	normal	CC:	bunk, grundler, kyle, protasnb
Priority:	P2
Hardware:	i386
OS:	Linux
Kernel Version:	2.6.11.6	Subsystem:
Regression:	---	Bisected commit-id:
Attachments:	Patch to mask address in tulip_read_eeprom() Proposed tulip_read_eeprom patch sent to netdev today

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