New Solarflare NIC EEPROM/Flash driver

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

Quoted 4 lines · Robert Stonehouse

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.

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?

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.

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

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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

Quoted 2 lines

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.

Fair enough. Then I would prefer the dead_device_operations approach.

Quoted 6 lines

Quoted 5 lines

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.

Quoted 4 lines

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.

Ok. A straight conversion to a mutex will likely cause trouble with your reset routine. Not sure what to do here.

It turns out that struct efx_mtd_operations is a redundant indirection layer, but I can introduce a dummy struct efx_spi_device which does the same thing.

Quoted 2 lines

Ok. A straight conversion to a mutex will likely cause trouble with your reset routine. Not sure what to do here.

We can use a mutex if the net driver is guaranteed to call reset_suspend() and reset_resume() in the same context. This is currently true (they are always called in a pair by efx_reset() in efx.c) but I'm not sure we want to guarantee that. Unfortunately it *is* sometimes necessary to reset the controller after it's been exposed through driverlink, so this is not something we can ignore.

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

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.

Quoted 3 lines

Ok. A straight conversion to a mutex will likely cause trouble with your reset routine. Not sure what to do here.

We can use a mutex if the net driver is guaranteed to call reset_suspend() and reset_resume() in the same context. This is currently true (they are always called in a pair by efx_reset() in efx.c) but I'm not sure we want to guarantee that.

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.

net/sfc/mtd.c new file +15 -0

@@ -0,0 +1,655 @@
+/****************************************************************************
+ * Driver for Solarflare network controllers
+ *           (including support for SFE4001 10GBT NIC)
+ *
+ * Copyright 2005-2006: Fen Systems Ltd.
+ * Copyright 2006-2008: Solarflare Communications Inc,
+ *                      9501 Jeronimo Road, Suite 250,
+ *                      Irvine, CA 92618, USA
+ *
+ * Initially developed by Michael Brown <[email protected]>
+ * Maintained by Solarflare Communications <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation, incorporated herein by reference.

net/sfc/mtd.c new file +31 -0

+ */
+
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include <linux/delay.h>
+
+#define EFX_DRIVER_NAME "sfc_mtd"
+#include "driverlink_api.h"
+#include "net_driver.h"
+#include "spi.h"
+
+/*
+ * Flash and EEPROM (MTD) device driver
+ *
+ * This file provides a separate kernel module (sfc_mtd) which
+ * exposes the flash and EEPROM devices present on Solarflare NICs as
+ * MTD devices, enabling you to reflash the boot ROM code (or use the
+ * remaining space on the flash as a jffs2 filesystem, should you want
+ * to do so).
+ */
+
+#define EFX_MTD_VERIFY_BUF_LEN 4096
+#define EFX_MAX_PARTITIONS 2
+#define EFX_FLASH_BOOTROM_OFFSET 0x8000U
+
+/* Write enable for EEPROM/Flash configuration area
+ *
+ * Normally, writes to parts of non-volatile storage which contain
+ * critical configuration are disabled to prevent accidents.  This
+ * parameter allows enabling of such writes.

net/sfc/mtd.c new file +23 -0

+			return rc;
+		if (!(status & SPI_STATUS_NRDY))
+			return 0;
+	}
+	EFX_ERR(efx_mtd->efx, "timed out waiting for %s last status=0x%02x\n",
+		efx_mtd->name, status);
+	return -ETIMEDOUT;
+}
+
+static int efx_spi_slow_wait(struct efx_mtd *efx_mtd)
+{
+	struct efx_spi_device *spi = efx_mtd->spi;
+	u8 status;
+	int rc, i;
+
+	/* Wait up to 4s for flash/EEPROM to finish a slow operation. */
+	for (i = 0; i < 40; i++) {
+		schedule_timeout_interruptible(HZ / 10);
+		rc = spi->read(spi, efx_mtd->efx, SPI_RDSR, -1,
+			       &status, sizeof(status));
+		if (rc != 0)
+			return rc;
+		if (!(status & SPI_STATUS_NRDY))

net/sfc/mtd.c new file +53 -0

+static int efx_spi_unlock(struct efx_mtd *efx_mtd)
+{
+	struct efx_spi_device *spi = efx_mtd->spi;
+	const u8 unlock_mask = (SPI_STATUS_BP2 | SPI_STATUS_BP1 |
+				     SPI_STATUS_BP0);
+	u8 status;
+	int rc;
+
+	rc = spi->read(spi, efx_mtd->efx, SPI_RDSR, -1, &status,
+		       sizeof(status));
+	if (rc != 0)
+		return rc;
+
+	if (!(status & unlock_mask))
+		return 0; /* already unlocked */
+
+	rc = efx_spi_write_enable(efx_mtd);
+	if (rc != 0)
+		return rc;
+	rc = spi->write(spi, efx_mtd->efx, SPI_SST_EWSR, -1, NULL, 0);
+	if (rc != 0)
+		return rc;
+
+	status &= ~unlock_mask;
+	rc = spi->write(spi, efx_mtd->efx, SPI_WRSR, -1, &status,
+			sizeof(status));
+	if (rc != 0)
+		return rc;
+	rc = efx_spi_fast_wait(efx_mtd);
+	if (rc != 0)
+		return rc;
+
+	return 0;
+}
+
+/* Dummy device used in case of a failed reset. */
+
+static int efx_spi_dummy_read(const struct efx_spi_device *spi,
+			      struct efx_nic *efx, unsigned int command,
+			      int address, void *data, unsigned int len)
+{
+	return -EIO;
+}
+
+static int efx_spi_dummy_write(const struct efx_spi_device *spi,
+			       struct efx_nic *efx, unsigned int command,
+			       int address, const void *data, unsigned int len)
+{
+	return -EIO;
+}
+
+static struct efx_spi_device efx_spi_dummy_device = {
+	.block_size	= 1,

net/sfc/mtd.c new file +35 -0

+static int efx_mtd_read(struct mtd_info *mtd, loff_t start, size_t len,
+			size_t *retlen, u8 *buffer)
+{
+	struct efx_mtd *efx_mtd = (struct efx_mtd *)mtd->priv;
+	struct efx_spi_device *spi;
+	unsigned int command;
+	unsigned int block_len;
+	unsigned int pos = 0;
+	int rc;
+
+	rc = down_interruptible(&efx_mtd->access_lock);
+	if (rc != 0)
+		goto out;
+	spi = efx_mtd->spi;
+
+	while (pos < len) {
+		block_len = min((unsigned int)len - pos,
+				efx_spi_read_limit(spi, start + pos));
+		command = efx_spi_munge_command(spi, SPI_READ, start + pos);
+		rc = spi->read(spi, efx_mtd->efx, command, start + pos,
+			       buffer + pos, block_len);
+		if (rc != 0)
+			goto out_up;
+		pos += block_len;
+
+		/* Avoid locking up the system */
+		cond_resched();
+		if (signal_pending(current))
+			return -EINTR;
+	}
+
+out_up:
+	up(&efx_mtd->access_lock);
+out:
+	*retlen = pos;

net/sfc/mtd.c new file +12 -0

+	u8 *verify_buffer = NULL;
+	size_t buf_len;
+	size_t read_len;
+	int i;
+	u8 expected = 0xff;
+	int rc = 0;
+
+	verify_buffer = kmalloc(EFX_MTD_VERIFY_BUF_LEN, GFP_KERNEL);
+	if (!verify_buffer) {
+		rc = -ENOMEM;
+		goto out;
+	}

net/sfc/mtd.c new file +11 -0

+
+		if (buffer)
+			buffer += buf_len;
+		offset += buf_len;
+		len -= buf_len;
+	}
+	EFX_ASSERT(len == 0);
+
+out:
+	kfree(verify_buffer);
+

net/sfc/mtd.c new file +11 -0

+	if (rc != 0)
+		goto out_up;
+	rc = efx_spi_write_enable(efx_mtd);
+	if (rc != 0)
+		goto out_up;
+	rc = spi->write(spi, efx_mtd->efx, spi->erase_command, erase->addr,
+			NULL, 0);
+	if (rc != 0)
+		goto out_up;
+	rc = efx_spi_slow_wait(efx_mtd);
+

net/sfc/mtd.c new file +59 -0

+	}
+	mtd_erase_callback(erase);
+	return rc;
+}
+
+static int efx_mtd_write(struct mtd_info *mtd, loff_t start,
+			 size_t len, size_t *retlen, const u8 *buffer)
+{
+	struct efx_mtd *efx_mtd = (struct efx_mtd *)mtd->priv;
+	struct efx_spi_device *spi;
+	unsigned int command;
+	unsigned int block_len;
+	unsigned int pos = 0;
+	int rc;
+
+	rc = down_interruptible(&efx_mtd->access_lock);
+	if (rc != 0)
+		goto out;
+	spi = efx_mtd->spi;
+
+	rc = efx_spi_unlock(efx_mtd);
+	if (rc != 0)
+		goto out_up;
+
+	while (pos < len) {
+		rc = efx_spi_write_enable(efx_mtd);
+		if (rc != 0)
+			goto out_up;
+
+		block_len = min((unsigned int)len - pos,
+				efx_spi_write_limit(spi, start + pos));
+		command = efx_spi_munge_command(spi, SPI_WRITE, start + pos);
+		rc = spi->write(spi, efx_mtd->efx, command, start + pos,
+				buffer + pos, block_len);
+		if (rc != 0)
+			goto out_up;
+		pos += block_len;
+
+		rc = efx_spi_fast_wait(efx_mtd);
+		if (rc != 0)
+			goto out_up;
+
+		/* Avoid locking up the system */
+		cond_resched();
+		if (signal_pending(current)) {
+			rc = -EINTR;
+			goto out_up;
+		}
+	}
+
+out_up:
+	up(&efx_mtd->access_lock);
+	if (rc == 0)
+		rc = efx_mtd_verify(mtd, start, len, buffer);
+out:
+	*retlen = pos;
+	return rc;
+}
+

net/sfc/mtd.c new file +55 -0

+/* Prepare for device reset */
+static void efx_mtd_reset_suspend(struct efx_dl_device *efx_dev)
+{
+	struct efx_mtd *efx_mtd = efx_dev->priv;
+
+	if (!efx_mtd)
+		return;
+
+	/* Acquire lock to ensure that any in-progress operations have
+	 * completed, and no new ones can start.
+	 */
+	down(&efx_mtd->access_lock);
+}
+
+/* Resume after device reset */
+static void efx_mtd_reset_resume(struct efx_dl_device *efx_dev, int ok)
+{
+	struct efx_mtd *efx_mtd = efx_dev->priv;
+
+	if (!efx_mtd)
+		return;
+
+	/* If device reset failed already, or SPI device doesn't
+	 * become ready, disable device.
+	 */
+	if (!ok || efx_spi_slow_wait(efx_mtd) != 0) {
+		efx_mtd->spi = &efx_spi_dummy_device;
+		EFX_ERR(efx_mtd->efx, "%s disabled after failed reset\n",
+			efx_mtd->name);
+	}
+
+	up(&efx_mtd->access_lock);
+}
+
+static void efx_mtd_remove(struct efx_dl_device *efx_dev)
+{
+	struct efx_mtd *efx_mtd = efx_dev->priv;
+
+	del_mtd_partitions(&efx_mtd->mtd);
+	kfree(efx_mtd);
+	efx_dev->priv = NULL;
+}
+
+static int __devinit
+efx_mtd_probe(struct efx_dl_device *efx_dev, struct efx_nic *efx,
+	      struct efx_spi_device *spi,
+	      const char *mtd_type_name, struct mtd_info *template,
+	      struct mtd_partition *partitions, unsigned int num_partitions)
+{
+	struct efx_mtd *efx_mtd;
+	size_t s = sizeof(efx_mtd->name);
+	int rc;
+	int i;
+
+	efx_mtd = kzalloc(sizeof(*efx_mtd), GFP_KERNEL);

net/sfc/mtd.c new file +6 -0

+	if (snprintf(efx_mtd->name, s, "%s %s", efx_mtd->efx->name,
+		     mtd_type_name) >= s)
+		goto err_len;
+
+	efx_mtd->mtd.priv = efx_mtd;
+	efx_mtd->mtd.name = efx_mtd->name;

net/sfc/mtd.c new file +127 -0

+	s = sizeof(efx_mtd->part_name[0]);
+
+	for (i = 0; i < num_partitions; i++) {
+		memcpy(&efx_mtd->mtd_part[i], &partitions[i],
+		       sizeof(efx_mtd->mtd_part[i]));
+		if (snprintf(efx_mtd->part_name[i], s, "%s %s",
+			     efx_mtd->efx->name, partitions[i].name) >= s)
+			goto err_len;
+
+		efx_mtd->mtd_part[i].name = efx_mtd->part_name[i];
+		if (efx_allow_nvconfig_writes != 0)
+			efx_mtd->mtd_part[i].mask_flags &= ~MTD_WRITEABLE;
+	}
+
+	rc = add_mtd_partitions(&efx_mtd->mtd, efx_mtd->mtd_part,
+				num_partitions);
+	if (rc != 0)
+		goto err;
+
+	return 0;
+
+ err_mem:
+	rc = -ENOMEM;
+	goto err;
+ err_len:
+	rc = -ENAMETOOLONG;
+ err:
+	efx_mtd_remove(efx_dev);
+	return rc;
+}
+
+static int __devinit
+efx_flash_probe(struct efx_dl_device *efx_dev,
+		const struct net_device *net_dev,
+		const struct efx_dl_device_info *dev_info,
+		const char *silicon_rev)
+{
+	struct efx_nic *efx = efx_dl_get_nic_port(efx_dev)->efx;
+	struct mtd_partition partitions[2] = {};
+	unsigned int num_partitions;
+	struct mtd_info template = {
+		.numeraseregions = 0,
+		.type = MTD_NORFLASH,
+		.flags = MTD_CAP_NORFLASH,
+		.writesize = 1
+	};
+
+	if (!efx->spi_flash)
+		return -EINVAL;
+
+	template.size = efx->spi_flash->size;
+	template.erasesize = efx->spi_flash->erase_size;
+
+	partitions[0].name = "sfc_flash_config";
+	partitions[0].offset = 0;
+	partitions[0].size = min(efx->spi_flash->size,
+				 EFX_FLASH_BOOTROM_OFFSET);
+	partitions[0].mask_flags = MTD_WRITEABLE;
+
+	if (efx->spi_flash->size <= EFX_FLASH_BOOTROM_OFFSET) {
+		num_partitions = 1;
+	} else {
+		/* Expansion ROM image */
+		partitions[1].name = "sfc_flash_bootrom";
+		partitions[1].offset = EFX_FLASH_BOOTROM_OFFSET;
+		partitions[1].size = (efx->spi_flash->size
+				      - EFX_FLASH_BOOTROM_OFFSET);
+		num_partitions = 2;
+	}
+
+	return efx_mtd_probe(efx_dev, efx, efx->spi_flash,
+			     "sfc_flash", &template,
+			     partitions, num_partitions);
+}
+
+static struct efx_dl_driver efx_flash_driver = {
+	.name		= "sfc_flash",
+	.probe		= efx_flash_probe,
+	.remove		= efx_mtd_remove,
+	.reset_suspend	= efx_mtd_reset_suspend,
+	.reset_resume	= efx_mtd_reset_resume,
+};
+
+static int __devinit
+efx_eeprom_probe(struct efx_dl_device *efx_dev,
+		 const struct net_device *net_dev,
+		 const struct efx_dl_device_info *dev_info,
+		 const char *silicon_rev)
+{
+	struct efx_nic *efx = efx_dl_get_nic_port(efx_dev)->efx;
+	struct mtd_partition partitions[1] = {};
+	struct mtd_info template = {
+		.numeraseregions = 0,
+		.type = MTD_DATAFLASH,
+		.flags = MTD_WRITEABLE,
+		.writesize = 1
+	};
+	const char *mtd_type_name;
+
+	if (!efx->spi_eeprom)
+		return -EINVAL;
+
+	template.size = efx->spi_eeprom->size;
+	template.erasesize = efx->spi_eeprom->size;
+
+	partitions[0].offset = 0;
+	partitions[0].size = efx->spi_eeprom->size;
+	partitions[0].mask_flags = MTD_WRITEABLE;
+
+	if (efx->spi_eeprom->size <= 0x200) {
+		mtd_type_name = "sfc_small_eeprom";
+		partitions[0].name = "sfc_small_config";
+	} else {
+		mtd_type_name = "sfc_large_eeprom";
+		partitions[0].name = "sfc_large_config";
+	}
+
+	return efx_mtd_probe(efx_dev, efx, efx->spi_eeprom,
+			     mtd_type_name, &template,
+			     partitions, 1);
+}
+
+static struct efx_dl_driver efx_eeprom_driver = {
+	.name		= "sfc_eeprom",
+	.probe		= efx_eeprom_probe,
+	.remove		= efx_mtd_remove,
+	.reset_suspend	= efx_mtd_reset_suspend,

net/sfc/mtd.c new file +19 -0

+static int __init efx_mtd_init_module(void)
+{
+	int rc;
+
+	rc = efx_dl_register_driver(&efx_flash_driver);
+	if (rc != 0)
+		return rc;
+	rc = efx_dl_register_driver(&efx_eeprom_driver);
+	if (rc != 0) {
+		efx_dl_unregister_driver(&efx_flash_driver);
+		return rc;
+	}
+
+	return 0;
+}
+
+static void __exit efx_mtd_exit_module(void)
+{
+	efx_dl_unregister_driver(&efx_eeprom_driver);

Quoted 7 lines · Ben Hutchings

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.

net/sfc/mtd.c line 66

Quoted 7 lines

net/sfc/mtd.c new file +7 -0

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

net/sfc/mtd.c line 68

Quoted 2 lines

net/sfc/mtd.c new file +2 -0

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

Quoted 7 lines

net/sfc/mtd.c new file +7 -0

+		/* 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,

net/sfc/mtd.c line 274

Quoted 6 lines

net/sfc/mtd.c new file +6 -0

+					(unsigned long)(offset + i),
+					verify_buffer[i], expected);
+				rc = -EIO;
+				goto out;
+			}
+		}

Home-brewn memcmp?

net/sfc/mtd.c line 295

Quoted 7 lines

net/sfc/mtd.c new file +7 -0

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

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

net/sfc/mtd.c line 329

Quoted 6 lines

net/sfc/mtd.c new file +6 -0

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

???

Quoted 4 lines

net/sfc/mtd.c new file +4 -0

+static void efx_mtd_sync(struct mtd_info *mtd)
+{
+	struct efx_mtd *efx_mtd = (struct efx_mtd *)mtd->priv;
+	int rc;

net/sfc/mtd.c line 398

Quoted 6 lines

net/sfc/mtd.c new file +6 -0

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

net/sfc/mtd.c line 472

Quoted 6 lines

net/sfc/mtd.c new file +6 -0

+	/* 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.

net/sfc/mtd.c line 488

Quoted 5 lines

net/sfc/mtd.c new file +5 -0

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

Quoted 16 lines

Quoted 4 lines

net/sfc/mtd.c new file +3 -0

+		/* Check contents */
+		for (i = 0; i < buf_len; i++) {
+			if (buffer)

net/sfc/mtd.c line 274

Quoted 10 lines

net/sfc/mtd.c new file +10 -0

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

Quoted 9 lines

net/sfc/mtd.c line 295

Quoted 7 lines

net/sfc/mtd.c new file +7 -0

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

Quoted 2 lines

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>

Quoted 8 lines

net/sfc/mtd.c line 329

Quoted 6 lines

net/sfc/mtd.c new file +6 -0

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

Quoted 4 lines

Quoted 4 lines

net/sfc/mtd.c new file +4 -0

+static void efx_mtd_sync(struct mtd_info *mtd)
+{
+	struct efx_mtd *efx_mtd = (struct efx_mtd *)mtd->priv;
+	int rc;

Quoted 8 lines

net/sfc/mtd.c line 398

Quoted 6 lines

net/sfc/mtd.c new file +6 -0

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

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

You can replace EFX_ASSERT with BUG_ON (with negated condition) throughout the code. Duplicating existing kernel infrastructure needs a very good reason to be accepted.

Jörn Engel wrote:
<snip>

Quoted 3 lines

You can replace EFX_ASSERT with BUG_ON (with negated condition) throughout the code. Duplicating existing kernel infrastructure needs a very good reason to be accepted.

<snip>

Assertions made with EFX_ASSERT() are only checked if EFX_ENABLE_DEBUG is set, and we don't really want them to be included in a production build. Is there a suitable kernel facility for more paranoid checks that will be disabled by default?