[PATCH] SPI core tweaks, bugfix

This includes various updates to the SPI core:

  - Fixes a driver model refcount bug in spi_unregister_master() paths.

  - The spi_master structures now have wrappers which help keep drivers
    from needing class-level get/put for device data or for refcounts.

  - Check for a few setup errors that would cause oopsing later.

  - Docs say more about memory management.  Highlights the use of DMA-safe
    i/o buffers, and zero-initializing spi_message and such metadata.

  - Provide a simple alloc/free for spi_message and its spi_transfer;
    this is only one of the possible memory management policies.

Nothing to break code that already works.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

3 files changed, 113 insertions, 23 deletions

diff --git a/Documentation/spi/spi-summary b/Documentation/spi/spi-summary
index c6152d1ff2b0..761debf748e9 100644
--- a/Documentation/spi/spi-summary
+++ b/Documentation/spi/spi-summary
@@ -363,6 +363,22 @@ upper boundaries might include sysfs (especially for sensor readings),
 the input layer, ALSA, networking, MTD, the character device framework,
 or other Linux subsystems.
 
+Note that there are two types of memory your driver must manage as part
+of interacting with SPI devices.
+
+  - I/O buffers use the usual Linux rules, and must be DMA-safe.
+    You'd normally allocate them from the heap or free page pool.
+    Don't use the stack, or anything that's declared "static".
+
+  - The spi_message and spi_transfer metadata used to glue those
+    I/O buffers into a group of protocol transactions.  These can
+    be allocated anywhere it's convenient, including as part of
+    other allocate-once driver data structures.  Zero-init these.
+
+If you like, spi_message_alloc() and spi_message_free() convenience
+routines are available to allocate and zero-initialize an spi_message
+with several transfers.
+
 
 How do I write an "SPI Master Controller Driver"?
 -------------------------------------------------
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 2ecb86cb3689..3ecedccdb96c 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -38,7 +38,7 @@ static void spidev_release(struct device *dev)
         if (spi->master->cleanup)
                 spi->master->cleanup(spi);
 
-        class_device_put(&spi->master->cdev);
+        spi_master_put(spi->master);
         kfree(dev);
 }
 
@@ -90,7 +90,7 @@ static int spi_suspend(struct device *dev, pm_message_t message)
         int                     value;
         struct spi_driver       *drv = to_spi_driver(dev->driver);
 
-        if (!drv || !drv->suspend)
+        if (!drv->suspend)
                 return 0;
 
         /* suspend will stop irqs and dma; no more i/o */
@@ -105,7 +105,7 @@ static int spi_resume(struct device *dev)
         int                     value;
         struct spi_driver       *drv = to_spi_driver(dev->driver);
 
-        if (!drv || !drv->resume)
+        if (!drv->resume)
                 return 0;
 
         /* resume may restart the i/o queue */
@@ -198,7 +198,7 @@ spi_new_device(struct spi_master *master, struct spi_board_info *chip)
 
         /* NOTE:  caller did any chip->bus_num checks necessary */
 
-        if (!class_device_get(&master->cdev))
+        if (!spi_master_get(master))
                 return NULL;
 
         proxy = kzalloc(sizeof *proxy, GFP_KERNEL);
@@ -244,7 +244,7 @@ spi_new_device(struct spi_master *master, struct spi_board_info *chip)
         return proxy;
 
 fail:
-        class_device_put(&master->cdev);
+        spi_master_put(master);
         kfree(proxy);
         return NULL;
 }
@@ -324,8 +324,6 @@ static void spi_master_release(struct class_device *cdev)
         struct spi_master *master;
 
         master = container_of(cdev, struct spi_master, cdev);
-        put_device(master->cdev.dev);
-        master->cdev.dev = NULL;
         kfree(master);
 }
 
@@ -339,8 +337,9 @@ static struct class spi_master_class = {
 /**
  * spi_alloc_master - allocate SPI master controller
  * @dev: the controller, possibly using the platform_bus
- * @size: how much driver-private data to preallocate; a pointer to this
- *      memory in the class_data field of the returned class_device
+ * @size: how much driver-private data to preallocate; the pointer to this
+ *      memory is in the class_data field of the returned class_device,
+ *      accessible with spi_master_get_devdata().
  *
  * This call is used only by SPI master controller drivers, which are the
  * only ones directly touching chip registers.  It's how they allocate
@@ -350,14 +349,17 @@ static struct class spi_master_class = {
  * master structure on success, else NULL.
  *
  * The caller is responsible for assigning the bus number and initializing
- * the master's methods before calling spi_add_master(), or else (on error)
- * calling class_device_put() to prevent a memory leak.
+ * the master's methods before calling spi_add_master(); and (after errors
+ * adding the device) calling spi_master_put() to prevent a memory leak.
  */
 struct spi_master * __init_or_module
 spi_alloc_master(struct device *dev, unsigned size)
 {
         struct spi_master       *master;
 
+        if (!dev)
+                return NULL;
+
         master = kzalloc(size + sizeof *master, SLAB_KERNEL);
         if (!master)
                 return NULL;
@@ -365,7 +367,7 @@ spi_alloc_master(struct device *dev, unsigned size)
         class_device_initialize(&master->cdev);
         master->cdev.class = &spi_master_class;
         master->cdev.dev = get_device(dev);
-        class_set_devdata(&master->cdev, &master[1]);
+        spi_master_set_devdata(master, &master[1]);
 
         return master;
 }
@@ -387,6 +389,8 @@ EXPORT_SYMBOL_GPL(spi_alloc_master);
  *
  * This must be called from context that can sleep.  It returns zero on
  * success, else a negative error code (dropping the master's refcount).
+ * After a successful return, the caller is responsible for calling
+ * spi_unregister_master().
  */
 int __init_or_module
 spi_register_master(struct spi_master *master)
@@ -396,6 +400,9 @@ spi_register_master(struct spi_master *master)
         int                     status = -ENODEV;
         int                     dynamic = 0;
 
+        if (!dev)
+                return -ENODEV;
+
         /* convention:  dynamically assigned bus IDs count down from the max */
         if (master->bus_num == 0) {
                 master->bus_num = atomic_dec_return(&dyn_bus_id);
@@ -425,7 +432,7 @@ EXPORT_SYMBOL_GPL(spi_register_master);
 static int __unregister(struct device *dev, void *unused)
 {
         /* note: before about 2.6.14-rc1 this would corrupt memory: */
-        device_unregister(dev);
+        spi_unregister_device(to_spi_device(dev));
         return 0;
 }
 
@@ -440,8 +447,9 @@ static int __unregister(struct device *dev, void *unused)
  */
 void spi_unregister_master(struct spi_master *master)
 {
-        class_device_unregister(&master->cdev);
         (void) device_for_each_child(master->cdev.dev, NULL, __unregister);
+        class_device_unregister(&master->cdev);
+        master->cdev.dev = NULL;
 }
 EXPORT_SYMBOL_GPL(spi_unregister_master);
 
@@ -487,6 +495,9 @@ EXPORT_SYMBOL_GPL(spi_busnum_to_master);
  * by leaving it selected in anticipation that the next message will go
  * to the same chip.  (That may increase power usage.)
  *
+ * Also, the caller is guaranteeing that the memory associated with the
+ * message will not be freed before this call returns.
+ *
  * The return value is a negative error code if the message could not be
  * submitted, else zero.  When the value is zero, then message->status is
  * also defined:  it's the completion code for the transfer, either zero
@@ -524,9 +535,9 @@ static u8	*buf;
  * is zero for success, else a negative errno status code.
  * This call may only be used from a context that may sleep.
  *
- * Parameters to this routine are always copied using a small buffer,
- * large transfers should use use spi_{async,sync}() calls with
- * dma-safe buffers.
+ * Parameters to this routine are always copied using a small buffer;
+ * performance-sensitive or bulk transfer code should instead use
+ * spi_{async,sync}() calls with dma-safe buffers.
  */
 int spi_write_then_read(struct spi_device *spi,
                 const u8 *txbuf, unsigned n_tx,
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index c851b3d13208..6a41e2650b2e 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -60,8 +60,8 @@ struct spi_device {
         u8                      mode;
 #define SPI_CPHA        0x01                    /* clock phase */
 #define SPI_CPOL        0x02                    /* clock polarity */
-#define SPI_MODE_0      (0|0)
-#define SPI_MODE_1      (0|SPI_CPHA)            /* (original MicroWire) */
+#define SPI_MODE_0      (0|0)                   /* (original MicroWire) */
+#define SPI_MODE_1      (0|SPI_CPHA)
 #define SPI_MODE_2      (SPI_CPOL|0)
 #define SPI_MODE_3      (SPI_CPOL|SPI_CPHA)
 #define SPI_CS_HIGH     0x04                    /* chipselect active high? */
@@ -209,6 +209,30 @@ struct spi_master {
         void                    (*cleanup)(const struct spi_device *spi);
 };
 
+static inline void *spi_master_get_devdata(struct spi_master *master)
+{
+        return class_get_devdata(&master->cdev);
+}
+
+static inline void spi_master_set_devdata(struct spi_master *master, void *data)
+{
+        class_set_devdata(&master->cdev, data);
+}
+
+static inline struct spi_master *spi_master_get(struct spi_master *master)
+{
+        if (!master || !class_device_get(&master->cdev))
+                return NULL;
+        return master;
+}
+
+static inline void spi_master_put(struct spi_master *master)
+{
+        if (master)
+                class_device_put(&master->cdev);
+}
+
+
 /* the spi driver core manages memory for the spi_master classdev */
 extern struct spi_master *
 spi_alloc_master(struct device *host, unsigned size);
@@ -271,11 +295,17 @@ extern struct spi_master *spi_busnum_to_master(u16 busnum);
  * stay selected until the next transfer.  This is purely a performance
  * hint; the controller driver may need to select a different device
  * for the next message.
+ *
+ * The code that submits an spi_message (and its spi_transfers)
+ * to the lower layers is responsible for managing its memory.
+ * Zero-initialize every field you don't set up explicitly, to
+ * insulate against future API updates.
  */
 struct spi_transfer {
         /* it's ok if tx_buf == rx_buf (right?)
          * for MicroWire, one buffer must be null
-         * buffers must work with dma_*map_single() calls
+         * buffers must work with dma_*map_single() calls, unless
+         *   spi_message.is_dma_mapped reports a pre-existing mapping
          */
         const void      *tx_buf;
         void            *rx_buf;
@@ -302,6 +332,11 @@ struct spi_transfer {
  * @status: zero for success, else negative errno
  * @queue: for use by whichever driver currently owns the message
  * @state: for use by whichever driver currently owns the message
+ *
+ * The code that submits an spi_message (and its spi_transfers)
+ * to the lower layers is responsible for managing its memory.
+ * Zero-initialize every field you don't set up explicitly, to
+ * insulate against future API updates.
  */
 struct spi_message {
         struct spi_transfer     *transfers;
@@ -336,6 +371,29 @@ struct spi_message {
         void                    *state;
 };
 
+/* It's fine to embed message and transaction structures in other data
+ * structures so long as you don't free them while they're in use.
+ */
+
+static inline struct spi_message *spi_message_alloc(unsigned ntrans, gfp_t flags)
+{
+        struct spi_message *m;
+
+        m = kzalloc(sizeof(struct spi_message)
+                        + ntrans * sizeof(struct spi_transfer),
+                        flags);
+        if (m) {
+                m->transfers = (void *)(m + 1);
+                m->n_transfer = ntrans;
+        }
+        return m;
+}
+
+static inline void spi_message_free(struct spi_message *m)
+{
+        kfree(m);
+}
+
 /**
  * spi_setup -- setup SPI mode and clock rate
  * @spi: the device whose settings are being modified
@@ -363,7 +421,10 @@ spi_setup(struct spi_device *spi)
  * The completion callback is invoked in a context which can't sleep.
  * Before that invocation, the value of message->status is undefined.
  * When the callback is issued, message->status holds either zero (to
- * indicate complete success) or a negative error code.
+ * indicate complete success) or a negative error code.  After that
+ * callback returns, the driver which issued the transfer request may
+ * deallocate the associated memory; it's no longer in use by any SPI
+ * core or controller driver code.
  *
  * Note that although all messages to a spi_device are handled in
  * FIFO order, messages may go to different devices in other orders.
@@ -445,6 +506,7 @@ spi_read(struct spi_device *spi, u8 *buf, size_t len)
         return spi_sync(spi, &m);
 }
 
+/* this copies txbuf and rxbuf data; for small transfers only! */
 extern int spi_write_then_read(struct spi_device *spi,
                 const u8 *txbuf, unsigned n_tx,
                 u8 *rxbuf, unsigned n_rx);
@@ -555,8 +617,9 @@ spi_register_board_info(struct spi_board_info const *info, unsigned n)
 
 
 /* If you're hotplugging an adapter with devices (parport, usb, etc)
- * use spi_new_device() to describe each device.  You would then call
- * spi_unregister_device() to start making that device vanish.
+ * use spi_new_device() to describe each device.  You can also call
+ * spi_unregister_device() to start making that device vanish, but
+ * normally that would be handled by spi_unregister_master().
  */
 extern struct spi_device *
 spi_new_device(struct spi_master *, struct spi_board_info *);