Hi,
On Fri, Jan 04, 2008 at 04:43:57AM +0100, Björn Steinbrink wrote:
> On 2008.01.03 01:42:09 +0200, Adrian Bunk wrote:
> > [ original bug report: http://lkml.org/lkml/2008/1/2/253 ]
> >
> > On Wed, Jan 02, 2008 at 10:48:43PM +0100, Andreas Mohr wrote:
> > > The nv_probe() function then nicely obeys this flag by reversing the
> > > address if needed, _however_ there's another function,
> > > nv_copy_mac_to_hw(), which does _NOT_ obey it and simply copies the
> > > _raw_ netdev dev_addr to the card register (NvRegMacAddrA etc.).
>
> nv_copy_mac_to_hw() is called from nv_probe() _after_ the MAC address
> has been fixed, and from nv_set_mac_address() which is supposed to get a
> correct MAC address anyway. So I don't see any problem there.
/* set permanent address to be correct aswell */
... orig_mac fumbling ...
Why, then, does this driver do such a nice hack as:
/* special op: write back the misordered MAC address - otherwise
* the next nv_probe would see a wrong address.
*/
writel(np->orig_mac[0], base + NvRegMacAddrA);
writel(np->orig_mac[1], base + NvRegMacAddrB);
I really don't see why this driver needs to do these things in such a
messy way.
One thing is for certain: netdev->dev_addr is always in operating system
order, and order should always be handled properly when copying to/from
hardware.
Such a driver needs exactly *one* central helper method
to reverse an input MAC address in 6 bytes u8 format
(which could arguably be added to include/linux/etherdevice.h even,
since a wee bit too many devices seem to be having trouble
with wrongly ordered MAC addresses).
Then it needs exactly *one* function for I/O register access
to read a MAC address from a device and exactly *one* function
to write it back (both doing raw, unsorted format transfers only,
and possibly as inline function).
And then it needs these card I/O functions wrapped into two functions which
interface with driver- and OS-related MAC variables
(struct variables ALWAYS stored in usual system order, NOT H/W order!!!!!!)
which optionally reverse the address (if needed for a particular card).
And then there will never be any confusion about any MAC address format
anywhere any more, right?
I really don't mean to sound cranky, but this driver did ask for trouble,
and lots of it ;)
Thank you for your reply, and especially thank you for this very fast
patch!
I might even have enough time this weekend to work on this...
Andreas Mohr
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
