[PATCH] spi: enable spi_board_info to be registered after spi_master

[PATCH] spi: enable spi_board_info to be registered after spi_master

[Feng Tang]

Currently spi_register_board_info() has to be called before its related
spi_master be registered, otherwise these board info will be just ignored.

This patch will remove this order limit, it adds a global spi master list
like the existing global board info listr. Whenever a board info is
registered, the spi master list will be scanned, and a new spi device will
be created if they match.

Signed-off-by: Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/spi/spi.c       |   66 +++++++++++++++++++++++++++++++++-------------
 include/linux/spi/spi.h |    3 ++
 2 files changed, 50 insertions(+), 19 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index b3a1f92..f4a5221 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -196,13 +196,16 @@ EXPORT_SYMBOL_GPL(spi_register_driver);
 
 struct boardinfo {
 	struct list_head	list;
-	unsigned		n_board_info;
-	struct spi_board_info	board_info[0];
+	struct spi_board_info	board_info;
 };
 
 static LIST_HEAD(board_list);
 static DEFINE_MUTEX(board_lock);
 
+
+static LIST_HEAD(master_list);
+static DEFINE_MUTEX(master_lock);
+
 /**
  * spi_alloc_device - Allocate a new SPI device
  * @master: Controller to which device is connected
@@ -365,6 +368,19 @@ struct spi_device *spi_new_device(struct spi_master *master,
 }
 EXPORT_SYMBOL_GPL(spi_new_device);
 
+static void scan_masterlist(struct spi_board_info *bi)
+{
+	struct spi_master *master;
+
+	mutex_lock(&master_lock);
+	list_for_each_entry(master, &master_list, list) {
+		if (master->bus_num != bi->bus_num)
+			continue;
+		(void) spi_new_device(master, bi);
+	}
+	mutex_unlock(&master_lock);
+}
+
 /**
  * spi_register_board_info - register SPI devices for a given board
  * @info: array of chip descriptors
@@ -387,41 +403,45 @@ EXPORT_SYMBOL_GPL(spi_new_device);
 int __init
 spi_register_board_info(struct spi_board_info const *info, unsigned n)
 {
-	struct boardinfo	*bi;
+	struct boardinfo	*bi, *tmp_bi;
+	int i;
 
-	bi = kmalloc(sizeof(*bi) + n * sizeof *info, GFP_KERNEL);
+	bi = kzalloc(n * sizeof *bi, GFP_KERNEL);
 	if (!bi)
 		return -ENOMEM;
-	bi->n_board_info = n;
-	memcpy(bi->board_info, info, n * sizeof *info);
 
+	tmp_bi = bi;
 	mutex_lock(&board_lock);
-	list_add_tail(&bi->list, &board_list);
+	for (i = 0; i < n; i++, bi++, info++) {
+		memcpy(&bi->board_info, info, sizeof *info);
+		list_add_tail(&bi->list, &board_list);
+	}
 	mutex_unlock(&board_lock);
+
+	bi = tmp_bi;
+	for (i = 0; i < n; i++)
+		scan_masterlist(&bi->board_info);
+
 	return 0;
 }
 
 /* FIXME someone should add support for a __setup("spi", ...) that
  * creates board info from kernel command lines
  */
-
 static void scan_boardinfo(struct spi_master *master)
 {
 	struct boardinfo	*bi;
 
 	mutex_lock(&board_lock);
 	list_for_each_entry(bi, &board_list, list) {
-		struct spi_board_info	*chip = bi->board_info;
-		unsigned		n;
-
-		for (n = bi->n_board_info; n > 0; n--, chip++) {
-			if (chip->bus_num != master->bus_num)
-				continue;
-			/* NOTE: this relies on spi_new_device to
-			 * issue diagnostics when given bogus inputs
-			 */
-			(void) spi_new_device(master, chip);
-		}
+		struct spi_board_info	*chip = &bi->board_info;
+
+		if (chip->bus_num != master->bus_num)
+			continue;
+		/* NOTE: this relies on spi_new_device to
+		 * issue diagnostics when given bogus inputs
+		 */
+		(void) spi_new_device(master, chip);
 	}
 	mutex_unlock(&board_lock);
 }
@@ -537,6 +557,10 @@ int spi_register_master(struct spi_master *master)
 	dev_dbg(dev, "registered master %s%s\n", dev_name(&master->dev),
 			dynamic ? " (dynamic)" : "");
 
+	mutex_lock(&master_lock);
+	list_add_tail(&master->list, &master_list);
+	mutex_unlock(&master_lock);
+
 	/* populate children from any spi device tables */
 	scan_boardinfo(master);
 	status = 0;
@@ -568,6 +592,10 @@ void spi_unregister_master(struct spi_master *master)
 {
 	int dummy;
 
+	mutex_lock(&master_lock);
+	list_del(&master->list);
+	mutex_unlock(&master_lock);
+
 	dummy = device_for_each_child(master->dev.parent, &master->dev,
 					__unregister);
 	device_unregister(&master->dev);
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index af56071..f4a29b6 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -204,6 +204,7 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
 /**
  * struct spi_master - interface to SPI master controller
  * @dev: device interface to this driver
+ * @list: link with the global spi_master list
  * @bus_num: board-specific (and often SOC-specific) identifier for a
  *	given SPI controller.
  * @num_chipselect: chipselects are used to distinguish individual
@@ -235,6 +236,8 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
 struct spi_master {
 	struct device	dev;
 
+	struct list_head list;
+
 	/* other than negative (== assign one dynamically), bus_num is fully
 	 * board-specific.  usually that simplifies to being SOC-specific.
 	 * example:  one SOC has three SPI controllers, numbered 0..2,
-- 
1.7.0.4


------------------------------------------------------------------------------
The Palm PDK Hot Apps Program offers developers who use the
Plug-In Development Kit to bring their C/C++ apps to Palm for a share 
of $1 Million in cash or HP Products. Visit us here for more details:
http://ad.doubleclick.net/clk;226879339;13503038;l?
http://clk.atdmt.com/CRS/go/247765532/direct/01/

Re: [PATCH] spi: enable spi_board_info to be registered after spi_master

[Grant Likely]

On Tue, Jul 27, 2010 at 3:42 AM, Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> Currently spi_register_board_info() has to be called before its related
> spi_master be registered, otherwise these board info will be just ignored.
>
> This patch will remove this order limit, it adds a global spi master list
> like the existing global board info listr. Whenever a board info is
> registered, the spi master list will be scanned, and a new spi device will
> be created if they match.
>
> Signed-off-by: Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/spi/spi.c       |   66 +++++++++++++++++++++++++++++++++-------------
>  include/linux/spi/spi.h |    3 ++
>  2 files changed, 50 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index b3a1f92..f4a5221 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -196,13 +196,16 @@ EXPORT_SYMBOL_GPL(spi_register_driver);
>
>  struct boardinfo {
>        struct list_head        list;
> -       unsigned                n_board_info;
> -       struct spi_board_info   board_info[0];
> +       struct spi_board_info   board_info;
>  };
>
>  static LIST_HEAD(board_list);
>  static DEFINE_MUTEX(board_lock);
>
> +
> +static LIST_HEAD(master_list);
> +static DEFINE_MUTEX(master_lock);
> +

Protect against global namespace collisions.  spi_master_list and
spi_master_list_lock.

You can probably use one mutex for both the board and master lists.

>  /**
>  * spi_alloc_device - Allocate a new SPI device
>  * @master: Controller to which device is connected
> @@ -365,6 +368,19 @@ struct spi_device *spi_new_device(struct spi_master *master,
>  }
>  EXPORT_SYMBOL_GPL(spi_new_device);
>
> +static void scan_masterlist(struct spi_board_info *bi)

spi_scan_master_list

> +{
> +       struct spi_master *master;
> +
> +       mutex_lock(&master_lock);
> +       list_for_each_entry(master, &master_list, list) {
> +               if (master->bus_num != bi->bus_num)
> +                       continue;
> +               (void) spi_new_device(master, bi);

Always check the return value.  At the very least print out an error
message.  I know you're copying the scan_boardinfo() lead, but that is
a bad example.  There are error paths in spi_new_device() that will
not print any output.

> +       }
> +       mutex_unlock(&master_lock);
> +}
> +
>  /**
>  * spi_register_board_info - register SPI devices for a given board
>  * @info: array of chip descriptors
> @@ -387,41 +403,45 @@ EXPORT_SYMBOL_GPL(spi_new_device);
>  int __init
>  spi_register_board_info(struct spi_board_info const *info, unsigned n)
>  {
> -       struct boardinfo        *bi;
> +       struct boardinfo        *bi, *tmp_bi;
> +       int i;
>
> -       bi = kmalloc(sizeof(*bi) + n * sizeof *info, GFP_KERNEL);
> +       bi = kzalloc(n * sizeof *bi, GFP_KERNEL);

sizeof(*bi)

>        if (!bi)
>                return -ENOMEM;
> -       bi->n_board_info = n;
> -       memcpy(bi->board_info, info, n * sizeof *info);
>
> +       tmp_bi = bi;
>        mutex_lock(&board_lock);
> -       list_add_tail(&bi->list, &board_list);
> +       for (i = 0; i < n; i++, bi++, info++) {

I'd do this the other way around (bi vs. tmp_bi).  I think this is
somewhat clearer:

for (i = 0, tmp_bi = bi; i < n; i++, tmp_bi++, info++)

> +               memcpy(&bi->board_info, info, sizeof *info);
> +               list_add_tail(&bi->list, &board_list);
> +       }
>        mutex_unlock(&board_lock);
> +
> +       bi = tmp_bi;
> +       for (i = 0; i < n; i++)
> +               scan_masterlist(&bi->board_info);

How does this work?  The same board_info struct is passed to
scan_masterlist() multiple times.

> +
>        return 0;
>  }
>
>  /* FIXME someone should add support for a __setup("spi", ...) that
>  * creates board info from kernel command lines
>  */
> -
>  static void scan_boardinfo(struct spi_master *master)
>  {
>        struct boardinfo        *bi;
>
>        mutex_lock(&board_lock);
>        list_for_each_entry(bi, &board_list, list) {
> -               struct spi_board_info   *chip = bi->board_info;
> -               unsigned                n;
> -
> -               for (n = bi->n_board_info; n > 0; n--, chip++) {
> -                       if (chip->bus_num != master->bus_num)
> -                               continue;
> -                       /* NOTE: this relies on spi_new_device to
> -                        * issue diagnostics when given bogus inputs
> -                        */
> -                       (void) spi_new_device(master, chip);
> -               }
> +               struct spi_board_info   *chip = &bi->board_info;
> +
> +               if (chip->bus_num != master->bus_num)
> +                       continue;
> +               /* NOTE: this relies on spi_new_device to
> +                * issue diagnostics when given bogus inputs
> +                */
> +               (void) spi_new_device(master, chip);
>        }
>        mutex_unlock(&board_lock);
>  }
> @@ -537,6 +557,10 @@ int spi_register_master(struct spi_master *master)
>        dev_dbg(dev, "registered master %s%s\n", dev_name(&master->dev),
>                        dynamic ? " (dynamic)" : "");
>
> +       mutex_lock(&master_lock);
> +       list_add_tail(&master->list, &master_list);
> +       mutex_unlock(&master_lock);
> +

There seems to be a race condition here.  If a boardinfo is registered
after this list add, but before scan_boardinfo(master), then a
boardinfo can get parsed twice for a single master.  I think the
proper thing to do is to hold the mutex over the whole operation; from
registering the list or master, until after the scan is complete.


Otherwise, in general I like the patch.  spin it again to address the
comments, and I think I can get it into linux-next for 2.6.36.

Thanks!
g.

>        /* populate children from any spi device tables */
>        scan_boardinfo(master);
>        status = 0;
> @@ -568,6 +592,10 @@ void spi_unregister_master(struct spi_master *master)
>  {
>        int dummy;
>
> +       mutex_lock(&master_lock);
> +       list_del(&master->list);
> +       mutex_unlock(&master_lock);
> +
>        dummy = device_for_each_child(master->dev.parent, &master->dev,
>                                        __unregister);
>        device_unregister(&master->dev);
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index af56071..f4a29b6 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -204,6 +204,7 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
>  /**
>  * struct spi_master - interface to SPI master controller
>  * @dev: device interface to this driver
> + * @list: link with the global spi_master list
>  * @bus_num: board-specific (and often SOC-specific) identifier for a
>  *     given SPI controller.
>  * @num_chipselect: chipselects are used to distinguish individual
> @@ -235,6 +236,8 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
>  struct spi_master {
>        struct device   dev;
>
> +       struct list_head list;
> +
>        /* other than negative (== assign one dynamically), bus_num is fully
>         * board-specific.  usually that simplifies to being SOC-specific.
>         * example:  one SOC has three SPI controllers, numbered 0..2,
> --
> 1.7.0.4
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

------------------------------------------------------------------------------
The Palm PDK Hot Apps Program offers developers who use the
Plug-In Development Kit to bring their C/C++ apps to Palm for a share 
of $1 Million in cash or HP Products. Visit us here for more details:
http://ad.doubleclick.net/clk;226879339;13503038;l?
http://clk.atdmt.com/CRS/go/247765532/direct/01/

Re: [PATCH] spi: enable spi_board_info to be registered after spi_master

[Feng Tang]

Hi Grant,

Thanks for your thorough reviews!

Will address most of the comments.



On Wed, 28 Jul 2010 01:27:16 +0800
Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:

> 
> for (i = 0, tmp_bi = bi; i < n; i++, tmp_bi++, info++)
> 
> > +               memcpy(&bi->board_info, info, sizeof *info);
> > +               list_add_tail(&bi->list, &board_list);
> > +       }
> >        mutex_unlock(&board_lock);
> > +
> > +       bi = tmp_bi;
> > +       for (i = 0; i < n; i++)
> > +               scan_masterlist(&bi->board_info);
> 
> How does this work?  The same board_info struct is passed to
> scan_masterlist() multiple times.

My bad, I tested 2 cases by calling register_board_info before and
after spi_register_master(), but only with one spi device. Will fix it


> > @@ -537,6 +557,10 @@ int spi_register_master(struct spi_master
> > *master) dev_dbg(dev, "registered master %s%s\n",
> > dev_name(&master->dev), dynamic ? " (dynamic)" : "");
> >
> > +       mutex_lock(&master_lock);
> > +       list_add_tail(&master->list, &master_list);
> > +       mutex_unlock(&master_lock);
> > +
> 
> There seems to be a race condition here.  If a boardinfo is registered
> after this list add, but before scan_boardinfo(master), then a
> boardinfo can get parsed twice for a single master.  I think the
> proper thing to do is to hold the mutex over the whole operation; from
> registering the list or master, until after the scan is complete.
> 

Good point. Maybe moving the list_add(master) after scan_boardinfo() 
can fix this race condition.

Thanks,
Feng

------------------------------------------------------------------------------
The Palm PDK Hot Apps Program offers developers who use the
Plug-In Development Kit to bring their C/C++ apps to Palm for a share 
of $1 Million in cash or HP Products. Visit us here for more details:
http://ad.doubleclick.net/clk;226879339;13503038;l?
http://clk.atdmt.com/CRS/go/247765532/direct/01/

Re: [PATCH] spi: enable spi_board_info to be registered after spi_master

[Feng Tang]

On Wed, 28 Jul 2010 09:23:06 +0800
Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:

> > > @@ -537,6 +557,10 @@ int spi_register_master(struct spi_master
> > > *master) dev_dbg(dev, "registered master %s%s\n",
> > > dev_name(&master->dev), dynamic ? " (dynamic)" : "");
> > >
> > > +       mutex_lock(&master_lock);
> > > +       list_add_tail(&master->list, &master_list);
> > > +       mutex_unlock(&master_lock);
> > > +
> > 
> > There seems to be a race condition here.  If a boardinfo is
> > registered after this list add, but before scan_boardinfo(master),
> > then a boardinfo can get parsed twice for a single master.  I think
> > the proper thing to do is to hold the mutex over the whole
> > operation; from registering the list or master, until after the
> > scan is complete.
> > 
> 
> Good point. Maybe moving the list_add(master) after scan_boardinfo() 
> can fix this race condition.

My solution still has problem, it was:

	scan_boardinfo(master);

        mutex_lock(&spi_master_lock);
        list_add_tail(&master->list, &spi_master_list);
        mutex_unlock(&spi_master_lock);

If another boardinfo registering just after scan_boardinfo() but before
list_add(), it will still get ignored.

I will just follow Grant's suggestion.

Thanks,
Feng

------------------------------------------------------------------------------
The Palm PDK Hot Apps Program offers developers who use the
Plug-In Development Kit to bring their C/C++ apps to Palm for a share 
of $1 Million in cash or HP Products. Visit us here for more details:
http://ad.doubleclick.net/clk;226879339;13503038;l?
http://clk.atdmt.com/CRS/go/247765532/direct/01/