Re: + e1000e-prevent-corruption-of-eeprom-nvm.patch added to -mm tree

[Ben Hutchings <bhutchings@solarflare.com>]

Zachary Amsden wrote:
> On Tue, 2008-09-02 at 14:58 -0700, Brandeburg, Jesse wrote:
[...]
> > VMWare issues really do not apply to the Linux kernel, and unless the
> > architecture of ethtool->ioctl->rtnl->e1000e has changed without me
> > noticing.  In that case every driver that used ethtool would be broken.
> 
> No, any drivers that have a state machine consisting of a base register
> that is used to index which datum is read from device EEPROM by the data
> register would be broken.
> 
> This is not a VMware issue, its a fundamental hardware synchronization
> issue caused by the dependence of data read/write on a device register.
> Does the net layer protect against e1000 issuing simultaneous EEPROM
> read / write?

All ethtool commands are serialised by the RTNL as Jesse said above.

> I have no idea.  It probably should.  Should the driver
> take extra precautions of its own to ensure this on non hot-paths, to
> avoid corrupting EEPROM memory and resulting in a non-functional NIC?
> 
> Absolutely.
> 
> From casual inspection it looks like set_mtu is done under dev_baselock, while ethtool is done under rtnl_lock.
> 
> Setting the MTU internally reads the EEPROM, which could contend with and corrupt and eeprom operations.
 
No, MTU changes are done by another ioctl which takes the RTNL.  It's
not hard to find this stuff in net/core/dev.c.

Please do VMware driver developers a favour and pay a little more
attention to such subtleties.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.