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

[Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

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/