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

["Jörn Engel" <joern@logfs.org>]

On Tue, 15 January 2008 17:35:50 +0000, Ben Hutchings wrote:
> > >
> > > +		/* 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.

I have this piece of code for similar purposes.  Notice the first
comment. :)

/*
 * TODO: move to lib/string.c
 */
/**
 * memchr_inv - Find a character in an area of memory.
 * @s: The memory area
 * @c: The byte to search for
 * @n: The size of the area.
 *
 * returns the address of the first character other than @c, or %NULL
 * if the whole buffer contains just @c.
 */
void *memchr_inv(const void *s, int c, size_t n)
{
	const unsigned char *p = s;
	while (n-- != 0) {
		if ((unsigned char)c != *p++) {
			return (void *)(p - 1);
		}
	}
	return NULL;
}

> <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.

Ok.  I will ignore the semaphore from now on.  Rest assured that someone
else will ask about it later.

> <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.

Hmm.  fail_addr is only used by jffs2 to mark blocks as bad, with
0xffffffff indicating that no block is to blame and nothing should be
marked bad.  Not the nicest interface I have ever seen, but your usage
of it seems proper.

> <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?

Can you prevent efx_spi_slow_wait from returning -EINTR?  If you can't,
a BUG_ON() at least makes the problem explicit.  

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

:)

Jörn

-- 
Data dominates. If you've chosen the right data structures and organized
things well, the algorithms will almost always be self-evident. Data
structures, not algorithms, are central to programming.
-- Rob Pike