Re: [PATCH] [MTD] [RFC] New Solarflare NIC EEPROM/Flash driver (2nd try)

[Ben Hutchings <bhutchings@solarflare.com>]

Jörn Engel wrote:
> On Mon, 14 January 2008 17:04:00 +0000, Ben Hutchings wrote:
> > 
> > This patch contains just the MTD driver code (mtd.c) and the two most
> > important header files it shares with our net driver.  The low-level
> > code to access the SPI bus through the network controller is not
> > included (and is unchanged from last time aside from a small change to
> > length validation).  Hopefully this should be more amenable to review,
> > though it cannot be compiled.
> 
> Hehe.  Maybe next time I can get both?  
> 
> Most comments below are fairly generic and can be applied many times
> over, both for mtd.c and the rest.
<snip>
> > +struct efx_mtd {
> > +	struct mtd_info mtd;
> > +	struct mtd_partition mtd_part[EFX_MAX_PARTITIONS];
> > +	struct semaphore access_lock;
> > +	char part_name[EFX_MAX_PARTITIONS][32];
> > +	char name[32];
> > +	struct efx_dl_device *efx_dev; /* driverlink */
> 
> Rename to struct efx_driver_link and kill the comment?

No, there are multiple structures involved in the driverlink API.  But
the "_dl_" in the structure name should be a sufficient clue.

> > +	struct efx_nic *efx; 
> > +	struct efx_spi_device *spi;
> 
> Needing three seperate pointers to some struct efc_* is... interesting.
> Usually one would be enough and the other two can be derived.

The efx_nic pointer can be derived from the efx_dev pointer, though
looking it up requires a function call.  The efx_spi_device pointer is
not redundant, because our NICs usually have 2 SPI devices.

> > +		/* Check contents */
> > +		for (i = 0; i < buf_len; i++) {
> > +			if (buffer)
> > +				expected = buffer[i];
> > +			if (verify_buffer[i] != expected) {
> > +				EFX_ERR(efx_mtd->efx, "%s offset %lx contains "
> > +					"%02x, should be %02x\n", efx_mtd->name,
> > +					(unsigned long)(offset + i),
> > +					verify_buffer[i], expected);
> > +				rc = -EIO;
> > +				goto out;
> > +			}
> > +		}
> 
> Home-brewn memcmp?

:-)  This reports non-matching addresses, and can compare with all-
ones rather than an explicit byte array (if the buffer pointer is NULL)
which we use after an erase.

<snip>
> > +static int efx_mtd_erase(struct mtd_info *mtd, struct erase_info *erase)
> > +{
> > +	struct efx_mtd *efx_mtd = (struct efx_mtd *)mtd->priv;
> > +	struct efx_spi_device *spi;
> > +	int rc;
> > +
> > +	rc = down_interruptible(&efx_mtd->access_lock);
> 
> What is this semaphore protecting?

It prevents efx_mtd->spi changing underneath us.

<snip> 
> My gut instinct tells me that you can push it through to only protect
> the pure bus access

We already have that for single comamnds, since the net driver reads
the SPI devices.  The access lock is needed to ensure that a sequence
of commands involved in writing is not interrupted, and to exclude a
reset which could interfere with access to the SPI device.

<snip>
> > +out:
> > +	if (rc == 0) {
> > +		erase->state = MTD_ERASE_DONE;
> > +	} else {
> > +		erase->state = MTD_ERASE_FAILED;
> > +		erase->fail_addr = 0xffffffff;
> 
> ???

According to mtd.h:

/* If the erase fails, fail_addr might indicate exactly which block failed.  If
   fail_addr = 0xffffffff, the failure was not at the device level or was not
   specific to any particular block. */

I read that as meaning we must set fail_addr.  Of course it would be
nicer to set it to a meaningful address.

<snip>
> > +static void efx_mtd_sync(struct mtd_info *mtd)
> > +{
> > +	struct efx_mtd *efx_mtd = (struct efx_mtd *)mtd->priv;
> > +	int rc;
> > +
> > +	down(&efx_mtd->access_lock);
> > +	rc = efx_spi_slow_wait(efx_mtd);
> > +	if (rc != 0)
> > +		EFX_ERR(efx_mtd->efx, "%s sync failed (%d)\n",
> > +			efx_mtd->name, rc);
> 
> How do you handle -EINTR?

Obviously it doesn't.  Given that it can't return an error, do you
have any better suggestions?

<snip>
> > +	/* Initialise structure */
> > +	efx_mtd->efx_dev = efx_dev;
> > +	efx_mtd->efx = efx;
> > +	efx_mtd->spi = spi;
> > +	sema_init(&efx_mtd->access_lock, 1);
> > +	memcpy(&efx_mtd->mtd, template, sizeof(efx_mtd->mtd));
> 
> I'm not too fond of this memcpy approach.  In particular because this
> function is called probe and does no such thing.
> 
> Instead this function could allocate a structure and initialize any
> common fields.  Remaining fields would then be initialized in the two
> actual probe functions below.

Right.  The memcpy() is there because the "template" structures used
to be static data.  I replaced them with dynamically generated
structures when adding support for more SPI devices, but didn't follow
through and rework this initialisation.

<snip>
> > +	EFX_ASSERT(template->numeraseregions == 0);
> > +
> > +	EFX_ASSERT(partitions != NULL);
> > +	EFX_ASSERT(num_partitions > 0);
> > +	EFX_ASSERT(num_partitions <= EFX_MAX_PARTITIONS);
> 
> And I assume most of these assertions are bogus and can be removed.

I suppose they are trivially true now.

Ben.

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