Re: [PATCH] [MTD] [RFC] New Solarflare NIC EEPROM/Flash driver

[Ben Hutchings <bhutchings@solarflare.com>]

I didn't originally write this driver, but I was the last person to
make significant changes to it.

Jörn Engel wrote:
> On Thu, 10 January 2008 18:51:13 +0000, Robert Stonehouse wrote:
> > 
> > In the last submission that we made to linux-netdev it was requested that
> > people knowledgeable about MTD drivers look over the code ... so I am sure
> >   I am in the right place
> 
> Not quite a review, just a couple of things that stuck out.
> 
>   Even if the patch is too large, appending the relevant hunk for the
>   mtd driver would have been appreciated.

Sorry about that.  We could have posted just mtd.c, but that would
have been missing the pieces inside the net driver that it depends on.

>   The prefix "efx_mtd_" causes me personally some trouble.  By the
>   time I have reached the relevant part of the name, my brain has
>   already fallen half asleep.  But maybe I'm just overreacting after
>   working with in-house code for five years.
> 
>   I have no idea why you need eraseregions, if there is just one.
>   Kill them?

Earlier versions of this driver supported our first controller and
NICs, EF1/EF1002, which did have multiple regions of flash.  I
simplified the code after this support was removed, but I wasn't aware
that we could avoid specifying eraseregions at all.

>   How many of the EFX_LOG() statements are still useful?  They may
>   have initially helped writing the code, but today they hurt people
>   reading the code.
>   As a general rule, if you cannot give a good reason why this
>   particular log statement is needed in 20s, there usually is none and
>   the code can get axed.

You're right, it is a bit verbose.

>   efx_mtd->dead is fun.  Does this still happen with production
>   hardware?

It shouldn't happen with anything that passed manufacturing tests.
This is playing safe.

>   Even if it does, instead of setting the flag and checking it in
>   every function, you could replace the operations with
>   dead_device_operations that simply return -EIO for every call.
>
>   struct semaphore access_lock; should become a mutex.

Right.  We've tended to be quite conservative in using newer kernel
features, since we also need to support old kernels, but we have a
good backward-compatibility layer now (unifdef'd out of the submitted
code) so this shouldn't be a problem.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.